From 19753cf74e5d2b5cea3adaed8996a4a1d6e799ae Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 18 Feb 2020 16:31:23 -0500 Subject: [PATCH 01/21] WIP: add third-party-auth scope and usage BOM-1264 --- .../third_party_auth/api/permissions.py | 77 ++++++++++++++++--- .../djangoapps/third_party_auth/api/views.py | 15 +--- 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 | 52 +++++++++++++ ...-scope-and-filter-for-third-party-auth.rst | 24 ++++++ .../how_tos/define_a_new_scope_or_filter.rst | 6 ++ .../docs/how_tos/testing_manually.rst | 4 + .../0008_applicationaccess_filters.py | 21 +++++ .../core/djangoapps/oauth_dispatch/models.py | 21 ++++- openedx/core/lib/api/permissions.py | 5 ++ 12 files changed, 233 insertions(+), 28 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/docs/how_tos/define_a_new_scope_or_filter.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 d6aa96095d68..56ca551aa8da 100644 --- a/common/djangoapps/third_party_auth/api/permissions.py +++ b/common/djangoapps/third_party_auth/api/permissions.py @@ -2,32 +2,91 @@ 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, IsAuthenticated from third_party_auth.models import ProviderApiPermissions +from openedx.core.lib.api.permissions import ApiKeyHeaderPermission + +log = logging.getLogger(__name__) + -class ThirdPartyAuthProviderApiPermission(permissions.BasePermission): +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): + """ + The JWT used to authenticate contains the appropriate tpa_provider + filter for the requested provider. + """ + 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 = ( + C(IsAuthenticated) & + (_NOT_JWT_RESTRICTED_TPA_PERMISSIONS | _JWT_RESTRICTED_TPA_PERMISSIONS) +) diff --git a/common/djangoapps/third_party_auth/api/views.py b/common/djangoapps/third_party_auth/api/views.py index 9360b44941e3..a4f6b1623379 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.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/lms/envs/common.py b/lms/envs/common.py index 514480309ac1..d5b5fb3e4c93 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -585,9 +585,10 @@ '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 1cb45be275ec..0754e7106bfb 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 @@ def get_authorization_filters(self, client): 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 application.access: + 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 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 filters + return filter_set diff --git a/openedx/core/djangoapps/oauth_dispatch/admin.py b/openedx/core/djangoapps/oauth_dispatch/admin.py index bafc136cbc5a..0c503f2c4295 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 000000000000..012a7f83b7b1 --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0011-scope-filter-support.rst @@ -0,0 +1,52 @@ +10. 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. + +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: All filters defined for an application are always included in every JWT access token, regardless of whether or not scopes were requested. + +.. _Transport JWT in HTTP Cookies: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst + + +******** TODO: DO we want to add notes about how to add a new filter or scope in a how to? 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 000000000000..3afbb4f2ead6 --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0012-scope-and-filter-for-third-party-auth.rst @@ -0,0 +1,24 @@ +10. Dealing with Django CSRF Protection in Frontend Apps +-------------------------------------------------------- + +Status +------ + +Accepted + +Context +------- + + +Decisions +--------- + +New scope: tpa:read + +New filter: tpa_provider:saml-ubc + +edx-drf-extensions is where permissions are implemented. + +Consequences +------------ + diff --git a/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst b/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst new file mode 100644 index 000000000000..acec41350535 --- /dev/null +++ b/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst @@ -0,0 +1,6 @@ +Define a New OAuth Scope or Filter +---------------------------------- + +This document explains ... + + 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 038d00b820c8..e747a622fb5d 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 000000000000..55610a7e1e72 --- /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 079ae38e8bf8..5bbe14f8e777 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 Meta: 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/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index 7b0ec63aa983..ebdc1b7e7e7b 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -5,6 +5,7 @@ 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 @@ 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 @@ def has_permission(self, request, view): 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 From 0ff1c8ceb97eeb76a9cedfa928a6c724bc175224 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 19 Feb 2020 11:14:06 -0500 Subject: [PATCH 02/21] fix bug --- openedx/core/djangoapps/oauth_dispatch/adapters/dot.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py index 0754e7106bfb..7e3a723ea61c 100644 --- a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py +++ b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py @@ -99,7 +99,7 @@ def get_authorization_filters(self, client): application = client filter_set = set() - if application.access: + if hasattr(application, 'access'): filter_set.update(application.access.filters) filter_set = self._add_org_relation_filters_to_set(application, filter_set) From cb8fcb54d6c6a8f1fd31325a1c12513a04f57151 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 19 Feb 2020 14:58:02 -0500 Subject: [PATCH 03/21] fix bug and update docs --- .../djangoapps/oauth_dispatch/adapters/dot.py | 2 +- .../decisions/0011-scope-filter-support.rst | 11 ++++--- ...-scope-and-filter-for-third-party-auth.rst | 31 ++++++++++++++++--- .../how_tos/define_a_new_scope_or_filter.rst | 6 ---- 4 files changed, 33 insertions(+), 17 deletions(-) delete mode 100644 openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst diff --git a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py index 7e3a723ea61c..bd6dd41e2446 100644 --- a/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py +++ b/openedx/core/djangoapps/oauth_dispatch/adapters/dot.py @@ -99,7 +99,7 @@ def get_authorization_filters(self, client): application = client filter_set = set() - if hasattr(application, 'access'): + 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) 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 index 012a7f83b7b1..c9e6de91fc25 100644 --- 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 @@ -1,4 +1,4 @@ -10. More General Scope Filter Support +11. More General Scope Filter Support ------------------------------------- Status @@ -31,6 +31,8 @@ Consequences * 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 @@ -44,9 +46,8 @@ This would result in a JWT that contains the following, assuming these two scope ... } -Note: All filters defined for an application are always included in every JWT access token, regardless of whether or not scopes were requested. +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: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst - - -******** TODO: DO we want to add notes about how to add a new filter or scope in a how to? 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 index 3afbb4f2ead6..d6a680bff619 100644 --- 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 @@ -1,5 +1,5 @@ -10. Dealing with Django CSRF Protection in Frontend Apps --------------------------------------------------------- +12. Scope and filter for Third-Party Auth +----------------------------------------- Status ------ @@ -9,16 +9,37 @@ 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 --------- -New scope: tpa:read +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:: -New filter: tpa_provider:saml-ubc + Scope: tpa:read + Filter: tpa_provider: (e.g. tpa_provider:saml-ubc) -edx-drf-extensions is where permissions are implemented. +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 because this permission is currently used by just ``UserMappingView``, and for only a single OAuth Client application in edX's Edge environment at this time. Keeping this permission local to edx-platform keeps this new permission from polluting the more general permission classes available in edx-drf-extensions. + +.. _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. + +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/define_a_new_scope_or_filter.rst b/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst deleted file mode 100644 index acec41350535..000000000000 --- a/openedx/core/djangoapps/oauth_dispatch/docs/how_tos/define_a_new_scope_or_filter.rst +++ /dev/null @@ -1,6 +0,0 @@ -Define a New OAuth Scope or Filter ----------------------------------- - -This document explains ... - - From 4caa0f84f6f9e79eb4d9c0948e23561f075f0db3 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 19 Feb 2020 17:39:28 -0500 Subject: [PATCH 04/21] add permission tests --- .../api/tests/test_permissions.py | 203 +++++++++++++++++- .../third_party_auth/tests/testutil.py | 4 +- 2 files changed, 195 insertions(+), 12 deletions(-) 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 2eab86196b6e..6fc07db9364d 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 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' @@ -25,9 +32,6 @@ 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), (1, 'invalid-provider-id', False), @@ -37,20 +41,199 @@ def setUp(self): ) @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): + authentication_classes = (JwtAuthentication, SessionAuthentication) + permission_classes = (TPA_PERMISSIONS,) + required_scopes = ['tpa:read'] + + def get(self, request, provider_id=None): # pylint: disable=unused-argument + 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=200, + ), + dict( + is_enforced=False, + is_restricted=False, + expected_response=403, + ), + + # restricted + dict( + is_enforced=False, + is_restricted=True, + expected_response=200, + ), + dict( + is_enforced=False, + is_restricted=True, + expected_response=403, + ), + + # **** Enforced **** + # unrestricted + dict( + is_enforced=True, + is_restricted=False, + expected_response=200, + ), + 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, + ), + 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, + ): + 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/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 71243551ea04..763194b6f6ee 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -173,8 +173,8 @@ def verify_user_email(cls, email): 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 From 86c0fd1ab36daf4437b764b40481fd1253cebc75 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 20 Feb 2020 10:28:27 -0500 Subject: [PATCH 05/21] more test fixes --- .../third_party_auth/api/permissions.py | 1 - .../api/tests/test_permissions.py | 24 +++---------------- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/common/djangoapps/third_party_auth/api/permissions.py b/common/djangoapps/third_party_auth/api/permissions.py index 56ca551aa8da..6dcad4f578e6 100644 --- a/common/djangoapps/third_party_auth/api/permissions.py +++ b/common/djangoapps/third_party_auth/api/permissions.py @@ -87,6 +87,5 @@ def has_permission(self, request, view): JwtHasTpaProviderFilterForRequestedProvider ) TPA_PERMISSIONS = ( - C(IsAuthenticated) & (_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 6fc07db9364d..4c0f04cf5efc 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -117,11 +117,6 @@ def test_session_user_fails(self): @ddt.data( # **** Unenforced **** # unrestricted - dict( - is_enforced=False, - is_restricted=False, - expected_response=200, - ), dict( is_enforced=False, is_restricted=False, @@ -129,11 +124,6 @@ def test_session_user_fails(self): ), # restricted - dict( - is_enforced=False, - is_restricted=True, - expected_response=200, - ), dict( is_enforced=False, is_restricted=True, @@ -141,12 +131,7 @@ def test_session_user_fails(self): ), # **** Enforced **** - # unrestricted - dict( - is_enforced=True, - is_restricted=False, - expected_response=200, - ), + # unrestricted (for example, jwt cookies) dict( is_enforced=True, is_restricted=False, @@ -159,11 +144,6 @@ def test_session_user_fails(self): is_restricted=True, expected_response=403, ), - dict( - is_enforced=True, - is_restricted=True, - expected_response=403, - ), ) @ddt.unpack def test_jwt_without_scopes_and_filters( @@ -172,6 +152,8 @@ def test_jwt_without_scopes_and_filters( is_restricted, expected_response, ): + # 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 with patch('edx_rest_framework_extensions.permissions.waffle.switch_is_active') as mock_toggle: mock_toggle.return_value = is_enforced user = self._create_user() From b1357996b4bd412d09d155cfa94ee45207b7f496 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 20 Feb 2020 11:09:41 -0500 Subject: [PATCH 06/21] Fix quality issues. --- common/djangoapps/third_party_auth/api/tests/test_permissions.py | 1 - 1 file changed, 1 deletion(-) 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 4c0f04cf5efc..98eda63a88d0 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -218,4 +218,3 @@ def _assert_jwt_enforced_restricted_case(self, scopes, filters, expected_respons response = self.SomeTpaClassView().dispatch(request, provider_id='some_tpa_provider') self.assertEqual(response.status_code, expected_response) - From 4a2a635175a8dca2dbdad8ea251a0e912b200dd7 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 20 Feb 2020 11:48:39 -0500 Subject: [PATCH 07/21] 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. --- common/djangoapps/third_party_auth/api/tests/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 06974c87c3cc..fb73e05650fb 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -234,8 +234,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 +336,7 @@ def test_user_mappings_only_return_requested_idp_mapping_by_provider_id(self): (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): From f5c7e80f7cfbddd9753b364ead8ca7cd5e7ce2c1 Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 20 Feb 2020 12:00:16 -0500 Subject: [PATCH 08/21] Removing unused import --- common/djangoapps/third_party_auth/api/permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/api/permissions.py b/common/djangoapps/third_party_auth/api/permissions.py index 6dcad4f578e6..e2f31b7cee6a 100644 --- a/common/djangoapps/third_party_auth/api/permissions.py +++ b/common/djangoapps/third_party_auth/api/permissions.py @@ -13,7 +13,7 @@ NotJwtRestrictedApplication ) from rest_condition import C -from rest_framework.permissions import BasePermission, IsAuthenticated +from rest_framework.permissions import BasePermission from third_party_auth.models import ProviderApiPermissions from openedx.core.lib.api.permissions import ApiKeyHeaderPermission From 241d8d908ee3549de19a00fc6decbee8bf41781d Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 20 Feb 2020 11:52:08 -0500 Subject: [PATCH 09/21] doc updates --- common/djangoapps/third_party_auth/api/permissions.py | 4 ++-- .../decisions/0012-scope-and-filter-for-third-party-auth.rst | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/third_party_auth/api/permissions.py b/common/djangoapps/third_party_auth/api/permissions.py index e2f31b7cee6a..0178fa0a550c 100644 --- a/common/djangoapps/third_party_auth/api/permissions.py +++ b/common/djangoapps/third_party_auth/api/permissions.py @@ -49,8 +49,8 @@ def has_permission(self, request, view): class JwtHasTpaProviderFilterForRequestedProvider(BasePermission): """ - The JWT used to authenticate contains the appropriate tpa_provider - filter for the requested provider. + 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.' 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 index d6a680bff619..754c9455cf73 100644 --- 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 @@ -31,7 +31,7 @@ The new scope and filter are:: 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 because this permission is currently used by just ``UserMappingView``, and for only a single OAuth Client application in edX's Edge environment at this time. Keeping this permission local to edx-platform keeps this new permission from polluting the more general permission classes available 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. .. _JwtHasScope: https://github.com/edx/edx-drf-extensions/blob/64f831d715d14dc2db5a1046201ff14e92fa7c9f/edx_rest_framework_extensions/permissions.py#L70 From 1d4e3819b1c849d05f64b01c0a6531c60936aabc Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 20 Feb 2020 12:28:28 -0500 Subject: [PATCH 10/21] quality fixes --- .../third_party_auth/api/tests/test_permissions.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 98eda63a88d0..05ea2dcf0c6d 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -28,7 +28,7 @@ @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): + def setUp(self): # pylint: disable=arguments-differ """ Create users and oauth client for use in the tests """ super(ThirdPartyAuthApiPermissionTest, self).setUp() @@ -71,11 +71,12 @@ 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): # pylint: disable=unused-argument + def get(self, request, provider_id=None): return Response(data="Success") def _create_user(self, is_superuser=False): @@ -152,8 +153,10 @@ def test_jwt_without_scopes_and_filters( 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 + # See https://github.com/edx/edx-drf-extensions/blob/609e1dbaa98f476b36e50143de97732f2f6a9b4f/edx_rest_framework_extensions/config.py#L5 pylint + # 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() From 88d34fdf28816e7e4b2e356b131a788f29b27b98 Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 20 Feb 2020 12:30:16 -0500 Subject: [PATCH 11/21] undo type --- .../djangoapps/third_party_auth/api/tests/test_permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 05ea2dcf0c6d..b2768bf90c41 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -155,7 +155,7 @@ def test_jwt_without_scopes_and_filters( ): # 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 + # 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 From 5e265635f452da3bc5a589e7f2bbf7c042e18ee5 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 20 Feb 2020 12:32:43 -0500 Subject: [PATCH 12/21] more doc fixes --- .../docs/decisions/0011-scope-filter-support.rst | 2 +- .../decisions/0012-scope-and-filter-for-third-party-auth.rst | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) 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 index c9e6de91fc25..ad33460705b9 100644 --- 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 @@ -50,4 +50,4 @@ Note: Every JWT access token created using a given OAuth Application will includ .. _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: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst +.. _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 index 754c9455cf73..5fdb7933ec96 100644 --- 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 @@ -33,6 +33,7 @@ The scope can be protected using the already existing `JwtHasScope`_ DRF permiss 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 @@ -42,4 +43,4 @@ Consequences * 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. -https://github.com/edx/edx-drf-extensions/blob/64f831d715d14dc2db5a1046201ff14e92fa7c9f/edx_rest_framework_extensions/permissions.py#L171 +.. _JWT_RESTRICTED_APPLICATION_OR_USER_ACCESS: https://github.com/edx/edx-drf-extensions/blob/64f831d715d14dc2db5a1046201ff14e92fa7c9f/edx_rest_framework_extensions/permissions.py#L171 From 45ed0638af0f10335ed638d0db5478739078dee4 Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 20 Feb 2020 12:56:18 -0500 Subject: [PATCH 13/21] Correcting location of pylint disable --- .../djangoapps/third_party_auth/api/tests/test_permissions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 b2768bf90c41..b0244ce7faf3 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -28,9 +28,9 @@ @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): # pylint: disable=arguments-differ + def setUp(self): """ Create users and oauth client for use in the tests """ - super(ThirdPartyAuthApiPermissionTest, self).setUp() + super(ThirdPartyAuthApiPermissionTest, self).setUp() # pylint: disable=arguments-differ @ddt.data( (1, PROVIDER_ID_TESTSHIB, True), From f59fb09fe94cb8005166c30473178db028a7d71d Mon Sep 17 00:00:00 2001 From: Manjinder Singh <49171515+jinder1s@users.noreply.github.com> Date: Thu, 20 Feb 2020 13:05:05 -0500 Subject: [PATCH 14/21] Removing unnecessary setup function --- .../djangoapps/third_party_auth/api/tests/test_permissions.py | 3 --- 1 file changed, 3 deletions(-) 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 b0244ce7faf3..8011589c8a97 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_permissions.py +++ b/common/djangoapps/third_party_auth/api/tests/test_permissions.py @@ -28,9 +28,6 @@ @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() # pylint: disable=arguments-differ @ddt.data( (1, PROVIDER_ID_TESTSHIB, True), From 6d3f91f55401e851b0e5bd7c30a23191e5a9ef6d Mon Sep 17 00:00:00 2001 From: jinder1s Date: Thu, 20 Feb 2020 14:48:04 -0500 Subject: [PATCH 15/21] Added test to make sure having permissions gives access to user mapping api --- .../third_party_auth/api/tests/test_views.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 fb73e05650fb..2dcdc1becfdd 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,7 @@ 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' @@ -346,6 +347,19 @@ def test_user_mapping_permission_logic(self, api_key_permission, token_permissio 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(JwtRestrictedApplication, 'has_permission', return_value=has_permission): + with patch.object(JwtHasScope, 'has_permission', return_value=has_permission): + with patch.object(JwtHasTpaProviderFilterForRequestedProvider, '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) From 82b4c3710dfc1414af8ee46f23671d3be8918b5a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 20 Feb 2020 15:31:47 -0500 Subject: [PATCH 16/21] Test new filters logic. Ensure that the filters we add to the application access model make it into the JWT correctly. --- openedx/core/djangoapps/oauth_dispatch/tests/test_views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py b/openedx/core/djangoapps/oauth_dispatch/tests/test_views.py index 6006aec93319..7cff0be40624 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 @@ def test_jwt_access_token_scopes_and_filters(self, grant_type): 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 @@ def test_jwt_access_token_scopes_and_filters(self, grant_type): ) 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')) From 2720027361df7f9f513f29fadfdd5008e81466e9 Mon Sep 17 00:00:00 2001 From: jinder1s Date: Thu, 20 Feb 2020 15:53:25 -0500 Subject: [PATCH 17/21] quality fix --- common/djangoapps/third_party_auth/api/tests/test_views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 2dcdc1becfdd..490f6d548967 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -349,14 +349,14 @@ def test_user_mapping_permission_logic(self, api_key_permission, token_permissio @ddt.data( (True, 200), - (False, 401) + (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(JwtRestrictedApplication, 'has_permission', return_value=has_permission): with patch.object(JwtHasScope, 'has_permission', return_value=has_permission): - with patch.object(JwtHasTpaProviderFilterForRequestedProvider, 'has_permission', return_value=has_permission): + with patch.object(JwtHasTpaProviderFilterForRequestedProvider, 'has_permission', return_value=has_permission): response = self.client.get(url) self.assertEqual(response.status_code, expect) From dffb4b8ec94bd13a65ef12305be964b478a20ac5 Mon Sep 17 00:00:00 2001 From: jinder1s Date: Fri, 21 Feb 2020 09:49:27 -0500 Subject: [PATCH 18/21] quality fix --- .../third_party_auth/api/tests/test_views.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) 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 490f6d548967..650b89fa64fe 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -23,7 +23,9 @@ 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 +from third_party_auth.api.permissions import (JwtRestrictedApplication, + JwtHasScope, + JwtHasTpaProviderFilterForRequestedProvider) VALID_API_KEY = "i am a key" IDP_SLUG_TESTSHIB = 'testshib' @@ -354,11 +356,11 @@ def test_user_mapping_permission_logic(self, api_key_permission, token_permissio @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(JwtRestrictedApplication, 'has_permission', return_value=has_permission): - with patch.object(JwtHasScope, 'has_permission', return_value=has_permission): - with patch.object(JwtHasTpaProviderFilterForRequestedProvider, 'has_permission', return_value=has_permission): - response = self.client.get(url) - self.assertEqual(response.status_code, expect) + with patch.object(JwtRestrictedApplication, 'has_permission', return_value=has_permission) \ + and patch.object(JwtHasScope, 'has_permission', return_value=has_permission) \ + and patch.object(JwtHasTpaProviderFilterForRequestedProvider, '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 """ From 191379617e1382d1ceb31879467c514136966685 Mon Sep 17 00:00:00 2001 From: jinder1s Date: Fri, 21 Feb 2020 09:54:44 -0500 Subject: [PATCH 19/21] disable pylint warning --- common/djangoapps/third_party_auth/api/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 650b89fa64fe..0004786a972c 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -49,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() From a0f2dc0b60e9e57dc5e0210c2dcf3122ee9f31bc Mon Sep 17 00:00:00 2001 From: jinder1s Date: Fri, 21 Feb 2020 10:25:24 -0500 Subject: [PATCH 20/21] quality fix --- .../third_party_auth/api/tests/test_views.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 0004786a972c..fefae3bc0198 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -24,8 +24,8 @@ 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) + JwtHasScope, + JwtHasTpaProviderFilterForRequestedProvider) VALID_API_KEY = "i am a key" IDP_SLUG_TESTSHIB = 'testshib' @@ -356,11 +356,11 @@ def test_user_mapping_permission_logic(self, api_key_permission, token_permissio @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(JwtRestrictedApplication, 'has_permission', return_value=has_permission) \ - and patch.object(JwtHasScope, 'has_permission', return_value=has_permission) \ - and patch.object(JwtHasTpaProviderFilterForRequestedProvider, 'has_permission', return_value=has_permission): - response = self.client.get(url) - self.assertEqual(response.status_code, expect) + 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 """ From 53dff229b10ca030cdc4a5916b657e6c2aa85091 Mon Sep 17 00:00:00 2001 From: jinder1s Date: Fri, 21 Feb 2020 10:29:25 -0500 Subject: [PATCH 21/21] fix indent prob --- common/djangoapps/third_party_auth/api/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fefae3bc0198..d79b7eac6e23 100644 --- a/common/djangoapps/third_party_auth/api/tests/test_views.py +++ b/common/djangoapps/third_party_auth/api/tests/test_views.py @@ -25,7 +25,7 @@ from third_party_auth.tests.testutil import ThirdPartyAuthTestMixin from third_party_auth.api.permissions import (JwtRestrictedApplication, JwtHasScope, - JwtHasTpaProviderFilterForRequestedProvider) + JwtHasTpaProviderFilterForRequestedProvider) VALID_API_KEY = "i am a key" IDP_SLUG_TESTSHIB = 'testshib'