refactor(account): move MFA endpoints to module actions #10793
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (22)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change removes MFA-related API endpoints from the legacy account controller architecture and introduces a new modular Account service structure. The old public routes under Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
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 (1)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php (1)
170-201: Trigger the messaging queue for SMS challengesAfter configuring
$queueForMessaging, we never call->trigger(), so the SMS job is never enqueued and phone challenges silently fail. Please trigger the queue once the payload is ready.Apply this diff:
$queueForMessaging ->setType(MESSAGE_SEND_TYPE_INTERNAL) ->setMessage(new Document([ '$id' => $challenge->getId(), 'data' => [ 'content' => $code, ], ])) ->setRecipients([$phone]) - ->setProviderType(MESSAGE_TYPE_SMS); + ->setProviderType(MESSAGE_TYPE_SMS) + ->trigger();
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php (1)
64-72: Consider removing unused parameter.The
$requestTimestampparameter is injected but never used in the action method. If it's not needed for future auditing or other purposes, consider removing it to clean up the method signature.Apply this diff if the parameter is not needed:
public function action( bool $mfa, - ?\DateTime $requestTimestamp, Response $response, Document $user, Document $session, Database $dbForProject, Event $queueForEvents ): void {And remove the injection in the constructor:
->param('mfa', null, new Boolean(), 'Enable or disable MFA.') - ->inject('requestTimestamp') ->inject('response')src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (1)
89-97: Consider removing unused parameter.The
$requestTimestampparameter is injected but never used in the action method. If it's not needed for future auditing or other purposes, consider removing it to clean up the method signature.Apply this diff if the parameter is not needed:
public function action( string $type, - ?\DateTime $requestTimestamp, Response $response, Document $project, Document $user, Database $dbForProject, Event $queueForEvents ): void {And remove the injection in the constructor:
->param('type', null, new WhiteList([Type::TOTP]), 'Type of authenticator. Must be `' . Type::TOTP . '`') - ->inject('requestTimestamp') ->inject('response')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/controllers/api/account.php(0 hunks)src/Appwrite/Platform/Appwrite.php(2 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Factors/XList.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Module.php(1 hunks)src/Appwrite/Platform/Modules/Account/Services/Http.php(1 hunks)
💤 Files with no reviewable changes (1)
- app/controllers/api/account.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php
🧬 Code graph analysis (13)
src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (5)
src/Appwrite/Platform/Modules/Account/Services/Http.php (1)
Http(17-34)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (2)
getName(23-26)action(81-104)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (2)
getName(23-26)action(81-103)
src/Appwrite/Platform/Modules/Account/Module.php (2)
src/Appwrite/Platform/Appwrite.php (2)
Appwrite(16-30)__construct(18-29)src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Create.php (7)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/Auth.php (3)
Auth(18-515)codeGenerator(352-361)tokenGenerator(331-341)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Event/Mail.php (13)
setSmtpHost(171-175)setSmtpPort(183-187)setSmtpUsername(195-199)setSmtpPassword(207-211)setSmtpSecure(219-223)setSmtpReplyTo(255-259)setSmtpSenderEmail(231-235)setSmtpSenderName(243-247)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setRecipient(57-62)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Template/Template.php (4)
Template(8-178)fromFile(25-33)fromString(45-54)render(66-85)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (6)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (5)
Type(8-61)setLabel(17-22)setIssuer(29-34)getSecret(41-44)getProvisioningUri(46-49)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php (3)
getName(25-28)__construct(30-82)action(84-106)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (3)
getName(27-30)__construct(32-86)action(88-134)
src/Appwrite/Platform/Modules/Account/Services/Http.php (11)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (3)
Create(24-143)__construct(33-87)getName(28-31)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Create.php (3)
Create(34-329)__construct(43-105)getName(38-41)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (3)
Create(19-105)__construct(28-79)getName(23-26)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php (3)
Delete(21-107)__construct(30-82)getName(25-28)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (3)
Update(23-135)__construct(32-86)getName(27-30)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php (3)
Update(22-160)__construct(31-88)getName(26-29)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (3)
Update(19-104)__construct(28-79)getName(23-26)src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php (3)
Update(19-99)__construct(28-62)getName(23-26)src/Appwrite/Platform/Modules/Account/Http/Mfa/Factors/XList.php (3)
XList(17-89)__construct(26-71)getName(21-24)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (3)
Get(16-86)__construct(25-70)getName(20-23)src/Appwrite/Platform/Modules/Account/Module.php (1)
__construct(10-13)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php (6)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (3)
getName(28-31)__construct(33-87)action(89-142)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (3)
getName(27-30)__construct(32-86)action(88-134)
src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (6)
src/Appwrite/Auth/MFA/Type.php (2)
Type(8-61)generateBackupCodes(51-60)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (1)
action(81-104)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (1)
action(72-85)
src/Appwrite/Platform/Appwrite.php (1)
src/Appwrite/Platform/Modules/Account/Module.php (1)
Module(8-14)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Factors/XList.php (3)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Utopia/Response.php (1)
Response(158-980)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (10)
src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php (2)
Update(22-160)action(90-159)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (2)
Update(19-104)action(81-103)src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php (2)
Update(19-99)action(64-98)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (1)
action(89-142)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php (1)
action(84-106)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (1)
action(81-104)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (1)
action(72-85)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php (9)
src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (2)
Update(23-135)action(88-134)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (2)
Update(19-104)action(81-103)src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Create.php (1)
action(107-328)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (1)
action(81-104)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (1)
action(72-85)
src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Create.php (7)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (2)
Type(8-61)generateBackupCodes(51-60)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (3)
getName(20-23)__construct(25-70)action(72-85)src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Update.php (3)
getName(23-26)__construct(28-79)action(81-103)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php (3)
src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Update.php (2)
Update(23-135)action(88-134)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php
91-91: Avoid unused parameters such as '$requestTimestamp'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Update.php
93-93: Avoid unused parameters such as '$project'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php
66-66: Avoid unused parameters such as '$requestTimestamp'. (undefined)
(UnusedFormalParameter)
⏰ 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 (8)
src/Appwrite/Platform/Appwrite.php (1)
5-5: LGTM! Clean module registration.The Account module is properly imported and registered following the established pattern used by other modules in the platform.
Also applies to: 21-21
src/Appwrite/Platform/Modules/Account/Module.php (1)
8-13: LGTM! Module structure is correct.The Account module is properly configured with the HTTP service registration following the framework's module pattern.
src/Appwrite/Platform/Modules/Account/Http/Mfa/RecoveryCodes/Get.php (1)
72-85: LGTM! Recovery codes retrieval logic is correct.The action properly validates that recovery codes exist before returning them, with appropriate error handling and response modeling.
src/Appwrite/Platform/Modules/Account/Services/Http.php (1)
17-33: LGTM! MFA actions are properly registered.The HTTP service correctly registers all MFA-related actions with proper naming and instantiation. The fluent interface pattern is applied consistently.
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Delete.php (1)
84-106: LGTM! Authenticator deletion is correctly implemented.The action properly validates the authenticator exists, deletes it, purges the user cache, and queues events. The 204 No Content response is appropriate for a DELETE operation.
src/Appwrite/Platform/Modules/Account/Http/Mfa/Factors/XList.php (1)
73-88: LGTM! Factor availability logic is sound.The action correctly determines which MFA factors are available for the user by checking verification status for TOTP, email, phone, and the presence of recovery codes.
src/Appwrite/Platform/Modules/Account/Http/Mfa/Update.php (1)
73-98: LGTM! MFA update logic is correct.The action properly toggles MFA status, and when enabling MFA, it correctly derives available factors based on user verification status and updates the session accordingly.
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php (1)
98-142: LGTM! Authenticator creation logic is well-implemented.The action correctly handles creating a new TOTP authenticator with proper error handling for unknown types and existing verified authenticators. The cleanup of unverified authenticators before creating a new one is appropriate, and permissions are properly configured.
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php
Show resolved
Hide resolved
✨ Benchmark results
⚡ Benchmark Comparison
|
src/Appwrite/Platform/Modules/Account/Http/Mfa/Challenge/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Mfa/Authenticators/Create.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php (1)
109-135: Restore recovery-code branchLine 112 compares against
\strtolower(Type::RECOVERY_CODE)and Line 134 uses the same lowercase key in the match. Because the stored type stays'recoveryCode', this branch never runs and every recovery-code challenge fails. Please compare directly againstType::RECOVERY_CODE.- $challenge->getAttribute('type') === \strtolower(Type::RECOVERY_CODE) + $challenge->getAttribute('type') === Type::RECOVERY_CODE ... - \strtolower(Type::RECOVERY_CODE) => $recoveryCodeChallenge($challenge, $user, $otp), + Type::RECOVERY_CODE => $recoveryCodeChallenge($challenge, $user, $otp),src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php (1)
109-135: Restore recovery-code branchLine 112 still checks
\strtolower(Type::RECOVERY_CODE)and Line 134 uses the lowercased key in the match, so the recovery-code path never executes and valid codes are rejected. Please compare directly against the constant.- $challenge->getAttribute('type') === \strtolower(Type::RECOVERY_CODE) + $challenge->getAttribute('type') === Type::RECOVERY_CODE ... - \strtolower(Type::RECOVERY_CODE) => $recoveryCodeChallenge($challenge, $user, $otp), + Type::RECOVERY_CODE => $recoveryCodeChallenge($challenge, $user, $otp),src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Create.php (1)
171-181: Still missing the SMS trigger call
We configure the messaging queue for phone MFA but never fire it, so no SMS leaves the system. This is the same breakage noted earlier and remains a blocker. Please trigger the queue right after setting the provider type.$queueForMessaging ->setType(MESSAGE_SEND_TYPE_INTERNAL) ->setMessage(new Document([ '$id' => $challenge->getId(), 'data' => [ 'content' => $code, ], ])) ->setRecipients([$phone]) - ->setProviderType(MESSAGE_TYPE_SMS); + ->setProviderType(MESSAGE_TYPE_SMS) + ->trigger();
🧹 Nitpick comments (3)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Update.php (1)
55-66: Drop unusedrequestTimestampinjection
requestTimestampis injected and threaded through the signature but never read. Trimming it avoids dead parameters and keeps the action signature minimal.- ->inject('requestTimestamp') ->inject('response') ->inject('user') ->inject('session') ->inject('dbForProject') ->inject('queueForEvents') ->callback($this->action(...)); } public function action( bool $mfa, - ?\DateTime $requestTimestamp, Response $response,src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (2)
141-141: Consider using a more robust path construction.The
dirname(__DIR__, 7)approach is fragile and tightly couples this code to the current directory structure. If the file location changes, this path will break.Consider using a constant or configuration value for the templates path:
- $templatesPath = \dirname(__DIR__, 7) . '/app/config/locale/templates'; + $templatesPath = APP_CONFIG_PATH . '/locale/templates';Or define a method that retrieves the templates path from a centralized location.
269-282: Validate custom template content for security.Custom templates allow overriding the email body and sender details without validating the content. While this provides flexibility, it could potentially be exploited if custom templates are populated from untrusted sources or contain malicious content.
Consider adding validation or sanitization for custom template content, especially if templates can be user-provided or modified through an API.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Factors/XList.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Get.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Update.php(1 hunks)src/Appwrite/Platform/Modules/Account/Services/Http.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Modules/Account/Services/Http.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Get.php
🧬 Code graph analysis (12)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Get.php (3)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (1)
Response(158-980)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Factors/XList.php (3)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Utopia/Response.php (1)
Response(158-980)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Update.php (4)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Utopia/Response.php (1)
Response(158-980)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Create.php (6)
src/Appwrite/Auth/Auth.php (3)
Auth(18-515)codeGenerator(352-361)tokenGenerator(331-341)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Event/Mail.php (14)
setSmtpHost(171-175)setSmtpPort(183-187)setSmtpUsername(195-199)setSmtpPassword(207-211)setSmtpSecure(219-223)setSmtpReplyTo(255-259)setSmtpSenderEmail(231-235)setSmtpSenderName(243-247)setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setRecipient(57-62)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Template/Template.php (4)
Template(8-178)fromFile(25-33)fromString(45-54)render(66-85)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php (3)
src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (5)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (5)
Type(8-61)setLabel(17-22)setIssuer(29-34)getSecret(41-44)getProvisioningUri(46-49)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Delete.php (3)
getName(25-28)__construct(30-82)action(84-106)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Update.php (3)
getName(27-30)__construct(32-86)action(88-134)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (7)
src/Appwrite/Auth/Auth.php (3)
Auth(18-515)codeGenerator(352-361)tokenGenerator(331-341)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Event/Mail.php (14)
setSmtpHost(171-175)setSmtpPort(183-187)setSmtpUsername(195-199)setSmtpPassword(207-211)setSmtpSecure(219-223)setSmtpReplyTo(255-259)setSmtpSenderEmail(231-235)setSmtpSenderName(243-247)setSubject(34-39)setPreview(102-107)setBodyTemplate(148-153)setVariables(357-361)setRecipient(57-62)src/Appwrite/Platform/Workers/Messaging.php (1)
Messaging(49-816)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (1)
Response(158-980)src/Appwrite/Template/Template.php (4)
Template(8-178)fromFile(25-33)fromString(45-54)render(66-85)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php (6)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (2)
Type(8-61)generateBackupCodes(51-60)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Get.php (3)
getName(20-23)__construct(25-70)action(72-85)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Update.php (3)
getName(23-26)__construct(28-79)action(81-103)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php (4)
src/Appwrite/Auth/MFA/Type.php (1)
Type(8-61)src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (1)
Response(158-980)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Update.php (5)
src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Utopia/Response.php (2)
Response(158-980)dynamic(700-733)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php (3)
Update(22-161)__construct(31-89)action(91-160)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (2)
__construct(33-87)action(89-142)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Delete.php (2)
__construct(30-82)action(84-106)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Delete.php (3)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (2)
getName(28-31)__construct(33-87)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Update.php (2)
getName(27-30)__construct(32-86)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Update.php (5)
src/Appwrite/Platform/Modules/Account/Services/Http.php (2)
Http(17-34)__construct(19-33)src/Appwrite/Auth/MFA/Type.php (2)
Type(8-61)generateBackupCodes(51-60)src/Appwrite/Extend/Exception.php (1)
Exception(7-464)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php (3)
getName(23-26)__construct(28-79)action(81-104)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Get.php (3)
getName(20-23)__construct(25-70)action(72-85)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Update.php
66-66: Avoid unused parameters such as '$requestTimestamp'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php
94-94: Avoid unused parameters such as '$project'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php
91-91: Avoid unused parameters such as '$requestTimestamp'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php
94-94: Avoid unused parameters such as '$project'. (undefined)
(UnusedFormalParameter)
⏰ 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). (20)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (3)
1-42: LGTM: Class structure is well-organized.The namespace, imports, and class definition follow standard patterns for the new modular action architecture.
108-140: LGTM: Challenge document creation is correct.The challenge document structure, permissions, and persistence logic are properly implemented. Token and code generation use the appropriate Auth utility methods.
324-329: LGTM: Event queuing and response handling are correct.The events are properly queued with the necessary parameters, and the response uses the appropriate model type.
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Update.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenge/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php
Show resolved
Hide resolved
…ges/Create.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…icators/Create.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (3)
92-92: Align acceptedfactorvalues with implemented handling and add a safe default.
factorvalidation allowsType::EMAIL,Type::PHONE 6D38,Type::TOTP, andType::RECOVERY_CODE, but the switch only handlesPHONE, and there is nodefaultbranch. ForTOTP,RECOVERY_CODE, or any unexpected value that slips past validation, you’ll create a challenge document but perform no associated action, which is hard to reason about and risks unusable/orphaned challenges.Consider:
- Either restricting the whitelist to only the factors this action actually handles, or
- Implementing explicit branches for
Type::TOTPandType::RECOVERY_CODEif they are meant to be supported here, and- Adding a
defaultcase that throws a clearExceptionfor unsupported factors to avoid silent no‑ops.Also applies to: 143-322
155-181: Fix SMS template handling and send the rendered message, not the raw code.Two issues in the SMS path:
- When a project custom SMS template is configured,
$messageis overwritten with$customTemplate['message'](a string), and then$message->setParam(...)is called. That will fail at runtime because$messageis no longer aTemplateinstance.- You fully render
$message(including localized content) into$messagebut then queue only the raw$codeas'content', effectively discarding the formatted SMS text. The earlier template rendering is dead code and users see just the bare OTP.Suggested fix sketch:
- $message = Template::fromFile($templatesPath . '/sms-base.tpl'); - - $customTemplate = $project->getAttribute('templates', [])['sms.mfaChallenge-' . $locale->default] ?? []; - if (!empty($customTemplate)) { - $message = $customTemplate['message'] ?? $message; - } + $message = Template::fromFile($templatesPath . '/sms-base.tpl'); + + $customTemplate = $project->getAttribute('templates', [])['sms.mfaChallenge-' . $locale->default] ?? []; + if (!empty($customTemplate) && !empty($customTemplate['message'])) { + $message = Template::fromString($customTemplate['message']); + } @@ - $messageContent = \strip_tags($messageContent->render()); - $message = $message->setParam('{{token}}', $messageContent); - - $message = $message->render(); + $messageContent = \strip_tags($messageContent->render()); + $message = $message + ->setParam('{{token}}', $messageContent) + ->render(); @@ - ->setMessage(new Document([ - '$id' => $challenge->getId(), - 'data' => [ - 'content' => $code, - ], - ])) + ->setMessage(new Document([ + '$id' => $challenge->getId(), + 'data' => [ + 'content' => $message, + ], + ]))This keeps the type of
$messageconsistent and actually delivers the localized, templated SMS.
183-202: Revisit rate limiting behavior for SMS whenauthPhoneis absent from the plan.The timelimit/abuse logic only runs when
$plan['authPhone']is set. If a plan doesn’t defineauthPhone, SMS challenges are still sent but bypass rate limiting and abuse checks, which can be problematic from both cost and abuse perspectives.Consider either:
- Enforcing the timelimit/
Abusecheck regardless of whetherauthPhoneis present, using a sensible default, or- Explicitly disallowing / gating SMS MFA for plans without
authPhone, instead of silently sending without limits.At minimum, this behavior should be intentional and documented, since it affects abuse protection for phone-based MFA.
🧹 Nitpick comments (3)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (2)
33-86: Endpoint wiring and SDK metadata look good; double‑check audits labels targetingresponse.$idThe HTTP method/path, scope, event label, and SDK
Methoddefinitions for bothcreateMfaAuthenticator(deprecated) andcreateMFAAuthenticatorare consistent and match the new module‑action style.One thing to verify: both
audits.resource(user/{response.$id}) andaudits.userId({response.$id}) reference the response payload, but this action ultimately returnsResponse::MODEL_MFA_TYPEbuilt from a$modelthat only containssecretanduri. IfMODEL_MFA_TYPEdoes not define an$idfield, these placeholders may resolve tonulland produce incomplete audit records.It might be safer to align these labels with whatever pattern other Account/MFA actions use (for example, basing them on the authenticated user or the authenticator document), e.g. something along the lines of
user/{user.$id}or a dedicateduserIdparam, depending on existing conventions.
88-140: MFA authenticator creation logic is sound; remove unused$authenticatorreassignmentThe main flow—validating
type, configuring TOTP, replacing an existing unverified authenticator, persisting the new one, purging the user cache, and queueing an event—looks consistent and preserves the “one TOTP authenticator per user” invariant.The only cleanup worth doing here is the final reassignment of
$authenticator:$authenticator = $dbForProject->createDocument('authenticators', $authenticator);
$authenticatoris not read after this line, so the reassignment is effectively unused. Dropping the assignment keeps things clearer and avoids potential static‑analysis warnings:- $authenticator = $dbForProject->createDocument('authenticators', $authenticator); + $dbForProject->createDocument('authenticators', $authenticator);Everything else in this method looks good to me.
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
324-327: Confirm whether an explicittrigger()/project binding is required for events.You populate
$queueForEventswithuserIdandchallengeIdbut never call something like->setProject($project)->trigger(). In other actions, events are often explicitly triggered; if that convention still applies in the new HTTP module layer, this code will never emit the configuredusers.[userId].challenges.[challengeId].createevent.Please confirm the expected pattern for
Eventin this module. If explicit triggering is still required, add the appropriatesetProject(...)andtrigger()calls.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php(1 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (1)
src/Appwrite/Auth/MFA/Type.php (5)
Type(8-61)setLabel(17-22)setIssuer(29-34)getSecret(41-44)getProvisioningUri(46-49)
⏰ 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). (19)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
108-140: Overall flow and validations look solid, aside from the specific issues noted.The general structure—challenge document creation, per-factor validation (env + user attributes), localized templates, SMTP configuration handling, and usage metrics—is consistent and readable. Once the SMS/template and factor-handling issues are resolved, this action should integrate cleanly with the new modular account HTTP layer.
Also applies to: 204-321
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