You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Introduce account linking in checkOAuthUserProfile: when a local user exists with the same email and the OAuth provider vouches for the email, attach the OAuth providerData under user.additionalProvidersData[provider] instead of erroring on Mongo's unique-email constraint.
Keep user.provider untouched on link — this preserves password reset (gated on provider === 'local') and local login.
Require emailVerifiedByProvider: true from the OAuth strategy before linking. Prevents account takeover via a misconfigured OIDC provider that returns email_verified: false for someone else's address.
Wire email_verified through the Google (providerData.email_verified) and Apple (decodedIdToken.email_verified, accepts boolean or 'true' string) strategies.
additionalProvidersData[provider][key] — linked users on subsequent signins.
email match — if provider verified the email, attach providerData and return.
No match → create new user (with emailVerified reflecting provider verification).
Why not overwrite user.provider
auth.password.controller.js:36 rejects password reset when provider !== 'local'. Overwriting would silently break password reset for linked users. Keeping provider stable and storing the OAuth link in additionalProvidersData (a field that already exists in the Mongoose schema and was already surfaced in /token responses) sidesteps the regression without touching the password flow.
Test plan
npm run test:unit — 544/544 green (added UserRepository mock to 2 unit tests that mock the user service)
npm run test:integration — 264/264 green (4 new tests cover linking, takeover rejection, fresh-create emailVerified)
npm run lint — 0 warnings
Post-merge: propagate via /update-all-projects then retest Google signin on trawl with the pre-existing local account
Security notes
Google and Apple both set email_verified: true (ID token claim) for addresses they authoritatively control. Linking is safe because the OAuth flow proves current ownership of that email.
The emailVerifiedByProvider gate is future-proofing: if a custom OIDC provider is wired into the strategies stack later and lies about email_verified, linking is refused and we fall through to the existing unique: true constraint.
Local user's password hash stays intact on link — they can continue signing in with email + password OR Google.
- checkOAuthUserProfile lookup order: (provider, sub) → additional
ProvidersData[provider][key] → email + provider-verified → create
- Store linked providers under additionalProvidersData, keep
user.provider intact (password reset + local login still work)
- Require emailVerifiedByProvider=true before linking (prevents
takeover via unverified OIDC claims)
- Set emailVerified=true on fresh OAuth create when provider
vouches for the email (closes#3494)
- Pass email_verified through google + apple strategies
@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 47 seconds before requesting another review.
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 47 seconds.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Reviewing files that changed from the base of the PR and between 177cf7f and 60c16c0.
📒 Files selected for processing (8)
MIGRATIONS.md
modules/auth/controllers/auth.controller.js
modules/auth/strategies/local/apple.js
modules/auth/strategies/local/google.js
modules/auth/tests/auth.integration.tests.js
modules/users/config/users.development.config.js
modules/users/repositories/users.repository.js
modules/users/services/users.service.js
Walkthrough
The changes implement OAuth account linking functionality. When authenticating via OAuth (Google/Apple), the system now searches for existing users by email and links their OAuth identity if the email is verified, preventing account takeover while improving UX. New OAuth users are created with emailVerified set to true based on provider verification status. The schema is extended to support multiple provider linkages.
Added emailVerifiedByProvider field to OAuth profile payload, derived from provider's email verification status (decodedIdToken.email_verified for Apple, email_verified for Google).
Implemented multi-step OAuth user resolution in checkOAuthUserProfile: lookup by provider identity, then by additionalProvidersData, then by verified email. Links verified email matches to existing users; rejects unverified local account matches; creates new users with emailVerified from provider. Added UserRepository import for persistence.
User Schema Extension modules/users/models/users.schema.js
Added optional additionalProvidersData field to support storing multiple OAuth provider linkages alongside the primary provider and providerData.
Added four comprehensive tests covering verified email linking to existing user, unverified email account creation, rejection of unverified local account linking, and fresh OAuth user creation with emailVerified=true.
Unit Test Mocking modules/auth/tests/auth.silent.catch.unit.tests.js, lib/services/tests/analytics.identify.unit.tests.js
Added users.repository mocking in test harnesses to ensure users.repository.update() resolves correctly during auth flow execution, preventing unexpected repository access.
Sequence Diagram(s)
sequenceDiagram
participant Client
participant AuthCtrl as AuthController
participant UserRepo as UserRepository
participant UserSvc as UserService
participant DB as Database
Client->>AuthCtrl: checkOAuthUserProfile(profile)
Note over AuthCtrl: profile includes emailVerifiedByProvider
AuthCtrl->>UserRepo: findOne(provider, providerData[key])
UserRepo->>DB: query by (provider, key)
DB-->>UserRepo: user or null
UserRepo-->>AuthCtrl: existing user?
alt User found by provider
AuthCtrl-->>Client: return user
else User not found
AuthCtrl->>UserRepo: findOne by additionalProvidersData
UserRepo->>DB: query additionalProvidersData
DB-->>UserRepo: user or null
UserRepo-->>AuthCtrl: linked user?
alt User found via additionalProvidersData
AuthCtrl-->>Client: return user
else User not found
alt emailVerifiedByProvider && email exists
AuthCtrl->>UserRepo: findOne by email
UserRepo->>DB: query by email
DB-->>UserRepo: local user found
UserRepo-->>AuthCtrl: user or null
alt Local user found
alt local user.emailVerified
AuthCtrl->>UserRepo: update(attach OAuth data)
UserRepo->>DB: update additionalProvidersData
DB-->>UserRepo: updated user
UserRepo-->>AuthCtrl: linked user
AuthCtrl-->>Client: return linked user
else local user unverified
AuthCtrl-->>Client: reject (security: prevent takeover)
end
else No local user by email
AuthCtrl->>UserSvc: create(emailVerified=true)
UserSvc->>DB: insert new user
DB-->>UserSvc: created user
UserSvc-->>AuthCtrl: new user
AuthCtrl-->>Client: return new user
end
else No verified email
AuthCtrl->>UserSvc: create(emailVerified=false)
UserSvc->>DB: insert new user
DB-->>UserSvc: created user
UserSvc-->>AuthCtrl: new user
AuthCtrl-->>Client: return new user
end
end
end
The title 'feat(auth): link OAuth signin to existing user by verified email' directly and accurately summarizes the main change: OAuth account linking based on verified email, which is the core objective of the PR.
Description check
✅ Passed
The PR description comprehensively covers all template sections: Summary (what/why), Scope (impacted modules, risk), Validation (test results), Guardrails (no secrets, no risky moves, tests added), and detailed Security and Implementation notes.
Linked Issues check
✅ Passed
The PR fully implements both linked issues: #3493 (OAuth-to-local account linking with verified email checks) and #3494 (OAuth-created users inherit emailVerified: true from provider claims).
Out of Scope Changes check
✅ Passed
All code changes are directly scoped to the linked objectives: OAuth account linking logic, provider email verification wiring, schema updates, and related test coverage. No unrelated changes detected.
Docstring Coverage
✅ Passed
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch feat/oauth-account-linking-3493
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.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR successfully implements OAuth account linking by verified email, ensuring users with existing local accounts can sign in via Google or Apple without duplication. All primary acceptance criteria and required test scenarios are addressed.
However, the implementation is currently not up to standards. Critical security issues were identified in modules/auth/controllers/auth.controller.js related to dynamic object property access and injection risks. Additionally, there is a layering violation where the controller interacts directly with UserRepository instead of UserService, which could skip essential validation and hooks. High complexity growth was also noted in the integration tests.
About this PR
The new authentication logic introduces object injection vulnerabilities by using unsanitized variables to access and modify user data. Furthermore, bypassing the Service layer for database updates breaks architectural consistency and may lead to issues with data validation or change detection in Mongoose.
Test suggestions
Link OAuth identity to existing local user with matching verified email
Prevent account linking and create a new user if provider email is not verified
Prevent account takeover when provider email is unverified and matches an existing local user
Automatically verify email for new OAuth signups when provider vouches for verification
Find and return a user who was previously linked via additionalProvidersData
❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.24%. Comparing base (35f28ff) to head (60c16c0). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/auth/controllers/auth.controller.js (1)
298-304: 🧹 Nitpick | 🔵 Trivial
Update JSDoc to document the new async return shape.
checkOAuthUserProfile was modified and is async but has no @returns. The linking branch can now return a mutated existing user, the create branch returns a freshly created one, and the function throws AppErrors for each distinct failure mode — worth documenting.
As per coding guidelines: "Every new or modified function must have a JSDoc header: one-line description, @param for each argument, @returns for any non-void return value (always include @returns for async functions to document the resolved value)".
📝 Suggested JSDoc
/**
- * `@desc` Endpoint to save oAuthProfile- * `@param` {Object} profil - OAuth user profile object- * `@param` {string} key - Provider key to lookup providerData- * `@param` {string} provider - OAuth provider name+ * `@desc` Resolve or create a user for an OAuth sign-in. Resolution order:+ * (1) match on (provider, providerData[key]); (2) match on additionalProvidersData[provider][key];+ * (3) if provider vouches for email, link providerData onto the existing local user;+ * (4) otherwise create a fresh OAuth user.+ * `@param` {Object} profil - OAuth profile (firstName, lastName, email, avatar, providerData, emailVerifiedByProvider)+ * `@param` {string} key - Provider key used to index providerData (e.g. 'sub', 'id')+ * `@param` {string} provider - OAuth provider name (e.g. 'google', 'apple')+ * `@returns` {Promise<Object>} Resolved, linked, or newly created user document
*/
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/auth/controllers/auth.controller.js` around lines 298 - 304, The
JSDoc for async function checkOAuthUserProfile is missing a `@returns` and must be
updated: add a one-line description (if changed), keep `@param` entries for
profil, key, provider, and add an `@returns` describing the Promise resolution
shape (either a mutated existing User object when linking or a newly created
User object when creating), and add an `@throws` (or `@throws` {AppError}) line
documenting the distinct AppError failure modes the function can throw;
reference the function name checkOAuthUserProfile and ensure the JSDoc reflects
the async return type and error conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/auth/controllers/auth.controller.js`:
- Around line 327-340: The current search → mutate → update sequence
(UserService.search followed by UserRepository.update) creates a TOCTOU race
where concurrent OAuth callbacks can overwrite each other's
additionalProvidersData; replace it with a single atomic conditional update: add
or use an atomic repository operation (e.g., UserRepository.findOneAndUpdate or
a new UserRepository.linkProviderAtomic) that matches the user by email and a
linkable flag (e.g., { email: profil.email, emailVerified: true }) and sets
additionalProvidersData.[provider] = profil.providerData and emailVerified =
true in one step, returning the updated user or null if no match so you avoid
the race and only link when the user is still linkable.
- Line 9: Controller is directly importing and calling UserRepository (import
UserRepository and the repository call currently in the linking branch),
violating layering; remove the UserRepository import and repository call and
instead route the operation through the service layer by calling a UserService
method (e.g., add/introduce a UserService.linkOAuthProvider(user, provider,
providerData) or reuse UserService.update(brutUser, patch, 'recover') used
elsewhere). Ensure the controller imports UserService (not the repository), that
the service performs Zod validation with UsersSchema.User before writing, and
update the controller to pass the user, provider identifier and provider payload
into that service method so the write no longer bypasses the schema validation.
In `@modules/auth/tests/auth.integration.tests.js`:
- Around line 712-739: The test title and behavior diverge: update the test to
actually verify "should NOT link when OAuth provider did not verify the email"
by using the same email as the created localUser (set profil.email =
sharedEmail) and keep profil.emailVerifiedByProvider = false, call
AuthController.checkOAuthUserProfile(profil, 'sub', 'google'), then assert that
the returned user is the newly created OAuth user (or that
localUser.additionalProvidersData is still undefined/unchanged) to prove no
linking occurred; ensure the test title and assertions reflect this scenario and
retain cleanup via UserService.remove for both localUser and the created user.
- Around line 676-710: The test seeds a local user with password undefined
because it uses credentials.password (credentials is an array), so replace uses
of credentials.password with a concrete password value (e.g., pick
credentials[0].password or a local literal like 'Test#1234') when creating the
local user via UserService.create in the OAuth-linking tests and any other tests
flagged (the ones invoking UserService.create before
AuthController.checkOAuthUserProfile); ensure the seeded user's password is
non-empty so the assertion about retaining a password-backed provider
(linked.provider === 'local') is valid, and keep cleanup via UserService.remove
unchanged.
- Around line 741-766: Update the test for AuthController.checkOAuthUserProfile
to assert the specific service error and verify the local account wasn't
modified instead of relying on Mongo unique index: call
AuthController.checkOAuthUserProfile(profil, 'sub', 'google') and assert the
rejection matches the expected error object (e.g., code: 'SERVICE_ERROR'), then
use UserService.search({ email: sharedEmail }) to assert exactly one user exists
and that its additionalProvidersData is undefined; keep the existing cleanup via
UserService.remove(localUser).
In `@modules/users/models/users.schema.js`:
- Line 27: The nested additionalProvidersData field currently exposes OAuth
tokens in responses; update the auth token flow to strip those secrets before
serialization by either (A) adding scrubbing for nested keys like
additionalProvidersData.*.{accessToken,refreshToken} to the output whitelist
used during serialization (matching how providerData is excluded), or (B) call
the existing removeSensitive sanitization on the auth controller’s token
response (the same approach used by users.update) after checkOAuthUserProfile
populates additionalProvidersData so that accessToken/refreshToken are removed
before the token endpoint returns to clients.
---
Outside diff comments:
In `@modules/auth/controllers/auth.controller.js`:
- Around line 298-304: The JSDoc for async function checkOAuthUserProfile is
missing a `@returns` and must be updated: add a one-line description (if changed),
keep `@param` entries for profil, key, provider, and add an `@returns` describing
the Promise resolution shape (either a mutated existing User object when linking
or a newly created User object when creating), and add an `@throws` (or `@throws`
{AppError}) line documenting the distinct AppError failure modes the function
can throw; reference the function name checkOAuthUserProfile and ensure the
JSDoc reflects the async return type and error conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 354621a1-8604-4498-aceb-222be76e3a40
📥 Commits
Reviewing files that changed from the base of the PR and between 35f28ff and 177cf7f.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds OAuth account-linking behavior to the auth flow so that an OAuth sign-in can attach to an existing user when the provider vouches for the email, and ensures newly-created OAuth users inherit provider email verification.
Changes:
Add emailVerifiedByProvider wiring in Google/Apple strategies and propagate it into user creation (emailVerified).
Extend checkOAuthUserProfile lookup order to include additionalProvidersData and attempt email-based linking.
Update Zod user schema + adjust unit/integration tests to cover new linking/verification behaviors and new repository dependency.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
File
Description
modules/auth/controllers/auth.controller.js
Implements linked-provider lookup + email-based linking and sets emailVerified for new OAuth users.
modules/auth/strategies/local/google.js
Adds emailVerifiedByProvider from Google email_verified claim.
modules/auth/strategies/local/apple.js
Adds emailVerifiedByProvider from Apple email_verified claim (boolean or 'true').
modules/users/models/users.schema.js
Adds additionalProvidersData to the Zod user schema.
modules/auth/tests/auth.integration.tests.js
Adds integration tests for linking + verified-email creation behavior.
…test accuracy
- Remove direct UserRepository import from auth controller (was layer violation)
- Replace UserRepository.update with UserService.update(brutUser, patch, 'recover') in step 3
- Add ALLOWED_PROVIDERS/ALLOWED_PROVIDER_KEYS allowlists before dynamic key construction
- Add sanitizeAdditionalProvidersData helper to strip accessToken/refreshToken from token endpoint
- Add additionalProvidersData to recover whitelist so service update can persist it
- Fix credentials.password → credentials[0].password (was array, not object)
- Fix unverified-email test to use same email and assert local account untouched
- Strengthen takeover test: assert specific error code + verify local account not modified
- Remove dead UserRepository mocks from unit tests (controller no longer imports it)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
checkOAuthUserProfile: when a local user exists with the same email and the OAuth provider vouches for the email, attach the OAuthproviderDataunderuser.additionalProvidersData[provider]instead of erroring on Mongo's unique-email constraint.user.provideruntouched on link — this preserves password reset (gated onprovider === 'local') and local login.emailVerifiedByProvider: truefrom the OAuth strategy before linking. Prevents account takeover via a misconfigured OIDC provider that returnsemail_verified: falsefor someone else's address.emailVerified: truewhen the provider vouches for the email (closes fix(auth): OAuth-created users should have emailVerified=true #3494).email_verifiedthrough the Google (providerData.email_verified) and Apple (decodedIdToken.email_verified, accepts boolean or'true'string) strategies.Closes #3493
Closes #3494
Lookup order in
checkOAuthUserProfile(provider, providerData[key])— primary identity, OAuth-first users.additionalProvidersData[provider][key]— linked users on subsequent signins.emailmatch — if provider verified the email, attachproviderDataand return.emailVerifiedreflecting provider verification).Why not overwrite
user.providerauth.password.controller.js:36rejects password reset whenprovider !== 'local'. Overwriting would silently break password reset for linked users. Keepingproviderstable and storing the OAuth link inadditionalProvidersData(a field that already exists in the Mongoose schema and was already surfaced in/tokenresponses) sidesteps the regression without touching the password flow.Test plan
npm run test:unit— 544/544 green (added UserRepository mock to 2 unit tests that mock the user service)npm run test:integration— 264/264 green (4 new tests cover linking, takeover rejection, fresh-create emailVerified)npm run lint— 0 warnings/update-all-projectsthen retest Google signin on trawl with the pre-existing local accountSecurity notes
email_verified: true(ID token claim) for addresses they authoritatively control. Linking is safe because the OAuth flow proves current ownership of that email.emailVerifiedByProvidergate is future-proofing: if a custom OIDC provider is wired into the strategies stack later and lies aboutemail_verified, linking is refused and we fall through to the existingunique: trueconstraint.Related (separate PRs)
req.bodyundefined (merged, fix(auth): handle GET OAuth callback with undefined req.body on Express 5 #3496)Summary by CodeRabbit
New Features
Tests