Code

Opened 15 months ago

Closed 15 months ago

Last modified 13 months ago

#19753 closed Uncategorized (needsinfo)

Custom user system shouldn't be dependent on all installed apps

Reported by: abraham@… Owned by: nobody
Component: contrib.auth Version: 1.5-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

function calls: 'get_user_model' => 'get_model' => _populate

_populate loads all installed apps. It seems like get_user_model should only rely on the app defining the custom user model.

Attachments (0)

Change History (14)

comment:1 Changed 15 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

That's the behavior of Django's app loading mechanism. Improving it is a huge endeavor. See #3591.

You didn't describe any concrete drawback that could justify this amount of work.

comment:2 Changed 15 months ago by abraham@…

I don't think get_model should be changed... rather... get_user_model should use something else. Basically, get_user_model can't be called when creating models (IE: Referencing the user object in another model) .

comment:3 Changed 15 months ago by charettes

I know it can't be called from the application containing the swapped for user model

# myapp/models.py
from django.contrib.auth import get_user_model
from django.contrib.auth.models import AbstractUser

class CustomUser(AbstractUser):
    pass

get_user_model() # Will fail because the app "myapp" is not installed

But that's not really an issue since you can refer to it directly.

The main problem is that every third-party that has to call get_user_model (signal attaching for example) must be placed after 'myapp' in your INSTALLED_APPS.

As pointed out by @aaugustin this a limitation of the current app loading mechanism.

Version 0, edited 15 months ago by charettes (next)

comment:4 Changed 15 months ago by anonymous

Right. I ran into this problem when updating django-guardian for my own purposes. Any third-party module that is dependent on a custom User model will have problems. I see what you guys are saying about the app loading mechanism. Is there no way to lazily load apps?

comment:5 Changed 15 months ago by abraham@…

I agree with what both of the guys above me are saying. I too ran into this problem when updating a third-party module. It will make dependency on a custom user model very difficult.

comment:6 Changed 15 months ago by russellm

@anon - the problem isn't "Any third party module that is dependent on a custom User". You can write a module that has foreign keys to the custom User, for example. What you can't do is have module level initialization logic that depends on the pluggable user. The docs already reference one specific case where this is an issue -- signals -- but if there are other common use cases for this, then perhaps they need to be documented. As @charettes and @aaugustin have pointed out, this is a fundamental problem with the load sequence for Django apps, which should be addressed by #3591.

So - What's the use case (beyond signals) that requires a module-level reference to the swappable User model?

comment:7 Changed 15 months ago by anonymous

The error I'm seeing is with the admin module and django-guardian, which supposedly has Django 1.5 support. It has a foreign key reference to the custom user model somewhere, but the very fact that it programatically chooses which User model to use seems to be the problem. I'll get the trace for you guys later tonight. It may very well be something I am doing wrong.

comment:8 Changed 15 months ago by abraham@…

Hey anonymous... I think we're running into the same issue... here's my guardian setup error:

ERROR 2013-02-06 01:26:23,372 mlogging 3343 139807877195520 Traceback (most recent call last):
  File "/site-packages/django/core/handlers/wsgi.py", line 255, in __call__
    response = self.get_response(request)
  File "/site-packages/django/core/handlers/base.py", line 178, in get_response
    response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
  File "/site-packages/django/core/handlers/base.py", line 220, in handle_uncaught_exception
    if resolver.urlconf_module is None:
  File "/site-packages/django/core/urlresolvers.py", line 342, in urlconf_module
    self._urlconf_module = import_module(self.urlconf_name)
  File "/site-packages/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "./urls.py", line 4, in <module>
    admin.autodiscover()
  File "/site-packages/django/contrib/admin/__init__.py", line 29, in autodiscover
    import_module('%s.admin' % app)
  File "/site-packages/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/site-packages/guardian/admin.py", line 13, in <module>
    from guardian.forms import UserObjectPermissionsForm
  File "/site-packages/guardian/forms.py", line 4, in <module>
    from guardian.shortcuts import assign
  File "/site-packages/guardian/shortcuts.py", line 11, in <module>
    from guardian.core import ObjectPermissionChecker
  File "/site-packages/guardian/core.py", line 6, in <module>
    from guardian.utils import get_identity
  File "/site-packages/guardian/utils.py", line 19, in <module>
    from guardian.compat import AnonymousUser
  File "/site-packages/guardian/compat.py", line 12, in <module>
    User = get_user_model()
  File "/site-packages/django/contrib/auth/__init__.py", line 125, in get_user_model
    user_model = get_model(app_label, model_name)
  File "/site-packages/django/db/models/loading.py", line 230, in get_model
    self._populate()
  File "/site-packages/django/db/models/loading.py", line 75, in _populate
    self.load_app(app_name)
  File "/site-packages/django/db/models/loading.py", line 96, in load_app
    models = import_module('.models', app_name)
  File "/site-packages/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/site-packages/guardian/models.py", line 11, in <module>
    from guardian.compat import User
ImportError: cannot import name User

comment:9 Changed 15 months ago by charettes

This `ForeignKey` should reference getattr(django.conf.settings, 'AUTH_USER_MODEL', 'auth.User'). This looks like the culprit.

comment:10 Changed 15 months ago by abraham@…

I'll give this a shot after work!

comment:11 Changed 15 months ago by abraham@…

Just tried it.

It seems like it has nothing to do with the foreign key reference. More to do with the importing using get_user_model() and inclusion of admin module (autodiscover maybe?).

comment:12 Changed 15 months ago by anonymous

To be more clear... it looks like admin.autodiscover() is executed before the models are imported. The guardian/models.py and guardian/admin.py. files are dependent on compat.py. I guess get_user_model() should not be called when importing any models.py files.

comment:13 Changed 15 months ago by daniel

Creating circular references is all too easy. If you want to avoid a lot of pain going forward, you're going to want to take a serious look at this issue.

comment:14 Changed 13 months ago by Fernando Rocha <fernandogrd@…>

This is a very annoying edge case.
It is hard to debug and third party apps are making the mistake of using get_user_model with ManyToManyField (even with warning in docs).

I would prefer that get_user_model did not depend on INSTALLED_APPS and deal with problems caused by defining AUTH_USER_MODEL without app_label in INSTALLED_APPS.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.