id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
8049	AdminSite login has_permission inconsistency	nathan	Brian Rosner	"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.

Thank you,"		closed	contrib.admin	1.0		fixed	AdminSite login has_permission		Accepted	1	0	0	0	0	0