logialogin_required does not check User.is_active
Sean Brant
Should it be marked for 1.2 or punt it until after the release
candidate? It looks to be a bug so it could probably go in at anytime.
Let me know your thoughts.
Gabriel Hurley
"This doesn’t control whether or not the user can log in. Nothing in
the authentication path checks the is_active flag, so if you want to
reject a login based on is_active being False, it is up to you to
check that in your own login view. However, permission checking using
the methods like has_perm() does check this flag and will always
return False for inactive users."
http://docs.djangoproject.com/en/1.1/topics/auth/#django.contrib.auth.models.User.is_active
So, if we're to believe the docs, this isn't a bug but a design
decision.
All the best,
- Gabriel
On Mar 16, 1:53 pm, Sean Brant <brant.s...@gmail.com> wrote:
> A co-worker of mine noticed this bug todayhttp://code.djangoproject.com/ticket/13125.
mattd
work around django's ever-so-convenient "login_required" decorator to
prevent a deactivated user from viewing a page they're no longer
allowed to view? a deactivated user *shouldn't even be allowed to be
be logged in*, but there's no way (that i know of) the purge the
session data for a given user on deactivation, and i can't just email
them to politely ask them to log out.
On Mar 16, 4:48 pm, Gabriel Hurley <gab...@gmail.com> wrote:
> The docs have this to say about is_active:
>
> "This doesn’t control whether or not the user can log in. Nothing in
> the authentication path checks the is_active flag, so if you want to
> reject a login based on is_active being False, it is up to you to
> check that in your own login view. However, permission checking using
> the methods like has_perm() does check this flag and will always
> return False for inactive users."
>
> http://docs.djangoproject.com/en/1.1/topics/auth/#django.contrib.auth...
Gabriel Hurley
"However, the AuthenticationForm used by the login() view does perform
this check, as do the permission-checking methods such as has_perm()
and the authentication in the Django admin."
So in this instance, if using the built-in django login view is your
only method of logging in to your site you would be safe. I'll admit
the whole business of not checking is_active seems a little odd to me,
but I also haven't looked to see what discussion there was around the
original decision. I'd hope it would make more sense if I did look
back in the archives.
I'm no expert on this one. Just thought I'd point out the fact that
the docs do discuss the subject of that bug ticket.
- Gabriel
mattd
waiting for 1.2 to be released before using it.
here's an example of my point: you run an internal tool for staff to
discuss the topics of the day. a few staff are let go or otherwise
deemed ineligible, and their access is revoked. so, you deactivate
their accounts in the lovely django admin, but then find that they are
still using the site. now, deactivating them doesn't log them out, and
that's to be expected since contrib.sessions is wisely decoupled from
contrib.auth. but, you thought you'd protected your views with
"login_required", but it turns out "login_required" only makes sure
that the user is currently logged in, riding on the assumption that
their status would not change in the duration of their session.
to me, it seems like an oversight. definitely interested in others'
opinions.
Russell Keith-Magee
I've just marked the ticket accepted. It isn't 1.2 critical, since the
currently behavior has existed since pretty much the beginning of
time, but it does strike me as an edge case that is worth catching.
The patch also requires some minor tweaks before it is trunk-ready;
comments on the patch.
Yours
Russ Magee %-)
Russell Keith-Magee
> interesting. i'm using the django-provided login form from 1.1,
> waiting for 1.2 to be released before using it.
>
> here's an example of my point: you run an internal tool for staff to
> discuss the topics of the day. a few staff are let go or otherwise
> deemed ineligible, and their access is revoked. so, you deactivate
> their accounts in the lovely django admin, but then find that they are
> still using the site. now, deactivating them doesn't log them out, and
> that's to be expected since contrib.sessions is wisely decoupled from
> contrib.auth. but, you thought you'd protected your views with
> "login_required", but it turns out "login_required" only makes sure
> that the user is currently logged in, riding on the assumption that
> their status would not change in the duration of their session.
>
> to me, it seems like an oversight. definitely interested in others'
> opinions.
Agreed that it is an oversight that should be fixed.
In the interim, there are two other ways you could limit your exposure
to this problem (other than the obvious "write your own
login_required" check):
* Use a permissions check in addition to the login_required check --
as noted it the docs, has_perm() will check the is_active status.
* Use a short-lived session (say, 24 hours). If a user has to log in
every 24 hours, the is_active check will be re-evaluated every 24
hours.
Yours
Russ Magee %-)
Giuseppe Ciotta
<freakb...@gmail.com> wrote:
>
> In the interim, there are two other ways you could limit your exposure
> to this problem (other than the obvious "write your own
> login_required" check):
>
> * Use a permissions check in addition to the login_required check --
> as noted it the docs, has_perm() will check the is_active status.
>
> * Use a short-lived session (say, 24 hours). If a user has to log in
> every 24 hours, the is_active check will be re-evaluated every 24
> hours.
Also, a middleware which checks for user.is_active and calls
logout(request) looks fine in the short term, look at
http://www.djangosnippets.org/snippets/1105/
Russell Keith-Magee
Looking at the problem a bit more, this isn't as simple as I first thought.
As the docs note, authentication backends aren't required to check
is_active. The background here is that authentication is separate from
granting permission. The authentication process validates that a
person are who they say they are. This is independent of determining
if that person is actually allowed to access the site. is_active is
the most basic way to check if a user is allowed to visit a site. The
role played by is_active could be completely replaced by a set of
permissions, for example.
Of course, the hiccough here is that Django's own authentication form
-- which probably represents the vast majority of auth.User installs
-- *does* validate is_active as part of the login process, which
causes the edge case failure where a logged in user is marked
inactive.
I suppose the root cause here is that login_required is badly named -
it's actually that the user requesting a view is authenticated, not
performing a login confirmation.
I'm no longer convinced that the solution is as simple as adding an
is_active check. This will break any login scheme that isn't using
is_active, which is specifically documented as an allowed use case.
Some possible (not necessarily mutually exclusive) solutions:
1) Don't touch the code. It's an annoying edge case, but it can be
caught by shortening session timeouts and making more use of
permissions. However, we should document the edge case with
login_required, as it is certainly contrary to obvious usage.
2) Add a specific "requires_active_user" decorator (and maybe a
decorator that combines the login_required and requires_active_user
check)
3) Allow login_required to take an argument that customizes the
'active' check. However, the default value for this argument would
need to be equivalent to the current behaviour; once you start down
this path, there really isn't much difference between a call to
login_required with an 'is_active' argument, and a call to
user_passes_test that does the authentication and active check.
4) Work out if there is a way to refactor the login process so that
the decision of whether is_active is checked is deferred. I don't have
any great ideas how this could be done, though.
I'm open to any other suggestions.
There is also some crossover with #3011, the proposal to refactor
auth.User. If we're going to allow pluggable or configurable user
models, we'll need to come to some understanding of how the login
process is supposed to interact with is_active (as well as other User
flags).
Yours,
Russ Magee %-)
mattd
but i did want to briefly mention the following:
1. simply shortening the length of sessions doesn't prevent a user
with revoked access from seeing new and potentially sensitive data
created after the user's deactivation. if deleting the user is not an
option (wasn't in my case), the only choice you have to force them out
(afaik) is to wipe the sessions table.
2. maybe this is just me, but if i'm *only* checking a binary "active"
flag, i'd rather not involve permissions/has_perm.
the path of least resistance would appear to be using
"user_passes_test", and perhaps packaging
"active_login_required" (named as you see fit) with django. this ties
usage directly to django's out-of-box auth (most likely used by the
majority of users), but if you're already using a custom auth backend,
it shouldn't be a deal breaker to write your own version.
thx,
matt
sub...@gmail.com
On Mar 17, 7:45 am, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
> 1) Don't touch the code. It's an annoying edge case, but it can be
> caught by shortening session timeouts and making more use of
> permissions. However, we should document the edge case with
> login_required, as it is certainly contrary to obvious usage.
This isn't appropriate in situations where HR wants to do a hard cut-
off of a user's access, in the event they are terminated.
> 2) Add a specific "requires_active_user" decorator (and maybe a
> decorator that combines the login_required and requires_active_user
> check)
Eh.
> 3) Allow login_required to take an argument that customizes the
> 'active' check. However, the default value for this argument would
> need to be equivalent to the current behaviour; once you start down
> this path, there really isn't much difference between a call to
> login_required with an 'is_active' argument, and a call to
> user_passes_test that does the authentication and active check.
Seems strange: "I want a user to be authenticated to use this view,
but I don't care if they're active." I'm not sure I agree that the
default should be the current behavior. I think in most cases this is
an oversight that most programmers simply don't account for in their
code--and closing this hatch is a favor and not an incompatibility
issue. Could be wrong.
> 4) Work out if there is a way to refactor the login process so that
> the decision of whether is_active is checked is deferred. I don't have
> any great ideas how this could be done, though.
Would it be too ugly or simplistic to have a callable
Backend.is_active() which these backends could manipulate to their
liking? Its flexible in the same way as get_profile(). Also, is it
possible we're expecting too much perfection given that, as you say,
there are legitimate proposals to refactor auth.User either way? (and
present solutions are more or less trivial).
-Steve
Harro
it says it does: Check if the user is logged in.
For me a user with is_active set to false shouldn't be allowed to
login, they either just created an account and still need to verify it
or they indicated that they wanted their account "removed", in which
case it's marked inactive so it doesn't cascade delete all their
related items too.
As the login view and the authentication middleware both use the
backend to get the user, this is is the place to implement the check
or not.
Which isn't that hard, the quick fix is to simply point out that the
is_active doesn't check this and if people want this they should
simply extend the ModelBackend's authenticate and get_user functions
and add the checks there.
Andy McCurdy
I think the problem isn't the login_required, but it simply does what
it says it does: Check if the user is logged in.
For me a user with is_active set to false shouldn't be allowed to
login, they either just created an account and still need to verify it
or they indicated that they wanted their account "removed", in which
case it's marked inactive so it doesn't cascade delete all their
related items too.
Russell Keith-Magee
need to be handled:
1) Normal Django authentication, honoring the is_active flag. For
this use case, you are completely correct - the current behavior is
wrong.
2) Custom authentication backends that *don't* honor the is_active
flag. This is entirely legal, and documented as such.
Unfortunately, the simple solution (add an is_active check to the
login_required decorator) is a completely valid and reasonable
solution for (1), but breaks (2). That's the sticking point.
>> 4) Work out if there is a way to refactor the login process so that
>> the decision of whether is_active is checked is deferred. I don't have
>> any great ideas how this could be done, though.
>
> Would it be too ugly or simplistic to have a callable
> Backend.is_active() which these backends could manipulate to their
> liking? Its flexible in the same way as get_profile(). Also, is it
> possible we're expecting too much perfection given that, as you say,
> there are legitimate proposals to refactor auth.User either way? (and
> present solutions are more or less trivial).
a few implementation kinks:
1) Backends aren't required to subclass a common base class, so we
have to assume that there will be custom backends out there that
*wont'* have an is_active check, even if we add one to ModelBackend.
This isn't a huge problem - we can work around it with a hasattr check
- but it's worth noting as something that will need to be addressed.
2) How do you get access to the right authentication backend in the
login_required decorator? The only argument you are guaranteed to have
access to is user, and the user doesn't (currently) have a way to get
access to the backend on which they were authenticated.
Yours,
Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
sub...@gmail.com
to the backend itself by calling something like... deactivate()?
In this event, the default backend's logic could be 'set is_active =
False and expire cookie' and custom backends could do (or not do)
whatever they want.
Forgive me for the hypothetical responses, as I don't have any
experience with custom backends.
-Steve
On Apr 15, 8:55 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote: