fix: preserve anonymous user on magic URL upgrade#11619
fix: preserve anonymous user on magic URL upgrade#11619abishekgiri wants to merge 2 commits intoappwrite:1.9.xfrom
Conversation
📝 WalkthroughWalkthroughThe changes implement logic to upgrade anonymous accounts to verified email accounts via the magic URL token flow. In the account controller's magic URL endpoint, code now detects when a user document represents an anonymous account (both Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi! This is my second contribution to Appwrite — happy to make any changes or adjustments if needed. I added a regression test and updated the magic URL flow to preserve the anonymous user during upgrade. Please let me know if there are any edge cases or improvements I should address. Thanks! |
Greptile SummaryThis PR fixes a bug where an authenticated anonymous user requesting a magic URL token would have the token attached to a freshly-created second account rather than their existing anonymous account, causing the anonymous account to be silently orphaned. The fix adds an Key observations:
Confidence Score: 3/5
Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (1)
3789-3792: Use structured query parsing instead of fixed-length token slicing.
substr(..., 64)is brittle if token length or email template changes. Reuse the existing link parsing helper for stability.♻️ Suggested refactor
- $token = substr($lastEmail['text'], strpos($lastEmail['text'], '&secret=', 0) + 8, 64); + $tokens = $this->extractQueryParamsFromEmailLink($lastEmail['html']); + $token = $tokens['secret'] ?? ''; + $this->assertNotEmpty($token);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Services/Account/AccountCustomClientTest.php` around lines 3789 - 3792, The token extraction using substr on $lastEmail['text'] is brittle; replace the fixed-length slice with the project's link parsing helper to robustly parse the URL and extract the "secret" query parameter. Locate the block using getLastEmailByAddress and swap the substr(...) line for a call to the existing link parsing helper (use it to find the verification/reset link in $lastEmail['text'] and then read the "secret" param), ensuring the extracted token is used the same way thereafter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/account.php`:
- Around line 2173-2191: In the $isAnonymousUser branch, replicate the
identity-email uniqueness check from the else/new-user branch (the same
validation that prevents claiming an email that already exists as an OAuth
identity) before proceeding with the $dbForProject->updateDocument(...) call,
and after updating create the email messaging target the same way the new-user
flow and the /v1/account/tokens/email endpoint do (reuse the helper or method
used there) so the upgraded anonymous user has an email target; reference the
existing symbols: $isAnonymousUser, Email, $authorization->skip,
$dbForProject->updateDocument(... 'users' ...), and
$dbForProject->purgeCachedDocument to find where to insert the identity check
and the email-target creation.
In `@tests/e2e/Services/Account/AccountCustomClientTest.php`:
- Line 3759: Add a short PHPDoc block above the
testConvertAnonymousAccountMagicUrl method describing the regression scenario it
covers (e.g., converting anonymous accounts via magic URL flow), include a
one-line summary, any relevant `@covers` or `@group` tags per project conventions,
and ensure the docblock follows the project's PHPDoc style and formatting
guidelines.
---
Nitpick comments:
In `@tests/e2e/Services/Account/AccountCustomClientTest.php`:
- Around line 3789-3792: The token extraction using substr on $lastEmail['text']
is brittle; replace the fixed-length slice with the project's link parsing
helper to robustly parse the URL and extract the "secret" query parameter.
Locate the block using getLastEmailByAddress and swap the substr(...) line for a
call to the existing link parsing helper (use it to find the verification/reset
link in $lastEmail['text'] and then read the "secret" param), ensuring the
extracted token is used the same way thereafter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a8568f8-8ec0-403b-b2d9-68911731563d
📒 Files selected for processing (2)
app/controllers/api/account.phptests/e2e/Services/Account/AccountCustomClientTest.php
| } elseif ($isAnonymousUser) { | ||
| try { | ||
| $emailCanonical = new Email($email); | ||
| } catch (Throwable) { | ||
| $emailCanonical = null; | ||
| } | ||
|
|
||
| $user = $authorization->skip(fn () => $dbForProject->updateDocument('users', $user->getId(), new Document([ | ||
| 'email' => $email, | ||
| 'emailVerification' => false, | ||
| 'search' => implode(' ', [$user->getId(), $email]), | ||
| 'emailCanonical' => $emailCanonical?->getCanonical(), | ||
| 'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(), | ||
| 'emailIsCorporate' => $emailCanonical?->isCorporate(), | ||
| 'emailIsDisposable' => $emailCanonical?->isDisposable(), | ||
| 'emailIsFree' => $emailCanonical?->isFree(), | ||
| ]))); | ||
|
|
||
| $dbForProject->purgeCachedDocument('users', $user->getId()); |
There was a problem hiding this comment.
Missing identity email check and email target creation for anonymous user upgrade.
The new branch for upgrading anonymous users is missing two important pieces that exist in the else branch (new user creation):
-
Identity email check: The
elsebranch (lines 2203-2208) validates that the email isn't already used in an OAuth identity. Without this check, an anonymous user could claim an email that belongs to another user's OAuth identity, potentially causing account conflicts or security issues. -
Email target creation: When creating a new user, an email target is created for the messaging system (see the pattern in
/v1/account/tokens/emailendpoint). The upgraded anonymous user won't have an email target, preventing them from receiving email notifications through the messaging system.
🔧 Proposed fix
} elseif ($isAnonymousUser) {
+ // Makes sure this email is not already used in another identity
+ $identityWithMatchingEmail = $dbForProject->findOne('identities', [
+ Query::equal('providerEmail', [$email]),
+ ]);
+ if (!$identityWithMatchingEmail->isEmpty()) {
+ throw new Exception(Exception::USER_EMAIL_ALREADY_EXISTS);
+ }
+
try {
$emailCanonical = new Email($email);
} catch (Throwable) {
$emailCanonical = null;
}
$user = $authorization->skip(fn () => $dbForProject->updateDocument('users', $user->getId(), new Document([
'email' => $email,
'emailVerification' => false,
'search' => implode(' ', [$user->getId(), $email]),
'emailCanonical' => $emailCanonical?->getCanonical(),
'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(),
'emailIsCorporate' => $emailCanonical?->isCorporate(),
'emailIsDisposable' => $emailCanonical?->isDisposable(),
'emailIsFree' => $emailCanonical?->isFree(),
])));
+ // Create email target for messaging
+ try {
+ $target = $authorization->skip(fn () => $dbForProject->createDocument('targets', new Document([
+ '$permissions' => [
+ Permission::read(Role::user($user->getId())),
+ Permission::update(Role::user($user->getId())),
+ Permission::delete(Role::user($user->getId())),
+ ],
+ 'userId' => $user->getId(),
+ 'userInternalId' => $user->getSequence(),
+ 'providerType' => MESSAGE_TYPE_EMAIL,
+ 'identifier' => $email,
+ ])));
+ $user->setAttribute('targets', [...$user->getAttribute('targets', []), $target]);
+ } catch (Duplicate) {
+ $existingTarget = $dbForProject->findOne('targets', [
+ Query::equal('identifier', [$email]),
+ ]);
+ if (!$existingTarget->isEmpty()) {
+ $user->setAttribute('targets', $existingTarget, Document::SET_TYPE_APPEND);
+ }
+ }
+
$dbForProject->purgeCachedDocument('users', $user->getId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/api/account.php` around lines 2173 - 2191, In the
$isAnonymousUser branch, replicate the identity-email uniqueness check from the
else/new-user branch (the same validation that prevents claiming an email that
already exists as an OAuth identity) before proceeding with the
$dbForProject->updateDocument(...) call, and after updating create the email
messaging target the same way the new-user flow and the /v1/account/tokens/email
endpoint do (reuse the helper or method used there) so the upgraded anonymous
user has an email target; reference the existing symbols: $isAnonymousUser,
Email, $authorization->skip, $dbForProject->updateDocument(... 'users' ...), and
$dbForProject->purgeCachedDocument to find where to insert the identity check
and the email-target creation.
| $this->assertEquals(401, $response['headers']['status-code']); | ||
| } | ||
|
|
||
| public function testConvertAnonymousAccountMagicUrl(): void |
There was a problem hiding this comment.
Add PHPDoc for the new test method.
Please add a short docblock describing the regression scenario covered by this test.
As per coding guidelines, "Include comprehensive PHPDoc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Services/Account/AccountCustomClientTest.php` at line 3759, Add a
short PHPDoc block above the testConvertAnonymousAccountMagicUrl method
describing the regression scenario it covers (e.g., converting anonymous
accounts via magic URL flow), include a one-line summary, any relevant `@covers`
or `@group` tags per project conventions, and ensure the docblock follows the
project's PHPDoc style and formatting guidelines.
1 participant
Fixes #9465
Summary
Anonymous users who request a magic URL token should keep the same account when they upgrade.
Before this change,
POST /account/tokens/magic-urllooked up or created a user by email and could attach the token to a second account instead of the authenticated anonymous account.This change preserves the existing anonymous user by attaching the submitted email to that user before issuing the magic URL token.
Changes
Testing
docker compose exec appwrite test /usr/src/code/tests/e2e/Services/Account --filter=testConvertAnonymousAccountMagicUrl
docker compose exec appwrite test /usr/src/code/tests/e2e/Services/Account --filter='/(testConvertAnonymousAccount|testConvertAnonymousAccountMagicUrl|testCreateMagicUrl|testCreateSessionWithMagicUrl)/'
Both test runs passed.