Merge pull request #1163 from amanosiadnan-cmyk/fix-api-hardening

Author: Caleb

client/app/api/admin/settings/__tests__/route.test.ts

@@ -1,29 +1,57 @@
 import { describe, it, expect, vi, beforeEach } from "vitest"
-import { PUT } from "../route"
+import { PUT, GET } from "../route"
 import { requireAuth, requireRole } from "@/lib/api/auth"
 import { NextRequest } from "next/server"
+import { ApiErrors } from "@/lib/api/errors"
+import {
+  getAdminSettings,
+  updateAdminSettings,
+} from "@/lib/admin-settings-store"
+import { emitAuditEvent } from "@/lib/api/audit"
+
+vi.mock("@/lib/supabase/server", () => ({
+  createClient: vi.fn(),
+}))
 
 vi.mock("@/lib/api/auth", () => ({
   requireAuth: vi.fn(),
   requireRole: vi.fn(),
   createRequestContext: vi.fn().mockReturnValue({ requestId: "test-admin-id" }),
 }))
 
+vi.mock("@/lib/admin-settings-store", () => ({
+  getAdminSettings: vi.fn(),
+  updateAdminSettings: vi.fn(),
+  resetAdminSettingsStore: vi.fn(),
+  getCachedAdminSettings: vi.fn(),
+}))
+
+vi.mock("@/lib/api/audit", () => ({
+  emitAuditEvent: vi.fn(),
+}))
+
 describe("Admin Settings API Route", () => {
   const mockOwner = { id: "user_owner_123", email: "owner@example.com" }
 
   beforeEach(() => {
     vi.clearAllMocks()
     vi.mocked(requireAuth).mockResolvedValue(mockOwner as any)
     vi.mocked(requireRole).mockResolvedValue(true as any)
+    vi.mocked(getAdminSettings).mockResolvedValue({
+      maintenanceMode: false,
+      enableRegistration: true,
+      rateLimitThreshold: 100,
+    })
   })
 
-  it("should update settings successfully with valid parameters", async () => {
+  it("persists settings and returns the saved state", async () => {
     const validBody = {
       maintenanceMode: true,
       enableRegistration: false,
       rateLimitThreshold: 100,
     }
+    const saved = { ...validBody }
+    vi.mocked(updateAdminSettings).mockResolvedValue(saved)
 
     const request = new NextRequest("http://localhost/api/admin/settings", {
       method: "PUT",
@@ -36,13 +64,18 @@ describe("Admin Settings API Route", () => {
     expect(response.status).toBe(200)
     expect(body.success).toBe(true)
     expect(body.data.updated).toBe(true)
-    expect(body.data.settings).toEqual(validBody)
+    expect(body.data.settings).toEqual(saved)
+    expect(updateAdminSettings).toHaveBeenCalledWith(validBody)
   })
 
-  it("should support partial settings updates", async () => {
-    const partialBody = {
+  it("returns merged saved state for partial updates, not only the submitted body", async () => {
+    const partialBody = { maintenanceMode: false }
+    const saved = {
       maintenanceMode: false,
+      enableRegistration: true,
+      rateLimitThreshold: 100,
     }
+    vi.mocked(updateAdminSettings).mockResolvedValue(saved)
 
     const request = new NextRequest("http://localhost/api/admin/settings", {
       method: "PUT",
@@ -53,63 +86,129 @@ describe("Admin Settings API Route", () => {
     const body = await response.json()
 
     expect(response.status).toBe(200)
-    expect(body.success).toBe(true)
-    expect(body.data.settings).toEqual(partialBody)
+    expect(body.data.settings).toEqual(saved)
+    expect(body.data.settings).not.toEqual(partialBody)
   })
 
-  it("should reject setting a negative rateLimitThreshold", async () => {
-    const invalidBody = {
-      rateLimitThreshold: -10,
-    }
+  it("emits an audit event for privileged settings changes", async () => {
+    const validBody = { maintenanceMode: true }
+    vi.mocked(updateAdminSettings).mockResolvedValue({
+      maintenanceMode: true,
+      enableRegistration: true,
+      rateLimitThreshold: 100,
+    })
+
+    const request = new NextRequest("http://localhost/api/admin/settings", {
+      method: "PUT",
+      body: JSON.stringify(validBody),
+    })
+
+    await PUT(request)
+
+    expect(emitAuditEvent).toHaveBeenCalledWith(
+      expect.objectContaining({
+        userId: mockOwner.id,
+        action: "admin.settings_update",
+        resourceType: "admin_settings",
+        metadata: expect.objectContaining({
+          route: "/api/admin/settings",
+          requestId: "test-admin-id",
+          changedFields: "maintenanceMode",
+          maintenanceMode: true,
+        }),
+      })
+    )
+  })
+
+  it("rejects unauthenticated users", async () => {
+    vi.mocked(requireAuth).mockRejectedValue(ApiErrors.unauthorized())
+
+    const request = new NextRequest("http://localhost/api/admin/settings", {
+      method: "PUT",
+      body: JSON.stringify({ maintenanceMode: true }),
+    })
+
+    const response = await PUT(request)
+    const body = await response.json()
+
+    expect(response.status).toBe(401)
+    expect(body.error.code).toBe("UNAUTHORIZED")
+    expect(updateAdminSettings).not.toHaveBeenCalled()
+  })
+
+  it("rejects non-owner users", async () => {
+    vi.mocked(requireRole).mockRejectedValue(
+      ApiErrors.forbidden("Requires one of: owner")
+    )
+
+    const request = new NextRequest("http://localhost/api/admin/settings", {
+      method: "PUT",
+      body: JSON.stringify({ maintenanceMode: true }),
+    })
+
+    const response = await PUT(request)
+    const body = await response.json()
+
+    expect(response.status).toBe(403)
+    expect(body.error.code).toBe("FORBIDDEN")
+    expect(updateAdminSettings).not.toHaveBeenCalled()
+  })
 
+  it("should reject setting a negative rateLimitThreshold", async () => {
     const request = new NextRequest("http://localhost/api/admin/settings", {
       method: "PUT",
-      body: JSON.stringify(invalidBody),
+      body: JSON.stringify({ rateLimitThreshold: -10 }),
     })
 
     const response = await PUT(request)
     const body = await response.json()
 
     expect(response.status).toBe(400)
-    expect(body.success).toBe(false)
     expect(body.error.code).toBe("VALIDATION_ERROR")
     expect(body.error.field).toBe("rateLimitThreshold")
   })
 
   it("should reject setting rateLimitThreshold to 0", async () => {
-    const invalidBody = {
-      rateLimitThreshold: 0,
-    }
-
     const request = new NextRequest("http://localhost/api/admin/settings", {
       method: "PUT",
-      body: JSON.stringify(invalidBody),
+      body: JSON.stringify({ rateLimitThreshold: 0 }),
     })
 
     const response = await PUT(request)
     const body = await response.json()
 
     expect(response.status).toBe(400)
-    expect(body.success).toBe(false)
     expect(body.error.code).toBe("VALIDATION_ERROR")
   })
 
   it("should reject invalid data types", async () => {
-    const invalidBody = {
-      maintenanceMode: "yes", // should be boolean
-    }
-
     const request = new NextRequest("http://localhost/api/admin/settings", {
       method: "PUT",
-      body: JSON.stringify(invalidBody),
+      body: JSON.stringify({ maintenanceMode: "yes" }),
     })
 
     const response = await PUT(request)
     const body = await response.json()
 
     expect(response.status).toBe(400)
-    expect(body.success).toBe(false)
     expect(body.error.code).toBe("VALIDATION_ERROR")
     expect(body.error.field).toBe("maintenanceMode")
   })
+
+  it("GET returns persisted settings for owners", async () => {
+    const settings = {
+      maintenanceMode: true,
+      enableRegistration: false,
+      rateLimitThreshold: 50,
+    }
+    vi.mocked(getAdminSettings).mockResolvedValue(settings)
+
+    const response = await GET(
+      new NextRequest("http://localhost/api/admin/settings")
+    )
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.data.settings).toEqual(settings)
+  })
 })

client/app/api/admin/settings/route.ts

@@ -1,20 +1,81 @@
-import { NextRequest } from 'next/server'
-import { createApiRoute, createSuccessResponse, validateRequestBody } from '@/lib/api'
-import { z } from 'zod'
+import { type NextRequest } from "next/server"
+import {
+  createApiRoute,
+  createSuccessResponse,
+  validateRequestBody,
+  emitAuditEvent,
+  ApiErrors,
+} from "@/lib/api/index"
+import { updateAdminSettings, getAdminSettings } from "@/lib/admin-settings-store"
+import { z } from "zod"
 
 const adminSettingsSchema = z.object({
   maintenanceMode: z.boolean().optional(),
   enableRegistration: z.boolean().optional(),
   rateLimitThreshold: z.number().int().positive().optional(),
 })
 
+export const GET = createApiRoute(
+  async (_request: NextRequest, context) => {
+    const settings = await getAdminSettings()
+    return createSuccessResponse({ settings }, undefined, context.requestId)
+  },
+  {
+    requireAuth: true,
+    requireRole: ["owner"],
+  }
+)
+
 export const PUT = createApiRoute(
-  async (request: NextRequest) => {
+  async (request: NextRequest, context, user) => {
+    if (!user) {
+      throw ApiErrors.unauthorized()
+    }
+
     const body = await validateRequestBody(request, adminSettingsSchema)
-    return createSuccessResponse({ updated: true, settings: body })
+    const changedFields = Object.keys(body).filter(
+      (key) => body[key as keyof typeof body] !== undefined
+    )
+
+    if (changedFields.length === 0) {
+      throw ApiErrors.validationError("At least one settings field is required")
+    }
+
+    let settings
+    try {
+      settings = await updateAdminSettings(body)
+    } catch (error) {
+      throw ApiErrors.internalError(
+        error instanceof Error ? error.message : "Failed to persist admin settings"
+      )
+    }
+
+    emitAuditEvent({
+      userId: user.id,
+      action: "admin.settings_update",
+      resourceType: "admin_settings",
+      resourceId: "platform",
+      metadata: {
+        route: "/api/admin/settings",
+        requestId: context.requestId,
+        changedFields: changedFields.join(","),
+        ...Object.fromEntries(
+          changedFields.map((field) => [
+            field,
+            body[field as keyof typeof body] as string | number | boolean,
+          ])
+        ),
+      },
+    })
+
+    return createSuccessResponse(
+      { updated: true, settings },
+      undefined,
+      context.requestId
+    )
   },
   {
     requireAuth: true,
-    requireRole: ['owner'],
+    requireRole: ["owner"],
   }
 )

client/app/api/admin/users/__tests__/route.test.ts

@@ -0,0 +1,79 @@
+import { describe, it, expect, vi, beforeEach } from "vitest"
+import { GET } from "../route"
+import { requireAuth, requireRole } from "@/lib/api/auth"
+import { NextRequest } from "next/server"
+import { ApiErrors } from "@/lib/api/errors"
+import { emitAuditEvent } from "@/lib/api/audit"
+
+vi.mock("@/lib/supabase/server", () => ({
+  createClient: vi.fn(),
+}))
+
+vi.mock("@/lib/api/auth", () => ({
+  requireAuth: vi.fn(),
+  requireRole: vi.fn(),
+  createRequestContext: vi.fn().mockReturnValue({ requestId: "admin-users-req" }),
+}))
+
+vi.mock("@/lib/api/audit", () => ({
+  emitAuditEvent: vi.fn(),
+}))
+
+describe("Admin Users API Route", () => {
+  const mockAdmin = { id: "admin_1", email: "admin@example.com" }
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+    vi.mocked(requireAuth).mockResolvedValue(mockAdmin as any)
+    vi.mocked(requireRole).mockResolvedValue(true as any)
+  })
+
+  it("emits an audit event on successful privileged user-list access", async () => {
+    const response = await GET(
+      new NextRequest("http://localhost/api/admin/users")
+    )
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.success).toBe(true)
+    expect(emitAuditEvent).toHaveBeenCalledWith(
+      expect.objectContaining({
+        userId: mockAdmin.id,
+        action: "admin.users_list",
+        resourceType: "admin_users",
+        metadata: expect.objectContaining({
+          route: "/api/admin/users",
+          requestId: "admin-users-req",
+        }),
+      })
+    )
+  })
+
+  it("rejects unauthenticated users without emitting audit", async () => {
+    vi.mocked(requireAuth).mockRejectedValue(ApiErrors.unauthorized())
+
+    const response = await GET(
+      new NextRequest("http://localhost/api/admin/users")
+    )
+    const body = await response.json()
+
+    expect(response.status).toBe(401)
+    expect(body.error.code).toBe("UNAUTHORIZED")
+    expect(emitAuditEvent).not.toHaveBeenCalled()
+  })
+
+  it("rejects forbidden users without emitting audit", async () => {
+    vi.mocked(requireRole).mockRejectedValue(
+      ApiErrors.forbidden("Requires one of: admin, owner")
+    )
+
+    const response = await GET(
+      new NextRequest("http://localhost/api/admin/users")
+    )
+    const body = await response.json()
+
+    expect(response.status).toBe(403)
+    expect(body.error.code).toBe("FORBIDDEN")
+    expect(emitAuditEvent).not.toHaveBeenCalled()
+  })
+})