Custom user models (#3011) #370

Closed
wants to merge 41 commits into
from

Projects

None yet
@jacobian
Member

No description provided.

@rafales

Those tests may fail when using custom user model (eg. when create_user() does not accept username param).

Member

Skip is the best solution here. But what about other tests in django or third party apps? Maybe there should be a hook for creating test users - create_test_user() on model manager. It could take arguments like username, email, is_staff and ignore them if they are not used.

But then we need solution for authenticating users in tests.

@jacobian jacobian commented on the diff Sep 15, 2012
django/contrib/admin/forms.py
@@ -26,17 +26,6 @@ def clean(self):
if username and password:
self.user_cache = authenticate(username=username, password=password)
if self.user_cache is None:
- if '@' in username:
- # Mistakenly entered e-mail address instead of username? Look it up.
- try:
- user = User.objects.get(email=username)
- except (User.DoesNotExist, User.MultipleObjectsReturned):
- # Nothing to do here, moving along.
- pass
- else:
- if user.check_password(password):
- message = _("Your e-mail address is not your username."
- " Try '%s' instead.") % user.username
@jacobian
jacobian Sep 15, 2012 Django member

Did this "@" in username check get moved elsewhere, or just removed? It looks like the latter. That's fine by me, but it is technically a backwards-incompatible change, so it should be mentioned in the release notes.

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

This check was removed because there's no guarantee that an email field exists anymore.

It might be possible to resurrect it by abstracting the underlying idea onto an optional method on the User class, but frankly, my inclination is to just kill the feature.

@jacobian
jacobian Sep 16, 2012 Django member

I'm fine with that - kill away. Let's just add a release note about it since it's theoretically backwards-incompatible (albeit in an incredibly minor way.)

@dlo
dlo commented Sep 15, 2012

First of all, this looks amazing. Fantastic job, I can't wait to use this in production! I've been skirting around the issue by implementing a custom auth backend with my own user model for months now (which worked), but the admin access was always confusing for clients.

I'm looking through here, and I still see first_name and last_name as required fields for the abstract user model. Any reason these should still be required?

@jacobian jacobian and 1 other commented on an outdated diff Sep 15, 2012
django/contrib/auth/backends.py
@@ -12,10 +12,13 @@ class ModelBackend(object):
# configurable.
def authenticate(self, username=None, password=None):
try:
- user = User.objects.get(username=username)
+ UserModel = get_user_model()
+ user = UserModel.objects.get(**{
+ getattr(UserModel, 'USERNAME_FIELD', 'username'): username
+ })
@jacobian
jacobian Sep 15, 2012 Django member

I wonder if this might be better done with a manager method - UserModel.objects.get_by_username('username') - to avoid the need for the USERNAME_FIELD indirection. That feels a bit cleaner to me, and seems to better encapsulate the query in the manager where it belongs.

@jacobian
jacobian Sep 15, 2012 Django member

Yeah, looking further, it appears this idiom is used a few more times, which to me triggers Fowler's "Third Time Refactor" rule. I think pushing this logic down into a manager method is going to lead to cleaner code (and easier-to-write code for end-users as well).

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

Fair call - I'll factor this out into a manager method.

@freakboy3742
freakboy3742 Sep 16, 2012 Django member

Turns out the method already exists: get_by_natural_key(). I've modified the code to use this method as appropriate.

@jacobian jacobian and 1 other commented on an outdated diff Sep 15, 2012
...o/contrib/auth/management/commands/createsuperuser.py
make_option('--database', action='store', dest='database',
default=DEFAULT_DB_ALIAS, help='Specifies the database to use. Default is "default".'),
+ ) + tuple(
+ make_option('--%s' % field, dest=field, default=None,
+ help='Specifies the %s for the superuser.' % field)
+ for field in getattr(get_user_model(), 'REQUIRED_FIELDS', ['email'])
@jacobian
jacobian Sep 15, 2012 Django member

I can't say I'm thrilled with the way these required fields are handled in this command - something here isn't passing the smell test. I understand why it's needed -- otherwise the createsuperuser command fails hard on custom user models -- and I can't put my finger on exactly what feels wrong. Nor can I suggest a better approach, so in all likelihood this should go in as-is. However, I'm flagging my discomfort so that we can think about it just a little bit and see if there's not something a little less... smelly.

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

Agreed with the code smell, but not sure I see a better option (other than not allowing command-line specification of non-username arguments). Suggestions welcome, but I can probably live with the code smell.

@alex alex commented on the diff Sep 15, 2012
django/contrib/auth/management/__init__.py
default_username = get_system_username()
try:
default_username = unicodedata.normalize('NFKD', default_username)\
.encode('ascii', 'ignore').decode('ascii').replace(' ', '').lower()
except UnicodeDecodeError:
return ''
- if not RE_VALID_USERNAME.match(default_username):
+
+ # Run the username validator
+ try:
+ auth_app.User._meta.get_field('username').run_validators(default_username)
@alex
alex Sep 15, 2012 Django member

Use get_field_by_name('username')[0], get_field is O(n) whereas get_field_by_name is O(1). No joke.

@apollo13
apollo13 Sep 15, 2012 Django member

Rofl, shouldn't we just change get_field to get_field_by_name('bla')[0] then? Using ugly API to be more performent smells bad if you ask me.

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

Hell yeah - fix the cause, not the symptom.

@freakboy3742
freakboy3742 Sep 16, 2012 Django member

Turns out it's not quite as simple as I thought...

Firstly - Yes, it's O(n), but, N is very small, and the way get_field_by_name() is O(1) is by pre-caching all possible lookups, so there's a first-call cache warming expense. In this usage, we're on a management command, and get_field will be invoked exactly once, so we're going to be in territory where the expense of cache warming to get the O(1) lookup will exceed the O(N) cost we're trying to avoid.

Secondly, it's not as simple as replacing the internals of get_field() with get_field_by_name(), because the cache priming in get_field_by_name() assumes all the models have been loaded (because it includes all reverse FK and M2M relations). get_field() only depends on self. This should get a little cleaner once app loading lands, but as it currently stands, it's difficult to guarantee that get_field_by_name() is actually callable.

So - I'm going to call this a no-fix; or, at least, a "not my bailiwick" fix. Meta certainly could be cleaned up, but that's a much bigger fish that needs frying.

@jacobian jacobian and 2 others commented on an outdated diff Sep 15, 2012
django/test/testcases.py
@@ -906,6 +908,13 @@ def skipUnlessDBFeature(feature):
"Database doesn't support feature %s" % feature)
+def skipIfCustomUser(test_func):
+ """
+ Skip a test if a custom user model is in use.
+ """
+ return skipIf(settings.AUTH_USER_MODEL != 'auth.User', 'Custom user model in use')(test_func)
+
+
@jacobian
jacobian Sep 15, 2012 Django member

