From 14199e5b99238a30a72eeb763d9e3f03b0ceef15 Mon Sep 17 00:00:00 2001 From: Ehmad Saeed Date: Fri, 15 Mar 2024 02:16:43 +0500 Subject: [PATCH] chore: use feature-flag, remove mocks for tests and add ADR in decisions --- ...-scope-user-id-for-jwt-password-grant.rst} | 0 ...ope-user-id-for-jwt-client-credentials.rst | 32 +++++++++++++++++++ .../dot_overrides/validators.py | 12 ++++--- .../tests/test_dot_overrides.py | 30 ++++++----------- 4 files changed, 50 insertions(+), 24 deletions(-) rename openedx/core/djangoapps/oauth_dispatch/docs/decisions/{0015-add-scope-user-id-for-jwt.rst => 0015-add-scope-user-id-for-jwt-password-grant.rst} (100%) create mode 100644 openedx/core/djangoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.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-password-grant.rst similarity index 100% rename from openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt.rst rename to openedx/core/djangoapps/oauth_dispatch/docs/decisions/0015-add-scope-user-id-for-jwt-password-grant.rst diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst new file mode 100644 index 0000000000..234ce2a28b --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0016-add-scope-user-id-for-jwt-client-credentials.rst @@ -0,0 +1,32 @@ +15. Add scope user_id for JWT token +################################### + +Status +------ + +Accepted + +Context +------- + +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. + +Decisions +--------- + +- The scope ``user_id`` will be added to all requests having grant_type ``client_credentials`` in the API `/oauth2/access_token/`, if it is an allowed scope in the DOT Application and the payload request does not have `scopes` attribute in it. + +Consequences +------------ + +- Enterprises will have the flexibility to request the user_id claim in JWT tokens issued through the client_credentials grant. This enables them to integrate seamlessly with LMS functionalities requiring user identification. +- The existing behavior for other grant types (like password) defined in ADR 0015 remains unchanged. +- This pattern will be used to clean-up the manually added ``user_id`` scope for oauth clients using the enterprise public APIs. + +Deferred/Rejected Alternatives +------------------------------ + +- Adding All Default Scopes: Including all scopes by default could potentially expose unnecessary user information. This approach prioritizes security over granular control. +- Default Inclusion with Opt-Out: This alternative explores automatically adding user_id for all cases and introducing an opt-out mechanism for specific scenarios where it's not required. Deferring this option allows for further analysis of potential security implications and user experience trade-offs. diff --git a/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py b/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py index 9f0122a246..f64d2eff51 100644 --- a/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py +++ b/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py @@ -5,6 +5,7 @@ Classes that override default django-oauth-toolkit behavior from datetime import datetime, timedelta +from django.conf import settings from django.contrib.auth import authenticate, get_user_model from django.db.models.signals import pre_save from django.dispatch import receiver @@ -93,13 +94,16 @@ class EdxOAuth2Validator(OAuth2Validator): def get_default_scopes(self, client_id, request, *args, **kwargs): """ + Returns the default scopes. + 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. + 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 request.grant_type == 'client_credentials' and not request.scopes: - if get_scopes_backend().has_user_id_in_application_scopes(application=request.client): - default_scopes.append('user_id') + if settings.FEATURES.get('ENABLE_USER_ID_SCOPE', False): + if request.grant_type == 'client_credentials' and not request.scopes: + if get_scopes_backend().has_user_id_in_application_scopes(application=request.client): + default_scopes.append('user_id') return default_scopes def validate_scopes(self, client_id, scopes, client, request, *args, **kwargs): diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py index 2bc4e92c1f..18cfe4a0b6 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_dot_overrides.py @@ -13,6 +13,7 @@ from django.utils import timezone from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangolib.testing.utils import skip_unless_lms +from openedx.core.djangoapps.oauth_dispatch.tests.factories import ApplicationAccessFactory # oauth_dispatch is not in CMS' INSTALLED_APPS so these imports will error during test collection if settings.ROOT_URLCONF == 'lms.urls': @@ -66,6 +67,7 @@ class CustomValidationTestCase(TestCase): ) self.validator = EdxOAuth2Validator() self.request_factory = RequestFactory() + self.default_scopes = list(settings.OAUTH2_DEFAULT_SCOPES.keys()) def test_active_user_validates(self): assert self.user.is_active @@ -78,39 +80,27 @@ class CustomValidationTestCase(TestCase): request = self.request_factory.get('/') assert self.validator.validate_user('darkhelmet', self.TEST_PASSWORD, client=None, request=request) - @mock.patch( - '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): + @mock.patch.dict(settings.FEATURES, ENABLE_USER_ID_SCOPE=True) + def test_get_default_scopes_with_user_id(self): """ Test that get_default_scopes returns the default scopes plus the user_id scope if it's available. """ - default_scopes = ['profile', 'email'] - mock_get_default_scopes.return_value = default_scopes.copy() - mock_has_user_id_in_application_scopes.return_value = True + application_access = ApplicationAccessFactory(scopes=['user_id']) - request = mock.Mock(grant_type='client_credentials', client=None, scopes=None) + request = mock.Mock(grant_type='client_credentials', client=application_access.application, scopes=None) overriden_default_scopes = self.validator.get_default_scopes(request=request, client_id='client_id') - self.assertEqual(overriden_default_scopes, default_scopes + ['user_id']) + self.assertEqual(overriden_default_scopes, self.default_scopes + ['user_id']) - @mock.patch( - '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_default_scopes(self, mock_get_default_scopes, mock_has_user_id_in_application_scopes): + @mock.patch.dict(settings.FEATURES, ENABLE_USER_ID_SCOPE=True) + def test_get_default_scopes(self): """ Test that get_default_scopes returns the default scopes if user_id scope is not available. """ - default_scopes = ['profile', 'email'] - mock_get_default_scopes.return_value = default_scopes.copy() - mock_has_user_id_in_application_scopes.return_value = False - request = mock.Mock(grant_type='client_credentials', client=None, scopes=None) overriden_default_scopes = self.validator.get_default_scopes(request=request, client_id='client_id') - self.assertEqual(overriden_default_scopes, default_scopes) + self.assertEqual(overriden_default_scopes, self.default_scopes) @skip_unless_lms