Opened 14 years ago

Closed 13 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

Description

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@…> 14 years ago.
Add "is_active" check to login_required
ticket_13125_patch2.diff (466 bytes ) - added by wim@… 13 years ago.
Ticket 13125 patch using getattr

Download all attachments as: .zip

Change History (11)

by Matt Dennewitz <mattdennewitz@…>, 14 years ago

Attachment: login_required_active.diff added

Add "is_active" check to login_required

comment:1 by Russell Keith-Magee, 14 years ago

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 by Ramiro Morales, 14 years ago

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 by Russell Keith-Magee, 14 years ago

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 by Gonzalo Saavedra, 14 years ago

Cc: Gonzalo Saavedra added

comment:5 by Boris Savelev, 13 years ago

Cc: Boris Savelev added

comment:6 by Luke Plant, 13 years ago

Type: Bug

comment:7 by Luke Plant, 13 years ago

Severity: Normal

comment:8 by anonymous, 13 years ago

Easy pickings: unset

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

by wim@…, 13 years ago

Attachment: ticket_13125_patch2.diff added

Ticket 13125 patch using getattr

in reply to:  2 comment:9 by Jacob, 13 years ago

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