Skip to content
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 14 commits into from
Oct 30, 2023
1 change: 1 addition & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,7 @@
'certificates:read': _('Retrieve your course certificates'),
'grades:read': _('Retrieve your grades for your enrolled courses'),
'tpa:read': _('Retrieve your third-party authentication username mapping'),
# user_id is added in code as a default scope for JWT cookies and all password grant_type JWTs
'user_id': _('Know your user identifier'),
}),
'DEFAULT_SCOPES': OAUTH2_DEFAULT_SCOPES,
Expand Down
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.
29 changes: 24 additions & 5 deletions openedx/core/djangoapps/oauth_dispatch/jwt.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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),
Expand All @@ -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.
Comment on lines 95 to -94
Copy link
Contributor

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:

jwt_token_dict = {
    ...
}

Copy link
Contributor Author

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 in token_dict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional thoughts:

  1. Can you restore the comment, and mention the refresh_token?
  2. I’m wondering if it would be best if the refresh_token had the extra scope as well, and if we didn’t need to add it here, because it would have been added earlier?
  3. If we called the api with the three password grant scopes explicitly (as a test from the command line), does it work or complain about the user_id request? If it complains, does that indicate where the scope update should happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Comment restored.
  2. The scope is only added in the JWT, it is not added in the access token or refresh token since it's not required there in this use case.
  3. If we call the API with payload having scopes set to grant types email, profile and user_id, it works in the same fashion and does not error out.

Copy link
Contributor

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.

# Note: only "refresh_token" is not overwritten at this point.
Copy link
Contributor

Choose a reason for hiding this comment

The 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
# Note: only "refresh_token" is not overwritten at this point.
# Note: only "refresh_token" is not overwritten at this point.
# 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".

# 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

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
3 changes: 2 additions & 1 deletion openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def setUp(self):
super().setUp()
self.user = UserFactory()
self.default_scopes = ['email', 'profile']
self.default_scopes_password_grant_type = ['email', 'profile', 'user_id']

def _create_client(self, oauth_adapter, client_restricted, grant_type=None):
"""
Expand Down Expand Up @@ -176,7 +177,7 @@ def test_password_grant_type(self):
jwt_token_dict = jwt_api.create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=False)

self.assert_valid_jwt_access_token(
jwt_token_dict["access_token"], self.user, self.default_scopes,
jwt_token_dict["access_token"], self.user, self.default_scopes_password_grant_type,
grant_type='password',
)

Expand Down
10 changes: 7 additions & 3 deletions openedx/core/djangoapps/oauth_dispatch/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,17 +315,21 @@ def test_jwt_access_token_scopes_and_filters(self, grant_type):
scopes=['grades:read'],
filters=['test:filter'],
)
scopes = dot_app_access.scopes
requested_scopes = dot_app_access.scopes
filters = self.dot_adapter.get_authorization_filters(dot_app)
assert 'test:filter' in filters

response = self._post_request(self.user, dot_app, token_type='jwt', scope=scopes)
response = self._post_request(self.user, dot_app, token_type='jwt', scope=requested_scopes)
assert response.status_code == 200
data = json.loads(response.content.decode('utf-8'))
scopes_in_response = data['scope'].split(' ')
for requested_scope in requested_scopes:
assert requested_scope in scopes_in_response

self.assert_valid_jwt_access_token(
data['access_token'],
self.user,
scopes,
scopes_in_response,
filters=filters,
grant_type=grant_type,
)
Expand Down
Loading