fix(billing): route member creation through Better Auth hooks#1319
Conversation
📝 WalkthroughWalkthroughReplaces direct member table inserts with calls to the centralized auth API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TRPC_Router as TRPC Router
participant Auth_API as Auth API
participant DB as Database
Client->>TRPC_Router: addMember(input, headers)
TRPC_Router->>Auth_API: auth.api.addMember(body{organizationId,userId,role}, headers)
Auth_API->>DB: insert member record
DB-->>Auth_API: inserted member
Auth_API-->>TRPC_Router: API response (member)
TRPC_Router-->>Client: return API response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/auth/src/lib/accept-invitation-endpoint.ts (1)
121-145:⚠️ Potential issue | 🟠 MajorInvitation marked "accepted" before member creation — failure leaves inconsistent state.
If
auth.api.addMemberthrows (e.g. seat-limit exceeded from thebeforeAddMemberhook), the invitation is already marked"accepted"(line 122-125) but no member record exists. The user would see a stale "accepted" invitation with no actual membership and no way to re-accept.Consider either:
- Reordering: create the member first, then mark the invitation accepted, or
- Wrapping both in a transaction with rollback on failure.
This was likely pre-existing with the old direct-insert path, but it's more likely to surface now that
addMemberenforces seat limits.packages/trpc/src/router/organization/organization.ts (1)
316-333:⚠️ Potential issue | 🔴 CriticalMissing authorization check — any authenticated user can add members to any organization.
Unlike
removeMember(lines 342-402) andupdateMemberRole(lines 463-543), theaddMembermutation performs no permission verification before calling the API. Both sibling mutations explicitly verify the caller is an organization member, check their role, and validate permissions—then delegate toauth.api.removeMemberorauth.api.updateMemberRole.The
beforeAddMemberhook inpackages/auth/src/server.tsreceives only{ organization }context, not caller information, so it cannot enforce who can add members.Add an explicit authorization check to match the pattern:
- Verify the caller is a member of the organization
- Verify the caller has owner or admin role
- Then call
ctx.auth.api.addMember
🧹 Nitpick comments (1)
packages/auth/src/lib/accept-invitation-endpoint.ts (1)
142-143: Unsafeascast oninvitation.role— consider runtime validation.The
as "member" | "owner" | "admin"is a compile-time-only assertion; ifinvitation.roleholds an unexpected value at runtime, it will be passed through unchecked. The?? "member"fallback only triggers onnull/undefined, not on an invalid string.A quick guard would be safer:
Proposed fix
- role: - (invitation.role as "member" | "owner" | "admin") ?? "member", + role: (["member", "owner", "admin"].includes(invitation.role) + ? invitation.role + : "member") as "member" | "owner" | "admin",
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Two codepaths were creating org members via raw db.insert(members), bypassing beforeAddMember (free plan limit) and afterAddMember (Stripe seat update + emails). Replace with auth.api.addMember() in both the tRPC addMember mutation and invitation acceptance endpoint.
saddlepaddle
force-pushed
the
super-283
branch
from
093c15f to
e660751
Compare
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)
packages/auth/src/lib/accept-invitation-endpoint.ts (1)
121-148:⚠️ Potential issue | 🔴 CriticalInvitation marked "accepted" before
addMember— leaves inconsistent state on failure.If
auth.api.addMemberthrows (e.g.,beforeAddMemberrejects because the org hit the free-plan seat limit), the invitation is already set to"accepted"(line 122-125) but no member record exists. The user can't retry because thestatus !== "pending"guard on line 60 will reject them.Move the invitation status update to after the
addMembercall so a failed add leaves the invitation retryable:Proposed fix
- // 5. Accept invitation by updating status and creating member - await db - .update(invitations) - .set({ status: "accepted" }) - .where(eq(invitations.id, invitationId)); - - // Create member record (check if not already a member) + // 5. Create member record (check if not already a member) const existingMember = await db.query.members.findFirst({ where: and( eq(members.organizationId, invitation.organization.id), eq(members.userId, user.id), ), }); if (!existingMember) { const { auth } = await import("../server"); await auth.api.addMember({ body: { organizationId: invitation.organization.id, userId: user.id, role: (invitation.role as "member" | "owner" | "admin") ?? "member", }, }); } + // Mark invitation accepted only after member creation succeeds + await db + .update(invitations) + .set({ status: "accepted" }) + .where(eq(invitations.id, invitationId));packages/trpc/src/router/organization/organization.ts (2)
316-333:⚠️ Potential issue | 🔴 CriticalMissing authorization — any authenticated user can add members to any organization.
removeMember(line 355-392) andupdateMemberRole(line 484-492) both verify the caller is a member with a sufficient role before proceeding.addMemberhas no such check: any logged-in user can add anyuserIdto anyorganizationId.Even if
auth.api.addMemberperforms some internal checks, the tRPC layer should enforce authorization consistently with the sibling mutations.Proposed fix
.mutation(async ({ ctx, input }) => { + const actorMembership = await db.query.members.findFirst({ + where: and( + eq(members.organizationId, input.organizationId), + eq(members.userId, ctx.session.user.id), + ), + }); + + if (!actorMembership || actorMembership.role === "member") { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Only owners and admins can add members", + }); + } + const member = await ctx.auth.api.addMember({ body: { organizationId: input.organizationId,
116-122:⚠️ Potential issue | 🟡 MinorThe founding owner will miss the welcome email since
createbypassesafterAddMemberhooks.While bypassing the seat-limit check in
beforeAddMemberis intentional (new org has no constraints), the founding owner should still receive the welcome email sent byafterAddMember. Consider callingctx.auth.api.addMember()like theaddMemberprocedure does (line 316) to trigger the email, or explicitly send the welcome email for the founding owner.
🤖 Fix all issues with AI agents
In `@packages/auth/src/lib/accept-invitation-endpoint.ts`:
- Around line 136-148: The call to auth.api.addMember is unprotected, so if it
throws the user remains logged-in but not actually added to the organization;
wrap the await import("../server") / auth.api.addMember invocation in a
try/catch inside accept-invitation-endpoint.ts, catch errors from
auth.api.addMember, log contextual information (invitation.id,
invitation.organization.id, user.id) and then either rollback the session/cookie
you set earlier (clear session and delete the auth cookie) before returning a
handled error response or re-throw a wrapped error with that context so the
caller can present a meaningful message; ensure you reference auth.api.addMember
and the session/cookie teardown logic currently run earlier in this handler when
implementing the rollback.
| // Dynamic import: this plugin needs to call the organization plugin's | ||
| // addMember API to trigger billing hooks (beforeAddMember/afterAddMember). | ||
| // server.ts imports this file as a plugin, so a static import would be circular. | ||
| // The import resolves at request time when all modules are fully initialized. | ||
| const { auth } = await import("../server"); | ||
| await auth.api.addMember({ | ||
| body: { | ||
| organizationId: invitation.organization.id, | ||
| userId: user.id, | ||
| role: | ||
| (invitation.role as "member" | "owner" | "admin") ?? "member", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
No error handling around auth.api.addMember — unhandled throw leaves partial state.
If addMember fails, the session and cookie are already set (lines 92-119) and the user lands in a broken state: logged in, session points to the org, but they aren't a member. Consider wrapping the call and returning a meaningful error, or at minimum logging context before re-throwing.
🤖 Prompt for AI Agents
In `@packages/auth/src/lib/accept-invitation-endpoint.ts` around lines 136 - 148,
The call to auth.api.addMember is unprotected, so if it throws the user remains
logged-in but not actually added to the organization; wrap the await
import("../server") / auth.api.addMember invocation in a try/catch inside
accept-invitation-endpoint.ts, catch errors from auth.api.addMember, log
contextual information (invitation.id, invitation.organization.id, user.id) and
then either rollback the session/cookie you set earlier (clear session and
delete the auth cookie) before returning a handled error response or re-throw a
wrapped error with that context so the caller can present a meaningful message;
ensure you reference auth.api.addMember and the session/cookie teardown logic
currently run earlier in this handler when implementing the rollback.
1 participant
Summary
db.insert(members), bypassingbeforeAddMember(free plan seat limit) andafterAddMember(Stripe seat quantity update + notification emails)addMembermutation — replaced withctx.auth.api.addMember(), matching the pattern already used byremoveMember,leaveOrganization, andupdateMemberRoleauth.api.addMember()via dynamic import to avoid circular dependency (server.tsimports this file)Seat quantities will self-correct on next member add/remove (the
afterAddMemberhook does a full member count, not an increment). Plan to email affected org owners ahead of time.Test plan
bun run typecheck— 17/17 passbun run lint:fix— cleanbun test— 1205 pass, 0 failSummary by CodeRabbit