From 41384f08fb794e197a7877bfd38cff1d081272ba Mon Sep 17 00:00:00 2001 From: Chris Parsons <438676+chrisparsons83@users.noreply.github.com> Date: Wed, 18 Mar 2026 00:41:56 -0700 Subject: [PATCH] Grant sitewide admins commissioner-level access in leagues (#162) * Grant sitewide admins commissioner-level access in leagues - `isCommissioner()` now returns true for site admins, covering all commissioner-gated loaders (league home, settings, sport season detail) and draft API routes (start, pause, resume, rollback, replace-pick, force-autopick, force-manual-pick, adjust-time-bank, make-pick) - Added `hasCommissionerRecord()` (DB-only, no admin bypass) for the "already a commissioner" duplicate-entry check in the settings action, preventing a false positive when adding a site admin as commissioner - `isCommissioner()` now runs the admin check and DB query in parallel via Promise.all to avoid a serial roundtrip on every check - Added "admin" to the `picked_by_type` enum (migration 0049) so picks forced by a site admin are recorded accurately in the audit log rather than as "commissioner" - 8 unit tests covering both isCommissioner and hasCommissionerRecord Co-Authored-By: Claude Sonnet 4.6 * Fix draft.force-manual-pick tests broken by isUserAdminByClerkId The route now calls isUserAdminByClerkId which hits database().query.users, but the test's mock DB had no query.users entry. Add a vi.mock for ~/models/user and default isUserAdminByClerkId to false in beforeEach. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- app/models/__tests__/commissioner.test.ts | 115 ++++++++++++++++++ app/models/commissioner.ts | 20 ++- .../__tests__/draft.force-manual-pick.test.ts | 7 ++ app/routes/api/draft.adjust-time-bank.ts | 10 +- app/routes/api/draft.force-autopick.ts | 12 +- app/routes/api/draft.force-manual-pick.ts | 22 ++-- app/routes/api/draft.make-pick.ts | 24 ++-- app/routes/api/draft.pause.ts | 12 +- app/routes/api/draft.replace-pick.ts | 10 +- app/routes/api/draft.resume.ts | 12 +- app/routes/api/draft.rollback.ts | 10 +- app/routes/api/draft.start.ts | 12 +- app/routes/leagues/$leagueId.settings.tsx | 6 +- database/schema.ts | 1 + drizzle/0049_add_admin_picked_by_type.sql | 9 ++ drizzle/meta/_journal.json | 7 ++ 16 files changed, 207 insertions(+), 82 deletions(-) create mode 100644 app/models/__tests__/commissioner.test.ts create mode 100644 drizzle/0049_add_admin_picked_by_type.sql diff --git a/app/models/__tests__/commissioner.test.ts b/app/models/__tests__/commissioner.test.ts new file mode 100644 index 0000000..28a15b7 --- /dev/null +++ b/app/models/__tests__/commissioner.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("~/models/user", () => ({ + isUserAdminByClerkId: vi.fn(), +})); + +vi.mock("~/database/context", () => ({ + database: vi.fn(), +})); + +import { isCommissioner, hasCommissionerRecord } from "../commissioner"; +import { isUserAdminByClerkId } from "~/models/user"; +import { database } from "~/database/context"; + +const LEAGUE_ID = "league-1"; +const USER_ID = "user-clerk-1"; + +function makeMockDb(commissionerRow: object | null) { + return { + query: { + commissioners: { + findFirst: vi.fn().mockResolvedValue(commissionerRow), + }, + }, + }; +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("isCommissioner", () => { + it("returns true for a site admin without a commissioner record", async () => { + vi.mocked(isUserAdminByClerkId).mockResolvedValue(true); + vi.mocked(database).mockReturnValue(makeMockDb(null) as never); + + const result = await isCommissioner(LEAGUE_ID, USER_ID); + expect(result).toBe(true); + }); + + it("returns true for a user with a commissioner record", async () => { + vi.mocked(isUserAdminByClerkId).mockResolvedValue(false); + vi.mocked(database).mockReturnValue( + makeMockDb({ id: "comm-1", leagueId: LEAGUE_ID, userId: USER_ID }) as never + ); + + const result = await isCommissioner(LEAGUE_ID, USER_ID); + expect(result).toBe(true); + }); + + it("returns true for a site admin who also has a commissioner record", async () => { + vi.mocked(isUserAdminByClerkId).mockResolvedValue(true); + vi.mocked(database).mockReturnValue( + makeMockDb({ id: "comm-1", leagueId: LEAGUE_ID, userId: USER_ID }) as never + ); + + const result = await isCommissioner(LEAGUE_ID, USER_ID); + expect(result).toBe(true); + }); + + it("returns false for a non-admin user without a commissioner record", async () => { + vi.mocked(isUserAdminByClerkId).mockResolvedValue(false); + vi.mocked(database).mockReturnValue(makeMockDb(null) as never); + + const result = await isCommissioner(LEAGUE_ID, USER_ID); + expect(result).toBe(false); + }); + + it("runs the admin check and DB query in parallel", async () => { + const order: string[] = []; + vi.mocked(isUserAdminByClerkId).mockImplementation(async () => { + order.push("admin"); + return false; + }); + const db = makeMockDb(null); + db.query.commissioners.findFirst = vi.fn().mockImplementation(async () => { + order.push("db"); + return null; + }); + vi.mocked(database).mockReturnValue(db as never); + + await isCommissioner(LEAGUE_ID, USER_ID); + + // Both should have been called; order is non-deterministic for parallel calls + expect(order).toContain("admin"); + expect(order).toContain("db"); + }); +}); + +describe("hasCommissionerRecord", () => { + it("returns true when a commissioner record exists", async () => { + vi.mocked(database).mockReturnValue( + makeMockDb({ id: "comm-1", leagueId: LEAGUE_ID, userId: USER_ID }) as never + ); + + const result = await hasCommissionerRecord(LEAGUE_ID, USER_ID); + expect(result).toBe(true); + }); + + it("returns false when no commissioner record exists", async () => { + vi.mocked(database).mockReturnValue(makeMockDb(null) as never); + + const result = await hasCommissionerRecord(LEAGUE_ID, USER_ID); + expect(result).toBe(false); + }); + + it("returns false for a site admin without a commissioner record", async () => { + // hasCommissionerRecord must NOT consult the admin flag + vi.mocked(database).mockReturnValue(makeMockDb(null) as never); + + const result = await hasCommissionerRecord(LEAGUE_ID, USER_ID); + expect(result).toBe(false); + expect(isUserAdminByClerkId).not.toHaveBeenCalled(); + }); +}); diff --git a/app/models/commissioner.ts b/app/models/commissioner.ts index e17a558..314e756 100644 --- a/app/models/commissioner.ts +++ b/app/models/commissioner.ts @@ -1,6 +1,7 @@ import { eq, and } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; +import { isUserAdminByClerkId } from "~/models/user"; export type Commissioner = typeof schema.commissioners.$inferSelect; export type NewCommissioner = typeof schema.commissioners.$inferInsert; @@ -36,7 +37,7 @@ export async function findCommissionersByUserId( }); } -export async function isCommissioner( +export async function hasCommissionerRecord( leagueId: string, userId: string ): Promise { @@ -50,6 +51,23 @@ export async function isCommissioner( return !!commissioner; } +export async function isCommissioner( + leagueId: string, + userId: string +): Promise { + const db = database(); + const [isAdmin, commissioner] = await Promise.all([ + isUserAdminByClerkId(userId), + db.query.commissioners.findFirst({ + where: and( + eq(schema.commissioners.leagueId, leagueId), + eq(schema.commissioners.userId, userId) + ), + }), + ]); + return isAdmin || !!commissioner; +} + export async function countCommissionersByLeagueId( leagueId: string ): Promise { diff --git a/app/routes/api/__tests__/draft.force-manual-pick.test.ts b/app/routes/api/__tests__/draft.force-manual-pick.test.ts index d1f5c0a..4164fa5 100644 --- a/app/routes/api/__tests__/draft.force-manual-pick.test.ts +++ b/app/routes/api/__tests__/draft.force-manual-pick.test.ts @@ -28,6 +28,9 @@ vi.mock("~/models/draft-utils", () => ({ checkAndTriggerNextAutodraft: vi.fn(), calculatePickInfo: vi.fn().mockReturnValue({ round: 1, pickInRound: 1, teamIndex: 0 }), })); +vi.mock("~/models/user", () => ({ + isUserAdminByClerkId: vi.fn(), +})); // ── Fixtures ───────────────────────────────────────────────────────────────── @@ -115,6 +118,10 @@ describe("draft.force-manual-pick action", () => { const { getAuth } = await import("@clerk/react-router/server"); vi.mocked(getAuth).mockResolvedValue({ userId: COMMISSIONER_ID } as any); + // User model: default to non-admin + const { isUserAdminByClerkId } = await import("~/models/user"); + vi.mocked(isUserAdminByClerkId).mockResolvedValue(false); + // Socket mockSocketIO = { to: vi.fn().mockReturnThis(), emit: vi.fn() }; const socketModule = await import("~/server/socket"); diff --git a/app/routes/api/draft.adjust-time-bank.ts b/app/routes/api/draft.adjust-time-bank.ts index 09cc59e..be549d3 100644 --- a/app/routes/api/draft.adjust-time-bank.ts +++ b/app/routes/api/draft.adjust-time-bank.ts @@ -2,6 +2,7 @@ import { getAuth } from "@clerk/react-router/server"; import { eq, and } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; +import { isCommissioner } from "~/models/commissioner"; import { getSocketIO } from "../../../server/socket"; import type { ActionFunctionArgs } from "react-router"; @@ -37,14 +38,7 @@ export async function action(args: ActionFunctionArgs) { return Response.json({ error: "Draft is not currently active" }, { status: 409 }); } - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json( { error: "Only commissioners can adjust time banks" }, { status: 403 } diff --git a/app/routes/api/draft.force-autopick.ts b/app/routes/api/draft.force-autopick.ts index a2fb8d8..b26e356 100644 --- a/app/routes/api/draft.force-autopick.ts +++ b/app/routes/api/draft.force-autopick.ts @@ -1,8 +1,9 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; -import { eq, and } from "drizzle-orm"; +import { eq } from "drizzle-orm"; import { executeAutoPick } from "~/models/draft-utils"; +import { isCommissioner } from "~/models/commissioner"; import type { ActionFunctionArgs } from "react-router"; export async function action(args: ActionFunctionArgs) { @@ -34,14 +35,7 @@ export async function action(args: ActionFunctionArgs) { } // Check if user is commissioner - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json({ error: "Only commissioners can force autopick" }, { status: 403 }); } diff --git a/app/routes/api/draft.force-manual-pick.ts b/app/routes/api/draft.force-manual-pick.ts index 4ef8066..1a84683 100644 --- a/app/routes/api/draft.force-manual-pick.ts +++ b/app/routes/api/draft.force-manual-pick.ts @@ -2,6 +2,7 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { eq, and, sql } from "drizzle-orm"; +import { isUserAdminByClerkId } from "~/models/user"; import { calculateDraftEligibility } from "~/lib/draft-eligibility"; import { getDraftPicksWithSports, getTeamDraftPicksWithSports } from "~/models/draft-pick"; import { getParticipantsForSeasonWithSports } from "~/models/participant"; @@ -39,15 +40,18 @@ export async function action(args: ActionFunctionArgs) { return Response.json({ error: "Season not found" }, { status: 404 }); } - // Check if user is commissioner - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); + // Check if user is commissioner or site admin; capture both to set pickedByType accurately + const [isAdmin, commissionerRecord] = await Promise.all([ + isUserAdminByClerkId(userId), + db.query.commissioners.findFirst({ + where: and( + eq(schema.commissioners.leagueId, season.leagueId), + eq(schema.commissioners.userId, userId) + ), + }), + ]); - if (!isCommissioner) { + if (!isAdmin && !commissionerRecord) { return Response.json({ error: "Only commissioners can force a manual pick" }, { status: 403 }); } @@ -152,7 +156,7 @@ export async function action(args: ActionFunctionArgs) { round: currentRound, pickInRound, pickedByUserId: userId, - pickedByType: "commissioner", + pickedByType: commissionerRecord ? "commissioner" : "admin", }) .returning(); diff --git a/app/routes/api/draft.make-pick.ts b/app/routes/api/draft.make-pick.ts index 95492cb..ff6cd5a 100644 --- a/app/routes/api/draft.make-pick.ts +++ b/app/routes/api/draft.make-pick.ts @@ -2,6 +2,7 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { eq, and, sql } from "drizzle-orm"; +import { isUserAdminByClerkId } from "~/models/user"; import { calculateDraftEligibility } from "~/lib/draft-eligibility"; import { getDraftPicksWithSports, getTeamDraftPicksWithSports } from "~/models/draft-pick"; import { getParticipantsForSeasonWithSports } from "~/models/participant"; @@ -63,17 +64,20 @@ export async function action(args: ActionFunctionArgs) { return Response.json({ error: "Invalid draft state" }, { status: 500 }); } - // Check permissions: must be team owner or commissioner + // Check permissions: must be team owner or commissioner/admin + // Capture both results to set pickedByType accurately in the audit record const isTeamOwner = currentDraftSlot.team.ownerId === userId; - const commissionerRecord = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - const isCommissioner = !!commissionerRecord; + const [isAdmin, commissionerRecord] = await Promise.all([ + isUserAdminByClerkId(userId), + db.query.commissioners.findFirst({ + where: and( + eq(schema.commissioners.leagueId, season.leagueId), + eq(schema.commissioners.userId, userId) + ), + }), + ]); - if (!isTeamOwner && !isCommissioner) { + if (!isTeamOwner && !isAdmin && !commissionerRecord) { return Response.json({ error: "You do not have permission to pick for this team" }, { status: 403 }); } @@ -149,7 +153,7 @@ export async function action(args: ActionFunctionArgs) { round: currentRound, pickInRound, pickedByUserId: userId, - pickedByType: isTeamOwner ? "owner" : "commissioner", + pickedByType: isTeamOwner ? "owner" : commissionerRecord ? "commissioner" : "admin", timeUsed: timerSnapshot?.timeRemaining ?? 0, }) .returning(); diff --git a/app/routes/api/draft.pause.ts b/app/routes/api/draft.pause.ts index 36a3c3d..dfc1771 100644 --- a/app/routes/api/draft.pause.ts +++ b/app/routes/api/draft.pause.ts @@ -1,7 +1,8 @@ import { getAuth } from "@clerk/react-router/server"; -import { eq, and } from "drizzle-orm"; +import { eq } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; +import { isCommissioner } from "~/models/commissioner"; import { getSocketIO } from "../../../server/socket"; import type { ActionFunctionArgs } from "react-router"; @@ -35,14 +36,7 @@ export async function action(args: ActionFunctionArgs) { } // Verify user is commissioner - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json( { error: "Only commissioners can pause the draft" }, { status: 403 } diff --git a/app/routes/api/draft.replace-pick.ts b/app/routes/api/draft.replace-pick.ts index 2c6de37..41c995c 100644 --- a/app/routes/api/draft.replace-pick.ts +++ b/app/routes/api/draft.replace-pick.ts @@ -2,6 +2,7 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { eq, and } from "drizzle-orm"; +import { isCommissioner } from "~/models/commissioner"; import { calculateDraftEligibility } from "~/lib/draft-eligibility"; import { getDraftPicksWithSports, getTeamDraftPicksWithSports } from "~/models/draft-pick"; import { getParticipantsForSeasonWithSports } from "~/models/participant"; @@ -36,14 +37,7 @@ export async function action(args: ActionFunctionArgs) { return Response.json({ error: "Season not found" }, { status: 404 }); } - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json({ error: "Only commissioners can replace picks" }, { status: 403 }); } diff --git a/app/routes/api/draft.resume.ts b/app/routes/api/draft.resume.ts index 20fd25e..21700b3 100644 --- a/app/routes/api/draft.resume.ts +++ b/app/routes/api/draft.resume.ts @@ -1,7 +1,8 @@ import { getAuth } from "@clerk/react-router/server"; -import { eq, and } from "drizzle-orm"; +import { eq } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; +import { isCommissioner } from "~/models/commissioner"; import { getSocketIO } from "../../../server/socket"; import type { ActionFunctionArgs } from "react-router"; @@ -35,14 +36,7 @@ export async function action(args: ActionFunctionArgs) { } // Verify user is commissioner - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json( { error: "Only commissioners can resume the draft" }, { status: 403 } diff --git a/app/routes/api/draft.rollback.ts b/app/routes/api/draft.rollback.ts index 839ff9e..ef3f7ca 100644 --- a/app/routes/api/draft.rollback.ts +++ b/app/routes/api/draft.rollback.ts @@ -2,6 +2,7 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { eq, and, gte } from "drizzle-orm"; +import { isCommissioner } from "~/models/commissioner"; import { getSocketIO } from "../../../server/socket"; import type { ActionFunctionArgs } from "react-router"; @@ -31,14 +32,7 @@ export async function action(args: ActionFunctionArgs) { return Response.json({ error: "Season not found" }, { status: 404 }); } - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json({ error: "Only commissioners can roll back the draft" }, { status: 403 }); } diff --git a/app/routes/api/draft.start.ts b/app/routes/api/draft.start.ts index c2d0ed8..754dbbf 100644 --- a/app/routes/api/draft.start.ts +++ b/app/routes/api/draft.start.ts @@ -1,8 +1,9 @@ import { getAuth } from "@clerk/react-router/server"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; -import { eq, and } from "drizzle-orm"; +import { eq } from "drizzle-orm"; import { deleteSeasonTimers, initializeDraftTimers } from "~/models/draft-timer"; +import { isCommissioner } from "~/models/commissioner"; import { getSocketIO } from "../../../server/socket"; import type { ActionFunctionArgs } from "react-router"; @@ -33,14 +34,7 @@ export async function action(args: ActionFunctionArgs) { } // Check if user is commissioner - const isCommissioner = await db.query.commissioners.findFirst({ - where: and( - eq(schema.commissioners.leagueId, season.leagueId), - eq(schema.commissioners.userId, userId) - ), - }); - - if (!isCommissioner) { + if (!(await isCommissioner(season.leagueId, userId))) { return Response.json({ error: "Only commissioners can start the draft" }, { status: 403 }); } diff --git a/app/routes/leagues/$leagueId.settings.tsx b/app/routes/leagues/$leagueId.settings.tsx index 9b0dcd2..db0feab 100644 --- a/app/routes/leagues/$leagueId.settings.tsx +++ b/app/routes/leagues/$leagueId.settings.tsx @@ -7,6 +7,7 @@ import { findLeagueById, updateLeague, deleteLeague } from "~/models/league"; import { sendStandingsUpdateNotification } from "~/services/discord"; import { isCommissioner, + hasCommissionerRecord, findCommissionersByLeagueId, createCommissioner, countCommissionersByLeagueId, @@ -583,8 +584,9 @@ export async function action(args: Route.ActionArgs) { return { error: "User is required" }; } - // Check if user is already a commissioner - const alreadyCommissioner = await isCommissioner(leagueId, userClerkId); + // Check if user already has a commissioner record (don't use isCommissioner — it + // returns true for site admins, causing a false "already a commissioner" error) + const alreadyCommissioner = await hasCommissionerRecord(leagueId, userClerkId); if (alreadyCommissioner) { return { error: "This user is already a commissioner" }; } diff --git a/database/schema.ts b/database/schema.ts index fb50d91..dd2df89 100644 --- a/database/schema.ts +++ b/database/schema.ts @@ -55,6 +55,7 @@ export const eventTypeEnum = pgEnum("event_type", [ export const pickedByTypeEnum = pgEnum("picked_by_type", [ "owner", "commissioner", + "admin", "auto", ]); diff --git a/drizzle/0049_add_admin_picked_by_type.sql b/drizzle/0049_add_admin_picked_by_type.sql new file mode 100644 index 0000000..066fad2 --- /dev/null +++ b/drizzle/0049_add_admin_picked_by_type.sql @@ -0,0 +1,9 @@ +DO $$ BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_enum + WHERE enumlabel = 'admin' + AND enumtypid = (SELECT oid FROM pg_type WHERE typname = 'picked_by_type') + ) THEN + ALTER TYPE "public"."picked_by_type" ADD VALUE 'admin'; + END IF; +END $$; diff --git a/drizzle/meta/_journal.json b/drizzle/meta/_journal.json index db0615a..218a718 100644 --- a/drizzle/meta/_journal.json +++ b/drizzle/meta/_journal.json @@ -344,6 +344,13 @@ "when": 1773765768905, "tag": "0048_eager_songbird", "breakpoints": true + }, + { + "idx": 49, + "version": "7", + "when": 1773800000000, + "tag": "0049_add_admin_picked_by_type", + "breakpoints": true } ] } \ No newline at end of file