WIP: Restricted OAuth Initial Design & Implementation by mettursathish · Pull Request #18117 · openedx/openedx-platform

mettursathish · 2018-05-03T08:25:28Z

Using JWT as OAuth2 Tokens
Enforcing Scopes in LMS APIs
Adding a version number V1 in the OAuth2 token payload
Restricted Applications receive unexpired JWTs, signed with a new key
Associate Available Organizations with Applications
Associate Available Organizations with DOT Applications
Organization Information in OAuth Tokens

Separate PR for the JWTPermissionClass against edx-drf-extensions here

Decision Document Reference here

restricted oauth changes & migration files

openedx-webhooks · 2018-05-03T08:25:41Z

Thanks for the pull request, @mettursathish! I've created OSPR-2385 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

nasthagiri

Thanks for pushing this out. Take a look at my comments and we can discuss synchronously if needed.

nasthagiri · 2018-05-04T17:45:38Z

        """
        now = int(time())
        expires_in = expires_in or self.jwt_auth['JWT_EXPIRATION']
+        filters = {}


Please review https://github.com/edx/edx-platform/pull/18139, which includes doc updates for using "filters" for organizations.

nasthagiri · 2018-05-05T18:47:39Z

+        SessionAuthentication,
+        OAuth2AuthenticationAllowInactiveUser,
+    )
+    permission_classes = (IsAuthenticated, JWTRestrictedApplicationPermission,)


Please see https://github.com/edx/edx-platform/pull/17426
We need to use a common framework to make sure all endpoints enforce scopes. Otherwise, if a developer introduces a new endpoint that supports JwtAuthentication but forgets to add JWTRestrictedApplicationPermission, then malicious users can call that endpoint with a Restricted token without the appropriate scopes.

Once the work in the PR is complete, we should use the view_auth_classes decorator for this view.

nasthagiri · 2018-05-05T18:49:39Z

+    # for RestrictedApplications (only). A RestrictedApplication can
+    # only call this method if it is allowed to receive a 'grades:read'
+    # scope
+    restricted_oauth_required = True


Why is this boolean needed? If OAuth is always needed, then why does this view even add SessionAuthentication in its list?

Reminder: this boolean can now be removed, right?

yes, its removed now.

nasthagiri · 2018-05-05T18:53:12Z

        """
+        # 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


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".

Will remove the comments in the next commit

nasthagiri · 2018-05-05T18:54:27Z

+        # 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, 'allowed_organization'):


Need to check that the organization type is "content_org" and not something else like "user_org".

nasthagiri · 2018-05-05T19:03:14Z

-    is_application_restricted = RestrictedApplication.objects.filter(application=instance.application).exists()
-    if is_application_restricted:
-        RestrictedApplication.set_access_token_as_expired(instance)
+    if settings.FEATURES.get('AUTO_EXPIRE_RESTRICTED_ACCESS_TOKENS', False):


Please use a WaffleSwitch instead since it will allow us to enable/disable dynamically at runtime in case we run into any issues in production.

nasthagiri · 2018-05-05T19:07:49Z

-    is_application_restricted = RestrictedApplication.objects.filter(application=instance.application).exists()
-    if is_application_restricted:
-        RestrictedApplication.set_access_token_as_expired(instance)
+    if settings.FEATURES.get('AUTO_EXPIRE_RESTRICTED_ACCESS_TOKENS', False):


Also, we do not want to enable this as soon as the PR merges. We will want to pro-actively enable this in production - decoupled from deployment of the code. So, the default path should continue to send expired tokens. The waffle switch should then be something like "ENABLE_OAUTH_SCOPE_ENFORCEMENT" or the like.

nasthagiri · 2018-05-05T19:25:26Z

+# IMPORTANT: Do not change this after data has been populated in database
+_DEFAULT_SEPARATOR = ' '

 class RestrictedApplication(models.Model):


As mentioned in associate available scopes with applications, we want to make these associations with the DOT Application. This RestrictedApplication model was intended to be only a temporary model (a short-term hack) that would be eliminated once we fully supported and rolled out scopes enforcement.

Given this, you will want to create a new table for linking DOT applications to their scopes and filters.

Also, let's discuss the following to see whether it much further simplifies the design:

Given that these changes should be applied to DOT Applications, per the design docs, is there a reason why RestrictedApplications should even come into the picture? We can eliminate all the checks related to RestrictedApplications and all the existing complexity around checking for it. We can then keep the RestrictedApplication code as is.

For the Microsoft application's configuration on edx.org, we would simply eliminate the RestrictedApplication binding once we enable enforcing of scopes.

