-
-
Notifications
You must be signed in to change notification settings - Fork 92
Case insensitive tokens #249
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
base: master
Are you sure you want to change the base?
Case insensitive tokens #249
Conversation
app/models/passwordless/session.rb
Outdated
def token=(token) | ||
Passwordless.config.case_insensitive_tokens ? modified_token = token.upcase : modified_token = token | ||
self.token_digest = Passwordless.digest(modified_token) | ||
@token = (modified_token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do anything at write time. Rather, let's just upcase in SQL when searching for tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def token=(token) | |
Passwordless.config.case_insensitive_tokens ? modified_token = token.upcase : modified_token = token | |
self.token_digest = Passwordless.digest(modified_token) | |
@token = (modified_token) | |
def token=(plaintext) | |
self.token_digest = Passwordless.digest(plaintext) | |
@token = (plaintext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to do this at write time - we're persisting the token digest, not the token, so I believe we cannot upcase the token digest when searching for tokens.
Co-authored-by: Mikkel Malmberg <[email protected]>
token = token.upcase if Passwordless.config.case_insensitive_tokens | ||
self.token_digest = Passwordless.digest(token) | ||
@token = token | ||
end | ||
|
||
def authenticate(token) | ||
token = token.upcase if Passwordless.config.case_insensitive_tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only case-insensitive so long as you're using a key generator that only outputs UPPER CASE keys. I think the option is a bit misleading then. We should do the upcasing as part of the db lookup instead, so we're case-insensitive no matter the generator output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. I believe this should work for a key generator that uses lower case keys (or mixed case, though that'd be silly). I'll add tests to confirm this when I have a chance.
Note that since the generator output is hashed before it is persisted to the database, we cannot store a mixed case token and hope to look it up with a case insensitive token.
closes #248