Opened 2 years ago

Closed 2 years ago

#19077 closed Uncategorized (fixed)

UserAdmin.add_view should handle USERNAME_FIELD on custom User models

Reported by: gwahl@… Owned by: nobody
Component: Documentation Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have a custom user model whose USERNAME_FIELD is email. The UserAdmin class provided by Django should work, once the the fieldsets and form properties are overridden. However, UserAdmin.add_view assumes there is a field called username on the user model, which is not true any more with custom users. It should use whatever field is defined in USERNAME_FIELD.

patch to come.

Change History (12)

comment:1 Changed 2 years ago by gwahl@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

A field called username is also assumed in user_change_password. str(user) should be used instead.

comment:3 Changed 2 years ago by ptone

  • Component changed from contrib.admin to contrib.auth
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Design decision needed

https://docs.djangoproject.com/en/dev/topics/auth/#custom-users-and-django-contrib-admin

does not list 'username' as a requirement - but what you have found is not the only place in admin where it is assumed:

contrib/admin/templates/admin/base.html
29:            <strong>{% filter force_escape %}{% firstof user.get_short_name user.username %}{% endfilter %}</strong>.

contrib/admin/templates/admin/object_history.html
32:            <td>{{ action.user.username }}{% if action.user.get_full_name %} ({{ action.user.get_full_name }}){% endif %}</td>

contrib/admin/templates/registration/password_reset_email.html
8:{% trans "Your username, in case you've forgotten:" %} {{ user.username }}

contrib/auth/admin.py
41:        (None, {'fields': ('username', 'password')}),
56:    list_display = ('username', 'email', 'first_name', 'last_name', 'is_staff')
58:    search_fields = ('username', 'first_name', 'last_name', 'email')
59:    ordering = ('username',)

