On Wed, Mar 17, 2010 at 3:46 PM, Russell Keith-Magee <freakboy3...@gmail.com> wrote: > On Wed, Mar 17, 2010 at 4:53 AM, Sean Brant <brant.s...@gmail.com> wrote: >> A co-worker of mine noticed this bug today >> http://code.djangoproject.com/ticket/13125. >> 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. > > 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.
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 %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
