Opened 6 months ago

Closed 5 months ago

Last modified 5 months ago

#30351 closed Bug (fixed)

Migration auth.0011_update_proxy_permissions fails for models recreated as a proxy.

Reported by: Julien Enselme Owned by: Carlton Gibson
Component: contrib.auth Version: 2.2
Severity: Release blocker Keywords:
Cc: Simon Charette, Arthur Rio, Antoine Catton Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by felixxm)

I am trying to update my project to Django 2.2. When I launch python manage.py migrate, I get this error message when migration auth.0011_update_proxy_permissions is applying (full stacktrace is available here):
django.db.utils.IntegrityError: duplicate key value violates unique constraint "idx_18141_auth_permission_content_type_id_01ab375a_uniq" DETAIL: Key (co.ntent_type_id, codename)=(12, add_agency) already exists.

It looks like the migration is trying to re-create already existing entries in the auth_permission table. At first I though it cloud because we recently renamed a model. But after digging and deleting the entries associated with the renamed model from our database in the auth_permission table, the problem still occurs with other proxy models.

I tried to update directly from 2.0.13 and 2.1.8. The issues appeared each time. I also deleted my venv and recreated it without an effect.

I searched for a ticket about this on the bug tracker but found nothing. I also posted this on django-users and was asked to report this here.

Change History (14)

comment:1 Changed 6 months ago by Tim Graham

Component: Uncategorizedcontrib.auth
Type: UncategorizedBug

Please provide a sample project or enough details to reproduce the issue.

comment:2 Changed 5 months ago by Kia Hosseini

Same problem for me.
If a Permission exists already with the new content_type and permission name, IntegrityError is raised since it violates the unique_key constraint on permission model i.e. content_type_id_code_name

Last edited 5 months ago by Kia Hosseini (previous) (diff)

comment:3 Changed 5 months ago by Sébastiaan Versteeg

To get into the situation where you already have permissions with the content type you should be able to do the following:

  • Start on Django <2.2
  • Create a model called 'TestModel'
  • Migrate
  • Delete the model called 'TestModel'
  • Add a new proxy model called 'TestModel'
  • Migrate
  • Update to Django >=2.2
  • Migrate

