search

jazzband / django-oauth-toolkit

OAuth2 goodies for the Djangonauts!
https://django-oauth-toolkit.readthedocs.io
Other
3.16k stars 794 forks source link

3.0.0 access token migration does not work #1490

Closed gardenerik closed 2 months ago

gardenerik commented 2 months ago

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

Additional context

gardenerik commented 2 months ago

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

n2ygk commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

n2ygk commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

I have the same issue, rolling back until resolved.

n2ygk commented 2 months ago

3.0.1 released

n2ygk commented 2 months ago

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

JasonLovesDoggo commented 2 months ago

@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 commented 2 months ago

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