Simplify jobserver.models.User

[mikerkelly]

#4625 removed User.is_staff and User.is_superuser as no longer required. They were present as we once used django.contrib.admin but no longer do (instead having a completely custom staff app).

While working on #4625 I noticed some possible further simplifications to the models.User module. There are several more bits of code or peculiarities that may exist for historical reasons but are no longer required. Simplifying the module and using less custom-behaviour where appropriate would aid understanding and maintenance.

This might need breaking up into multiple issues. Possible changes to investigate:

• We could be better off extending BaseUserManager, as we extend AbstractBaseUser, rather than UserManager that is intended for managing BaseUser. The names are a little confusing here, careful.
  • This could remove the need to access the pseudo-private super()._create_user.
  • We may not need the UserManager.create_user behaviour around normalizing username and e-mail and hashing the password as we use social auth, but this needs investigation.
    • This might not be the case for Interactive users that enter e-mail as free text -- and enter a password(?). We are planning to remove Interactive, but may still need this for now. Or maybe not, if we're not creating any new Interactive users? To investigate.
• " TODO: rename name and remove the name property once all users have filledin their names"
  • Is get_full_name needed?

• We may be able to remove some or all of:
  • the custom UserQuerySet for sorting
    • "TODO: remove this method in favour of order_by(Lower("fullname")) once all users have fullname populated"
  • custom UserManager -- as above. Could it be the default?
  • custom create_user -- could this be default, or at least be called User.objects.create() to be idiomatic?
    • get_or_create_user -- if we do the above, is this function then needed or do we get it "for free" by the normal User.objects.get_or_create_user machinery?
  • Separate email and username properties if these are always the same?
    • email and username are distinct -- for social-auth users it's GitHub email vs GitHub username.

• There were several comments that I didn't understand or seemed out-of-date, inaccurate or unhelpful. Those included:
  • "Using a custom Model allows us to add extra fields trivially, eg Roles."
    • Obvious.
  • UserQuerySet docstring / TODO
  • "We cannot use get_or_create here because the create_user factory function is special, and does additional work when creating users." -- why can't the extra work be on User.create, then we would get it "for free" (would we?).
  • "PATs are generated for bot users. They can only be generated via a shell so they can't be accidentally exposed via the UI."
    • Could this use a reference to more info about this functionality/bot users?
  • "single use token login"
    • Is it clear what this means?
  • "normally this would be nullable..."
    • It is now.

[iaindillingham]

@mikerkelly to break this into smaller issues.

[mikerkelly]

This comment investigates the first two bullets from the issue description. How much of UserManager, UserManager.create_user, get_or_create_user and UserQuerySet do we need?

Do we need django.contrib.auth.models.UserManager?

The UserManager that our User model's manager extends provides the following functionality:

• _create_user function that ensures:
  • username is set.
  • email is normalized by BaseUserManager.normalize_email to lowercase the domain.
  • username is normalized by GlobalUserModel.normalize_username to normalize unicode.
  • password is hashed.
• create_user and create_superuser functions to create users with fields is_staff and is_superuser set.
  • These fields are not needed, see Remove User.is_staff and User.is_superuser #4625. We don't use these functions since that issue was resolved.
• with_perm function to apply a Permission.
  • We do not use Django's Permission model. Therefore we don't need this.

It requires a bit of digging to understand this. None of these behaviours were documented as important in our code or tested for (except for the _create_user behaviours in one #4625 unit test test_create_user where this behaviour was recently identified). If they're needed, we should clarify and test them. If not, we can simplify by slightly extending django.db.models.Manager or removing the manager entirely.

Social-auth users (GitHub) don't need these steps, as fields are validated through GitHub OAuth. Interactive users are created via interactive.commands.create_user, which calls User.objects.create and bypasses UserManager.create_user altogether.

Our UserManager create_user relies on the private parent _create_user function because the public methods automatically set is_staff and is_superuser, which we don’t want. Depending on an internal method is both difficult to follow and risks breaking when we update Django.

Our get_or_create_user function mimics standard Python Django get_or_create. There is a comment that this is needed because of the above special behaviour, as the standard manager calls create instead of _create_user. However, if we don't need _create_user, we don't need this function. It's used in two places:

• auth_pipeline.py, populated by GitHub OAuth with appropriate values.
• jobserver management command create_user, where the password is not populated, and username is guaranteed to be populated. While email and username are normalized, it's unclear how important this is. It does prevent creating multiple users with equivalent but distinct strings when using this command manually.

We don’t seem to rely on any of the special behaviors of Django's UserManager. The create_user management command only marginally benefits from normalization. We don't allow password-based logins, and currently, no users are created with an actual password. However, as a precaution, we should still hash any password passed to User creation in case it’s used in the future or if a real password is provided.

Conclusion: we can simplify by extending django.db.models.Manager with a custom create method that handles normalization and password hashing. This will make it clearer the behaviours User creation exhibits. This also lets us remove the custom get_or_create_user. Both of these reduce the amount of code to understand and maintain.

Do we need UserQuerySet?

UserQuerySet exists to order User instances so that those with a blank fullname appear last.

We can replicate this behaviour using Case and When clauses in Meta.ordering, making it the default ordering for the User model. This way, client code can call objects.all() without needing to explicitly use order_by_name. It’s also more in line with standard Django practices to define it this way.

[mikerkelly]

A quick prototype showed that most of the troublesome code in the User model could just be removed and everything still worked fine with not-too-many-changes. So instead of splitting this I've created PR #4698 with that and some other small improvements from this ticket or noted during that work.

The only outstanding thing from the description is this:

" TODO: rename name and remove the name property once all users have filledin their names"

Raised #4699 for this for triage.