From ec85b596c2a45b27afcc1b57f3255fd38c0fcc29 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Tue, 8 Sep 2026 14:17:20 -0400 Subject: [PATCH] fix(admin): match the list windows the auth API enforces The API validates the window on its admin list routes: limit is 1 to 100 and offset is 0 or more. The CLI checked both flags against one range with a floor of zero, which was right for --offset and wrong for --limit, so users list --limit 0 and users list --limit 500 were sent and came back as a 400 naming neither the flag nor the bound. Each flag is checked against its own range now, and the message says which one was wrong and what it accepts. --limit 0 is an error rather than a request for nothing. The old comment called zero a legitimate answer, but the floor was really there for --offset, which the same function validated. Asking the server for zero rows and printing "No users." said there were none when the CLI had not looked, which is worse than saying the flag is out of range. org list gains --limit, --offset and --search. It sent no window at all, so once the API started defaulting to 50 it printed the first 50 organizations followed by a count of every organization, claiming rows it had never shown. It reports where the page sits now, the way users list already did, and --search matches the name and slug server-side. The window parsing and the position line move to adminShared, since two commands need both and the next list command will need them too. --- .changeset/steady-lions-page.md | 22 +++++++++++ src/commands/adminShared.ts | 70 +++++++++++++++++++++++++++++++++ src/commands/helpTopics.ts | 9 ++++- src/commands/org.test.ts | 38 +++++++++++++++++- src/commands/org.ts | 20 ++++++++-- src/commands/users.test.ts | 34 +++++++++++++++- src/commands/users.ts | 34 ++++------------ src/core/admin.ts | 20 +++++++++- 8 files changed, 211 insertions(+), 36 deletions(-) create mode 100644 .changeset/steady-lions-page.md diff --git a/.changeset/steady-lions-page.md b/.changeset/steady-lions-page.md new file mode 100644 index 0000000..65a7df9 --- /dev/null +++ b/.changeset/steady-lions-page.md @@ -0,0 +1,22 @@ +--- +"seamless-cli": minor +--- + +Match the list windows the auth API now enforces, and let `org list` page. + +The API validates the window on its admin list routes: `limit` is 1 to 100 and +`offset` is 0 or more. The CLI checked both flags against one range with a floor +of zero, which was right for `--offset` and wrong for `--limit`, so +`users list --limit 0` and `users list --limit 500` were sent and came back as a +400 naming neither the flag nor the bound. Each flag is checked against its own +range now, and the message says which one was wrong and what it accepts. + +`--limit 0` is therefore an error rather than a request for nothing. Asking the +server for zero rows and reporting "No users." said there were none when the CLI +had not looked, which is worse than saying the flag is out of range. + +`org list` gains `--limit`, `--offset` and `--search`. It sent no window at all, +so once the API started defaulting to 50 it printed the first 50 organizations +and then a count of every organization, claiming rows it had not shown. It now +reports where the page sits, the way `users list` already did, and `--search` +matches the name and slug server-side. diff --git a/src/commands/adminShared.ts b/src/commands/adminShared.ts index 1cb09e2..933a27c 100644 --- a/src/commands/adminShared.ts +++ b/src/commands/adminShared.ts @@ -1,6 +1,10 @@ import kleur from "kleur"; import { ReauthRequiredError } from "../core/authClient.js"; import { AdminApiError, PermissionError } from "../core/admin.js"; +import { extractFlag } from "../core/args.js"; + +const DEFAULT_LIMIT = 50; +const LIMIT_MAX = 100; export function reportAdminError(err: unknown): never { if (err instanceof ReauthRequiredError) { @@ -21,3 +25,69 @@ export function parseList(value?: string): string[] | undefined { .map((item) => item.trim()) .filter((item) => item.length > 0); } + +// The two flags do not share a range. `--offset 0` is the first page, but the API +// takes a limit of 1 to 100 and answers anything outside that with a 400 naming +// neither the flag nor the bound. Checking here says which flag was wrong and what +// it accepts, and a non-numeric page stays a typo the server would reinterpret. +function pageNumber( + raw: string | undefined, + flag: string, + fallback: number, + bounds: { min: number; max?: number }, +): number { + if (raw === undefined) return fallback; + + const value = Number(raw); + const { min, max } = bounds; + + if (!Number.isInteger(value) || value < min || (max !== undefined && value > max)) { + console.error( + kleur.red( + max === undefined + ? `--${flag} must be a whole number of ${min} or more.` + : `--${flag} must be a whole number between ${min} and ${max}.`, + ), + ); + process.exit(1); + } + + return value; +} + +/** Reads `--limit` and `--offset` off a list command, and the arguments left over. */ +export function parseWindow(args: string[]): { + limit: number; + offset: number; + rest: string[]; +} { + const limitFlag = extractFlag(args, "limit"); + const offsetFlag = extractFlag(limitFlag.rest, "offset"); + + return { + limit: pageNumber(limitFlag.value, "limit", DEFAULT_LIMIT, { + min: 1, + max: LIMIT_MAX, + }), + offset: pageNumber(offsetFlag.value, "offset", 0, { min: 0 }), + rest: offsetFlag.rest, + }; +} + +/** + * The position of the rows on screen within the whole result set. + * + * Printed rather than a bare count, because a list route returns one page and a + * count of every match. Reporting the count alone reads as though every row is on + * screen, which is how `org list` came to claim a total it had not shown. + */ +export function pagePosition( + offset: number, + shown: number, + total: number, + noun: string, +): string { + return `Showing ${offset + 1}-${offset + shown} of ${total} ${noun}${ + total === 1 ? "" : "s" + }.`; +} diff --git a/src/commands/helpTopics.ts b/src/commands/helpTopics.ts index e98fd76..bbef1d2 100644 --- a/src/commands/helpTopics.ts +++ b/src/commands/helpTopics.ts @@ -356,7 +356,9 @@ config oauth-providers body: `Admin user management (requires an admin role). users list [--limit ] [--offset ] [--json] - • List users + • List users, 50 at a time by default + • --limit is 1 to 100, --offset is 0 or more; total counts every user, not + just the page shown users delete [--force] • Delete a user (asks for confirmation) users credentials [--json] @@ -383,7 +385,10 @@ users prepare-device-replacement [--force] [--keep-sessions] [--keep-passke "org , org members ", body: `Admin organization management (requires an admin role). -org list [--json] +org list [--limit ] [--offset ] [--search ] [--json] + • Lists 50 at a time; --limit is 1 to 100, --offset is 0 or more + • --search matches the name and slug, and total counts every match rather + than the page shown org create [--slug ] org get [--json] org update [--name ] [--slug ] diff --git a/src/commands/org.test.ts b/src/commands/org.test.ts index cb3b4d5..f98e873 100644 --- a/src/commands/org.test.ts +++ b/src/commands/org.test.ts @@ -128,7 +128,10 @@ describe("runOrg list", () => { total: 1, }); await runOrg(["list", "--json"]); - expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient); + expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, { + limit: 50, + offset: 0, + }); expect(logs()).toContain(JSON.stringify([{ id: "o1", name: "Acme" }], null, 2)); }); @@ -160,6 +163,39 @@ describe("runOrg list", () => { expect(logs()).toContain("(no name)"); expect(logs()).toContain("2 organizations."); }); + + // The endpoint returns one page and a count of every match, so printing the + // count alone claimed rows that were never shown. + it("reports where the page sits in the result set", async () => { + vi.mocked(listOrgs).mockResolvedValue({ + organizations: [{ id: "o1", name: "Acme" }], + total: 140, + }); + await runOrg(["list", "--limit", "1", "--offset", "50"]); + expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, { + limit: 1, + offset: 50, + }); + expect(logs()).toContain("Showing 51-51 of 140 organizations."); + }); + + it("sends a search term when one is given", async () => { + vi.mocked(listOrgs).mockResolvedValue({ + organizations: [{ id: "o1", name: "Acme" }], + total: 1, + }); + await runOrg(["list", "--search", "acme"]); + expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, { + limit: 50, + offset: 0, + search: "acme", + }); + }); + + it("rejects a window the API would refuse", async () => { + await expect(runOrg(["list", "--limit", "0"])).rejects.toBeInstanceOf(ExitError); + expect(vi.mocked(listOrgs)).not.toHaveBeenCalled(); + }); }); describe("runOrg create", () => { diff --git a/src/commands/org.ts b/src/commands/org.ts index d98a73b..700d8da 100644 --- a/src/commands/org.ts +++ b/src/commands/org.ts @@ -12,7 +12,12 @@ import { updateOrg, type Json, } from "../core/admin.js"; -import { parseList, reportAdminError } from "./adminShared.js"; +import { + pagePosition, + parseList, + parseWindow, + reportAdminError, +} from "./adminShared.js"; import { confirmDestructive, hasForceFlag, @@ -83,7 +88,14 @@ async function runOrgMembers(args: string[]): Promise { async function orgList(client: AuthClient, rest: string[]): Promise { const json = rest.includes("--json"); - const { organizations, total } = await listOrgs(client); + const { limit, offset, rest: remaining } = parseWindow(rest); + const { value: search } = extractFlag(remaining, "search"); + + const { organizations, total } = await listOrgs(client, { + limit, + offset, + ...(search ? { search } : {}), + }); if (json) { console.log(JSON.stringify(organizations, null, 2)); @@ -96,7 +108,9 @@ async function orgList(client: AuthClient, rest: string[]): Promise { for (const org of organizations) { printOrgRow(org); } - console.log(kleur.dim(`${total} organization${total === 1 ? "" : "s"}.`)); + console.log( + kleur.dim(pagePosition(offset, organizations.length, total, "organization")), + ); } async function orgCreate(client: AuthClient, rest: string[]): Promise { diff --git a/src/commands/users.test.ts b/src/commands/users.test.ts index 698528f..db3a238 100644 --- a/src/commands/users.test.ts +++ b/src/commands/users.test.ts @@ -183,13 +183,45 @@ describe("runUsers list", () => { ["--limit", "abc"], ["--limit", "-1"], ["--offset", "1.5"], + ["--offset", "-1"], ])("rejects %s %s", async (flag, value) => { await expect(runUsers(["list", flag, value])).rejects.toBeInstanceOf(ExitError); expect(errorSpy).toHaveBeenCalledWith( - expect.stringContaining("must be a non-negative whole number"), + expect.stringContaining("must be a whole number"), ); expect(vi.mocked(listUsers)).not.toHaveBeenCalled(); }); + + // The two flags do not share a range, so the message has to name the one that + // was wrong and the bound it broke. + it.each(["0", "101"])( + "rejects --limit %s outside the range the API accepts", + async (value) => { + await expect(runUsers(["list", "--limit", value])).rejects.toBeInstanceOf(ExitError); + expect(errorSpy).toHaveBeenCalledWith( + "--limit must be a whole number between 1 and 100.", + ); + expect(vi.mocked(listUsers)).not.toHaveBeenCalled(); + }, + ); + + it("still accepts an offset of zero", async () => { + vi.mocked(listUsers).mockResolvedValue({ users: [{ id: "u1" }], total: 1 }); + await runUsers(["list", "--offset", "0"]); + expect(vi.mocked(listUsers)).toHaveBeenCalledWith(fakeClient, { + limit: 50, + offset: 0, + }); + }); + + it("accepts the largest window the API allows", async () => { + vi.mocked(listUsers).mockResolvedValue({ users: [{ id: "u1" }], total: 1 }); + await runUsers(["list", "--limit", "100"]); + expect(vi.mocked(listUsers)).toHaveBeenCalledWith(fakeClient, { + limit: 100, + offset: 0, + }); + }); }); describe("runUsers delete", () => { diff --git a/src/commands/users.ts b/src/commands/users.ts index ab501f4..51cf8f4 100644 --- a/src/commands/users.ts +++ b/src/commands/users.ts @@ -8,7 +8,11 @@ import { prepareDeviceReplacement, type Json, } from "../core/admin.js"; -import { reportAdminError } from "./adminShared.js"; +import { + pagePosition, + parseWindow, + reportAdminError, +} from "./adminShared.js"; import { confirmDestructive, hasForceFlag, @@ -45,27 +49,9 @@ export async function runUsers(args: string[]): Promise { } } -const DEFAULT_LIMIT = 50; - -// `--limit 0` is a legitimate answer (ask for nothing), so the floor is 0 rather -// than 1, but a negative or non-numeric page is a typo the server would silently -// reinterpret. -function pageNumber(raw: string | undefined, flag: string, fallback: number): number { - if (raw === undefined) return fallback; - const value = Number(raw); - if (!Number.isInteger(value) || value < 0) { - console.error(kleur.red(`--${flag} must be a non-negative whole number.`)); - process.exit(1); - } - return value; -} - async function usersList(client: AuthClient, rest: string[]): Promise { const json = rest.includes("--json"); - const limitFlag = extractFlag(rest, "limit"); - const offsetFlag = extractFlag(limitFlag.rest, "offset"); - const limit = pageNumber(limitFlag.value, "limit", DEFAULT_LIMIT); - const offset = pageNumber(offsetFlag.value, "offset", 0); + const { limit, offset } = parseWindow(rest); const { users, total } = await listUsers(client, { limit, offset }); @@ -82,13 +68,7 @@ async function usersList(client: AuthClient, rest: string[]): Promise { for (const user of users) { printUserRow(user); } - console.log( - kleur.dim( - `Showing ${offset + 1}-${offset + users.length} of ${total} user${ - total === 1 ? "" : "s" - }.`, - ), - ); + console.log(kleur.dim(pagePosition(offset, users.length, total, "user"))); } async function usersDelete(client: AuthClient, rest: string[]): Promise { diff --git a/src/core/admin.ts b/src/core/admin.ts index 7df157a..ff881e9 100644 --- a/src/core/admin.ts +++ b/src/core/admin.ts @@ -142,13 +142,29 @@ export interface OrgList { total: number; } -export async function listOrgs(client: AuthClient): Promise { +export interface ListOrgsOptions { + limit?: number; + offset?: number; + search?: string; +} + +export async function listOrgs( + client: AuthClient, + opts: ListOrgsOptions = {}, +): Promise { + const query = new URLSearchParams(); + if (opts.limit !== undefined) query.set("limit", String(opts.limit)); + if (opts.offset !== undefined) query.set("offset", String(opts.offset)); + if (opts.search) query.set("search", opts.search); + const suffix = query.size ? `?${query}` : ""; + const res = await call<{ organizations?: unknown; total?: number }>( client, "GET", - "/admin/organizations", + `/admin/organizations${suffix}`, ); if (!res.ok) throw new AdminApiError(`Could not list organizations (${res.status}).`); + // `total` counts every match, not just this page, so callers can report position. return { organizations: arr(res.data?.organizations), total: res.data?.total ?? 0,