fix: allow linking identities with verified emails#10986
fix: allow linking identities with verified emails#10986TorstenDittmann merged 6 commits into1.8.xfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces email verification checks to the OAuth2 authentication flow. It adds a new mock OAuth provider configuration entry ('mock-unverified'), creates a new MockUnverified OAuth2 class that returns unverified email status, updates the account controller to validate email verification during OAuth login and prevent linking accounts with unverified emails, adds a mock endpoint for unverified OAuth users, modifies the existing Mock provider to derive verification status from user data, and includes test cases to verify the new behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
- Add MockUnverified OAuth2 provider config - Add /v1/mock/tests/general/oauth2/user-unverified endpoint - Add MockUnverified class for unverified OAuth2 flow - Update Mock::isEmailVerified to respect user['verified'] flag - Add end-to-end tests for linking unverified and verified OAuth2 users - Enable stopOnFailure in phpunit.xml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Auth/OAuth2/MockUnverified.php (1)
5-29: Consider parameterizing the endpoint URL to reduce duplication.The
MockUnverifiedclass duplicates the structure of its parentMockclass, differing only in the provider name and the endpoint URL. While this works correctly, you could reduce duplication by parameterizing the endpoint URL (e.g., via a protected property that subclasses can override, or a constructor parameter).That said, for mock/test code, this simple inheritance pattern is perfectly acceptable and keeps the code straightforward.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/config/oAuthProviders.php(1 hunks)app/controllers/api/account.php(1 hunks)app/controllers/mock.php(1 hunks)phpunit.xml(1 hunks)src/Appwrite/Auth/OAuth2/Mock.php(1 hunks)src/Appwrite/Auth/OAuth2/MockUnverified.php(1 hunks)tests/e2e/Services/Account/AccountCustomClientTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Appwrite/Auth/OAuth2/Mock.php (1)
src/Appwrite/Auth/OAuth2/MockUnverified.php (1)
getUser(20-29)
src/Appwrite/Auth/OAuth2/MockUnverified.php (1)
src/Appwrite/Auth/OAuth2/Mock.php (3)
Mock(7-165)getName(34-37)getUser(155-164)
app/controllers/api/account.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-472)
tests/e2e/Services/Account/AccountCustomClientTest.php (1)
tests/e2e/Scopes/Scope.php (1)
getRoot(160-202)
⏰ 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). (8)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: Benchmark
🔇 Additional comments (9)
phpunit.xml (1)
9-9: Verify the stopOnFailure configuration change is intentional.Changing
stopOnFailurefromfalsetotruewill halt test execution on the first failure, which may hide other failing tests. This is useful for debugging specific failures but is typically not desired in CI/CD pipelines where you want to see all test failures.Was this change intentional for debugging the new OAuth verification tests, or should it be reverted before merging?
src/Appwrite/Auth/OAuth2/Mock.php (1)
131-136: LGTM! Email verification now derives from user data.The change from a hardcoded
truereturn value to deriving the verification status from user data is well-implemented. The fallback totruewhen theverifiedfield is missing ensures backward compatibility with existing mock data.app/controllers/mock.php (2)
116-122: LGTM! Explicit verification status added to mock user.Adding the
verifiedfield to the existing mock OAuth user response is a clean change that supports the new data-driven verification logic inMock.php.
124-143: LGTM! New unverified user endpoint supports test scenarios.The new endpoint correctly mirrors the verified user endpoint structure but returns
verified => false. The minor code duplication in token validation is acceptable for mock/test code and keeps each endpoint self-contained and readable.app/config/oAuthProviders.php (1)
465-475: LGTM! Mock unverified provider configuration is correct.The new
mock-unverifiedprovider configuration properly mirrors the existingmockprovider and correctly references theMockUnverifiedclass. All settings are appropriate for a mock test provider.tests/e2e/Services/Account/AccountCustomClientTest.php (3)
2186-2197: LGTM! Proper test cleanup added.Adding cleanup code to delete the test user after the OAuth conversion test is a good practice that prevents test data accumulation. The cleanup uses appropriate authorization and verifies successful deletion.
2199-2261: LGTM! Comprehensive test for unverified email linking prevention.This test properly validates that OAuth logins with unverified emails cannot link to existing accounts, which is the core security requirement of this PR. The test:
- Sets up a pre-existing user with the target email
- Attempts OAuth login with unverified provider
- Correctly expects failure (400 status, 'failure' result)
- Cleans up test data
2263-2338: LGTM! Thorough test for verified email linking.This test comprehensively validates that OAuth logins with verified emails successfully link to existing accounts. The test properly:
- Creates a pre-existing user
- Performs OAuth login with verified provider
- Asserts successful linking (200 status, 'success' result)
- Verifies the session is created
- Confirms the OAuth identity linked to the existing user (not created new user)
- Cleans up test data
The test coverage is excellent and validates the expected behavior.
app/controllers/api/account.php (1)
1653-1664: LGTM: Email verification enforcement for user-email linking.The security check correctly prevents linking an OAuth2 account with an unverified email to an existing user account. Using
GENERAL_BAD_REQUESTappropriately avoids account enumeration.
There was a problem hiding this comment.
Pull request overview
This pull request enhances OAuth2 authentication by allowing verified OAuth email addresses to link to existing accounts, while preventing unverified email addresses from doing so. This addresses a security concern where unverified OAuth emails could potentially hijack existing accounts.
Key Changes:
- Modified OAuth2 authentication flow to check email verification status before linking accounts
- Added verification check for both direct user email matches and identity email matches
- Introduced comprehensive test coverage for verified and unverified OAuth email linking scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/api/account.php |
Added email verification checks before linking OAuth identities to existing users or identities with matching emails; moved identity email check earlier in the flow |
src/Appwrite/Auth/OAuth2/Mock.php |
Updated isEmailVerified() method to return verification status from user data instead of hardcoded true value |
src/Appwrite/Auth/OAuth2/MockUnverified.php |
New mock OAuth provider for testing unverified email scenarios |
app/controllers/mock.php |
Added mock endpoint for unverified OAuth user data and added verified flag to existing mock endpoint |
app/config/oAuthProviders.php |
Registered the new mock-unverified OAuth provider |
tests/e2e/Services/Account/AccountCustomClientTest.php |
Added end-to-end tests for both verified and unverified OAuth email linking scenarios; added cleanup to existing test |
phpunit.xml |
Changed stopOnFailure configuration to true |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
No description provided.