#3011 - Custom User Models -- Call for final review

Showing 21-44 of 44 messages
#3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/15/12 5:34 AM
Hi all,

The implementation of custom User models is now ready for final
review, prior to commit to trunk.

The code is available in my Github repo, in the t3011 branch.

https://github.com/freakboy3742/django/tree/t3011

The diff is available here:

https://github.com/freakboy3742/django/compare/master...t3011

Documentation is also available in the auth topic guide:

https://github.com/freakboy3742/django/blob/t3011/docs/topics/auth.txt#L1761

Barring the discovery of major problems, I'm aiming to commit this
towards the end of next week. Any and all feedback is welcome.

Yours
Russ Magee %-)
Re: #3011 - Custom User Models -- Call for final review Jacob Kaplan-Moss 9/15/12 8:49 AM
Hey Russ --

I took the liberty of opening up a pull request
(https://github.com/django/django/pull/370) so that I can leave
comments inline. Hope you don't mind.

Jacob
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
Re: #3011 - Custom User Models -- Call for final review Anssi Kääriäinen 9/15/12 8:59 AM
On 15 syys, 15:34, Russell Keith-Magee <russ...@keith-magee.com>
wrote:
> Hi all,
>
> The implementation of custom User models is now ready for final
> review, prior to commit to trunk.
>
> The code is available in my Github repo, in the t3011 branch.
>
> https://github.com/freakboy3742/django/tree/t3011
>
> The diff is available here:
>
> https://github.com/freakboy3742/django/compare/master...t3011
>
> Documentation is also available in the auth topic guide:
>
> https://github.com/freakboy3742/django/blob/t3011/docs/topics/auth.tx...
>
> Barring the discovery of major problems, I'm aiming to commit this
> towards the end of next week. Any and all feedback is welcome.

The above branch does not work on Python3, which seems like a blocker
issue.

Some questions:
  - If I proxy the auth.User model when it is swapped out I get an
error from model validation: "AttributeError: type object 'UserProxy'
has no attribute '_base_manager'". The error could be cleaner.
  - The last_login field is in the AbstractBaseUser, but it isn't
documented as a required field. Is this field required for something?
Is it needed as part of AbstractBaseUser?
  - There is no way to do single-table extension of the default User
model. Should there be a way? To me it seems this would be beneficial
when dealing with "project profiles". There is little reason I would
like to store a required employee number in another table. Maybe it
would work if current User was moved to AbstractUser, and User would
be class User(AbstractUser): class Meta: swappable =
'AUTH_USER_MODEL'? Quick testing indicates this could work... See
commit: https://github.com/akaariai/django/commit/08bcb4aec1ed154cefc631b8510ee13e9af0c19d
  - I did a little change to REQUIRED_FIELDS handling in conjunction
with create_(super)user(). See commit:
https://github.com/akaariai/django/commit/d9f5e5addbad5e1a01f67e7358e4f5091c3cad81

With the above commits I can do this:

class MyUser(AbstractUser):
    employee_no = models.CharField(max_length=5)
    some_other_field = models.TextField(null=True, blank=True)
    REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']

syncb & create_superuser commands work. Admin doesn't work directly,
but it seems you can easily subclass the default user admin & change
form to make it work.

Single-table inheritance seems useful to me, and the added complexity
to code isn't much.


The above questions aren't meant to be commit blockers. In my
(somewhat limited) testing the only real issue was the py3
incompatibility, and that seems to be an easy fix. Overall, this looks
good to me.

 - Anssi

BTW: I did also a little test to allow select_related() for user
profiles: https://github.com/akaariai/django/commit/81f91db3234110b90572b25ca69b9012a475c460
- the idea is that the profile models could just to
get_user_model().SELECT_RELATED_MODELS.add('user_reverse_name'). I
wonder if we want something like this now/later...
Re: #3011 - Custom User Models -- Call for final review Anssi Kääriäinen 9/15/12 10:07 AM
On 15 syys, 18:59, Anssi Kääriäinen <anssi.kaariai...@thl.fi> wrote:
> With the above commits I can do this:
>
> class MyUser(AbstractUser):
>     employee_no = models.CharField(max_length=5)
>     some_other_field = models.TextField(null=True, blank=True)
>     REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']
>
> syncb & create_superuser commands work. Admin doesn't work directly,
> but it seems you can easily subclass the default user admin & change
> form to make it work.
>
> Single-table inheritance seems useful to me, and the added complexity
> to code isn't much.

Actually, there is more common use case which doesn't seem to be
solved by the current branch: getting proxy instances into
request.user. If you use just ProxyUser(auth.User) the proxy model
will not be loaded into request.user, and if you set the ProxyUser as
AUTH_USER_MODEL, then it will not work as you can't subclass the
swapped out auth.User.

The added commits solves this by allowing you to do:
    class MyUser(AbstractUser):
        # define additional methods etc.
and:
    settings.AUTH_USER_MODEL = 'myuser'

This could also be solved by somehow allowing fetching proxy instance
to request.user. In any case, IMO this needs some solution before
release.


This feature is already looking really good. I know that every single
one of my projects will have uses for this feature, so I am really
looking forward to getting this into 1.5.

 - Anssi
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/15/12 4:15 PM
On Sat, Sep 15, 2012 at 11:59 PM, Anssi Kääriäinen
<anssi.ka...@thl.fi> wrote:
> On 15 syys, 15:34, Russell Keith-Magee <russ...@keith-magee.com>
> wrote:
>> Hi all,
>>
>> The implementation of custom User models is now ready for final
>> review, prior to commit to trunk.
>>
>> The code is available in my Github repo, in the t3011 branch.
>>
>> https://github.com/freakboy3742/django/tree/t3011
>>
>> The diff is available here:
>>
>> https://github.com/freakboy3742/django/compare/master...t3011
>>
>> Documentation is also available in the auth topic guide:
>>
>> https://github.com/freakboy3742/django/blob/t3011/docs/topics/auth.tx...
>>
>> Barring the discovery of major problems, I'm aiming to commit this
>> towards the end of next week. Any and all feedback is welcome.
>
> The above branch does not work on Python3, which seems like a blocker
> issue.

Dang - I knew I forgot something. I'll put this on my list to look at.

> Some questions:
>   - If I proxy the auth.User model when it is swapped out I get an
> error from model validation: "AttributeError: type object 'UserProxy'
> has no attribute '_base_manager'". The error could be cleaner.


>   - The last_login field is in the AbstractBaseUser, but it isn't
> documented as a required field. Is this field required for something?
> Is it needed as part of AbstractBaseUser?

Yes, last_login is required - it's needed in order to generate
password reset tokens etc.

It isn't documented because we *really* want people to be subclassing
AbstractBaseUser, not building their own User from scratch.

>   - There is no way to do single-table extension of the default User
> model. Should there be a way? To me it seems this would be beneficial
> when dealing with "project profiles". There is little reason I would
> like to store a required employee number in another table. Maybe it
> would work if current User was moved to AbstractUser, and User would
> be class User(AbstractUser): class Meta: swappable =
> 'AUTH_USER_MODEL'? Quick testing indicates this could work... See
> commit: https://github.com/akaariai/django/commit/08bcb4aec1ed154cefc631b8510ee13e9af0c19d
>   - I did a little change to REQUIRED_FIELDS handling in conjunction
> with create_(super)user(). See commit:
> https://github.com/akaariai/django/commit/d9f5e5addbad5e1a01f67e7358e4f5091c3cad81
>
> With the above commits I can do this:
>
> class MyUser(AbstractUser):
>     employee_no = models.CharField(max_length=5)
>     some_other_field = models.TextField(null=True, blank=True)
>     REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']
>
> syncb & create_superuser commands work. Admin doesn't work directly,
> but it seems you can easily subclass the default user admin & change
> form to make it work.
>
> Single-table inheritance seems useful to me, and the added complexity
> to code isn't much.

To be clear -- is this just targeting the "I'm completely happy with
auth.User; I just want to put XXX on the model" use case? If so, I can
certainly agree with this; I'll merge these changes in.

> The above questions aren't meant to be commit blockers. In my
> (somewhat limited) testing the only real issue was the py3
> incompatibility, and that seems to be an easy fix. Overall, this looks
> good to me.
>
>  - Anssi
>
> BTW: I did also a little test to allow select_related() for user
> profiles: https://github.com/akaariai/django/commit/81f91db3234110b90572b25ca69b9012a475c460
> - the idea is that the profile models could just to
> get_user_model().SELECT_RELATED_MODELS.add('user_reverse_name'). I
> wonder if we want something like this now/later...

Possibly; but if we do, I'd rather attack it at the model Meta level
-- i.e., an analog to Meta: ordering that applies a select_related
automatically to every queryset.

Yours,
Russ Magee %-)
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/15/12 4:25 PM
I'm not sure there can be *any* clean answer for this. A base class
can't be swapped out at time of class construction, so I'm not sure
how we would handle proxied User models in any meaningful way.

