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)', () => {