Opened 3 weeks ago

Last modified 6 hours ago

#36793 assigned Bug

Reverting a model rename in an environment with pre-Django 6.0 permission records leaves wrong users & groups associated

Reported by: Jacob Walls Owned by: Artyom Kotovskiy
Component: contrib.auth Version: 6.0
Severity: Release blocker Keywords:
Cc: Antoliny, Clifford Gama Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

  • Checkout Django 5.2
  • Assign user 1 to have view permissions on FooModel
  • Rename the model to BarModel
  • Migrate

(Result: no permissions transfer to BarModel, FooModel permissions left behind in the db).

  • Assign user 2 to have view permissions on BarModel
  • Checkout Django 6.0
  • Rename the model back to FooModel and migrate forward

Expected: ?
What user should have view permissions on FooModel now, 1 or 2?

As of #27489, which makes the permissions tag along with RenameModel operations, I think we would expect user 2. (Proposed fix PR makes it Users 1 *and* 2, but I'm interested to hear more on that.)

Actual result: User 1 has the permission on FooModel, and BarModel permissions for User 2 are left behind in the db.

Bug in f02b49d2f3bf84f5225de920ca510149f1f9f1da.

Change History (13)

comment:1 by Jacob Walls, 3 weeks ago

comment:2 by Jacob Walls, 3 weeks ago

Summary: Reversing RenameModel in an environment with pre-Django 6.0 permission records leaves wrong users & groups associatedReverting a model rename in an environment with pre-Django 6.0 permission records leaves wrong users & groups associated

comment:3 by Antoliny, 3 weeks ago

I think only user 2 should have the permission, and raising an error would be appropriate.
From the perspective of the person performing this operation, they would naturally expect user 2 to have the permission (considering the intent of #27489).
It feels strange that ghost data created by a bug would come back into play.
When an error occurs, it would help the developer become aware of the issue.
I'm also curious to hear others opinions :)

Version 1, edited 3 weeks ago by Antoliny (previous) (next) (diff)

comment:4 by Antoliny, 3 weeks ago

Cc: Antoliny added

comment:5 by Clifford Gama, 3 weeks ago

I share your position, Jacob and Antoliny.

That user 2 does not have the permissions is definitely a bug in 6.0, so that needs to be fixed and backported.

Whether user 1 also ends up having the permissions should depend on whether that was the behavior before the patch in #27489 landed. If it wasn't then it would need a new-feature-like release note, meaning it cannot be backported. If it was, then it should be fixed as a regression in a new feature.

comment:6 by Clifford Gama, 3 weeks ago

Triage Stage: UnreviewedAccepted

comment:7 by Jacob Walls, 3 weeks ago

Thanks for the gut checks. So my understanding is that #27489 fixed the bug where permissions for a model being renamed were left stranded. So when un-renaming in Django 6, if we choose to overwrite any records stranded before Django 6, that's a bugfix and not something subject to the backward compat guarantee, since they shouldn't have been there at all. 🤔

Last edited 3 weeks ago by Jacob Walls (previous) (diff)

comment:8 by Clifford Gama, 3 weeks ago

All right, that makes sense. In that case maybe we want user 1 to also have the permission, since it possibly was never explicitly deleted and it should have been carried over in the model renaming. But I don't have a strong opinion about it.

comment:9 by Clifford Gama, 3 weeks ago

Cc: Clifford Gama added

comment:10 by Jacob Walls, 3 weeks ago

I think my hesitation there is that if you've done a model rename in Django 5, you've started over on your permissions, and you could have made N edits. If on Django 6 you then do a model un-rename, you'd probably be pretty surprised to see some random edits from N edits ago merged in.

comment:11 by Clifford Gama, 3 weeks ago

Let's make it user 2 only, then. Its the safer option. We don't want to surprise folks by giving users outdated permissions.

comment:12 by Jacob Walls, 2 weeks ago

Needs documentation: set
Needs tests: set

comment:13 by Jacob Walls, 6 hours ago

Alternative PR with PR comments annotating salient behavior changes:
https://github.com/django/django/pull/20481

Also, some remarks about prior precedents in this area:
https://github.com/django/django/pull/20339#issuecomment-3702832303

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