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.
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
Last edit: Dave Brondsema 2024-04-30
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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:
user_id
field, you can add auser = RelationProperty('User')
line similar to oauth1 models. That lets you dosomeClient.user
and it'll automatically load a User object for you based on the user_id value.description_cache
field, so that thedescription_html
helper property can workaccount_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?TestOAuth2
to oauth1: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 hasextra_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 usernametest-admin
(the default for all tests normally)invalidate_authorization_code
andvalidate_code
andsave_authorization_code
andsave_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.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)save_bearer_token
say it should save the userThese can be separate followup work:
wiki-copy.py
example to use oauth2 (or a separate copy for oauth2).challenge
andchallenge_method
for PKCE aren't used yet. That can be followup too.option to add multiple, verify them, edit them, etc. (validate_redirect_uri
but docs also mentionconfirm_redirect_uri
? also get_default_redirect_uri)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.Related
Tickets:
#7272Last edit: Dave Brondsema 2024-04-18
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 redirectsallura/tests/functional/test_root.py
has failures because of thisallura/tests/functional/test_auth.py
andallura/tests/functional/test_rest.py
test failures. Interestingly, they pass if I haveauth.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:
TestOAuth2
to oauth1: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 hasextra_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 usernametest-admin
(the default for all tests normally)M.OAuth2AuthorizationCode.query.update
andM.OAuth2AccessToken.query.update
(2x) are too broad. Are updates even needed?save_bearer_token
usesowner_id = client.owner_id
, are we sure that's right? what aboutc.user
? docs say "request.user"Oauth2Negotiator._authenticate
doesn't need first 4 lines?do_authorize
: if its returning a full 'body' does the template oauth2_authorize_ok.html get used? And is the try/except needed?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
ondef authorize
. If you accidentally do have a trailing slash, the form submits to the wrong place. This decorator will prevent thatwiki-copy.py
example to use oauth2 (or a separate copy for oauth2).Last edit: Dave Brondsema 2024-04-30