Re: [Django] #11154: Inconsistency with permissions for proxy models

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------------+------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: contrib.auth
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------+------------------------------
Changes (by thepapermen):

* cc: thepapermen (added)
* ui_ux: => 0
* easy: => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:20>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#11154: Inconsistency with permissions for proxy models

------------------------------+------------------------------------

Reporter: etianen | Owner: nobody
Type: Bug | Status: new

Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by kmike):

* cc: kmike84@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:22>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by danny.adair@…):

* cc: danny.adair@… (added)

Comment:

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

}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:23>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:24>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by the_drow):

* cc: omer.drow@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:25>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:26>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (by charettes):

I think that allowing
[https://code.djangoproject.com/browser/django/trunk/django/contrib/contenttypes/models.py#L29
`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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:27>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:28>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by Kronuz):

* cc: Kronuz (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:29>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by danols):

* cc: danols@… (added)

Comment:

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

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:30>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:31>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (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".

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:32>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by charettes):

* cc: charette.s@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:33>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by saxix):

* cc: saxix.rome@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:34>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Changes (by charettes):

* needs_docs: 0 => 1
* needs_tests: 0 => 1

Comment:

Here's the
[https://github.com/charettes/django/compare/master...ticket-11154
-inconsistency-permissions-proxy-models.diff 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
[http://www.mahner.org/posts/separating-staff-and-user-accounts-in-
djangos-admin/ 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:35>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:36>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (by charettes):

Replying to [comment:36 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
[http://dpaste.com/hold/697615/ two tests breaks]. However apps relying on
`contrib.ContentType` in the wild could break badly.

i.e. ([https://github.com/alex/django-taggit 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 issue is
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):
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:37>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (by danny.adair@…):

Replying to [comment:37 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
[http://dpaste.com/hold/697615/ 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:38>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (by charettes):

Replying to [comment:38 danny.adair@…]:

> Replying to [comment:37 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
[http://dpaste.com/hold/697615/ 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:39>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:40>

[Django]

#11154: Inconsistency with permissions for proxy models
------------------------------+------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------

Comment (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/.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:41>

[Django]

#11154: Inconsistency with permissions for proxy models

-------------------------------------+-------------------------------------

Reporter: etianen | Owner: nobody
Type: Bug | Status: new

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):

* keywords: => proxy contenttype permission
* has_patch: 0 => 1
* needs_tests: 1 => 0

Comment:

Now that #18399 is fixed, I created a
[https://github.com/django/django/pull/146 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?

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:42>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:43>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:44>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:45>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------

Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by charettes):

* status: new => assigned
* owner: nobody => charettes

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:46>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by 4glitch@…):

* cc: 4glitch@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:47>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by maa@…):

* cc: maa@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:48>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by kegan):

* cc: kegan@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:49>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by anonymous):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:50>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by guettli):

* cc: hv@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:51>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:52>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner:

Type: Bug | Status: new

Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: proxy contenttype | Triage Stage: Accepted
permission | Needs documentation: 1

Has patch: 1 | Patch needs improvement: 1

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by charettes):

* status: assigned => new
* needs_better_patch: 0 => 1
* owner: charettes =>
* cc: charettes (added)

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:53>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner:
Type: Bug | Status: new
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

-------------------------------------+-------------------------------------

Comment (by cho-is):

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:
[https://gist.github.com/magopian/7543724 Gist]

Perhaps it helpful for others running on the same issue.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:54>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner:
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Comment (by timgraham):

I requested better documentation on the latest
[https://github.com/django/django/pull/4681 pull request].

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:55>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: etianen | Owner:
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Comment (by timgraham):

The [https://github.com/django/django/pull/4681 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:56>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------

Reporter: Dave Hall | Owner: (none)

Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

Comment (by 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()),
],
),
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:57>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: Dave Hall | Owner: (none)
Type: Bug | Status: new
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
-------------------------------------+-------------------------------------

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

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:58>

[Django]

#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]

#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 Simon Charette):

* cc: Simon Charette (removed)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:60>

[Django]

#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):

* cc: Arthur Rio (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:61>

[Django]

#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
-------------------------------------+-------------------------------------

Comment (by Arthur Rio):

As mentioned on the [https://github.com/django/django/pull/10381 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 [https://github.com/django/django/pull/10381 pull request] for
more details about the current state of things and how this patch would
impact permissions.

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:62>

[Django]

#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 Simon Charette):

* cc: charette.s@… (removed)

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:63>

[Django]

#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: 0

Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Arthur Rio):

* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:64>

[Django]

#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: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Arthur Rio):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:65>

[Django]

#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: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Arthur Rio):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:67>

[Django]

#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: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:68>

[Django]

#11154: Inconsistency with permissions for proxy models
-------------------------------------+-------------------------------------
Reporter: Dave Hall | Owner: Arthur
| Rio

Type: Bug | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed

Keywords: proxy contenttype | Triage Stage: Accepted
permission |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham <timograham@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"181fb60159e54d442d3610f4afba6f066a6dac05" 181fb601]:
{{{
#!CommitTicketReference repository=""
revision="181fb60159e54d442d3610f4afba6f066a6dac05"
Fixed #11154, #22270 -- Made proxy model permissions use correct content
type.

Co-Authored-By: Simon Charette <chare...@gmail.com>
Co-Authored-By: Antoine Catton <aca...@fusionbox.com>
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/11154#comment:69>