feat: add user_id in the default scopes if no scopes are requested from payload #34300

justEhmadSaeed · 2024-02-27T11:49:28Z

Description

This change will impact API users with Application Access and grant type of client_credentials.
Defined scopes in the ApplicationAccess were not being utilized if request payload didn't have scope attribute. Only default scopes were being used.
Now user_id will be included in the default scopes if no scopes are requested and user_id is an allowed scope.

Supporting information

Ticket: ENT-8426: Let API users get a user_id in their JWTs

Testing instructions

Create an Application for a user with a grant type of client credentials.
Create an ApplicationAccess record for that Application and add its scope attributes, which are delimited by a comma.
Request access token from the API, and it should include all attributes defined in the Application scopes.

iloveagent57

Ah, this is a good idea! It looks good to me, might be worth running by the arch-bom team, too.

robrap · 2024-03-04T18:28:51Z

openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py

+        client credentials, it should use available scopes as default.
+        """
+        if request.grant_type == 'client_credentials' and not request.scopes:
+            return get_scopes_backend().get_available_scopes(application=request.client, request=request)


Thanks. Some high-level thoughts:

I don't know how this will interact with the email/profile defaults implemented in the JWT code? Whatever change we might make, we need to ensure we have good test coverage.

See feat: Add scope user_id to JWT payload #33455 for related background around the user_id claim. I wish we had a cleaner simpler story for where user_id should not be added. It also includes default scope code, which seems to exist in other locations, so I'm not clear on the best implementation.

It feels like adding all scopes as default would make things less-secure. I'm wondering if we should keep this scoped (pun-intended) down to just user_id? I think we are moving to a place where we are trying to make user_id a default if user_id is an allowable scope.

Here are some specific requests. Please let me know if you have any questions or concerns. I don't think any of this will take very long.

Instead of adding all allowable scopes, only add user_id if it is in the allowable scopes list. (Note: we can always grow the default list in the future if need be, but it will be difficult to impossible to ever shrink it.)

Update the ADR in feat: Add scope user_id to JWT payload #33455 with this addition. You can add a Change History section, like is used in OEPs.

Add a temporary rollout toggle setting. It is easy to get auth stuff wrong, or have unintended consequences, and this will make it simple to turn on/off. We can remove the toggle after all has proven ok.

Add or update unit tests. This includes tests around the JWTs. Having the rollout toggle should make it simple to test that scopes are working as we expect before and after.

@iloveagent57: You can also let me know if you think any of this seems unreasonable.

That all sounds extremely reasonable! Thanks for reviewing this.

For my own clarity on (1) - what are the reasons to not return all scopes to the requesting application when the corresponding ApplicationAccess record says it's ok for that application to have those scopes?

For my own clarity on (1) - what are the reasons to not return all scopes to the requesting application when the corresponding ApplicationAccess record says it's ok for that application to have those scopes?

a. For (1), it's about the principle of least privilege. One could ask why users have to request a scope at all when we could always return a token that can do anything the user can do? That's basically what this proposal would be. Although this is perfectly valid, it would mean that tokens may have more access than a user needs.
b. Related, we don't have a lot of scopes at this time. Sorry although it seemingly isn't a big deal, because this decision would be difficult to reverse, I'd rather add as little as possible as a default.
c. The user_id claim and our needs around it are unique. I'm not sure, but I'm guessing that the system or person requesting the token doesn't care about user_id, and only we care about it. So, we don't want the user to have to think about requesting this scope. Actually, when the user does request a specific scope, I think other defaults are not included unless requested. This could be an issue if we really want user_id on all of these JWTs. Maybe scope isn't the right way to go around this at all? That said, we can iterate on improvements, as long as you get user_id where you need it for now.

sameenfatima78

Would be nice if we could add tests for this newly added logic.

robrap · 2024-03-06T23:02:17Z

openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py

+        If the request payload does not have `scopes` attribute for a grant_type of
+        client credentials, it should use available scopes as default.
+        """
+        if request.grant_type == 'client_credentials' and not request.scopes:


@moeez96: Once this lands, would it be possible to consolidate the password grant code in this same location, or are there reasons why the user_id default code needs to be all over the place? If it can't be consolidated, I wonder where we could best comment about this to help our future selves? In the ADR? In _get_updated_scopes? Other?

@robrap please have a look at the updated logic and let me know if there's anything out of logic.

robrap

Generally looks good. Thank you. Any "nit" comments you can ignore or follow as you wish.

Additionally, there are these remaining comments from the past:

Update the ADR in feat: Add scope user_id to JWT payload #33455 with this addition. You can add a Change History section, like is used in OEPs.

Add a temporary rollout toggle setting. It is easy to get auth stuff wrong, or have unintended consequences, and this will make it simple to turn on/off. We can remove the toggle after all has proven ok.

robrap · 2024-03-12T19:19:24Z

openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py

+        If the request payload does not have `scopes` attribute for a grant_type of
+        client credentials, it should add `user_id` in the default scopes.


Suggested change

If the request payload does not have `scopes` attribute for a grant_type of

client credentials, it should add `user_id` in the default scopes.

