Permission checks for approvement pages #138
Conversation
| template_name = "approve_hospitals.html" | ||
|
|
||
| def get(self, request, p_type): | ||
| # If you are not a staff user, the method decorator takes care of showing a 404 page, |
There was a problem hiding this comment.
I don't get a 404 as fake user A1:
There was a problem hiding this comment.
Good catch - the django decorator staff_member_required redirects to the login page under the assumption that you need to log in again with the higher ranked user.
You can override this behaviour like I did in cc5057d and 892abf0, but one might forget this. Under the assumption that we want to show as little information as possible about our internal url structures (debatable as an open source plattform), I added a customized version of the staff_member_required decorator, which has this behavior built-in, but also customizable. Let me know what you think!
There was a problem hiding this comment.
Is there a possibility to override staff_member_required with our version project-wide?
There was a problem hiding this comment.
We could give it the same name and import it from our repo, but I thought that might be too easy to miss in a future PR (to check whether the import is right when the decorator is used later in the file). I don't think we can override an import from a django package.
There was a problem hiding this comment.
We could in theory add a test for it though - but that might be overkill.
Normally, staff user check redirects to login page.
Baschdl
left a comment
Baschdl
left a comment
There was a problem hiding this comment.
http://localhost:8000/use_statistics/view/ shows the 404 page, http://localhost:8000/use_statistics/view redirects to login
|
The 404 is not intended by behaviour added in this PR, it is a result of the problem described in #157. The login redirect is a result of the login decorator sitting before the other one. We need to discuss what behaviour we want, e.g. what page do you see for a URL that you have access to, but are not logged in yet. This is a decision between obfuscation and good UX. |
| p_type = self.kwargs["p_type"] | ||
| if "delete" in post_params: | ||
| p = get_object_or_404(Participant[self.kwargs["p_type"]], uuid=post_params["uuid"]) | ||
| if not request.user.has_perm("matching.delete_participant%s" % p_type.lower()): |
There was a problem hiding this comment.
I would also test for matching.delete_participant%s and matching.can_approve_type_%s with a class-based decorator. A person who can't change the values, should probably also not see these pages.
There was a problem hiding this comment.
I don't understand what you mean - those get tested in lines 27-30. These here are for the actual action.
I think using a class decorator would be less expressive as it would probably have a long name. If you use them separately as decorators, both would be required. But I think not every approver would have the rights to delete participants and vice versa?
There was a problem hiding this comment.
You're right, I missed that. I still think it would be more consistent to have a method decorator and not have decorators on classes, decorators on methods and checks in a method (which makes it hard to check with a quick glance if the right permissions are set). But I see that this decorator would also not be totally self-explanatory.
| return self.post_approve(request, p_type, args, kwargs) | ||
| return self.get(request, p_type) | ||
|
|
||
| def post_delete(self, request, p_type, *args, **kwargs): |
There was a problem hiding this comment.
Didn't we want to add the decorators here?
|
This will also wait for a decision on permission names (refer to #183). |
|
Will test thoroughly again and also revise checks for buttons on staff-profile. |
Closes #121
Closes #137