Shouldn't this decorator live in django.contrib.auth somewhere instead of here?

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

Fair call. I'll move it.

@jacobian jacobian and 1 other commented on an outdated diff Sep 15, 2012
docs/topics/auth.txt
@@ -57,6 +57,10 @@ API reference
Fields
~~~~~~
+TODO document which attributes/methods come from AbstractBaseUser
+TODO tone down references to get_profile - it's not the best way of doing things
+any more.
+
@jacobian
jacobian Sep 15, 2012 Django member

These probably have to be TO-DONE before the patch lands. At the very least, make 'em into comments so they don't show up in the rendered output and make us look unprofessional :)

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

I'm pretty sure these TODOs are TO-DONE; but I'll check to be sure.

@jacobian jacobian commented on the diff Sep 15, 2012
docs/topics/auth.txt
+
+Django allows you to override the default User model by providing a value for
+the :setting:`AUTH_USER_MODEL` setting that references a custom model::
+
+ AUTH_USER_MODEL = 'myapp.MyUser'
+
+This dotted pair describes the name of the Django app, and the name of the Django
+model that you wish to use as your User model.
+
+.. admonition:: Warning
+
+ Changing :setting:`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 :setting:`AUTH_USER_MODEL`, you should set it before running
+ ``manage.py syncdb`` for the first time.
@jacobian
jacobian Sep 15, 2012 Django member

Perhaps here's a good point to mention using South if you do need to change it post-facto.

@jacobian jacobian commented on the diff Sep 15, 2012
docs/topics/auth.txt
+
+.. admonition:: Warning
+
+ Changing :setting:`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 :setting:`AUTH_USER_MODEL`, you should set it before running
+ ``manage.py syncdb`` for the first time.
+
+Referencing the User model
+--------------------------
+
+If you reference :class:`~django.contrib.auth.models.User` directly (for
+example, by referring to it in a foreign key), your code will not work in
+projects where the :setting:`AUTH_USER_MODEL` setting has been changed to a
+different User model.
@jacobian
jacobian Sep 15, 2012 Django member

Maybe mention that this is appropriate for reusable/pluggable models, but not really needed in projects where you know what your user model is and where it's not going to change.

