From 673841e70527f40f8fd5c08de4c063c64b76d97c Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Tue, 8 Sep 2026 13:59:40 -0400 Subject: [PATCH] feat(admin): validate and document the window on the user list The route has always read limit, offset and search, but it declared no query schema, so it was the one admin collection whose parameters were absent from openapi.json. A generated client could not know they existed, and a reader checking the document would conclude the endpoint took none. That is the same wrong inference that led to organization paging being reported missing. The parameters are declared now, so they appear in the generated contract alongside the ones on /admin/sessions, /admin/auth-events and /admin/organizations, and they are validated the same way: limit between 1 and 100 defaulting to 50, offset from 0, and search trimmed. Three inputs that used to be accepted are rejected with a 400. A limit above 100 was honoured in full, so one call could ask for every user in the deployment. A non-numeric limit reached Sequelize as NaN and failed in the database rather than at the edge. An all-whitespace search built a %% pattern matching every row, so a filter that looked empty returned the unfiltered list. seamless-cli accepts users list --limit 0 deliberately, meaning ask for nothing, and that value is now a 400. The changeset says so, since the flag needs a floor of 1 or to skip the request when asked for zero rows. --- .changeset/olive-swans-repeat.md | 27 +++++++++++ openapi.json | 62 ++++++++++++++++++++++++++ src/controllers/admin.ts | 10 +++-- src/generated/api.ts | 41 ++++++++++++++++- src/routes/admin.routes.ts | 5 ++- src/schemas/admin.query.ts | 12 +++++ tests/integration/admin/admin.spec.ts | 64 +++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 6 deletions(-) create mode 100644 .changeset/olive-swans-repeat.md diff --git a/.changeset/olive-swans-repeat.md b/.changeset/olive-swans-repeat.md new file mode 100644 index 0000000..dc1a433 --- /dev/null +++ b/.changeset/olive-swans-repeat.md @@ -0,0 +1,27 @@ +--- +'seamless-auth-api': minor +--- + +Validate and document the window on `GET /admin/users`. + +The route has always read `limit`, `offset` and `search`, but it declared no +query schema, so it was the one admin collection whose parameters were absent +from `openapi.json`. A generated client could not know they existed, and a +reader checking the document would conclude the endpoint took none. That is the +same wrong inference that led to organization paging being reported missing. + +The parameters are now declared, so they appear in the generated contract +alongside the ones on `/admin/sessions`, `/admin/auth-events` and +`/admin/organizations`, and they are validated the same way: `limit` between 1 +and 100 defaulting to 50, `offset` from 0, and `search` trimmed. + +Two inputs that used to be accepted are now rejected with a `400`. A `limit` +above 100 was previously honoured in full, so a single call could ask for every +user in the deployment. A non-numeric `limit` reached Sequelize as `NaN` and +failed in the database rather than at the edge. An all-whitespace `search` built +a `%%` pattern that matched every row, so a filter that looked empty returned +the unfiltered list. + +`seamless-cli` accepts `users list --limit 0` deliberately, meaning ask for +nothing, and that value is now a `400`. The flag needs a floor of 1, or to skip +the request when asked for zero rows. diff --git a/openapi.json b/openapi.json index a0c939d..4512828 100644 --- a/openapi.json +++ b/openapi.json @@ -1638,8 +1638,29 @@ "/admin/users": { "get": { "summary": "List users (internal)", + "description": "Returns a window of users. `total` counts every user matching `search`, not the returned page.", "tags": ["Admin"], "security": [{ "bearerAuth": [] }], + "parameters": [ + { + "schema": { "type": "number", "minimum": 1, "maximum": 100, "default": 50 }, + "required": false, + "name": "limit", + "in": "query" + }, + { + "schema": { "type": "number", "nullable": true, "minimum": 0, "default": 0 }, + "required": false, + "name": "offset", + "in": "query" + }, + { + "schema": { "type": "string", "minLength": 1, "maxLength": 120 }, + "required": false, + "name": "search", + "in": "query" + } + ], "responses": { "200": { "description": "HTTP 200", @@ -1692,6 +1713,47 @@ } } }, + "400": { + "description": "HTTP 400", + "content": { + "application/json": { + "example": { + "error": "string", + "message": "string", + "details": { "issues": [null] } + }, + "schema": { + "type": "object", + "properties": { + "error": { "type": "string" }, + "message": { "type": "string" }, + "details": { + "type": "object", + "properties": { + "issues": { + "type": "array", + "items": { + "type": "object", + "properties": { + "path": { + "type": "array", + "items": { "anyOf": [{ "type": "string" }, { "type": "number" }] } + }, + "code": { "type": "string" }, + "message": { "type": "string" } + }, + "required": ["path", "code", "message"] + } + } + }, + "required": ["issues"] + } + }, + "required": ["error"] + } + } + } + }, "429": { "description": "HTTP 429", "content": { diff --git a/src/controllers/admin.ts b/src/controllers/admin.ts index 330964e..88df180 100644 --- a/src/controllers/admin.ts +++ b/src/controllers/admin.ts @@ -15,6 +15,7 @@ import { getSequelize } from '../models/index.js'; import { Session } from '../models/sessions.js'; import { TotpCredential } from '../models/totpCredentials.js'; import { User } from '../models/users.js'; +import { AdminUserListQuerySchema } from '../schemas/admin.query.js'; import { CreateUserSchema, DeviceReplacementRecoverySchema, @@ -82,7 +83,10 @@ function actingAdminId(req: Request): string | null { } export const getUsers = async (req: ServiceRequest, res: Response) => { - const { limit = 50, offset = 0, search } = req.query; + // Re-parsed rather than read straight off `req.query`: `defineRoute` has already + // validated it, so this cannot fail, and it is how the coerced numbers recover their + // types on an Express query whose values are otherwise strings. + const { limit, offset, search } = AdminUserListQuerySchema.parse(req.query); const where: WhereOptions = search ? { @@ -109,8 +113,8 @@ export const getUsers = async (req: ServiceRequest, res: Response) => { 'createdAt', 'updatedAt', ], - limit: Number(limit), - offset: Number(offset), + limit, + offset, }), User.count({ where }), ]); diff --git a/src/generated/api.ts b/src/generated/api.ts index 7634e07..c765637 100644 --- a/src/generated/api.ts +++ b/src/generated/api.ts @@ -1487,10 +1487,17 @@ export interface paths { path?: never; cookie?: never; }; - /** List users (internal) */ + /** + * List users (internal) + * @description Returns a window of users. `total` counts every user matching `search`, not the returned page. + */ get: { parameters: { - query?: never; + query?: { + limit?: number; + offset?: number | null; + search?: string; + }; header?: never; path?: never; cookie?: never; @@ -1534,6 +1541,36 @@ export interface paths { }; }; }; + /** @description HTTP 400 */ + 400: { + headers: { + [name: string]: unknown; + }; + content: { + /** + * @example { + * "error": "string", + * "message": "string", + * "details": { + * "issues": [ + * null + * ] + * } + * } + */ + 'application/json': { + error: string; + message?: string; + details?: { + issues: { + path: (string | number)[]; + code: string; + message: string; + }[]; + }; + }; + }; + }; /** @description HTTP 429 */ 429: { headers: { diff --git a/src/routes/admin.routes.ts b/src/routes/admin.routes.ts index 9d7d680..6b9da88 100644 --- a/src/routes/admin.routes.ts +++ b/src/routes/admin.routes.ts @@ -33,7 +33,7 @@ import { import { createRouter } from '../lib/createRouter.js'; import { requireAdmin } from '../middleware/requireAdmin.js'; import { requireStepUp } from '../middleware/requireStepUp.js'; -import { UserIdParamSchema } from '../schemas/admin.query.js'; +import { AdminUserListQuerySchema, UserIdParamSchema } from '../schemas/admin.query.js'; import { CreateUserSchema, DeviceReplacementRecoverySchema, @@ -248,10 +248,13 @@ adminRouter.get( { auth: 'access', summary: 'List users (internal)', + description: + 'Returns a window of users. `total` counts every user matching `search`, not the returned page.', tags: ['Admin'], middleware: [requireAdmin('read')], schemas: { + query: AdminUserListQuerySchema, response: { 200: UsersListResponseSchema, 500: InternalErrorSchema, diff --git a/src/schemas/admin.query.ts b/src/schemas/admin.query.ts index 769c427..4c0dc79 100644 --- a/src/schemas/admin.query.ts +++ b/src/schemas/admin.query.ts @@ -4,4 +4,16 @@ * See LICENSE file in the project root for full license information */ +import { PaginationQuerySchema } from '@seamless-auth/types'; +import { z } from 'zod'; + export { UserIdParamSchema } from '@seamless-auth/types'; + +/** + * Kept local for the same reason the organization list query is: the response + * shape is already `{ users, total }`, so only this server's own list route + * needs the window. + */ +export const AdminUserListQuerySchema = PaginationQuerySchema.extend({ + search: z.string().trim().min(1).max(120).optional(), +}); diff --git a/tests/integration/admin/admin.spec.ts b/tests/integration/admin/admin.spec.ts index cfee6dc..71b21ec 100644 --- a/tests/integration/admin/admin.spec.ts +++ b/tests/integration/admin/admin.spec.ts @@ -782,6 +782,70 @@ describe('GET /admin/users (additional branches)', () => { expect(res.status).toBe(200); expect(res.body.users).toEqual([]); }); + + it('applies the default window when none is sent', async () => { + (User.findAll as any).mockResolvedValue([buildUser()]); + (User.count as any).mockResolvedValue(1); + + const res = await request(app).get('/admin/users'); + + expect(res.status).toBe(200); + expect(User.findAll).toHaveBeenCalledWith(expect.objectContaining({ limit: 50, offset: 0 })); + }); + + it('applies the window that was sent', async () => { + (User.findAll as any).mockResolvedValue([buildUser()]); + (User.count as any).mockResolvedValue(400); + + const res = await request(app).get('/admin/users').query({ limit: 25, offset: 100 }); + + expect(res.status).toBe(200); + // The count is of everything matching, not of the page, so the caller can + // tell there is more to ask for. + expect(res.body.total).toBe(400); + expect(User.findAll).toHaveBeenCalledWith(expect.objectContaining({ limit: 25, offset: 100 })); + }); + + it('trims the search term before matching', async () => { + (User.findAll as any).mockResolvedValue([buildUser()]); + (User.count as any).mockResolvedValue(1); + + const res = await request(app).get('/admin/users').query({ search: ' ada ' }); + + expect(res.status).toBe(200); + const where = (User.findAll as any).mock.calls[0][0].where; + expect(where[Op.or]).toEqual([ + { email: { [Op.iLike]: '%ada%' } }, + { phone: { [Op.iLike]: '%ada%' } }, + ]); + }); + + // Previously unvalidated: the window went to Sequelize as it arrived, so a + // limit of 100000 was honoured and a non-numeric one reached the database as + // NaN. + it('rejects a window outside the allowed range', async () => { + const res = await request(app).get('/admin/users').query({ limit: 500 }); + + expect(res.status).toBe(400); + expect(User.findAll).not.toHaveBeenCalled(); + }); + + it('rejects a non-numeric window', async () => { + const res = await request(app).get('/admin/users').query({ limit: 'all' }); + + expect(res.status).toBe(400); + expect(User.findAll).not.toHaveBeenCalled(); + }); + + // An all-whitespace term trims to nothing. Accepting it would build `%%`, + // which matches every row, so a search that looks empty would silently + // return the unfiltered list. + it('rejects a search term that is only whitespace', async () => { + const res = await request(app).get('/admin/users').query({ search: ' ' }); + + expect(res.status).toBe(400); + expect(User.findAll).not.toHaveBeenCalled(); + }); }); describe('POST /admin/users (additional branches)', () => {