My inclination here is to raise an error if, during class
construction, the base class of a proxy class is found to be swapped,
and document this as a known limitation.

Russ %-)
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/16/12 3:18 AM
Hi all,

I've just pushed fixes that:

 * Correct the patch for Python 3
 * Merges Anssi's AbstractUser base class changes
 * Moves skipIfCustomUser to django.contrib.auth.tests.utils
 * Improves the error message when you have a Proxy to a swapped class
 * Updates the documentation to address the feedback received to date

Based on the feedback on the patch so far, it looks like we're on
track to merge this later this week.

Yours,
Russ Magee %-)
Re: #3011 - Custom User Models -- Call for final review Anssi Kääriäinen 9/16/12 5:15 AM
On 16 syys, 02:15, Russell Keith-Magee <russ...@keith-magee.com>
wrote:
> >   - The last_login field is in the AbstractBaseUser, but it isn't
> > documented as a required field. Is this field required for something?
> > Is it needed as part of AbstractBaseUser?
>
> Yes, last_login is required - it's needed in order to generate
> password reset tokens etc.
>
> It isn't documented because we *really* want people to be subclassing
> AbstractBaseUser, not building their own User from scratch.

+1 to this.

> >   - There is no way to do single-table extension of the default User
> > model. Should there be a way? To me it seems this would be beneficial
> > when dealing with "project profiles". There is little reason I would
> > like to store a required employee number in another table. Maybe it
> > would work if current User was moved to AbstractUser, and User would
> > be class User(AbstractUser): class Meta: swappable =
> > 'AUTH_USER_MODEL'? Quick testing indicates this could work... See
> > commit:https://github.com/akaariai/django/commit/08bcb4aec1ed154cefc631b8510...
> >   - I did a little change to REQUIRED_FIELDS handling in conjunction
> > with create_(super)user(). See commit:
> >https://github.com/akaariai/django/commit/d9f5e5addbad5e1a01f67e7358e...
>
> > With the above commits I can do this:
>
> > class MyUser(AbstractUser):
> >     employee_no = models.CharField(max_length=5)
> >     some_other_field = models.TextField(null=True, blank=True)
> >     REQUIRED_FIELDS = AbstractUser.REQUIRED_FIELDS + ['employee_no']
>
> > syncb & create_superuser commands work. Admin doesn't work directly,
> > but it seems you can easily subclass the default user admin & change
> > form to make it work.
>
> > Single-table inheritance seems useful to me, and the added complexity
> > to code isn't much.
>
> To be clear -- is this just targeting the "I'm completely happy with
> auth.User; I just want to put XXX on the model" use case? If so, I can
> certainly agree with this; I'll merge these changes in.