@jacobian jacobian and 1 other commented on an outdated diff Sep 15, 2012
docs/topics/auth.txt
+easiest way to construct a compliant custom User model is to inherit from
+:class:`~django.contrib.auth.models.AbstractBaseUser` and provide some key
+definitions:
+
+.. attribute:: User.USERNAME_FIELD
+
+ A string describing the name of the field on the User model that is
+ used as the unique identifier. This will usually be a username of
+ some kind, but it can also be an email address, or any other unique
+ identifier.
+
+.. attribute:: User.REQUIRED_FIELDS
+
+ A list of the field names that *must* be provided when creating
+ a user.
+
@jacobian
jacobian Sep 15, 2012 Django member

Examples for these two attributes would be good; I can see people trying:

class MyUser(AbstractBaseUser):
    email = models.EmailField(...)
    ...
    USERNAME_FIELD = [email]

Which "works" in that it doesn't fail at load-time, but fails later on in really fun and interesting ways.

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

I thought this would be covered by the worked example lower down. However, more examples can't hurt, I suppose.

@jacobian
Member

@freakboy3742 - other than my inline comments, which fall mostly under the rubric of nit-picking, this looks really fantastic. Great work, and I'm fine with the answer to my comments being "you are high as a kite." +1 on merge.

@mlavin mlavin referenced this pull request in mlavin/django-all-access Sep 15, 2012
Closed

Swapable User Support #24

@apollo13 apollo13 and 1 other commented on an outdated diff Sep 15, 2012
docs/topics/auth.txt
+
+ Returns True if the user account is currently active.
+
+.. method:: User.has_perm(perm, obj=None):
+
+ Returns True if the user has the named permission. If `obj` is
+ provided, the permission needs to be checked against a specific object
+ instance.
+
+.. method:: User.has_module_perms(app_label):
+
+ Returns True if the user has permission to access models in
+ the given app.
+
+
+Worked Example
@apollo13
apollo13 Sep 15, 2012 Django member

Shouldn't that be "Working Example"? I've never heard the term "Worked Example" and even if it's proper English it probably will sound odd for every non-native speaker.

@freakboy3742
freakboy3742 Sep 15, 2012 Django member

"Worked example" is legitimate english; however if it's going to cause confusion, I'll give some thought to an alternate label.

@ironfroggy ironfroggy and 1 other commented on an outdated diff Sep 16, 2012
django/contrib/admin/sites.py
@@ -2,7 +2,7 @@
from django.http import Http404, HttpResponseRedirect
from django.contrib.admin import ModelAdmin, actions
from django.contrib.admin.forms import AdminAuthenticationForm
-from django.contrib.auth import REDIRECT_FIELD_NAME
+from django.contrib.auth import REDIRECT_FIELD_NAME, get_user_model
@ironfroggy
ironfroggy Sep 16, 2012

This new import isn't actually used that i can see.

@freakboy3742
freakboy3742 Sep 16, 2012 Django member

Not sure where this import came from... I've just killed it.

@apollo13
Member

I think we should update https://github.com/freakboy3742/django/blob/5a04cde342cc860384eb844cfda5af55204564ad/django/contrib/auth/models.py#L25 to use .save(update_fields=['last_login']) to prevent unnecessary big update queries (Just seeing this on a project where we store shitloads of data on the user ;)). Thoughts?

@freakboy3742
Member

No argument from me on the principle -- but it's completely orthogonal to this patch. From my perspective, feel free to commit that change as a standalone feature.

