AdminSite login has_permission inconsistency
|Reported by:||nathan||Owned by:||Brian Rosner|
|Severity:||Keywords:||AdminSite login has_permission|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
This is mainly a suggestion for refactoring but it may have security implications. Within the new AdminSite class you have a has_permission method and a login method.
has_permission checks for:
request.user.is_authenticated() and request.user.is_staff
while after authenticating login checks for:
if user.is_active and user.is_staff:
This results in a situation where a user may no longer be able to login but they still have permission to use the site, for example if a user were deactivated while logged in. Since the has_perm method of the user object checks if the user is active this doesn't seem to be too big of a problem under the current configuration, any attempt to access a specific object fails when the user.has_perm method is called. However this seems to be relying on a side effect, as the code evolves or for people subclassing the AdminSite real problems may result. Things could get ugly if someone were to override the has_permission method in a subclass and not the login method or vice versa.
I perhaps may not have dug deeply enough and there is a good reason for having two sets of tests that can be different but in general this seems to violate the DRY principle and open the door for future problems. It appears the code would be more robust if there were a single set of tests that both login and has_permission checked during verification. At the very least it would seem to make sense to explicitly check for user.is_active within the has_permission method rather than waiting for the admin model object to implicitly check it during an add/change/delete has_perm test.
Change History (9)
comment:3 Changed 8 years ago by
|Owner:||changed from nobody to Brian Rosner|
|Triage Stage:||Design decision needed → Accepted|
comment:4 Changed 7 years ago by
|Owner:||changed from Brian Rosner to Ivan Sagalaev|
|Status:||new → assigned|
comment:6 Changed 7 years ago by
|Owner:||changed from Ivan Sagalaev to Brian Rosner|
|Status:||assigned → new|