Opened 6 years ago

Closed 4 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: gonz, boris 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@…> 6 years ago.
Add "is_active" check to login_required
ticket_13125_patch2.diff (466 bytes) - added by wim@… 4 years ago.
Ticket 13125 patch using getattr

Download all attachments as: .zip

Change History (11)

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

Add "is_active" check to login_required

comment:1 Changed 6 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 follow-up: Changed 6 years ago by ramiro

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 6 years ago by russellm

  • Triage Stage changed from Accepted to Design 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 5 years ago by gonz

  • Cc gonz added

comment:5 Changed 5 years ago by boris

  • Cc boris added

comment:6 Changed 5 years ago by lukeplant

  • Type set to Bug

comment:7 Changed 5 years ago by lukeplant

  • Severity set to Normal

comment:8 Changed 4 years ago by anonymous

  • Easy pickings unset

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

Changed 4 years ago by wim@…

Ticket 13125 patch using getattr

comment:9 in reply to: ↑ 2 Changed 4 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed
  • 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