Apache Allura™

First pass of feedback, also including earlier merge request(s). More to come probably, but I wanted to give you what I have so far:

• the commits should use Allura ticket numbers. I found a ticket for oauth2 support here: https://forge-allura.apache.org/p/allura/tickets/7272/ so can you rebase the branch and edit the commit messages to be [#7272]?
• test_revoke_tokens fails
• comparing model classes to oauth1 ones:
  • those that have a user_id field, you can add a user = RelationProperty('User') line similar to oauth1 models. That lets you do someClient.user and it'll automatically load a User object for you based on the user_id value.
  • OAuth2Client should have a description_cache field, so that the description_html helper property can work
  • last_access field would nice to have, and set similarly to oauth1. Not sure which classes need it.
  • OAuth2Token probably should have a user_id and user field? and populate it accordingly. https://oauthlib.readthedocs.io/en/latest/oauth2/server.html#bearer-token-oauth-2-standard-token recommends it
• plugin.py account_navigation should have a link to oauth2. OR it might be more intuitive for users to have just one page for Oauth, no matter which version. And maybe we have an option disable oauth1 at some point?
• RestController._check_security has special handling for /rest/oauth/ Probabably want it for oauth2 too?
• comparing TestOAuth2 to oauth1:
  • need equivalents for TestOAuthRequestToken and TestOAuthAccessToken?
  • oauth1 used oauth1_webtest helper I think because oauth1 has more complex parameters. Seems like we can do oauth2 directly without that kind of helper, great! BUT it has extra_environ = {'username': '*anonymous'} # we don't want to be magically logged in when hitting /rest/oauth/ and we need that for many of the oauth2 tests I'm pretty sure. You can pass that as a kwarg for every get/post call. Or if basically all gets/posts need it, you can set it as self.app.extra_environ in the test or a shared setup_method and it'll apply to all gets & posts. Then when you DO want to be logged in, you can pass it as a kwarg with username test-admin (the default for all tests normally)
  • do the oauth2 tests cover command-line and web redirect modes? The oauth1 tests have more test cases, so it makes me wonder if we're missing anything. Or maybe oauth2 is just simpler?
• following on to the web redirect concept... I tested with https://oauthdebugger.com/ and after the approval page I ended up on a page that said "Authorization Code:" (blank). I expected to not see a page like that at all, but to be redirect back to oauthdebugger.com
• if I approve an app as a user that is NOT the owner of the Client, I think we should display that authorization on the Oauth/2 settings page. So that they can revoke that access again in the future.
• invalidate_authorization_code and validate_code and save_authorization_code and save_bearer_token query for all codes based just on a given client_id. Seems like it should query based on the given code too? Because there may be many separate users of one client. It would be good to have a test using an invalid code, it could've caught this issue I think.
• docs for save_authorization_code say it should save the redirect_uri and the user. (We're not using scopes, so I think that one is ok to skip)
• docs for save_bearer_token say it should save the user
• probably should make a .ini config setting to enable oauth2 support or not. At the very least so we can have it disabled while this work is still ongoing, and then enable when its fully ready. And then maybe in the future we use the same pattern for disabling oauth1 support.

These can be separate followup work:

• we'll need to update Allura/docs/api-rest/* files. And probably change wiki-copy.py example to use oauth2 (or a separate copy for oauth2).
• challenge and challenge_method for PKCE aren't used yet. That can be followup too.
• need a UI to generate bearer tokens like oauth1?
• redirect uris: option to add multiple, verify them, edit them, etc. (validate_redirect_uri but docs also mention confirm_redirect_uri? also get_default_redirect_uri)
• try restricting what validate_grant_type permits? https://oauthlib.readthedocs.io/en/latest/oauth2/grants/grants.html describe several options and it seems like maybe we only need authorization codes? If so, lets restrict to just that, and not have to worry about anyone trying to use the other grant types.

• need to restore the csp.form_actions_enforce code that is commented out. Either now or in a following merge request, can make it skip only for the oauth redirects
  • allura/tests/functional/test_root.py has failures because of this
• allura/tests/functional/test_auth.py and allura/tests/functional/test_rest.py test failures. Interestingly, they pass if I have auth.oauth2.enabled = false in development.ini (but then some oauth2 tests fail of course)

If those get addressed, then I can merge this as an incremental step forward. Feedback for the next round of updates:

• CSP (if not done already)
• comparing TestOAuth2 to oauth1:
  • need equivalents for TestOAuthRequestToken and TestOAuthAccessToken?
  • oauth1 used oauth1_webtest helper I think because oauth1 has more complex parameters. Seems like we can do oauth2 directly without that kind of helper, great! BUT it has extra_environ = {'username': '*anonymous'} # we don't want to be magically logged in when hitting /rest/oauth/ and we need that for many of the oauth2 tests I'm pretty sure. You can pass that as a kwarg for every get/post call. Or if basically all gets/posts need it, you can set it as self.app.extra_environ in the test or a shared setup_method and it'll apply to all gets & posts. Then when you DO want to be logged in, you can pass it as a kwarg with username test-admin (the default for all tests normally)
  • do the oauth2 tests cover command-line and web redirect modes? The oauth1 tests have more test cases, so it makes me wonder if we're missing anything. Or maybe oauth2 is just simpler?
• PKCE
• just one page for Oauth1 and Oauth2 to be managed
  • update how it oauth2 tokens are queried, should get clients & tokens separately (see how oauth1 does it) so the ClientApp owner cannot see all the authorizations made by other users. The other users should see their own authorizations and be able to revoke them
  • a button to generate bearer (personal use) tokens, like oauth1 has
  • ability to edit name/description/redirect_uri
• M.OAuth2AuthorizationCode.query.update and M.OAuth2AccessToken.query.update (2x) are too broad. Are updates even needed?
• save_bearer_token uses owner_id = client.owner_id, are we sure that's right? what about c.user? docs say "request.user"
• last_access on OAuth2AuthorizationCode?
• Oauth2Negotiator._authenticate doesn't need first 4 lines?
• in do_authorize: if its returning a full 'body' does the template oauth2_authorize_ok.html get used? And is the try/except needed?
• unique indexes on just authorization_code, access_token, refresh_token on their own? to ensure those codes never get accidentally duplicated?
• M.OAuth2ClientApp.set_credentials isn't going to work out, saving it for the whole ClientApp (what if 2 users were authorizing the same app at the same time). What's a better way?
• @without_trailing_slash on def authorize. If you accidentally do have a trailing slash, the form submits to the wrong place. This decorator will prevent that
• we'll need to update Allura/docs/api-rest/* files. And probably change wiki-copy.py example to use oauth2 (or a separate copy for oauth2).
• client apps need a "client secret" in addition to id
• refresh token endpoint