#15915 closed Bug (fixed)
Permission codename duplication is not handled well
Reported by: | Valentin Golev | 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)
Change History (21)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
Triage Stage: | Unreviewed → 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.
follow-ups: 4 6 comment:3 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
Sorry, seems like my fault. It doesn't solve the bug though. And what do you think about my case?
comment:8 by , 14 years ago
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 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
UI/UX: | unset |
comment:10 by , 13 years ago
Has patch: | set |
---|
comment:11 by , 12 years ago
Cc: | added |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:13 by , 12 years ago
Owner: | set to |
---|
comment:14 by , 12 years ago
Patch needs improvement: | set |
---|
comment:15 by , 12 years ago
Patch needs improvement: | unset |
---|
Patch available here: https://github.com/mhaligowski/django/compare/ticket_15915 [unavailable, already merged]
comment:16 by , 12 years ago
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 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
follow-up: 19 comment:18 by , 12 years ago
@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
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