[1b] Google OAuth sign-in + JWT session#23
Open
andrmaz wants to merge 4 commits into
Open
Conversation
- Add AuthModule with GoogleStrategy (passport-google-oauth20) and JwtStrategy (passport-jwt) for two-factor identity verification - Add AuthService with issueToken / verifyToken / decodeToken methods - Add JwtAuthGuard that rejects missing/expired tokens with a structured UnauthorizedException - Add AuthController: GET /auth/google (OAuth redirect) and GET /auth/google/callback (upsert user, issue JWT) - Add MeController: GET /api/me (returns authenticated user identity) - Add UserService for DB-backed user lookup / first-time provisioning - Add PrismaService + PrismaModule (global) wrapping the generated client - Declare JWT_SECRET, GOOGLE_* env vars in turbo.json globalEnv - Add .env.example documenting required environment variables - Add tsconfig paths for db/client so TS resolves the Prisma generated client from within the API workspace - Write 14 unit tests covering token issuance, verification, expiry, tampering, and JWT strategy payload validation (all passing) - Update app.module.spec.ts to mock GoogleStrategy + PrismaService so the existing integration tests remain green without real credentials Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 43 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements a complete Google OAuth and JWT authentication system for the NestJS API. It adds authentication controllers for login and user retrieval, Passport strategies for Google OAuth and JWT validation, route-protecting guards, database user provisioning with organization linking, token issuance/verification services, and updates the application module to wire everything together. ChangesAuthentication system with Google OAuth and JWT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
5 tasks
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@apps/api/src/auth/auth.controller.ts`:
- Around line 51-60: The code issues JWTs before ensuring dbUser.organizationId
is truthy, causing tokens that jwt.strategy.ts will immediately reject; update
the flow in auth.controller.ts so you only build AuthenticatedUser and call
this.authService.issueToken(...) after confirming a valid organizationId: check
dbUser.organizationId after this.userService.findOrCreate({ googleSub, email });
if it's falsy either (a) await a new/existing userService method to assign or
ensure an organization (e.g., ensureOrganizationAssigned(dbUser) or
assignDefaultOrganization(dbUser)) and refresh dbUser, or (b) reject the auth
callback / throw a clear error so no token is minted; in short, move the
AuthenticatedUser construction and issueToken call to occur only after
organization assignment/verification (or return an explicit failure) so tokens
are never self-invalidating.
In `@apps/api/src/auth/auth.module.ts`:
- Around line 14-15: The code currently falls back to a hard-coded JWT secret;
update the JwtModule registration (the secret property used in
JwtModule.register / JwtModule.registerAsync inside AuthModule) to remove the
"changeme-dev-secret" fallback and instead read process.env.JWT_SECRET and fail
fast when it's missing (e.g., throw a clear Error at startup or assert the env
var before passing it to JwtModule). Concretely: replace secret:
process.env["JWT_SECRET"] ?? "changeme-dev-secret" with logic that reads
process.env.JWT_SECRET, throws an Error("JWT_SECRET environment variable is
required") if falsy, and returns the value so the app will not start without a
configured secret.
In `@apps/api/src/auth/guards/jwt-auth.guard.ts`:
- Around line 14-18: handleRequest in the JwtAuthGuard currently exposes
err?.message in the UnauthorizedException; change it to always throw a stable
public message (e.g. new UnauthorizedException("Missing or invalid
authentication token")) and do NOT include err.message in the exception; instead
log the underlying error server-side—import and use Nest's Logger (or an
existing logger) inside handleRequest (e.g. Logger.error(err?.message, err) or
logger.error(err?.stack || err?.message)) so the raw verifier/passport details
are recorded internally but never sent to the client.
In `@apps/api/src/auth/strategies/google.strategy.ts`:
- Around line 27-33: The code currently accepts profile.emails?.[0]?.value
without ensuring the email is verified; update the Google strategy callback to
look for a verified email (check profile.emails array for an entry where .value
exists and .verified is true, not just taking index 0) and only call done(null,
user) with that verified address; if no verified email is found, call done(new
Error("No verified email returned from Google"), undefined) to prevent
provisioning. Ensure you reference the same profile.emails and done symbols so
UserService.findOrCreate never receives an unverified email.
In `@apps/api/src/auth/strategies/jwt.strategy.ts`:
- Around line 17-18: The JWT validation in JwtStrategy currently rejects falsy
organizationId which conflicts with UserService provisioning that sets
organizationId to an empty string; update the JwtStrategy.validate (or
equivalent) check to only fail when payload.organizationId is strictly undefined
or null (allowing an empty string) OR change UserService to provision a
non-empty organizationId so the existing check remains valid—pick one consistent
contract and apply it across JwtStrategy.validate and UserService
(user.service.ts) so token issuance and validation agree.
- Around line 12-13: The code currently falls back to a predictable secret
("changeme-dev-secret") via process.env["JWT_SECRET"] ?? "changeme-dev-secret",
which must be removed; instead ensure the JWT secret is required at startup by
throwing a clear error or failing fast when process.env["JWT_SECRET"] is
missing, and wire secretOrKey to that required value (or to a secure config
provider), e.g. compute const jwtSecret = process.env["JWT_SECRET"]; if
(!jwtSecret) throw new Error("JWT_SECRET must be set"); then set secretOrKey:
jwtSecret so the Jwt strategy uses a non-predictable secret.
In `@apps/api/src/auth/user.service.ts`:
- Line 38: The current domain extraction const domain =
input.email.split("@")[1] ?? "" can silently produce an empty domain for
malformed emails; update the code in the user service where input.email is
processed to validate the email first (e.g., ensure input.email is a non-empty
string containing exactly one "@" and both local and domain parts are non-empty,
or run a simple regex), and if invalid fail-fast by throwing a clear error or
returning a validation failure before using domain; replace the direct split
with a guarded extraction (check input.email.includes("@") or use email.match)
and only set domain when the validation passes.
- Around line 43-50: The code in create user flow sets organizationId: org?.id
?? "" which will violate the required FK when no organization is found; in
apps/api/src/auth/user.service.ts (look for the prisma.user.create call and the
org variable from findFirst) change the behavior so you do not insert an empty
string: check if org is missing and either (A) reject the request by throwing a
clear error (e.g., throw new Error('No organization found for email domain') or
a domain-specific HTTP error) before calling prisma.user.create and keep
organizationId set to org.id, or (B) implement an explicit fallback (create/use
a real “Unassigned” organization and use its id) or (C) change the schema to
make User.organizationId nullable and handle null downstream — pick one of these
approaches and update the prisma.user.create invocation to pass a valid
organizationId (org.id) instead of an empty string.
🪄 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: d67b0968-c591-4249-98b5-3553a25ba8c6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.env.exampleapps/api/jest.config.jsapps/api/package.jsonapps/api/src/__mocks__/db-client.mock.tsapps/api/src/app.module.spec.tsapps/api/src/app.module.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/auth.service.spec.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/auth.types.tsapps/api/src/auth/guards/google-auth.guard.tsapps/api/src/auth/guards/jwt-auth.guard.tsapps/api/src/auth/strategies/google.strategy.tsapps/api/src/auth/strategies/jwt.strategy.spec.tsapps/api/src/auth/strategies/jwt.strategy.tsapps/api/src/auth/user.service.tsapps/api/src/prisma/prisma.module.tsapps/api/src/prisma/prisma.service.tsapps/api/tsconfig.jsonturbo.json
Owner
Author
|
@cursoragent Fix all conversations |
Contributor
|
All 8 CodeRabbit review issues have been resolved. Here's a summary of what was fixed: Security hardening:
Contract correctness:
Tests: Added   |
…ixes
- jwt-auth.guard.ts: replace err?.message with a stable public message
("Missing or invalid authentication token") and log the raw error
server-side with NestJS Logger to prevent internal detail leakage
- auth.module.ts: switch to JwtModule.registerAsync; throw at startup
when JWT_SECRET is absent instead of falling back to a hard-coded value
- jwt.strategy.ts: read JWT_SECRET eagerly in constructor and throw when
it is missing; remove the "changeme-dev-secret" fallback
- google.strategy.ts: require a verified email (emails[].verified===true)
before forwarding the profile; unverified addresses are now rejected
- user.service.ts:
* validate email format (throw if no '@' or empty local-part)
* throw UnauthorizedException when no Organization is provisioned for the
email domain — eliminates the empty-string organizationId that would
cause FK constraint failures and self-invalidating JWTs
* remove the org?.id ?? "" sentinel that masked missing org assignment
- auth.controller.ts: update jsdoc to reflect that JWT issuance is now
guarded by the service-level org check
- app.module.spec.ts: set JWT_SECRET in beforeAll so JwtModule.registerAsync
and JwtStrategy can initialise without a real secret in tests
- jwt.strategy.spec.ts: add test asserting constructor throws when
JWT_SECRET is unset
- user.service.spec.ts: new test suite (6 tests) covering findOrCreate —
returns existing user, creates with valid org, rejects missing org,
rejects malformed email (no '@', leading '@'), correct domain extraction
All 50 tests pass.
Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/api/src/auth/user.service.ts`:
- Around line 45-47: The code uses prisma.organization.findFirst({ where: {
name: domain } }) which is ambiguous because Organization.name is not unique;
update the Prisma schema to add a uniqueness constraint on Organization.name
(add `@unique` to the name field) and run a migration, then replace the lookup
call in the user service (the this.prisma.organization.findFirst call) with a
unique lookup such as this.prisma.organization.findUnique({ where: { name:
domain } }) or this.prisma.organization.findUniqueOrThrow({ where: { name:
domain } }) and adjust error handling for the not-found case accordingly.
- Around line 31-34: The check-then-create in findOrCreate is racy: remove the
separate pre-read and make creation atomic by using an upsert or by doing a
guarded create-with-recovery; specifically, replace the pattern that calls
findByGoogleSub then create with either (a) a single upsert call that returns
the existing-or-new row, or (b) attempt create(...) inside findOrCreate and
catch the unique-constraint error and on that error call findByGoogleSub(...) to
return the already-created user; update symbols: findOrCreate, findByGoogleSub,
and create to follow one of these atomic approaches so concurrent requests
cannot both pass the read and trigger a failing insert.
- Around line 36-43: Replace the raw Error throws that interpolate user input
with Nest HTTP exceptions: in the code that validates input.email (the checks
using atIndex = input.email.indexOf("@") and domain = input.email.slice(...)),
throw new BadRequestException('Invalid email format') instead of new
Error(`Invalid email format: ${input.email}`); also import BadRequestException
from '`@nestjs/common`' and remove any usage of the raw email in the exception
message so no PII is exposed.
🪄 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: 4b05d618-9a0c-45e9-9bc7-2f823305814e
📒 Files selected for processing (9)
apps/api/src/app.module.spec.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/guards/jwt-auth.guard.tsapps/api/src/auth/strategies/google.strategy.tsapps/api/src/auth/strategies/jwt.strategy.spec.tsapps/api/src/auth/strategies/jwt.strategy.tsapps/api/src/auth/user.service.spec.tsapps/api/src/auth/user.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/auth/strategies/jwt.strategy.spec.ts
- apps/api/src/auth/guards/jwt-auth.guard.ts
- apps/api/src/app.module.spec.ts
- apps/api/src/auth/auth.controller.ts
Owner
Author
|
@cursoragent Fix all conversations |
Contributor
|
Taking a look! |
Contributor
|
All three CodeRabbit findings from round 2 are resolved. Here's what was done:
Tests: expanded |
…ueness Organization.name uniqueness (schema + migration): - Add @unique to Organization.name in packages/db/prisma/schema.prisma so domain-based org lookup is unambiguous and free of multi-row races - Add migration 0002_organization_name_unique with the corresponding CREATE UNIQUE INDEX statement - Regenerate Prisma client (findUnique now accepts { where: { name } }) user.service.ts — three fixes: 1. Race-safe findOrCreate: attempt prisma.user.create; on P2002 unique- constraint collision (concurrent OAuth callback), fall back to prisma.user.upsert({ update: {} }) which atomically returns the row already inserted by the racing request 2. BadRequestException instead of raw Error for malformed email format; error message contains no PII (email value removed) 3. Replace prisma.organization.findFirst with prisma.organization.findUnique (correct since Organization.name is now @unique) db-client.mock.ts: - Add user.upsert and organization.findUnique to the Jest mock so new code paths are exercisable in unit tests user.service.spec.ts: - Update organization lookup assertions to use findUnique - Add test: existing user skips org lookup and create/upsert entirely - Add test: P2002 race recovery resolves via upsert - Add test: non-P2002 DB errors are re-thrown unchanged - Add test: BadRequestException message contains no PII All 53 tests pass. Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
The PR targets the develop branch but ci.yml only fired on push/PR events targeting main, so CI was silently skipped on all feature PRs merging into develop. Add develop to both push and pull_request branch filters. Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
Owner
Author
|
@cursoragent CI test did not run. CI test is currently on develop |
Contributor
|
CI is now fully green. The fix from the previous turn — adding
The root cause was that the workflow only listened for events on |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What was built
Implements the Google OAuth sign-in flow and JWT-based session management for the Cortex API.
Changes
New:
apps/api/src/auth/auth.types.ts— shared types:GoogleProfile,JwtPayload,AuthenticatedUserauth.service.ts—issueToken,verifyToken,decodeToken(wraps@nestjs/jwt)auth.controller.ts— two controllers:AuthController:GET /auth/google(OAuth redirect) andGET /auth/google/callback(upsert user, return JWT)MeController:GET /api/me(returns authenticated user identity, protected byJwtAuthGuard)user.service.ts—findByGoogleSub,findById,findOrCreate(first-time provisioning viaPrismaService)auth.module.ts— wiresPassportModule,JwtModule(8 h expiry), strategies, guards, servicesstrategies/google.strategy.ts—passport-google-oauth20strategy; requires a verified email addressstrategies/jwt.strategy.ts—passport-jwtBearer-token strategy; validates payload fields; fails fast whenJWT_SECRETis absentguards/jwt-auth.guard.ts— NestJS guard; throws stable publicUnauthorizedException, logs raw error server-sideguards/google-auth.guard.ts— thin wrapper aroundAuthGuard("google")New:
apps/api/src/prisma/prisma.service.ts—PrismaClientwrapper withOnModuleInit/OnModuleDestroylifecycle hooksprisma.module.ts—@Global()module soPrismaServiceis available everywhereUnit tests (53 total, all passing)
auth.service.spec.ts— 10 tests: token issuance, verification, decodestrategies/jwt.strategy.spec.ts— 5 tests: payload validation + constructor throws whenJWT_SECRETmissinguser.service.spec.ts— 9 tests: existing user fast-path, create with org, reject no-org, bad email format (no PII), domain extraction, P2002 race recovery via upsert, non-P2002 error re-throwSecurity hardening (CodeRabbit round 1)
JWT_SECRETfail-fast:JwtModule.registerAsyncandJwtStrategyconstructor both throw at startup whenJWT_SECRETis unsetJwtAuthGuardalways emits"Missing or invalid authentication token"; raw errors logged server-side viaLoggerGoogleStrategyrequiresemails[].verified === trueUserService.findOrCreatethrowsUnauthorizedExceptionwhen no org matchesAtomicity + correctness (CodeRabbit round 2)
findOrCreate:prisma.user.createfollowed byprisma.user.upsert({ update: {} })onP2002— concurrent OAuth callbacks for the samegoogleSubare handled without duplicate-insert failuresBadRequestExceptionfor malformed email: replaces rawError; message contains no PIIOrganization.name @unique: schema constraint added so domain lookup usesfindUnique(unambiguous, no multi-row hazard); migration0002_organization_name_uniqueincludedSupporting changes
packages/db/prisma/schema.prisma—Organization.name @uniquepackages/db/prisma/migrations/0002_organization_name_unique/—CREATE UNIQUE INDEXmigrationapps/api/src/__mocks__/db-client.mock.ts— addeduser.upsertandorganization.findUniqueapp.module.ts— importsPrismaModuleandAuthModuleturbo.json—globalEnvfor auth env vars.env.example— documents required environment variablesAcceptance criteria
GET /auth/google/callbackreturns{ accessToken })JwtStrategy+JwtAuthGuard)/api/mereturns authenticated user identityUnauthorizedException)Environment variables required
Set these before running the API (see
.env.example):Summary by CodeRabbit
Release Notes
New Features
Chores