feat(otp): add one_time_tokens query helpers - #2797
Open
annabkr wants to merge 4 commits into
Open
Conversation
annabkr
marked this pull request as draft
September 9, 2026 16:15
annabkr
force-pushed
the
annabaker/auth-1553-ott-query-helpers
branch
from
September 9, 2026 19:08
fa21e86 to
7577774
Compare
…yRelatesTo Two lookups for the OTT-as-source-of-truth work. FindOneTimeTokenWithPKCEFallback accepts a token hash or its pkce_-prefixed form in a single query and prefers the exact match. FindOneTimeTokenByRelatesTo returns the newest row for a relates_to value and token type. relates_to is not unique for PhoneChangeToken, so the doc comment tells callers to check the user against the request.
models cannot import api, so the constant moves down and api.PKCEPrefix points at it. Replaces the bare "pkce_" literals in the one_time_tokens finders with the constant.
FindOneTimeToken and FindOneTimeTokenWithPKCEFallback delegate to findOneTimeToken, which takes a pkceFallback flag. With the flag off it builds the statement FindOneTimeToken built before: same WHERE string, same args in the same order, same Eager connection, no ORDER BY. The flag only switches the token_hash clause and adds the ORDER BY.
Pin the exact-hash path: no pkce_ match, the type filter applies, and either of two types is found. seedToken replaces the truncate, create user, insert row boilerplate in the hash lookup tests.
annabkr
force-pushed
the
annabaker/auth-1553-ott-query-helpers
branch
from
September 9, 2026 19:14
7577774 to
fc7f405
Compare
annabkr
marked this pull request as ready for review
September 9, 2026 19:15
xlgmokha
approved these changes
Sep 9, 2026
xlgmokha
left a comment
xlgmokha
left a comment
Contributor
There was a problem hiding this comment.
The changes LGTM.
Note: I don't have a lot of experience with this ORM so I think it would be helpful to me to see the SQL queries that are generated and the explain plan for them.
|
|
||
| if pkceFallback { | ||
| // true sorts before false in descending order, so this allows us to prefer an exact match | ||
| query = query.Order("token_hash = ? desc", tokenHash) |
Contributor
There was a problem hiding this comment.
praise: I didn't know that you could do this.
| case 2: | ||
| query = query.Where("(token_type = ? or token_type = ?) and token_hash = ?", tokenTypes[0], tokenTypes[1], tokenHash) // #nosec G602 | ||
| args := append([]interface{}{tokenTypes[0], tokenTypes[1]}, hashArgs...) // #nosec G602 | ||
| query = query.Where("(token_type = ? or token_type = ?) and "+hashClause, args...) |
Contributor
There was a problem hiding this comment.
question(non-blocking): would it be easier to ready to chain the where clauses together? I'm not sure if pop supports something like query.Where(...).And(..)?
| Order("created_at desc"). | ||
| First(oneTimeToken) | ||
| if errors.Cause(err) == sql.ErrNoRows { | ||
| return nil, OneTimeTokenNotFoundError{} |
Contributor
There was a problem hiding this comment.
thought(non-blocking): It looks like we have an errNotFound err in internal/models/errors.go. I'm not sure if it's better to reuse that or create a new error type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Feature
What is the current behavior?
There is no single-query lookup for a token hash and its
pkce_-prefixed form, and no lookup byrelates_to.Callers such as
FindUserByEmailChangeCurrentAndAudiencerun twoFindOneTimeTokenqueries to cover the prefix, which is inefficient.What is the new behavior?
Adds two parent query methods used by the upcoming OTT-as-source-of-truth work:
FindOneTimeTokenWithPKCEFallback: single-query lookup that accepts either an exact token hash or itspkce_-prefixed form, preferring the exact match.FindOneTimeTokenByRelatesTo: looks up the newest token row for a givenrelates_tovalue (e.g. phone number) and token type.Along with:
PKCEPrefixis now a constant inmodelsthat we reference vs. string literalsFindOneTimeTokenandFindOneTimeTokenWithPKCEFallbacknow sharefindOneTimeToken, guided by apkceFallbackflag.Adds tests for those methods. Not updating existing callers for now to reduce the radius of this change.
Additional context
Contributes to AUTH-1553 but does not close.