Inconsistency with permissions for proxy models

[Dave Hall]

Summary: The content type framework treats proxy models and their parent as the same model. The permissions framework treats them as separate models.

This is complicated, and best explained by example. First of all, create this model:

class UserProxy(User):
    class Meta:
        proxy = True
        app_label = "foo"

This will result in three new permission objects being created: "auth.add_userproxy", "auth.change_userproxy", "auth.delete_userproxy"

However, when the admin site checks for permissions for this model, it will be looking for permissions called: "foo.add_userproxy", "foo.change_userproxy", "foo.delete_userproxy"

The inconsistency lies in that permissions are created using the ContentType of the proxy model's parent class. However, permissions are checked using the app_label of the proxy model itself.

In order to reconcile this, there appear to be two options:

• Allow each proxy model to have it's own ContentType.
• Make ProxyModel._meta.app_label return the app label of the parent model.

Personally, it makes sense to me that each proxy model has its own ContentType. Since proxy models can represent subsets of the parent model, they require different admin permissions. In order to have different admin permissions, they need to have their own content types.

The current situation is a horrible mess of both approaches. The content type framework treats proxy models as being the same as their parent model. The permissions framework treats them as different.

I realise that this is something of a design decision for the framework. I leave it up to greater minds than mine to come up with a solution!

[Jacob]

For 1.1 we're going to need to simply say "don't do that" -- setting a different app_label is confusing things. For 1.2 we'll need to work this out better.

[Dave Hall]

Unfortunately, it's not that simple. Setting the app_label is not required to replicate this bug.

All you need to do is create a new application with a proxy model that references a model in another application. The two models then have different app_labels, and the problem occurs.

For example:

# staff/models.py
class UserProxy(User):
    class Meta:
       proxy = True

This is a pretty trivial use of proxy models, yet it does not work with the admin system. Not good.

[Russell Keith-Magee]

This really isn't a multidb problem.

[Walter Doekes]

Workaround for issue 11154

[Walter Doekes]

We've successfully employed the workaround as found in osso.core.management.init.py on django-1.1.

Regards,
Walter Doekes
OSSO B.V.

[Walter Doekes]

(Although I must add that we do not use the admin interface for users, so I cannot confirm or deny that it solves issues in the admin interface.)

[macakraca]

It may be obvious how to implement this, but for the rest of us a quick rundown.

• paste patch in my_app/init.py
• run ./manage.py syncdb

[Russell Keith-Magee]

We've lived with this for the duration of 1.2; it can wait a little longer.

[Malcolm Tredinnick]

Proxy models should also proxy the permissions of the model they are shadowing. So any app_label check should first check if the instance is a proxy and look in the parent's app_label instead.

[Simon Meers]

Proxy models should also proxy the permissions of the model they are shadowing

Are you sure about that Malcolm? Proxy models could be quite useful for limiting permissions to a subset of objects.

For example, I could make a Student proxy for User with a manager whose get_query_set returns only Users in the "student" Group, and a save method which automatically assigns Users to that group. What if I'd then like to restrict Teachers to only being able to add/change/delete Students and not Users in general?

[empty]

Replying to DrMeers:

Are you sure about that Malcolm? Proxy models could be quite useful for limiting permissions to a subset of objects.

That's my general use case.

[Paul Garner]

This patch worked well for me, successfully using it in a production website where we needed a proxy on the User model without breaking the admin site.

[Russell Keith-Magee]

#14492 and #15054 describe very closely related problems.

[Jacob]

Milestone 1.3 deleted

[danny.adair@…]

I also think that Proxy models should have their own permissions. The current problem seems to lie in the way that ContentTypeManager determines the app_label of a model:

ModelBase will - proxy or not - report the app_label either as explicitly specified or from the parent's module name.
https://code.djangoproject.com/browser/django/trunk/django/db/models/base.py#L50

if getattr(meta, 'app_label', None) is None:
    # Figure out the app_label by looking one level up.
    # For 'django.contrib.sites.models', this would be 'sites'.
    model_module = sys.modules[new_class.__module__]
        kwargs = {"app_label": model_module.__name__.split('.')[-2]}
    else:
        kwargs = {}

However, ContentTypeManager._get_opts() traverses proxies up to the non-proxy model and then uses the entire meta of that model, incl. app_label.
https://code.djangoproject.com/browser/django/trunk/django/contrib/contenttypes/models.py#L18