Returns the default scopes.

If the request payload does not have `scopes` attribute for a grant_type of

client credentials, add `user_id` as a default scope if it is an allowed scope.

robrap · 2024-03-12T19:21:47Z

openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py

+        'openedx.core.djangoapps.oauth_dispatch.scopes.ApplicationModelScopes.has_user_id_in_application_scopes'
+    )
+    @mock.patch('oauth2_provider.oauth2_validators.OAuth2Validator.get_default_scopes')
+    def test_get_updated_default_scopes(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes):


Nit:

Suggested change

def test_get_updated_default_scopes(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes):

def test_get_default_scopes_with_user_id(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes):

robrap · 2024-03-12T19:24:38Z

openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py

+        default_scopes = ['profile', 'email']
+        mock_get_default_scopes.return_value = default_scopes.copy()
+        mock_has_user_id_in_application_scopes.return_value = True


Nit:

Suggested change

default_scopes = ['profile', 'email']

mock_get_default_scopes.return_value = default_scopes.copy()

mock_has_user_id_in_application_scopes.return_value = True

expected_default_scopes = ['profile', 'email', 'user_id']

mock_get_default_scopes.return_value = ['profile', 'email']

mock_has_user_id_in_application_scopes.return_value = True

robrap · 2024-03-12T19:25:26Z

openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py

+        default_scopes = ['profile', 'email']
+        mock_get_default_scopes.return_value = default_scopes.copy()
+        mock_has_user_id_in_application_scopes.return_value = True


Is there a simple way to do this with a real application, without mocking?

robrap · 2024-03-12T19:26:12Z

openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py

+        """
+        default_scopes = ['profile', 'email']
+        mock_get_default_scopes.return_value = default_scopes.copy()
+        mock_has_user_id_in_application_scopes.return_value = False


Wouldn't this case just work without mocking?

robrap

Thanks. It's looking good.

...angoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst

robrap · 2024-03-15T16:37:37Z

...angoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst

+
+This ADR builds upon ADR 0015: Add Scope user_id for JWT Token. It inherits the context from that ADR, including the initial addition of the user_id claim for analytics and the rationale for using a scope to control its inclusion.
+
+However, this ADR focuses specifically on the enterprise context highlighted in the consequences of ADR 0015. External organizations often utilize the LMS API with the grant_type: client_credentials to request tokens on behalf of their users. These tokens are crucial for enterprise integrations and require access to the user's user_id for proper functionality.


Maybe something like the following addition?: The current implementation requires clients to remember to request using the scope, which is not a trustworthy solution.

robrap · 2024-03-15T16:45:30Z

...angoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst

+This ADR builds upon ADR 0015: Add Scope user_id for JWT Token. It inherits the context from that ADR, including the initial addition of the user_id claim for analytics and the rationale for using a scope to control its inclusion.
+
+However, this ADR focuses specifically on the enterprise context highlighted in the consequences of ADR 0015. External organizations often utilize the LMS API with the grant_type: client_credentials to request tokens on behalf of their users. These tokens are crucial for enterprise integrations and require access to the user's user_id for proper functionality.
+


We could add something like the following:

Based on ADR 5 and ADR 6, it may be that the original purpose of the user_id scope was to protect against leakage of the user_id in the case of some combination of Authorization Grant and Restricted Applications. More investigation would be required to refactor based on this context, but it may be useful for future iterations.

You could replace ADR 5 (and 6) with a more complete title and links, which could even be to readthedocs (see edx-platform README).

https://github.com/openedx/edx-platform/blob/49dcb68a5e6978e29b4256e65c23fe5e2b9f3f6e/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0005-restricted-application-for-SSO.rst

https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0006-enforce-scopes-in-LMS-APIs.rst#3-restricted-applications-receive-unexpired-jwts-signed-with-a-new-key

...angoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst

robrap · 2024-03-15T16:58:45Z

openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py

+        client credentials, add `user_id` as a default scope if it is an allowed scope.
+        """
+        default_scopes = super().get_default_scopes(client_id, request, *args, **kwargs)
+        if settings.FEATURES.get('ENABLE_USER_ID_SCOPE', True):


This doesn't need to live in FEATURES, and could be top-level, but not a huge deal for a temporary toggle.

I would rename to something more specific like ENABLE_DEFAULT_ALLOWABLE_USER_ID_SCOPE.

You could use a SettingToggle. Again, your choice on updating, but that is the best practice.

The toggle should be documented via annotations. See other SettingToggles in edx-platform and https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html.

robrap · 2024-03-15T16:59:38Z

openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py

@@ -77,6 +80,28 @@ def test_inactive_user_validates(self):
        request = self.request_factory.get('/')
        assert self.validator.validate_user('darkhelmet', self.TEST_PASSWORD, client=None, request=request)

+    @mock.patch.dict(settings.FEATURES, ENABLE_USER_ID_SCOPE=True)
+    def test_get_default_scopes_with_user_id(self):


Add tests for the False version as well, to make this really clear.

edx-pipeline-bot · 2024-03-19T10:55:03Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-03-19T11:14:52Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-03-19T11:14:52Z

