Permission codename duplication is not handled well

[Valentin Golev]

Example:

from django.db import models

# Create your models here.


class Tweet(models.Model):
    message = models.CharField(max_length=140)

    class Meta:
        permissions = (("change_tweet", "can, like, change tweet or something..."),)

Stacktrace:

(bug-with-perms)[.../bugwithperms]$ python manage.py syncdb
Creating tables ...
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_user_permissions
Creating table auth_user_groups
Creating table auth_user
Creating table auth_message
Creating table django_content_type
Creating table django_session
Creating table django_site
Creating table trybug_tweet

You just installed Django's auth system, which means you don't have any superusers defined.
Would you like to create one now? (yes/no): no
Traceback (most recent call last):
  File "manage.py", line 14, in <module>
    execute_manager(settings)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/__init__.py", line 438, in execute_manager
    utility.execute()
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/__init__.py", line 379, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/base.py", line 191, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/base.py", line 220, in execute
    output = self.handle(*args, **options)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/base.py", line 351, in handle
    return self.handle_noargs(**options)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/commands/syncdb.py", line 109, in handle_noargs
    emit_post_sync_signal(created_models, verbosity, interactive, db)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/core/management/sql.py", line 190, in emit_post_sync_signal
    interactive=interactive, db=db)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/dispatch/dispatcher.py", line 172, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/contrib/auth/management/__init__.py", line 51, in create_permissions
    content_type=ctype
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/manager.py", line 138, in create
    return self.get_query_set().create(**kwargs)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/query.py", line 360, in create
    obj.save(force_insert=True, using=self.db)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/base.py", line 460, in save
    self.save_base(using=using, force_insert=force_insert, force_update=force_update)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/base.py", line 553, in save_base
    result = manager._insert(values, return_id=update_pk, using=using)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/manager.py", line 195, in _insert
    return insert_query(self.model, values, **kwargs)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/query.py", line 1436, in insert_query
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 791, in execute_sql
    cursor = super(SQLInsertCompiler, self).execute_sql(None)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/models/sql/compiler.py", line 735, in execute_sql
    cursor.execute(sql, params)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/backends/util.py", line 34, in execute
    return self.cursor.execute(sql, params)
  File "/media/oldroot/home/valentin/Envs/bug-with-perms/lib/python2.6/site-packages/django/db/backends/sqlite3/base.py", line 234, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.IntegrityError: columns content_type_id, codename are not unique

What's going on?

Basically, if there are two permissions for a model with same codenames and different descriptions, Django tries to add both in the database, but there is a unique index on codename so it all crashes horribly.

The code which is responsible for this is at create_permissions() function[1]. There is a set: searched_perms, so it should help to avoid duplicate values. But the second element of every tuple in set, namely "perm", is another tuple of (codename, name), and the "name" is a human-readable name and not an identifier. So if we have two permissions with same codenames, but different "name"s, set will see them as different permissions and django will try to insert them both in the database. However, the unique index is only on two fields: ctype and codename, so permissions with same ctype and codename, but different "names", can't be inserted. That results in a pretty odd stacktrace.

[1]: ​http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/management/__init__.py#L19