We think this is what happened in our case where we found this issue (https://sentry.thalia.nu/share/issue/68be0f8c32764dec97855b3cbb3d8b55/).
We have a proxy model with the same name that a previous non-proxy model once had. This changed during a refactor and the permissions + content type for the original model still exist.
Our solution will probably be removing the existing permissions from the table, but that's really only a workaround.

Last edited 5 months ago by Sébastiaan Versteeg (previous) (diff)

comment:4 Changed 5 months ago by felixxm

Cc: Simon Charette Arthur Rio Antoine Catton added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Reproduced with steps from comment. It's probably regression in 181fb60159e54d442d3610f4afba6f066a6dac05.

comment:5 Changed 5 months ago by felixxm

Description: modified (diff)
Summary: Migration auth.0011_update_proxy_permissions from Django 2.2 fails to applyMigration auth.0011_update_proxy_permissions fails for models recreated as a proxy.

comment:6 Changed 5 months ago by Arthur Rio

Owner: changed from nobody to Arthur Rio
Status: newassigned

What happens when creating a regular model, deleting it and creating a new proxy model:

  1. Create model 'RegularThenProxyModel'
+----------------------------------+---------------------------+-----------------------+
| name                             | codename                  | model                 |
+----------------------------------+---------------------------+-----------------------+
| Can add regular then proxy model | add_regularthenproxymodel | regularthenproxymodel |
+----------------------------------+---------------------------+-----------------------+
  1. Migrate
  2. Delete the model called 'RegularThenProxyModel'
  3. Add a new proxy model called 'RegularThenProxyModel'
+----------------------------------+---------------------------+-----------------------+
| name                             | codename                  | model                 |
+----------------------------------+---------------------------+-----------------------+
| Can add concrete model           | add_concretemodel         | concretemodel         |
| Can add regular then proxy model | add_regularthenproxymodel | concretemodel         |
| Can add regular then proxy model | add_regularthenproxymodel | regularthenproxymodel |
+----------------------------------+---------------------------+-----------------------+

What happens when creating a proxy model right away:

  1. Create a proxy model 'RegularThenProxyModel'
    +----------------------------------+---------------------------+---------------+
    | name                             | codename                  | model         |
    +----------------------------------+---------------------------+---------------+
    | Can add concrete model           | add_concretemodel         | concretemodel |
    | Can add regular then proxy model | add_regularthenproxymodel | concretemodel |
    +----------------------------------+---------------------------+---------------+
    

As you can see, the problem here is that permissions are not cleaned up, so we are left with an existing | Can add regular then proxy model | add_regularthenproxymodel | regularthenproxymodel | row. When the 2.2 migration is applied, it tries to create that exact same row, hence the IntegrityError. Unfortunately, there is no remove_stale_permission management command like the one for ContentType. So I think we can do one of the following:

  1. Show a nice error message to let the user delete the conflicting migration

OR

  1. Re-use the existing permission

I think 1. is much safer as it will force users to use a new permission and assign it accordingly to users/groups.

Edit: I revised my initial comment after reproducing the error in my environment.

Last edited 5 months ago by Arthur Rio (previous) (diff)

comment:7 Changed 5 months ago by B Martsberger

It's also possible to get this kind of integrity error on the auth.0011 migration if another app is migrated first causing the auth post_migrations hook to run. The auth post migrations hook runs django.contrib.auth.management.create_permissions, which writes the new form of the auth_permission records to the table. Then when the auth.0011 migration runs it tries to update things to the values that were just written.

To reproduce this behavior:

  1. pip install Django==2.1.7
  2. Create an app, let's call it app, with two models, TestModel(models.Model) and ProxyModel(TestModel) the second one with proxy=True
  3. python manage.py makemigrations
  4. python manage.py migrate
  5. pip install Django==2.2
  6. Add another model to app
  7. python manage.py makemigrations
  8. migrate the app only, python manage.py migrate app. This does not run the auth migrations, but does run the auth post_migrations hook
  9. Note that new records have been added to auth_permission
  10. python manage.py migrate, this causes an integrity error when the auth.0011 migration tries to update records that are the same as the ones already added in step 8.

This has the same exception as this bug report, I don't know if it's considered a different bug, or the same one.

comment:8 Changed 5 months ago by Arthur Rio

Yes it is the same issue. My recommendation to let the users figure it out with a helpful message still stands even if it may sound a bit painful, because:

  1. It prevents data loss (we don't do an automatic delete/create of permissions)
  2. It prevents security oversights (we don't re-use an existing permission)
  3. It shouldn't happen for most use cases

Again, I would love to hear some feedback or other alternatives.

comment:9 Changed 5 months ago by Arthur Rio

Owner: Arthur Rio deleted
Status: assignednew

I won’t have time to work on this for the next 2 weeks so I’m de-assigning myself. I’ll pick it up again if nobody does and I’m available to discuss feedback/suggestions.

comment:10 Changed 5 months ago by Carlton Gibson

Owner: set to Carlton Gibson
Status: newassigned

comment:11 Changed 5 months ago by Carlton Gibson

I'll make a patch for this.

I'll see about raising a suitable warning from the migration but we already warn in the release notes for this to audit permissions: my initial thought was that re-using the permission would be OK. (I see Arthur's comment. Other thoughts?)

comment:12 Changed 5 months ago by Arthur Rio

Being my first contribution I wanted to be super (super) careful with security concerns, but given the existing warning in the release notes for auditing prior to update, I agree that re-using the permission feels pretty safe and would remove overhead for people running into this scenario.

Thanks for taking this on Carlton, I'd be happy to review.

comment:13 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 98296f86:

Fixed #30351 -- Handled pre-existing permissions in proxy model permissions data migration.

Regression in 181fb60159e54d442d3610f4afba6f066a6dac05.

comment:14 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 4f8ebdd0:

[2.2.x] Fixed #30351 -- Handled pre-existing permissions in proxy model permissions data migration.

Regression in 181fb60159e54d442d3610f4afba6f066a6dac05.

Backport of 98296f86b340c8c9c968375d59f1d3a3479e60c2 from master

Note: See TracTickets for help on using tickets.
Back to Top