Opened 11 years ago

Closed 10 years ago

#13125 closed Bug (wontfix)

login_required does not check User.is_active

Reported by: Matt Dennewitz <mattdennewitz@…> Owned by: nobody
Component: contrib.auth Version: 1.1
Severity: Normal Keywords: login_required, authorization, users
Cc: Gonzalo Saavedra, Boris Savelev Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no


The login_required decorator is not checking User.is_active, as staff_member_required does. If an authenticated user is deactivated (via setting is_active to False), the user is still able to browse login_required-protected views.

Attachments (2)

login_required_active.diff (496 bytes) - added by Matt Dennewitz <mattdennewitz@…> 11 years ago.
Add "is_active" check to login_required
ticket_13125_patch2.diff (466 bytes) - added by wim@… 10 years ago.
Ticket 13125 patch using getattr

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by Matt Dennewitz <mattdennewitz@…>

Attachment: login_required_active.diff added

Add "is_active" check to login_required

comment:1 Changed 11 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

It's an edge case, but one worth catching.

There is an edge case that the proposed patch doesn't catch - the case of a custom user object (from a custom auth backend) that doesn't provide is_active (since auth backends aren't required to provide it). Changing the simple 'user.is_active' check to 'getattr(user, 'is_active', True)' should provide a backwards compatible solution.

Patch also requires tests.

comment:2 Changed 11 years ago by Ramiro Morales

See #7011 (and its related [9176] commit) for some info about why this is currently like it is. Also, #12113 (and [12193]) introduced further refinements in the related documentation.

comment:3 Changed 11 years ago by Russell Keith-Magee

Triage Stage: AcceptedDesign decision needed

Thanks for the historical pointers ramiro. Thinking about this a bit more, there are some edge cases that require some additional consideration. Discussion on django-dev

comment:4 Changed 11 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added

comment:5 Changed 11 years ago by Boris Savelev

Cc: Boris Savelev added

comment:6 Changed 10 years ago by Luke Plant

Type: Bug

comment:7 Changed 10 years ago by Luke Plant

Severity: Normal

comment:8 Changed 10 years ago by anonymous

Easy pickings: unset

This should be changed, fast! It's a silly design decision!

Changed 10 years ago by wim@…

Attachment: ticket_13125_patch2.diff added

Ticket 13125 patch using getattr

comment:9 in reply to:  2 Changed 10 years ago by Jacob

Resolution: wontfix
Status: newclosed
UI/UX: unset

See ramiro's comments above. is_active is a hook for custom auth sources and custom auth logic; the built-in stuff doesn't check it. Yes it's a bit counter-intuitive, but changing it is going to break a number of expectations in users' code, so it's going to need to stay as is.

Note: See TracTickets for help on using tickets.
Back to Top