fixup! [#7272] Implement additional security features for OAuth2 support
diff --git a/Allura/allura/controllers/auth.py b/Allura/allura/controllers/auth.py
index d8e4366..1b9ea3d 100644
--- a/Allura/allura/controllers/auth.py
+++ b/Allura/allura/controllers/auth.py
@@ -1416,8 +1416,8 @@ def _check_security(self):
# 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 @@ def _revoke_all(self, client_id):
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 @@ def register(self, application_name=None, application_description=None, redirect
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 @@ def register(self, application_name=None, application_description=None, redirect
@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 d072206..b1bafea 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 @@ def invalidate_authorization_code(self, client_id: str, code: str, request: oaut
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 @@ def confirm_redirect_uri(self, client_id: str, code: str, redirect_uri: str, cli
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 @@ def save_authorization_code(self, client_id: str, code, request: oauthlib.common
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 @@ def save_authorization_code(self, client_id: str, code, request: oauthlib.common
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 @@ def server(self):
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 @@ def _authenticate(self):
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 @@ def authorize(self, **kwargs):
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)
- # 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']
+ client_id = request.params.get('client_id')
+ client = M.OAuth2ClientApp.query.get(client_id=client_id)
- return dict(client=client, credentials=credentials)
- except Exception as e:
- log.exception(e)
+ # 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=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 @@ def do_authorize(self, yes=None, no=None):
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
- )
+ 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
+ )
- response.status_int = status
- for k, v in headers.items():
- response.headers[k] = v
+ response.status_int = status
+ for k, v in headers.items():
+ response.headers[k] = v
- return body
- except Exception as ex:
- log.exception(ex)
-
+ 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 9e9b10c..6ce0877 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 __mongometa__:
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 __mongometa__:
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 2ed7b06..e50a348 100644
--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -2090,14 +2090,14 @@ def test_authorize(self):
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 @@ def test_do_authorize_no(self):
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 @@ def test_do_authorize(self):
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 @@ def test_do_authorize(self):
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 @@ def test_create_access_token(self):
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 @@ def test_create_access_token(self):
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 @@ def test_create_access_token(self):
# 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 @@ def test_revoke_tokens(self):
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 @@ def test_revoke_tokens(self):
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 @@ def test_revoke_tokens(self):
access_token='12345',
refresh_token='54321',
expires_at=datetime.utcnow() + timedelta(minutes=20),
- owner_id=user._id,
+ user_id=user._id,
)
ThreadLocalODMSession.flush_all()
