Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom User subclasses #171

Merged
merged 6 commits into from
Sep 7, 2023
Merged

Conversation

Cleptomania
Copy link
Contributor

@Cleptomania Cleptomania commented Sep 5, 2023

Description

This PR changes the behavior of the DescopeUser model somewhat.

Previously the DescopeUser model subclassed directly from Django's built-in User model, and was configured as a proxy model. This works fine if you only intend to use the Django built-in model, however this does not work if a user wants to define a custom model via the AUTH_USER_MODEL setting.

This PR makes use of the get_user_model function from Django, and subclasses from that, which means that if a user specifies a custom model from their application in the AUTH_USER_MODEL setting, then the DescopeUser model will subclass from that.

The two important things to note

DescopeUser is no longer a proxy model, this functionally I don't think changes much, but does mean it creates it's own DB table, wether it adds any new fields or not. This has been changed because in Django, you can not create a proxy model off of another model which is not a base model. So for example, the proxy model would work with Django's built-in user as that is a base model, but it would not work on a custom user model that subclasses from Django's built-in.

This does mean that the descope migrations can not be pre-shipped, and must be generated during the user application's run of makemigrations, because the DescopeUser migration will be dependent on the application's User model(and this may be Django's built-in, but in this scenario django-descope can't know that until the user creates their migrations).

@omercnet
Copy link
Member

omercnet commented Sep 6, 2023

Thanks for thinking this through @Cleptomania ! This is definitely a valid use case

I would like to keep this a proxy model as it does not store any data but only alter python behavior
According to Django you should be able to proxy non-abstract models, will you be able to attempt this change and keep the proxy model?

@omercnet
Copy link
Member

omercnet commented Sep 6, 2023

see Cleptomania#1

@Cleptomania
Copy link
Contributor Author

Thanks for the review @omercnet.

I've merged your PR in and made one change to it. The pre-generated migration by default uses the auth.user model from Django, so while the model itself was respecting the AUTH_USER_MODEL setting, the migration for it wasn't. The change I made dynamically pulls the base for the DescopeUser model as part of the migration.

From my testing with the current changes it works perfectly with either the default built-in Django model or a custom one, so I think this is probably ready to merge if you are happy with it. This should also mean less interference for anyone who already had this deployed because the change to the migration shouldn't functionally change anything if you were already using the built-in django model.

@omercnet
Copy link
Member

omercnet commented Sep 6, 2023

Looks great!

Let me run a few tests with internal packages to see the upgrade process is smooth and I'll be able to merge this hopefully tomorrow

@omercnet
Copy link
Member

omercnet commented Sep 7, 2023

failing tests are due to an underlying issue with descope-python sdk that is being fixed on descope/python-sdk#283

@omercnet omercnet merged commit a931ece into descope:main Sep 7, 2023
4 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants