fix: Expand DRF permission composition support to JwtRedirectToLoginIfUnauthenticatedMiddleware #528
Conversation
…fUnauthenticatedMiddleware IDAs which enable this middleware should still be able to use DRF permission composition. This PR reinforces the stated promise that this middleware is a no-op for any ViewSet that does not actually use LoginRedirectIfUnauthenticated.
pwnage101
force-pushed
the
pwnage101/redirect-to-login-middleware-supports-perm-composition
branch
from
9980c6d to
38870ea
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances DRF permission composition support within JWT-related middleware, ensuring composite permissions are correctly handled and the JWT authentication middleware no-ops when login-redirection isn’t used.
- Extracted
_iter_included_base_classesto the module level for reuse - Updated both
EnsureJWTAuthSettingsMiddlewareand login-redirect middleware to leverage composed-permission detection - Bumped version to
10.6.1and added a corresponding changelog entry
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| edx_rest_framework_extensions/auth/jwt/middleware.py | Extracted helper and updated permission-check calls for composition |
| edx_rest_framework_extensions/init.py | Updated __version__ to 10.6.1 |
| CHANGELOG.rst | Added [10.6.1] entry describing the permission composition fix |
Comments suppressed due to low confidence (2)
edx_rest_framework_extensions/auth/jwt/middleware.py:38
- Consider adding unit tests for
_iter_included_base_classes, covering both simple and nested composed permissions to verify it yields all expected base classes.
def _iter_included_base_classes(view_permissions):
CHANGELOG.rst:17
- [nitpick] The changelog mentions
JwtRedirectToLoginIfUnauthenticatedMiddlewarebut the code changes affectEnsureJWTAuthSettingsMiddlewareand the login-redirect middleware. Update the entry to accurately reflect both class names or the actual class being modified.
* fix: Expand DRF permission composition support to JwtRedirectToLoginIfUnauthenticatedMiddleware.
|
@awais786 would you consider reviewing? |
|
hm, who can review and has write access? |
robrap
left a comment
robrap
left a comment
There was a problem hiding this comment.
Can we get a unit test that requires your fix to pass?
| 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()) |
There was a problem hiding this comment.
Nit: Not really introduced by you, but above we have:
view_permissions = list(getattr(view_class, 'permission_classes', []))
Is there a reason for the inconsistency?
There was a problem hiding this comment.
It looks like an older version of this code used to iterate over the permission_classes using list.pop():
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.
|
I'm going to pause work on this PR as we've found a workaround by setting |
|
@pwnage101: Would it make sense to create an issue in this repo describing the bug and pointing to this PR as a potential fix? Would you mind taking care of that? It could even get a "good first issue" label, since you handled much of it already. |
|
done: #533 |
|
Thank you. |
4 participants
IDAs which enable this middleware should still be able to use DRF permission composition. This PR reinforces the stated promise that this middleware is a no-op for any ViewSet that does not actually use LoginRedirectIfUnauthenticated.