nasthagiri · 2018-05-05T19:26:39Z

+    # this field will be used to implement appropriate data filtering
+    # so that clients of a specific OAuth2 Application will only be
+    # able retrieve datasets that the OAuth2 Application is allowed to retrieve.
+    _org_associations = models.ForeignKey(Organization, default = '')


We also need organization type specified in the model. For example, content_org, user_org, etc.

Should this now be removed since the OauthRestrictOrganization table is now introduced?

nasthagiri · 2018-05-05T19:41:05Z

        request.grant_type = grant_type
        request.user = user
+
+    def validate_scopes(self, client_id, scopes, client, request, *args, **kwargs):


As described in associate available scopes with applications, the more DOT standard way of doing this is to extend the SettingsScope backend and to set the new class as the value for the SCOPES_BACKEND_CLASS setting.

nasthagiri · 2018-05-18T17:52:49Z

Hey @mettursathish Do you have an ETA on when you'll be able to push forward this PR? Thanks.

Associate Available Scopes with Applications Associate Available Organizations with Applications Waffle Switch for oauth2.unexpired_restricted_applications Also, addressed code review feedback's

nasthagiri · 2018-05-24T14:00:26Z

jenkins ok to test

estute · 2018-05-24T14:01:20Z

jenkins ok to test

nasthagiri

I'm still going through this, but here are some initial comments.

nasthagiri · 2018-05-24T14:36:30Z

+    # for RestrictedApplications (only). A RestrictedApplication can
+    # only call this method if it is allowed to receive a 'grades:read'
+    # scope
+    restricted_oauth_required = True


Reminder: this boolean can now be removed, right?

nasthagiri · 2018-05-24T14:37:45Z

+        # 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, 'filters'):


To avoid namespace collisions by other apps storing values on the request object, we should add a namespace prefix to new fields we store on the shared request object. So filters should be prefixed by the app name that stores it.

addressed in the latest commit.

nasthagiri · 2018-05-24T14:39:43Z

+        # based on the association with a RestrictedApplication
+        if hasattr(request, 'auth') and hasattr(request, 'filters'):
+            course_key = CourseKey.from_string(course_id)
+            if course_key.org not in request.filters['content_org']:


Since we are supporting different types of filters, we cannot assume that there's always a content_org filter. So we should check if a content_org filter exists, and if so, we should honor it. If it doesn't exist, we don't worry about filtering by content organization.

addressed in the latest commit, good catch.

nasthagiri · 2018-05-24T14:40:27Z

+        # based on the association with a RestrictedApplication
+        if hasattr(request, 'auth') and hasattr(request, 'filters'):
+            course_key = CourseKey.from_string(course_id)
+            if course_key.org not in request.filters['content_org']:


You also need to check for the user=me filter, right?

This code is for grades v0 and not for bulk grades; code validate current user earlier and throws valid exception. So, we don't need to validate user=me, we can discuss further if needed.

Remainder: In the next PR, we will move the changes to Grades V1

nasthagiri · 2018-05-24T15:15:30Z

 # otherwise it fails saying this attribute is not present in Settings
-OAUTH2_PROVIDER_APPLICATION_MODEL = 'oauth2_provider.Application'
-
+##OAUTH2_PROVIDER_APPLICATION_MODEL = 'oauth2_provider.Application'


Reminder: remove commented out code.

nasthagiri · 2018-05-24T15:50:51Z

 from oauth2_provider import models

-from .models import RestrictedApplication
+from .models import RestrictedApplication, OauthRestrictOrganization, OauthRestrictedApplication


How does OauthRestrictedApplication differ from RestrictedApplication? We need a better name.

nasthagiri · 2018-05-24T15:52:03Z

-    if is_application_restricted:
-        RestrictedApplication.set_access_token_as_expired(instance)
-
+    if not is_oauth_scope_enforcement_enabled():


nasthagiri · 2018-05-24T15:54:59Z

+        related_name='restricted_application'
+    )
+
+    # a space separated list of scopes that this application can request


To discuss: Don't we want the new data model and fields to go in a new table? It doesn't need to be part of this to-be-deprecated RestrictedApplication concept.

cleaned up the old code.

nasthagiri · 2018-05-24T15:57:06Z

+    # this field will be used to implement appropriate data filtering
+    # so that clients of a specific OAuth2 Application will only be
+    # able retrieve datasets that the OAuth2 Application is allowed to retrieve.
+    _org_associations = models.ForeignKey(Organization, default = '')


Should this now be removed since the OauthRestrictOrganization table is now introduced?

nasthagiri · 2018-05-24T15:59:01Z

