fix: honor inviteId on existing-user sign-in paths (#385)#386
Merged
lindesvard merged 2 commits intoJun 4, 2026
Merged
Conversation
Invites were only consumed during new-account creation, so an invited person who authenticated as an existing user (OAuth sign-in or email sign-in) had their inviteId silently dropped. With zero org memberships they were redirected into the Create Project onboarding wizard instead of joining the inviting organization. - connectUserToOrganization is now idempotent: reuse an existing membership instead of creating a duplicate (no unique constraint on members.organizationId+userId), then still consume the invite. - OAuth handleExistingUser now accepts inviteId and connects the org (best-effort, logged) for both GitHub and Google callbacks. - signInEmail accepts an optional inviteId and consumes it on success; for 2FA users it is carried through the challenge via a short-lived cookie and consumed in signInTotp. - Frontend forwards inviteId on the sign-in path (email + OAuth), the login route reads it from search params, and the onboarding "Sign in" link preserves ?inviteId so returning users don't lose it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
Contributor
📝 WalkthroughWalkthroughThis PR carries an optional inviteId through sign-in (email, OAuth, 2FA) and consumes invites to connect authenticated users to organizations; it prevents duplicate memberships and adds a DB migration enforcing (organizationId, userId) uniqueness. ChangesInvite-based Organization Onboarding Through Auth
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related Issues
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/start/src/routes/_login.login.tsx (1)
39-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
inviteIdin the onboarding link.This page now carries invite context, but the "Create one today" link drops it. Invited users who switch from sign-in to sign-up will lose the invite before reaching the
signUpEmailpath.Suggested fix
- <a - className="font-medium text-foreground underline" - href="/onboarding" - > + <a + className="font-medium text-foreground underline" + href={ + inviteId + ? `/onboarding?inviteId=${encodeURIComponent(inviteId)}` + : '/onboarding' + } + > Create one today </a>🤖 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 `@apps/start/src/routes/_login.login.tsx` around lines 39 - 45, The "Create one today" anchor in _login.login.tsx drops the invite context; read the current inviteId (e.g., via useSearchParams or location.search) and append/preserve it when building the onboarding link for the anchor that renders "Create one today" so the inviteId query param is carried through to the signUpEmail flow (ensure you merge with existing query params rather than overwriting them).apps/api/src/controllers/oauth-callback.controller.tsx (1)
302-324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume and clear the invite cookie in the callback.
Both callbacks read
req.cookies.inviteIdbut never clear it. That leaves the old invite attached to the browser for the next OAuth attempt, so a later login can reuse the previous invite and join the wrong account to the organization.Suggested fix
export async function githubCallback(req: FastifyRequest, reply: FastifyReply) { + const inviteId = req.cookies.inviteId; + reply.clearCookie('inviteId'); try { const { code } = await validateOAuthCallback(req, 'github'); - const inviteId = req.cookies.inviteId; const tokens = await github.validateAuthorizationCode(code);export async function googleCallback(req: FastifyRequest, reply: FastifyReply) { + const inviteId = req.cookies.inviteId; + reply.clearCookie('inviteId'); try { const { code } = await validateOAuthCallback(req, 'google'); - const inviteId = req.cookies.inviteId; const codeVerifier = req.cookies.google_code_verifier!;Also applies to: 344-368
🤖 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 `@apps/api/src/controllers/oauth-callback.controller.tsx` around lines 302 - 324, Read and then clear the invite cookie so it cannot persist across OAuth attempts: after capturing req.cookies.inviteId into the inviteId variable in oauth-callback.controller.tsx, call reply.clearCookie with the same cookie name (e.g., 'inviteId') so the cookie is removed; do this in both places where inviteId is read (the block around handleExistingUser where inviteId is set and the other callback block around lines 344-368) so the invite cannot be reused on subsequent logins.packages/trpc/src/routers/auth.ts (1)
130-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale
INVITE_COOKIEvalues on non-invite auth attempts.This cookie is only written when
input.inviteIdexists, but in this file it is only cleared after a successful TOTP consumption. If a user starts an invite flow, abandons it, and then signs in again without an invite before the TTL expires, the oldinviteIdcan still be consumed duringsignInTotpand attach the wrong account to the organization.Suggested fix
if (input.inviteId) { - ctx.setCookie('inviteId', input.inviteId, { - maxAge: 60 * 10, - }); + ctx.setCookie(INVITE_COOKIE, input.inviteId, { + maxAge: 60 * 10, + ...COOKIE_OPTIONS, + }); + } else { + ctx.setCookie(INVITE_COOKIE, '', { + maxAge: 0, + ...COOKIE_OPTIONS, + }); }if (input.inviteId) { ctx.setCookie(INVITE_COOKIE, input.inviteId, { maxAge: TWO_FACTOR_CHALLENGE_TTL_SECONDS, + ...COOKIE_OPTIONS, }); + } else { + ctx.setCookie(INVITE_COOKIE, '', { + maxAge: 0, + ...COOKIE_OPTIONS, + }); }const inviteId = ctx.cookies[INVITE_COOKIE]; + ctx.setCookie(INVITE_COOKIE, '', { + maxAge: 0, + ...COOKIE_OPTIONS, + }); if (inviteId) { await consumeInviteForUser(challenge.userId, inviteId, ctx.req.log); - ctx.setCookie(INVITE_COOKIE, '', { maxAge: 0 }); }Also applies to: 280-286, 371-375
🤖 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 `@packages/trpc/src/routers/auth.ts` around lines 130 - 133, The issue: the inviteId cookie (INVITE_COOKIE) is only written when input.inviteId exists but not cleared when a non-invite auth attempt occurs, allowing stale inviteIds to be consumed later (e.g., in signInTotp). Fix: in each auth entry-point that currently sets ctx.setCookie('inviteId', ...) (referenced here as input.inviteId and ctx.setCookie) add logic to explicitly clear the invite cookie when input.inviteId is falsy — i.e., call the cookie clear/remove API on ctx (the same mechanism used in signInTotp to clear post-success) whenever no inviteId is provided. Apply this change to the three locations noted (the current diff and the other blocks around lines ~280-286 and ~371-375) so stale INVITE_COOKIE values are removed on non-invite sign-ins.
🤖 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 `@packages/db/src/services/organization.service.ts`:
- Around line 164-184: The current findFirst + create sequence
(db.member.findFirst and db.member.create) is racy and can still produce
duplicate Member rows; add a unique constraint on Member for (organizationId,
userId) in the Prisma schema and run a migration, then replace the non-atomic
logic in the invite consumption (the existingMember / member assignment that
reads invite and user) with a database atomic operation: use db.member.upsert
with a composite unique where (organizationId_userId) or perform the create
inside a transaction and handle unique-constraint failures by re-querying the
existing member; ensure the code references the same invite.role,
invite.createdById, invite.organizationId and user.id/email when
creating/upserting so the operation is idempotent.
---
Outside diff comments:
In `@apps/api/src/controllers/oauth-callback.controller.tsx`:
- Around line 302-324: Read and then clear the invite cookie so it cannot
persist across OAuth attempts: after capturing req.cookies.inviteId into the
inviteId variable in oauth-callback.controller.tsx, call reply.clearCookie with
the same cookie name (e.g., 'inviteId') so the cookie is removed; do this in
both places where inviteId is read (the block around handleExistingUser where
inviteId is set and the other callback block around lines 344-368) so the invite
cannot be reused on subsequent logins.
In `@apps/start/src/routes/_login.login.tsx`:
- Around line 39-45: The "Create one today" anchor in _login.login.tsx drops the
invite context; read the current inviteId (e.g., via useSearchParams or
location.search) and append/preserve it when building the onboarding link for
the anchor that renders "Create one today" so the inviteId query param is
carried through to the signUpEmail flow (ensure you merge with existing query
params rather than overwriting them).
In `@packages/trpc/src/routers/auth.ts`:
- Around line 130-133: The issue: the inviteId cookie (INVITE_COOKIE) is only
written when input.inviteId exists but not cleared when a non-invite auth
attempt occurs, allowing stale inviteIds to be consumed later (e.g., in
signInTotp). Fix: in each auth entry-point that currently sets
ctx.setCookie('inviteId', ...) (referenced here as input.inviteId and
ctx.setCookie) add logic to explicitly clear the invite cookie when
input.inviteId is falsy — i.e., call the cookie clear/remove API on ctx (the
same mechanism used in signInTotp to clear post-success) whenever no inviteId is
provided. Apply this change to the three locations noted (the current diff and
the other blocks around lines ~280-286 and ~371-375) so stale INVITE_COOKIE
values are removed on non-invite sign-ins.
🪄 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: ade9aa0d-55aa-4859-915c-018220dea6ee
📒 Files selected for processing (9)
apps/api/src/controllers/oauth-callback.controller.tsxapps/start/src/components/auth/sign-in-email-form.tsxapps/start/src/components/auth/sign-in-github.tsxapps/start/src/components/auth/sign-in-google.tsxapps/start/src/routes/_login.login.tsxapps/start/src/routes/_public.onboarding.tsxpackages/db/src/services/organization.service.tspackages/trpc/src/routers/auth.tspackages/validation/src/index.ts
The findFirst + create sequence in connectUserToOrganization was non-atomic and could produce duplicate Member rows when an invite was consumed concurrently. Enforce uniqueness at the database level and make the membership creation atomic. - Add @@unique([organizationId, userId]) to the Member model. NULLs are distinct in Postgres, so pending email-only invite rows are unaffected. - Migration de-duplicates existing memberships (keeping the earliest) before creating the unique index so it is deploy-safe. - Replace the racy findFirst ?? create with an atomic upsert keyed on the composite unique, reusing an existing membership unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/db/prisma/migrations/20260603111500_add_member_org_user_unique/migration.sql (2)
5-13: 💤 Low valueDedup DELETE logic is correct.
The self-join keeps the earliest
createdAtper(organizationId, userId)withidas a deterministic tie-breaker, and them."userId" IS NOT NULLguard correctly excludes pending email-only invites. Note this is a destructive, irreversible delete with no down migration; ensure a backup/snapshot exists before applying in production.🤖 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 `@packages/db/prisma/migrations/20260603111500_add_member_org_user_unique/migration.sql` around lines 5 - 13, The migration's destructive DELETE on "public"."members" removes duplicate rows and has no down migration; before running it create a reversible backup and a restore path: copy affected rows to a backup table (or archive table) that captures full rows matching the duplicate criteria used by the DELETE (referencing "organizationId", "userId", "createdAt", "id"), then perform the DELETE; also add a down migration that restores rows from that backup table (or provide explicit restore instructions) so the change is reversible and safe to apply in production.
16-16: ⚖️ Poor tradeoffNon-concurrent unique index build takes an
ACCESS EXCLUSIVE-blocking lock.
CREATE UNIQUE INDEX(withoutCONCURRENTLY) holds aSHARElock that blocks writes tomembersfor the duration of the build. On a large table this can cause a noticeable write stall during deploy. Prisma wraps migrations in a transaction, soCONCURRENTLYcannot be used here directly; ifmembersis large, consider building the index out-of-band (concurrently, outside the migration transaction) and then marking this migration as applied. For small tables this is a non-issue.🤖 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 `@packages/db/prisma/migrations/20260603111500_add_member_org_user_unique/migration.sql` at line 16, The migration currently creates a non-concurrent unique index "members_organizationId_userId_key" on table "members" which will take an ACCESS EXCLUSIVE-blocking lock for the duration of the build; to avoid downtime for large tables, build the index out-of-band using CREATE UNIQUE INDEX CONCURRENTLY "members_organizationId_userId_key" ON "public"."members"( "organizationId","userId") in a separate non-transactional session, then update this Prisma migration to not run the blocking CREATE UNIQUE INDEX (remove or replace it with a no-op/comment) and mark the migration as applied (e.g., prisma migrate resolve or record the migration as succeeded) so the migration state matches the database.
🤖 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.
Nitpick comments:
In
`@packages/db/prisma/migrations/20260603111500_add_member_org_user_unique/migration.sql`:
- Around line 5-13: The migration's destructive DELETE on "public"."members"
removes duplicate rows and has no down migration; before running it create a
reversible backup and a restore path: copy affected rows to a backup table (or
archive table) that captures full rows matching the duplicate criteria used by
the DELETE (referencing "organizationId", "userId", "createdAt", "id"), then
perform the DELETE; also add a down migration that restores rows from that
backup table (or provide explicit restore instructions) so the change is
reversible and safe to apply in production.
- Line 16: The migration currently creates a non-concurrent unique index
"members_organizationId_userId_key" on table "members" which will take an ACCESS
EXCLUSIVE-blocking lock for the duration of the build; to avoid downtime for
large tables, build the index out-of-band using CREATE UNIQUE INDEX CONCURRENTLY
"members_organizationId_userId_key" ON "public"."members"(
"organizationId","userId") in a separate non-transactional session, then update
this Prisma migration to not run the blocking CREATE UNIQUE INDEX (remove or
replace it with a no-op/comment) and mark the migration as applied (e.g., prisma
migrate resolve or record the migration as succeeded) so the migration state
matches the database.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a0ff8b8-a9ca-45c9-b0df-fa59512689ba
📒 Files selected for processing (3)
packages/db/prisma/migrations/20260603111500_add_member_org_user_unique/migration.sqlpackages/db/prisma/schema.prismapackages/db/src/services/organization.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/services/organization.service.ts
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
3 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.
Invites were only consumed during new-account creation, so an invited person who authenticated as an existing user (OAuth sign-in or email sign-in) had their inviteId silently dropped. With zero org memberships they were redirected into the Create Project onboarding wizard instead of joining the inviting organization.
closes #385
Summary by CodeRabbit
New Features
Bug Fixes