Yes, that is the use case. The proxy use case I was worried about in
the other post is about just overriding methods or the manager
without adding any fields. The ability to actually have a _proxy_
isn't important, it is enough you can extend the default user model
easily.

Creating an admin class for no-added-fields extended user model is
somewhat straightforward, although the admin forms currently assume
the auth.User as the base user. I did a little hack to make it easier
to use these forms, see commit:
https://github.com/akaariai/django/commit/04ae5183df8613fd0d91b43834a4582fdf6b0d04
- but I am not totally happy about this approach. If you use an
AbstractBaseUser derived user model, these forms aren't actually
usable.

Creating an admin class for added fields model is also somewhat easy,
you just need to play with UserAdmin.fieldsets to have the additional
fields actually present in the edit form, and you need to make your
required fields "blank=False, null=True", so that the two-stage user
creation works correctly.

> > BTW: I did also a little test to allow select_related() for user
> > profiles:https://github.com/akaariai/django/commit/81f91db3234110b90572b25ca69...
> > - the idea is that the profile models could just to
> > get_user_model().SELECT_RELATED_MODELS.add('user_reverse_name'). I
> > wonder if we want something like this now/later...
>
> Possibly; but if we do, I'd rather attack it at the model Meta level
> -- i.e., an analog to Meta: ordering that applies a select_related
> automatically to every queryset.

Rethinking this, and I don't see the need anymore. It is now possible
to control the user model's manager, so you can just override
get_query_set() and use select_related() manually. Automatic
select_related/prefetch_related could be good for other use cases,
though.

Some additional findings:
  - The get_by_natural_field() manager method has a bug: getattr(self,
'USERNAME_FIELD', ...) should be getattr(self.model,
'USERNAME_FIELD', ...)
  - Do we want to future-proof some of the interface methods by adding
**kwargs to the method signatures? At least has_perm /
has_module_perms seem like possible candidates.

