-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add scope user_id to JWT payload #33455
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
fb31afc
feat: Add user_id to JWT payload
moeez96 ebbe087
feat: Add scope user_id for JWT tokens
moeez96 3198db5
fix: add unique scope
moeez96 0ac5b01
refactor: update decision document
moeez96 8fb0140
test: Fix tests
moeez96 f512a53
Merge branch 'master' into moeez96/LEARNER-9640
moeez96 5ce6c4d
refactor: Nit
moeez96 a0bd320
feat: add updated scope to API response
moeez96 f87fee5
refactor: Simplify code and revert test changes
moeez96 5fd92bc
test: add missing line
moeez96 91390f6
test: nit
moeez96 df587c3
Merge branch 'master' into moeez96/LEARNER-9640
moeez96 b0e47d1
refactor: nit
moeez96 727b247
test: Add assertion for requested scopes
moeez96 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
36 changes: 36 additions & 0 deletions
36
...ore/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
15. Add scope user_id for JWT token | ||
################################### | ||
|
||
Status | ||
------ | ||
|
||
Accepted | ||
|
||
Context | ||
------- | ||
|
||
In Feb 2018, to enable analytics (Segment) from Microfrontends (MFEs), a ``user_id`` claim was added to the JWT token `in this PR<https://github.com/openedx/edx-platform/pull/19765>`__. | ||
|
||
The LMS API `to create authentication tokens`_ is used by external organizations to request a token on behalf of their users, mostly using grant_type ``client_credentials`` in the request. Since ``user_id`` is considered sensitive information, especially when combined with email and username which were already available in the JWT, it was decided to only add the ``user_id`` claim when a ``user_id`` scope was supplied. All MFE JWT cookies, which are known to only be used directly by the user, automatically used the ``user_id`` scope in order to get the required ``user_id`` claim. | ||
|
||
No ADR could be found for the Feb 2018 decision detailed above. | ||
|
||
In June 2019, an `ADR was captured in ecommerce`_ around the requirements to have the LMS user_id available for requests to ecommerce. | ||
|
||
In 2022, the mobile apps switched to using JWTs for authentication. However, these JWTs were missing the ``user_id`` scope and claim required by the ecommerce service. | ||
|
||
.. _to create authentication tokens: https://github.com/openedx/edx-platform/blob/caf8e456e28f9b9a1f5fa7186d3d155112fb75be/openedx/core/djangoapps/oauth_dispatch/urls.py#L14 | ||
.. _ADR was captured in ecommerce: https://github.com/openedx/ecommerce/blob/master/docs/decisions/0004-unique-identifier-for-users.rst | ||
|
||
Decisions | ||
--------- | ||
|
||
- The original decision to add the ``user_id`` claim to the JWT token using the ``user_id`` scope has been captured in the context of this ADR, because no ADR could be found. | ||
- The scope ``user_id`` will be added to all requests having grant_type ``password`` in the API `/oauth2/access_token/`. | ||
|
||
Consequences | ||
------------ | ||
|
||
- The claim ``user_id`` will be present in the JWT token for all requesters who already have access to the login credentials of the user account. | ||
- The ``user_id`` scope will continue to protect other JWT requests that don't require this sensitive information. | ||
- This pattern could potentially be used to clean-up the manually added ``user_id`` scope for oauth clients involved in the social auth flow in the future. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -12,6 +12,7 @@ | |||||||||||||
from edx_toggles.toggles import SettingToggle | ||||||||||||||
from jwt import PyJWK | ||||||||||||||
from jwt.utils import base64url_encode | ||||||||||||||
from oauth2_provider.models import Application | ||||||||||||||
|
||||||||||||||
from common.djangoapps.student.models import UserProfile, anonymous_id_for_user | ||||||||||||||
|
||||||||||||||
|
@@ -79,10 +80,11 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): | |||||||||||||
# .. custom_attribute_name: create_jwt_grant_type | ||||||||||||||
# .. custom_attribute_description: The grant type of the newly created JWT. | ||||||||||||||
set_custom_attribute('create_jwt_grant_type', grant_type) | ||||||||||||||
scopes = _get_updated_scopes(token_dict['scope'].split(' '), grant_type) | ||||||||||||||
|
||||||||||||||
jwt_access_token = _create_jwt( | ||||||||||||||
access_token.user, | ||||||||||||||
scopes=token_dict['scope'].split(' '), | ||||||||||||||
scopes=scopes, | ||||||||||||||
expires_in=jwt_expires_in, | ||||||||||||||
use_asymmetric_key=use_asymmetric_key, | ||||||||||||||
is_restricted=oauth_adapter.is_client_restricted(client), | ||||||||||||||
|
@@ -91,11 +93,16 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): | |||||||||||||
) | ||||||||||||||
|
||||||||||||||
jwt_token_dict = token_dict.copy() | ||||||||||||||
# Note: only "scope" is not overwritten at this point. | ||||||||||||||
# Note: only "refresh_token" is not overwritten at this point. | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's a possible code comment trail if we decide to ignore #2 from this comment: https://github.com/openedx/edx-platform/pull/33455/files#r1372861173
Suggested change
|
||||||||||||||
# At this time, the user_id scope added for grant type password is only added to the | ||||||||||||||
# JWT, and is not added for the DOT access token or refresh token, so we must override | ||||||||||||||
# here. If this inconsistency becomes an issue, then the user_id scope should be | ||||||||||||||
# added earlier with the DOT tokens, and we would no longer need to override "scope". | ||||||||||||||
jwt_token_dict.update({ | ||||||||||||||
"access_token": jwt_access_token, | ||||||||||||||
"token_type": "JWT", | ||||||||||||||
"expires_in": jwt_expires_in, | ||||||||||||||
"scope": ' '.join(scopes), | ||||||||||||||
}) | ||||||||||||||
return jwt_token_dict | ||||||||||||||
|
||||||||||||||
|
@@ -167,9 +174,7 @@ def _create_jwt( | |||||||||||||
else: | ||||||||||||||
increment('create_symmetric_jwt_count') | ||||||||||||||
|
||||||||||||||
# Default scopes should only contain non-privileged data. | ||||||||||||||
# Do not be misled by the fact that `email` and `profile` are default scopes. They | ||||||||||||||
# were included for legacy compatibility, even though they contain privileged data. | ||||||||||||||
# Scopes `email` and `profile` are included for legacy compatibility. | ||||||||||||||
scopes = scopes or ['email', 'profile'] | ||||||||||||||
iat, exp = _compute_time_fields(expires_in) | ||||||||||||||
|
||||||||||||||
|
@@ -285,3 +290,17 @@ def _encode_and_sign(payload, use_asymmetric_key, secret): | |||||||||||||
|
||||||||||||||
jwk = PyJWK(key, algorithm) | ||||||||||||||
return jwt.encode(payload, jwk.key, algorithm=algorithm) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def _get_updated_scopes(scopes, grant_type): | ||||||||||||||
""" | ||||||||||||||
Default scopes should only contain non-privileged data. | ||||||||||||||
Do not be misled by the fact that `email` and `profile` are default scopes. | ||||||||||||||
They were included for legacy compatibility, even though they contain privileged | ||||||||||||||
data. The scope `user_id` must be added for requests with grant_type password. | ||||||||||||||
""" | ||||||||||||||
scopes = scopes or ['email', 'profile'] | ||||||||||||||
|
||||||||||||||
if grant_type == Application.GRANT_PASSWORD and 'user_id' not in scopes: | ||||||||||||||
scopes.append('user_id') | ||||||||||||||
return scopes |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is true that we are now overwriting all response values, we no longer need to copy the dict. We could just have:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refresh_token
is still not overwritten intoken_dict
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional thoughts:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scopes
set to grant types email, profile and user_id, it works in the same fashion and does not error out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #2 would be more accurate/consistent, because the refresh token would match the actual scopes.
That said, I agree that this shouldn't be a blocker, so I added a potential code comment just to capture what is going on.