def _get_opts(self, model):
    opts = model._meta
    while opts.proxy:
        model = opts.proxy_for_model
        opts = model._meta
    return opts

This is where app_label needs to be preserved. Maybe just put it aside, traverse, then readjust:

def _get_opts(self, model):
    opts = model._meta
    # Preserve proxy model's attr_label
    attr_label = opts.attr_label
    while opts.proxy:
        model = opts.proxy_for_model
        opts = model._meta
    opts.attr_label = attr_label
    return opts

[danny.adair@…]

Hmmm, on second thought - looking at what parts of _meta are actually used by ContentTypeManager - maybe it shouldn't traverse at all, just stick with the proxy model itself...

[German M. Bravo]

I'm working in polymorphic models (django_polymorphic) and I'm adding proxied polymorphic models... this is also a problem in this case, where I'm in the need to have content types for proxied models as well.

[Simon Charette]

I think that allowing `ContentTypeManager.get_for_model` to return a proxy model (using a boolean kwarg to bypass the ContentTypeManager._get_opt part) could fix issues. Defaulting this boolean flag to false would preserve backward compatibility.

The auth framework could then use that flag to create the missings permissions for proxy model. Plus it would allow polymorphic solutions that saves the object ct to be retreived calling ContentTypeManager.get_for_model('app', 'label', True). I think the addition of this new feature/approach should be discussed on django-dev since it involves a design decision.

[danny.adair@…]

I would consider it a bug if two different places give two different app_label's for the same model, and fixing it _will_ break backwards compatibility. Imho the only question is for which of the two.

[Daniel Sokolowski]

I have found a permission on proxy models related bug, the admin back end fails to display the administrative options for proxy models.

I have a proxy model extending the Flatpage model, I unregistered the ModelAdmin for it and registered my custom one. I assigned 'change_myflatpage', and 'edit_myflatpage' to the user hover the django admin fails to have any options for my custom flatpage model; If I make the user a super user the edit options appear.

Attempting to access the url directly (/admin/flatpages_ext/projectflatpage/) throws a 403 error.

If there is anything else that you guys need please advise.

[danny.adair@…]

@danols: That's exactly the inconsisteny described in this ticket. The "change_myflatpage" permission has the app label of your class ("yourapp.change_myflatpage") but when django admin checks it will use the app_label of the parent class ("contrib.change_myflatpage"). Look at the database tables "auth_permission" and "django_content_type". In the latter, your proxy model is listed with "myapp" as the app_label. When django admin checks the permission though, it traverses up to the parent of myflatpage and then uses _that_ app_label, and a combination of "contrib" and "change_myflatpage" is not in your permissions table.

[danny.adair@…]

Just wanted to add that etianen the ticket reporter is the author of django-reversion and this inconsistency breaks real-life use cases, e.g. ​https://github.com/etianen/django-reversion/issues/94

It may be a matter of discussion whether proxy models should have their own permission or not (though I personally find that a no-brainer), but really the issue is that two different vital locations report two different "app_label" for the same class. I don't see a valid argument for an explicitly specified "app_label" of a proxy model being reported as its parent (root) model's "app_label".

[Simon Charette]

Here's the ​actual approach I was talking about at comment:27.

It solves the issue but proxy_for_model kwarg is really an odd name, anyone thinking of something better?

It doesn't introduce any backward incompatibly issues since the admin relied on the opts of the proxy for the has_perm checks. In other words, ModelAdmin registered with a proxy model couldn't be accessed at all if you weren't a superuser thus approach such as ​this one wouldn't actually work.

It might also be worth documenting that new kwarg since it can be quite useful when using the ContentType framework. The fact that you can now add permissions for proxy models can be quite handy (see comment:12) and IMHO is also worth documenting.

All those features need extra testing and doc that I'll be happy providing if I can get some feedback toward my approach.

[danny.adair@…]

follow_proxy, traverse_proxy? (or even _proxies)

The default behaviour would still be "broken" - ModelBase and ContentManager would still be giving different stories regarding app_label

[Simon Charette]

Replying to danny.adair@…:

follow_proxy, traverse_proxy? (or even _proxies)