I wonder if we want to do something about allowing easier password
handling in (admin)forms for custom user classes. Currently making an
admin class for your user class is somewhat hard - you need to either
dig out what auth.admin does for the password field or reinvent it
yourself.

In total this is looking really good, but I can see the need to do
additional polish for some time. Maybe we could take a somewhat
relaxed approach to what constitutes a feature addition when it comes
to this feature? To me it seems there might be something we want to do
about the password forms for example. But, I am not sure we can get
that something done about this by the end of month...

 - Anssi
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/16/12 5:50 AM
On Sun, Sep 16, 2012 at 8:15 PM, Anssi Kääriäinen
<anssi.ka...@thl.fi> wrote:
> On 16 syys, 02:15, Russell Keith-Magee <russ...@keith-magee.com>
> wrote:
> Creating an admin class for no-added-fields extended user model is
> somewhat straightforward, although the admin forms currently assume
> the auth.User as the base user. I did a little hack to make it easier
> to use these forms, see commit:
> https://github.com/akaariai/django/commit/04ae5183df8613fd0d91b43834a4582fdf6b0d04
> - but I am not totally happy about this approach. If you use an
> AbstractBaseUser derived user model, these forms aren't actually
> usable.
>
> Creating an admin class for added fields model is also somewhat easy,
> you just need to play with UserAdmin.fieldsets to have the additional
> fields actually present in the edit form, and you need to make your
> required fields "blank=False, null=True", so that the two-stage user
> creation works correctly.

I'll take a closer look at this patch; however, see my comments below
about admin integration.

> Some additional findings:
>   - The get_by_natural_field() manager method has a bug: getattr(self,
> 'USERNAME_FIELD', ...) should be getattr(self.model,
> 'USERNAME_FIELD', …)

Good catch - I've just pushed a fix.

>   - Do we want to future-proof some of the interface methods by adding
> **kwargs to the method signatures? At least has_perm /
> has_module_perms seem like possible candidates.

I'm not convinced by this -- at least, not for the permissions
methods. The API for module permissions isn't a new invention -- it's
something that's been part of the auth backend API since pretty much
day 1, and the only modification that has been required is to support
object permissions.

If there's any other API where you think there's room for expansion,
I'm open to discussion.

> I wonder if we want to do something about allowing easier password
> handling in (admin)forms for custom user classes. Currently making an
> admin class for your user class is somewhat hard - you need to either
> dig out what auth.admin does for the password field or reinvent it
> yourself.
>
> In total this is looking really good, but I can see the need to do
> additional polish for some time. Maybe we could take a somewhat
> relaxed approach to what constitutes a feature addition when it comes
> to this feature? To me it seems there might be something we want to do
> about the password forms for example. But, I am not sure we can get
> that something done about this by the end of month...

We could easily hold up this patch on making the admin forms
completely flexible for all possible user models. However, for my
money, we still have a couple of months to nail down any problem areas
in the API, and we'll get more benefit from having this in the trunk
repo and getting more eyeballs looking at it than we will from
endlessly polishing behind the scenes aiming at some Platonic ideal.

However, that said -- even if we don't make any significant changes
between now and 1.5 final, it's now *possible* to write and use a
custom User model that is compatible with admin. That's the big step
IMHO, and the piece that needs to land for the alpha. We can continue
to improve forms and documentation until the 1.5 release, and we can
continue to improve post-release. I'm not denying that there is room
for improvement, but the problems you're talking about are on the
scale of "making difficult things easier", not "making impossible
things possible".

Russ %-)
Re: #3011 - Custom User Models -- Call for final review Anssi Kääriäinen 9/16/12 6:13 AM
On 16 syys, 15:50, Russell Keith-Magee <russ...@keith-magee.com>
wrote:
> We could easily hold up this patch on making the admin forms
> completely flexible for all possible user models. However, for my
> money, we still have a couple of months to nail down any problem areas
> in the API, and we'll get more benefit from having this in the trunk
> repo and getting more eyeballs looking at it than we will from
> endlessly polishing behind the scenes aiming at some Platonic ideal.

My intention wasn't to hold this patch out until it is perfect - I
meant to say that after this is committed to 1.5, should we allow for
some polishing changes to get in after feature freeze even if they are
technically feature additions. Though, I can see a point in shipping
with what we get into 1.5 by the end of month, collect feedback and
then improve things for 1.6...

I am definitely +1 to committing this before feature freeze.

 - Anssi
