Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.0.0 access token migration does not work #1490

Closed
1 of 2 tasks
gardenerik opened this issue Sep 6, 2024 · 13 comments
Closed
1 of 2 tasks

3.0.0 access token migration does not work #1490

gardenerik opened this issue Sep 6, 2024 · 13 comments
Assignees
Labels

Comments

@gardenerik
Copy link
Contributor

gardenerik commented Sep 6, 2024

Describe the bug

The AccessToken has a new checksum field, that is both unique and blank, which means that existing tokens will be assigned empty checksum by default. However, empty string is not unique.

django.db.utils.IntegrityError: could not create unique index  oauth2_provider_accesstoken_token_checksum_key"
DETAIL:  Key (token_checksum)=() is duplicated.          

To Reproduce
Run migrate on an existing database.

Expected behavior

The migration should run successfully.

Version

3.0.0

  • I have tested with the latest published release and it's still a problem.
  • I have tested with the master branch and it's still a problem.

Additional context

@gardenerik gardenerik added the bug label Sep 6, 2024
@gardenerik
Copy link
Contributor Author

seems to be related to #1489, but we are not using custom models in DOT.

@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

Yeah I believe the problem is the migration needs to have explicit code added to generate and set the checksum for existing tokens. Easy to see how this snuck through testing as an empty database won't have any old tokens in it.

@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

A workaround might be to delete existing access tokens before migrating -- which is not ideal if you are trying to upgrade a live system.

@n2ygk n2ygk pinned this issue Sep 6, 2024
@n2ygk n2ygk self-assigned this Sep 6, 2024
@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

I'm currently working on a fix for the migration. Given the use of pre_save hooks and the like, the model may need to retain the unique and blank values. We can iterate on that once the fix proves out....

@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

@gardenerik #1491 is not fully tested but I wanted to get it out there so you could try it. I will try to reproduce it myself as well but am running up against a time deadline for other work at the moment.

@gardenerik
Copy link
Contributor Author

That looks like it would resolve the problem, I can try to run it against a copy of our production tomorrow if needed.

@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

That looks like it would resolve the problem, I can try to run it against a copy of our production tomorrow if needed.

Hang on a sec. I need to split the migration into two parts since the index constraint can't happen until after the checksums are added for the old tokens.

@n2ygk
Copy link
Member

n2ygk commented Sep 6, 2024

That looks like it would resolve the problem, I can try to run it against a copy of our production tomorrow if needed.

Hang on a sec. I need to split the migration into two parts since the index constraint can't happen until after the checksums are added for the old tokens.

OK to try it now. I still think there's some work needed to make the model consistent with the result of the migration.

@JasonLovesDoggo
Copy link

I have the same issue, rolling back until resolved.

@n2ygk
Copy link
Member

n2ygk commented Sep 7, 2024

3.0.1 released

@n2ygk n2ygk closed this as completed Sep 7, 2024
@n2ygk
Copy link
Member

n2ygk commented Sep 9, 2024

@gardenerik @JasonLovesDoggo please let me know for sure that this is working for you now. Thanks.

@n2ygk n2ygk reopened this Sep 9, 2024
@JasonLovesDoggo
Copy link

@gardenerik @JasonLovesDoggo please let me know for sure that this is working for you now. Thanks.

Worked fine for me, I can't 100% remember if I updated prod yet but it passed local tests just fine

@gardenerik
Copy link
Contributor Author

I can also confirm that 3.0.1 works on a copy of our production data.

@n2ygk n2ygk closed this as completed Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants