plea for re-opening ticket 13125 marked as won't fix

[Wim Feijen]

Jakob, thanks for looking into 13125 and taking action on it.

I'd like to make a case to re-open ticket 13125.

I understand that changing the current behaviour is backwards-
incompatible and therefor very unwanted. But, I'd say the current
implementation is forward-incompatible: meaning that current and
future users will stumble on something counter-intuitive and be amazed
that an inactive user can pass a login_required.

For me, the current behaviour is contrary to most peoples expectation,
and my proposal would be to make the backwards-incompatible change to
make django more consistent (I might even say: more logical), which I
think is a good thing.

My proposal is also to add an active_or_inactive_login_required
decorator (a better name is welcome) which just checks whether a user
is authenticated; and then people could import that as login_required.

The consequence is that some people would need to make a change to
keep their code working in Django 1.4 , but it is my belief that this
is only a small part of the Django population who have the skills to
adapt and that it will have a benificial effect to most current and
all future users.

Sorry that I raise this question again, but it is my strongest belief
that it will make Django better.

Wim

[Anssi Kääriäinen]

One approach would be to invalidate the sessions of the user when
is_active is changed from True to False. This way the current
registration method would work, and there would not be surprising "can
use site as long as session is open" situations, because there would
not be any open sessions.

There are numerous counter arguments to the idea: Unintended
consequences. There is a possibility for race conditions, which would
then be security issues. Action at distance. I don't know if this is
possible to implement for all session backends.

Just an idea maybe worth discussing.

- Anssi

[Paul McMillan]

> There are numerous counter arguments to the idea: Unintended
> consequences. There is a possibility for race conditions, which would
> then be security issues. Action at distance. I don't know if this is
> possible to implement for all session backends.

It's impossible to implement for cookie-based session backends. I'd
probably be opposed to a behavior like that which worked with some
backends and not others (though that backend is a weird special case).

-Paul

[Paul McMillan]

> I'd like to make a case to re-open ticket 13125.

Thanks for taking this to the mailing list rather than arguing in trac.

> I understand that changing the current behaviour is backwards-
> incompatible and therefor very unwanted. But, I'd say the current
> implementation is forward-incompatible: meaning that current and
> future users will stumble on something counter-intuitive and be amazed
> that an inactive user can pass a login_required.

No. Django makes an incredibly strong promise about backwards
compatibility to its users. Security releases are the ONLY reason we
modify behavior in backwards incompatible fashions, and we try very
hard to avoid that.

> For me, the current behaviour is contrary to most peoples expectation,
> and my proposal would be to make the backwards-incompatible change to
> make django more consistent (I might even say: more logical), which I
> think is a good thing.

Yeah, I agree that the current behavior is counter intuitive. It is an
oddity and a wart that exists.

> My proposal is also to add an active_or_inactive_login_required
> decorator (a better name is welcome) which just checks whether a user
> is authenticated; and then people could import that as login_required.

I wouldn't be opposed to an additional decorator which makes better
grammatical sense and does explicitly what you want. We just can't
change the behavior of the current one. If you can come up with two
new ones that make better sense there might be an argument for slowly
deprecating the existing one.

> The consequence is that some people would need to make a change to
> keep their code working in Django 1.4 , but it is my belief that this
> is only a small part of the Django population who have the skills to
> adapt and that it will have a benificial effect to most current and
> all future users.

No. We do not do this. Otherwise every release would end up stuffed
full of dozens of "tiny easy changes" which means nobody would bother
updating.

Regards,
-Paul

[Wim Feijen]

Thanks Paul,

I like the idea of the additional decorator! Let's do that.

Wim

[Wim Feijen]

I added an active_login_required decorator. See:

https://code.djangoproject.com/ticket/16797

Is it a good name? It is a good patch? Or is it stupid?

Thanks, Wim

[Florian Apolloner]

Stupid question, but why do you let inactive users login at all? I mean is this really a problem of the decorator and not of the login system you use?!

[Wim Feijen]

The case is as follows:

1. An active user is logged in and has a valid session.
2. The user is inactivated by a system admin, who wants to disable the
user.
3. Because the user is still logged in, (maybe for two weeks, or for
whatever expiration time the developer has set), he passes the
login_required decorator, and still can see those pages which we
naively thought were being protected by the login_required decorator,
because that decorator doesn't check for is_active.

This patch is a patch for that problem.

Wim

[Wim Feijen]

Paul,

On further thinking, I do believe that the current behaviour of
login_required should be considered a bug.

Wim

On 10 sep, 02:03, Paul McMillan <p...@mcmillan.ws> wrote:

[Florian Apolloner]

On Sunday, September 11, 2011 8:52:03 PM UTC+2, Wim Feijen wrote:

3. Because the user is still logged in, (maybe for two weeks, or for
whatever expiration time the developer has set)

Imo in that case the developer should write a middleware that logs the user out on the next request. I see your problem, but imo your system needs a bit of tweaking if you allow inactive users to browse your site till their session expires (which with SAVE_EVERY_REQUEST +  a high timeout) could as well be never…

[Wim Feijen]

Hi Florian,

Then again, the default behaviour now is as you describe. That's why I
would call it a security leak.

Unfortunately, it is not only my system, it is the system of any
unaware Django programmer.

Wim

[Florian Apolloner]

Probably yeah, on the other hand the docs tell you that is_active doesn't neccessarily have to be checked by backends, so if a backend allows to login inactive users it makes no sense to check that flag in login_required… I guess what I am proposing is that the login_required flag checks via the auth backends whether or not the user should be allowed to pass, that way all the neccessary checks stay in one place…

[Justine Tunney]

Here's an idea that might work better than a decorator:

Create a setting called ALLOW_DEACTIVATED_LOGINS.  Then modify auth.login() to enforce this as well as changing ModelBackend.get_user() to logout users whose accounts are disabled.  Make the setting True by default in 1.4 and announce it'll be set to False in either 1.5 or 1.6.

This way is_active can be secure by default, not require users to implement its functionality themselves while also following the principle of least astonishment.  The only confusing this about this solution is that users might not understand it doesn't affect the admin login form and auth.views.login

On Mon, Sep 12, 2011 at 3:43 AM, Florian Apolloner <f.apo...@gmail.com> wrote:

Probably yeah, on the other hand the docs tell you that is_active doesn't neccessarily have to be checked by backends, so if a backend allows to login inactive users it makes no sense to check that flag in login_required… I guess what I am proposing is that the login_required flag checks via the auth backends whether or not the user should be allowed to pass, that way all the neccessary checks stay in one place…

--
You received this message because you are subscribed to the Google Groups "Django developers" group.

To view this discussion on the web visit https://groups.google.com/d/msg/django-developers/-/A922lTjpZc8J.

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.