This is an automated email from the ASF dual-hosted git repository. brondsem pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/allura.git
The following commit(s) were added to refs/heads/master by this push: new 634892b56 fixup! [#7272] Implement additional security features for OAuth2 support 634892b56 is described below commit 634892b56142f3cd3385e786e5c8c16494f12e74 Author: Carlos Cruz <carlos.c...@slashdotmedia.com> AuthorDate: Fri May 10 19:38:02 2024 +0000 fixup! [#7272] Implement additional security features for OAuth2 support --- Allura/allura/controllers/auth.py | 14 ++--- Allura/allura/controllers/rest.py | 91 ++++++++++++++--------------- Allura/allura/model/oauth.py | 26 ++++----- Allura/allura/tests/functional/test_auth.py | 31 ++++++---- 4 files changed, 84 insertions(+), 78 deletions(-) diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py index d8e436657..1b9ea3d55 100644 --- a/Allura/allura/controllers/auth.py +++ b/Allura/allura/controllers/auth.py @@ -1416,8 +1416,8 @@ class OAuth2Controller(BaseController): # Revokes the authorization code and access tokens for a given client and user def _revoke_user_tokens(self, client_id, user_id): - M.OAuth2AuthorizationCode.query.remove({'client_id': client_id, 'owner_id': user_id}) - M.OAuth2AccessToken.query.remove({'client_id': client_id, 'owner_id': user_id}) + M.OAuth2AuthorizationCode.query.remove({'client_id': client_id, 'user_id': user_id}) + M.OAuth2AccessToken.query.remove({'client_id': client_id, 'user_id': user_id}) # Revokes the authorization code and access tokens for a given client and all its users def _revoke_all(self, client_id): @@ -1429,12 +1429,12 @@ class OAuth2Controller(BaseController): def index(self, **kw): c.form = F.oauth2_application_form provider = plugin.AuthenticationProvider.get(request) - clients = M.OAuth2ClientApp.for_owner(c.user) + clients = M.OAuth2ClientApp.for_user(c.user) model = [] for client in clients: - authorization = M.OAuth2AuthorizationCode.query.get(client_id=client.client_id, owner_id=c.user._id) - token = M.OAuth2AccessToken.query.get(client_id=client.client_id, owner_id=c.user._id) + authorization = M.OAuth2AuthorizationCode.query.get(client_id=client.client_id, user_id=c.user._id) + token = M.OAuth2AccessToken.query.get(client_id=client.client_id, user_id=c.user._id) model.append(dict(client=client, authorization=authorization, token=token)) return dict( @@ -1449,7 +1449,7 @@ class OAuth2Controller(BaseController): M.OAuth2ClientApp(name=application_name, description=application_description, redirect_uris=[redirect_url], - owner_id=c.user._id) + user_id=c.user._id) flash('Oauth2 Client registered') redirect('.') @@ -1457,7 +1457,7 @@ class OAuth2Controller(BaseController): @require_post() def do_client_action(self, _id=None, deregister=None, revoke=None): client = M.OAuth2ClientApp.query.get(client_id=_id) - if client is None or client.owner_id != c.user._id: + if client is None or client.user_id != c.user._id: flash('Invalid client ID', 'error') redirect('.') diff --git a/Allura/allura/controllers/rest.py b/Allura/allura/controllers/rest.py index d07220616..b1bafea33 100644 --- a/Allura/allura/controllers/rest.py +++ b/Allura/allura/controllers/rest.py @@ -17,7 +17,6 @@ """REST Controller""" from __future__ import annotations -import ast import json import logging from datetime import datetime, timedelta @@ -294,9 +293,7 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): M.OAuth2AuthorizationCode.query.remove({'client_id': client_id, 'authorization_code': code}) def authenticate_client(self, request: oauthlib.common.Request, *args, **kwargs) -> bool: - client_id = request.body.get('client_id') - client_secret = request.body.get('client_secret') - request.client = M.OAuth2ClientApp.query.get(client_id=client_id, client_secret=client_secret) + request.client = M.OAuth2ClientApp.query.get(client_id=request.client_id, client_secret=request.client_secret) return request.client is not None def validate_code(self, client_id: str, code: str, client: oauthlib.oauth2.Client, request: oauthlib.common.Request, *args, **kwargs) -> bool: @@ -314,11 +311,11 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): return authorization.redirect_uri == redirect_uri def save_authorization_code(self, client_id: str, code, request: oauthlib.common.Request, *args, **kwargs) -> None: - authorization = M.OAuth2AuthorizationCode.query.get(client_id=client_id, owner_id=c.user._id, authorization_code=code['code']) + authorization = M.OAuth2AuthorizationCode.query.get(client_id=client_id, user_id=c.user._id, authorization_code=code['code']) # Remove the existing authorization code if it exists and create a new record if authorization: - M.OAuth2AuthorizationCode.query.remove({'client_id': client_id, 'owner_id': c.user._id, 'authorization_code': code['code']}) + M.OAuth2AuthorizationCode.query.remove({'client_id': client_id, 'user_id': c.user._id, 'authorization_code': code['code']}) log.info('Saving authorization code for client: %s', client_id) auth_code = M.OAuth2AuthorizationCode( @@ -326,7 +323,7 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): authorization_code=code['code'], expires_at=datetime.utcnow() + timedelta(minutes=10), redirect_uri=request.redirect_uri, - owner_id=c.user._id, + user_id=c.user._id, code_challenge=request.code_challenge, code_challenge_method=request.code_challenge_method ) @@ -335,18 +332,18 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): def save_bearer_token(self, token, request: oauthlib.common.Request, *args, **kwargs) -> object: authorization_code = M.OAuth2AuthorizationCode.query.get(client_id=request.client_id, authorization_code=request.code) - current_token = M.OAuth2AccessToken.query.get(client_id=request.client_id, owner_id=authorization_code.owner_id) + current_token = M.OAuth2AccessToken.query.get(client_id=request.client_id, user_id=authorization_code.user_id) if current_token: - M.OAuth2AccessToken.remove({'client_id': request.client_id, 'owner_id': c.user._id}) + M.OAuth2AccessToken.query.remove({'client_id': request.client_id, 'user_id': c.user._id}) bearer_token = M.OAuth2AccessToken( client_id = request.client_id, scopes = token.get('scope', []), access_token = token.get('access_token'), refresh_token = token.get('refresh_token'), - expires_at = datetime.utcfromtimestamp(token.get('expires_in')), - owner_id = authorization_code.owner_id + expires_at = datetime.utcnow() + timedelta(seconds=token.get('expires_in')), + user_id = authorization_code.user_id ) session(bearer_token).flush() @@ -495,11 +492,6 @@ class Oauth2Negotiator: return oauthlib.oauth2.WebApplicationServer(Oauth2Validator()) def _authenticate(self): - bearer_token_prefix = 'Bearer ' # noqa: S105 - auth_header = request.headers.get('Authorization') - if auth_header and auth_header.startswith(bearer_token_prefix): - access_token = auth_header[len(bearer_token_prefix):] - valid, req = self.server.verify_request( request.url, http_method=request.method, @@ -509,9 +501,16 @@ class Oauth2Negotiator: if not valid: raise exc.HTTPUnauthorized - access_token = M.OAuth2AccessToken.query.get(access_token=req.access_token) - access_token.last_access = datetime.utcnow() - return access_token + bearer_token_prefix = 'Bearer ' # noqa: S105 + auth_header = req.headers.get('Authorization') + if auth_header and auth_header.startswith(bearer_token_prefix): + access_token = auth_header[len(bearer_token_prefix):] + else: + raise exc.HTTPUnauthorized + + token = M.OAuth2AccessToken.query.get(access_token=access_token) + token.last_access = datetime.utcnow() + return token @expose('jinja:allura:templates/oauth2_authorize.html') @@ -525,26 +524,23 @@ class Oauth2Negotiator: decoded_body = str(request.body, 'utf-8') json_body = json.loads(decoded_body) - try: - scopes, credentials = self.server.validate_authorization_request(uri=request.url, http_method=request.method, headers=request.headers, body=json_body) - client_id = request.params.get('client_id') - client = M.OAuth2ClientApp.query.get(client_id=client_id) + scopes, credentials = self.server.validate_authorization_request(uri=request.url, http_method=request.method, headers=request.headers, body=json_body) + + client_id = request.params.get('client_id') + client = M.OAuth2ClientApp.query.get(client_id=client_id) - # The credentials object has a request object that it's too big to be serialized, - # so we remove it because we don't need it for the rest of the authorization workflow - del credentials['request'] + # The credentials object has a request object that it's too big to be serialized, + # so we remove it because we don't need it for the rest of the authorization workflow + del credentials['request'] - return dict(client=client, credentials=credentials) - except Exception as e: - log.exception(e) + return dict(client=client, credentials=json.dumps(credentials)) @expose('jinja:allura:templates/oauth2_authorize_ok.html') @require_post() def do_authorize(self, yes=None, no=None): security.require_authenticated() - credentials = ast.literal_eval(request.params['credentials']) client_id = request.params['client_id'] client = M.OAuth2ClientApp.query.get(client_id=client_id) @@ -552,30 +548,31 @@ class Oauth2Negotiator: flash(f'{client.name} NOT AUTHORIZED', 'error') redirect('/auth/oauth2/') - try: - headers, body, status = self.server.create_authorization_response( - uri=request.url, http_method=request.method, body=request.body, headers=request.headers, scopes=[], credentials=credentials - ) - - response.status_int = status - for k, v in headers.items(): - response.headers[k] = v + credentials = json.loads(request.params['credentials']) + headers, body, status = self.server.create_authorization_response( + uri=request.url, http_method=request.method, body=request.body, headers=request.headers, scopes=[], credentials=credentials + ) - return body - except Exception as ex: - log.exception(ex) + response.status_int = status + for k, v in headers.items(): + response.headers[k] = v + return body @expose('json:') @require_post() def token(self, **kwargs): + decoded_body = str(request.body, 'utf-8') + + # We try to parse the request body as JSON, if it fails we just use the body as is + # so it's treated as x-www-form-urlencoded try: - decoded_body = str(request.body, 'utf-8') - json_body = json.loads(decoded_body) - headers, body, status = self.server.create_token_response(uri=request.url, http_method=request.method, body=json_body, headers=request.headers) - return body - except Exception as e: - log.exception(e) + request_body = json.loads(decoded_body) + except json.decoder.JSONDecodeError: + request_body = decoded_body + + headers, body, status = self.server.create_token_response(uri=request.url, http_method=request.method, body=request_body, headers=request.headers) + return body def rest_has_access(obj, user, perm): diff --git a/Allura/allura/model/oauth.py b/Allura/allura/model/oauth.py index 9e9b10c23..6ce08776a 100644 --- a/Allura/allura/model/oauth.py +++ b/Allura/allura/model/oauth.py @@ -158,7 +158,7 @@ class OAuth2ClientApp(MappedClass): class __mongometa__: session = main_orm_session name = 'oauth2_client_app' - unique_indexes = [('client_id', 'owner_id')] + unique_indexes = [('client_id', 'user_id')] query: 'Query[OAuth2ClientApp]' @@ -168,20 +168,20 @@ class OAuth2ClientApp(MappedClass): name = FieldProperty(str) description = FieldProperty(str, if_missing='') description_cache = FieldProperty(MarkdownCache) - owner_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) + user_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) grant_type = FieldProperty(str, if_missing='authorization_code') response_type = FieldProperty(str, if_missing='code') scopes = FieldProperty([str]) redirect_uris = FieldProperty([str]) - owner = RelationProperty('User') + user = RelationProperty('User') @classmethod - def for_owner(cls, owner=None): - if owner is None: - owner = c.user - return cls.query.find(dict(owner_id=owner._id)).all() + def for_user(cls, user=None): + if user is None: + user = c.user + return cls.query.find(dict(user_id=user._id)).all() @property def description_html(self): @@ -192,13 +192,13 @@ class OAuth2AuthorizationCode(MappedClass): class __mongometa__: session = main_orm_session name = 'oauth2_authorization_code' - unique_indexes = [('authorization_code', 'client_id', 'owner_id')] + unique_indexes = [('authorization_code', 'client_id', 'user_id')] query: 'Query[OAuth2AuthorizationCode]' _id = FieldProperty(S.ObjectId) client_id = FieldProperty(str) - owner_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) + user_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) scopes = FieldProperty([str]) redirect_uri = FieldProperty(str) authorization_code = FieldProperty(str) @@ -207,27 +207,27 @@ class OAuth2AuthorizationCode(MappedClass): code_challenge = FieldProperty(str) code_challenge_method = FieldProperty(str) - owner = RelationProperty('User') + user = RelationProperty('User') class OAuth2AccessToken(MappedClass): class __mongometa__: session = main_orm_session name = 'oauth2_access_token' - unique_indexes = [('access_token', 'client_id', 'owner_id')] + unique_indexes = [('access_token', 'client_id', 'user_id')] query: 'Query[OAuth2AccessToken]' _id = FieldProperty(S.ObjectId) client_id = FieldProperty(str) - owner_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) + user_id: ObjectId = AlluraUserProperty(if_missing=lambda: c.user._id) scopes = FieldProperty([str]) access_token = FieldProperty(str) refresh_token = FieldProperty(str) expires_at = FieldProperty(S.DateTime) last_access = FieldProperty(datetime) - owner = RelationProperty('User') + user = RelationProperty('User') def dummy_oauths(): diff --git a/Allura/allura/tests/functional/test_auth.py b/Allura/allura/tests/functional/test_auth.py index 2ed7b06f4..e50a34817 100644 --- a/Allura/allura/tests/functional/test_auth.py +++ b/Allura/allura/tests/functional/test_auth.py @@ -2090,14 +2090,14 @@ class TestOAuth2(TestController): user = M.User.by_username('test-admin') M.OAuth2ClientApp( client_id='client_12345', - owner_id=user._id, + user_id=user._id, name='testoauth2', description='test client', response_type='code', redirect_uris=['https://localhost/'] ) ThreadLocalODMSession.flush_all() - r = self.app.get('/rest/oauth2/authorize/', params={'client_id': 'client_12345', 'response_type': 'code'}) + r = self.app.get('/rest/oauth2/authorize', params={'client_id': 'client_12345', 'response_type': 'code', 'redirect_uri': 'https://localhost/'}) assert 'testoauth2' in r.text assert 'client_12345' in r.text @@ -2106,7 +2106,7 @@ class TestOAuth2(TestController): user = M.User.by_username('test-admin') M.OAuth2ClientApp( client_id='client_12345', - owner_id=user._id, + user_id=user._id, name='testoauth2', description='test client', response_type='code', @@ -2121,7 +2121,7 @@ class TestOAuth2(TestController): user = M.User.by_username('test-admin') M.OAuth2ClientApp( client_id='client_12345', - owner_id=user._id, + user_id=user._id, name='testoauth2', description='test client', response_type='code', @@ -2133,7 +2133,10 @@ class TestOAuth2(TestController): r = self.app.get('/rest/oauth2/authorize', params={'client_id': 'client_12345', 'response_type': 'code', 'redirect_uri': 'https://localhost/'}) # The submit authorization for the authorization code to be created - r = self.app.post('/rest/oauth2/do_authorize', params={'yes': '1', 'client_id': 'client_12345', 'response_type': 'code', 'redirect_uri': 'https://localhost/'}) + mock_credentials = dict(client_id='client_12345', redirect_uri='https://localhost/', response_type='code', state=None) + r = self.app.post('/rest/oauth2/do_authorize', + params={'yes': '1', 'client_id': 'client_12345', 'response_type': 'code', + 'redirect_uri': 'https://localhost/', 'credentials': json.dumps(mock_credentials)}) q = M.OAuth2AuthorizationCode.query.get(client_id='client_12345') assert q is not None @@ -2146,7 +2149,8 @@ class TestOAuth2(TestController): user = M.User.by_username('test-admin') M.OAuth2ClientApp( client_id='client_12345', - owner_id=user._id, + client_secret='98765', + user_id=user._id, name='testoauth2', description='test client', response_type='code', @@ -2158,7 +2162,10 @@ class TestOAuth2(TestController): r = self.app.get('/rest/oauth2/authorize', params={'client_id': 'client_12345', 'response_type': 'code', 'redirect_uri': 'https://localhost/'}) # The submit authorization for the authorization code to be created - r = self.app.post('/rest/oauth2/do_authorize', params={'yes': '1', 'client_id': 'client_12345', 'response_type': 'code', 'redirect_uri': 'https://localhost/'}) + mock_credentials = dict(client_id='client_12345', redirect_uri='https://localhost/', response_type='code', state=None) + r = self.app.post('/rest/oauth2/do_authorize', + params={'yes': '1', 'client_id': 'client_12345', 'response_type': 'code', + 'redirect_uri': 'https://localhost/', 'credentials': json.dumps(mock_credentials)}) ac = M.OAuth2AuthorizationCode.query.get(client_id='client_12345') assert ac is not None @@ -2169,8 +2176,10 @@ class TestOAuth2(TestController): # Create the authorization token oauth2_params = dict( client_id='client_12345', + client_secret='98765', code=ac.authorization_code, - grant_type='authorization_code' + grant_type='authorization_code', + redirect_uri='https://localhost/' ) r = self.app.post_json('/rest/oauth2/token', oauth2_params) t = M.OAuth2AccessToken.query.get(client_id='client_12345') @@ -2186,7 +2195,7 @@ class TestOAuth2(TestController): user = M.User.by_username('test-admin') M.OAuth2ClientApp( client_id='client_12345', - owner_id=user._id, + user_id=user._id, name='testoauth2', description='test client', response_type='code', @@ -2197,7 +2206,7 @@ class TestOAuth2(TestController): client_id='client_12345', authotization_code='authcode_12345', expires_at=datetime.utcnow() + timedelta(minutes=10), - owner_id=user._id, + user_id=user._id, ) M.OAuth2AccessToken( @@ -2205,7 +2214,7 @@ class TestOAuth2(TestController): access_token='12345', refresh_token='54321', expires_at=datetime.utcnow() + timedelta(minutes=20), - owner_id=user._id, + user_id=user._id, ) ThreadLocalODMSession.flush_all()