The default behaviour would still be "broken" - ModelBase and ContentManager would still be giving different stories regarding app_label

Defaulting follow_proxy (like that one :) to False would unify both API but would break backward compatibility.

I ran the test with follow_proxy=False as default and only ​two tests breaks. However apps relying on contrib.ContentType in the wild could break badly.

i.e. (​django-taggit) If you defined a TaggableManager() (uses GenericForeignKey internally) on a proxy model then all tags will be associated with the concrete model ContentType in the db. If we release follow_proxy=False as default then the generic relation between the proxy model and the tags will be broken since they were never associated with the proxy's ct.

We can either break the API to unite them or offer the user to opt-in in order to maintain backward compatibility. But this whole discussion is getting bigger then the ticket itself. We should maybe open another issue to fix the ContentType proxy issue that blocks this one and then, once it's fixed, use the new hooks to fix this one with:

diff --git a/django/contrib/auth/management/__init__.py b/django/contrib/auth/management/__init__.py
index 78a51cf..a1d2d9e 100644
--- a/django/contrib/auth/management/__init__.py
+++ b/django/contrib/auth/management/__init__.py
@@ -31,7 +31,8 @@ def create_permissions(app, created_models, verbosity, **kwargs):
     searched_perms = list()
     # The codenames and ctypes that should exist.
     ctypes = set()
-    ctypes_for_models = ContentType.objects.get_for_models(*app_models)
+    ctypes_for_models = ContentType.objects.get_for_models(*app_models,
+                                                           follow_proxy=False)
     for klass, ctype in ctypes_for_models.iteritems():
         ctypes.add(ctype)
         for perm in _get_all_permissions(klass._meta):

[danny.adair@…]

Replying to charettes:

[...]

Defaulting follow_proxy (like that one :) to False would unify both API but would break backward compatibility.

[...]

I ran the test with follow_proxy=False as default and only ​two tests breaks. However apps relying on contrib.ContentType in the wild could break badly.
[...]
We can either break the API to unite them or offer the user to opt-in in order to maintain backward compatibility. But this whole discussion is getting bigger then the ticket itself. We should maybe open another issue to fix the ContentType proxy issue that blocks this one and then, once it's fixed, use the new hooks to fix this one with:

I agree an implementation in two steps would help everyone (and faster to get checked in).
Just want to point out that if you consider this a bug (like I do) then backwards-compatibility (= keep software happy that relies on the bug) is a less strong argument for retaining it. Right the wrong or continue its adoption? django-reversion is _currently_ broken.

[Simon Charette]

Replying to danny.adair@…:

Replying to charettes:

[...]

Defaulting follow_proxy (like that one :) to False would unify both API but would break backward compatibility.

[...]

I ran the test with follow_proxy=False as default and only ​two tests breaks. However apps relying on contrib.ContentType in the wild could break badly.
[...]
We can either break the API to unite them or offer the user to opt-in in order to maintain backward compatibility. But this whole discussion is getting bigger then the ticket itself. We should maybe open another issue to fix the ContentType proxy issue that blocks this one and then, once it's fixed, use the new hooks to fix this one with:

I agree an implementation in two steps would help everyone (and faster to get checked in).
Just want to point out that if you consider this a bug (like I do) then backwards-compatibility (= keep software happy that relies on the bug) is a less strong argument for retaining it. Right the wrong or continue its adoption? django-reversion is _currently_ broken.

I'm in the process of writing a new ticket describing the issue in which I'll suggest a design decision is taken toward breaking backward compatibility.

[Simon Charette]

Ticket #17648 fixes this issue (see attached path there). I finally agree that this is nothing but a bug that was introduced back in r10523 for ticket #10738.

[Simon Charette]

While this gets fixed I wrote down a snippet that creates permissions without modifying django's code base ​http://djangosnippets.org/snippets/2677/.

[Simon Charette]

Now that #18399 is fixed, I created a ​pull request that fixes this bug and bring back the get_for_models permission creation optimization reverted back in #17904. I'm still wondering if this needs any doc? I guess a release note wouldn't hurt?

[Anssi Kääriäinen]

I would really like to see a release note for this. I am currently looking at the pull request and wondering what exactly will change. I am going to close the pull request (per the policy of commit or close on review). Reopen with the added docs. I have a suspicious feeling I should know by now what this changes, but can't remember...

