#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: Dave Hall | Owner: Arthur
| Rio
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Arthur Rio):
* status: new => assigned
* owner: (none) => Arthur Rio
Comment:
>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.
--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:59>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/065.2b92d135aea21fa3b9cdfa3246f3e47d%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
