The key of permissions model

[Anssi Kääriäinen]

We use the style of "app_label.permission_name" in multiple places of
our code to refer to given auth.models.Permission. However, there is
no unique key for that combination, the key is content_type,
permission_name. I verified that it is actually possible to hit this
problem by using the Meta.permissions.

I am not sure what we can do about this... Or if we even need to do
anything about this.

Without schema changes we can't have a DB constraint for the key. But,
maybe we could validate (as part of model validation) that there are
no overlapping permissions? If the user goes and creates overlapping
permissions despite of this, we can't do anything about that.

The reason why I am wondering about this is that I'd like to add
Permissions.objects.get_permission('app_label.permission_name')
convenience method. And, while investigating that I realized that the
app_label, permission name isn't a key...

I am likely going to add the get_permission() anyways, it will just
throw "returned more than 1" if the above problem is hit.

Thoughts?

- Anssi

[Donald Stufft]

Can't you add the constraint in both code and in the DB. On older sites

the constraint just won't exist in the DB (Could include it in the release

notes so people can add it to existing sites if they wish).

--

You received this message because you are subscribed to the Google Groups "Django developers" group.

To post to this group, send email to django-d...@googlegroups.com.

To unsubscribe from this group, send email to django-develop...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Anssi Kääriäinen]

On 20 syys, 01:47, Donald Stufft <donald.stu...@gmail.com> wrote:
> Can't you add the constraint in both code and in the DB. On older sites
> the constraint just won't exist in the DB (Could include it in the release
> notes so people can add it to existing sites if they wish).

Unfortunately no - the app label doesn't exist in the model. There is
only reference to ContentType, and the app label is stored in there.
So, a normal unique constraint doesn't work...

- Anssi

[Michael Manfre]

Instead of get_permission('app_label.permission_name'), why not punt on the problem
until schema migrations lands. Only provide a plural helper method that always returns
a list. The first argument could be either a string or a list of strings. This leaves it up to
the caller to determine what to do if more than one is returned when they only expected
a single result. Having all of the conflicts is a lot more useful than the "returned more than
1" exception.

Regards,
Michael Manfre

[Anssi Kääriäinen]

On 20 syys, 17:11, Michael Manfre <mman...@gmail.com> wrote:
> Instead of get_permission('app_label.permission_name'), why not punt on the
> problem
> until schema migrations lands. Only provide a plural helper method that
> always returns
> a list. The first argument could be either a string or a list of strings.
> This leaves it up to
> the caller to determine what to do if more than one is returned when they
> only expected
> a single result. Having all of the conflicts is a lot more useful than the
> "returned more than
> 1" exception.

It is a bad idea to allow multiple permissions with the same key to
exists at all. I checked quickly what user.has_perm() does. It happily
reports that the user has the permission if the user has any
permission matching the asked key. This again means it is possible
that a permission check will pass for the wrong instance of
'myapp.someperm'.

Luckily this isn't too serious, as I don't believe it is common to
have overlapping permission keys. The possibility is there, and if
this does happen, then there is a possibility for security issues. We
should not encourage this pattern, but instead document that applabel,
permission_name is a key for permissions (because we already treat it
so), and try to stop overlapping permissions where possible.

For the above reasons I don't like adding APIs which encourage
duplicate app-label keys for permissions, and get_permission()
returning a list is such. If get_permission() returning a single
permission isn't acceptable currently, then I see it as better to wait
until enforced key for app_label, permission_name is implemented than
add the method.

- Anssi

[Hanne Moa]

On 20 September 2012 16:11, Michael Manfre <mma...@gmail.com> wrote:
> The first argument could be either a string or a list of strings.

Bad pattern. Then you must test for the existence of a string in order
to avoid ('t', 'h', 'i', 's').


HM

[Flavio Curella]

I'd like to take another try at this.

My proposal is to temporarily supports two formats: the new `app_label.model_name.codename` (ref #25281) and the old `app.codename`. The old format will raise a PendingDeprecationWarning. Docs, code and tests will be updated to use the new format.

I believe that by having `app_label.model_name`. we can identify the `.content_type`. That, together with `.codename`, should be enough to uniquely identify the `Permission` instance.

There will be no model changes. Just checks and new code for all the methods that accepts the 'string api' for permissions.

Thanks,

–Flavio.