Either it is a bug, or a good error message should be added. This one is misleading (and it's much more misleading on postgres). I had to use pdb to investigate this pretty innocent code

[Valentin Golev]

I could try to create a patch for that, but please decide if it's a bug or a good behavior with bad error message

[Carl Meyer]

It's not a bug that you can't have two permissions for the same model with the same codename.

It is a bug that if you try to, you get a cryptic database error message. This should be caught before it's ever saved to the database, and there should be an intelligible error message explaining the problem.

[Valentin Golev]

But if you add two permissions with same codename and description, everything will be fine thanks to set(). Isn't inconsistent a little bit?

[Carl Meyer]

Replying to valyagolev:

But if you add two permissions with same codename and description, everything will be fine thanks to set(). Isn't inconsistent a little bit?

Yes - sorry, didn't catch that subtlety. It is a little inconsistent, and we could start raising errors in that case -- but I'm not sure it's actually worth doing so. It could break some people's currently-working code, and for what benefit? If two permissions are defined with exactly the same info, creating a single permission with that info seems like a choice that's never wrong. (I also wonder, without having looked into it, if there are cases with model inheritance where this situation might crop up unintentionally?)

So while I don't have strong feelings, my leaning is that we may as well restrict ourselves to just making the error nicer when there are two permissions defined with the same codename and different descriptions.

[Valentin Golev]

The case in which we had this error was like this:

We are using row-level permissions, and we needed several: "edit_page", "view_page", "delete_page", "act_as_page" etc. So we have just written them all down in "class Meta:", and got the error.

If Django will restrict re-stating permissions, we have two choices:

• Either delete/comment out "edit" and "delete" permissions, and try to remember or make notes about them.
• add our prefix to permissions, which will lead to duplicating permissions: "edit_page" and "ourproject_edit_page"

I don't like either of them.

[Carl Meyer]

Replying to valyagolev:

But if you add two permissions with same codename and description, everything will be fine thanks to set(). Isn't inconsistent a little bit?

Actually, I just looked at the code, and this is not true - your analysis in the ticket description isn't quite right. searched_perms is not a set, it's a list. So it doesn't matter whether the description (name) is the same or not; both perms will appear in the list and the IntegrityError will be raised either way if two perms have the same codename.

[Valentin Golev]

Sorry, seems like my fault. It doesn't solve the bug though. And what do you think about my case?

[Carl Meyer]

Replying to valyagolev:

We are using row-level permissions, and we needed several: "edit_page", "view_page", "delete_page", "act_as_page" etc. So we have just written them all down in "class Meta:", and got the error.

If Django will restrict re-stating permissions, we have two choices:

• Either delete/comment out "edit" and "delete" permissions, and try to remember or make notes about them.
• add our prefix to permissions, which will lead to duplicating permissions: "edit_page" and "ourproject_edit_page"

I don't like either of them.

If I understand right, you're requesting as a new feature that you be allowed to explicitly define a permission in Meta with the same codename as a built-in permission, and your name for it will replace the default one, with no error?

I don't have much opinion on that; it doesn't seem like a terrible thing to just use the built-in ones as-is (with a comment in Meta to remind you of them, if you need that), but I also don't see any huge problems with your proposal.

In any case, let's keep this ticket for the better-error-message only, which is a separate issue that should be fixed regardless of your feature request (for instance, if someone defines two perms explicitly in Meta with the same codename, this almost certainly is an error in their code and should be flagged rather than letting one silently override the other). You can file a separate ticket for your feature request, and perhaps post to django-developers to point to the ticket and clarify exactly what it is you're requesting so others can comment.

[Nikolay Zakharov]

Initial patch with test

[Preston Holmes]

updated patch for 16730

[mhaligowski]

Is that opened for development? Or is the patch sufficient?

[mhaligowski]

Patch available here: https://github.com/mhaligowski/django/compare/ticket_15915 [unavailable, already merged]

[Anssi Kääriäinen]

Doing a final test run and then committing the patch with some final polishing.

This brings back the idea of validating globally that <applable, codename> pairs are unique. Doing that would of course solve also this ticket's issue. In my opinion we should do that validation sooner rather than later. Still, it seems adding the currently available implementation is a step forward, and it will not hurt anybody even if the global validation is added.

[Anssi Kääriäinen <akaariai@…>]

In 8c427448d53ec0d860e1669f35deed73d0240ba1:

Fixed #15915 -- Cleaned handling of duplicate permission codenames

Previously, a duplicate model, codename for permission would lead to
database integrity error. Cleaned the implementation so that this case
now raises an CommandError instead.

[Thomas Schreiber]

@Mateusz When changing this from TearUp to TearDown, there are failing tests, I think it may need another look:

​https://github.com/django/django/blob/master/django/contrib/auth/tests/management.py#L178

[mhaligowski]

Replying to rizumu:

@Mateusz When changing this from TearUp to TearDown, there are failing tests, I think it may need another look:

​https://github.com/django/django/blob/master/django/contrib/auth/tests/management.py#L178

Thank you for your comment, but I don't really get what you mean by "changing from TearUp to TearDown". Would you please elaborate on that? Thank you in advance.