Clarify the contract regarding is_active on custom users

[Russell Keith-Magee]

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.

[Preston Holmes]

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'm think it is is the way fields contribute_to_class - so it should be something pretty stable.

[Russell Keith-Magee]

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.

[Preston Holmes]

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.

[Preston Holmes]

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

[Preston Holmes <preston@…>]

In f427ecdc88f71bbd2e8590da514e59c37c453217:

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

[Preston Holmes <preston@…>]

In 4ea8105120121c7ef0d3dd6eb23f2bf5f55b496a:

Fixed #19061 -- added is_active attribute to AbstractBaseUser

[Russell Keith-Magee <russell@…>]

In 81f5d4a1a7b955afe530d5292726b3a8a93d7038:

Added some test guards for some recently added auth tests.

Refs #19061, #19057.