From d26c0a7af56c3a962fa16dfee8f31e863300ee4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 18 Aug 2026 00:06:14 +0200 Subject: [PATCH 1/2] fix(auth): don't block existing OAuth users when registration is disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `signInOAuth` ran `getIsRegistrationAllowed()` before the IdP redirect, where the server has no identity for the caller yet. With ALLOW_REGISTRATION=false that rejected every OAuth sign-in equally — including users who already have a User + Account row — so a locked-down self-hosted install locked out its whole existing user base as sessions expired. Those users have no recovery path either: signInEmail and requestResetPassword both look up provider='email' only, which OAuth-only users don't have. Move the check to the OAuth callback's `handleNewUser`, the first point where we know the account doesn't exist. Existing users complete OAuth normally; new users without a valid invite are redirected to /login with the error and no User row is created. `getIsRegistrationAllowed` moves to @openpanel/db so both the tRPC router and the API controller can share one implementation. Policy semantics are unchanged — first-user bootstrap, ALLOW_INVITATION and ALLOW_REGISTRATION all behave exactly as before, and signUpEmail keeps its upfront check since that request explicitly declares a sign-up. Fixes #363 Claude-Session: https://claude.ai/code/session_01ETRr6KYLYATzBVaLRZcwwk --- .../controllers/oauth-callback.controller.tsx | 18 ++++- packages/db/index.ts | 1 + .../src/services/registration.service.test.ts | 80 +++++++++++++++++++ .../db/src/services/registration.service.ts | 41 ++++++++++ packages/trpc/src/routers/auth.ts | 46 ++--------- 5 files changed, 145 insertions(+), 41 deletions(-) create mode 100644 packages/db/src/services/registration.service.test.ts create mode 100644 packages/db/src/services/registration.service.ts diff --git a/apps/api/src/controllers/oauth-callback.controller.tsx b/apps/api/src/controllers/oauth-callback.controller.tsx index 46d789150..7f1dd425c 100644 --- a/apps/api/src/controllers/oauth-callback.controller.tsx +++ b/apps/api/src/controllers/oauth-callback.controller.tsx @@ -8,7 +8,12 @@ import { setLastAuthProviderCookie, setSessionTokenCookie, } from '@openpanel/auth'; -import { type Account, connectUserToOrganization, db } from '@openpanel/db'; +import { + type Account, + connectUserToOrganization, + db, + getIsRegistrationAllowed, +} from '@openpanel/db'; import type { FastifyReply, FastifyRequest } from 'fastify'; import { z } from 'zod'; import { LogError } from '@/utils/errors'; @@ -132,6 +137,17 @@ async function handleNewUser({ ); } + // Enforce the self-hosting registration policy here rather than before the + // IdP redirect — this is the first point where we know the user is new, so + // returning users are never caught by it. + if (!(await getIsRegistrationAllowed(inviteId))) { + throw new LogError('Registrations are not allowed', { + oauthUser, + providerName, + inviteId, + }); + } + const user = await db.user.create({ data: { email: oauthUser.email, diff --git a/packages/db/index.ts b/packages/db/index.ts index 727ea5663..15c8ccea3 100644 --- a/packages/db/index.ts +++ b/packages/db/index.ts @@ -31,6 +31,7 @@ export * from './src/services/profile.service'; export * from './src/services/project.service'; export * from './src/services/reference.service'; export * from './src/services/referrer-spikes.service'; +export * from './src/services/registration.service'; export * from './src/services/reports.service'; export * from './src/services/retention.service'; export * from './src/services/salt.service'; diff --git a/packages/db/src/services/registration.service.test.ts b/packages/db/src/services/registration.service.test.ts new file mode 100644 index 000000000..2283ce4d2 --- /dev/null +++ b/packages/db/src/services/registration.service.test.ts @@ -0,0 +1,80 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockUserCount = vi.hoisted(() => vi.fn()); +const mockInviteFindUnique = vi.hoisted(() => vi.fn()); + +vi.mock('../prisma-client', () => ({ + db: { + user: { count: mockUserCount }, + invite: { findUnique: mockInviteFindUnique }, + }, +})); + +import { getIsRegistrationAllowed } from './registration.service'; + +const ORIGINAL_ENV = { ...process.env }; + +beforeEach(() => { + vi.clearAllMocks(); + // Not the first user unless a test says otherwise + mockUserCount.mockResolvedValue(5); + mockInviteFindUnique.mockResolvedValue(null); +}); + +afterEach(() => { + process.env = { ...ORIGINAL_ENV }; +}); + +describe('getIsRegistrationAllowed', () => { + it('allows everything in cloud (ALLOW_REGISTRATION unset)', async () => { + process.env.ALLOW_REGISTRATION = undefined; + delete process.env.ALLOW_REGISTRATION; + + await expect(getIsRegistrationAllowed()).resolves.toBe(true); + expect(mockUserCount).not.toHaveBeenCalled(); + }); + + it('allows the very first user even when registration is disabled', async () => { + process.env.ALLOW_REGISTRATION = 'false'; + mockUserCount.mockResolvedValue(0); + + await expect(getIsRegistrationAllowed()).resolves.toBe(true); + }); + + it('blocks a new user with no invite when registration is disabled', async () => { + process.env.ALLOW_REGISTRATION = 'false'; + + await expect(getIsRegistrationAllowed()).resolves.toBe(false); + }); + + it('allows a new user holding a valid invite when registration is disabled', async () => { + process.env.ALLOW_REGISTRATION = 'false'; + process.env.ALLOW_INVITATION = 'true'; + mockInviteFindUnique.mockResolvedValue({ id: 'invite-1' }); + + await expect(getIsRegistrationAllowed('invite-1')).resolves.toBe(true); + }); + + it('blocks an unknown invite id', async () => { + process.env.ALLOW_REGISTRATION = 'false'; + process.env.ALLOW_INVITATION = 'true'; + mockInviteFindUnique.mockResolvedValue(null); + + await expect(getIsRegistrationAllowed('nope')).resolves.toBe(false); + }); + + it('blocks a valid invite when invitations are disabled', async () => { + process.env.ALLOW_REGISTRATION = 'false'; + process.env.ALLOW_INVITATION = 'false'; + mockInviteFindUnique.mockResolvedValue({ id: 'invite-1' }); + + await expect(getIsRegistrationAllowed('invite-1')).resolves.toBe(false); + expect(mockInviteFindUnique).not.toHaveBeenCalled(); + }); + + it('allows open self-hosted registration', async () => { + process.env.ALLOW_REGISTRATION = 'true'; + + await expect(getIsRegistrationAllowed()).resolves.toBe(true); + }); +}); diff --git a/packages/db/src/services/registration.service.ts b/packages/db/src/services/registration.service.ts new file mode 100644 index 000000000..434254a0e --- /dev/null +++ b/packages/db/src/services/registration.service.ts @@ -0,0 +1,41 @@ +import { db } from '../prisma-client'; + +/** + * Whether a *new* user may be created right now. + * + * This must only be consulted at the point where we know the account does not + * exist yet. Calling it before an identity is known (e.g. when kicking off an + * OAuth redirect) would reject returning users too, since we cannot tell a + * sign-in from a sign-up at that stage. + */ +export async function getIsRegistrationAllowed(inviteId?: string | null) { + // ALLOW_REGISTRATION is always undefined in cloud + if (process.env.ALLOW_REGISTRATION === undefined) { + return true; + } + + // Self-hosting logic + // 1. First user is always allowed + const count = await db.user.count(); + if (count === 0) { + return true; + } + + // 2. If there is an invite, check if it is valid + if (inviteId) { + if (process.env.ALLOW_INVITATION === 'false') { + return false; + } + + const invite = await db.invite.findUnique({ + where: { + id: inviteId, + }, + }); + + return !!invite; + } + + // 3. Otherwise, check if general registration is allowed + return process.env.ALLOW_REGISTRATION !== 'false'; +} diff --git a/packages/trpc/src/routers/auth.ts b/packages/trpc/src/routers/auth.ts index 1442d7630..e765bfdbb 100644 --- a/packages/trpc/src/routers/auth.ts +++ b/packages/trpc/src/routers/auth.ts @@ -26,6 +26,7 @@ import { db, decrypt, encrypt, + getIsRegistrationAllowed, getShareOverviewById, getUserAccount, } from '@openpanel/db'; @@ -75,38 +76,6 @@ async function consumeInviteForUser( } } -async function getIsRegistrationAllowed(inviteId?: string | null) { - // ALLOW_REGISTRATION is always undefined in cloud - if (process.env.ALLOW_REGISTRATION === undefined) { - return true; - } - - // Self-hosting logic - // 1. First user is always allowed - const count = await db.user.count(); - if (count === 0) { - return true; - } - - // 2. If there is an invite, check if it is valid - if (inviteId) { - if (process.env.ALLOW_INVITATION === 'false') { - return false; - } - - const invite = await db.invite.findUnique({ - where: { - id: inviteId, - }, - }); - - return !!invite; - } - - // 3. Otherwise, check if general registration is allowed - return process.env.ALLOW_REGISTRATION !== 'false'; -} - export const authRouter = createTRPCRouter({ signOut: publicProcedure.mutation(async ({ ctx }) => { deleteSessionTokenCookie(ctx.setCookie); @@ -117,14 +86,11 @@ export const authRouter = createTRPCRouter({ signInOAuth: publicProcedure .input(z.object({ provider: zProvider, inviteId: z.string().nullish() })) .mutation(async ({ input, ctx }) => { - const isRegistrationAllowed = await getIsRegistrationAllowed( - input.inviteId - ); - - if (!isRegistrationAllowed) { - throw new TRPCAccessError('Registrations are not allowed'); - } - + // NOTE: no registration check here. At this point we have no identity for + // the caller — the IdP hasn't been hit yet — so we cannot tell a returning + // user from a new sign-up. Gating here locks out every existing OAuth user + // as soon as their session expires. The check lives in the OAuth callback + // (`handleNewUser`), which is the only place we know the user is new. const { provider } = input; if (input.inviteId) { From 26994cf92ef44bb57e7320be65eaae17f1657e30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesva=CC=88rd?= Date: Tue, 18 Aug 2026 08:08:28 +0200 Subject: [PATCH 2/2] fix(auth): don't log OAuth profile data when registration is denied From CodeRabbit review on #431. This branch rejects people who are not users, so their email, name and provider id shouldn't be written to application logs. Keep only `providerName` and `inviteId` (an opaque invite record id). Nothing is lost for support: `redirectWithError` already puts the request id in the redirect as `correlationId`, which is what ties a user's error page back to the log line. Claude-Session: https://claude.ai/code/session_01ETRr6KYLYATzBVaLRZcwwk --- apps/api/src/controllers/oauth-callback.controller.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/api/src/controllers/oauth-callback.controller.tsx b/apps/api/src/controllers/oauth-callback.controller.tsx index 7f1dd425c..ff311a352 100644 --- a/apps/api/src/controllers/oauth-callback.controller.tsx +++ b/apps/api/src/controllers/oauth-callback.controller.tsx @@ -141,8 +141,11 @@ async function handleNewUser({ // IdP redirect — this is the first point where we know the user is new, so // returning users are never caught by it. if (!(await getIsRegistrationAllowed(inviteId))) { + // Deliberately no `oauthUser` here — this rejects people who are not users, + // so their email and name shouldn't land in application logs. The redirect + // carries `correlationId` (the request id), which is what ties a user's + // error page back to this log line if an operator needs to investigate. throw new LogError('Registrations are not allowed', { - oauthUser, providerName, inviteId, });