From fb31afce9d7e3dbe5595dd5e3aad487d4b233caa Mon Sep 17 00:00:00 2001 From: moeez96 Date: Tue, 10 Oct 2023 15:28:34 +0500 Subject: [PATCH 01/12] feat: Add user_id to JWT payload --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index 1d88e835bccd..e040ca6e5b5f 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -187,6 +187,7 @@ def _create_jwt( 'filters': filters or [], 'is_restricted': is_restricted, 'email_verified': user.is_active, + 'user_id': user.id, } payload.update(additional_claims or {}) _update_from_additional_handlers(payload, user, scopes) From ebbe087d1addd38d822ee021985b5d49ddba45ad Mon Sep 17 00:00:00 2001 From: moeez96 Date: Thu, 19 Oct 2023 16:35:52 +0500 Subject: [PATCH 02/12] feat: Add scope user_id for JWT tokens --- .../0015-add-scope-user-id-for-jwt.rst | 30 +++++++++++++++++++ openedx/core/djangoapps/oauth_dispatch/jwt.py | 6 +++- .../oauth_dispatch/tests/test_jwt.py | 3 +- 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst new file mode 100644 index 000000000000..c0f89a69fc86 --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst @@ -0,0 +1,30 @@ +15. Add scope user_id for JWT token +################################### + +Status +------ + +Accepted + +Context +------- + +JWT tokens are expected to have the ``user_id`` claim, which helps services like e-commerce in identifying the user associated with the token. +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. +This API is also used by the mobile apps to authenticate end users, using grant_type ``password`` in the request. + +Since ``user_id`` is considered sensitive information, It is required that the value be exposed via the API/Token only to end users who can access it by other means. +It is also required that the claim ``user_id`` is present in the JWT token for end users to be identified by systems like e-commerce. + +.. _to create authentication tokens: https://github.com/openedx/edx-platform/blob/caf8e456e28f9b9a1f5fa7186d3d155112fb75be/openedx/core/djangoapps/oauth_dispatch/urls.py#L14 + +Decisions +--------- + +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. diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index e040ca6e5b5f..c3fc483cf9b1 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -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 @@ -170,7 +171,11 @@ def _create_jwt( # 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: + scopes.append('user_id') + iat, exp = _compute_time_fields(expires_in) payload = { @@ -187,7 +192,6 @@ def _create_jwt( 'filters': filters or [], 'is_restricted': is_restricted, 'email_verified': user.is_active, - 'user_id': user.id, } payload.update(additional_claims or {}) _update_from_additional_handlers(payload, user, scopes) diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py index abacfe6bee8a..7851158614f6 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py @@ -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): """ @@ -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', ) From 3198db58b1a30ced93df7724042c6d96a2fe715a Mon Sep 17 00:00:00 2001 From: moeez96 Date: Thu, 19 Oct 2023 20:14:04 +0500 Subject: [PATCH 03/12] fix: add unique scope --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index c3fc483cf9b1..ec404c6839c1 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -172,10 +172,7 @@ def _create_jwt( # 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: - scopes.append('user_id') - + scopes = _update_user_id_in_scopes(scopes or ['email', 'profile'], grant_type) iat, exp = _compute_time_fields(expires_in) payload = { @@ -290,3 +287,9 @@ def _encode_and_sign(payload, use_asymmetric_key, secret): jwk = PyJWK(key, algorithm) return jwt.encode(payload, jwk.key, algorithm=algorithm) + + +def _update_user_id_in_scopes(scopes, grant_type): + if grant_type == Application.GRANT_PASSWORD and 'user_id' not in scopes: + scopes.append('user_id') + return scopes From 0ac5b010400c513ba4b58401d14649409a6e67ea Mon Sep 17 00:00:00 2001 From: moeez96 Date: Thu, 19 Oct 2023 20:27:31 +0500 Subject: [PATCH 04/12] refactor: update decision document --- .../0015-add-scope-user-id-for-jwt.rst | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst index c0f89a69fc86..4bd15b651734 100644 --- a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst @@ -9,22 +9,28 @@ Accepted Context ------- -JWT tokens are expected to have the ``user_id`` claim, which helps services like e-commerce in identifying the user associated with the token. -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. -This API is also used by the mobile apps to authenticate end users, using grant_type ``password`` in the request. +In Feb 2018, to enable analytics (Segment) from Microfrontends (MFEs), a ``user_id`` claim was added to the JWT token `in this PR`__. -Since ``user_id`` is considered sensitive information, It is required that the value be exposed via the API/Token only to end users who can access it by other means. -It is also required that the claim ``user_id`` is present in the JWT token for end users to be identified by systems like e-commerce. +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 scope ``user_id`` will be added to all requests having grant_type ``password`` in the API `/oauth2/access_token/`. - +- 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 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. From 8fb0140c79878e5716a4b33327095e799ea89c72 Mon Sep 17 00:00:00 2001 From: moeez96 Date: Fri, 20 Oct 2023 12:38:57 +0500 Subject: [PATCH 05/12] test: Fix tests --- .../oauth_dispatch/tests/test_views.py | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 7bd991ba6051..37029690789c 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -174,16 +174,22 @@ def _test_jwt_access_token(self, client_attr, token_type=None, headers=None, gra expected_default_expires_in = 60 * 60 assert data['expires_in'] == expected_default_expires_in assert data['token_type'] == 'JWT' + expected_scopes = data['scope'].split(' ') + self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - data['scope'].split(' '), + expected_scopes, grant_type=grant_type, should_be_restricted=False, expires_in=expected_default_expires_in, should_be_asymmetric_key=asymmetric_jwt ) + def _update_expected_scopes_with_user_id(self, expected_scopes, grant_type): + if grant_type == 'password' and 'user_id' not in expected_scopes: + expected_scopes.append('user_id') + @ddt.data('dot_app') def test_access_token_fields(self, client_attr): client = getattr(self, client_attr) @@ -258,18 +264,20 @@ def test_restricted_jwt_access_token(self): response = self._post_request(self.user, self.restricted_dot_app, token_type='jwt') assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) - + grant_type = 'password' + expected_scopes = data['scope'].split(' ') + self._update_expected_scopes_with_user_id(expected_scopes, grant_type) assert 'expires_in' in data assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' self.assert_valid_jwt_access_token( data['access_token'], self.user, - data['scope'].split(' '), + expected_scopes, should_be_expired=False, should_be_asymmetric_key=True, should_be_restricted=True, - grant_type='password' + grant_type=grant_type ) def test_restricted_access_token(self): @@ -322,6 +330,7 @@ def test_jwt_access_token_scopes_and_filters(self, grant_type): response = self._post_request(self.user, dot_app, token_type='jwt', scope=scopes) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) + self._update_expected_scopes_with_user_id(scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, @@ -427,14 +436,20 @@ def _test_jwt_access_token(self, client_attr, token_type=None, headers=None, gra assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' + expected_scopes = data['scope'].split(' ') + self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - data['scope'].split(' '), + expected_scopes, grant_type=grant_type, should_be_asymmetric_key=asymmetric_jwt ) + def _update_expected_scopes_with_user_id(self, expected_scopes, grant_type): + if grant_type == 'password' and 'user_id' not in expected_scopes: + expected_scopes.append('user_id') + @ddt.data('dot_app') def test_access_token_exchange_calls_dispatched_view(self, client_attr): client = getattr(self, client_attr) @@ -451,11 +466,14 @@ def test_jwt_access_token_exchange_calls_dispatched_view(self, client_attr): response = self._post_request(self.user, client, token_type='jwt') assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) + grant_type = 'password' + expected_scopes = data['scope'].split(' ') + self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - data['scope'].split(' '), - grant_type='password' + expected_scopes, + grant_type=grant_type ) assert 'expires_in' in data @@ -470,11 +488,14 @@ def test_asymmetric_jwt_access_token_exchange_calls_dispatched_view(self, client response = self._post_request(self.user, client, token_type='jwt', asymmetric_jwt=True) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) + grant_type = 'password' + expected_scopes = data['scope'].split(' ') + self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - data['scope'].split(' '), - grant_type='password', + expected_scopes, + grant_type=grant_type, should_be_asymmetric_key=True ) From 5ce6c4d11956fe59dff67243068ef4540fa4e6ab Mon Sep 17 00:00:00 2001 From: moeez96 Date: Fri, 20 Oct 2023 22:18:00 +0500 Subject: [PATCH 06/12] refactor: Nit --- lms/envs/common.py | 1 + openedx/core/djangoapps/oauth_dispatch/jwt.py | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 0bad10728565..5dbdb09dcfa7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1251,6 +1251,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, diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index ec404c6839c1..c1bc7341b46b 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -168,11 +168,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. - # The scope `user_id` must be added for requests with grant_type password. - scopes = _update_user_id_in_scopes(scopes or ['email', 'profile'], grant_type) + scopes = _get_updated_scopes(scopes, grant_type) iat, exp = _compute_time_fields(expires_in) payload = { @@ -289,7 +285,15 @@ def _encode_and_sign(payload, use_asymmetric_key, secret): return jwt.encode(payload, jwk.key, algorithm=algorithm) -def _update_user_id_in_scopes(scopes, grant_type): +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 From a0bd320ef83b894af90cb6be1bf80df4d34e603e Mon Sep 17 00:00:00 2001 From: moeez96 Date: Tue, 24 Oct 2023 10:30:18 +0500 Subject: [PATCH 07/12] feat: add updated scope to API response --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index c1bc7341b46b..d983daf0f072 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -80,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 = token_dict['scope'].split(' ') 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), @@ -92,11 +93,11 @@ 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. jwt_token_dict.update({ "access_token": jwt_access_token, "token_type": "JWT", "expires_in": jwt_expires_in, + "scope": ' '.join(_get_updated_scopes(scopes, grant_type)), }) return jwt_token_dict From f87fee5bdb1c30e01bd01f4ae365c7f898b88b7b Mon Sep 17 00:00:00 2001 From: moeez96 Date: Wed, 25 Oct 2023 11:21:23 +0500 Subject: [PATCH 08/12] refactor: Simplify code and revert test changes --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 6 ++- .../oauth_dispatch/tests/test_views.py | 40 +++++-------------- 2 files changed, 13 insertions(+), 33 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index d983daf0f072..dc3cc174a5a5 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -81,6 +81,7 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): # .. custom_attribute_description: The grant type of the newly created JWT. set_custom_attribute('create_jwt_grant_type', grant_type) scopes = token_dict['scope'].split(' ') + scopes = _get_updated_scopes(scopes, grant_type) jwt_access_token = _create_jwt( access_token.user, @@ -97,7 +98,7 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): "access_token": jwt_access_token, "token_type": "JWT", "expires_in": jwt_expires_in, - "scope": ' '.join(_get_updated_scopes(scopes, grant_type)), + "scope": ' '.join(scopes), }) return jwt_token_dict @@ -169,7 +170,8 @@ def _create_jwt( else: increment('create_symmetric_jwt_count') - scopes = _get_updated_scopes(scopes, grant_type) + # Scopes `email` and `profile` are included for legacy compatibility. + scopes = scopes or ['email', 'profile'] iat, exp = _compute_time_fields(expires_in) payload = { diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 37029690789c..12aff26a5d6b 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -174,22 +174,16 @@ def _test_jwt_access_token(self, client_attr, token_type=None, headers=None, gra expected_default_expires_in = 60 * 60 assert data['expires_in'] == expected_default_expires_in assert data['token_type'] == 'JWT' - expected_scopes = data['scope'].split(' ') - self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - expected_scopes, + data['scope'].split(' '), grant_type=grant_type, should_be_restricted=False, expires_in=expected_default_expires_in, should_be_asymmetric_key=asymmetric_jwt ) - def _update_expected_scopes_with_user_id(self, expected_scopes, grant_type): - if grant_type == 'password' and 'user_id' not in expected_scopes: - expected_scopes.append('user_id') - @ddt.data('dot_app') def test_access_token_fields(self, client_attr): client = getattr(self, client_attr) @@ -264,20 +258,17 @@ def test_restricted_jwt_access_token(self): response = self._post_request(self.user, self.restricted_dot_app, token_type='jwt') assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) - grant_type = 'password' - expected_scopes = data['scope'].split(' ') - self._update_expected_scopes_with_user_id(expected_scopes, grant_type) assert 'expires_in' in data assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' self.assert_valid_jwt_access_token( data['access_token'], self.user, - expected_scopes, + data['scope'].split(' '), should_be_expired=False, should_be_asymmetric_key=True, should_be_restricted=True, - grant_type=grant_type + grant_type='password' ) def test_restricted_access_token(self): @@ -330,11 +321,10 @@ def test_jwt_access_token_scopes_and_filters(self, grant_type): response = self._post_request(self.user, dot_app, token_type='jwt', scope=scopes) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) - self._update_expected_scopes_with_user_id(scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - scopes, + data['scope'].split(' '), filters=filters, grant_type=grant_type, ) @@ -436,20 +426,14 @@ def _test_jwt_access_token(self, client_attr, token_type=None, headers=None, gra assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' - expected_scopes = data['scope'].split(' ') - self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - expected_scopes, + data['scope'].split(' '), grant_type=grant_type, should_be_asymmetric_key=asymmetric_jwt ) - def _update_expected_scopes_with_user_id(self, expected_scopes, grant_type): - if grant_type == 'password' and 'user_id' not in expected_scopes: - expected_scopes.append('user_id') - @ddt.data('dot_app') def test_access_token_exchange_calls_dispatched_view(self, client_attr): client = getattr(self, client_attr) @@ -466,14 +450,11 @@ def test_jwt_access_token_exchange_calls_dispatched_view(self, client_attr): response = self._post_request(self.user, client, token_type='jwt') assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) - grant_type = 'password' - expected_scopes = data['scope'].split(' ') - self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - expected_scopes, - grant_type=grant_type + data['scope'].split(' '), + grant_type='password' ) assert 'expires_in' in data @@ -488,14 +469,11 @@ def test_asymmetric_jwt_access_token_exchange_calls_dispatched_view(self, client response = self._post_request(self.user, client, token_type='jwt', asymmetric_jwt=True) assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) - grant_type = 'password' - expected_scopes = data['scope'].split(' ') - self._update_expected_scopes_with_user_id(expected_scopes, grant_type) self.assert_valid_jwt_access_token( data['access_token'], self.user, - expected_scopes, - grant_type=grant_type, + data['scope'].split(' '), + grant_type='password', should_be_asymmetric_key=True ) From 5fd92bce9ed9bb05fcf092caf0c83dcd0523a8af Mon Sep 17 00:00:00 2001 From: moeez96 Date: Wed, 25 Oct 2023 11:26:21 +0500 Subject: [PATCH 09/12] test: add missing line --- openedx/core/djangoapps/oauth_dispatch/tests/test_views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 12aff26a5d6b..d316f1770422 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -261,6 +261,7 @@ def test_restricted_jwt_access_token(self): assert 'expires_in' in data assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' + self.assert_valid_jwt_access_token( data['access_token'], self.user, From 91390f6bde18ccb9a396c23151f230fb82275fa8 Mon Sep 17 00:00:00 2001 From: moeez96 Date: Wed, 25 Oct 2023 17:21:52 +0500 Subject: [PATCH 10/12] test: nit --- openedx/core/djangoapps/oauth_dispatch/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index d316f1770422..96425e84e574 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -258,10 +258,10 @@ def test_restricted_jwt_access_token(self): response = self._post_request(self.user, self.restricted_dot_app, token_type='jwt') assert response.status_code == 200 data = json.loads(response.content.decode('utf-8')) + assert 'expires_in' in data assert data['expires_in'] > 0 assert data['token_type'] == 'JWT' - self.assert_valid_jwt_access_token( data['access_token'], self.user, From b0e47d12b93999b61ec5d5be6aae28feacc7e791 Mon Sep 17 00:00:00 2001 From: moeez96 Date: Thu, 26 Oct 2023 14:19:08 +0500 Subject: [PATCH 11/12] refactor: nit --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index dc3cc174a5a5..b9d63334090e 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -80,8 +80,7 @@ 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 = token_dict['scope'].split(' ') - scopes = _get_updated_scopes(scopes, grant_type) + scopes = _get_updated_scopes(token_dict['scope'].split(' '), grant_type) jwt_access_token = _create_jwt( access_token.user, @@ -94,6 +93,7 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): ) jwt_token_dict = token_dict.copy() + # Note: only "refresh_token" is not overwritten at this point. jwt_token_dict.update({ "access_token": jwt_access_token, "token_type": "JWT", From 727b2476997092f954a25db8abede17e32d7c36a Mon Sep 17 00:00:00 2001 From: moeez96 Date: Fri, 27 Oct 2023 15:15:41 +0500 Subject: [PATCH 12/12] test: Add assertion for requested scopes --- openedx/core/djangoapps/oauth_dispatch/jwt.py | 4 ++++ .../core/djangoapps/oauth_dispatch/tests/test_views.py | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/jwt.py b/openedx/core/djangoapps/oauth_dispatch/jwt.py index b9d63334090e..cf13a2259be3 100644 --- a/openedx/core/djangoapps/oauth_dispatch/jwt.py +++ b/openedx/core/djangoapps/oauth_dispatch/jwt.py @@ -94,6 +94,10 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): jwt_token_dict = token_dict.copy() # 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". jwt_token_dict.update({ "access_token": jwt_access_token, "token_type": "JWT", diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 96425e84e574..ef03dab0ac8a 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -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, - data['scope'].split(' '), + scopes_in_response, filters=filters, grant_type=grant_type, )