Skip to content
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

Introduce a new Permission class to enforce scopes #1

Merged
merged 6 commits into from
May 25, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion edx_rest_framework_extensions/permissions.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,68 @@
""" Permission classes. """
from rest_framework.permissions import BasePermission

from provider.oauth2.models import AccessToken as DOPAccessToken
from edx_rest_framework_extensions.utils import jwt_decode_handler

class IsSuperuser(BasePermission):
""" Allows access only to superusers. """

def has_permission(self, request, view):
return request.user and request.user.is_superuser

class JWTRestrictedApplicationPermission(BasePermission):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class needs to extend DOT's TokenHasScope, otherwise never actually verify the token's scopes.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier we used TokenHasScope, but its restricted to OAuth2Authentication.

Instead, user getattr from the view to validate the scope.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be better as HasScopedToken.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"""
This permission class will inspect a request which contains an
OAuth2 access_token. The following business logic is applied:
1) Is the OAuth2 token passed in a legacy django-oauth-provider (DOP) token, if so
all applications connecting via DOP reflect trusted internal applications
2) Is the access_token associated with a RestrictedApplication? If not,
the caller is viewed as a trusted application and the permission check is passed
3) If the access_token is associated with a RestrictedApplication, then:
2a) Get the 'scopes' from the access_token and inspect the passed in 'view'
2b) Inspect the view object for a 'required_scopes' attribute on the view
2c) If there is no 'required_scopes', then FAIL THE REQUEST, since the
view has not declared how to allow RestrictedApplication to access it
2d) If there is a 'required_scopes' attribute on the view object then make
sure that the access_token has that scope on it
2e) If access_token does not contain the 'required_scopes' then fail the request
2f) If all above checks succees, pass the permissions check

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sub-list items for bullet 3 are misnumbered. I wonder if we need the docstring to be this detailed, maybe just let the code speak for itself?

The description here does not match what the code actually does or what it was supposed to do as outlined in the decision record.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comments

"""

def has_permission(self, request, view):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder: check for feature toggle.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"""
Implement the business logic discussed above
"""
restricted_oauth_required = False
if hasattr(view, 'restricted_oauth_required'):
restricted_oauth_required = True

token = request.auth

if not token:
if not restricted_oauth_required:
# If we are not an OAuth2 request - some APIs in Open edX allow for Django Session
# based authentication, then we must pass here and continue with other
# possible authorization checks declared on the API endpoint

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check needed? Can you just check whether the token is a DOT token?

return True
else:
return False

# check to see if token is a DOP token
# if so this represents a client which is implicitly trusted
# (since it is an internal Open edX application)
if isinstance(token, DOPAccessToken):
return True

has_permission = super(JWTRestrictedApplicationPermission, self).has_permission(request, view)
if has_permission:
# Add a new attributes to the Django request which sets an 'content_org' and 'user' filters

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check here whether scopes enforcement is enabled?

# which will be used by the view handlers for course filtering
# based on the rights declared on the RestrictedApplication
if jwt_decode_handler(token).has_key('filters'):
if jwt_decode_handler(token)['filters'].has_key('content_org'):
setattr(request,'allowed_organization',[jwt_decode_handler(token)['filters']['content_org']])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's namespace the attribute on the request object. Maybe something like: oauth_scopes.filters.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass through the type of the filter. Right now the Grades API will support only content_org. But we will need to support user_org and others in the future. 2 choices:

  1. Just pass through the filters information as a dictionary value and each endpoint can see the various filters listed.
  2. Create a new OAuthScopesFilters class that encapsulates the parsing of filters and provides methods for retrieving the required filters. This abstraction then allows us to evolve the filters in the future without updating all endpoints.

if jwt_decode_handler(token)['filters'].has_key('user'):
setattr(request,'allowed_user',[jwt_decode_handler(token)['filters']['user']])

return has_permission

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to verify the version number of the token somewhere. For now, we need to make sure the version number is less than or equal to the defined version number (1).