Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19061 closed Cleanup/optimization (fixed)

Clarify the contract regarding is_active on custom users

Reported by: russellm Owned by: nobody
Component: Documentation Version: master
Severity: Release blocker Keywords:
Cc: preston@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The is_active flag isn't strictly needed for custom users to work, but it is relied upon by many login mechanisms.

To that end, it would be good to clarify the exact status of the is_active flag in the User contract.

Suggested contract is:

  • is_active is an optional attribute on a User object
  • It may be a database attribute, but doesn't have to be. (similar to is_staff for admin access)
  • If it exists, is_active is used to indicate whether the user is allowed access to the system at all. If it doesn't exist, the user is assumed to have access, pending any other permission checks that need to be performed.

Documenting the User contract is essential before 1.5 is finalised, so marking this as a release blocker.

Change History (8)

comment:1 Changed 3 years ago by russellm

  • Version changed from 1.4 to master

comment:2 Changed 3 years ago by ptone

I'd advocate making it a non-optional part of the contract, and a non-optional attribute.

The reason being, it has been the conventional way to detect a "disabled" user and so we should do what we can to guard against any chance of an inactive user being authorized to do something unintentionally.

If we are going to define the contract of being a default of True unless unless otherwise implemented, what about just putting this on AbstractBaseUser?

    @property
    def is_active(self):
        return True

this would eliminate the need for a bunch of conditional checks on that attribute, would be overridden by a field if specified, and raises a meaningful error if you try to set is_active on a model that doesn't implement an alternative.

A caveat being, I haven't dug fully into exactly why/how fields override the property - so I'm not sure how fragile that is, but I did verify that it is the case. I think it is the way fields contribute_to_class - so it should be something pretty stable.

Last edited 3 years ago by ptone (previous) (diff)

comment:3 Changed 3 years ago by russellm

Putting is_active on the AbstractBaseUser seems like a good approach to me, as long AbstractUser doesn't break anything when it defines the is_active database property, and then we can make the whole thing simpler.

comment:4 Changed 3 years ago by ptone

One potential issue with the property approach is that a non-field implementation will prevent the use of is_active in querysets - this won't break current User.objects.filter(is_active=True) usage with existing auth.User objects, but will limit an optimized approach that can be used in a universal user-object-neutral way.

Part of the issue in making this call is the very messy history is_active has had in the way people have struggled with a clear definition of what it means:

see #13125 and doc patches reference there.

Last edited 3 years ago by ptone (previous) (diff)

comment:5 Changed 3 years ago by ptone

  • Cc preston@… added
  • Has patch set

Not sure what I had going on, but my test project where I had tried the property approach - later showed that going with a property is not going to work, there is a setattr in Model.init for is_active fields - and that can't replace the property.

In the current patch - I take the approach of forcing the attribute to be read-only unless it is explicitly implemented in a subclass, to avoid any chance of something assuming it is a saveable attribute.

In the tests I try to handle what I assume would be some (odd) edge cases where people may want to use is_active as a non-field. Something I think would be a peculiar thing to do, but since it could be accommodated without too gross of a setattr approach.

If we wanted to remove the setattr guard - the default attr would still be provided, but any attempt to set the attr would result in an ephemeral instance attribute, and could cause confusion, hence the decision to put in the guard.

https://github.com/ptone/django/compare/is-active-required

comment:6 Changed 3 years ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f427ecdc88f71bbd2e8590da514e59c37c453217:

[1.5.x] Fixed #19061 -- added is_active attribute to AbstractBaseUser

comment:7 Changed 3 years ago by Preston Holmes <preston@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 4ea8105120121c7ef0d3dd6eb23f2bf5f55b496a:

Fixed #19061 -- added is_active attribute to AbstractBaseUser

comment:8 Changed 3 years ago by Russell Keith-Magee <russell@…>

In 81f5d4a1a7b955afe530d5292726b3a8a93d7038:

Added some test guards for some recently added auth tests.

Refs #19061, #19057.

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