fix(core): return one-time token status as expired when expiration time is passed #7257
Conversation
This file contains 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
charIeszhao
requested review from
simeng-li,
wangsijie and
gao-sun
as code owners
charIeszhao
changed the title
COMPARE TO
|
| Name | Diff |
|---|---|
| packages/core/src/libraries/one-time-token.ts | 📈 +135 Bytes |
| packages/core/src/queries/one-time-tokens.ts | 📈 +976 Bytes |
| packages/core/src/routes/one-time-tokens.ts | 📈 +12 Bytes |
| packages/integration-tests/src/tests/api/one-time-tokens.test.ts | 📈 +1.01 KB |
charIeszhao
force-pushed
the
charles-log-11241-fetched-token-status-does-not-updated-to-expired
branch
2 times, most recently
from
db069f1 to
03776ba
Compare
charIeszhao
force-pushed
the
charles-log-11241-fetched-token-status-does-not-updated-to-expired
branch
from
03776ba to
0d47ce5
Compare
simeng-li
approved these changes
Apr 9, 2025
charIeszhao
deleted the
charles-log-11241-fetched-token-status-does-not-updated-to-expired
branch
darcyYe
reviewed
Apr 9, 2025
Comment on lines
+88
to
+120
| const buildFindEntitiesSql = ( | ||
| where: Partial<Pick<OneTimeToken, 'id' | 'token' | 'email' | 'status'>>, | ||
| pagination?: { limit: number; offset: number } | ||
| ) => { | ||
| const whereEntries = Object.entries(where).filter(([_, value]) => Boolean(value)); | ||
| return sql` | ||
| select | ||
| ${sql.join( | ||
| Object.values(fields).filter((field) => field !== fields.status), | ||
| sql`, ` | ||
| )}, | ||
| case | ||
| when | ||
| ${fields.status} = ${OneTimeTokenStatus.Active} and | ||
| ${fields.expiresAt} < now() | ||
| then ${OneTimeTokenStatus.Expired} | ||
| else ${fields.status} | ||
| end as "status" | ||
| from ${table} | ||
| ${conditionalSql( | ||
| whereEntries.length > 0 && whereEntries, | ||
| (whereEntries) => | ||
| sql`where ${sql.join( | ||
| whereEntries.map(([column, value]) => | ||
| conditionalSql(value, (value) => sql`${sql.identifier([column])} = ${value}`) | ||
| ), | ||
| sql` and ` | ||
| )}` | ||
| )} | ||
| order by ${fields.createdAt} desc | ||
| ${conditionalSql(pagination, ({ limit, offset }) => sql`limit ${limit} offset ${offset}`)} | ||
| `; | ||
| }; |
There was a problem hiding this comment.
should we also update the status? o/w the query response will be different than the DB data.
There was a problem hiding this comment.
I'm afraid there will be a performance issue, since this logic is also used when fetching list, and there're chances that it ends up updating all the entities in the entire table. We can have an offline discussion if you want.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The
GETone-time token APIs should automatically calculate the expiration time of the tokens, and modify the status in return data toexpiredwhen necessary.Previously, the GET APIs return the DB entries directly. However, since we only update the token status on verifying tokens, those unverified ones don't have the opportunity to have their status updated even if the expiration time has passed. Therefore, the token status might be out-of-date in such situations.
Testing
Integration tests added
Checklist
.changeset