Introduce a new Permission class to enforce scopes #1
Conversation
| 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 |
There was a problem hiding this comment.
Why is this check needed? Can you just check whether the token is a DOT token?
| # 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']]) |
There was a problem hiding this comment.
Let's namespace the attribute on the request object. Maybe something like: oauth_scopes.filters.
| # 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']]) |
There was a problem hiding this comment.
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:
- Just pass through the filters information as a dictionary value and each endpoint can see the various filters listed.
- Create a new
OAuthScopesFiltersclass 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.
|
|
||
| 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 |
There was a problem hiding this comment.
Do we need to check here whether scopes enforcement is enabled?
| setattr(request,'allowed_user',[jwt_decode_handler(token)['filters']['user']]) | ||
|
|
||
| return has_permission | ||
|
|
There was a problem hiding this comment.
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).
|
@douglashall FYI |
|
|
||
| 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): |
There was a problem hiding this comment.
This class needs to extend DOT's TokenHasScope, otherwise never actually verify the token's scopes.
There was a problem hiding this comment.
Earlier we used TokenHasScope, but its restricted to OAuth2Authentication.
Instead, user getattr from the view to validate the scope.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'] |
There was a problem hiding this comment.
The indentation is not right here.
There was a problem hiding this comment.
Can you pass the decoded token to this function since you already decoded in has_permission?
There was a problem hiding this comment.
fixed the indentation, nice catch. also, passed decoded token
| # Add a new attributes to the Django request which sets an 'content_org' and 'user' filters | ||
| # 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)) |
There was a problem hiding this comment.
I think we wanted to namespace this attribute oauth_scopes.filters.
There was a problem hiding this comment.
not sure whether we can use "."; so used "_" (oauth_scopes_filters).
| return True | ||
|
|
||
| if not float(jwt_decode_handler(token)['version']) <= 1.0: | ||
| return False |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Get the current version "1.0" from the setting file instead of hardcoding.
|
jenkins ok to test |
|
|
||
|
|
||
| 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): |
There was a problem hiding this comment.
This might be better as HasScopedToken.
| filters[each[0]] = [each[1]] | ||
| return filters | ||
|
|
||
| def has_permission(self, request, view): |
There was a problem hiding this comment.
Reminder: check for feature toggle.
|
Create a new pull request against edx-drf-extensions master. |
Introducing JWTRestrictedApplicationPermission
Decision Document Reference here