stormpath-django icon indicating copy to clipboard operation
stormpath-django copied to clipboard

Change import to get_user_model to direct import to StormpathUser

Open moemen opened this issue 10 years ago • 10 comments

Projects that use the app can have their own user model. They may don't want include fields you include in your model (like username, surname and given_name).

moemen avatar Mar 08 '16 13:03 moemen

Thanks for the PR! I'm looking into this. The only thing I'm unsure about is if I actually want to do this or not. My understanding of Django's AUTH_USER_MODEL is that it should NOT be changed once set, eg: https://docs.djangoproject.com/en/1.9/ref/settings/#auth-user-model

So, because of that, I'm not sure I want to support it that way. Thoughts?

rdegges avatar Mar 14 '16 23:03 rdegges

Yeah, that's right. You shouldn't change it once added to project. But this is a reusable app, and each project use it has different requirements. Some of them may require to include specific fields, or not to include other fields.

So you should be open to those changes. Django supports it, and you shouldn't restrict it unless you have specific requirements in the service itself.

I have just seen that, you are using username in django_stormpath.backends. My project doesn't have username field and use email for authentication, so why not to use UserModel.USERNAME_FIELD ?

moemen avatar Mar 17 '16 13:03 moemen

@moemen, if you look at the code for StormpathBaseUser you'll see that there's quite a bit implemented there and the django_stormpath library relies on this code to work properly and integrate the Stormpath functionality. You cannot just leave all that behind by completely replacing the AUTH_USER_MODEL with your own and expect things to work.

Knowing that, the approach I have taken in my project is to have my own User model extend the StormpathBaseUser class and override / add fields in my custom class.

class MyCustomUser(StormpathBaseUser):
    custom_field = models.CharField(max_length=10, null=True, blank=True)

    # override methods / class attributes as needed

Then in settings.py:

AUTH_USER_MODEL = 'myapp.MyCustomUser'

davidmarquis avatar Mar 17 '16 15:03 davidmarquis

I'll look into this more. I'm going to be re-digging into this library pretty soon. My prio for the next month or two is bringing both Flask / Django up-to-date with all of our latest features =)

rdegges avatar Mar 18 '16 17:03 rdegges

@davidmarquis Thanks for this clearification. I didn't notice this not in docs first time The required fields are: given_name, surname, email, and password. . username still not required for stormpath service but it's OK :smile:

The other problem is if swappable = 'AUTH_USER_MODEL' not exist, django will create a table for StormpathUser model. And it's useless for me.

moemen avatar Mar 20 '16 10:03 moemen

@moemen Django will not create a table for StormpathUser if you're not using it as your AUTH_USER_MODEL.

If you look at my example above, the MyCustomUser class inherits from StormpathBaseUser (and not StormpathUser). This is an abstract Django model. Thus, there is no need for Django to create a table related to Stormpath with that setup, only a table related to your concrete user model class.

I can assure you that the above strategy does not create any table belonging to the django_stormpath application. Try it you'll see!

davidmarquis avatar Mar 20 '16 10:03 davidmarquis

@davidmarquis Creating this model raises this error:

SystemCheckError: System check identified some issues:

ERRORS: accounts.User.groups: (fields.E304) Reverse accessor for 'User.groups' clashes with reverse accessor for 'StormpathUser.groups'. HINT: Add or change a related_name argument to the definition for 'User.groups' or 'StormpathUser.groups'. accounts.User.user_permissions: (fields.E304) Reverse accessor for 'User.user_permissions' clashes with reverse accessor for 'StormpathUser.user_permissions'. HINT: Add or change a related_name argument to the definition for 'User.user_permissions' or 'StormpathUser.user_permissions'. django_stormpath.StormpathUser.groups: (fields.E304) Reverse accessor for 'StormpathUser.groups' clashes with reverse accessor for 'User.groups'. HINT: Add or change a related_name argument to the definition for 'StormpathUser.groups' or 'User.groups'. django_stormpath.StormpathUser.user_permissions: (fields.E304) Reverse accessor for 'StormpathUser.user_permissions' clashes with reverse accessor for 'User.user_permissions'. HINT: Add or change a related_name argument to the definition for 'StormpathUser.user_permissions' or 'User.user_permissions'.

moemen avatar Mar 20 '16 12:03 moemen

I have added more enhancements here.

  • First created StormpathMixin, and moved the mandatory fields and methods that required to use stormpath.
  • Ensure to serialize and de-serialize date, datetime, and time fields
  • Add STORMPATH_RETRIEVE_SOCIAL_CUSTOM_DATA settings variable, to be able to add my extra fields to user model, in social login as stormpath doesn't support this yet.

I'm working on migrating an already running project to use Stormpath. And those changes are essential to complete my work. I'm happy to listen to your comments.

Thanks

moemen avatar Apr 27 '16 11:04 moemen

I'm still not totally sure I understand the purpose of this.

Let me ask a different question. Stormpath supports CustomData for Accounts. CustomData is basically a JSON store that you can use to persist arbitrary data to a Stormpath Account.

In our underlying Python library, you can do something like this:

account.custom_data['blah'] = {
    'woot': {
        'hi': 'there',
        'whats': 127.001,
    }
}
account.save()

Would it satisfy your requirement if we simply exposed the CustomData functionality to Django? I intended to do this originally, by basically creating a TEXT field in Django and using that to serialize / deserialize data to customdata transparently.

rdegges avatar Aug 10 '16 21:08 rdegges

I think the point is: django-stormpath should not force a custom AUTH_USER_MODEL on users.

Django docs (https://docs.djangoproject.com/en/1.10/topics/auth/customizing):

  • Warning Changing AUTH_USER_MODEL has a big effect on your database structure. It changes the tables that are available, and it will affect the construction of foreign keys and many-to-many relationships. If you intend to set AUTH_USER_MODEL, you should set it before creating any migrations or running manage.py migrate for the first time.

Therefore bad for existing apps.

  • Reusable apps and AUTH_USER_MODEL Reusable apps shouldn’t implement a custom user model. A project may use many apps, and two reusable apps that implemented a custom user model couldn’t be used together. If you need to store per user information in your app, use a ForeignKey or OneToOneField to settings.AUTH_USER_MODEL as described below.

Once again the same thing.

  • Model design considerations Think carefully before handling information not directly related to authentication in your custom User Model.

I think you get the point....

Taking a step back, what exactly is the point/need for the custom user model in this case? I'm thinking its primary for syncing? And can that be solved in a more opt-in fashion with signal or something?

I have an existing app, and this requirement is pretty much a blocker unless I feel like refactoring this project. Of course, the reason why to consider storm path was that I didn't need to do that kind of thing :) Food for thought.

unsignedint avatar Aug 23 '16 08:08 unsignedint