Git Merge Request #412: Implement security features for OAuth2 support (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Carlos Cruz wants to merge 1 commit from /u/ccruz/allura/ to master, 7 days ago

  • Automatically generates a secret for all newly created client apps
  • Authorization codes and access tokens are created per user on a given client
  • Codes are tokens revocation are now also per user
  • Added support for PKCE (optional)
  • Removed the code that saved oauthlib's credentials object to the database. It now removes a big nested request object which is not needed, and passes it around to the authorization form in a hidden input.
  • Additional validations in the authorization and access tokens workflow
Commit Date  
[600cd2] (cc/9357) by Carlos Cruz Carlos Cruz

[#7272] Implement additional security features for OAuth2 support

 2024-05-08 17:04:30 Tree

Discussion

  • Dave Brondsema

    Dave Brondsema - 2024-05-09
    • instead of ast.literal_eval what about json.loads? I think that'd be a bit more typical and safer (literal_eval is more safe than regular eval, but I just checked the docs and it does have some warnings about ways it might not be safe)
    • several TestOAuth2 tests fail with 
    allura/controllers/rest.py:547: in do_authorize
    credentials = ast.literal_eval(request.params['credentials'])
../../env3-allura/lib/python3.11/site-packages/webob/multidict.py:344: in __getitem__
    raise KeyError(key)
E   KeyError: 'credentials'
    • after authorizing a client, and getting a token, I tried using it curl 'https://my-dev-site/rest/' -H 'Authorization: Bearer REDACTED' but it didn't work, I got a 401. Maybe because the token's expires_at is set to 1970-01-01T01:00:00?
     

  • Dave Brondsema

    Dave Brondsema - 2024-05-09

    I also got this error when re-authorizing an app a 2nd time 

    15:55:11,232 ERROR [allura.controllers.rest] type object 'OAuth2AccessToken' has no attribute 'remove'
Traceback (most recent call last):
  File "/src/allura/Allura/allura/controllers/rest.py", line 575, in token
    headers, body, status = self.server.create_token_response(uri=request.url, http_method=request.method, body=json_body, headers=request.headers)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/local/env-allura/lib/python3.11/site-packages/oauthlib/oauth2/rfc6749/endpoints/base.py", line 112, in wrapper
    return f(endpoint, uri, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/local/env-allura/lib/python3.11/site-packages/oauthlib/oauth2/rfc6749/endpoints/token.py", line 114, in create_token_response
    return grant_type_handler.create_token_response(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/local/env-allura/lib/python3.11/site-packages/oauthlib/oauth2/rfc6749/grant_types/authorization_code.py", line 312, in create_token_response
    self.request_validator.save_token(token, request)
  File "/var/local/env-allura/lib/python3.11/site-packages/oauthlib/oauth2/rfc6749/request_validator.py", line 315, in save_token
    return self.save_bearer_token(token, request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/src/allura/Allura/allura/controllers/rest.py", line 341, in save_bearer_token
    M.OAuth2AccessToken.remove({'client_id': request.client_id, 'owner_id': c.user._id})
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: type object 'OAuth2AccessToken' has no attribute 'remove'

    And it was kind of confusing because from the request side, I just got a response that was 204 No Content with no message at all. I think we should remove all the 

    try:
    ...
except Exception as ex:
    log.exception(ex)

    in rest.py. If there's some unexpected error its good to let the page completely error out so the client gets a 500 error and we get fatal errors in the normal places

     

  • Carlos Cruz - 2024-05-10

    Fixes:

    • Reverted the use of owner and owner_id back to user and user_id in order to be consistent with the user objects set in the authentication pipeline.
    • Removed the use of ast.literal_eval and replaced it in favor of json.dumps to serialize the credentials object and json.loads to reconstruct it
    • Fixed the issue with access tokens' expiration date
    • Fixed failing tests
    • Removed try / except clauses from the authorization and token endpoints
    • The OAuth2AccessToken has no attribute remove error was fixed by executing OAuth2AccessToken.query.remove instead of OAuth2AccessToken.remove
    • Fixed the authentication workflow for OAuth2 bearer tokens by first validating the request and then attempting to extract the token from the header
     

  • Dave Brondsema

    Dave Brondsema - 7 days ago
    • Status: open --> merged
     

Log in to post a comment.