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 bf7da30a3 [#7272] Implement additional security features for OAuth2 support bf7da30a3 is described below commit bf7da30a3a45e13be4edde7cc460f25771e58096 Author: Carlos Cruz <carlos.c...@slashdotmedia.com> AuthorDate: Wed May 8 17:04:30 2024 +0000 [#7272] Implement additional security features for OAuth2 support --- Allura/allura/controllers/auth.py | 17 ++-- Allura/allura/controllers/rest.py | 108 +++++++++++++---------- Allura/allura/model/oauth.py | 6 +- Allura/allura/templates/oauth2_applications.html | 1 + Allura/allura/templates/oauth2_authorize.html | 1 + 5 files changed, 76 insertions(+), 57 deletions(-) diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py index 3f3f82fdb..d8e436657 100644 --- a/Allura/allura/controllers/auth.py +++ b/Allura/allura/controllers/auth.py @@ -117,7 +117,7 @@ class AuthController(BaseController): if asbool(config.get('auth.oauth2.enabled', False)): self.oauth2 = OAuth2Controller() - + if asbool(config.get('auth.allow_user_to_disable_account', False)): self.disable = DisableAccountController() @@ -1414,6 +1414,12 @@ class OAuth2Controller(BaseController): def _check_security(self): require_authenticated() + # 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}) + + # Revokes the authorization code and access tokens for a given client and all its users def _revoke_all(self, client_id): M.OAuth2AuthorizationCode.query.remove({'client_id': client_id}) M.OAuth2AccessToken.query.remove({'client_id': client_id}) @@ -1427,8 +1433,8 @@ class OAuth2Controller(BaseController): model = [] for client in clients: - authorization = M.OAuth2AuthorizationCode.query.get(client_id=client.client_id) - token = M.OAuth2AccessToken.query.get(client_id=client.client_id) + 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) model.append(dict(client=client, authorization=authorization, token=token)) return dict( @@ -1442,7 +1448,8 @@ class OAuth2Controller(BaseController): def register(self, application_name=None, application_description=None, redirect_url=None, **kw): M.OAuth2ClientApp(name=application_name, description=application_description, - redirect_uris=[redirect_url]) + redirect_uris=[redirect_url], + owner_id=c.user._id) flash('Oauth2 Client registered') redirect('.') @@ -1460,7 +1467,7 @@ class OAuth2Controller(BaseController): flash('Client deleted and access tokens revoked.') if revoke: - self._revoke_all(_id) + self._revoke_user_tokens(_id, c.user._id) flash('Access tokens revoked.') redirect('.') diff --git a/Allura/allura/controllers/rest.py b/Allura/allura/controllers/rest.py index b16ae0dec..d07220616 100644 --- a/Allura/allura/controllers/rest.py +++ b/Allura/allura/controllers/rest.py @@ -17,6 +17,7 @@ """REST Controller""" from __future__ import annotations +import ast import json import logging from datetime import datetime, timedelta @@ -31,6 +32,7 @@ import tg from tg import expose, flash, redirect, config from tg import tmpl_context as c, app_globals as g from tg import request, response +from tg.decorators import without_trailing_slash import colander from ming.odm import session @@ -277,12 +279,24 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): def get_default_redirect_uri(self, client_id: str, request: oauthlib.common.Request, *args, **kwargs) -> str: return request.uri + def is_pkce_required(self, client_id: str, request: oauthlib.common.Request) -> bool: + return False + + def get_code_challenge(self, code: str, request: oauthlib.common.Request) -> str: + authorization_code = M.OAuth2AuthorizationCode.query.get(authorization_code=code) + return authorization_code.code_challenge + + def get_code_challenge_method(self, code: str, request: oauthlib.common.Request) -> str: + authorization_code = M.OAuth2AuthorizationCode.query.get(authorization_code=code) + return authorization_code.code_challenge_method + def invalidate_authorization_code(self, client_id: str, code: str, request: oauthlib.common.Request, *args, **kwargs) -> None: 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['client_id'] - request.client = M.OAuth2ClientApp.query.get(client_id=client_id) + 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) 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: @@ -294,50 +308,49 @@ class Oauth2Validator(oauthlib.oauth2.RequestValidator): return access_token.expires_at >= datetime.utcnow() if access_token else False def confirm_redirect_uri(self, client_id: str, code: str, redirect_uri: str, client: oauthlib.oauth2.Client, request: oauthlib.common.Request, *args, **kwargs) -> bool: - return True + # This method is called when the client is exchanging the authorization code for an access token. + # If a redirect uri was provided when the authorization code was created, it must match the redirect uri provided here. + authorization = M.OAuth2AuthorizationCode.query.get(client_id=client_id, authorization_code=code) + 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, authorization_code=code['code']) - log.info('Saving authorization code for client: %s', client_id) + authorization = M.OAuth2AuthorizationCode.query.get(client_id=client_id, owner_id=c.user._id, authorization_code=code['code']) - if not authorization: - auth_code = M.OAuth2AuthorizationCode( - client_id=client_id, - authorization_code=code['code'], - expires_at=datetime.utcnow() + timedelta(minutes=10), - redirect_uri=request.redirect_uri, - owner_id=c.user._id - ) - session(auth_code).flush() - log.info(f'Saving new authorization code for client: {client_id}') - else: - log.info(f'Updating authorization code for {client_id}') - M.OAuth2AuthorizationCode.query.update( - {'client_id': client_id}, - {'$set': {'authorization_code': code['code'], 'expires_at': datetime.utcnow() + timedelta(minutes=10)}}) - log.info(f'Updating authorization code for client: {client_id}') + # 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']}) + + log.info('Saving authorization code for client: %s', client_id) + auth_code = M.OAuth2AuthorizationCode( + client_id=client_id, + authorization_code=code['code'], + expires_at=datetime.utcnow() + timedelta(minutes=10), + redirect_uri=request.redirect_uri, + owner_id=c.user._id, + code_challenge=request.code_challenge, + code_challenge_method=request.code_challenge_method + ) + session(auth_code).flush() + log.info(f'Saving new authorization code for client: {client_id}') def save_bearer_token(self, token, request: oauthlib.common.Request, *args, **kwargs) -> object: - current_token = M.OAuth2AccessToken.query.get(client_id=request.client_id, token=token.get('access_token')) - client = M.OAuth2ClientApp.query.get(client_id=request.client_id) - - if not current_token: - 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 = client.owner_id - ) + 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) + + if current_token: + M.OAuth2AccessToken.remove({'client_id': request.client_id, 'owner_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 + ) - session(bearer_token).flush() - log.info(f'Saving new bearer token for client: {request.client_id}') - else: - M.OAuth2AccessToken.query.update( - {'client_id': request.client_id}, - {'$set': {'access_token': token.get('access_token'), 'expires_at': datetime.utcfromtimestamp(token.get('expires_in')), 'refresh_token': token.get('refresh_token')}}) - log.info(f'Updating bearer token for client: {request.client_id}') + session(bearer_token).flush() + log.info(f'Saving new bearer token for client: {request.client_id}') class AlluraOauth1Server(oauthlib.oauth1.WebApplicationServer): @@ -502,6 +515,7 @@ class Oauth2Negotiator: @expose('jinja:allura:templates/oauth2_authorize.html') + @without_trailing_slash def authorize(self, **kwargs): security.require_authenticated() json_body = None @@ -513,16 +527,15 @@ class Oauth2Negotiator: 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) - # We need to save the credentials to the current session so we can use it later in the POST request. - # We also need to use __dict__ because the internal oauthlib request object cannot be directly serialized - # and saved to Ming - credentials['request'] = credentials['request'].__dict__ - M.OAuth2ClientApp.set_credentials(client_id, credentials) + # 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) + return dict(client=client, credentials=credentials) except Exception as e: log.exception(e) @@ -531,6 +544,7 @@ class Oauth2Negotiator: 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) @@ -540,7 +554,7 @@ class Oauth2Negotiator: try: headers, body, status = self.server.create_authorization_response( - uri=request.url, http_method=request.method, body=request.body, headers=request.headers, scopes=[], credentials=client.credentials + uri=request.url, http_method=request.method, body=request.body, headers=request.headers, scopes=[], credentials=credentials ) response.status_int = status diff --git a/Allura/allura/model/oauth.py b/Allura/allura/model/oauth.py index f6ddcadbc..9e9b10c23 100644 --- a/Allura/allura/model/oauth.py +++ b/Allura/allura/model/oauth.py @@ -164,7 +164,7 @@ class OAuth2ClientApp(MappedClass): _id = FieldProperty(S.ObjectId) client_id = FieldProperty(str, if_missing=lambda: h.nonce(20)) - credentials = FieldProperty(S.Anything) + client_secret = FieldProperty(str, if_missing=h.cryptographic_nonce) name = FieldProperty(str) description = FieldProperty(str, if_missing='') description_cache = FieldProperty(MarkdownCache) @@ -183,10 +183,6 @@ class OAuth2ClientApp(MappedClass): owner = c.user return cls.query.find(dict(owner_id=owner._id)).all() - @classmethod - def set_credentials(cls, client_id, credentials): - cls.query.update({'client_id': client_id }, {'$set': {'credentials': credentials}}) - @property def description_html(self): return g.markdown.cached_convert(self, 'description') diff --git a/Allura/allura/templates/oauth2_applications.html b/Allura/allura/templates/oauth2_applications.html index 510342edf..a0d5f01e0 100644 --- a/Allura/allura/templates/oauth2_applications.html +++ b/Allura/allura/templates/oauth2_applications.html @@ -95,6 +95,7 @@ <tr><th>Name:</th><td>{{app.client.name}}</td></tr> <tr class="description"><th>Description:</th><td>{{app.client.description }}</td></tr> <tr class="client_id"><th>Client ID:</th><td>{{app.client.client_id}}</td></tr> + <tr class="client_secret"><th>Client Secret:</th><td>{{app.client.client_secret}}</td></tr> <tr class="redirect_url"><th>Redirect URL:</th><td>{{app.client.redirect_uris[0] if app.client.redirect_uris else ''}}</td></tr> {% if app.authorization %} diff --git a/Allura/allura/templates/oauth2_authorize.html b/Allura/allura/templates/oauth2_authorize.html index 28cc65055..8bcba060f 100644 --- a/Allura/allura/templates/oauth2_authorize.html +++ b/Allura/allura/templates/oauth2_authorize.html @@ -53,6 +53,7 @@ <div class="flex-container"> <form method="POST" action="do_authorize"> <input type="hidden" name="client_id" value="{{client.client_id}}"/> + <input type="hidden" name="credentials" value="{{credentials}}"/> <input type="submit" class="submit" style="background: #ccc;color:#555" name="no" value="No, do not authorize {{ client.name }}"> <input type="submit" class="button" name="yes" value="Yes, authorize {{ client.name }}"><br> {{lib.csrf_token()}}