feat(account): add expire param to account token endpoints - #13726
HarshMN2345 wants to merge 30 commits into
Conversation
…n magic URL and email tokens
# Conflicts: # app/controllers/api/account.php
 |
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
| "emails.magicSession.preview": "Sign in to {{project}} with your secure link. Expires at {{expire}}.", | ||
| "emails.magicSession.hello": "Hello {{user}},", | ||
| "emails.magicSession.optionButton": "Click the button below to securely sign in to your {{b}}{{project}}{{/b}} account. This link will expire in 1 hour.", | ||
| "emails.magicSession.optionButton": "Click the button below to securely sign in to your {{b}}{{project}}{{/b}} account. Expires at {{expire}}.", |
There was a problem hiding this comment.
Lets pls have screenshot of devmail with this email, to mamake sure the value is readable well.
Because it was relative before. So we should keep it relative. But how would relative look? "Expires at 5 minutes"? Lets double check and fix if needed.
And lets keep locale in mind. Pluralization is pretty big problem, .. Maybe it wont be possible easily. Maybe we will need to switch to absolute time, but then lets have screenshot and get leads/eldad approval for this change.
There was a problem hiding this comment.
Before / after / fr with expire: 300:
Drop the length option and its privileged ranges. Each endpoint now accepts expire from 60 seconds up to its existing default lifetime for every caller, and generates secrets exactly as before, so the code templates no longer need to wrap long codes. Emails show the expiry as a UTC date and time instead of the raw ISO timestamp. The recovery and email verification link emails do not render an expiry, so they no longer receive the variable.
Assert the default, explicit null, minimum and maximum lifetimes, the delivered expiry, and redemption for each endpoint, and reject values outside the range for client and API key callers. Remove the length and GraphQL generator isolation cases along with the option.
| ->param('email', '', new EmailValidator(), 'User email.') | ||
| ->param('url', '', fn ($redirectValidator) => $redirectValidator, 'URL to redirect the user back to your app from the magic URL login. Only URLs from hostnames in your project platform list are allowed. This requirement helps to prevent an [open redirect](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html) attack against your project API.', true, ['redirectValidator']) | ||
| ->param('phrase', false, new Boolean(), 'Toggle for security phrase. If enabled, email will be send with a randomly generated phrase and the phrase will also be included in the response. Confirming phrases match increases the security of your authentication flow.', true) | ||
| ->param('expire', TOKEN_EXPIRATION_CONFIRM, new Range(60, TOKEN_EXPIRATION_CONFIRM), 'Token expiration period in seconds. The default and maximum expiration is 1 hour.', true) |
There was a problem hiding this comment.
The changes since the previous review remove the length parameter from all eight account flows and cap expire at each flow's existing default for every caller. For example, this route no longer includes length in its REST, GraphQL, or SDK contract and rejects expiration values above 3,600 seconds. The same removal appears in the other account token routes and the MFA challenge route. As a result, callers cannot configure token length, and trusted callers cannot request the documented extended lifetime of up to one year, so the main feature promised by this PR is unavailable.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/api/account.php
Line: 2346
Comment:
**Token Configuration Was Removed**
The changes since the previous review remove the `length` parameter from all eight account flows and cap `expire` at each flow's existing default for every caller. For example, this route no longer includes `length` in its REST, GraphQL, or SDK contract and rejects expiration values above 3,600 seconds. The same removal appears in the other account token routes and the MFA challenge route. As a result, callers cannot configure token length, and trusted callers cannot request the documented extended lifetime of up to one year, so the main feature promised by this PR is unavailable.
**Knowledge Base Used:**
- [Account authentication and identity](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/account-authentication.md)
- [API platform and request surface](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/api-platform.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…rite into codex/feat-13636-account-token-options
|
Looked into this. utopia-php/locale has no plural support, so I'd add it there rather than start a new library. ICU already has CLDR plural rules for every language, even with the English-only ICU data in our image, so no image change is needed. What's missing is the localized "in X minutes" wording. Getting that at runtime needs Proposal: add I'd generate The alternative, if you want the library intl-free, is Order: locale PR, release, then bump and translations in this PR. OK with this direction before I start? |
Magic URL, email OTP, email verification OTP and MFA emails say when the code or link expires relative to now, with plural forms from CLDR for each language. Phrases are filled in the language of the sentence that uses them, so English fallback sentences get English phrases. Whole hours read as hours, anything else as minutes rounded down. Translations that had drifted into another language (pa, or, si, sa, sn) are rewritten in the file's language.
| $phrase = match ($expire) { | ||
| 60 => 'in 1 minute', | ||
| 900 => 'in 15 minutes', | ||
| 3600 => 'in 1 hour', | ||
| }; |
There was a problem hiding this comment.
These assertions hard-code the exact English pluralized text from the production translation templates. This violates the repository directive to test observable behavior instead of mirroring source text or configuration. The token lifetime is already checked independently through the returned creation and expiration timestamps, so these copy assertions add no independent expiry protection and will fail on harmless translation changes. This repository requirement must be satisfied before merging. The same issue appears in tests/e2e/Services/Account/AccountBase.php:218 and tests/e2e/Services/Account/AccountCustomClientTest.php:4436.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Services/Account/TokensBase.php
Line: 110-114
Comment:
**Tests Mirror Translation Copy**
These assertions hard-code the exact English pluralized text from the production translation templates. This violates the repository directive to test observable behavior instead of mirroring source text or configuration. The token lifetime is already checked independently through the returned creation and expiration timestamps, so these copy assertions add no independent expiry protection and will fail on harmless translation changes. This repository requirement must be satisfied before merging. The same issue appears in `tests/e2e/Services/Account/AccountBase.php:218` and `tests/e2e/Services/Account/AccountCustomClientTest.php:4436`.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What does this PR do?
Account tokens, verification codes and MFA challenges always use a fixed lifetime. This adds an optional
expireparameter, in seconds, to the eight endpoints that create them, so apps can shorten that lifetime. The existing default is also the maximum for every caller, API keys and console sessions included. The minimum is 60 seconds, and an explicitnullkeeps the default.POST /v1/account/tokens/magic-urlPOST /v1/account/tokens/emailPOST /v1/account/tokens/phonePOST /v1/account/recoveryPOST /v1/account/verifications/emailPOST /v1/account/verifications/email/otpPOST /v1/account/verifications/phonePOST /v1/account/mfa/challengesSecrets are generated as before, and the response
expireand theexpirequery param in email links are unchanged.The magic URL, email OTP, email verification OTP and MFA emails keep relative wording, now with the real lifetime and correct plural forms per language: "This code will expire in 5 minutes.", "Он истечет через 5 минут.", "Platnost kódu vyprší za 1 minutu.", "21 分後に有効期限が切れます。" Whole hours read as hours, anything else as minutes rounded down.
getPlural()and apluralsargument ongetText()(feat: add plural translations utopia-php/locale#20). Phrases are ICU MessageFormat patterns formatted with CLDR plural rules, which ICU ships even with the English-only ICU data in our image. A phrase is formatted in the language of the sentence it fills, so English fallback sentences such as most previews get English phrases.emails.expire.minutesandemails.expire.hoursare generated from CLDR relative-time data for 67 files, with Hebrew and Bhojpuri written by hand. la, lb, sa, sd and sn have no usable CLDR data and fall back to English. The 154 expiry sentences now contain{{expire}}; each was checked with 1, 2, 5, 11, 15, 21 and 22 minutes and 1 and 2 hours. Strings in pa, or, si, sa and sn that were in another language are rewritten in the file's language.app/init/locales.phppasses the loaded file's name as the plural rules locale, so codes that load another file (e.g.srloading en.json) use that file's rules.{{expire}}, formatted in the request language.Two reference docs had wrong lifetimes and are corrected: email verification links last 1 hour, not 7 days, and phone verification codes last 1 hour, not 15 minutes.
Test Plan
New e2e tests:
testCreateTokenExpirecovers magic URL, email and phone, on the client and API key suites.testCreateRecoveryExpirecovers the client and API key suites.testCreateEmailVerificationExpire,testCreateEmailVerificationOTPExpire,testCreatePhoneVerificationExpireandtestCreateMFAChallengeExpire(email and phone) cover the session endpoints.Each one creates tokens with
expireomitted,null, 60 and the maximum. It checks the lifetime, reads the delivered email or SMS, redeems it, and expects400 general_argument_invalidfor 59 and for the maximum + 1.Run against a local stack built from this branch:
testEmailOTPSessionpasses.Before merge: release utopia-php/locale with #20 and replace
dev-feat-plurals as 0.9.0in composer.json with the release.Related PRs and Issues
89103c3d0c, with its original authorship.expiryparameter to Magic URL and Email OTP #13636.Checklist