feat(admin): validate and document the window on the user list - #299
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the caveat that fells-code/seamless-auth-docs#42 had to write down:
GET /admin/userswas the one admin collection reading query parameters it did not declare.What changed
The route has always read
limit,offsetandsearch, but declared no query schema, so the parameters were absent fromopenapi.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 in the first place.They are declared now, so the generated contract lists them alongside
/admin/sessions,/admin/auth-eventsand/admin/organizations, and they are validated the same way:limit1 to 100 defaulting to 50,offsetfrom 0,searchtrimmed.Three inputs that used to be accepted now return 400
limitabove 100 was honoured in full, so one call could ask for every user in the deployment.limitreached Sequelize asNaNand failed in the database rather than at the edge.searchbuilt a%%pattern matching every row, so a filter that looked empty returned the unfiltered list.seamless-cliacceptsusers list --limit 0deliberately, meaning ask for nothing, and that value is now a 400. The changeset says so. The flag needs a floor of 1, or to skip the request when asked for zero rows. I have not touched the CLI.I kept the shared
PaginationQuerySchemabounds rather than special-casing this route to allowlimit=0, since a route that accepts a window the other three reject would recreate exactly the inconsistency this change removes.Something worth knowing before this ships
While checking which callers send a large
limit, I found that neither adapter forwards the query string onGET /admin/users. It is the only admin list route they drop it for. Verified empirically against the built Express adapter:In
packages/core/src/handlers/admin.tsthe users handler takesBaseOptswhere the other list handlers takeWithQuery, and neitherpackages/express/src/handlers/admin.tsnor the Fastify route table passesqueryfor it.The consequence is that the admin dashboard's user search and paging do nothing in production: it sends
?search=...&limit=10&offset=N, the adapter forwards a bare/admin/users, and the API answers with the default first 50. Its e2e tests pass because the Playwright mock intercepts at the dashboard boundary and never exercises the adapter.That is an adapter fix, in its own PR in
seamless-auth-server, and it is not blocked by this one. This change is a prerequisite for it being meaningful, and it is safe to land first: through the adapter no query arrives today, so nothing sees the new validation until the adapter starts forwarding.Verification
typecheck,lint,format:check,buildcleantest:run: 114 files, 1374 passing (up from 1364)coveragepasses the 98 percent line and 95 percent branch thresholdsopenapi.jsonregenerated;GET /admin/usersnow reports['limit', 'offset', 'search']