8000 Refactor usage of UserPassesTestMixin by koopmant · Pull Request #3928 · comic/grand-challenge.org · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactor usage of UserPassesTestMixin #3928

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Refactor usage of UserPassesTestMixin #3928

wants to merge 13 commits into from

Conversation

koopmant
Copy link
Contributor

Replace usage of UserPassesTestMixin with AccessMixin and place the test directly in dispatch to allow for subclassing with additional tests.

Closes https://github.com/DIAGNijmegen/rse-grand-challenge-admin/issues/347

@koopmant
Copy link
Contributor Author
koopmant commented Mar 27, 2025 •
edited
Loading

Running into an issue with ConversationCreate which inherits from LoginRequiredMixin. In the old case, the MRO for dispatch() was
LoginRequiredMixin -> UserPassesTestMixin -> ...
With this change it becomes
ConversationCreate -> LoginRequiredMixin -> ...
This is a problem because LoginRequiredMixin places a decorator around super().dispatch() and that would essentially prevent UserPassesTestMixin.dispatch() from being reached if the user is not logged-in, which is desired. With this change we do the test before checking if the user is logged in, but the test is not made for not-logged-in users.

So it seems that if you use LoginRequiredMixin, you're not really supposed update the dispatch() method.

I could:

  • write our own UserPassesTestMixin that allows for inheritance.
  • inside ConversationCreate.dispatch() inspect the response from LoginRequiredMixin.dispatch() and if it returned a redirect to login, return before trying the user test. (Not my preferred solution)
  • something else...?

@koopmant koopmant self-assigned this Mar 27, 2025
@koopmant
Copy link
Contributor Author

2f15d4f is not strictly necessary, but I think it helps by showing that you can and should call super().test_func() if you inherit these classes.

@amickan
Copy link
Contributor
amickan commented Mar 28, 2025

Thomas and I synchronously discussed this and realized that there is another LoginRequiredMixin (form django.contrib.auth.mixins) that subclasses AccessMixin, but otherwise does the exact same thing as the guardian mixin. So we think that's the one we should be using instead.

koopmant added 2 commits March 28, 2025 11:14
@koopmant
Revert Create new UserPassesTestMixin
ecacd2c 
Revert "Add docstring"

This reverts commit e26d4f5.

Revert "Call super() in all implementation to clarify usage"

This reverts commit 2f15d4f.

Revert "Create new UserPassesTestMixin that allows for multiple inheritance"

This reverts commit f8d2903.
@koopmant
Replace LoginRequiredMixin from guardian with django.contrib.auth
1a8ce89
@amickan
Copy link
Contributor
amickan commented Mar 28, 2025

If we all agree that this is the correct solution, then it might be worth adding the guardian mixin to the banned modules in setup.cfg to make sure we use the correct one in the future.

@koopmant
Copy link
Contributor Author
koopmant commented Mar 28, 2025 •
edited
Loading

I just removed the usages of UserPassesTestMixin again, but realized we have the same problem as before. Also replacing guardian.mixins.LoginRequiredMixin with django.contrib.auth.mixins.LoginRequiredMixin shows the same issue with a lot of other tests. The mixin from django.contrib.auth simply returns the redirect from handle_no_permission(). But that means you have to inspect the returned object if you want to stop there and just return the redirect before doing anything else.

From the guardian.mixins.LoginRequired docs:

Due to parent class order traversal this mixin must be added as the left most mixin of a view.

And this is because dispatch must be reach there first. You cannot update the dispatch method and still benefit from decorator placed in guardians LoginRequiredMixin.

So I think creating the new class is the best way to go.

koopmant added 4 commits March 28, 2025 12:10
@koopmant
Revert Remove usages of UserPassesTestMixin
8963ca8 
Revert "Fix MRO issue"

This reverts commit 69f85e1.

Revert "Remove usages of UserPassesTestMixin"

This reverts commit c3ba650.

Revert "Replace LoginRequiredMixin from guardian with django.contrib.auth"

This reverts commit 1a8ce89.
@koopmant
Create new UserPassesTestMixin that allows for multiple inheritance
1ab82f0
@koopmant
Move UserPassesTestMixin to core.guardian
5614bca
@koopmant
Add django.contrib.auth.mixins.UserPassesTestMixin to banned modules
2fb8fb1
@koopmant
Copy link
Contributor Author

Anne and I discussed it synchronously again.

Indeed the problem is that you cannot update the dispatch method with a user test in a class where you use guardian LoginRequiredMixin and still have the benefit of checking if the user is logged in first (and if not logged-in automatically skipping the user test).

An alternative would be to create dedicated mixins whereever you want to create a user test together with LoginRequiredMixin while you want (or would expect) that the login required check is performed first and stops the chain if it fails. Such a mixin would then inherit AccessMixin and the dispatch method would be updated in the mixin. Then it would still be wrapped by LoginRequiredMixin. But you would need to create such a mixin even if you use it on only one class, so that is not intuitive. And nothing prevents you from forgetting it and updating dispatch in the class itself.

We also discussed using this new class everywhere we are now using django's AccessMixin combined with LoginRequiredMixin, since this has the same behaviour (if you update the dispatch method in the final class, the login check is not wrapped.) However, in these cases we add tests not related to the user. So the login check does not necessarily need to go first. If we do want to go this route, I would rename to new class to AccessMixin and ban the use of both UserPassesTestMixin and django's AccessMixin.

@koopmant koopmant marked this pull request as ready for review March 28, 2025 12:39
@koopmant koopmant requested review from amickan and jmsmkn as code owners March 28, 2025 12:39
@koopmant koopmant assigned jmsmkn and unassigned koopmant Mar 31, 2025
@jmsmkn
Copy link
Member
jmsmkn commented Apr 1, 2025

I'm not sure about this one, especially with leaving the default implementation to return True, I worry that could somehow end up as the only implementation and allow permission pass through. Do we need our own implementation of LoginRequiredMixin instead?

@jmsmkn jmsmkn removed their assignment Apr 1, 2025
@koopmant
Copy link
Contributor Author
koopmant commented Apr 1, 2025

@jmsmkn Did you have a pattern in mind there? I've not yet found an alternative to the guardian and django implementations, but that may be a case of WYSIATI...

@jmsmkn
Copy link
Member
jmsmkn commented Apr 1, 2025

I have no idea I'm afraid! All options have their drawbacks, I wonder if we should then just stick with what the frameworks provide.

@amickan amickan mentioned this pull request Apr 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amickan amickan Awaiting requested review from amickan amickan is a code owner

@jmsmkn jmsmkn Awaiting requested review from jmsmkn jmsmkn is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@koopmant @amickan @jmsmkn
0