-
Notifications
You must be signed in to change notification settings - Fork 20
fix: Expand DRF permission composition support to JwtRedirectToLoginIfUnauthenticatedMiddleware #528
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
base: master
Are you sure you want to change the base?
fix: Expand DRF permission composition support to JwtRedirectToLoginIfUnauthenticatedMiddleware #528
Changes from all commits
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 |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| """ edx Django REST Framework extensions. """ | ||
|
|
||
| __version__ = '10.6.0' # pragma: no cover | ||
| __version__ = '10.6.1' # pragma: no cover |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,33 +35,34 @@ | |
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _iter_included_base_classes(view_permissions): | ||
| """ | ||
| Yield all the permissions that are encapsulated in provided view_permissions, directly or as | ||
| a part of DRF's composed permissions. | ||
| """ | ||
| # Not all permissions are classes, some will be OperandHolder | ||
| # objects from DRF. So we have to crawl all those and expand them to see | ||
| # if our target classes are inside the conditionals somewhere. | ||
| for permission in view_permissions: | ||
| # Composition using DRF native support in 3.9+: | ||
| # IsStaff | IsSuperuser -> [IsStaff, IsSuperuser] | ||
| # IsOwner | IsStaff | IsSuperuser -> [IsOwner | IsStaff, IsSuperuser] | ||
| if isinstance(permission, OperandHolder): | ||
| decomposed_permissions = [permission.op1_class, permission.op2_class] | ||
| yield from _iter_included_base_classes(decomposed_permissions) | ||
| elif isinstance(permission, SingleOperandHolder): | ||
| yield permission.op1_class | ||
| else: | ||
| yield permission | ||
|
|
||
|
|
||
| class EnsureJWTAuthSettingsMiddleware(MiddlewareMixin): | ||
| """ | ||
| Django middleware object that ensures the proper Permission classes | ||
| are set on all endpoints that use JWTAuthentication. | ||
| """ | ||
| _required_permission_classes = (NotJwtRestrictedApplication,) | ||
|
|
||
| def _iter_included_base_classes(self, view_permissions): | ||
| """ | ||
| Yield all the permissions that are encapsulated in provided view_permissions, directly or as | ||
| a part of DRF's composed permissions. | ||
| """ | ||
| # Not all permissions are classes, some will be OperandHolder | ||
| # objects from DRF. So we have to crawl all those and expand them to see | ||
| # if our target classes are inside the conditionals somewhere. | ||
| for permission in view_permissions: | ||
| # Composition using DRF native support in 3.9+: | ||
| # IsStaff | IsSuperuser -> [IsStaff, IsSuperuser] | ||
| # IsOwner | IsStaff | IsSuperuser -> [IsOwner | IsStaff, IsSuperuser] | ||
| if isinstance(permission, OperandHolder): | ||
| decomposed_permissions = [permission.op1_class, permission.op2_class] | ||
| yield from self._iter_included_base_classes(decomposed_permissions) | ||
| elif isinstance(permission, SingleOperandHolder): | ||
| yield permission.op1_class | ||
| else: | ||
| yield permission | ||
|
|
||
| def _add_missing_jwt_permission_classes(self, view_class): | ||
| """ | ||
| Adds permissions classes that should exist for Jwt based authentication, | ||
|
|
@@ -71,7 +72,7 @@ def _add_missing_jwt_permission_classes(self, view_class): | |
| view_permissions = list(getattr(view_class, 'permission_classes', [])) | ||
|
|
||
| for perm_class in self._required_permission_classes: | ||
| if not _includes_base_class(self._iter_included_base_classes(view_permissions), perm_class): | ||
| if not _includes_base_class(_iter_included_base_classes(view_permissions), perm_class): | ||
| message = ( | ||
| "The view %s allows Jwt Authentication. The required permission class, %s,", | ||
| " was automatically added." | ||
|
|
@@ -161,8 +162,11 @@ def _check_and_cache_login_required_found(self, view_func): | |
| Checks for LoginRedirectIfUnauthenticated permission and caches the result. | ||
| """ | ||
| view_class = _get_view_class(view_func) | ||
| view_permission_classes = getattr(view_class, 'permission_classes', tuple()) | ||
| is_login_required_found = _includes_base_class(view_permission_classes, LoginRedirectIfUnauthenticated) | ||
| view_permissions = getattr(view_class, 'permission_classes', tuple()) | ||
|
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. Nit: Not really introduced by you, but above we have: Is there a reason for the inconsistency?
Author
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. It looks like an older version of this code used to iterate over the permission_classes using The recursive helper function _iter_included_base_classes appears to support any iterable, so it shouldn't matter whether a list or tuple is passed. That said, I'm happy to make it consistently a list. |
||
| is_login_required_found = _includes_base_class( | ||
| _iter_included_base_classes(view_permissions), | ||
| LoginRedirectIfUnauthenticated, | ||
| ) | ||
| self._get_request_cache()[self._LOGIN_REQUIRED_FOUND_CACHE_KEY] = is_login_required_found | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.