Opened 16 years ago
Closed 6 years ago
#11154 closed Bug (fixed)
Inconsistency with permissions for proxy models
Reported by: | Dave Hall | Owned by: | Arthur Rio |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | proxy contenttype permission |
Cc: | omat@…, david@…, walter+django@…, Simon Meers, Jari Pennanen, Dave Hall, thepapermen, kmike84@…, danny.adair@…, omer.drow@…, German M. Bravo, danols@…, saxix.rome@…, 4glitch@…, maa@…, kegan@…, hv@…, Arthur Rio | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
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)
Change History (70)
comment:1 by , 16 years ago
Cc: | added |
---|
comment:2 by , 16 years ago
milestone: | 1.1 → 1.2 |
---|---|
Triage Stage: | Unreviewed → Design decision needed |
comment:3 by , 16 years ago
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 by , 15 years ago
Version: | SVN → soc2009/multidb |
---|
comment:6 by , 15 years ago
Cc: | 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 by , 15 years ago
(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 by , 15 years ago
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 by , 15 years ago
milestone: | 1.2 → 1.3 |
---|
We've lived with this for the duration of 1.2; it can wait a little longer.
comment:10 by , 15 years ago
Cc: | added |
---|
comment:11 by , 14 years ago
Triage Stage: | Design decision needed → 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.
follow-up: 13 comment:12 by , 14 years ago
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 User
s to that group. What if I'd then like to restrict Teacher
s to only being able to add/change/delete Student
s and not User
s in general?
comment:13 by , 14 years ago
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 by , 14 years ago
Cc: | added |
---|
comment:15 by , 14 years ago
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:17 by , 14 years ago
Cc: | added |
---|
comment:18 by , 14 years ago
Cc: | added |
---|---|
Severity: | → Normal |
Type: | → Uncategorized |
comment:19 by , 14 years ago
Type: | Uncategorized → Bug |
---|
comment:20 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:22 by , 13 years ago
Cc: | added |
---|
comment:23 by , 13 years ago
Cc: | 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 by , 13 years ago
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 by , 13 years ago
Cc: | added |
---|
comment:26 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
Cc: | added |
---|
comment:30 by , 13 years ago
Cc: | 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.
comment:31 by , 13 years ago
@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 by , 13 years ago
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 by , 13 years ago
Cc: | added |
---|
comment:34 by , 13 years ago
Cc: | added |
---|
comment:35 by , 13 years ago
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.
follow-up: 37 comment:36 by , 13 years ago
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
follow-up: 38 comment:37 by , 13 years ago
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):
follow-up: 39 comment:38 by , 13 years ago
Replying to charettes:
[...]
Defaulting
follow_proxy
(like that one :) toFalse
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 oncontrib.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 theContentType
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 by , 13 years ago
Replying to danny.adair@…:
Replying to charettes:
[...]
Defaulting
follow_proxy
(like that one :) toFalse
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 oncontrib.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 theContentType
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 by , 13 years ago
comment:41 by , 13 years ago
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 by , 13 years ago
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 by , 12 years ago
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 by , 12 years ago
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 by , 12 years ago
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 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 11 years ago
Cc: | added |
---|
comment:48 by , 11 years ago
Cc: | added |
---|
comment:50 by , 11 years ago
Using 1.6 and can confirm this is still an issue. Any idea what's the latest on this? Thanks,
comment:51 by , 11 years ago
Cc: | added |
---|
comment:52 by , 10 years ago
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 by , 10 years ago
Cc: | added |
---|---|
Owner: | removed |
Patch needs improvement: | set |
Status: | assigned → 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.
comment:54 by , 10 years ago
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.
comment:56 by , 9 years ago
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.
comment:57 by , 8 years ago
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()), ], ),
follow-up: 59 comment:58 by , 7 years ago
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).
comment:59 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
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.
comment:60 by , 6 years ago
Cc: | removed |
---|
comment:61 by , 6 years ago
Cc: | added |
---|
comment:62 by , 6 years ago
As mentioned on the pull request, I moved away from my initial comment because:
- It's only a temporary fix and we would still need to address the bigger issue about how permissions are created for proxy models.
- 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.
comment:63 by , 6 years ago
Cc: | removed |
---|
comment:64 by , 6 years ago
Needs documentation: | unset |
---|
comment:65 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:66 by , 6 years ago
Patch needs improvement: | set |
---|
comment:67 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:68 by , 6 years ago
Patch needs improvement: | set |
---|
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.