2U Release Notice: This PR has been deployed to the edX production environment.

feat: add application scopes into default

4a8cc1e

justEhmadSaeed self-assigned this Feb 27, 2024

justEhmadSaeed requested a review from iloveagent57 February 27, 2024 13:40

iloveagent57 approved these changes Mar 4, 2024

View reviewed changes

robrap reviewed Mar 4, 2024

View reviewed changes

sameenfatima78 reviewed Mar 5, 2024

View reviewed changes

robrap reviewed Mar 6, 2024

View reviewed changes

robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 9, 2024

justEhmadSaeed changed the title ~~feat: use application scopes as default if no scopes are requested from payload~~ feat: add user_id in the default scopes if no scopes are requested from payload Mar 12, 2024

justEhmadSaeed force-pushed the asaeed/add-application-scopes-to-default-scopes branch from 147c5cd to f832f91 Compare March 12, 2024 17:38

feat: add user_id in the default scopes

51de2c6

justEhmadSaeed force-pushed the asaeed/add-application-scopes-to-default-scopes branch from f832f91 to 51de2c6 Compare March 12, 2024 17:58

robrap reviewed Mar 12, 2024

View reviewed changes

justEhmadSaeed added 2 commits March 15, 2024 02:16

chore: use feature-flag, remove mocks for tests and add ADR in decisions

14199e5

fix: copy default scopes to prevent mutating the original list

2694f82

robrap reviewed Mar 15, 2024

View reviewed changes

fix: update ADRs and add test cases with ENABLE_USER_ID_SCOPE flag off

5be6ed6

justEhmadSaeed force-pushed the asaeed/add-application-scopes-to-default-scopes branch from 2ff78b5 to 5be6ed6 Compare March 15, 2024 19:56

justEhmadSaeed added 2 commits March 16, 2024 01:05

fix: quality check for trailing whitespace

f07f8f2

docs: add ENABLE_USER_ID_SCOPE feature flag documentation

070582a

justEhmadSaeed force-pushed the asaeed/add-application-scopes-to-default-scopes branch from 6909468 to 070582a Compare March 15, 2024 20:39

justEhmadSaeed added 3 commits March 16, 2024 02:07

fix: toggle annotations for ENABLE_USER_ID_SCOPE

fe1fcc2

fix: toggle_use_cases and public ticket for its removal

c48fbb9

fix: add toggle_target_removal_date annotation

de60471

sameenfatima78 approved these changes Mar 18, 2024

View reviewed changes

justEhmadSaeed merged commit 57970bd into master Mar 19, 2024
67 checks passed

justEhmadSaeed deleted the asaeed/add-application-scopes-to-default-scopes branch March 19, 2024 09:46

MueezKhan246 mentioned this pull request Apr 17, 2024

Removed ENABLE_USER_ID_SCOPE flag from validator #34526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add user_id in the default scopes if no scopes are requested from payload #34300

feat: add user_id in the default scopes if no scopes are requested from payload #34300

justEhmadSaeed commented Feb 27, 2024 •

edited

Loading

iloveagent57 left a comment

robrap Mar 4, 2024

robrap Mar 6, 2024

iloveagent57 Mar 7, 2024

robrap Mar 8, 2024 •

edited

Loading

sameenfatima78 left a comment

robrap Mar 6, 2024

justEhmadSaeed Mar 12, 2024

robrap left a comment

robrap Mar 12, 2024

robrap Mar 12, 2024

robrap Mar 12, 2024

robrap Mar 12, 2024

robrap Mar 12, 2024

robrap left a comment

robrap Mar 15, 2024

robrap Mar 15, 2024

robrap Mar 15, 2024

robrap Mar 15, 2024

edx-pipeline-bot commented Mar 19, 2024

edx-pipeline-bot commented Mar 19, 2024

edx-pipeline-bot commented Mar 19, 2024

		If the request payload does not have `scopes` attribute for a grant_type of
		client credentials, it should add `user_id` in the default scopes.

	def test_get_updated_default_scopes(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes):
	def test_get_default_scopes_with_user_id(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes):


		This ADR builds upon ADR 0015: Add Scope user_id for JWT Token. It inherits the context from that ADR, including the initial addition of the user_id claim for analytics and the rationale for using a scope to control its inclusion.

		However, this ADR focuses specifically on the enterprise context highlighted in the consequences of ADR 0015. External organizations often utilize the LMS API with the grant_type: client_credentials to request tokens on behalf of their users. These tokens are crucial for enterprise integrations and require access to the user's user_id for proper functionality.

feat: add user_id in the default scopes if no scopes are requested from payload #34300

feat: add user_id in the default scopes if no scopes are requested from payload #34300

Conversation

justEhmadSaeed commented Feb 27, 2024 • edited Loading

Description

Supporting information

Testing instructions

iloveagent57 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

sameenfatima78 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-pipeline-bot commented Mar 19, 2024

edx-pipeline-bot commented Mar 19, 2024

edx-pipeline-bot commented Mar 19, 2024

justEhmadSaeed commented Feb 27, 2024 •

edited

Loading

robrap Mar 8, 2024 •

edited

Loading