Other places a username field is assumed (Caveat I've not yet analyzed these occurrences - they may be optional uses):

contrib/auth/middleware.py
58:            if request.user.username == self.clean_username(username, request):

contrib/comments/admin.py
27:    search_fields = ('comment', 'user__username', 'user_name', 'user_email', 'user_url', 'ip_address')

contrib/comments/models.py
114:                    userinfo["name"] = u.username
195:            (self.flag, self.comment_id, self.user.username)

contrib/comments/views/comments.py
43:            data["name"] = request.user.get_full_name() or request.user.username

I'm left wondering, given that we can't de-reference USERNAME_FIELD in all cases (eg admin list_view args etc), whether requiring a field that represents a username, and requiring a class USERNAME_FIELD attr, just so we can call the field something else may not be worth it in the end. We could just stipulate that the unique field be called username out of historical convention, but that it could be nearly any field type - and need not be a simple varchar

alternately/additionally we may want to say something about what more is in the contract for contrib.admin when it comes to username

comment:4 Changed 2 years ago by gwahl@…

How about a User.get_username() method? I use email addresses as usernames a lot, and would be dissapointed to have to call my field username. get_username() could be implimented in AbstractBaseUser as simply as

def get_username(self):
    return getattr(self, getattr(self, 'USERNAME_FIELD', 'username'))
    # or, I'm not sure what the preferred way to do this is
    username_field = self._meta.get_field(getattr(self, 'USERNAME_FIELD', 'username'))
    return username_field.value_from_object(self)

comment:5 Changed 2 years ago by ptone

Yes, get_username could also be a reasonable compromise, as it can be used in templates etc - Just to clarify - I was musing, not prescribing in my previous comment.

It is also why I marked this as DDN + release blocker as Russ and I have agreed we need to nail down these contracts as best we can in this release.

comment:6 Changed 2 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

One immediate improvement I can see is to actually make USERNAME_FIELD required, like it says in the docs, rather than a "defaults to username" attribute. That simplifies a lot of internal logic. Beyond that: I can certainly see the benefit in having a get_username()-esque field.

As for the admin registration issues: UserAdmin will really only ever work for auth.User. If you subclass AbstractBaseUser, you'll have a completely different set of fields to display in admin; If you're subclassing AbstractUser, you're doing so do add fields, so you'll have at least *some* different admin requirements. Writing your own UserAdmin is inevitable; all we can do is make it easier.

The good news is that if you subclass UserAdmin, you can just override the fieldset and form definitions, register your User class, and everything will work as you expect. This warrants documentation, but I don't think massive changes to UserAdmin (the original issue raised in this ticket notwithstanding) are called for.

Admin registrations for Comments (and any other model that references User) are an other issue entirely... however I think we can get some traction with lazy evaluation of attributes.

comment:7 Changed 2 years ago by ptone

  • Component changed from contrib.auth to Documentation
  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

I've clarified this in the docs:

https://github.com/django/django/pull/430

comment:8 Changed 2 years ago by gwahl@…

The documentation change alone doesn't fix this -- they were changed to say that UserAdmin respects USERNAME_FIELD, but without the code to support USERNAME_FIELD, there is not much functionality left in UserAdmin to subclass. I understand that the fieldsets and forms must be changed, but I think the actual view code in UserAdmin should work with any User that follows the basic contract. My normal user model just changes username to email, and first/last name for preferred/full name. Without UserAdmin respecting USERNAME_FIELD, it is almost entirely useless for even the slightest modification to users because several of the view functions have to be completely overridden.

I agree that UserAdmin should not undergo massive changes to support custom users, but it should at least support the USERNAME_FIELD part of the contract, so it is possible to usefully subclass it for your own user.

comment:9 Changed 2 years ago by ptone

  • Patch needs improvement set

I think the conclusion is that UserAdmin is a ModelAdmin first and foremost for contrib.AbstractUser and that if you are going to contribute a custom user model to your project, you are essentially expected to contribute a custom ModelAdmin for it to work in the admin.

I do think that two admin levels of admin contract could be potentially documented. Currently what is documented is what your model needs to support the permission machinery of the admin - and so is the first, lower, bar. The next bar would be to document what would be required to use your model with UserAdmin. My current docs modification is conflating those.

So to consider three options:

  1. modify UserAdmin to be very flexible to work with custom user models
  2. document the UserAdmin contract that custom user models must conform to in order to use UserAdmin
  3. document how to subclass or replace UserAdmin to allow your custom model to best work with the admin

1: isn't ultimately possible, there is no way to have UserAdmin support the range of custom user models. It is debatable whether adding a get_username and adding support for USERNAME_FIELD to UserAdmin just falsely increases this expectation, and in the end provide very few custom user models what they need. That is, having UserAdmin reference USERNAME_FIELD is unlikely to make UserAdmin work with many custom user models.

2: The contract for UserAdmin may essentially be too coupled to contrib.AbstractUser, and so isn't an worthwhile contract to document and build against for a AbstractBaseUser subclass, and instead point people to option 3

3: recognize that a common desire will be to extend User, and document the technique of subclassing UserAdmin and registering that subclass for the custom user, and otherwise point people to the need to create their own ModelAdmin for their own custom user.

So I'd say the current doc patch needs to be improved to:

clarify that what is currently documented as admin compliance is not UserAdmin compliance, and that meeting the bar for UserAdmin compat is better achieved by extending User or registering their own ModelAdmin.

I'm also going to put together a little test to see if my hunch is correct that the requirement for a custom user model to have a 'username' field goes beyond just the requirement for UserAdmin - as there are just so many references in the admin code to username.

comment:10 Changed 2 years ago by Russell Keith-Magee <russell@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c433fcb3fb34fccd69782979f0e7cd5f2d4a4893:

Fixed #19077, #19079 -- Made USERNAME_FIELD a required field, and modified UserAdmin to match.

comment:11 Changed 2 years ago by Gavin Wahl <gwahl@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

This patch missed one use of username -- on line 136 of django.contrib.auth.admin, in user_change_password, escape(user.username) should be escape(user.get_username()).

comment:12 Changed 2 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

This has been reported as part of the reopen of #19056.

Note: See TracTickets for help on using tickets.
Back to Top