[#7272] Implement additional security features for OAuth2 support
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index 3f3f82f..d8e4366 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -117,7 +117,7 @@
 
         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 @@
     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 @@
         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 @@
     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 @@
             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 b16ae0d..d072206 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 @@
 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 @@
     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 @@
         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)
+        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 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
-            )
+        if current_token:
+            M.OAuth2AccessToken.remove({'client_id': request.client_id, 'owner_id': c.user._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}')
+        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}')
 
 
 class AlluraOauth1Server(oauthlib.oauth1.WebApplicationServer):
@@ -502,6 +515,7 @@
 
 
     @expose('jinja:allura:templates/oauth2_authorize.html')
+    @without_trailing_slash
     def authorize(self, **kwargs):
         security.require_authenticated()
         json_body = None
@@ -513,16 +527,15 @@
 
         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 @@
     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 @@
 
         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 f6ddcad..9e9b10c 100644
--- a/Allura/allura/model/oauth.py
+++ b/Allura/allura/model/oauth.py
@@ -164,7 +164,7 @@
 
     _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 @@
             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 510342e..a0d5f01 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 28cc650..8bcba06 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()}}