Users add new email attributes #10688
Conversation
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
…rs-add-attributes
📝 WalkthroughWalkthroughThis PR adds five new public user attributes to the Users collection: emailCanonical (string) and four booleans — emailIsFree, emailIsDisposable, emailIsCorporate, emailIsCanonical. Controllers (account, teams, users) switch parameter validation to Appwrite\Network\Validator\Email aliased as EmailValidator, and attempt to canonicalize provided emails using Utopia\Emails\Email inside try/catch blocks. The canonical string and related boolean flags are attached to created/updated user documents, tokens, sessions, and membership/invite flows. An end-to-end test (tests/e2e/Services/Account/AccountBase.php) was modified to dump a response body and includes an always-failing assertion. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Areas needing extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/controllers/api/users.php (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/config/collections/common.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/api/account.php (2)
379-405: Missing canonical email attributes in account creation.The POST
/v1/accountendpoint creates users with email (line 386) but doesn't set the canonical email attributes that are being added to the schema. This is inconsistent with the email token creation flow (lines 2231-2260) which does set them.Impact: Users created via email/password won't have email canonicalization metadata, leading to:
- Incomplete data for analytics/filtering on email types
- Inconsistent user records across different registration methods
Apply this diff to add canonical email attributes:
+ $emailCanonical = new EmailCanonical($email); try { $userId = $userId == 'unique()' ? ID::unique() : $userId; $user->setAttributes([ '$id' => $userId, '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::user($userId)), Permission::delete(Role::user($userId)), ], 'email' => $email, 'emailVerification' => false, + 'emailCanonical' => $emailCanonical->getCanonical(), + 'emailIsCanonical' => $emailCanonical->isCanonicalSupported(), + 'emailIsCorporate' => $emailCanonical->isCorporate(), + 'emailIsDisposable' => $emailCanonical->isDisposable(), + 'emailIsFree' => $emailCanonical->isFree(), 'status' => true, 'password' => $password,
1981-2005: Missing canonical email attributes in magic URL user creation.When creating a new user via the magic URL token flow, email is set (line 1988) but canonical email attributes are not. This is the same inconsistency found in the account creation endpoint.
Impact: Users created via magic URL won't have email canonicalization metadata.
Apply this diff:
$userId = $userId === 'unique()' ? ID::unique() : $userId; + $emailCanonical = new EmailCanonical($email); $user->setAttributes([ '$id' => $userId, '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::user($userId)), Permission::delete(Role::user($userId)), ], 'email' => $email, 'emailVerification' => false, + 'emailCanonical' => $emailCanonical->getCanonical(), + 'emailIsCanonical' => $emailCanonical->isCanonicalSupported(), + 'emailIsCorporate' => $emailCanonical->isCorporate(), + 'emailIsDisposable' => $emailCanonical->isDisposable(), + 'emailIsFree' => $emailCanonical->isFree(), 'status' => true, 'password' => null,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/account.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/account.php (1)
src/Appwrite/Auth/OAuth2/Oidc.php (1)
getUserEmail(130-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
app/controllers/api/account.php (2)
60-60: LGTM on the import statement.The import and alias are clear and appropriate for the email canonicalization functionality.
2231-2260: LGTM! Clean implementation of canonical email attributes.This code correctly:
- Creates the
EmailCanonicalobject once from the validated email- Sets all five canonical attributes during user creation
- Leverages the existing email validation from the param validator
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/controllers/api/teams.php (1)
606-610: Duplicate issue: unhandled exceptions from EmailCanonical methods.The same TODO comments and potential exception handling issues exist here as in users.php (lines 134-138). The
isDisposable()andisFree()methods may throw exceptions that are not caught.This affects the team invitation flow when creating new users. Please address this consistently across both files.
app/controllers/api/account.php (1)
1594-1621: Critical: OAuth2 new users still missing canonical email attributes.This is the same critical issue flagged in the previous review. When new OAuth2 users are created with an email on line 1603, the canonical email attributes (emailCanonical, emailIsCanonical, emailIsCorporate, emailIsDisposable, emailIsFree) are not set. The subsequent check at line 1690 for
empty($user->getAttribute('email'))will be false for these newly created users, so that block is skipped and canonical attributes are never populated.Apply this fix to add canonical email attributes during OAuth2 user creation:
$identityWithMatchingEmail = $dbForProject->findOne('identities', [ Query::equal('providerEmail', [$email]), ]); if (!$identityWithMatchingEmail->isEmpty()) { $failureRedirect(Exception::GENERAL_BAD_REQUEST); /** Return a generic bad request to prevent exposing existing accounts */ } + try { + $emailCanonical = new EmailCanonical($email); + } catch (Throwable) { + $emailCanonical = null; + } + try { $userId = ID::unique(); $user->setAttributes([ '$id' => $userId, '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::user($userId)), Permission::delete(Role::user($userId)), ], 'email' => $email, 'emailVerification' => true, 'status' => true, // Email should already be authenticated by OAuth2 provider 'password' => null, 'hash' => Auth::DEFAULT_ALGO, 'hashOptions' => Auth::DEFAULT_ALGO_OPTIONS, 'passwordUpdate' => null, 'registration' => DateTime::now(), 'reset' => false, 'name' => $name, 'mfa' => false, 'prefs' => new \stdClass(), 'sessions' => null, 'tokens' => null, 'memberships' => null, 'authenticators' => null, 'search' => implode(' ', [$userId, $email, $name]), 'accessedAt' => DateTime::now(), + 'emailCanonical' => $emailCanonical?->getCanonical(), + 'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(), + 'emailIsCorporate' => $emailCanonical?->isCorporate(), + 'emailIsDisposable' => $emailCanonical?->isDisposable(), + 'emailIsFree' => $emailCanonical?->isFree(), ]);
🧹 Nitpick comments (1)
app/controllers/api/account.php (1)
1690-1703: Inconsistent error handling: silent failure.While this code sets canonical attributes for OAuth2 users without email, it uses a different error handling pattern compared to account creation (lines 378-382) and email token creation (lines 2247-2251). The empty catch block silently suppresses any exceptions, making debugging difficult if the EmailCanonical library throws unexpected errors.
Consider using the same pattern as lines 378-382 for consistency:
if (empty($user->getAttribute('email'))) { $user->setAttribute('email', $oauth2->getUserEmail($accessToken)); try { $emailCanonical = new EmailCanonical($user->getAttribute('email')); - - $user->setAttribute('emailCanonical', $emailCanonical->getCanonical()); - $user->setAttribute('emailIsCanonical', $emailCanonical->isCanonicalSupported()); - $user->setAttribute('emailIsCorporate', $emailCanonical->isCorporate()); - $user->setAttribute('emailIsDisposable', $emailCanonical->isDisposable()); - $user->setAttribute('emailIsFree', $emailCanonical->isFree()); } catch (Throwable) { + $emailCanonical = null; } + + $user + ->setAttribute('emailCanonical', $emailCanonical?->getCanonical()) + ->setAttribute('emailIsCanonical', $emailCanonical?->isCanonicalSupported()) + ->setAttribute('emailIsCorporate', $emailCanonical?->isCorporate()) + ->setAttribute('emailIsDisposable', $emailCanonical?->isDisposable()) + ->setAttribute('emailIsFree', $emailCanonical?->isFree()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/account.php(7 hunks)app/controllers/api/teams.php(3 hunks)app/controllers/api/users.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (6)
app/controllers/api/users.php (1)
52-52: LGTM!The import statement correctly aliases the
Utopia\Emails\Emailclass asEmailCanonicalfor use in email canonicalization.app/controllers/api/teams.php (2)
51-51: LGTM!The import statement is consistent with the same change in users.php.
568-572: No action required—the exception handling is already in place.The original concern states that "the EmailCanonical constructor may throw an exception that would not be caught," but this is contradicted by the code: lines 569–572 explicitly wrap the instantiation in a
try-catchblock that catchesThrowableexceptions.The code flow confirms that
try-catchpattern here is intentional—the exception is caught, and$emailCanonicalis safely set tonullif validation fails. This aligns with how the code later handles the result at line 584:'email' => empty($email) ? null : $email.Likely an incorrect or invalid review comment.
app/controllers/api/account.php (3)
60-60: LGTM!The import alias is well-chosen and follows clear naming conventions.
378-417: Good defensive error handling for canonical email attributes.The try-catch block with null fallback and null-safe operators ensures account creation proceeds even if canonical email computation fails. This is appropriate for a non-critical enhancement.
2247-2280: LGTM!The error handling pattern here is consistent with account creation (lines 378-382): try-catch with null fallback and null-safe operators for attribute setting. This ensures user creation succeeds even if canonical email computation fails.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/controllers/api/teams.php (1)
569-611: Consider extracting email canonicalization logic into a helper function.This email canonicalization pattern (constructing
EmailCanonical, handling errors, and populating user document fields) appears to be repeated across multiple controllers (account.php, users.php, and teams.php per the AI summary).Consider extracting this into a shared helper method to improve maintainability and ensure consistency.
Example structure:
// In a helper class (e.g., src/Appwrite/Auth/EmailHelper.php) public static function getCanonicalEmailFields(?string $email): array { if (empty($email)) { return [ 'emailCanonical' => null, 'emailIsCanonical' => null, 'emailIsCorporate' => null, 'emailIsDisposable' => null, 'emailIsFree' => null, ]; } try { $emailCanonical = new EmailCanonical($email); return [ 'emailCanonical' => $emailCanonical->getCanonical(), 'emailIsCanonical' => $emailCanonical->isCanonicalSupported(), 'emailIsCorporate' => $emailCanonical->isCorporate(), 'emailIsDisposable' => $emailCanonical->isDisposable(), 'emailIsFree' => $emailCanonical->isFree(), ]; } catch (SpecificException $e) { // Log the error for debugging return [ 'emailCanonical' => null, 'emailIsCanonical' => null, 'emailIsCorporate' => null, 'emailIsDisposable' => null, 'emailIsFree' => null, ]; } }Then use it consistently across all controllers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/teams.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/teams.php (1)
src/Appwrite/Auth/Auth.php (3)
Auth(18-515)passwordHash(220-257)passwordGenerator(317-320)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (2)
app/controllers/api/teams.php (2)
51-51: LGTM! Clean import alias.The EmailCanonical alias clearly distinguishes the email canonicalization utility from the validator imported on line 13.
609-610: Clarify and handle exceptions from EmailCanonical::isDisposable()/::isFree()Location: app/controllers/api/teams.php: lines 609–610.
- Null-safe operator ($emailCanonical?->...) only guards against null; it does not catch exceptions — the "todo: fix throw" implies these methods may throw.
- Repo search did not locate isDisposable()/isFree() implementations — confirm whether they throw. If they can throw, either wrap these calls in try/catch and return a safe default (e.g., false/null) or add a non-throwing accessor on EmailCanonical.
- Standardize the comment to "TODO: ..." for consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Utopia/Response/Model/User.php(1 hunks)tests/e2e/Services/Account/AccountBase.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Utopia/Response/Model/User.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
…rs-add-attributes # Conflicts: # composer.lock
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/controllers/api/teams.php (1)
570-574: Validate email before canonicalization to prevent empty string instantiation.The
$invitee->getAttribute('email', '')). Passing an empty string toEmailCanonicalmay not be appropriate. Consider adding an empty check:- try { - $emailCanonical = new EmailCanonical($email); - } catch (Throwable) { - $emailCanonical = null; - } + $emailCanonical = null; + if (!empty($email)) { + try { + $emailCanonical = new EmailCanonical($email); + } catch (Throwable) { + $emailCanonical = null; + } + }Based on past review analysis.
app/controllers/api/account.php (2)
1711-1724: Critical: OAuth2 new users still missing canonical email attributes.The previous review comment on these lines has NOT been addressed. New OAuth2 users created at lines 1615-1659 are instantiated with an email (line 1624), so when execution reaches line 1711, the condition
if (empty($user->getAttribute('email')))evaluates to false and this entire block is skipped. As a result, new OAuth2 users will NOT have canonical email attributes set.The fix should add canonical attribute setting in the new user creation block (around lines 1615-1642) where the email is first assigned.
3111-3120: Major: Email update still missing error handling for EmailCanonical.The previous review comment on lines 3111-3121 has NOT been addressed. Unlike account creation (lines 399-403) and email token creation (lines 2268-2272), this email update path does not wrap EmailCanonical instantiation in a try-catch block. If the EmailCanonical library throws an exception for an edge-case email format, the user's email update will fail entirely—degrading user experience for what should be a non-critical metadata enhancement.
Wrap the instantiation in try-catch and use null-safe operators when setting attributes, consistent with the other email flows.
🧹 Nitpick comments (2)
app/controllers/api/users.php (1)
101-105: Guard EmailCanonical instantiation with an empty check instead of null coalescing.While the null coalescing operator prevents passing
nulltoEmailCanonical, passing an empty string whenApply this pattern to avoid unnecessary instantiation:
- try { - $emailCanonical = new EmailCanonical($email ?? ''); - } catch (Throwable) { - $emailCanonical = null; - } + $emailCanonical = null; + if (!empty($email)) { + try { + $emailCanonical = new EmailCanonical($email); + } catch (Throwable) { + $emailCanonical = null; + } + }app/controllers/api/teams.php (1)
590-590: Remove or resolve the TODO comment about password handling.The TODO at line 590 suggests uncertainty about the password handling approach. However, the comment at lines 594-598 clearly explains that the random password is intentional to allow password updates without requiring an old password. Either:
- Remove the TODO if the current approach is finalized, or
- Document the decision more clearly
If the current approach is correct, apply this diff:
- // TODO: Set password empty? 'password' => Auth::passwordHash(Auth::passwordGenerator(), Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS),As per past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/account.php(7 hunks)app/controllers/api/teams.php(2 hunks)app/controllers/api/users.php(3 hunks)
🔇 Additional comments (4)
app/controllers/api/users.php (1)
134-138: Verify exception handling for EmailCanonical method calls.Previous reviews indicated that
isDisposable()andisFree()can throw exceptions when data files are unavailable. While marked as addressed in commit 5031457, the current code only wraps the constructor in a try-catch (lines 101-105). The null-safe operator (?->) prevents calls on null but doesn't catch exceptions thrown during method execution.Please confirm whether:
- These methods have been updated to never throw exceptions, or
- Additional exception handling is needed around these method calls
If exceptions can still occur, wrap each attribute assignment in exception handling:
- 'emailCanonical' => $emailCanonical?->getCanonical(), - 'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(), - 'emailIsCorporate' => $emailCanonical?->isCorporate(), - 'emailIsDisposable' => $emailCanonical?->isDisposable(), - 'emailIsFree' => $emailCanonical?->isFree(), + 'emailCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->getCanonical(); } catch (Throwable) { return null; } + })() : null, + 'emailIsCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCanonicalSupported(); } catch (Throwable) { return false; } + })() : false, + 'emailIsCorporate' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCorporate(); } catch (Throwable) { return false; } + })() : false, + 'emailIsDisposable' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isDisposable(); } catch (Throwable) { return false; } + })() : false, + 'emailIsFree' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isFree(); } catch (Throwable) { return false; } + })() : false,Based on past review analysis.
app/controllers/api/teams.php (1)
608-612: Verify exception handling for EmailCanonical method calls.Similar to
users.php, these method calls use the null-safe operator but may still throw exceptions during execution. Previous reviews indicated thatisDisposable()andisFree()can throw when data files are unavailable.If these methods can throw exceptions, consider wrapping each call in exception handling. See the pattern suggested for
users.phplines 134-138.Based on past review analysis.
app/controllers/api/account.php (2)
399-403: LGTM: Proper error handling in account creation.The try-catch block ensures that EmailCanonical instantiation failures won't break account creation, and the null-safe operator correctly handles the null case when setting attributes.
Also applies to: 433-437
2268-2272: LGTM: Consistent error handling in email token creation.The implementation matches the account creation flow with proper try-catch and null-safe operators, ensuring email token creation doesn't fail if canonical email processing encounters an error.
Also applies to: 2296-2300
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/account.php (1)
1723-1736: Optimize: Reuse existing email variable.Line 1724 calls
$oauth2->getUserEmail($accessToken)but theApply this optimization:
- if (empty($user->getAttribute('email'))) { - $user->setAttribute('email', $oauth2->getUserEmail($accessToken)); + if (empty($user->getAttribute('email'))) { + $user->setAttribute('email', $email); try { - $emailCanonical = new EmailCanonical($user->getAttribute('email')); + $emailCanonical = new EmailCanonical($email); } catch (Throwable) { $emailCanonical = null; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/account.php(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Linter
- GitHub Check: CodeQL
- GitHub Check: scan
🔇 Additional comments (7)
app/controllers/api/account.php (7)
60-60: LGTM! Clear aliasing for email canonicalization utility.The import statement clearly indicates the purpose of the utility through the alias.
398-403: LGTM! Proper error handling for email canonicalization in account creation.The try-catch block prevents user creation failures if the EmailCanonical library throws an exception, and the nullsafe operators ensure graceful degradation.
Also applies to: 433-437
1615-1619: LGTM! Critical OAuth2 issue from past reviews has been addressed.New OAuth2 users now receive canonical email attributes during account creation. Previously, the canonical attributes were only set in a conditional block that was skipped for newly created users.
Also applies to: 1648-1652
2031-2035: LGTM! Consistent error handling for magic URL flow.Email canonicalization is properly handled with try-catch and nullsafe operators, consistent with other authentication flows.
Also applies to: 2061-2065
2292-2296: LGTM! Consistent error handling for email token flow.Email canonicalization follows the same reliable pattern used throughout the file.
Also applies to: 2320-2324
2672-2676: LGTM! Correct handling for phone-only users.Setting email canonical attributes to null for phone authentication is the appropriate approach, as these users don't have an email address.
3140-3144: LGTM! Email update now includes proper error handling.The try-catch block addresses the concern from previous reviews, ensuring that email updates don't fail if the EmailCanonical library encounters an edge-case email format.
Also applies to: 3149-3153
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/api/users.php (1)
1461-1465: Duplicate: Verify exception safety of EmailCanonical methods.Same concern as lines 134-138—if
isDisposable()andisFree()can throw exceptions, the user update operation will fail. Please verify these methods are exception-safe or wrap the calls appropriately.
🧹 Nitpick comments (2)
app/controllers/api/users.php (2)
101-105: Consider guarding against empty email before instantiation.The try-catch handles exceptions gracefully, but since
+if (!empty($email)) { try { $emailCanonical = new EmailCanonical($email); } catch (Throwable) { $emailCanonical = null; } +} else { + $emailCanonical = null; +}This makes the intent explicit and avoids relying on exception handling for expected conditions.
1452-1456: Consider guarding against empty email before instantiation.Similar to lines 101-105, the email could be an empty string at this point (as checked by the
strlen($email) !== 0condition above). Consider checking before instantiation:+if (\strlen($email) !== 0) { try { $emailCanonical = new EmailCanonical($email); } catch (Throwable) { $emailCanonical = null; } +} else { + $emailCanonical = null; +}This avoids relying on exception handling for expected empty-email scenarios.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/users.php(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/users.php (1)
52-52: LGTM: Clean import aliasing.The EmailCanonical alias is clear and aligns with the usage pattern across other controllers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/controllers/api/teams.php (2)
590-590: [Duplicate] Remove or clarify TODO comment about password handling.This TODO was previously flagged. The code intentionally generates a random password (lines 591-593) to allow team-invited users to set a password later without providing an old password (explained in the comment at lines 594-598).
Either remove the TODO if the current approach is correct, or clarify the intended behavior.
Suggested clarification:
- // TODO: Set password empty? + // Random password generated for team-invited users; allows password updates without old password 'password' => Auth::passwordHash(Auth::passwordGenerator(), Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS),
570-574: [Duplicate] Empty email validation missing and unprotected method calls can throw exceptions.Two issues previously flagged but still present:
Empty email not validated before canonicalization: At line 570,
$email = $invitee->getAttribute('email', '');). Attempting to canonicalize an empty string is unnecessary and may cause unexpected behavior.Unprotected method calls: Similar to users.php, the constructor at lines 570-574 is protected, but method calls at lines 608-612 are not. If
isDisposable()orisFree()throw exceptions, user creation will fail.Apply these fixes:
+ if (!empty($email)) { try { $emailCanonical = new Email($email); } catch (Throwable) { $emailCanonical = null; } + } else { + $emailCanonical = null; + }And wrap method calls similarly to the suggestion for users.php:
- 'emailCanonical' => $emailCanonical?->getCanonical(), - 'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(), - 'emailIsCorporate' => $emailCanonical?->isCorporate(), - 'emailIsDisposable' => $emailCanonical?->isDisposable(), - 'emailIsFree' => $emailCanonical?->isFree(), + 'emailCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->getCanonical(); } catch (Throwable) { return null; } + })() : null, + 'emailIsCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCanonicalSupported(); } catch (Throwable) { return null; } + })() : null, + 'emailIsCorporate' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCorporate(); } catch (Throwable) { return false; } + })() : false, + 'emailIsDisposable' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isDisposable(); } catch (Throwable) { return false; } + })() : false, + 'emailIsFree' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isFree(); } catch (Throwable) { return false; } + })() : false,Also applies to: 608-612
app/controllers/api/users.php (2)
101-105: [Duplicate] Unprotected method calls can throw exceptions despite constructor protection.Past reviews identified that
isDisposable()andisFree()methods can throw exceptions when data files are invalid. While the constructor at lines 101-105 is wrapped in try-catch, the method calls at lines 134-138 remain unprotected. The null-safe operator (?->) prevents null pointer exceptions but does not catch exceptions thrown during method execution, which will crash user creation operations.This was previously flagged and marked as addressed, but the method calls still lack exception handling.
Wrap each method call in try-catch:
- 'emailCanonical' => $emailCanonical?->getCanonical(), - 'emailIsCanonical' => $emailCanonical?->isCanonicalSupported(), - 'emailIsCorporate' => $emailCanonical?->isCorporate(), - 'emailIsDisposable' => $emailCanonical?->isDisposable(), - 'emailIsFree' => $emailCanonical?->isFree(), + 'emailCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->getCanonical(); } catch (Throwable) { return null; } + })() : null, + 'emailIsCanonical' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCanonicalSupported(); } catch (Throwable) { return null; } + })() : null, + 'emailIsCorporate' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isCorporate(); } catch (Throwable) { return false; } + })() : false, + 'emailIsDisposable' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isDisposable(); } catch (Throwable) { return false; } + })() : false, + 'emailIsFree' => $emailCanonical ? (function() use ($emailCanonical) { + try { return $emailCanonical->isFree(); } catch (Throwable) { return false; } + })() : false,Also applies to: 134-138
1461-1465: [Duplicate] Unprotected method calls in email update path can throw exceptions.Similar to the user creation path, the email update path wraps the constructor (lines 1452-1456) but leaves method calls (lines 1461-1465) unprotected. If
isDisposable()orisFree()throw exceptions, the email update operation will fail.Additionally, this is compounded by the bug at line 1453 where the wrong class is being instantiated.
Apply the same protection pattern as suggested for lines 134-138, wrapping each method call in try-catch blocks with safe fallback values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/account.php(19 hunks)app/controllers/api/teams.php(4 hunks)app/controllers/api/users.php(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.
Applied to files:
app/controllers/api/teams.php
🧬 Code graph analysis (3)
app/controllers/api/users.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
app/controllers/api/account.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
app/controllers/api/teams.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (11)
app/controllers/api/users.php (1)
19-19: LGTM! Import alias and validator usage correctly updated.The changes properly separate concerns:
EmailValidatoralias forAppwrite\Network\Validator\Emailused for parameter validationUtopia\Emails\Emailused for canonicalization logicAll parameter declarations and inline validator instantiations correctly use
EmailValidator().Also applies to: 52-52, 223-223, 258-258, 293-293, 328-328, 363-363, 405-405, 440-440, 488-488, 542-542, 1417-1417, 1726-1726
app/controllers/api/teams.php (2)
13-13: LGTM! Import alias and validator usage correctly implemented.The changes properly separate the validator (
EmailValidator) from the canonicalization class (Also applies to: 51-51, 472-472
576-616: Well-structured user document creation for team invitations.The userDocument includes all necessary fields and properly uses
Authorization::skipfor the team invitation flow. The structure is consistent with thecreateUserfunction in users.php, including the canonical email attributes.app/controllers/api/account.php (8)
23-23: LGTM! Import aliases follow review feedback.The import structure properly separates validator and canonicalization concerns:
EmailValidatorfor parameter validationAlso applies to: 60-60
341-341: LGTM! Consistent parameter validation.All email parameter declarations consistently use
EmailValidatorfor validation.Also applies to: 920-920, 1985-1985, 2249-2249, 3105-3105, 3390-3390
399-437: LGTM! Robust error handling in account creation.The canonicalization flow properly handles failures:
- Try-catch prevents exceptions from aborting user creation
- Null-safe operators gracefully handle failed canonicalization
- Email is normalized to lowercase before processing
1615-1653: LGTM! OAuth2 canonical attributes properly set for new users.The OAuth2 flow correctly sets canonical attributes during new user creation (lines 1648-1652), addressing the critical issue from previous reviews. New users are created with both email and canonical attributes in a single operation.
1723-1737: LGTM! OAuth2 existing user email update handles canonicalization correctly.The flow for existing OAuth2 users with empty email properly:
- Sets email from OAuth provider
- Attempts canonicalization with error handling
- Sets canonical attributes using null-safe operators
2031-2065: LGTM! Token creation flows handle canonicalization consistently.Both Magic URL and Email OTP token creation flows:
- Wrap Email instantiation in try-catch blocks
- Use null-safe operators for canonical attributes
- Properly handle user creation with canonical data
Also applies to: 2292-2324
2672-2676: LGTM! Phone-based user creation correctly handles missing email.Phone authentication users appropriately have all email canonical attributes set to
nullsince they don't have an email address.
3140-3153: LGTM! Email update flow properly refreshes canonical attributes.The email update endpoint:
- Wraps canonicalization in try-catch for resilience
- Uses null-safe operators to handle failures gracefully
- Normalizes email to lowercase before processing
- Correctly resets email verification after update
2 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist