aliev / aioauth

Asynchronous OAuth 2.0 provider for Python 3
https://aliev.me/aioauth
MIT License
214 stars 19 forks source link

Fix token introspection and revocation authentication checks #93

Closed shawnz closed 3 months ago

shawnz commented 3 months ago

This PR fixes two issues I noticed with the token introspection and revocation endpoints:

This PR fixes the first issue by adding calls to storage.get_client, which validates the secret.

The second issue is fixed by adding a new parameter to the get_client_credentials function, secret_required, which is set to False for the revoke token endpoint. This new parameter is also used to simplify some of the logic in the create_token_response function, which previously used a complicated try/except block to achieve the same thing.

Some tests are also added to ensure the new functionality works.

Before making the change:

FAILED tests/test_endpoint.py::test_introspect_token_with_wrong_client_secret - assert <HTTPStatus.OK: 200> == <HTTPStatus.UNAUTHORIZED: 401>
FAILED tests/test_endpoint.py::test_revoke_access_token_without_client_secret - assert <HTTPStatus.UNAUTHORIZED: 401> == <HTTPStatus.NO_CONTENT: 204>
FAILED tests/test_endpoint.py::test_revoke_access_token_with_wrong_client_secret - assert <HTTPStatus.NO_CONTENT: 204> == <HTTPStatus.UNAUTHORIZED: 401>

Results (0.58s):
      44 passed
       3 failed
         - tests/test_endpoint.py:163 test_introspect_token_with_wrong_client_secret
         - tests/test_endpoint.py:272 test_revoke_access_token_without_client_secret
         - tests/test_endpoint.py:297 test_revoke_access_token_with_wrong_client_secret

After making the change:

Results (0.81s):
      47 passed
shawnz commented 3 months ago

An additional note: the specification for token introspection, RFC 7662, doesn't actually say client authentication is necessary for token introspection. But, it does say that some kind of authentication is necessary, and suggests client authentication as one possible option. However, in its current state, this library only supports client authentication and not any other possible authentication system for token introspection.

I didn't make any effort to fix that here, I simply added validation to make sure that the client secret is correct (since it was already required to be present with the existing logic anyway).