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

remove unique index from IDToken's token field #10

Open
wants to merge 2 commits into
base: openid-connect
Choose a base branch
from

Conversation

michaelhelvey
Copy link

@michaelhelvey michaelhelvey commented Jun 24, 2020

Description of the Change

On MySQL 8.0 (and before, as far as I know) creating a unique TEXT index
will fail with err 1170 "BLOB/TEXT column 'token' used in key specification
without a key length"

This PR simply removes the invalid unique index.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added (N/A for this change as far as I know)
  • documentation updated (N/A for this change as far as I know)
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

On MySQL 8.0 (and before, as far as I know) creating a unique TEXT index
will fail with err 1170 "BLOB/TEXT column 'token' used in key specification
without a key length"
@@ -449,7 +449,7 @@ class AbstractIDToken(models.Model):
settings.AUTH_USER_MODEL, on_delete=models.CASCADE, blank=True, null=True,
related_name="%(app_label)s_%(class)s"
)
token = models.TextField(unique=True)
token = models.TextField()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or should we move to uuid?

Copy link
Author

@michaelhelvey michaelhelvey Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but isn't this value not actually a uuid, but a raw JWT (see the get_claims method on this class?), in which case a TextField is the appropriate type? Maybe I'm not understanding what this value does.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field needs to be a text field because that will be persisted a long string (the jwt token) on this, but, I think that independently of the database backend, it seems there is no reason for this field to be a unique field.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, in terms of security, I think that this token really needs to be unique. so every request will receive a valid response with a different valid token. in the token generation process, more information can be put to guarantee that uniqueness won't be invalid. I don't know, but I think that need another approach to resolve this problem for mysql backends

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea would be to add an additional unique CharField that contains a hash of the token. So we can make sure it is really unique on an mysql backend.

@auvipy
Copy link
Collaborator

auvipy commented Jun 25, 2020

@fvlima
Copy link

fvlima commented Sep 8, 2020

The openid flow is already in the master through this PR jazzband#859. I think that this proposal fix here can be pointed to the master now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants