feat(auth): add OAuth2 login with Google and GitHub#493
Conversation
Implement Passport OAuth strategies, extend the user model for provider accounts, add auth routes and login UI, and document setup in the README. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR adds complete OAuth2 authentication for Google and GitHub to the full-stack application. Changes include User model provider support, OAuth utility layer, Passport strategy configuration, backend auth routes with OAuth endpoints, server session security hardening, frontend API client, Login/Signup UI refactoring, environment configuration, and comprehensive documentation with tests. ChangesOAuth2 Sign-In Implementation
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend (Login)
participant Backend as Backend (OAuth Routes)
participant PassportLib as Passport + OAuth Strategies
participant OAuthProvider as Google/GitHub OAuth
participant Database as MongoDB User
User->>Frontend: Click "Continue with Google"
Frontend->>Backend: GET /auth/google
Backend->>PassportLib: passport.authenticate('google')
PassportLib->>OAuthProvider: Redirect to OAuth consent
OAuthProvider->>User: OAuth consent screen
User->>OAuthProvider: Grant permission
OAuthProvider->>Backend: Redirect with auth code
Backend->>PassportLib: Handle OAuth callback
PassportLib->>OAuthProvider: Exchange code for profile
PassportLib->>Backend: Call GoogleStrategy callback
Backend->>Backend: findOrCreateOAuthUser()
Backend->>Database: Query by {provider, providerId}
Database-->>Backend: Existing user or null
alt New OAuth User
Backend->>Backend: ensureUniqueUsername()
Backend->>Database: Create user (no password)
end
Database-->>Backend: User document
Backend->>Backend: toSessionUser() → session shape
Backend->>Backend: req.logIn(user, ...)
Backend-->>Frontend: Redirect to /login?oauth=success
Frontend->>Backend: Fetch current user (GET /me)
Backend-->>Frontend: {user: AuthUser}
Frontend->>User: Redirect to dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/models/User.js`:
- Line 34: The current unique index on UserSchema (UserSchema.index({ provider:
1, providerId: 1 }, { unique: true, sparse: true })) still conflicts for local
users because provider always exists and providerId can be missing; replace this
with a partial unique index so uniqueness only applies when providerId is
present by calling UserSchema.index(...) with a partialFilterExpression such as
{ providerId: { $exists: true } } (and keep unique: true) so only documents that
actually have a providerId are constrained.
In `@backend/utils/oauthUser.js`:
- Around line 32-45: OAuth email lookup lowercases the address
(`normalizedEmail`) but local accounts are stored/queried with the raw case,
causing missed matches; fix by canonicalizing emails consistently across both
auth flows: normalize (trim().toLowerCase()) before saving local user records
and use the same normalized value (or a dedicated canonical field) for all
User.findOne lookups (including the OAuth path using normalizedEmail) and for
User.create/update logic so lookups (User.findOne) and persistence share the
same normalized key.
- Around line 47-53: The code is overwriting user.provider for existing local
accounts (user.provider === "local") which breaks LocalStrategy; instead do not
change user.provider when it's "local": remove the assignment user.provider =
provider and avoid converting the account to an OAuth-only type; if you need to
record the OAuth identity, store provider/providerId in a separate linked
identity field (e.g., linkedProviders or identities) or only set user.providerId
(without changing user.provider), save the user, and return toSessionUser(user)
so email/password login remains intact.
In `@README.md`:
- Around line 102-103: Replace hardcoded callback URLs in the README with a
BACKEND_URL-based example so they match backend/.env.example; update the
occurrences that currently show "http://localhost:5001/api/auth/google/callback"
(and the related provider examples at 119-120) to use
"{BACKEND_URL}/api/auth/{provider}/callback" or explicitly show
"http://localhost:5000/api/auth/{provider}/callback" with
"http://localhost:5001" as an optional fallback so the documented callback
host/port aligns with BACKEND_URL/default port.
In `@src/pages/Login/Login.tsx`:
- Around line 50-52: The oauthMessage value coming from
searchParams.get("message") is already decoded, so remove the extra
decodeURIComponent call in the oauth error branch; change the logic in the block
that checks oauthStatus === "error" (where setMessage(...) is called) to use
oauthMessage directly (or fall back to "OAuth sign-in failed") and then call
setSearchParams({}, { replace: true }) as before—update references to
oauthStatus, oauthMessage, setMessage, and setSearchParams accordingly to avoid
double-decoding and potential URIError.
In `@src/utils/authApi.ts`:
- Around line 27-33: The fetchCurrentUser function currently returns null for
any error; change the catch to inspect the thrown error (from authApi.get) and
only return null when the response indicates an unauthorized status (e.g.,
status === 401 or 403), otherwise rethrow the error so network/backend failures
bubble up; locate the fetchCurrentUser function and modify its catch to check
the error's response.status (or use AxiosError type guards) and handle 401/403
with return null, rethrow for all other cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7461be07-7ff8-4080-8e06-59d3da916e2c
📒 Files selected for processing (15)
.env.exampleREADME.mdbackend/.env.examplebackend/config/passportConfig.jsbackend/models/User.jsbackend/package.jsonbackend/routes/auth.jsbackend/server.jsbackend/utils/oauthUser.jsspec/auth.routes.spec.cjsspec/oauthUser.spec.cjsspec/user.model.spec.cjssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsxsrc/utils/authApi.ts
| // ✅ FIXED: no next() | ||
| UserSchema.pre('save', async function () { | ||
| if (!this.isModified('password')) return; | ||
| UserSchema.index({ provider: 1, providerId: 1 }, { unique: true, sparse: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant portion of the schema
sed -n '1,120p' backend/models/User.js | cat -n
# Print index-related lines specifically
rg -n "UserSchema\.index|index\(" backend/models/User.js
# Check whether provider/providerId are set in defaults or validation logic
rg -n "providerId|provider" backend/models/User.jsRepository: GitMetricsLab/github_tracker
Length of output: 1791
🌐 Web query:
MongoDB sparse unique compound index semantics provider providerId missing providerId $exists true partialFilterExpression
💡 Result:
MongoDB semantics for a “sparse unique compound index where providerId is missing / $exists:true / partialFilterExpression provider.providerId …” 1) sparse + unique means “uniqueness only among indexed documents; missing fields are allowed” - MongoDB sparse indexes only include documents that have the indexed field(s) present, and a sparse unique index prevents duplicates for the indexed key values while allowing multiple documents that omit the key [1]. - For sparse compound indexes, MongoDB indexes a document if it contains at least one of the indexed fields; the combination of sparse-ness with compound keys affects whether documents without all fields get included in the index entry [1]. 2) You cannot combine sparse with partialFilterExpression in a single index definition - MongoDB partial indexes explicitly say: “You cannot specify both the partialFilterExpression option and the sparse option.” [2][3]. So the “sparse unique … partialFilterExpression …” combination is not valid as a single index spec. 3) If your real goal is “unique only when provider.providerId exists (and maybe is non-null),” use a partial unique index (not sparse) - Partial indexes include only documents that match partialFilterExpression, and when a unique constraint is defined on that partial index, the uniqueness constraint applies only to documents that meet the partial filter [2][3]. - The docs example shows partial unique indexes expressed with { $exists: true } (e.g., password: { $exists: true }) [2][3]. Example pattern (provider/providerId): - Index that enforces uniqueness of (provider, providerId) only for documents where providerId exists: db.collection.createIndex( { "provider": 1, "provider.providerId": 1 }, { unique: true, partialFilterExpression: { "provider.providerId": { $exists: true } } }) This matches the documented behavior that partial unique constraints apply only to documents satisfying partialFilterExpression [2][3], and it avoids any need for sparse. 4) $exists:true in queries and index usage: sparse can help, but it’s not the right tool for “uniqueness conditioned on field presence” - The $exists operator matches documents that contain the field (including documents where the field value is null) [4]. - MongoDB docs also note that you can make $exists: true queries more efficient by creating a sparse index on that field [4]. However, for uniqueness guarantees specifically, partial indexes provide the conditional uniqueness semantics you want (unique only for docs matching the filter), whereas sparse unique semantics are based on presence rules of sparse indexing and compound sparse indexing can still index documents with only partial key field presence [1]. 5) Practical takeaway for “providerId missing” cases - If you use sparse+unique on a compound key, documents where providerId is missing can still end up being indexed depending on which compound fields are present (sparse compound indexes index a document if it contains at least one indexed field) [1]. That can lead to surprising uniqueness behavior. - If you instead use a partial unique index with partialFilterExpression: { "provider.providerId": { $exists: true } }, then documents missing providerId are simply not part of the unique index constraint [2][3].
Citations:
- 1: https://www.mongodb.com/docs/manual/core/index-sparse/
- 2: https://www.mongodb.com/docs/v8.2/core/index-partial/
- 3: https://www.mongodb.com/docs/manual/core/index-partial/
- 4: https://www.mongodb.com/docs/manual/reference/operator/query/exists/
Fix User unique index to avoid local-providerId collisions
UserSchema.index({ provider: 1, providerId: 1 }, { unique: true, sparse: true })can still index local documents becauseprovideralways exists; whenproviderIdis missing, those documents share the same indexed key value for uniqueness, so the 2nd local signup can hit a duplicate-key error.- Apply uniqueness only to documents that actually have
providerIdvia a partial unique index.
Proposed fix
-UserSchema.index({ provider: 1, providerId: 1 }, { unique: true, sparse: true });
+UserSchema.index(
+ { provider: 1, providerId: 1 },
+ {
+ unique: true,
+ partialFilterExpression: { providerId: { $exists: true } },
+ }
+);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/models/User.js` at line 34, The current unique index on UserSchema
(UserSchema.index({ provider: 1, providerId: 1 }, { unique: true, sparse: true
})) still conflicts for local users because provider always exists and
providerId can be missing; replace this with a partial unique index so
uniqueness only applies when providerId is present by calling
UserSchema.index(...) with a partialFilterExpression such as { providerId: {
$exists: true } } (and keep unique: true) so only documents that actually have a
providerId are constrained.
| const normalizedEmail = email?.trim().toLowerCase(); | ||
|
|
||
| if (!providerId) { | ||
| throw new Error("OAuth profile is missing a provider id"); | ||
| } | ||
|
|
||
| let user = await User.findOne({ provider, providerId }); | ||
|
|
||
| if (user) { | ||
| return toSessionUser(user); | ||
| } | ||
|
|
||
| if (normalizedEmail) { | ||
| user = await User.findOne({ email: normalizedEmail }); |
There was a problem hiding this comment.
Canonicalize email consistently across local and OAuth auth paths.
Lines 32-45 lowercase the OAuth email before lookup, but local accounts in this PR are still persisted and queried with raw input elsewhere. A local account like Alice@Example.com can therefore miss this match and get a second OAuth account instead of being linked to the existing user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/utils/oauthUser.js` around lines 32 - 45, OAuth email lookup
lowercases the address (`normalizedEmail`) but local accounts are stored/queried
with the raw case, causing missed matches; fix by canonicalizing emails
consistently across both auth flows: normalize (trim().toLowerCase()) before
saving local user records and use the same normalized value (or a dedicated
canonical field) for all User.findOne lookups (including the OAuth path using
normalizedEmail) and for User.create/update logic so lookups (User.findOne) and
persistence share the same normalized key.
| if (user) { | ||
| if (user.provider === "local" && user.password) { | ||
| user.provider = provider; | ||
| user.providerId = providerId; | ||
| await user.save(); | ||
| return toSessionUser(user); | ||
| } |
There was a problem hiding this comment.
Don't convert local accounts into OAuth-only accounts here.
Lines 47-53 overwrite user.provider on an existing local account. After that, LocalStrategy rejects the same user, so email/password login is no longer retained for anyone who signs in with OAuth once. Keep the local account type intact here, or model linked identities separately.
Minimal safe change
if (user.provider === "local" && user.password) {
- user.provider = provider;
- user.providerId = providerId;
- await user.save();
return toSessionUser(user);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/utils/oauthUser.js` around lines 47 - 53, The code is overwriting
user.provider for existing local accounts (user.provider === "local") which
breaks LocalStrategy; instead do not change user.provider when it's "local":
remove the assignment user.provider = provider and avoid converting the account
to an OAuth-only type; if you need to record the OAuth identity, store
provider/providerId in a separate linked identity field (e.g., linkedProviders
or identities) or only set user.providerId (without changing user.provider),
save the user, and return toSessionUser(user) so email/password login remains
intact.
| if (oauthStatus === "error") { | ||
| setMessage(oauthMessage ? decodeURIComponent(oauthMessage) : "OAuth sign-in failed"); | ||
| setSearchParams({}, { replace: true }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does URLSearchParams.get() return decoded values, and can decodeURIComponent throw URIError when input contains a raw '%'?
💡 Result:
Yes. URLSearchParams.get returns decoded values. For decoding: MDN’s URLSearchParams docs state that when constructing from a query string, “percent-encoding is decoded,” and that when retrieving values “only decoded values are returned”; likewise, when getting an individual value, you use the decoded key and get the decoded value [1][2]. About decodeURIComponent and raw “%”: decodeURIComponent throws URIError when its input contains a “%” that is not followed by two hexadecimal digits (i.e., not valid percent-encoding) [3]. Separately, it’s also documented that decodeURIComponent throws URIError if encodedURI contains a “%” not followed by two hexadecimal digits [3]. So if the raw input you pass to decodeURIComponent contains a literal “%” that isn’t part of a valid %XX sequence (e.g., just “%” or “a%AFc” where the % sequence is not valid/complete for the decoder’s expectations), decodeURIComponent can throw URIError [3].
Citations:
- 1: https://dev.mozilla.org/en-US/docs/Web/API/URLSearchParams
- 2: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/pages/Login/Login.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Show the relevant region with line numbers
nl -ba "$FILE" | sed -n '1,140p' | sed -n '40,90p'Repository: GitMetricsLab/github_tracker
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/pages/Login/Login.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
# Print lines ~40-90 with line numbers via awk
awk 'NR>=35 && NR<=90 {printf "%5d: %s\n", NR, $0}' "$FILE"
# Also locate where oauthMessage is defined
rg -n "oauthMessage" "$FILE" || trueRepository: GitMetricsLab/github_tracker
Length of output: 2415
Avoid double-decoding the OAuth message query param (can throw URIError for raw %).
In src/pages/Login/Login.tsx, oauthMessage is already obtained via searchParams.get("message") (URLSearchParams returns decoded values). Calling decodeURIComponent again can throw when the decoded message contains a literal % (e.g., produced from an original %25). This can crash the login screen.
Suggested fix
if (oauthStatus === "error") {
- setMessage(oauthMessage ? decodeURIComponent(oauthMessage) : "OAuth sign-in failed");
+ setMessage(oauthMessage || "OAuth sign-in failed");
setSearchParams({}, { replace: true });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (oauthStatus === "error") { | |
| setMessage(oauthMessage ? decodeURIComponent(oauthMessage) : "OAuth sign-in failed"); | |
| setSearchParams({}, { replace: true }); | |
| if (oauthStatus === "error") { | |
| setMessage(oauthMessage || "OAuth sign-in failed"); | |
| setSearchParams({}, { replace: true }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Login/Login.tsx` around lines 50 - 52, The oauthMessage value
coming from searchParams.get("message") is already decoded, so remove the extra
decodeURIComponent call in the oauth error branch; change the logic in the block
that checks oauthStatus === "error" (where setMessage(...) is called) to use
oauthMessage directly (or fall back to "OAuth sign-in failed") and then call
setSearchParams({}, { replace: true }) as before—update references to
oauthStatus, oauthMessage, setMessage, and setSearchParams accordingly to avoid
double-decoding and potential URIError.
| export async function fetchCurrentUser(): Promise<AuthUser | null> { | ||
| try { | ||
| const { data } = await authApi.get<{ user: AuthUser }>("/me"); | ||
| return data.user; | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Handle only unauthenticated responses as null in fetchCurrentUser().
Returning null for every failure hides backend/network outages as “logged out” and makes auth state unreliable. Return null only for unauthorized responses, and rethrow everything else.
Suggested fix
export async function fetchCurrentUser(): Promise<AuthUser | null> {
try {
const { data } = await authApi.get<{ user: AuthUser }>("/me");
return data.user;
- } catch {
- return null;
+ } catch (error: unknown) {
+ const status =
+ typeof error === "object" &&
+ error !== null &&
+ "response" in error &&
+ typeof (error as { response?: { status?: unknown } }).response?.status === "number"
+ ? (error as { response?: { status?: number } }).response?.status
+ : undefined;
+
+ if (status === 401) return null;
+ throw error;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/authApi.ts` around lines 27 - 33, The fetchCurrentUser function
currently returns null for any error; change the catch to inspect the thrown
error (from authApi.get) and only return null when the response indicates an
unauthorized status (e.g., status === 401 or 403), otherwise rethrow the error
so network/backend failures bubble up; locate the fetchCurrentUser function and
modify its catch to check the error's response.status (or use AxiosError type
guards) and handle 401/403 with return null, rethrow for all other cases.
mehul-m-prajapati
added
onhold
and removed
level:intermediate
quality:clean
gssoc:approved
type:accessibility
labels
2 participants
Adds OAuth2 authentication for login using Google and GitHub, while keeping existing email/password signup and login.
passport-google-oauth20) and GitHub (passport-github2)provider,providerId, and optional password for OAuth accounts/api/auth/google,/api/auth/github, callbacks,/api/auth/me,/api/auth/oauth/providersauthApiclient withwithCredentialsfor session cookiesbackend/.env.exampleand.env.examplefor required environment variablesMotivation
Improves security and UX with standardized third-party sign-in, aligned with the OAuth2 feature request.
Setup (for reviewers)
See README → OAuth2 sign-in (Google & GitHub):
cp .env.example .envandcp backend/.env.example backend/.env{BACKEND_URL}/api/auth/google/callbackand.../github/callbackVITE_BACKEND_URLto matchBACKEND_URL5001if5000is taken by AirPlayTest plan
backend/.env, log in via Googlebackend/.env, log in via GitHubGET /api/auth/mereturns user when session is activeGET /api/auth/logoutclears sessionnpx jasmine spec/**/*.spec.cjswith MongoDB runningSummary by CodeRabbit
New Features
Documentation
Tests