Re: #3011 - Custom User Models -- Call for final review Karen Tracey 9/16/12 8:58 AM
On Sun, Sep 16, 2012 at 9:13 AM, Anssi Kääriäinen <anssi.ka...@thl.fi> wrote:
I meant to say that after this is committed to 1.5, should we allow for
some polishing changes to get in after feature freeze even if they are
technically feature additions.

The kinds of changes you are describing can, I believe, be made after alpha (i.e. feature freeze). Major features like this one may very well need some additions/polishing after landing on master, once many more people start to experiment with them and find out what is needed/useful/etc. The intent of "feature freeze" is to stabilize the code base in general and not introduce side-effect bugs that may go unnoticed until after release if made late in a release cycle.

For big brand-new features, sticking to a hard "feature freeze" line after alpha is more likely to be harmful than helpful, and I don't believe we've ever taken that sort of hard line for major features. It's even possible (though clearly not desirable) to introduce backwards-incompatible changes in the public interfaces for a new feature between alpha/beta/final. In this phase we're seeking to gather feedback from early adopters and experimenters, and not being able to incorporate that feedback into the in-progress release would be silly.

Karen
Re: #3011 - Custom User Models -- Call for final review Ben Slavin 9/18/12 6:13 PM
Hi Russ,

First, let me apologize for being a bit late to the party on this. If there's been prior discussion of any of the points below kindly tell me to get stuffed and so shall I do.


Our team has been working with the t3011 branch today.

We ran into some trouble running tests locally. The tests were in our own project (not django), but BaseDatabaseIntrospection.sequence_list() was listing sequences for all models, and not taking into account that some might be swappable. Josh Ourisman threw together a patch to fix this. Russ: You should have seen a pull request, but the diff is available at https://github.com/joshourisman/django/commit/ef46bbb5520499ebcda2e074446d2c4a055dd6e8

We've run into quite a few issues with third party packages that have foreign keys to auth.User. Fixing this may be outside the scope of Django proper, but it will require work with the community. To enable our testing, we've had to fork quite a few apps.

An especially large problem has been with South. Existing migrations will explicitly refer to auth.User. That is to say: a fresh installation on the initial `./manage.py migrate` will behave incorrectly because foreign keys will be pointing to an incorrect model. I inquired over on #django-south and didn't get any response on any possible plans to handle the new use cases introduced by the t3011 changes. Not a core problem, but something we should work with the community on to ensure a smooth path.

Lastly, I haven't seen a path to easily allow third-party apps to gracefully support both The Old Way and The New Way (1.4 and 1.5). It feels a bit wrong, but should we be considering  the addition of get_user_model and settings.AUTH_USER_MODEL to 1.4.x that's hardcoded to contrib.auth.User so third-party apps can rely on the presence of these mechanisms?


All of this aside, this is great work. This has been an itch I've longed to scratch for quite some time, and this work does it more completely and more correctly than my efforts in django-samaritan. Cheers to you, Russ, and to everyone who's been part of forming this solution over the last six years!


Best,
 - Ben
Re: #3011 - Custom User Models -- Call for final review Donald Stufft 9/18/12 6:34 PM
On Tuesday, September 18, 2012 at 9:13 PM, Ben Slavin wrote:
Lastly, I haven't seen a path to easily allow third-party apps to gracefully support both The Old Way and The New Way (1.4 and 1.5). It feels a bit wrong, but should we be considering  the addition of get_user_model and settings.AUTH_USER_MODEL to 1.4.x that's hardcoded to contrib.auth.User so third-party apps can rely on the presence of these mechanisms?

I don't think adding a hardcoded AUTH_USER_MODEL is going to give the kind of coverage people would want. Likely the best way would be for people to write a wrapper function around the relevant methods/settings and include it in their own projects. This sucks for duplication across all those projects but otherwise it means they'll only be able to support the latest 1.4.x and 1.5+. Likely to be a better user compatibility story by handling the fallback on a per app basis (Django could provide examples though).
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/18/12 6:39 PM
On Wed, Sep 19, 2012 at 9:13 AM, Ben Slavin <benjami...@gmail.com> wrote:
> Hi Russ,
>
> First, let me apologize for being a bit late to the party on this. If
> there's been prior discussion of any of the points below kindly tell me to
> get stuffed and so shall I do.

You asked for it… :-)

> Our team has been working with the t3011 branch today.
>
> We ran into some trouble running tests locally. The tests were in our own
> project (not django), but BaseDatabaseIntrospection.sequence_list() was
> listing sequences for all models, and not taking into account that some
> might be swappable. Josh Ourisman threw together a patch to fix this. Russ:
> You should have seen a pull request, but the diff is available at
> https://github.com/joshourisman/django/commit/ef46bbb5520499ebcda2e074446d2c4a055dd6e8

