From 372d2e927c9eec2a5342ab120f84cc290578aa90 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Fri, 21 Feb 2020 11:25:28 -0500 Subject: [PATCH] BOM-1264: add third-party-auth scope and usage (#23135) * WIP: add third-party-auth scope and usage BOM-1264 * Fix tests now that we do permissions in a more standard way. Rather than manually setting the permission class we previously explicitly raised a PermissionDenied exception. The way DRF permissoning logic works, if we use the WWW-Authenticate header in the highest priority auth class, it will return a 401 instead of a 403. * Added test to make sure having permissions gives access to user mapping api * Test new filters logic. Ensure that the filters we add to the application access model make it into the JWT correctly. * quality fix * quality fix * disable pylint warning * quality fix * fix indent prob Co-authored-by: Feanil Patel Co-authored-by: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> --- .../third_party_auth/api/permissions.py | 76 ++++++- .../api/tests/test_permissions.py | 190 ++++++++++++++++-- .../third_party_auth/api/tests/test_views.py | 24 ++- .../djangoapps/third_party_auth/api/views.py | 15 +- .../third_party_auth/tests/testutil.py | 4 +- lms/envs/common.py | 5 +- .../djangoapps/oauth_dispatch/adapters/dot.py | 29 ++- .../core/djangoapps/oauth_dispatch/admin.py | 2 +- .../decisions/0011-scope-filter-support.rst | 53 +++++ ...-scope-and-filter-for-third-party-auth.rst | 46 +++++ .../docs/how_tos/testing_manually.rst | 4 + .../0008_applicationaccess_filters.py | 21 ++ .../core/djangoapps/oauth_dispatch/models.py | 21 +- .../oauth_dispatch/tests/test_views.py | 3 + openedx/core/lib/api/permissions.py | 5 + 15 files changed, 451 insertions(+), 47 deletions(-) create mode 100644 openedx/core/djangoapps/oauth_dispatch/docs/decisions/0011-scope-filter-support.rst create mode 100644 openedx/core/djangoapps/oauth_dispatch/docs/decisions/0012-scope-and-filter-for-third-party-auth.rst create mode 100644 openedx/core/djangoapps/oauth_dispatch/migrations/0008_applicationaccess_filters.py diff --git a/common/djangoapps/third_party_auth/api/permissions.py b/common/djangoapps/third_party_auth/api/permissions.py index d6aa96095d..0178fa0a55 100644 --- a/common/djangoapps/third_party_auth/api/permissions.py +++ b/common/djangoapps/third_party_auth/api/permissions.py @@ -2,32 +2,90 @@ Third party auth API related permissions """ +import logging -from rest_framework import permissions - +from edx_django_utils.monitoring import set_custom_metric +from edx_rest_framework_extensions.auth.jwt.decoder import decode_jwt_filters +from edx_rest_framework_extensions.permissions import ( + IsSuperuser, + JwtHasScope, + JwtRestrictedApplication, + NotJwtRestrictedApplication +) +from rest_condition import C +from rest_framework.permissions import BasePermission from third_party_auth.models import ProviderApiPermissions +from openedx.core.lib.api.permissions import ApiKeyHeaderPermission -class ThirdPartyAuthProviderApiPermission(permissions.BasePermission): +log = logging.getLogger(__name__) + + +class ThirdPartyAuthProviderApiPermission(BasePermission): """ Allow someone to access the view if they have valid OAuth client credential. - """ - def __init__(self, provider_id): - """ Initialize the class with a provider_id """ - self.provider_id = provider_id + Deprecated: Only works for DOP oauth applications. To be removed as part of DOPrecation. + + """ def has_permission(self, request, view): """ Check if the OAuth client associated with auth token in current request has permission to access the information for provider """ - if not request.auth or not self.provider_id: + provider_id = view.kwargs.get('provider_id') + if not request.auth or not provider_id: # doesn't have access token or no provider_id specified return False try: - ProviderApiPermissions.objects.get(client__pk=request.auth.client_id, provider_id=self.provider_id) + ProviderApiPermissions.objects.get(client__pk=request.auth.client_id, provider_id=provider_id) except ProviderApiPermissions.DoesNotExist: return False + set_custom_metric('deprecated_ThirdPartyAuthProviderApiPermission', True) return True + + +class JwtHasTpaProviderFilterForRequestedProvider(BasePermission): + """ + Ensures the JWT used to authenticate contains the appropriate tpa_provider + filter for the provider_id requested in the view. + """ + message = 'JWT missing required tpa_provider filter.' + + def has_permission(self, request, view): + """ + Ensure that the provider_id kwarg provided to the view exists exists + in the tpa_provider filters in the JWT used to authenticate. + """ + provider_id = view.kwargs.get('provider_id') + if not provider_id: + log.warning("Permission JwtHasTpaProviderFilterForRequestedProvider requires a view with provider_id.") + return False + + jwt_filters = decode_jwt_filters(request.auth) + for filter_type, filter_value in jwt_filters: + if filter_type == 'tpa_provider' and filter_value == provider_id: + return True + + log.warning( + "Permission JwtHasTpaProviderFilterForRequestedProvider: required filter tpa_provider:%s was not found.", + provider_id, + ) + return False + + +# TODO: Remove ApiKeyHeaderPermission. Check deprecated_api_key_header custom metric for active usage. +_NOT_JWT_RESTRICTED_TPA_PERMISSIONS = ( + C(NotJwtRestrictedApplication) & + (C(IsSuperuser) | ApiKeyHeaderPermission | ThirdPartyAuthProviderApiPermission) +) +_JWT_RESTRICTED_TPA_PERMISSIONS = ( + C(JwtRestrictedApplication) & + JwtHasScope & + JwtHasTpaProviderFilterForRequestedProvider +) +TPA_PERMISSIONS = ( + (_NOT_JWT_RESTRICTED_TPA_PERMISSIONS | _JWT_RESTRICTED_TPA_PERMISSIONS) +) diff --git a/common/djangoapps/third_party_auth/api/tests/test_permissions.py b/common/djangoapps/third_party_auth/api/tests/test_permissions.py index 2eab86196b..8011589c8a 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -7,10 +7,17 @@ import unittest import ddt from django.conf import settings -from mock import Mock +from django.test import RequestFactory, TestCase +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt +from mock import Mock, patch +from rest_framework.authentication import SessionAuthentication +from rest_framework.response import Response from rest_framework.test import APITestCase +from rest_framework.views import APIView +from student.tests.factories import UserFactory -from third_party_auth.api.permissions import ThirdPartyAuthProviderApiPermission +from third_party_auth.api.permissions import ThirdPartyAuthProviderApiPermission, TPA_PERMISSIONS from third_party_auth.tests.testutil import ThirdPartyAuthTestMixin IDP_SLUG_TESTSHIB = 'testshib' @@ -21,12 +28,6 @@ PROVIDER_ID_TESTSHIB = 'saml-' + IDP_SLUG_TESTSHIB @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class ThirdPartyAuthApiPermissionTest(ThirdPartyAuthTestMixin, APITestCase): """ Tests for third party auth API permission """ - def setUp(self): - """ Create users and oauth client for use in the tests """ - super(ThirdPartyAuthApiPermissionTest, self).setUp() - - client = self.configure_oauth_client() - self.configure_api_permission(client, PROVIDER_ID_TESTSHIB) @ddt.data( (1, PROVIDER_ID_TESTSHIB, True), @@ -37,20 +38,183 @@ class ThirdPartyAuthApiPermissionTest(ThirdPartyAuthTestMixin, APITestCase): ) @ddt.unpack def test_api_permission(self, client_pk, provider_id, expect): + dop_client = self.configure_oauth_dop_client() + self.configure_api_permission(dop_client, PROVIDER_ID_TESTSHIB) + request = Mock() request.auth = Mock() request.auth.client_id = client_pk + view = Mock(kwargs={'provider_id': provider_id}) - result = ThirdPartyAuthProviderApiPermission(provider_id).has_permission(request, None) + result = ThirdPartyAuthProviderApiPermission().has_permission(request, view) self.assertEqual(result, expect) def test_api_permission_unauthorized_client(self): - client = self.configure_oauth_client() - self.configure_api_permission(client, 'saml-anotherprovider') + dop_client = self.configure_oauth_dop_client() + self.configure_api_permission(dop_client, 'saml-anotherprovider') request = Mock() request.auth = Mock() - request.auth.client_id = client.pk + request.auth.client_id = dop_client.pk + view = Mock(kwargs={'provider_id': PROVIDER_ID_TESTSHIB}) - result = ThirdPartyAuthProviderApiPermission(PROVIDER_ID_TESTSHIB).has_permission(request, None) + result = ThirdPartyAuthProviderApiPermission().has_permission(request, view) self.assertEqual(result, False) + + +@ddt.ddt +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +class ThirdPartyAuthPermissionTest(TestCase): + """ Tests for third party auth TPA_PERMISSIONS """ + + class SomeTpaClassView(APIView): + """view used to test TPA_permissions""" + authentication_classes = (JwtAuthentication, SessionAuthentication) + permission_classes = (TPA_PERMISSIONS,) + required_scopes = ['tpa:read'] + + def get(self, request, provider_id=None): + return Response(data="Success") + + def _create_user(self, is_superuser=False): + return UserFactory(username='this_user', is_superuser=is_superuser) + + def _create_request(self, auth_header=None): + url = '/' + extra = dict(HTTP_AUTHORIZATION=auth_header) if auth_header else dict() + return RequestFactory().get(url, **extra) + + def _create_session(self, request, user): + request.user = user + + def _create_jwt_header(self, user, is_restricted=False, scopes=None, filters=None): + token = generate_jwt(user, is_restricted=is_restricted, scopes=scopes, filters=filters) + return "JWT {}".format(token) + + def test_anonymous_fails(self): + request = self._create_request() + response = self.SomeTpaClassView().dispatch(request) + self.assertEqual(response.status_code, 401) + + def test_session_superuser_succeeds(self): + user = self._create_user(is_superuser=True) + request = self._create_request() + self._create_session(request, user) + + response = self.SomeTpaClassView().dispatch(request) + self.assertEqual(response.status_code, 200) + + def test_session_user_fails(self): + user = self._create_user() + request = self._create_request() + self._create_session(request, user) + + response = self.SomeTpaClassView().dispatch(request) + self.assertEqual(response.status_code, 403) + + @ddt.data( + # **** Unenforced **** + # unrestricted + dict( + is_enforced=False, + is_restricted=False, + expected_response=403, + ), + + # restricted + dict( + is_enforced=False, + is_restricted=True, + expected_response=403, + ), + + # **** Enforced **** + # unrestricted (for example, jwt cookies) + dict( + is_enforced=True, + is_restricted=False, + expected_response=403, + ), + + # restricted (note: further test cases for scopes and filters are in tests below) + dict( + is_enforced=True, + is_restricted=True, + expected_response=403, + ), + ) + @ddt.unpack + def test_jwt_without_scopes_and_filters( + self, + is_enforced, + is_restricted, + expected_response, + ): + # pylint: disable=line-too-long + # Note: Unenforced tests can be retired when rollout waffle switch `oauth2.enforce_jwt_scopes` is retired. + # See https://github.com/edx/edx-drf-extensions/blob/609e1dbaa98f476b36e50143de97732f2f6a9b4f/edx_rest_framework_extensions/config.py#L5 + # pylint: enable=line-too-long + with patch('edx_rest_framework_extensions.permissions.waffle.switch_is_active') as mock_toggle: + mock_toggle.return_value = is_enforced + user = self._create_user() + + auth_header = self._create_jwt_header(user, is_restricted=is_restricted) + request = self._create_request( + auth_header=auth_header, + ) + + response = self.SomeTpaClassView().dispatch(request) + self.assertEqual(response.status_code, expected_response) + + @ddt.data( + # valid scopes + dict(scopes=['tpa:read'], expected_response=200), + dict(scopes=['tpa:read', 'another_scope'], expected_response=200), + + # invalid scopes + dict(scopes=[], expected_response=403), + dict(scopes=['another_scope'], expected_response=403), + ) + @ddt.unpack + def test_jwt_scopes(self, scopes, expected_response): + self._assert_jwt_enforced_restricted_case( + scopes=scopes, + filters=['tpa_provider:some_tpa_provider'], + expected_response=expected_response, + ) + + @ddt.data( + # valid provider filters + dict( + filters=['tpa_provider:some_tpa_provider', 'tpa_provider:another_tpa_provider'], + expected_response=200, + ), + + # invalid provider filters + dict( + filters=['tpa_provider:another_tpa_provider'], + expected_response=403, + ), + dict( + filters=[], + expected_response=403, + ), + ) + @ddt.unpack + def test_jwt_org_filters(self, filters, expected_response): + self._assert_jwt_enforced_restricted_case( + scopes=['tpa:read'], + filters=filters, + expected_response=expected_response, + ) + + def _assert_jwt_enforced_restricted_case(self, scopes, filters, expected_response): + with patch('edx_rest_framework_extensions.permissions.waffle.switch_is_active') as mock_toggle: + mock_toggle.return_value = True + user = self._create_user() + + auth_header = self._create_jwt_header(user, is_restricted=True, scopes=scopes, filters=filters) + request = self._create_request(auth_header=auth_header) + + response = self.SomeTpaClassView().dispatch(request, provider_id='some_tpa_provider') + self.assertEqual(response.status_code, expected_response) diff --git a/common/djangoapps/third_party_auth/api/tests/test_views.py b/common/djangoapps/third_party_auth/api/tests/test_views.py index 06974c87c3..d79b7eac6e 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -23,6 +23,9 @@ from student.tests.factories import UserFactory from third_party_auth.api.permissions import ThirdPartyAuthProviderApiPermission from third_party_auth.models import ProviderApiPermissions from third_party_auth.tests.testutil import ThirdPartyAuthTestMixin +from third_party_auth.api.permissions import (JwtRestrictedApplication, + JwtHasScope, + JwtHasTpaProviderFilterForRequestedProvider) VALID_API_KEY = "i am a key" IDP_SLUG_TESTSHIB = 'testshib' @@ -46,7 +49,7 @@ def get_mapping_data_by_usernames(usernames): class TpaAPITestCase(ThirdPartyAuthTestMixin, APITestCase): """ Base test class """ - def setUp(self): + def setUp(self): # pylint: disable=arguments-differ """ Create users for use in the tests """ super(TpaAPITestCase, self).setUp() @@ -234,8 +237,8 @@ class UserMappingViewAPITests(TpaAPITestCase): """ @ddt.data( (VALID_API_KEY, PROVIDER_ID_TESTSHIB, 200, get_mapping_data_by_usernames(LINKED_USERS)), - ("i am an invalid key", PROVIDER_ID_TESTSHIB, 403, None), - (None, PROVIDER_ID_TESTSHIB, 403, None), + ("i am an invalid key", PROVIDER_ID_TESTSHIB, 401, None), + (None, PROVIDER_ID_TESTSHIB, 401, None), (VALID_API_KEY, 'non-existing-id', 404, []), ) @ddt.unpack @@ -336,7 +339,7 @@ class UserMappingViewAPITests(TpaAPITestCase): (True, True, 200), (False, True, 200), (True, False, 200), - (False, False, 403) + (False, False, 401) ) @ddt.unpack def test_user_mapping_permission_logic(self, api_key_permission, token_permission, expect): @@ -346,6 +349,19 @@ class UserMappingViewAPITests(TpaAPITestCase): response = self.client.get(url) self.assertEqual(response.status_code, expect) + @ddt.data( + (True, 200), + (False, 401), + ) + @ddt.unpack + def test_list_all_user_mappings_tpa_permission_logic(self, has_permission, expect): + url = reverse('third_party_auth_user_mapping_api', kwargs={'provider_id': PROVIDER_ID_TESTSHIB}) + with patch.object(JwtHasTpaProviderFilterForRequestedProvider, 'has_permission', return_value=has_permission): + with patch.object(JwtRestrictedApplication, 'has_permission', return_value=has_permission): + with patch.object(JwtHasScope, 'has_permission', return_value=has_permission): + response = self.client.get(url) + self.assertEqual(response.status_code, expect) + def _verify_response(self, response, expect_code, expect_result): """ verify the items in data_list exists in response and data_results matches results in response """ self.assertEqual(response.status_code, expect_code) diff --git a/common/djangoapps/third_party_auth/api/views.py b/common/djangoapps/third_party_auth/api/views.py index 9360b44941..a4f6b16233 100644 --- a/common/djangoapps/third_party_auth/api/views.py +++ b/common/djangoapps/third_party_auth/api/views.py @@ -25,7 +25,7 @@ from openedx.core.lib.api.authentication import ( from openedx.core.lib.api.permissions import ApiKeyHeaderPermission from third_party_auth import pipeline from third_party_auth.api import serializers -from third_party_auth.api.permissions import ThirdPartyAuthProviderApiPermission +from third_party_auth.api.permissions import TPA_PERMISSIONS from third_party_auth.provider import Registry @@ -334,9 +334,9 @@ class UserMappingView(ListAPIView): * remote_id: The Id from third party auth provider """ - authentication_classes = ( - JwtAuthentication, BearerAuthentication, - ) + authentication_classes = (JwtAuthentication, BearerAuthentication, ) + permission_classes = (TPA_PERMISSIONS, ) + required_scopes = ['tpa:read'] serializer_class = serializers.UserMappingSerializer provider = None @@ -344,13 +344,6 @@ class UserMappingView(ListAPIView): def get_queryset(self): provider_id = self.kwargs.get('provider_id') - # permission checking. We allow both API_KEY access and OAuth2 client credential access - if not ( - self.request.user.is_superuser or ApiKeyHeaderPermission().has_permission(self.request, self) or - ThirdPartyAuthProviderApiPermission(provider_id).has_permission(self.request, self) - ): - raise exceptions.PermissionDenied() - # provider existence checking self.provider = Registry.get(provider_id) if not self.provider: diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 71243551ea..763194b6f6 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -173,8 +173,8 @@ class ThirdPartyAuthTestMixin(object): user.save() @staticmethod - def configure_oauth_client(): - """ Configure a oauth client for testing """ + def configure_oauth_dop_client(): + """ Configure an oauth DOP client for testing """ return OAuth2Client.objects.create(client_type=constants.CONFIDENTIAL) @staticmethod diff --git a/lms/envs/common.py b/lms/envs/common.py index 6f3b4ed5b2..75b24a142c 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -598,9 +598,10 @@ OAUTH2_PROVIDER = { 'REFRESH_TOKEN_EXPIRE_SECONDS': 7776000, 'SCOPES_BACKEND_CLASS': 'openedx.core.djangoapps.oauth_dispatch.scopes.ApplicationModelScopes', 'SCOPES': dict(OAUTH2_DEFAULT_SCOPES, **{ - 'user_id': _('Retrieve your user identifier'), - 'grades:read': _('Retrieve your grades for your enrolled courses'), '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': _('Know your user identifier'), }), 'DEFAULT_SCOPES': OAUTH2_DEFAULT_SCOPES, 'REQUEST_APPROVAL_PROMPT': 'auto_even_if_expired', diff --git a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py index 1cb45be275..bd6dd41e24 100644 --- a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py +++ b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py @@ -2,7 +2,7 @@ Adapter to isolate django-oauth-toolkit dependencies """ - +from edx_django_utils.monitoring import set_custom_metric from oauth2_provider import models from openedx.core.djangoapps.oauth_dispatch.models import RestrictedApplication @@ -97,13 +97,34 @@ class DOTAdapter(object): Get the authorization filters for the given client application. """ application = client - filters = [org_relation.to_jwt_filter_claim() for org_relation in application.organizations.all()] + + filter_set = set() + if hasattr(application, 'access') and application.access.filters: + filter_set.update(application.access.filters) + filter_set = self._add_org_relation_filters_to_set(application, filter_set) # Allow applications configured with the client credentials grant type to access # data for all users. This will enable these applications to fetch data in bulk. # Applications configured with all other grant types should only have access # to data for the request user. if application.authorization_grant_type != application.GRANT_CLIENT_CREDENTIALS: - filters.append(self.FILTER_USER_ME) + filter_set.add(self.FILTER_USER_ME) - return filters + return list(filter_set) + + def _add_org_relation_filters_to_set(self, application, filter_set): + """ + Adds Organization related filters to the filter_set. + + TODO: BOM-1292: Retire Application Organizations once all filters have been migrated + to Application Access. When retiring, this entire function can be deleted. + + """ + filter_set_before_orgs = filter_set.copy() + filter_set.update([org_relation.to_jwt_filter_claim() for org_relation in application.organizations.all()]) + + set_custom_metric('filter_set_before_orgs', list(filter_set_before_orgs)) + set_custom_metric('filter_set_after_orgs', list(filter_set)) + set_custom_metric('filter_set_difference', list(filter_set.difference(filter_set_before_orgs))) + + return filter_set diff --git a/openedx/core/djangoapps/oauth_dispatch/admin.py b/openedx/core/djangoapps/oauth_dispatch/admin.py index bafc136cbc..0c503f2c42 100644 --- a/openedx/core/djangoapps/oauth_dispatch/admin.py +++ b/openedx/core/djangoapps/oauth_dispatch/admin.py @@ -80,7 +80,7 @@ class ApplicationAccessAdmin(ModelAdmin): """ ModelAdmin for ApplicationAccess """ - list_display = [u'application', u'scopes'] + list_display = ['application', 'scopes', 'filters'] class ApplicationOrganizationAdmin(ModelAdmin): diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0011-scope-filter-support.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0011-scope-filter-support.rst new file mode 100644 index 0000000000..ad33460705 --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0011-scope-filter-support.rst @@ -0,0 +1,53 @@ +11. More General Scope Filter Support +------------------------------------- + +Status +------ + +Accepted + +Context +------- + +For background, please see: + +* `Include Organizations in Tokens`_, where we decided to include a `content_org` filter in JWT tokens. + +The implementation of the `content_org` filter included a new model for relating OAuth Applications and Organizations. This design made it difficult to add new types of filters, especially if they weren't tied to organizations. + +Decisions +--------- + +#. **Add ApplicationAccess filters** Add a ``filters`` field to the ApplicationAccess model to more quickly allow for new filter types. + +#. **Remove ApplicationOrganization** Deprecate and remove the ApplicationOrganization model which could only handle a very small subset of filters. + +Consequences +------------ + +* Adding the `filters` field to the ApplicationAccess model allows for a simpler design with the following benefits: + + * This enables filters, which typically have some relationship to scopes, to be defined in the same admin screen. This should make it simpler to define oAuth Applications with proper security. + + * This enables the removal of the separate ApplicationOrganization model, which was more complex to configure and less clear regarding its impact on the JWT. + +*. The new `filters` field must be added to the EdxOAuth2AuthorizationView_ to handle user authorization for OAuth Applications with grant type 'Authorization code'. This work will be done in a future PR detailed in BOM-1291_. + +Using the example from `Include Organizations in Tokens`_, we would now simply use the Application Access admin screen to set:: + + Scopes: grades:read,enrollments:read + Filters: content_org:Microsoft + +This would result in a JWT that contains the following, assuming these two scopes were requested:: + + { + "scopes": ["grades:read", "enrollments:read"], + "filters": ["content_org:Microsoft", "user:me"], + ... + } + +Note: Every JWT access token created using a given OAuth Application will include **all filters** defined for that application. This was also true as of the initial introduction of filters. + +.. _EdxOAuth2AuthorizationView: https://github.com/edx/edx-platform/blob/9cf2f9f298e5e8be3b3abcaadaf0b7a96d0de0df/openedx/core/djangoapps/oauth_dispatch/dot_overrides/views.py#L16 +.. _BOM-1291: https://openedx.atlassian.net/browse/BOM-1291 +.. _Transport JWT in HTTP Cookies: 0007-include-organizations-in-tokens.rst diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0012-scope-and-filter-for-third-party-auth.rst b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0012-scope-and-filter-for-third-party-auth.rst new file mode 100644 index 0000000000..5fdb7933ec --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0012-scope-and-filter-for-third-party-auth.rst @@ -0,0 +1,46 @@ +12. Scope and filter for Third-Party Auth +----------------------------------------- + +Status +------ + +Accepted + +Context +------- + +The permission class ``ThirdPartyAuthProviderApiPermission`` exists to protect a single view, ``UserMappingView``. The permission ensures that the OAuth Client Application used during authentication has a related mapping in the ``ProviderApiPermissions`` model for the ``provider_id`` passed to the view. + +An example call to this view looks like:: + + GET /api/third_party_auth/v0/providers/{provider_id}/users + +The problem is that ``ProviderApiPermissions`` has a foreign-key reference to a django-oauth-provider (DOP) table which is no longer supported as of the decision to `Migrate to Django OAuth Toolkit (DOT)`_. + +.. _Migrate to Django OAuth Toolkit (DOT): 0002-migrate-to-dot.rst + +Decisions +--------- + +A new scope and filter will be introduced to provide this same Third-Party Auth authorization, and taking advantage of the `More General Scope Filter Support`_ decision. + +The new scope and filter are:: + + Scope: tpa:read + Filter: tpa_provider: (e.g. tpa_provider:saml-ubc) + +The scope can be protected using the already existing `JwtHasScope`_ DRF permission class in edx-drf-extensions. + +The new filter permission class, ``JwtHasTpaProviderFilterForRequestedProvider``, will be implemented in edx-platform to start because it is only used by an edx-platform view, ``UserMappingView``. Additionally, the permission class is used in conjunction with other legacy permissions and it is simpler to keep all the tests together. + +.. _More General Scope Filter Support: 0011-scope-filter-support.rst +.. _JwtHasScope: https://github.com/edx/edx-drf-extensions/blob/64f831d715d14dc2db5a1046201ff14e92fa7c9f/edx_rest_framework_extensions/permissions.py#L70 + +Consequences +------------ + +* The django-oauth-provider related model ``ProviderApiPermissions`` can be retired without adding a new model, simplifying our OAuth story. + +* The complicated method of handling compound permissions, like `JWT_RESTRICTED_APPLICATION_OR_USER_ACCESS`_ from edx-drf-extensions, needs to be duplicated in edx-platform to properly handle Restricted Applications and ``JwtHasTpaProviderFilterForRequestedProvider``. Simplifying this design is being left to a later decision. + +.. _JWT_RESTRICTED_APPLICATION_OR_USER_ACCESS: https://github.com/edx/edx-drf-extensions/blob/64f831d715d14dc2db5a1046201ff14e92fa7c9f/edx_rest_framework_extensions/permissions.py#L171 diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst b/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst index 038d00b820..e747a622fb 100644 --- a/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst +++ b/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/testing_manually.rst @@ -38,6 +38,10 @@ to test other grant types if they are substituted in the appropriate places. iii. Click Save. + iv. If the temporary waffle switch `oauth2.enforce_jwt_scopes`_ is still defined in your codebase, you will need to enable this switch in the LMS under http://localhost:18000/admin/waffle/switch/add/ + +.. _oauth2.enforce_jwt_scopes: https://github.com/edx/edx-drf-extensions/blob/609e1dbaa98f476b36e50143de97732f2f6a9b4f/edx_rest_framework_extensions/config.py#L5-L18 + 3. Create a publicly accessible URL to the LMS if you are testing on devstack. This step is needed to support the redirecting handshake in the Authorization Code protocol from Google's server back to localhost. i. Install `localtunnel`_: diff --git a/openedx/core/djangoapps/oauth_dispatch/migrations/0008_applicationaccess_filters.py b/openedx/core/djangoapps/oauth_dispatch/migrations/0008_applicationaccess_filters.py new file mode 100644 index 0000000000..55610a7e1e --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/migrations/0008_applicationaccess_filters.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.28 on 2020-02-14 21:30 +from __future__ import unicode_literals + +from django.db import migrations, models +import django_mysql.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('oauth_dispatch', '0007_restore_application_id_constraints'), + ] + + operations = [ + migrations.AddField( + model_name='applicationaccess', + name='filters', + field=django_mysql.models.ListCharField(models.CharField(max_length=32), blank=True, help_text='Comma-separated list of filters that this application will be allowed to request.', max_length=825, null=True, size=25), + ), + ] diff --git a/openedx/core/djangoapps/oauth_dispatch/models.py b/openedx/core/djangoapps/oauth_dispatch/models.py index 079ae38e8b..5bbe14f8e7 100644 --- a/openedx/core/djangoapps/oauth_dispatch/models.py +++ b/openedx/core/djangoapps/oauth_dispatch/models.py @@ -65,6 +65,9 @@ class ApplicationAccess(models.Model): """ Specifies access control information for the associated Application. + For usage details, see: + - openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst + .. no_pii: """ @@ -77,6 +80,15 @@ class ApplicationAccess(models.Model): help_text=_('Comma-separated list of scopes that this application will be allowed to request.'), ) + filters = ListCharField( + base_field=models.CharField(max_length=32), + size=25, + max_length=(25 * 33), # 25 * 32 character filters, plus commas + help_text=_('Comma-separated list of filters that this application will be allowed to request.'), + null=True, + blank=True, + ) + class Meta: app_label = 'oauth_dispatch' @@ -84,13 +96,18 @@ class ApplicationAccess(models.Model): def get_scopes(cls, application): return cls.objects.get(application=application).scopes + @classmethod + def get_filters(cls, application): + return cls.objects.get(application=application).filters + def __str__(self): """ Return a unicode representation of this object. """ - return u"{application_name}:{scopes}".format( + return u"{application_name}:{scopes}:{filters}".format( application_name=self.application.name, scopes=self.scopes, + filters=self.filters, ) @@ -102,6 +119,8 @@ class ApplicationOrganization(models.Model): See openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst for the intended use of this model. + Deprecated: Use filters in ApplicationAccess instead. + .. no_pii: """ RELATION_TYPE_CONTENT_ORG = u'content_org' diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 6006aec933..7cff0be406 100644 --- a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py +++ b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py @@ -348,6 +348,7 @@ class TestAccessTokenView(AccessTokenLoginMixin, mixins.AccessTokenMixin, _Dispa dot_app_access = models.ApplicationAccess.objects.create( application=dot_app, scopes=['grades:read'], + filters=['test:filter'], ) models.ApplicationOrganization.objects.create( application=dot_app, @@ -355,6 +356,8 @@ class TestAccessTokenView(AccessTokenLoginMixin, mixins.AccessTokenMixin, _Dispa ) 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) self.assertEqual(response.status_code, 200) data = json.loads(response.content.decode('utf-8')) diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index 7b0ec63aa9..ebdc1b7e7e 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -5,6 +5,7 @@ API library for Django REST Framework permissions-oriented workflows from django.conf import settings from django.http import Http404 +from edx_django_utils.monitoring import set_custom_metric from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from rest_condition import C @@ -18,6 +19,9 @@ from student.roles import CourseInstructorRole, CourseStaffRole class ApiKeyHeaderPermission(permissions.BasePermission): """ Django REST Framework permissions class used to manage API Key integrations + + Deprecated + """ def has_permission(self, request, view): @@ -33,6 +37,7 @@ class ApiKeyHeaderPermission(permissions.BasePermission): audit_log("ApiKeyHeaderPermission used", path=request.path, ip=request.META.get("REMOTE_ADDR")) + set_custom_metric('deprecated_api_key_header', True) return True return False