refactor(auth): replace admin plugin with ban guard (#2303)

2026-08-16 16:16:36 +08:00
parent 88625a8d84
commit c25791a357
15 changed files with 3969 additions and 70 deletions
@@ -0,0 +1,2 @@
+ALTER TABLE "session" DROP COLUMN "impersonated_by";--> statement-breakpoint
+ALTER TABLE "user" DROP COLUMN "role";
@@ -148,6 +148,13 @@
      "when": 1786787455390,
      "tag": "0020_smart_war_machine",
      "breakpoints": true
+    },
+    {
+      "idx": 21,
+      "version": "7",
+      "when": 1786864179375,
+      "tag": "0021_chilly_starjammers",
+      "breakpoints": true
    }
  ]
 }
@@ -76,7 +76,6 @@ const EnvSchema = object({
  TEST_AUTH_USER_ID: optional(pipe(string(), nonEmpty('TEST_AUTH_USER_ID must not be empty when set')), 'test-user'),
  TEST_AUTH_USER_EMAIL: optional(pipe(string(), nonEmpty('TEST_AUTH_USER_EMAIL must not be empty when set')), 'test@example.com'),
  TEST_AUTH_USER_NAME: optional(pipe(string(), nonEmpty('TEST_AUTH_USER_NAME must not be empty when set')), 'Test User'),
-  TEST_AUTH_USER_ROLE: optional(string(), ''),

  STRIPE_SECRET_KEY: optional(string()),
  STRIPE_WEBHOOK_SECRET: optional(string()),
@@ -18,7 +18,6 @@ interface RequestAuthEnv {
  TEST_AUTH_USER_ID: string
  TEST_AUTH_USER_EMAIL: string
  TEST_AUTH_USER_NAME: string
-  TEST_AUTH_USER_ROLE: string
 }

 interface TokenIssuerEnv {
@@ -49,8 +48,6 @@ function resolveTestAuthToken(env: RequestAuthEnv, accessToken: string): AuthSes

  const now = new Date()
  const expiresAt = new Date(now.getTime() + 60 * 60 * 1000)
-  const role = env.TEST_AUTH_USER_ROLE.trim()
-
  return {
    user: {
      id: env.TEST_AUTH_USER_ID,
@@ -58,7 +55,6 @@ function resolveTestAuthToken(env: RequestAuthEnv, accessToken: string): AuthSes
      name: env.TEST_AUTH_USER_NAME,
      emailVerified: true,
      image: null,
-      role: role || null,
      banned: false,
      banReason: null,
      banExpires: null,
@@ -47,7 +47,6 @@ describe('parseEnv', () => {
    expect(env.TEST_AUTH_USER_ID).toBe('test-user')
    expect(env.TEST_AUTH_USER_EMAIL).toBe('test@example.com')
    expect(env.TEST_AUTH_USER_NAME).toBe('Test User')
-    expect(env.TEST_AUTH_USER_ROLE).toBe('')
  })

  it('parses TEST_AUTH_TOKEN virtual user overrides', () => {
@@ -57,13 +56,11 @@ describe('parseEnv', () => {
      TEST_AUTH_USER_ID: 'admin-user',
      TEST_AUTH_USER_EMAIL: 'admin@example.com',
      TEST_AUTH_USER_NAME: 'Admin User',
-      TEST_AUTH_USER_ROLE: 'admin',
    })

    expect(env.TEST_AUTH_USER_ID).toBe('admin-user')
    expect(env.TEST_AUTH_USER_EMAIL).toBe('admin@example.com')
    expect(env.TEST_AUTH_USER_NAME).toBe('Admin User')
-    expect(env.TEST_AUTH_USER_ROLE).toBe('admin')
  })

  it('lLM_ROUTER_MASTER_KEY decodes a valid 32-byte base64 value into a Buffer', () => {
@@ -20,7 +20,6 @@ const mockEnv = {
  TEST_AUTH_USER_ID: 'test-user',
  TEST_AUTH_USER_EMAIL: 'test@example.com',
  TEST_AUTH_USER_NAME: 'Test User',
-  TEST_AUTH_USER_ROLE: '',
 } as const

 function createUser(overrides: Partial<RequestAuthSession['user']> = {}): RequestAuthSession['user'] {
@@ -31,7 +30,6 @@ function createUser(overrides: Partial<RequestAuthSession['user']> = {}): Reques
    name: 'User',
    emailVerified: true,
    image: null,
-    role: null,
    banned: false,
    banReason: null,
    banExpires: null,
@@ -147,14 +145,12 @@ describe('resolveRequestAuth', () => {
        TEST_AUTH_USER_ID: 'test-user-1',
        TEST_AUTH_USER_EMAIL: 'Test@Example.com',
        TEST_AUTH_USER_NAME: 'Local Test User',
-        TEST_AUTH_USER_ROLE: 'admin',
      },
      new Headers({ Authorization: 'Bearer test-secret' }),
    )

    expect(result?.user.id).toBe('test-user-1')
    expect(result?.user.email).toBe('test@example.com')
-    expect(result?.user.role).toBe('admin')
    expect(mockedJwtVerify).not.toHaveBeenCalled()
  })

@@ -17,16 +17,17 @@ import { betterAuth } from 'better-auth'
 import { drizzleAdapter } from 'better-auth/adapters/drizzle'
 import { createAuthMiddleware } from 'better-auth/api'
 import { deleteSessionCookie } from 'better-auth/cookies'
-import { admin, bearer, jwt, magicLink } from 'better-auth/plugins'
+import { bearer, jwt, magicLink } from 'better-auth/plugins'
 import { eq } from 'drizzle-orm'

 import * as authSchema from '@proj-airi/auth-shared'

 import { ApiError } from './error'
-import { oidcJwtBearer } from './oidc-jwt-bearer'
 import { getAuthTrustedOrigins, getTrustedOrigin } from './origin'
+import { banGuard } from './plugins/ban-guard'
+import { oidcJwtBearer } from './plugins/oidc-jwt-bearer'
+import { steam } from './plugins/steam'
 import { createAppleClientSecret, createSocialAuthorizationRevoker } from './social-authorization'
-import { steam } from './steam'

 const logger = useLogger('auth').useGlobalConfig()

@@ -445,36 +446,16 @@ export function createAuth(
      },
    }),

-    // NOTICE: Keep the admin plugin for its role and ban data contract. Disable
-    // all of its HTTP endpoints here, while routes.ts blocks the full namespace.
+    // Keep Better Auth's built-in token route disabled. oauthProvider owns the
+    // public OIDC token endpoint at /oauth2/token.
    disabledPaths: [
      '/token',
-      '/admin/ban-user',
-      '/admin/create-user',
-      '/admin/get-user',
-      '/admin/has-permission',
-      '/admin/impersonate-user',
-      '/admin/list-user-sessions',
-      '/admin/list-users',
-      '/admin/remove-user',
-      '/admin/revoke-user-session',
-      '/admin/revoke-user-sessions',
-      '/admin/set-role',
-      '/admin/set-user-password',
-      '/admin/stop-impersonating',
-      '/admin/unban-user',
-      '/admin/update-user',
    ],

    plugins: [
      bearer(),
      jwt(),
-      // Role-based admin: adds `user.role/banned/banReason/banExpires` and
-      // `session.impersonatedBy`, gates /admin/* by `role === 'admin'`, and
-      // blocks banned users at `session.create.before`. The stateless OIDC JWT
-      // hot path is NOT covered by that hook, so `resolveRequestAuth` and the
-      // /oauth2/userinfo guard re-check `user.banned` themselves.
-      admin({ adminRoles: ['admin'] }),
+      banGuard(),
      // NOTICE:
      // Bridges OIDC JWT access tokens (RS256, signed by our oauthProvider)
      // into a real better-auth session so `sessionMiddleware` and every
@@ -775,10 +756,9 @@ export function createAuth(
        },
        update: {
          // NOTICE:
-          // Revoke OAuth credentials when a user gets banned. The admin plugin's
-          // `banUser` sets `banned=true` via internalAdapter.updateUser (firing
-          // this hook) and deletes sessions, but leaves oauth_refresh_token /
-          // oauth_access_token rows. oauthProvider's /oauth2/token refresh grant
+          // Revoke OAuth credentials when a user gets banned. The Auth-owned ban
+          // operation must update the user through Better Auth's adapter so this
+          // hook fires. oauthProvider's /oauth2/token refresh grant
          // (node_modules/@better-auth/oauth-provider/dist/index.mjs L718) loads
          // the user without checking `banned`, so a banned user could otherwise
          // mint a fresh access token from a live refresh token. That token is
@@ -796,9 +776,8 @@ export function createAuth(
      },
      session: {
        create: {
-          // NOTE: login-time ban enforcement is the admin plugin's
-          // `session.create.before` (checks `user.banned`). We only keep the
-          // `after` hook for last-seen / analytics.
+          // banGuard checks the user before Better Auth creates this session.
+          // This hook records last-seen activity and login analytics after it.
          after: async (session) => {
            metrics?.userLogin.add(1)
            // Best-effort analytics: session creation must not fail because
@@ -0,0 +1,70 @@
+import type { BetterAuthPlugin } from 'better-auth'
+
+import { isUserBannedNow } from '@proj-airi/auth-shared'
+import { APIError } from 'better-auth'
+
+interface BanState {
+  banned?: boolean | null
+  banExpires?: Date | string | null
+}
+
+/**
+ * Rejects new Better Auth sessions for users with an active account ban.
+ *
+ * The private management backend owns ban and unban authorization. This plugin
+ * only applies the persisted ban state when Better Auth creates a session. It
+ * does not clear expired bans because a concurrent management request can
+ * renew a ban after the session hook reads it.
+ */
+export function banGuard(): BetterAuthPlugin {
+  return {
+    id: 'ban-guard',
+    schema: {
+      user: {
+        fields: {
+          banned: {
+            type: 'boolean',
+            defaultValue: false,
+            required: false,
+            input: false,
+          },
+          banReason: {
+            type: 'string',
+            required: false,
+            input: false,
+          },
+          banExpires: {
+            type: 'date',
+            required: false,
+            input: false,
+          },
+        },
+      },
+    },
+    init() {
+      return {
+        options: {
+          databaseHooks: {
+            session: {
+              create: {
+                async before(session, context) {
+                  if (!context)
+                    return
+
+                  const user = await context.context.internalAdapter.findUserById(session.userId) as BanState | null
+                  if (!isUserBannedNow(user ?? {}))
+                    return
+
+                  throw APIError.from('FORBIDDEN', {
+                    code: 'BANNED_USER',
+                    message: 'This account has been banned',
+                  })
+                },
+              },
+            },
+          },
+        },
+      }
+    },
+  }
+}
@@ -1,7 +1,7 @@
 import type { BetterAuthPlugin } from 'better-auth'
 import type { JSONWebKeySet } from 'jose'

-import type { AuthEnv } from './env'
+import type { AuthEnv } from '../env'

 import { createHmac } from 'node:crypto'

@@ -92,6 +92,7 @@ export function steam() {
        headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
        body: verifyParams.toString(),
        responseType: 'text',
+        timeout: 10_000,
      })
      return body.split('\n').some(line => line.trim() === 'is_valid:true')
    }
@@ -0,0 +1,150 @@
+import type { BetterAuthOptions } from 'better-auth'
+
+import { describe, expect, it, vi } from 'vitest'
+
+import { banGuard } from '../plugins/ban-guard'
+
+type SessionCreateHook = NonNullable<NonNullable<NonNullable<BetterAuthOptions['databaseHooks']>['session']>['create']>['before']
+type BanSessionCreateHook = NonNullable<SessionCreateHook>
+type BanSession = Parameters<BanSessionCreateHook>[0]
+type BanContext = Parameters<BanSessionCreateHook>[1]
+
+async function getSessionCreateHook(): Promise<BanSessionCreateHook> {
+  const initialized = await banGuard().init?.({} as never)
+  const before = initialized?.options?.databaseHooks?.session?.create?.before
+  if (!before)
+    throw new TypeError('Expected the ban guard to register a session-create hook')
+  return before
+}
+
+function createContext(user: { banned: boolean, banExpires: Date | null }) {
+  const updateUser = vi.fn()
+  return {
+    updateUser,
+    context: {
+      context: {
+        internalAdapter: {
+          findUserById: vi.fn(async () => user),
+          updateUser,
+        },
+      },
+    } as unknown as BanContext,
+  }
+}
+
+function createSession(userId: string): BanSession {
+  const now = new Date()
+  return {
+    id: 'session-1',
+    token: 'session-token',
+    userId,
+    expiresAt: new Date(now.getTime() + 60 * 60 * 1000),
+    createdAt: now,
+    updatedAt: now,
+  }
+}
+
+describe('banGuard', () => {
+  // Review: https://github.com/moeru-ai/airi/pull/2303
+  // ROOT CAUSE:
+  //
+  // Better Auth generates the shared schema from registered plugin schemas.
+  // The removed admin plugin declared the ban fields.
+  // Without this declaration, a later generated migration can remove them.
+  //
+  // The ban guard now owns this schema contract.
+  it('keeps ban fields in the generated schema', () => {
+    expect(banGuard().schema).toMatchObject({
+      user: {
+        fields: {
+          banned: {
+            defaultValue: false,
+            input: false,
+            required: false,
+            type: 'boolean',
+          },
+          banReason: {
+            input: false,
+            required: false,
+            type: 'string',
+          },
+          banExpires: {
+            input: false,
+            required: false,
+            type: 'date',
+          },
+        },
+      },
+    })
+  })
+
+  it('allows a session for an account that is not banned', async () => {
+    const before = await getSessionCreateHook()
+    const { context, updateUser } = createContext({ banned: false, banExpires: null })
+
+    await expect(before(
+      createSession('user-1'),
+      context,
+    )).resolves.toBeUndefined()
+
+    expect(updateUser).not.toHaveBeenCalled()
+  })
+
+  it('rejects a session for an account with a permanent ban', async () => {
+    const before = await getSessionCreateHook()
+    const { context, updateUser } = createContext({ banned: true, banExpires: null })
+
+    await expect(before(
+      createSession('user-1'),
+      context,
+    )).rejects.toMatchObject({
+      body: {
+        code: 'BANNED_USER',
+      },
+    })
+
+    expect(updateUser).not.toHaveBeenCalled()
+  })
+
+  // Review: https://github.com/moeru-ai/airi/pull/2303
+  // ROOT CAUSE:
+  //
+  // The old cleanup read an expired ban, then cleared it in a later write.
+  // A management request can renew the ban before the cleanup write.
+  // The stale write can then clear the renewed ban.
+  //
+  // The guard now reads the active-ban rule without writing ban state.
+  it('allows an expired temporary ban without changing persisted ban state', async () => {
+    const before = await getSessionCreateHook()
+    const { context, updateUser } = createContext({
+      banned: true,
+      banExpires: new Date(Date.now() - 1000),
+    })
+
+    await expect(before(
+      createSession('user-1'),
+      context,
+    )).resolves.toBeUndefined()
+
+    expect(updateUser).not.toHaveBeenCalled()
+  })
+
+  it('rejects a session for an account with an active temporary ban', async () => {
+    const before = await getSessionCreateHook()
+    const { context, updateUser } = createContext({
+      banned: true,
+      banExpires: new Date(Date.now() + 60 * 1000),
+    })
+
+    await expect(before(
+      createSession('user-1'),
+      context,
+    )).rejects.toMatchObject({
+      body: {
+        code: 'BANNED_USER',
+      },
+    })
+
+    expect(updateUser).not.toHaveBeenCalled()
+  })
+})
@@ -4,9 +4,17 @@ import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'

 import * as schema from '@proj-airi/auth-shared'

-import { steam } from '../steam'
+import { steam } from '../plugins/steam'
 import { createTestDatabase } from './mock-db'

+const { ofetchMock } = vi.hoisted(() => ({
+  ofetchMock: vi.fn(),
+}))
+
+vi.mock('ofetch', () => ({
+  ofetch: ofetchMock,
+}))
+
 /** Test fixture: arbitrary valid-format SteamID64 used in fake OpenID callbacks. */
 const STEAM_ID = '76561198012345678'

@@ -74,21 +82,15 @@ describe('steam auth plugin', () => {
    auth = await createTestAuth()
  })

-  afterEach(() => vi.unstubAllGlobals())
+  afterEach(() => ofetchMock.mockReset())

  // NOTICE:
-  // We mock the module-global `fetch` for Steam's `check_authentication`
-  // dumb-mode verification POST instead of hitting the real
-  // steamcommunity.com endpoint, keeping this test hermetic and fast.
+  // We mock `ofetch` at the external Steam boundary instead of hitting the
+  // real steamcommunity.com endpoint, keeping this test hermetic and fast.
  // Root cause of picking dumb mode over signature verification: see the
  // plugin's own doc comment in ./steam.ts.
  function mockSteamVerification(isValid: boolean) {
-    vi.stubGlobal('fetch', vi.fn(async (url: string | URL) => {
-      if (url.toString() === 'https://steamcommunity.com/openid/login') {
-        return new Response(`ns:http://specs.openid.net/auth/2.0\nis_valid:${isValid}`, { status: 200 })
-      }
-      throw new Error(`Unexpected fetch to ${url}`)
-    }))
+    ofetchMock.mockResolvedValue(`ns:http://specs.openid.net/auth/2.0\nis_valid:${isValid}`)
  }

  it('redirects to the Steam OpenID login URL on sign-in start', async () => {
@@ -173,6 +175,31 @@ describe('steam auth plugin', () => {
    expect(callbackResponse.headers.get('location')).toContain('error=steam_openid_verification_failed')
  })

+  it('sets a 10-second timeout for Steam callback verification', async () => {
+    mockSteamVerification(true)
+
+    const { response: startResponse, headers: startHeaders } = await auth.api.signInSteam({
+      body: { callbackURL: 'http://localhost/ui/profile' },
+      returnHeaders: true,
+    })
+    const returnToState = new URL(new URL(startResponse.url).searchParams.get('openid.return_to')!).searchParams.get('state')!
+
+    await auth.handler(
+      new Request(`http://localhost/api/auth/steam/callback?${buildCallbackQuery(returnToState)}`, {
+        headers: { cookie: forwardableCookieHeader(startHeaders) },
+      }),
+    )
+
+    expect(ofetchMock).toHaveBeenCalledWith(
+      'https://steamcommunity.com/openid/login',
+      expect.objectContaining({
+        method: 'POST',
+        responseType: 'text',
+        timeout: 10_000,
+      }),
+    )
+  })
+
  it('links a second Steam account to the already-signed-in user instead of creating a new one', async () => {
    mockSteamVerification(true)
    const context = await auth.$context