From 0a54a297ae88632a1313861232475ff905078853 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 12 2019 10:30:28 +0000 Subject: Provide more information about invalid tokens We now include an "errors" field in the JSON data returned that specifies the reason why the token is invalid (entirely invalid, expired, missing ACLs). Unit-tests are also adjusted for this. Fixes https://pagure.io/pagure/issue/4307 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/utils.py b/pagure/utils.py index d575df8..5637bcd 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -86,33 +86,58 @@ def check_api_acls(acls, optional=False): token_str = authorization.split("token", 1)[1].strip() token_auth = False + error_msg = None if token_str: token = pagure.lib.query.get_api_token(flask.g.session, token_str) - if token and not token.expired: - flask.g.authenticated = True - if acls and set(token.acls_list).intersection(set(acls)): - token_auth = True - flask.g.fas_user = token.user - # To get a token, in the `fas` auth user must have signed - # the CLA, so just set it to True - flask.g.fas_user.cla_done = True - flask.g.token = token - flask.g.authenticated = True - elif not acls and optional: - token_auth = True - flask.g.fas_user = token.user - # To get a token, in the `fas` auth user must have signed - # the CLA, so just set it to True - flask.g.fas_user.cla_done = True - flask.g.token = token + if token: + if token.expired: + error_msg = "Expired token" + else: flask.g.authenticated = True + + # Some ACLs are required + if acls: + token_acls_set = set(token.acls_list) + needed_acls_set = set(acls or []) + overlap = token_acls_set.intersection(needed_acls_set) + # Our token has some of the required ACLs: auth successful + if overlap: + token_auth = True + flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have + # signed the CLA, so just set it to True + flask.g.fas_user.cla_done = True + flask.g.token = token + flask.g.authenticated = True + # Our token has none of the required ACLs -> auth fail + else: + error_msg = "Missing ACLs: %s" % ", ".join( + sorted(set(acls) - set(token.acls_list)) + ) + # No ACL required + else: + if optional: + token_auth = True + flask.g.fas_user = token.user + # To get a token, in the `fas` auth user must have + # signed the CLA, so just set it to True + flask.g.fas_user.cla_done = True + flask.g.token = token + flask.g.authenticated = True + else: + error_msg = "Invalid token" + elif optional: return + else: + error_msg = "Invalid token" + if not token_auth: output = { "error_code": pagure.api.APIERROR.EINVALIDTOK.name, "error": pagure.api.APIERROR.EINVALIDTOK.value, + "errors": error_msg, } jsonout = flask.jsonify(output) jsonout.status_code = 401 diff --git a/tests/test_pagure_flask_api.py b/tests/test_pagure_flask_api.py index 301e3f0..8daea1d 100644 --- a/tests/test_pagure_flask_api.py +++ b/tests/test_pagure_flask_api.py @@ -215,7 +215,8 @@ class PagureFlaskApitests(tests.SimplePagureTest): u'error': u'Invalid or expired token. Please visit ' 'http://localhost.localdomain/settings#api-keys to get or ' 'renew your API token.', - u'error_code': u'EINVALIDTOK' + u'error_code': u'EINVALIDTOK', + u'errors': 'Invalid token', } ) diff --git a/tests/test_pagure_flask_api_issue.py b/tests/test_pagure_flask_api_issue.py index b3f0d8a..92c6440 100644 --- a/tests/test_pagure_flask_api_issue.py +++ b/tests/test_pagure_flask_api_issue.py @@ -2490,11 +2490,12 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): output = self.app.get('/api/0/test/issue/2', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual(data['errors'], 'Invalid token') # Create a new token for another user item = pagure.lib.model.Token( @@ -3047,11 +3048,12 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): '/api/0/foo/issue/1/milestone', data={}, headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual(data['errors'], 'Invalid token') @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) @patch( @@ -3561,11 +3563,13 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): '/api/0/foo/issue/1/assign', data=data, headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], 'Missing ACLs: issue_assign, issue_update') # No comment added repo = pagure.lib.query.get_authorized_project(self.session, 'foo') @@ -3938,11 +3942,12 @@ class PagureFlaskApiIssuetests(tests.SimplePagureTest): '/api/0/test/issue/1/custom/bugzilla', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual(data['errors'], 'Invalid token') headers = {'Authorization': 'token aaabbbcccddd'} diff --git a/tests/test_pagure_flask_api_project.py b/tests/test_pagure_flask_api_project.py index 80cea7b..704a2e2 100644 --- a/tests/test_pagure_flask_api_project.py +++ b/tests/test_pagure_flask_api_project.py @@ -2112,11 +2112,14 @@ class PagureFlaskApiProjecttests(tests.Modeltests): output = self.app.post('/api/0/new', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), [ + 'error', 'error_code', "errors"]) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], "Missing ACLs: create_project") headers = {'Authorization': 'token aaabbbcccddd'} @@ -2353,11 +2356,14 @@ class PagureFlaskApiProjecttests(tests.Modeltests): output = self.app.post('/api/0/new', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), [ + 'error', 'error_code', "errors"]) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], "Missing ACLs: create_project") headers = {'Authorization': 'token aaabbbcccddd'} @@ -2529,11 +2535,14 @@ class PagureFlaskApiProjecttests(tests.Modeltests): output = self.app.post('/api/0/fork', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), [ + 'error', 'error_code', "errors"]) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], "Missing ACLs: fork_project") headers = {'Authorization': 'token aaabbbcccddd'} @@ -2652,11 +2661,14 @@ class PagureFlaskApiProjecttests(tests.Modeltests): output = self.app.post('/api/0/fork', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), [ + 'error', 'error_code', "errors"]) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], "Missing ACLs: fork_project") headers = {'Authorization': 'token aaabbbcccddd'} @@ -3123,11 +3135,14 @@ class PagureFlaskApiProjectFlagtests(tests.Modeltests): headers=headers, data=data) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), [ + 'error', 'error_code', "errors"]) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual( + data['errors'], "Invalid token") def test_flag_commit_invalid_status(self): """ Test flagging a commit with an invalid status. """ @@ -3986,7 +4001,8 @@ class PagureFlaskApiProjectOptionsTests(tests.Modeltests): u'error': u'Invalid or expired token. Please visit ' 'http://localhost.localdomain/settings#api-keys to get ' 'or renew your API token.', - u'error_code': u'EINVALIDTOK' + u'error_code': u'EINVALIDTOK', + u'errors': u'Invalid token', } ) @@ -4049,7 +4065,8 @@ class PagureFlaskApiProjectOptionsTests(tests.Modeltests): u'error': u'Invalid or expired token. Please visit ' 'http://localhost.localdomain/settings#api-keys to get ' 'or renew your API token.', - u'error_code': u'EINVALIDTOK' + u'error_code': u'EINVALIDTOK', + u'errors': u'Invalid token', } ) diff --git a/tests/test_pagure_flask_api_ui_private_repo.py b/tests/test_pagure_flask_api_ui_private_repo.py index 166f678..ead19c4 100644 --- a/tests/test_pagure_flask_api_ui_private_repo.py +++ b/tests/test_pagure_flask_api_ui_private_repo.py @@ -2990,11 +2990,12 @@ class PagurePrivateRepotest(tests.Modeltests): output = self.app.get('/api/0/test4/issue/1', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual(data['errors'], 'Invalid token') headers = {'Authorization': 'token aaabbbcccddd'} @@ -3216,11 +3217,12 @@ class PagurePrivateRepotest(tests.Modeltests): output = self.app.post('/api/0/test4/issue/1/comment', headers=headers) self.assertEqual(output.status_code, 401) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual(sorted(data.keys()), ['error', 'error_code', 'errors']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.value, data['error']) self.assertEqual( pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + self.assertEqual(data['errors'], 'Invalid token') headers = {'Authorization': 'token aaabbbcccddd'} # No input diff --git a/tests/test_pagure_flask_rebase.py b/tests/test_pagure_flask_rebase.py index e43afea..f651d7e 100644 --- a/tests/test_pagure_flask_rebase.py +++ b/tests/test_pagure_flask_rebase.py @@ -283,7 +283,8 @@ class PagureRebasetests(tests.Modeltests): u'error': u'Invalid or expired token. Please visit ' 'http://localhost.localdomain/settings#api-keys to get ' 'or renew your API token.', - u'error_code': u'EINVALIDTOK' + u'error_code': u'EINVALIDTOK', + u'errors': 'Invalid token', } )