Code

Opened 3 years ago

Closed 22 months ago

Last modified 21 months ago

#15915 closed Bug (fixed)

Permission codename duplication is not handled well

Reported by: valyagolev Owned by: mhaligowski
Component: contrib.auth Version: 1.3
Severity: Normal Keywords:
Cc: me@…, nikolay@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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

Attachments (2)

15915_patch.diff (5.2 KB) - added by desh 3 years ago.
Initial patch with test
15915.2.diff (4.8 KB) - added by ptone 3 years ago.
updated patch for 16730

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by valyagolev

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by carljm

  • Triage Stage changed from Unreviewed to Accepted

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.

comment:3 follow-ups: Changed 3 years ago by 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?

comment:4 in reply to: ↑ 3 Changed 3 years ago by carljm

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.

comment:5 Changed 3 years ago by valyagolev

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.

comment:6 in reply to: ↑ 3 Changed 3 years ago by carljm

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.

comment:7 Changed 3 years ago by valyagolev

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

comment:8 Changed 3 years ago by carljm

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.

comment:9 Changed 3 years ago by desh

  • Owner changed from nobody to desh
  • Status changed from new to assigned
  • UI/UX unset

Changed 3 years ago by desh

Initial patch with test

comment:10 Changed 3 years ago by desh

  • Has patch set

Changed 3 years ago by ptone

updated patch for 16730

comment:11 Changed 23 months ago by desh

  • Cc nikolay@… added
  • Owner desh deleted
  • Status changed from assigned to new

comment:12 Changed 22 months ago by mhaligowski

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

comment:13 Changed 22 months ago by mhaligowski

  • Owner set to mhaligowski

comment:14 Changed 22 months ago by mhaligowski

  • Patch needs improvement set

comment:15 Changed 22 months ago by mhaligowski

  • Patch needs improvement unset

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

Last edited 22 months ago by mhaligowski (previous) (diff)

comment:16 Changed 22 months ago by akaariai

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.

comment:17 Changed 22 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:18 follow-up: Changed 21 months ago by 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

comment:19 in reply to: ↑ 18 Changed 21 months ago by mhaligowski

Last edited 21 months ago by mhaligowski (previous) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.