[clay.evil@…]

hi, after reading all the comments, i'm not sure if this is fixed or not in 1.4, however i have found another issue with this.
I have tracked it to sites.py line 338 has_module_perms = user.has_module_perms(app_label).

The problem is that the admin site won't even start to look for model_admin.get_model_perms(request) because i don't have permission to the "foo" module (as the permissions created are under "auth".

I know you busy, but this I think is a serious issue if someone has to make use of the build in admin site, and won't have an superuser (because an superuser is very evil).

Thank you.

[anonymous]

Has a fix for this made its way into 1.5?
I'm still using 1.4 ATM, and have just had this bug smack me upside the head.

[Simon Charette]

No fix made it to 1.5. I'll try to rebase the patch and add a release note.

I'll have to test possible issues with this, to make sure this doesn't introduce security issues related to permissions. As far as I can see there should be none in the admin since no permissions were created for proxy models, I'll have to checkout the other various permission API.

We should also document how to create those missing proxy model permissions by running syncdb.

[Kegan Gan]

Seems like not fix in 1.6 still. Just encountered this bug. Any idea if it could be fixed in 1.6 point releases?

[anonymous]

Using 1.6 and can confirm this is still an issue. Any idea what's the latest on this? Thanks,

[jorgecarleitao]

Ticket #22270 proposes to document how permissions work in proxies. I think the PR for this ticket could contain that documentation.

@charettes, do you want help writing the docs? It would be nice to finish this now that most of the work is done.

I also suggest having a regression test for this.

[Simon Charette]

Hi @jorgecarleitao, unfortunately don't have time to work on this ticket in the next few weeks so I'll deassign myself. Feel free to assign it to yourself.

I think the biggest part of this patch is to figure out how to enable proxy models permissions in a backward compatible way without introducing any security issues in applications relying on the existing behavior.

Since this ticket has been opened for 5 years it wouldn't hurt to document the actual caveats of the implementation meanwhile. Even if the ticket gets fixed in this release cycle we should backport the documentation for all supported versions.

[Cho Garcia]

Any updates on this ticket?

As I commented on Djangocon Europe in Cardiff I found this management command for fixing the proxy models and create their own permissions: ​Gist

Perhaps it helpful for others running on the same issue.

[Tim Graham]

I requested better documentation on the latest ​pull request.

[Tim Graham]

The ​pull request was closed as the submitter couldn't address the security concerns raised there. Someone else is welcome to follow up and try to address them.

[Stephen Wolff]

For anyone else struggling with this, we've solved it by writing migrations to add the permissions (django 1.8) by creating the 'model':

migrations.CreateModel(
            name='XYZUserProxyModel',
            fields=[
            ],
            options={
                'verbose_name': 'User (XYZ User)',
                'managed': False,
                'proxy': True,
                'verbose_name_plural': 'Users (XYZ Users)',
            },
            bases=('proxyapp.user',),
            managers=[
                ('objects', django.contrib.auth.models.UserManager()),
            ],
        ),

[Clayton Daley]

The patch for this issue was rejected due to security concerns. Specifically, the patch was designed to create permissions for proxy models (a necessary condition for solving this issue). Since the behavior was "automatic", a proxy model that was "disabled" due to the bug could be unexpectedly "enabled" by the patch (at least that was the concern).

I don't know the framework well enough to assess the actual security impact of the original change, but would like to propose the following as a less risky path forward:

• Update the patch so that, by default, proxy models have no permissions. This default would be baked into the logic of proxy = True and override anything inherited from a parent Model.
• If a user provides permissions, we assume the user wants permissions for the proxy model and create them (i.e. otherwise using the patch as-is)
• (Optionally) The next time Django puts out a major release (too bad this missed 2.0!) change the default to always create permissions.

This proposal reduces the risk for users:

• If you're not providing permissions on a proxy model -- explicitly or via a Meta inheritance chain -- you will be unaffected. I assume most users fall into this category.
• If you're intentionally adding permissions (indirectly!) by exploiting this bug, something should break when you upgrade Django. This provides sufficient warning of a change to discover the other effects.
• To be affected but not notified, you need to be including duplicate (or otherwise unused) permissions in the Meta class of your proxy model. This should be rare and mostly arise from inheritance in your Meta class (where the duplicate permissions aren't explicit).

[Arthur Rio]

The patch for this issue was rejected due to security concerns. Specifically, the patch was designed to create permissions for proxy models (a necessary condition for solving this issue). Since the behavior was "automatic", a proxy model that was "disabled" due to the bug could be unexpectedly "enabled" by the patch (at least that was the concern).

If I'm not mistaken, I don't think it's entirely true. The patch was designed to recreate permissions using the content type of the proxy model instead of the one of the concrete model. The permissions for the proxy models are already automatically generated. For example, if you proxy a model in the same app (i.e. with the same app_label), you can create and admin page for it and use its permissions just fine. It's only if you create a proxy model in a different app (most common use case is to proxy auth.User), that you can't use the admin + permissions, because of that app_label mismatch when the admin is building the string to lookup the permission.

---

In order to reconcile this, there appear to be two options:

• Allow each proxy model to have it's own ContentType.
• Make ProxyModel._meta.app_label return the app label of the parent model.

The first approach would fix it altogether but is a larger discussion that I think is preventing this ticket to be closed (it's the approach used by the patch).
Instead, I think we should focus on the second recommandation (with some slight modifications) so that the ticket can be closed and the discussion about
content types can happen separately.

---

To help understand the original problem and some of the approaches we can take, I want to make some clarifications with a simple example:

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


class Vehicle(models.Model):
    pass

class Car(Vehicle):
    class Meta:
        proxy = True

class Driver(get_user_model()):
    class Meta:
        proxy = True

The current behavior is to use the concrete model content type to generate the default permissions, so for view it would be:

• myapp.view_vehicle
• myapp.view_car
• auth.view_driver

As you can see the permission for driver is what makes the admin fail to lookup the correct permissions for the admin page of Driver, only a superuser would be able to access it.

---

What I suggest is to refactor django/contrib/admin/options.py to use the following function, that builds the proper app_label from an opts parameter (to follow the same pattern as get_permission_codename).

# django/contrib/auth/__init__.py

def get_permission_app_label(opts):
    """
    Return the app_label of the permission for the specified model.
    Proxy models use the content type of the concrete model.
    """
    return opts.concrete_model._meta.app_label


# django/contrib/admin/options.py
# Replace the many similar calls ('view', 'change', 'add', 'delete', etc.)

- codename = get_permission_codename('add', opts)
- return request.user.has_perm("%s.%s" % (opts.app_label, codename))
+ app_label = get_permission_app_label(opts)
+ codename = get_permission_codename(action, opts)
+ return request.user.has_perm("%s.%s" % (app_label, codename))

By looking at the concrete model (opts.concrete_model._meta.app_label. as opposed to opts.app_label), it would keep the existing behavior intact for other models
(For a concrete model, opts.concrete_model._meta == opts), and make the admin work with proxy model inheriting concrete models from a different app (i.e. with a different app_label).

The other benefit of using this approach as first step to fix the broader issue, is that it centralizes where the change would occur if we decided to go with having a ContentType per proxy model and using that to generate the initial permissions instead of the one of the concrete type.

Finally, regarding the security concerns previously raised, the only scenario that I can think of is the following:

Pre-conditions:

• You have a proxy model whose concrete model has a different app_label
• You gave the permissions for that proxy model to a user
• You registered the model on the admin

Before the patch:
The user wouldn't see or be able to access the admin page, only superusers could.

After the patch:
The user would be able to access the admin page based on its permissions.

---

I will create a PR with this approach, let me know if there are any corner case or security concerns I'm missing.

[Arthur Rio]

As mentioned on the ​pull request, I moved away from my initial comment because:

1. It's only a temporary fix and we would still need to address the bigger issue about how permissions are created for proxy models.
2. It makes it actually more risky in terms of security as users with proxy permissions would suddenly be able to access the admin pages they couldn't see before (even though they should have been in the first place...)

I think I'm pretty close to wrapping up the code changes and I'd love some feedback before I start writing up the docs and the release notes. You can check the ​pull request for more details about the current state of things and how this patch would impact permissions.

[Tim Graham <timograham@…>]

In 181fb601:

Fixed #11154, #22270 -- Made proxy model permissions use correct content type.

Co-Authored-By: Simon Charette <charette.s@…>
Co-Authored-By: Antoine Catton <acatton@…>