@uruz uruz commented on the diff Sep 17, 2012
...o/contrib/auth/management/commands/createsuperuser.py
make_option('--database', action='store', dest='database',
default=DEFAULT_DB_ALIAS, help='Specifies the database to use. Default is "default".'),
+ ) + tuple(
+ make_option('--%s' % field, dest=field, default=None,
+ help='Specifies the %s for the superuser.' % field)
+ for field in get_user_model().REQUIRED_FIELDS
@uruz
uruz Sep 17, 2012

There is the possible clash with the previosly defined options, isn't it? What will happen if we have 'settings' in the REQUIRED_FIELDS?

@freakboy3742
freakboy3742 Sep 18, 2012 Django member

You're correct - this risk does exist. I'm wondering if the right approach is to catch this as a validation issue, and reject any User model with certain required attributes.

@uruz
uruz Sep 18, 2012

Other option would be to prefix those dangerous fields with some prefix, like --field-settings or so. Not very consistent though.

@viniciuscainelli
viniciuscainelli Sep 21, 2012

Maybe this could solve the problem:

for field in getattr(get_user_model(), 'REQUIRED_FIELDS', []) 
@ramiro ramiro and 1 other commented on an outdated diff Sep 19, 2012
docs/topics/auth.txt
+ User model, you may need to look into using a migration tool like South_
+ to ease the transition.
+
+.. _South: http://south.aeracode.org
+
+Referencing the User model
+--------------------------
+
+If you reference :class:`~django.contrib.auth.models.User` directly (for
+example, by referring to it in a foreign key), your code will not work in
+projects where the :setting:`AUTH_USER_MODEL` setting has been changed to a
+different User model.
+
+Instead of referring to :class:`~django.contrib.auth.models.User` directly,
+you should reference the user model using
+:meth:`~django.contrib.auth.get_user_model()`. This method will return the
@ramiro
ramiro Sep 19, 2012 Django member

Wouldn't be it better to remove the ~ to leave the full path to django.contrib.auth.get_user_model() visible (we have no API reference docs for it yet)? This would ease the adaptation process for interested readers. Also, it is a function not a method.

@freakboy3742
freakboy3742 Sep 23, 2012 Django member

Good point - I've just made this change.

@ptone ptone and 1 other commented on an outdated diff Sep 19, 2012
django/contrib/auth/__init__.py
@@ -86,6 +91,19 @@ def logout(request):
from django.contrib.auth.models import AnonymousUser
request.user = AnonymousUser()
+
+def get_user_model():
+ "Return the User model that is active in this project"
+ from django.conf import settings
+ from django.db.models import get_model
+
+ try:
+ app_label, model_name = settings.AUTH_USER_MODEL.split('.')
+ except ValueError:
+ raise ImproperlyConfigured("AUTH_USER_MODEL must be of the form 'app_label.model_name'")
+ return get_model(app_label, model_name)
@ptone
ptone Sep 19, 2012 Django member

if settings.AUTH_USER_MODEL is set to an invalid value that conforms to the pattern ie: "typodapp.nonmodel" then get_model and hence get_user_model will return None.

I'd propose that get_user_model test for None and raise ImproperlyConfigured

@freakboy3742
freakboy3742 Sep 23, 2012 Django member

Good point - I've just implemented this.

@ptone ptone commented on the diff Sep 19, 2012
django/contrib/admin/models.py
@python_2_unicode_compatible
class LogEntry(models.Model):
action_time = models.DateTimeField(_('action time'), auto_now=True)
- user = models.ForeignKey(User)
+ user = models.ForeignKey(settings.AUTH_USER_MODEL)
@ptone
ptone Sep 19, 2012 Django member

Would it not be more future proof to have the FK set to get_user_model() ?

The disadvantage is it is somewhat less direct - but would allow more flexibility in future refactors of how the swapped model might be specified (hint hint, wink wink app-loading ;-)

@freakboy3742
freakboy3742 Sep 23, 2012 Django member

This is deliberate -- otherwise we have a 'chicken and egg' situation. In order for get_user_model() to run, you need to have the app cache parsed; but the app cache can't be fully populated until all the models are loaded.

@ptone
ptone Sep 23, 2012 Django member

ah yes - I forgot just how much I had changed to actually cook that particular chicken and scramble that particular egg in the app-loading branch. Looks like promoting the convention of explicitly referring to the setting is unavoidable for now then.

@apollo13
Member

The standard auth forms still use the User model instead of get_user_model. Is that on purpose or an oversight?

@freakboy3742
Member

"On purpose" is a little strong, but yes, it's deliberate. The reason is that a making a single auth form adapt to any User model is a complex task. The alternative is asking the user to write their own forms.

The auth forms aren't that complex to write from scratch; perhaps there are some elements we can factor out as utility methods (and suggestions are welcome here), so the overhead associated with maintaining a complex, adaptive form implementation just didn't seem worth it.

@apollo13
Member

Sry, didn't mean to sound harsh, I just didn't find better English words to express the sentence better. A quick search to auth/views|forms showed two instances where I think changes could be beneficial (I rather have users use our password reset stuff instead of writing their own broken forms).

The first two items are motivated by "making login/logout/pwreset work by default" since they usually don't have strong requirements on the usermodel (eg username-field, email, password, pk should be enough to make them just work)

@freakboy3742
Member

Good catch on the password reset views -- we can certainly clean those up without too much trouble. I've taken a look, and I've got a patch mostly ready.

There is also some extra documentation required here, too. We need to be clear about the requirements for each of these forms -- for example, that the password reset views assume an integer primary key, plus an "email" field that can be used for retrieval, and an "is_active" attribute.

@freakboy3742
Member

Patch applied as 70a0de3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment