-
Notifications
You must be signed in to change notification settings - Fork 4.3k
WIP: Restricted OAuth Initial Design & Implementation #18117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e159561
b1176d3
09774d5
fdc9e0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,10 @@ | |
| from rest_framework.exceptions import AuthenticationFailed | ||
| from rest_framework.generics import GenericAPIView, ListAPIView | ||
| from rest_framework.response import Response | ||
|
|
||
| from rest_framework.permissions import IsAuthenticated | ||
| from edx_rest_framework_extensions.permissions import JWTRestrictedApplicationPermission | ||
| from edx_rest_framework_extensions.authentication import JwtAuthentication | ||
| from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser | ||
| from courseware.access import has_access | ||
| from lms.djangoapps.courseware import courses | ||
| from lms.djangoapps.courseware.exceptions import CourseAccessRedirect | ||
|
|
@@ -160,6 +163,18 @@ class UserGradeView(GradeViewMixin, GenericAPIView): | |
| }] | ||
|
|
||
| """ | ||
| authentication_classes = ( | ||
| JwtAuthentication, | ||
| OAuth2AuthenticationAllowInactiveUser, | ||
| ) | ||
| permission_classes = (IsAuthenticated, JWTRestrictedApplicationPermission,) | ||
|
|
||
| # needed for passing JWTRestrictedApplicationPermission checks | ||
| # for RestrictedApplications (only). A RestrictedApplication can | ||
| # only call this method if it is allowed to receive a 'grades:read' | ||
| # scope | ||
| required_scopes = ['grades:read'] | ||
|
|
||
| def get(self, request, course_id): | ||
| """ | ||
| Gets a course progress status. | ||
|
|
@@ -171,6 +186,18 @@ def get(self, request, course_id): | |
| Return: | ||
| A JSON serialized representation of the requesting user's current grade status. | ||
| """ | ||
| # See if the request has an explicit sattr(request, 'allowed_organizations')) | ||
| # which limits which OAuth2 clients can see the courses | ||
| # based on the association with a RestrictedApplication | ||
| if hasattr(request, 'auth') and hasattr(request, 'oauth_scopes_filters'): | ||
| course_key = CourseKey.from_string(course_id) | ||
| if 'content_org' in request.oauth_scopes_filters.keys(): | ||
| if course_key.org not in request.oauth_scopes_filters['content_org']: | ||
| return self.make_error_response( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| developer_message='The OAuth2 RestrictedApplication is not associated with org.', | ||
| error_code='course_org_not_associated_with_calling_application' | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be good in some kind of scope enforcing decorator possibly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly a DRF filter. |
||
|
|
||
| course = self._get_course(course_id, request.user, 'load') | ||
| if isinstance(course, Response): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -498,20 +498,22 @@ | |
| OAUTH2_PROVIDER = { | ||
| 'OAUTH2_VALIDATOR_CLASS': 'openedx.core.djangoapps.oauth_dispatch.dot_overrides.validators.EdxOAuth2Validator', | ||
| 'REFRESH_TOKEN_EXPIRE_SECONDS': 20160, | ||
| 'SCOPES_BACKEND_CLASS':'openedx.core.djangoapps.oauth_dispatch.scopes.DynamicScopes', | ||
| 'SCOPES': { | ||
| 'read': 'Read access', | ||
| 'write': 'Write access', | ||
| 'email': 'Know your email address', | ||
| # conform profile scope message that is presented to end-user | ||
| # to lms/templates/provider/authorize.html. This may be revised later. | ||
| 'profile': 'Know your name and username', | ||
| 'grades:read': 'Retrieve your grades for your enrolled courses', | ||
| 'certificates:read': 'Retrieve your course certificates' | ||
| }, | ||
| 'REQUEST_APPROVAL_PROMPT': 'auto_even_if_expired', | ||
| } | ||
| # This is required for the migrations in oauth_dispatch.models | ||
| # otherwise it fails saying this attribute is not present in Settings | ||
| OAUTH2_PROVIDER_APPLICATION_MODEL = 'oauth2_provider.Application' | ||
|
|
||
| OAUTH2_PROVIDER_APPLICATION_MODEL = 'oauth_dispatch.OauthRestrictedApplication' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will introduce backward compatibility concerns. We will need to provide some way to migrate existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we definitely want to go this route (overriding the application model). We just want to make sure we have a rollout plan for this change in existing environments. It may be as simple as adding a data migration to this PR that copies the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked with our DevOps team. We are good with adding a data migration that copies existing data from the old table to the new. This data migration would be adding after the existing migrations which add the new tables. Could you add that to this PR? |
||
| ################################## TEMPLATE CONFIGURATION ##################################### | ||
| # Mako templating | ||
| import tempfile | ||
|
|
@@ -2402,6 +2404,8 @@ def _make_locale_paths(settings): | |
| "reddit", | ||
| ] | ||
|
|
||
| DEFAULT_JWT_ISSUER = 'test-issuer-1', | ||
| DEFAULT_RESTRICTED_JWT_ISSUER = 'test-issuer-2' | ||
| # JWT Settings | ||
| JWT_AUTH = { | ||
| # TODO Set JWT_SECRET_KEY to a secure value. By default, SECRET_KEY will be used. | ||
|
|
@@ -2416,6 +2420,18 @@ def _make_locale_paths(settings): | |
| 'JWT_DECODE_HANDLER': 'edx_rest_framework_extensions.utils.jwt_decode_handler', | ||
| # Number of seconds before JWT tokens expire | ||
| 'JWT_EXPIRATION': 30, | ||
| 'JWT_ISSUERS': [ | ||
| { | ||
| 'ISSUER':'test-issuer-1', | ||
| 'SECRET_KEY':'test-secret-key-1', | ||
| 'AUDIENCE':'test-audience-1', | ||
| }, | ||
| { | ||
| 'ISSUER':'test-issuer-2', | ||
| 'SECRET_KEY':'test-secret-key-2', | ||
| 'AUDIENCE':'test-audience-2', | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| # The footer URLs dictionary maps social footer names | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| from django.contrib.admin import ModelAdmin, site | ||
| from oauth2_provider import models | ||
|
|
||
| from .models import RestrictedApplication | ||
| from .models import RestrictedApplication, OauthRestrictOrganization, OauthRestrictedApplication | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does |
||
|
|
||
|
|
||
| def reregister(model_class): | ||
|
|
@@ -52,17 +52,6 @@ class DOTRefreshTokenAdmin(ModelAdmin): | |
| search_fields = [u'token', u'user__username', u'access_token__token'] | ||
|
|
||
|
|
||
| @reregister(models.Application) | ||
| class DOTApplicationAdmin(ModelAdmin): | ||
| """ | ||
| Custom Application Admin | ||
| """ | ||
| list_display = [u'name', u'user', u'client_type', u'authorization_grant_type', u'client_id'] | ||
| list_filter = [u'client_type', u'authorization_grant_type'] | ||
| raw_id_fields = [u'user'] | ||
| search_fields = [u'name', u'user__username', u'client_id'] | ||
|
|
||
|
|
||
| @reregister(models.Grant) | ||
| class DOTGrantAdmin(ModelAdmin): | ||
| """ | ||
|
|
@@ -79,7 +68,28 @@ class RestrictedApplicationAdmin(ModelAdmin): | |
| """ | ||
| ModelAdmin for the Restricted Application | ||
| """ | ||
| list_display = [u'application'] | ||
|
|
||
| list_display = [u'application', u'_org_associations'] | ||
|
|
||
| site.register(RestrictedApplication, RestrictedApplicationAdmin) | ||
|
|
||
|
|
||
| @reregister(OauthRestrictedApplication) | ||
| class OauthRestrictedApplicationAdmin(ModelAdmin): | ||
| """ | ||
| ModelAdmin for the Restricted Application | ||
| """ | ||
| list_display = [u'name', u'user', u'client_type', u'authorization_grant_type', u'client_id'] | ||
| list_filter = [u'client_type', u'authorization_grant_type'] | ||
| raw_id_fields = [u'user'] | ||
| search_fields = [u'name', u'user__username', u'client_id'] | ||
|
|
||
|
|
||
| class OauthRestrictOrganizationAdmin(ModelAdmin): | ||
| """ | ||
| ModelAdmin for the Restricted Application | ||
| """ | ||
| list_display = [u'application','organization_type'] | ||
| filter_horizontal = ('_org_associations',) | ||
|
|
||
| site.register(OauthRestrictOrganization, OauthRestrictOrganizationAdmin) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,9 @@ | |
| from oauth2_provider.oauth2_validators import OAuth2Validator | ||
| from pytz import utc | ||
| from ratelimitbackend.backends import RateLimitMixin | ||
|
|
||
| from ..models import RestrictedApplication | ||
|
|
||
| from django.conf import settings | ||
| from ..models import RestrictedApplication, OauthRestrictedApplication | ||
| from openedx.core.djangoapps.oauth_dispatch.utils import is_oauth_scope_enforcement_enabled | ||
|
|
||
| @receiver(pre_save, sender=AccessToken) | ||
| def on_access_token_presave(sender, instance, *args, **kwargs): # pylint: disable=unused-argument | ||
|
|
@@ -25,12 +25,9 @@ def on_access_token_presave(sender, instance, *args, **kwargs): # pylint: disab | |
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to update this docstring. |
||
| We do this as a pre-save hook on the ORM | ||
| """ | ||
|
|
||
| is_application_restricted = RestrictedApplication.objects.filter(application=instance.application).exists() | ||
| if is_application_restricted: | ||
| if not is_oauth_scope_enforcement_enabled(): | ||
| RestrictedApplication.set_access_token_as_expired(instance) | ||
|
|
||
|
|
||
| # TODO: Remove Django 1.11 upgrade shim | ||
| # SHIM: Allow users that are inactive to still authenticate while keeping rate-limiting functionality. | ||
| if django.VERSION < (1, 10): | ||
|
|
@@ -112,8 +109,7 @@ def save_bearer_token(self, token, request, *args, **kwargs): | |
|
|
||
| super(EdxOAuth2Validator, self).save_bearer_token(token, request, *args, **kwargs) | ||
|
|
||
| is_application_restricted = RestrictedApplication.objects.filter(application=request.client).exists() | ||
| if is_application_restricted: | ||
| if not is_oauth_scope_enforcement_enabled(): | ||
| # Since RestrictedApplications will override the DOT defined expiry, so that access_tokens | ||
| # are always expired, we need to re-read the token from the database and then calculate the | ||
| # expires_in (in seconds) from what we stored in the database. This value should be a negative | ||
|
|
@@ -122,7 +118,6 @@ def save_bearer_token(self, token, request, *args, **kwargs): | |
| access_token = AccessToken.objects.get(token=token['access_token']) | ||
| utc_now = datetime.utcnow().replace(tzinfo=utc) | ||
| expires_in = (access_token.expires - utc_now).total_seconds() | ||
|
|
||
| # assert that RestrictedApplications only issue expired tokens | ||
| # blow up processing if we see otherwise | ||
| assert expires_in < 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # -*- coding: utf-8 -*- | ||
| # Generated by Django 1.11.12 on 2018-05-15 17:50 | ||
| from __future__ import unicode_literals | ||
|
|
||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
| import django.db.models.deletion | ||
| import oauth2_provider.generators | ||
| import oauth2_provider.validators | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('organizations', '0006_auto_20171207_0259'), | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| migrations.swappable_dependency(settings.OAUTH2_PROVIDER_APPLICATION_MODEL), | ||
| ('oauth_dispatch', '0001_initial'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='OauthRestrictedApplication', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('client_id', models.CharField(db_index=True, default=oauth2_provider.generators.generate_client_id, max_length=100, unique=True)), | ||
| ('redirect_uris', models.TextField(blank=True, help_text='Allowed URIs list, space separated', validators=[oauth2_provider.validators.validate_uris])), | ||
| ('client_type', models.CharField(choices=[('confidential', 'Confidential'), ('public', 'Public')], max_length=32)), | ||
| ('authorization_grant_type', models.CharField(choices=[('authorization-code', 'Authorization code'), ('implicit', 'Implicit'), ('password', 'Resource owner password-based'), ('client-credentials', 'Client credentials')], max_length=32)), | ||
| ('client_secret', models.CharField(blank=True, db_index=True, default=oauth2_provider.generators.generate_client_secret, max_length=255)), | ||
| ('name', models.CharField(blank=True, max_length=255)), | ||
| ('skip_authorization', models.BooleanField(default=False)), | ||
| ('allowed_scope', models.TextField(blank=True)), | ||
| ('user', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='oauth_dispatch_oauthrestrictedapplication', to=settings.AUTH_USER_MODEL)), | ||
| ], | ||
| options={ | ||
| 'abstract': False, | ||
| }, | ||
| ), | ||
| migrations.CreateModel( | ||
| name='OauthRestrictOrganization', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('organization_type', models.CharField(choices=[(b'content_provider', 'Content Provider'), (b'user_provider', 'User Provider')], default=b'content_provider', max_length=32)), | ||
| ('_org_associations', models.ManyToManyField(to='organizations.Organization')), | ||
| ('application', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.OAUTH2_PROVIDER_APPLICATION_MODEL)), | ||
| ], | ||
| ), | ||
| migrations.AlterField( | ||
| model_name='restrictedapplication', | ||
| name='application', | ||
| field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='restricted_application', to=settings.OAUTH2_PROVIDER_APPLICATION_MODEL), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having a comment about this, consider Clean Code's recommendation to have self-describing code by creating a helper method with a self-describing name, such as "verify_content_organization_filter".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove the comments in the next commit