Custom user model

[tymees]

Currently we use the default user model, which is fine for now.

However, if we're going to keep track of people's faculties (#509), we're going to need to add fields to users.

Django offers two options:

• Extending the existing model
• Creating a custom model
  (See https://docs.djangoproject.com/en/4.2/topics/auth/customizing/)

The first is easy, but not as nice and somewhat limited. The second is hard, but imo a lot better.

[tymees]

As an aside, creating a custom model would allow us to solve issues like #518 permanently by overriding the __str__ method

[tymees]

Just a thought, we might be able to store the faculty information as a group instead of a custom field.

While a custom user model is still preferred in the long term, it's very hard to switch over at this point. Doing the faculty-as-a-group thing might be easier short-term

[tymees]

@miggol @EdoStorm96 @Meesch

What approach do you guys think is better? Going the custom user model route, or the group route?

[miggol]

At this point I would be against implementing a custom user model for all the migration hassles it would cause. It's a shame, but I wouldn't want to jump through those hoops for what we're missing out on right now.

I'm assuming that by extending the user model, you mean what the Django docs calls a "proxy model". If we went that route, does that mean we could distribute the proxy model as a part of DSC federated_auth? Having something like a UuSamlUser proxy model which could be updated for all our applications if UU SAML properties change sounds like a benefit to me.

Even if it wasn't distributed in DSC, having all the bespoke UU-SAML-related fields (assuming there might be more in the future) compartmentalized in a proxy model appeals to me. Just plug and play different proxy models if we know some kind of schema change from IAM is coming in the future.

As an aside, creating a custom model would allow us to solve issues like #518 permanently by overriding the str method

I'm okay with the tradeoff of having a __str__() on the proxy model or using an external function similar to is_secretary(User) for displaying names. However, it does makes me wonder if we could create a custom user model and avoid most of the migration problems as follows:

• Point the custom user model to the DB table that previously belonged to the default user, or rename/copy the old table to the new custom user table.
• NEVER add a field to it. Only ever add simple methods to it like is_secretary() or __str__()

If it worked, I would still not be in favour of this, it's like planting a massive "DO NOT TOUCH THIS MODEL OR EVERYTHING EXPLODES" bomb in the codebase. But it's fun to think about jank solutions.

[tymees]

At this point I would be against implementing a custom user model for all the migration hassles it would cause. It's a shame, but I wouldn't want to jump through those hoops for what we're missing out on right now.

Yeah, I agree... It's probably not worth it.

I'm assuming that by extending the user model, you mean what the Django docs calls a "proxy model".

No, proxy models cannot have extra fields and are thus not the solution for this problem. I was referring to the 'create an extra model and link it with a one-to-one' method described on the linked page.

(Although, a proxy model can be used to avoid hide of the jank created by this method I guess? Just create a property that passes stuff to the related model? Not sure if that's really worth it tho, as hiding jank is not always the answer)

If we went that route, does that mean we could distribute the proxy model as a part of DSC federated_auth?

We could, but I don't see the benefit.

Every app is going to have its own unique set of attributes, so creating a generic one is not really helpful. (As it either will be filled with unused fields, or just a very large file of extremely simple classes).

In addition, ethics should be the only app that doesn't have a custom user model, which can just hold the extra fields there instead. (Which, imo, is the way it should be done).

[..] if UU SAML properties change [..]

In the unlikely scenario IAM wants to change attributes, we can just update the attribute map. That way we only have to update some config, and not our actual code.

I'm okay with the tradeoff of having a __str__() on the proxy model or using an external function similar to is_secretary(User) for displaying names.

Creating a proxy model for this purpose appeals to me; not entirely sure if the external function really makes sense over the proxy model approach tho?

However, it does makes me wonder if we could create a custom user model and avoid most of the migration problems as follows:

* Point the custom user model to the DB table that previously belonged to the default user, or rename/copy the old table to the new custom user table.

* NEVER add a field to it. Only ever add simple methods to it like `is_secretary()` or `__str__()`

If it worked, I would still not be in favour of this, it's like planting a massive "DO NOT TOUCH THIS MODEL OR EVERYTHING EXPLODES" bomb in the codebase. But it's fun to think about jank solutions.

Actually, one of the more successful solutions to switch to a custom model does something similar:

1. Create a new User model identical to the default one, in a new app, override the table name to use the existing table
   • The new app part is because Django always assumes the user model is created in the first migration of an app, for some reason
2. Set the new model as the auth model in settings.py
3. Create a migration for the new model
4. Convince Django you've run said migration with manual hacking in the DB
5. Convince Django that the new model has always been the User model with manual hacking in the DB
6. ?
7. Profit!

If not for the DB hacking, it's almost elegant.

[miggol]

Ah yes, I see now that the docs refer to a one-to-one linked as a profile model. That's the one I would consider attaching the faculty or other SAML-ish fields to. You make a valid point about distributing such a model with DSC: if we ever find we do need more fields we can add those to a profile model at that time, and otherwise we should probably defer to YAGNI.

Creating a proxy model for this purpose appeals to me; not entirely sure if the external function really makes sense over the proxy model approach tho?

You are right, using external functions is a tradeoff that we don't have to make. Now that I understand the difference between proxy and profile models we should probably just use a proxy model for user methods anyway.

So then considering we have a good enough and easy to implement solution for user methods in proxy models, the remaining question is as follows: how does a profile model compare to the group solution for assigning faculties? Groups feel very django-esque, and they would allow us to add users to multiple faculties where they have blended roles. But then they would be mixed in with all the other groups a user belongs to.

If we ever end up asking "which faculties does this user belong to" rather than just "is this person in GW", we might regret using groups rather than a custom faculties field. Same goes with adding methods or custom fields to a Faculty object. We'd be right back at the problem we're facing now, but then needing a custom group model.

So despite the YAGNI principle mentioned earlier, using groups here feels like locking ourselves in. My current preference would go to a faculties ManyToManyField from a user profile model to a custom Faculty model appended to by the SAML implementation.

Am I making sense? Sometimes I can have an overactive imagination for future problems.

[tymees]

Am I making sense? Sometimes I can have an overactive imagination for future problems.

You are definitely making perfect sense here, I think you make a very good case for using a custom Faculty model.

It would mean we can not use Django's built-in access control mixins, but it would be easy to create our own (based on Django's mixins). The only (hypothetical) potential problem would be that we can not assign permissions to those objects like groups, but we don't really use permissions anyway.
However, if we ever needed to use permissions we can always find a solution to that also (I have 2 ideas already I'm not going to mention for brevity).

The only point I would change is using said model through a different profile model. It's not needed, we can create a ManyToManyField from the faculty model to the User model directly instead. Maybe in the future we'll need a profile model as well, but to apply a little bit of YAGNI we can postpone that until it's actually needed.
This would also make the migration a bit easier, as we don't need to create a profile object for every user in the migration. Not that that is particularly hard, it's just annoying to do.