-
Notifications
You must be signed in to change notification settings - Fork 6
feat: enhance account update logic with admin checks and authentication #388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: test
Are you sure you want to change the base?
Changes from all commits
6c230aa
0cf4a28
a38159d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { NextResponse } from "next/server"; | ||
|
|
||
| vi.mock("@/lib/networking/safeParseJson", () => ({ | ||
| safeParseJson: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/accounts/validateCreateAccountBody", () => ({ | ||
| validateCreateAccountBody: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/accounts/createAccountHandler", () => ({ | ||
| createAccountHandler: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/accounts/patchAccountHandler", () => ({ | ||
| patchAccountHandler: vi.fn(), | ||
| })); | ||
|
|
||
| const { POST, PATCH } = await import("@/app/api/accounts/route"); | ||
| const { safeParseJson } = await import("@/lib/networking/safeParseJson"); | ||
| const { validateCreateAccountBody } = await import("@/lib/accounts/validateCreateAccountBody"); | ||
| const { createAccountHandler } = await import("@/lib/accounts/createAccountHandler"); | ||
| const { patchAccountHandler } = await import("@/lib/accounts/patchAccountHandler"); | ||
|
|
||
| describe("POST /api/accounts", () => { | ||
| const req = {} as never; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns validation error when body invalid", async () => { | ||
| vi.mocked(safeParseJson).mockResolvedValue({}); | ||
| vi.mocked(validateCreateAccountBody).mockReturnValue( | ||
| NextResponse.json({ status: "error" }, { status: 400 }), | ||
| ); | ||
|
|
||
| const response = await POST(req); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(createAccountHandler).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("delegates to createAccountHandler when body valid", async () => { | ||
| const payload = { email: "a@b.com" }; | ||
| vi.mocked(safeParseJson).mockResolvedValue(payload); | ||
| vi.mocked(validateCreateAccountBody).mockReturnValue(payload as never); | ||
| vi.mocked(createAccountHandler).mockResolvedValue(NextResponse.json({ ok: true })); | ||
|
|
||
| await POST(req); | ||
|
|
||
| expect(createAccountHandler).toHaveBeenCalledWith(payload); | ||
| }); | ||
| }); | ||
|
|
||
| describe("PATCH /api/accounts", () => { | ||
| const req = {} as never; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("delegates to patchAccountHandler", async () => { | ||
| const expected = NextResponse.json({ data: {} }, { status: 200 }); | ||
| vi.mocked(patchAccountHandler).mockResolvedValue(expected); | ||
|
|
||
| const response = await PATCH(req); | ||
|
|
||
| expect(patchAccountHandler).toHaveBeenCalledWith(req); | ||
| expect(response).toBe(expected); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { NextResponse } from "next/server"; | ||
| import type { NextRequest } from "next/server"; | ||
| import { patchAccountHandler } from "@/lib/accounts/patchAccountHandler"; | ||
|
|
||
| vi.mock("@/lib/auth/validateAuthContext", () => ({ | ||
| validateAuthContext: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/networking/safeParseJson", () => ({ | ||
| safeParseJson: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/accounts/validateUpdateAccountBody", () => ({ | ||
| validateUpdateAccountBody: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/admins/checkIsAdmin", () => ({ | ||
| checkIsAdmin: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/accounts/updateAccountHandler", () => ({ | ||
| updateAccountHandler: vi.fn(), | ||
| })); | ||
|
|
||
| const { validateAuthContext } = await import("@/lib/auth/validateAuthContext"); | ||
| const { safeParseJson } = await import("@/lib/networking/safeParseJson"); | ||
| const { validateUpdateAccountBody } = await import("@/lib/accounts/validateUpdateAccountBody"); | ||
| const { checkIsAdmin } = await import("@/lib/admins/checkIsAdmin"); | ||
| const { updateAccountHandler } = await import("@/lib/accounts/updateAccountHandler"); | ||
|
|
||
| describe("patchAccountHandler", () => { | ||
| const accountA = "11111111-1111-4111-8111-111111111111"; | ||
| const accountB = "22222222-2222-4222-8222-222222222222"; | ||
| const req = {} as NextRequest; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns auth error when unauthenticated", async () => { | ||
| vi.mocked(validateAuthContext).mockResolvedValue( | ||
| NextResponse.json({ status: "error", error: "Unauthorized" }, { status: 401 }), | ||
| ); | ||
|
|
||
| const response = await patchAccountHandler(req); | ||
|
|
||
| expect(response.status).toBe(401); | ||
| expect(validateUpdateAccountBody).not.toHaveBeenCalled(); | ||
| expect(updateAccountHandler).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("updates authenticated caller account when body omits accountId", async () => { | ||
| vi.mocked(validateAuthContext).mockResolvedValue({ | ||
| accountId: accountA, | ||
| orgId: null, | ||
| authToken: "token", | ||
| }); | ||
| vi.mocked(safeParseJson).mockResolvedValue({ name: "Alice" }); | ||
| vi.mocked(validateUpdateAccountBody).mockReturnValue({ name: "Alice" }); | ||
| vi.mocked(updateAccountHandler).mockResolvedValue( | ||
| NextResponse.json({ data: { account_id: accountA } }, { status: 200 }), | ||
| ); | ||
|
|
||
| const response = await patchAccountHandler(req); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(checkIsAdmin).not.toHaveBeenCalled(); | ||
| expect(updateAccountHandler).toHaveBeenCalledWith({ | ||
| name: "Alice", | ||
| accountId: accountA, | ||
| }); | ||
| }); | ||
|
|
||
| it("rejects cross-account override for non-admin", async () => { | ||
| vi.mocked(validateAuthContext).mockResolvedValue({ | ||
| accountId: accountA, | ||
| orgId: null, | ||
| authToken: "token", | ||
| }); | ||
| vi.mocked(safeParseJson).mockResolvedValue({ | ||
| accountId: accountB, | ||
| name: "Bob", | ||
| }); | ||
| vi.mocked(validateUpdateAccountBody).mockReturnValue({ | ||
| accountId: accountB, | ||
| name: "Bob", | ||
| }); | ||
| vi.mocked(checkIsAdmin).mockResolvedValue(false); | ||
|
|
||
| const response = await patchAccountHandler(req); | ||
| const body = await response.json(); | ||
|
|
||
| expect(response.status).toBe(403); | ||
| expect(body.error).toBe("accountId override is only allowed for admin accounts"); | ||
| expect(updateAccountHandler).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("allows cross-account override for admin", async () => { | ||
| vi.mocked(validateAuthContext).mockResolvedValue({ | ||
| accountId: accountA, | ||
| orgId: null, | ||
| authToken: "token", | ||
| }); | ||
| vi.mocked(safeParseJson).mockResolvedValue({ | ||
| accountId: accountB, | ||
| name: "Bob", | ||
| }); | ||
| vi.mocked(validateUpdateAccountBody).mockReturnValue({ | ||
| accountId: accountB, | ||
| name: "Bob", | ||
| }); | ||
| vi.mocked(checkIsAdmin).mockResolvedValue(true); | ||
| vi.mocked(updateAccountHandler).mockResolvedValue( | ||
| NextResponse.json({ data: { account_id: accountB } }, { status: 200 }), | ||
| ); | ||
|
|
||
| const response = await patchAccountHandler(req); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(checkIsAdmin).toHaveBeenCalledWith(accountA); | ||
| expect(updateAccountHandler).toHaveBeenCalledWith({ | ||
| accountId: accountB, | ||
| name: "Bob", | ||
| }); | ||
| }); | ||
|
|
||
| it("returns validation error for invalid body", async () => { | ||
| vi.mocked(validateAuthContext).mockResolvedValue({ | ||
| accountId: accountA, | ||
| orgId: null, | ||
| authToken: "token", | ||
| }); | ||
| vi.mocked(safeParseJson).mockResolvedValue({ image: "bad-url" }); | ||
| vi.mocked(validateUpdateAccountBody).mockReturnValue( | ||
| NextResponse.json({ status: "error", error: "invalid body" }, { status: 400 }), | ||
| ); | ||
|
|
||
| const response = await patchAccountHandler(req); | ||
|
|
||
| expect(response.status).toBe(400); | ||
| expect(updateAccountHandler).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { NextResponse } from "next/server"; | ||
| import { validateUpdateAccountBody } from "@/lib/accounts/validateUpdateAccountBody"; | ||
|
|
||
| describe("validateUpdateAccountBody", () => { | ||
| it("returns 400 when no update fields are provided", async () => { | ||
| const result = validateUpdateAccountBody({}); | ||
| expect(result).toBeInstanceOf(NextResponse); | ||
|
|
||
| const response = result as NextResponse; | ||
| expect(response.status).toBe(400); | ||
| const body = await response.json(); | ||
| expect(body.error).toBe("At least one update field is required"); | ||
| }); | ||
|
|
||
| it("accepts body with update field only", () => { | ||
| const result = validateUpdateAccountBody({ name: "Alice" }); | ||
| expect(result).toEqual({ name: "Alice" }); | ||
| }); | ||
|
|
||
| it("accepts admin override shape with accountId plus update field", () => { | ||
| const accountId = "11111111-1111-4111-8111-111111111111"; | ||
| const result = validateUpdateAccountBody({ accountId, roleType: "manager" }); | ||
| expect(result).toEqual({ accountId, roleType: "manager" }); | ||
| }); | ||
|
|
||
| it("returns 400 when only accountId is provided (no update fields)", async () => { | ||
| const accountId = "11111111-1111-4111-8111-111111111111"; | ||
| const result = validateUpdateAccountBody({ accountId }); | ||
| expect(result).toBeInstanceOf(NextResponse); | ||
|
|
||
| const response = result as NextResponse; | ||
| expect(response.status).toBe(400); | ||
| const body = await response.json(); | ||
| expect(body.error).toBe("At least one update field is required"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; | ||
| import { safeParseJson } from "@/lib/networking/safeParseJson"; | ||
| import { | ||
| validateUpdateAccountBody, | ||
| type UpdateAccountBody, | ||
| } from "@/lib/accounts/validateUpdateAccountBody"; | ||
| import { validateAuthContext } from "@/lib/auth/validateAuthContext"; | ||
| import { updateAccountHandler } from "@/lib/accounts/updateAccountHandler"; | ||
| import { checkIsAdmin } from "@/lib/admins/checkIsAdmin"; | ||
|
|
||
| /** | ||
| * Handles PATCH /api/accounts: auth, optional admin-only accountId override, then profile update. | ||
| * | ||
| * @param req - Incoming Next.js request | ||
| * @returns Updated account JSON or error response | ||
| */ | ||
| export async function patchAccountHandler(req: NextRequest): Promise<NextResponse> { | ||
| const authResult = await validateAuthContext(req); | ||
| if (authResult instanceof NextResponse) { | ||
| return authResult; | ||
| } | ||
|
|
||
| const body = await safeParseJson(req); | ||
|
|
||
| const validated = validateUpdateAccountBody(body); | ||
| if (validated instanceof NextResponse) { | ||
| return validated; | ||
| } | ||
|
|
||
| const targetAccountId = validated.accountId ?? authResult.accountId; | ||
| if (validated.accountId && validated.accountId !== authResult.accountId) { | ||
| const isAdmin = await checkIsAdmin(authResult.accountId); | ||
| if (!isAdmin) { | ||
| return NextResponse.json( | ||
| { status: "error", error: "accountId override is only allowed for admin accounts" }, | ||
| { status: 403, headers: getCorsHeaders() }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return updateAccountHandler({ | ||
| ...(validated as UpdateAccountBody), | ||
| accountId: targetAccountId, | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,17 +2,28 @@ import { NextResponse } from "next/server"; | |
| import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; | ||
| import { z } from "zod"; | ||
|
|
||
| export const updateAccountBodySchema = z.object({ | ||
| accountId: z.string().uuid("accountId must be a valid UUID"), | ||
| name: z.string().optional(), | ||
| instruction: z.string().optional(), | ||
| organization: z.string().optional(), | ||
| image: z.string().url("image must be a valid URL").optional().or(z.literal("")), | ||
| jobTitle: z.string().optional(), | ||
| roleType: z.string().optional(), | ||
| companyName: z.string().optional(), | ||
| knowledges: z.array(z.string()).optional(), | ||
| }); | ||
| export const updateAccountBodySchema = z | ||
| .object({ | ||
| accountId: z.string().uuid("accountId must be a valid UUID").optional(), | ||
| name: z.string().optional(), | ||
| instruction: z.string().optional(), | ||
| organization: z.string().optional(), | ||
| image: z.string().url("image must be a valid URL").optional().or(z.literal("")), | ||
| jobTitle: z.string().optional(), | ||
| roleType: z.string().optional(), | ||
| companyName: z.string().optional(), | ||
| knowledges: z.array(z.string()).optional(), | ||
| }) | ||
| .refine( | ||
| body => { | ||
| const { accountId: _accountId, ...updateFields } = body; | ||
| return Object.values(updateFields).some(v => v !== undefined); | ||
| }, | ||
| { | ||
| message: "At least one update field is required", | ||
| path: ["body"], | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This chained A more maintainable approach: .refine(
body => {
const { accountId, ...updateFields } = body;
return Object.values(updateFields).some(v => v !== undefined);
},
{
message: "At least one update field is required",
path: ["body"],
},
)This destructures out |
||
| ); | ||
|
|
||
| export type UpdateAccountBody = z.infer<typeof updateAccountBodySchema>; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the target account derived from auth, not the PATCH body.
validated.accountIdstill becomes the authoritative write target here, andupdateAccountHandleruses thataccountIdfor all downstream reads and writes. Even with the admin gate, this reintroduces a public body parameter for account selection on/api/accounts; if admins need cross-account updates, move that to a separate privileged route or a server-only parameter instead.Based on learnings: "Never use
account_idin request bodies or tool schemas; always derive the account ID from authentication".🤖 Prompt for AI Agents