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 <[email protected]>
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()}}