I've seen the pull request. It looks good on first inspection; lI'll
try and find some time tonight to merge it in.

> We've run into quite a few issues with third party packages that have
> foreign keys to auth.User. Fixing this may be outside the scope of Django
> proper, but it will require work with the community. To enable our testing,
> we've had to fork quite a few apps.

Ok - this is how the migration path *should* work (at least, this is
how it was planned to work):

If your project is using the default User, nothing changes. Third
party apps have an explicit foreign key to User, and they continue to
work.

If your project specifies a custom user, you should get validation
warnings saying that there are foreign keys to a swapped model (and
indicate which foreign keys are affected).

This does means that there is a "1.5 compatible" barrier to entry for
apps. However, the barrier is clearly marked and validated, it only
matters if you want to use the 1.5-specific "custom User" feature, and
the migration path should be fairly simple. The update verges on being
fixable with a regex, but not quite, since you really do need to
engage a brain in the process to make sure you haven't got any implied
User interface contracts.

Yes, this does mean we need to work with the community. The 1.5
release notes do mention this, but would warrant making a bit more
noise about this when we do the release -- possibly a "If you read
nothing else, read this" section of the release notes.

> An especially large problem has been with South. Existing migrations will
> explicitly refer to auth.User. That is to say: a fresh installation on the
> initial `./manage.py migrate` will behave incorrectly because foreign keys
> will be pointing to an incorrect model. I inquired over on #django-south and
> didn't get any response on any possible plans to handle the new use cases
> introduced by the t3011 changes. Not a core problem, but something we should
> work with the community on to ensure a smooth path.

Agreed - migrations are going to be a problem. That's why the docs
currently say "get this right before you sync". I'm open to
suggestions on how to make this more prominent/obvious.

> Lastly, I haven't seen a path to easily allow third-party apps to gracefully
> support both The Old Way and The New Way (1.4 and 1.5). It feels a bit
> wrong, but should we be considering  the addition of get_user_model and
> settings.AUTH_USER_MODEL to 1.4.x that's hardcoded to contrib.auth.User so
> third-party apps can rely on the presence of these mechanisms?

This is a good suggestion, and one that has some precedent with other
features we've introduced in the past.

> All of this aside, this is great work. This has been an itch I've longed to
> scratch for quite some time, and this work does it more completely and more
> correctly than my efforts in django-samaritan. Cheers to you, Russ, and to
> everyone who's been part of forming this solution over the last six years!

You're most welcome. Like you've said, my contribution is really only
the last little part -- this has been a work in various forms of
progress for a long time, and I've certainly benefited from the
experiences of others.

Russ %-)
Re: #3011 - Custom User Models -- Call for final review Ben Slavin 9/18/12 7:24 PM
If your project specifies a custom user, you should get validation
warnings saying that there are foreign keys to a swapped model (and
indicate which foreign keys are affected).

Indeed I did. This was greatly appreciated.
 
This does means that there is a "1.5 compatible" barrier to entry for
apps. However, the barrier is clearly marked and validated, it only
matters if you want to use the 1.5-specific "custom User" feature, and
the migration path should be fairly simple. The update verges on being
fixable with a regex, but not quite, since you really do need to
engage a brain in the process to make sure you haven't got any implied
User interface contracts.

You're right that my concern overlooks the case where reliance on the User goes beyond a mere ForeignKey, but I've found that in many cases usage is restricted to this simple interface.

Maybe this is just a case of "sometimes life sucks", but I felt like I was wasting bits by forking a handful of packages for testing purposes only to change one line per project. I don't believe that there's a universal solution here and I'll respect that the current decision correctly chooses one pain over another.
 
Yes, this does mean we need to work with the community. The 1.5
release notes do mention this, but would warrant making a bit more
noise about this when we do the release -- possibly a "If you read
nothing else, read this" section of the release notes.

My concern here is that we're creating a situation where "if you try to use custom User models on day 1, don't plan to use third party packages that don't have proactive management". As above, this may be unavoidable, but it's unfortunate.
 
> An especially large problem has been with South. Existing migrations will
> explicitly refer to auth.User. That is to say: a fresh installation on the
> initial `./manage.py migrate` will behave incorrectly because foreign keys
> will be pointing to an incorrect model. I inquired over on #django-south and
> didn't get any response on any possible plans to handle the new use cases
> introduced by the t3011 changes. Not a core problem, but something we should
> work with the community on to ensure a smooth path.

