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 2 commits
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
57 changes: 56 additions & 1 deletion edx_rest_framework_extensions/permissions.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,64 @@
""" 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 _token_filters(self, token):
# get filters list from jwt token and return dict
if jwt_decode_handler(token).has_key('filters'):
filters_list = jwt_decode_handler(token)['filters']

Choose a reason for hiding this comment

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

The indentation is not right here.

Choose a reason for hiding this comment

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

Can you pass the decoded token to this function since you already decoded in has_permission?

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed the indentation, nice catch. also, passed decoded token

filters = {}
for each in filters_list:
each = each.split(':')
if each[0] in filters.keys():
filters[each[0]].append(each[1])
else:
filters[each[0]] = [each[1]]
return filters

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

token = request.auth

# 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

if not float(jwt_decode_handler(token)['version']) <= 1.0:
return False

Choose a reason for hiding this comment

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

@nasthagiri Is this what you were thinking when verifying the token version? I'm not sure when to return True and when to return False.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Get the current version "1.0" from the setting file instead of hardcoding.

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

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
setattr(request, 'filters', self._token_filters(token))

Choose a reason for hiding this comment

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

I think we wanted to namespace this attribute oauth_scopes.filters.

Copy link
Owner Author

Choose a reason for hiding this comment

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

not sure whether we can use "."; so used "_" (oauth_scopes_filters).

return has_permission