fix(auth): don't block existing OAuth users when ALLOW_REGISTRATION=false

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,20 @@ 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))) {
+    // 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', {
+      providerName,
+      inviteId,
+    });
+  }
+
   const user = await db.user.create({
     data: {
       email: oauthUser.email,

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';

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);
+  });
+});

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';
+}

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) {