+        which is Jan. 1, 1970
+        """
+
+        instance.expires = datetime(1970, 1, 1, tzinfo=utc)


Can you explain what this new model is meant to be responsible for? Don't we already have the RestrictedApplication class for the expired tokens?

Based on previous discussion, I thought we are going to remove RestrictedApplication. Hence added expired tokens method in the new model. Now, its reverted back.

douglashall · 2018-05-25T17:20:42Z

+                        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'
+                    )


This would be good in some kind of scope enforcing decorator possibly.

Possibly a DRF filter.

douglashall · 2018-05-25T17:50:53Z

 # 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'


This will introduce backward compatibility concerns. We will need to provide some way to migrate existing Application models to the new ScopedApplication model.

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 Application models to new ScopedApplication models. I just want to verify this is the "right" approach with our DevOps team.

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?

douglashall · 2018-05-25T17:54:07Z

        return access_token.expires == datetime(1970, 1, 1, tzinfo=utc)
+
+
+class OauthRestrictedApplication(AbstractApplication):


Rename to ScopedApplication.

douglashall · 2018-05-25T17:54:28Z

+        return app_scopes.intersection(all_scopes)
+
+
+class OauthRestrictOrganization(models.Model):


Rename to ScopedOrganization.

nasthagiri · 2018-05-25T18:27:29Z

Here's the diagram we drew on the whiteboard today.

nedbat · 2018-06-05T18:24:53Z

jenkins run all

nedbat · 2018-06-05T18:25:02Z

Where do we all stand on this work?

douglashall · 2018-06-05T18:27:49Z

@mettursathish Hi Sathish! I just wanted to give you an update on this. I'm currently working on a PR against your branch here that outlines some of the changes I think we want to make. For me, this was an easier approach than writing things up in comments. I'm hoping to have something for you to look at in the next couple of days. Let me know if you have any objections to this approach.

mettursathish · 2018-06-06T01:07:00Z

@douglashall Yes, it works. Feel free to update the comments. I see already you commented on few things, which will be addressed soon.

@nedbat Two things, (1) We are waiting for the edx-drf-extension library changes to be checked-in (2) There are few comments, which will be addressed as well as migration script that we are working.

Adding org in authorize html file

edx-status-bot · 2018-06-20T07:54:17Z

Your PR has finished running tests. The following contexts failed:

jenkins/a11y
jenkins/lettuce
jenkins/bokchoy
jenkins/python
jenkins/quality

nasthagiri · 2018-06-20T18:26:15Z

@mettursathish The team here is trying to push these changes through this week.
We have broken it up into the following 2 PRs:

nasthagiri · 2018-06-20T18:27:04Z

This ticket contains all the subtasks: https://openedx.atlassian.net/browse/ARCH-99

mettursathish · 2018-06-20T19:48:23Z

Thanks! Appreciate the cooperation!!

nasthagiri · 2018-07-10T14:35:59Z

Closing this PR in favor of:

openedx-webhooks · 2018-07-10T14:36:09Z

@mettursathish Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

WIP: Restricted OAuth Initial Design & Implementation (#275)

e159561

restricted oauth changes & migration files

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels May 3, 2018

nasthagiri reviewed May 6, 2018

View reviewed changes

Implement Associate Available Scopes & Organizations with Applications

b1176d3

Associate Available Scopes with Applications Associate Available Organizations with Applications Waffle Switch for oauth2.unexpired_restricted_applications Also, addressed code review feedback's

nasthagiri requested a review from douglashall May 23, 2018 13:39

nasthagiri reviewed May 24, 2018

View reviewed changes

Addressing the feedback and cleaning the code

09774d5

douglashall reviewed May 25, 2018

View reviewed changes

Adding organization name filter to authorize html

fdc9e0e

Adding org in authorize html file

mettursathish mentioned this pull request Jun 20, 2018

OAuth2 Per-Application Scopes #18317

Closed

nasthagiri closed this Jul 10, 2018

openedx-webhooks added rejected and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 22, 2021

		return access_token.expires == datetime(1970, 1, 1, tzinfo=utc)


		class OauthRestrictedApplication(AbstractApplication):

		return app_scopes.intersection(all_scopes)


		class OauthRestrictOrganization(models.Model):

Uh oh!

Conversation

mettursathish commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented May 3, 2018

Uh oh!

nasthagiri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasthagiri commented May 18, 2018

Uh oh!

nasthagiri commented May 24, 2018

Uh oh!

estute commented May 24, 2018

Uh oh!

nasthagiri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mettursathish commented May 3, 2018 •

edited

Loading