Agreed - migrations are going to be a problem. That's why the docs
currently say "get this right before you sync". I'm open to
suggestions on how to make this more prominent/obvious.

I don't think the problem here is one of obviousness. I think there are unaddressed problems here that 'get it right before you sync' trivializes.


In this migration, South has created an explicit reference to auth.User. The only two options I see are: 1) fork and manually patch the migrations to point to a custom user model or 2) abandon migrations (./manage.py syncdb --all && ./manage.py migrate fake)

To be clear, the majority of this burden lies outside of Django (at least until we merge the migration branch). I tried to start the dialogue with the South community today, but I think we're well advised to make sure that community is aware of the issues this can create.


As we've reviewed this, aside from the patch mentioned in my previous post, we have found no technical issues. The only concerns that remain from our team are focused on pragmatism and minimizing the burden on external developers.

Best,
   - Ben

Re: #3011 - Custom User Models -- Call for final review Ben Slavin 9/18/12 7:34 PM
I agree that this does limit things to only 1.4.X and 1.5+, but that seems acceptable. I did consider the possibility of not adding this support layer, but after consideration still felt it will justify the effort.

Those apps that require (or choose to offer) a deeper stack of version support can choose to do so, but the pragmatism of making the common case easy (and removing the need for cross-project duplication) seems to justify the unified interface.

Not speaking as a matter of official policy, but running the latest micro version of the previous minor version seems a reasonable requirement for third-party app developers to impose.

 - Ben

Re: #3011 - Custom User Models -- Call for final review Donald Stufft 9/18/12 7:35 PM
On Tuesday, September 18, 2012 at 10:34 PM, Ben Slavin wrote:
Those apps that require (or choose to offer) a deeper stack of version support can choose to do so, but the pragmatism of making the common case easy (and removing the need for cross-project duplication) seems to justify the unified interface.
After I sent that I thought about it more, adding explicit values in 1.4.x doesn't really hurt anything. And the example wrapper functions could still be added to the docs for people who need more.
Re: #3011 - Custom User Models -- Call for final review rizumu 9/28/12 5:14 AM

It is great to see this merged, it has been a long time coming. :)

I would like to add backwards compatibility to some apps and I'm looking for a recommended technique. Could the following, or a better option, find its way into the docs in a section specific to app developers tasked with adding support for auth_user_model. Having a recommendation in the docs would make pull requests with this feature more likely and easier to handle.

try:
    from django.contrib.auth import get_user_model
    auth_user_model = get_user_model()
except ImportError:
    from django.contrib.auth.models import User
    auth_user_model = User

class SomeModel(models.Model):
    author = models.ForeignKey(auth_user_model)

Because this isn't backwards compatible:

class SomeModel(models.Model):
    author = models.ForeignKey(settings.AUTH_USER_MODEL)

best,
Tom

Re: #3011 - Custom User Models -- Call for final review Mark Lavin 9/28/12 5:38 AM
I've been thinking about external app compatibility as well and it seems like:

from django.conf import settings

AUTH_USER_MODEL = getattr(settings, 'AUTH_USER_MODEL', 'auth.User')

class SomeModel(models.Model):
    author = models.ForeignKey(AUTH_USER_MODEL)

is the easiest solution to me.

Best,

Mark
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 9/28/12 5:49 AM
On Fri, Sep 28, 2012 at 8:14 PM, rizumu <funk...@gmail.com> wrote:
>
> It is great to see this merged, it has been a long time coming. :)
>
> I would like to add backwards compatibility to some apps and I'm looking for
> a recommended technique. Could the following, or a better option, find its
> way into the docs in a section specific to app developers tasked with adding
> support for auth_user_model. Having a recommendation in the docs would make
> pull requests with this feature more likely and easier to handle.

Adding a backwards compatibility shim has already been discussed (a
few messages earlier in this thread). The plan is to back port two
parts of the #3011 patch to 1.4:

 * The global setting AUTH_USER_MODEL = 'auth.User'

 * An implementation of get_user_model() that always returns auth.User.

This means that when 1.5 is released (and we release 1.4.2), the same
User access code will run on the current stable and security versions.
I haven't added this code to the 1.4 branch yet, but my intention is
to do so before the 1.5 release is finalised.

Yours,
Russ Magee %-)
Re: #3011 - Custom User Models -- Call for final review Enrico B. da Luz 10/22/12 5:12 PM
That's a great addition to the project! Congratulations.

I couldn't hold myself from adding my two cents to this topic.

Having the custom user model available on "django.contrib.auth.models.User" would be transparent for third party apps and south migrations.

