ARCH-603: Add user_id scope to various defaults. #20057
Conversation
| """ | ||
| use_asymmetric_key = _get_use_asymmetric_key_value(is_restricted, use_asymmetric_key) | ||
| scopes = scopes or ['email', 'profile'] | ||
| scopes = scopes or ['user_id', 'email', 'profile'] |
There was a problem hiding this comment.
Reviewer: I don't think this was needed for ecommerce social-auth flow, but I'm not sure about other use cases. It should be more clear when adding a new scope what needs to change. Maybe we need some constants? For example, should these default scopes in cookies always match here? If not, maybe we could add comments or well-named constants to help explain.
There was a problem hiding this comment.
Are we trying to limit the combination of profile and user_id scopes to only privileged clients? If so, I recommend not making this change.
There was a problem hiding this comment.
Thanks. I may introduce a constant named NON_PRIVILEGED_DEFAULT_SCOPES to make this more clear, and maybe one for the cookies.
There was a problem hiding this comment.
@nasthagiri: FYI: I switched this to a comment to try to clarify my concern.
robrap
changed the title
nasthagiri
left a comment
nasthagiri
left a comment
There was a problem hiding this comment.
@robrap A passing response to your question above. Since I don't have time to dive into the details, I'm not sure whether #1 in your comment is really needed. I leave it to you to dive into it.
Regarding #2, I assume this change is only needed for those services that use the new OAuth2+SSO flow. So that's only Credentials and e-commerce at this point.
There was a problem hiding this comment.
I wouldn't really call this non_privileged_default_scopes since once would say that email and profile are also privileged. These are simply scopes that we default to when one isn't passed in.
There was a problem hiding this comment.
Thanks. I see that this change didn't communicate what I wish to. I will try again.
|
Thanks @nasthagiri. Your passing comment was helpful. I will try the simpler major version upgrade for auth-backends, and document that the OAuth2+SSO flow now requires that an application has access to the |
|
@nasthagiri: FYI: This is ready once it gets a thumb. |
robrap
changed the title
This should have been done when the scope was first added as part of (#19765). ARCH-603
robrap
force-pushed
the
robrap/ARCH-603-fix-user-id-scope
branch
from
00f3636 to
e52db40
Compare
|
Your PR has finished running tests. There were no failures. |
|
@nasthagiri: if you want any changes to the comments I’m happy to do a follow-up PR. I just wanted to get this blocking change out in edx-platform asap. Thanks. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, March 29, 2019. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
Add user_id scope to the list of available scopes. This should have been done when the scope was added as part of https://github.com/edx/edx-platform/pull/19765.
Note: This is to enable the user_id claim to make it to ecommerce when requested through social-auth oAuth+SSO flow.
ARCH-603