Code

Opened 5 years ago

Last modified 4 weeks ago

#11154 new Bug

Inconsistency with permissions for proxy models

Reported by: etianen Owned by:
Component: contrib.auth Version: master
Severity: Normal Keywords: proxy contenttype permission
Cc: omat@…, david@…, walter+django@…, DrMeers, Ciantic, etianen, thepapermen, kmike84@…, danny.adair@…, omer.drow@…, Kronuz, danols@…, charette.s@…, saxix.rome@…, 4glitch@…, maa@…, kegan@…, hv@…, charettes Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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!

Attachments (1)

osso.core.management.__init__.py (2.2 KB) - added by wdoekes 4 years ago.
Workaround for issue 11154

Download all attachments as: .zip

Change History (54)

comment:1 Changed 5 years ago by etianen

  • Cc david@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by jacob

  • milestone changed from 1.1 to 1.2
  • Triage Stage changed from Unreviewed to Design decision needed

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.

comment:3 Changed 5 years ago by etianen

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.

comment:4 Changed 5 years ago by anonymous

  • Version changed from SVN to soc2009/multidb

comment:5 Changed 5 years ago by russellm

  • Version changed from soc2009/multidb to SVN

This really isn't a multidb problem.

Changed 4 years ago by wdoekes

Workaround for issue 11154

comment:6 Changed 4 years ago by wdoekes

  • Cc walter+django@… added

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

Regards,
Walter Doekes
OSSO B.V.

comment:7 Changed 4 years ago by wdoekes

(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.)

comment:8 Changed 4 years ago by 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

comment:9 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

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

comment:10 Changed 4 years ago by DrMeers

  • Cc DrMeers added

comment:11 Changed 4 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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.

comment:12 follow-up: Changed 4 years ago by DrMeers

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?

comment:13 in reply to: ↑ 12 Changed 4 years ago by 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.

comment:14 Changed 4 years ago by omat

  • Cc omat@… added

comment:15 Changed 4 years ago by anentropic

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.

comment:16 Changed 3 years ago by russellm

#14492 and #15054 describe very closely related problems.

comment:17 Changed 3 years ago by Ciantic

  • Cc Ciantic added

comment:18 Changed 3 years ago by etianen

  • Cc etianen added
  • Severity set to Normal
  • Type set to Uncategorized

comment:19 Changed 3 years ago by julien

  • Type changed from Uncategorized to Bug

comment:20 Changed 3 years ago by thepapermen

  • Cc thepapermen added
  • Easy pickings unset
  • UI/UX unset

comment:21 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:22 Changed 3 years ago by kmike

  • Cc kmike84@… added

comment:23 Changed 3 years ago by danny.adair@…

  • Cc danny.adair@… added

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

comment:24 Changed 3 years ago by 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...

comment:25 Changed 3 years ago by the_drow

  • Cc omer.drow@… added

comment:26 Changed 3 years ago by Kronuz

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.

comment:27 Changed 3 years ago by charettes

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.

comment:28 Changed 3 years ago by 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.

comment:29 Changed 3 years ago by Kronuz

  • Cc Kronuz added

comment:30 Changed 3 years ago by danols

  • Cc danols@… added

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' (and even 'change_flatepage'/'add_flatpage') to the user. Howeverr 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.

Last edited 3 years ago by danols (previous) (diff)

comment:31 Changed 3 years ago by 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.

comment:32 Changed 3 years ago by 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".

comment:33 Changed 2 years ago by charettes

  • Cc charette.s@… added

comment:34 Changed 2 years ago by saxix

  • Cc saxix.rome@… added

comment:35 Changed 2 years ago by charettes

  • Needs documentation set
  • Needs tests set

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.

Last edited 2 years ago by charettes (previous) (diff)

comment:36 follow-up: Changed 2 years ago by 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

comment:37 in reply to: ↑ 36 ; follow-up: Changed 2 years ago by charettes

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):
Last edited 2 years ago by charettes (previous) (diff)

comment:38 in reply to: ↑ 37 ; follow-up: Changed 2 years ago by 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.

comment:39 in reply to: ↑ 38 Changed 2 years ago by charettes

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.

comment:40 Changed 2 years ago by charettes

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.

comment:41 Changed 2 years ago by charettes

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

comment:42 Changed 2 years ago by charettes

  • Has patch set
  • Keywords proxy contenttype permission added
  • Needs tests unset

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?

comment:43 Changed 2 years ago by akaariai

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...

comment:44 Changed 23 months ago by 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.

comment:45 Changed 16 months ago by 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.

comment:46 Changed 16 months ago by charettes

  • Owner changed from nobody to charettes
  • Status changed from new to assigned

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.

comment:47 Changed 12 months ago by 4glitch@…

  • Cc 4glitch@… added

comment:48 Changed 7 months ago by maa@…

  • Cc maa@… added

comment:49 Changed 7 months ago by kegan

  • Cc kegan@… added

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

Last edited 7 months ago by kegan (previous) (diff)

comment:50 Changed 3 months ago by anonymous

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

comment:51 Changed 3 months ago by guettli

  • Cc hv@… added

comment:52 Changed 4 weeks ago by 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.

comment:53 Changed 4 weeks ago by charettes

  • Cc charettes added
  • Owner charettes deleted
  • Patch needs improvement set
  • Status changed from assigned to new

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.