feat: add user verification enforcement for Console project #10558
Conversation
📝 WalkthroughWalkthroughAdds a verification gate in app/controllers/shared/api.php that activates when no API key is provided, a global verification flag is on, the project is the console, and a user exists. Requests to non-allowlisted endpoints require the user to have either email or phone verified; otherwise, a USER_VERIFICATION_REQUIRED error is thrown. Introduces an end-to-end test suite validating behavior across console vs. regular projects, allowlisted endpoints, env flag control, API key bypass, and unauthenticated cases. Adds unit tests covering global and project verification flags, endpoint matching, and user verification states (email or phone). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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! |
✨ Benchmark results
⚡ Benchmark Comparison
|
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 (3)
app/controllers/shared/api.php(1 hunks)tests/e2e/Services/Projects/ProjectsVerificationE2ETest.php(1 hunks)tests/unit/Network/Validators/VerificationTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/Services/Projects/ProjectsVerificationE2ETest.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (1)
Scope(12-256)
app/controllers/shared/api.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)
🪛 GitHub Actions: Linter
tests/e2e/Services/Projects/ProjectsVerificationE2ETest.php
[error] 1-1: PSR12: no_unused_imports
app/controllers/shared/api.php
[error] 1-1: PSR12: no_whitespace_in_blank_line
tests/unit/Network/Validators/VerificationTest.php
[error] 1-1: PSR12: no_whitespace_in_blank_line
| if (!$emailVerified && !$phoneVerified) { | ||
| throw new Exception(Exception::USER_VERIFICATION_REQUIRED); | ||
| } |
There was a problem hiding this comment.
Define USER_VERIFICATION_REQUIRED before throwing it
Line [361] throws Exception::USER_VERIFICATION_REQUIRED, but Appwrite\Extend\Exception does not currently declare that constant (see src/Appwrite/Extend/Exception.php). At runtime this will trigger a fatal error (Undefined class constant 'USER_VERIFICATION_REQUIRED') the first time an unverified console request hits the middleware. Please add the constant (and corresponding error metadata) or reuse an existing error type before merging.
Based on relevant code snippet
🤖 Prompt for AI Agents
In app/controllers/shared/api.php around lines 360 to 362, the code throws
Exception::USER_VERIFICATION_REQUIRED but that class constant is not defined,
which will cause a fatal error at runtime; fix by either adding
USER_VERIFICATION_REQUIRED to src/Appwrite/Extend/Exception.php with appropriate
error code/message/metadata consistent with other constants, or change the throw
to reuse an existing defined exception constant (e.g., a suitable
ACCESS/VALIDATION error) and ensure the error metadata aligns with how callers
expect verification errors to be represented.
There was a problem hiding this comment.
@HarshMN2345 please see this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| public function testVerificationMiddlewareWithConsoleProject() | ||
| { | ||
| // Test with console project (should trigger verification check when enabled) | ||
| $this->client->setProject('console'); | ||
|
|
||
| // Test accessing a protected endpoint without verification | ||
| // Note: This test assumes verification is enabled via environment variable | ||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); | ||
|
|
||
| // Should either work (if verification disabled) or require verification | ||
| $this->assertContains($response['headers']['status-code'], [200, 401, 403]); | ||
| } | ||
|
|
||
| public function testVerificationMiddlewareWithRegularProject() | ||
| { | ||
| // Test with regular project (should not trigger verification check) | ||
| $this->client->setProject($this->getProject()['$id']); | ||
|
|
||
| // Test accessing a protected endpoint | ||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); | ||
|
|
||
| // Should work normally for non-console projects | ||
| $this->assertContains($response['headers']['status-code'], [200, 401]); | ||
| } | ||
|
|
||
| public function testAllowedEndpointsWorkWithoutVerification() | ||
| { | ||
| // Test that allowed endpoints work without verification | ||
| $this->client->setProject('console'); | ||
|
|
||
| $allowedEndpoints = [ | ||
| '/account', | ||
| '/console/variables', | ||
| '/health/version' | ||
| ]; | ||
|
|
||
| foreach ($allowedEndpoints as $endpoint) { | ||
| $response = $this->client->call('GET', $endpoint, [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); | ||
|
|
||
| // Should work regardless of verification status | ||
| $this->assertNotEquals(403, $response['headers']['status-code'], "Endpoint $endpoint should be allowed without verification"); | ||
| } | ||
| } | ||
|
|
||
| public function testEnvironmentVariableControl() | ||
| { | ||
| // Test that environment variable controls the verification system | ||
| // This test verifies the system respects the _APP_VERIFICATION_REQUIRED setting | ||
|
|
||
| $this->client->setProject('console'); | ||
|
|
||
| // Test with verification potentially enabled | ||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); | ||
|
|
||
| // Should work or require verification based on environment setting | ||
| $this->assertContains($response['headers']['status-code'], [200, 401, 403]); | ||
| } | ||
|
|
||
| public function testVerificationMiddlewareWithApiKey() | ||
| { | ||
| // Test that API key authentication bypasses verification | ||
| $this->client->setProject('console'); | ||
|
|
||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-key' => $this->getProject()['apiKey'] | ||
| ]); | ||
|
|
||
| // Should work with API key regardless of verification | ||
| $this->assertNotEquals(403, $response['headers']['status-code'], "API key should bypass verification"); | ||
| } | ||
|
|
||
| public function testVerificationMiddlewareWithEmptyUser() | ||
| { | ||
| // Test that empty/unauthenticated users are handled properly | ||
| $this->client->setProject('console'); | ||
|
|
||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); | ||
|
|
||
| // Should either work or require authentication, but not verification specifically | ||
| $this->assertContains($response['headers']['status-code'], [200, 401, 403]); | ||
| } |
There was a problem hiding this comment.
E2E assertions aren’t enforcing the new behavior
Across Lines [28], [42], [79], [93], and [107] the assertions accept almost any outcome (e.g. [200, 401, 403]), so these tests will pass even if the verification gate never activates. That means a regression—like the middleware failing to block unverified console users—would go unnoticed. Please structure the scenarios so that you control _APP_VERIFICATION_REQUIRED and the user verification state, then assert the specific status codes the middleware must return (e.g. expect 403 for an unverified console user when the flag is enabled, expect 200 for allowlisted endpoints, etc.).
🤖 Prompt for AI Agents
In tests/e2e/Services/Projects/ProjectsVerificationE2ETest.php lines 16 to 107,
the assertions accept broad status arrays which mask regressions; modify each
test to explicitly set the _APP_VERIFICATION_REQUIRED environment flag and the
user's verification/auth state (mock or create a verified/unverified user or use
an API key) so the test scenario is deterministic, then replace the loose
assertContains checks with exact expectations: assertEquals(403 for an
unverified console user when verification is enabled; assertEquals(200 for
verified console user or when verification is disabled; assertEquals(200 for
allowlisted endpoints regardless of verification; assertNotEquals(403 when using
a valid API key)). Ensure setup/teardown toggles the env flag and restores it
for other tests.
| public function testVerificationMiddlewareWithGlobalDisabled() | ||
| { | ||
| // Mock environment variable | ||
| $_ENV['_APP_VERIFICATION_REQUIRED'] = 'disabled'; | ||
|
|
||
| $verificationRequired = System::getEnv('_APP_VERIFICATION_REQUIRED', 'disabled') === 'enabled'; | ||
|
|
||
| $this->assertFalse($verificationRequired); | ||
| } | ||
|
|
||
| public function testVerificationMiddlewareWithGlobalEnabled() | ||
| { | ||
| // Test the logic directly | ||
| $globalVerificationEnabled = 'enabled' === 'enabled'; | ||
|
|
||
| $this->assertTrue($globalVerificationEnabled); | ||
| } | ||
|
|
||
| public function testProjectVerificationRequired() | ||
| { | ||
| $project = new Document([ | ||
| '$id' => 'console', | ||
| 'verificationRequired' => true | ||
| ]); | ||
|
|
||
| $projectVerificationRequired = $project->getAttribute('verificationRequired', false); | ||
|
|
||
| $this->assertTrue($projectVerificationRequired); | ||
| } | ||
|
|
||
| public function testProjectVerificationNotRequired() | ||
| { | ||
| $project = new Document([ | ||
| '$id' => 'regular-project', | ||
| 'verificationRequired' => false | ||
| ]); | ||
|
|
||
| $projectVerificationRequired = $project->getAttribute('verificationRequired', false); | ||
|
|
||
| $this->assertFalse($projectVerificationRequired); | ||
| } | ||
|
|
||
| public function testCombinedVerificationLogic() | ||
| { | ||
| // Test both global and project settings must be true | ||
| $globalVerificationEnabled = true; | ||
| $projectVerificationRequired = true; | ||
|
|
||
| $verificationActive = $globalVerificationEnabled && $projectVerificationRequired; | ||
|
|
||
| $this->assertTrue($verificationActive); | ||
| } | ||
|
|
||
| public function testCombinedVerificationLogicGlobalDisabled() | ||
| { | ||
| // Test global disabled overrides project setting 8000 | ||
| $globalVerificationEnabled = false; | ||
| $projectVerificationRequired = true; | ||
|
|
||
| $verificationActive = $globalVerificationEnabled && $projectVerificationRequired; | ||
|
|
||
| $this->assertFalse($verificationActive); | ||
| } | ||
|
|
||
| public function testCombinedVerificationLogicProjectDisabled() | ||
| { | ||
| // Test project disabled overrides global setting | ||
| $globalVerificationEnabled = true; | ||
| $projectVerificationRequired = false; | ||
|
|
||
| $verificationActive = $globalVerificationEnabled && $projectVerificationRequired; | ||
|
|
||
| $this->assertFalse($verificationActive); | ||
| } | ||
|
|
||
| public function testAllowedEndpoints() | ||
| { | ||
| $allowedEndpoints = [ | ||
| '/v1/account', | ||
| '/v1/console/variables', | ||
| '/v1/health/version', | ||
| '/v1/account/verification', | ||
| '/v1/account/verification/phone', | ||
| '/v1/account/recovery', | ||
| '/v1/account/sessions', | ||
| '/v1/account/tokens', | ||
| '/v1/account/mfa' | ||
| ]; | ||
|
|
||
| $currentPath = '/v1/account/sessions'; | ||
|
|
||
| $isAllowedEndpoint = false; | ||
| foreach ($allowedEndpoints as $allowedEndpoint) { | ||
| if (str_starts_with($currentPath, $allowedEndpoint)) { | ||
| $isAllowedEndpoint = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| $this->assertTrue($isAllowedEndpoint); | ||
| } | ||
|
|
||
| public function testProtectedEndpoints() | ||
| { | ||
| $allowedEndpoints = [ | ||
| '/v1/account', | ||
| '/v1/console/variables', | ||
| '/v1/health/version', | ||
| '/v1/account/verification', | ||
| '/v1/account/verification/phone', | ||
| '/v1/account/recovery', | ||
| '/v1/account/sessions', | ||
| '/v1/account/tokens', | ||
| '/v1/account/mfa' | ||
| ]; | ||
|
|
||
| $currentPath = '/v1/users'; | ||
|
|
||
| $isAllowedEndpoint = false; | ||
| foreach ($allowedEndpoints as $allowedEndpoint) { | ||
| if (str_starts_with($currentPath, $allowedEndpoint)) { | ||
| $isAllowedEndpoint = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| $this->assertFalse($isAllowedEndpoint); | ||
| } | ||
|
|
||
| public function testVerifiedUserAccess() | ||
| { | ||
| $emailVerified = $this->verifiedUser->getAttribute('emailVerification', false); | ||
| $phoneVerified = $this->verifiedUser->getAttribute('phoneVerification', false); | ||
|
|
||
| $this->assertTrue($emailVerified || $phoneVerified); | ||
| } | ||
|
|
||
| public function testUnverifiedUserAccess() | ||
| { | ||
| $emailVerified = $this->unverifiedUser->getAttribute('emailVerification', false); | ||
| $phoneVerified = $this->unverifiedUser->getAttribute('phoneVerification', false); | ||
|
|
||
| $this->assertFalse($emailVerified || $phoneVerified); | ||
| } | ||
|
|
||
| public function testEmptyUserAccess() | ||
| { | ||
| $this->assertTrue($this->emptyUser->isEmpty()); | ||
| } | ||
|
|
||
| public function testPhoneVerifiedUser() | ||
| { | ||
| $phoneVerifiedUser = new Document([ | ||
| '$id' => 'user3', | ||
| 'emailVerification' => false, | ||
| 'phoneVerification' => true, | ||
| ]); | ||
|
|
||
| $emailVerified = $phoneVerifiedUser->getAttribute('emailVerification', false); | ||
| $phoneVerified = $phoneVerifiedUser->getAttribute('phoneVerification', false); | ||
|
|
||
| $this->assertTrue($emailVerified || $phoneVerified); | ||
| } |
There was a problem hiding this comment.
Unit tests only assert hard-coded booleans
Lines [52], [84]-[111], and similar sections merely compare literal booleans (e.g. 'enabled' === 'enabled', $verificationActive = true && true). These tests never execute the actual verification middleware or any shared helper, so they cannot fail even if the production logic is broken. Please refactor the tests to exercise the real validator/middleware entry point (or a dedicated helper) with representative Document fixtures and environment settings; otherwise they give a false sense of coverage.
| if (!$emailVerified && !$phoneVerified) { | ||
| throw new Exception(Exception::USER_VERIFICATION_REQUIRED); | ||
| } |
There was a problem hiding this comment.
@HarshMN2345 please see this.
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json' | ||
| ]); |
There was a problem hiding this comment.
This isn't a API call that is typically done on the console project 🧐
| $this->client->setProject('console'); | ||
|
|
||
| $response = $this->client->call('GET', '/users', [], [ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-key' => $this->getProject()['apiKey'] | ||
| ]); |
There was a problem hiding this comment.
this makes the /users call against the console project? I would expect this to always return 401 🧐
| $allowedEndpoints = [ | ||
| '/v1/account', // User account operations | ||
| '/v1/console/variables', // Console configuration | ||
| '/v1/health/version', // Health checks | ||
| '/v1/account/verification', // Email verification | ||
| '/v1/account/verification/phone', // Phone verification | ||
| '/v1/account/recovery', // Account recovery | ||
| '/v1/account/sessions', // Session management | ||
| '/v1/account/tokens', // Token management | ||
| '/v1/account/mfa' // Multi-factor authentication | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Instead of having list of endpoints here, we can do one of 2 things:
A) Have a special group() for endpoints that are supposed to be blocked (might be bad idea if its too many)
B) Have a special label() for marking endpoint as allowed without verification
Also, this would enforce email verification on self-hosted too. It might feel like extra blocker, requiring to setup SMTP, causing people to leave the onboarding.
For that reason, I think this user verification exception for Console should be cloud-specific, in Cloud repository.
There was a problem hiding this comment.
I think anything under /account should be accessible even if not verified
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