Exit staff mode after permission revocation #2355
Conversation
evap/staff/staff_mode.py
Outdated
| @@ -52,6 +52,9 @@ def is_in_staff_mode(request): | |||
|
|
|||
|
|
|||
| def update_staff_mode(request): | |||
| if not request.user.has_staff_permission: | |||
| exit_staff_mode(request) | |||
| return | |||
| assert request.user.has_staff_permission | |||
There was a problem hiding this comment.
I think this assertion is no longer needed
There was a problem hiding this comment.
I'd move it up to line 33, where I think it would still add value.
The call graphs we can get now are a bit wonky. We can have
enter_staff_mode -> update_staff_mode -> finds out that not request.user.has_staff_permission -> exit_staff_mode and returns without exception, which is probably never what the initial caller intended?
It seems to me that update_staff_mode was about "update the timestamp in the session to now()" in the past, now it has changed semantics. Could use a little clean up in general, I think.
There was a problem hiding this comment.
I moved the assertion to line 33
There was a problem hiding this comment.
I agree that, while it's not super urgent, the names could be improved. In general, enter_staff_mode seems unneeded since #1587. The current update_staff_mode is more of a try_entering_or_refreshing_staff_mode. I don't have a strong opinion, but having a "try to enter, but we will check whether you are allowed" as the only way to enter staff mode seems fair to me. Maybe the enter_staff_mode view should check that entering worked?
| if not request.user.has_staff_permission: | ||
| exit_staff_mode(request) | ||
| return |
There was a problem hiding this comment.
@richardebeling @Kakadus Should we log something here?
There was a problem hiding this comment.
I'm fine with both adding and not adding logging.
There was a problem hiding this comment.
I think we can only manage one of the two
There was a problem hiding this comment.
If you both don't care, I would prefer if we added a small info-level log like "user {} tried to update staff mode session without permission" @ybrnr
Edit: nevermind, @richardebeling convinced me against this afterall
ybrnr
changed the title
|
@ybrnr How about "Exit staff mode after permission revocation"? |
Kakadus
changed the title
fix #2205