Apache Allura™

• can delete oauth2_authorize_ok.html too
• it's a bit annoying to have to make a client app, just to generate a bearer token. But that was good enough for oauth1 and would require probably a fair bit of changes to have a token without an app. Maybe we should just have a sentence on the OAuth page explaining for direct API usage, create a client app and then generate a bearer token
• for indexes, this change (below) would make all 4 fields together be unique. It'd probably be better to have multiple unique indexes, with access_token on its own and refresh_token on its own being unique. Similar for the client app class and auth code class, i think additional unique indexes should be added. To make sure there's no way to duplicate an id/code/token ever.

-        unique_indexes = [('access_token', 'client_id', 'user_id')]
+        unique_indexes = [('access_token', 'refresh_token', 'client_id', 'user_id')]

• the redirect('/auth/' code could work, but I think the "normal" redirect behavior is in LoginRedirectMiddleware. It has an exception for /rest/ but I think you could allow the authorize urls to work there. And then it'll use the same code as everything else. OR maybe better we could move the authorize endpoint to be under /auth/ instead of under /rest/ It doesn't really make sense to have a regular web page under /rest/ (which is for APIs)
• in test_pkce after loading the authorize page, you don't need to build up all the post params in the test. See test_authorize for example of accessing the form on the page (which has all the params in it) and submitting it. That's more realistic too.
• test_oauth2_expired_token_authentication is missing the @mock.patch.dict(config, {'auth.oauth2.enabled': True}) and has a future expiration date (should be in the past)
• might be good to have a few more negative tests? for example test_refresh_token could make variations where the token is bad, or the client secret is bad, etc. Similar with authorization code requests.
• test failure:

Allura/allura/tests/functional/test_rest.py:463: in test_oauth2_expired_token_authentication
    assert '401 Unauthorized' in str(ex.value)
../env3-allura/lib/python3.11/site-packages/_pytest/_code/code.py:559: in value
    self._excinfo is not None
E   AssertionError: .value can only be used after the context manager exits

• Deleted oauth2_authorize_ok.html which is no longer needed
• Added a message at the top of the OAuth apps page saying that need to create a client app and generate a bearer token for direct API usage
• Created unique indices for access_token, refresh_token, and authorization_token
• Moved the OAuth2 authorization pages to the auth controller as a better option to redirect to the login page when accessing while logged out
• Replaced all instances of /rest/oauth2/authorize to the new /auth/oauth2/authorize
• Made test fixes and added additional tests that use wrong values and validate error responses
• Fixed a bug that allowed to create multiple authorization codes and bearer tokens for the same user and client id combination

Nice work on all the negative tests!

• generate_bearer_token i think it could be okay to allow multiple tokens. I've done that before to have different ones for different things. And it could be surprising when you click "Generate Bearer Token" and it replaces your old one, making it not work any more.
• don't need log.info(f'Validating client id: {client_id}')
• OAuth2ClientApp can we add a unique index on client_id?

• Reverted generate_bearer_token to generate a different token every time it's clicked
• Removed the client validation log message
• Added unique index on client_id to OAuth2ClientApp

Uh oh our indexes are having an issue with multiple bearer tokens now. If I try to generate a 2nd bearer token for myself I get an error: E11000 duplicate key error collection: pyforge.oauth2_access_token index: refresh_token_1 dup key: { refresh_token: null }

Would it be ok to ignore null refresh_tokens? Probably? If so, then we could move that index to be like this I believe:

custom_indexes = [
    dict(fields=('refresh_token',), sparse=True, unique=True),
]