This is kinda the way django-shop solves the problem of customizing its models.
Custom models can be imported both from its custom location or from the original (canonical) module.

They have a base abstract model and an isolated concrete implementation that is used as a fallback if no custom model is configured on project's settings.

Best regards,
Enrico
Re: #3011 - Custom User Models -- Call for final review Russell Keith-Magee 10/27/12 5:05 PM

On Sat, Oct 27, 2012 at 7:25 PM, Ludwig Kraatz <lud...@hotmail.com> wrote:
Hi Russ,
 

>   - The last_login field is in the AbstractBaseUser, but it isn't
> documented as a required field. Is this field required for something?
> Is it needed as part of AbstractBaseUser?

Yes, last_login is required - it's needed in order to generate
password reset tokens etc.

It isn't documented because we *really* want people to be subclassing
AbstractBaseUser, not building their own User from scratch.


I totally understand u want people to subclass the AbstractBaseClass. But what if some Django based system just doesn't need any passwords at all? If All authentification might be handled by LDAP, SSO or whatever..
It might be *ok* then to have a password field for every user just set to *unusable* (but not nice at all..). But then having the user table always being updated on logins is just not necessary. If one (me) maybe even wants to store recent actions - as logins - in a completely different app / table / db - than the user table would definitely benefit from not being accessed without any need on every login.

So I really don't think its the best way to force any subclassed model to use the last_login (and password) field. Why not having an AbstractPasswordUser and an 
 * AbstractNaked-I-Dont-Have-Anything-except-a-username-User?

I really appreciate you're offering the possibility of having Custom Users this easy now, but please think again about the password and especially the last_login field.

Thanks for your feedback. However, I'm going to say no in this case, for two reasons.

Firstly, a framework like Django can't ever hope to suppose *every* possible use case. That's one of the side effects of being a framework; in order to make the framework easily usable in 90% of cases, and possible to use in 95% of cases, you have to throw 5% of cases overboard. That's unfortunate, but it's also the price you pay for having something easy to use for 90% of use cases.

Secondly, because this is Python, there's actually nothing preventing you from doing exactly what you describe in your own code. Although Django provides an AbstractBaseUser, there are any explicit type checks for that specific class. We rely on duck typing when the implemented class is actually used. The base class is just an implementation assistant, providing the common pieces that most users will need (and, in the case of password, we explicitly *don't* want them implementing themselves).

So - that means that if you want to write a User object that has no password field, you can do that; the only catch is that you'll need to make sure you don't ever hit a line of code that expects the existence of a password field. That means none of the login forms will work, none of the user creation forms (or createsuperuser) will work, and you'll need to write your own authentication backend. However, the important part of the swappable User mechanic -- the ability to make ForeignKey/M2M relations with *your* User model -- won't be encumbered at all. As far as I'm aware, with the exception of createsuperuser, all these password-dependent places should be user-configurable; so, for example, you can specify your own login form and template for the login view.

If I'm mistaken on any of this, I'm certainly open to patches that to remove or weaken the dependency on a specific password field. However, I don't think we need to ship a super-minimal User base class, if for no other reason that we *really* want to discourage people from writing their own implementation of a password field.

Yours,
Russ Magee %-)

Re: #3011 - Custom User Models -- Call for final review Ludwig Kraatz 10/28/12 5:54 AM
Yeah i got your point and thats totally fine - i'll definitely find a workaround ;-)

Just found a way to describe my point of view in a little different way:
* Why is there a need to have Authentication and Authorization in one App - when both answer a totally different purpose? *

But the answer - as you said - it might be the more convenient way in 90% of the use cases... And because of that - easier to use.  
=> right..? Just want to make sure thinking abt it this way brings you to the same answer.. I'm a little biased, because for me the union of both is unfavorable.


So thanks you took the time to think abt it. Nice job anyway!

best regards
ludwig
Re: #3011 - Custom User Models -- Call for final review Paul McMillan 10/28/12 11:11 AM
On Sun, Oct 28, 2012 at 8:54 AM, Ludwig Kraatz <lud...@hotmail.com> wrote:
> Just found a way to describe my point of view in a little different way:
> * Why is there a need to have Authentication and Authorization in one App -
> when both answer a totally different purpose? *

> But the answer - as you said - it might be the more convenient way in 90% of
> the use cases... And because of that - easier to use.
> => right..?

You nailed it. In systems which separate the two, it's extremely
common for developers to mistake one for the other, and build insecure
systems. By providing both together by default, developers who have
never considered the fact that the two concepts can be separate will
do the right thing, and the 90% case for the rest of us of "I need
both together" is also met.

-Paul