Skip to content

fix(api): Fix pagination limit, filters, and rows - #1115

Open
ShJ-code wants to merge 1 commit into
openstack-experimental:mainfrom
ShJ-code:fix/pagination-effective-limit
Open

fix(api): Fix pagination limit, filters, and rows#1115
ShJ-code wants to merge 1 commit into
openstack-experimental:mainfrom
ShJ-code:fix/pagination-effective-limit

Conversation

@ShJ-code

Copy link
Copy Markdown
Collaborator

Summary

Three defects in the shared v3/v4 pagination helpers (crates/keystone/src/api/common.rs),
which every paginated collection inherits. A prerequisite of #1035; split
out because none of it is policy-specific.

1. Per-resource list_limit was dead config. PaginationQuery::limit
defaulted to Some(20), so a request that omitted ?limit still arrived with
one. Config::resolve_list_limit only consults the configured values when the
request carries no limit, so every per-provider and global list_limit was
silently ignored — contrary to the precedence chain ADR 0029 documents ("user
limit → per-resource → global list_limitmax_db_limit"). The default is
now None, which also matches python keystone, whose [DEFAULT] list_limit is
unset out of the box.

2. The paginators used the raw client limit, not the effective one.
Handlers fetch resolve_list_limit(...) + 1 rows, but paginate_forward /
paginate_bidirectional compared against query.limit. A request for
limit=100 against a max_list_limit of 10 therefore returned all 11 fetched
rows — exceeding the cap — and emitted no next link, silently ending
pagination. Both paginators now resolve the effective limit themselves from the
provider's ListLimitConfig, so the fetch, the slice, and the link cannot
diverge. federation/identity_provider/list additionally passed the raw limit
to the backend and only read config afterwards, so an unbounded query read the
whole table before truncating; it now resolves up front. All 19 list handlers
were audited — federation was the only one.

3. next links dropped resource filters. build_pagination_link rebuilt
the href from the pagination controls alone, so following the link from
GET /v3/policies?type=application/json silently widened page two to the
unfiltered collection. Non-pagination query parameters are now carried over
verbatim; the paginators take the request URI rather than only its path.

Separately, a list endpoint that re-checks each item against the per-item read
policy (security model I8) cannot over-fetch limit + 1 rows just once: if any
are filtered out, the surviving count can fall to limit or below while more
visible rows remain, which the paginator reads as "no next page".
collect_authorized_page keeps pulling batches — advancing the marker over raw
rows — until limit + 1 authorized items are collected or the backend is
exhausted. identity/credential/list adopts it.

Behaviour change worth flagging

With no configuration, omitting ?limit now returns the full collection instead
of an implicit 20-item page. That is the ADR 0029 / python-keystone default;
deployments wanting a cap should set [DEFAULT] list_limit (which now actually
works).

Test plan

All measured at eb8a2b9d with only this commit applied (base upstream/main
8fc78201):

  • cargo test -p openstack-keystone --lib820 passed, 0 failed
  • cargo nextest run -p test_integration --profile ci194 passed, 106 raft-skipped
  • cargo nextest run --profile api -p test_api --test integration_api_v3117 passed, 0 failed
  • opa test policy341/341
  • cargo fmt --check, pre-commit run --all-files — clean
  • Security gates B1 / C / E / I / J — all pass

10 new tests, covering each defect:

Test Locks
test_paginate_forward_unlimited_by_default no implicit page size
test_paginate_forward_uses_provider_default_limit per-provider list_limit is live config
test_paginate_forward_clamps_to_max_list_limit cap governs slice and link
test_paginate_forward_link_keeps_filters_with_effective_limit filter + clamped limit coexist, no duplicate limit=
test_list_backend_gets_global_default_limit backend receives the resolved limit
test_list_backend_limit_clamped_to_max_db_limit clamp applied before the query
test_collect_authorized_page_refills_past_denied_items a fully-denied batch cannot truncate pagination
test_collect_authorized_page_stops_when_backend_exhausted no spin on a repeated marker
test_collect_authorized_page_propagates_authorize_errors a policy-engine failure is not a denial
test_collect_authorized_page_unlimited_filters_all unpaginated path still filters

Two pre-existing federation tests were updated: they asserted the backend saw
limit: Some(20) for requests sending no ?limit, which was the bug.

Security review checklist

Applies: the diff touches identity/credential/list, an I8 per-item
authorization handler.

  • Does any delegation/authorization decision read the scope where it should read the chain? (I1/I2) — No new authorization decision. Only how many items are checked and how links are built changed.
  • New delegation-sensitive policy rule? (I2/I3) — None. No .rego changes; opa test unchanged at 341/341.
  • New scope shape or redemption path for a delegated auth? (I4) — None.
  • New ScopeInfo variant or auth method? (I5, Gate J) — None. Gate J passes.
  • New lookup by a client-derivable key? (I6) — The marker is client-supplied, and collect_authorized_page advances it over raw rows including unauthorized ones. That marker is internal only: the emitted next link is built from items.last() of the authorized, truncated page, so no unauthorized object's id is ever disclosed.
  • Does any new data reaching OPA include secrets/decrypted blobs? (I7, Gate I) — No change to policy input. ResourceIdentifier is implemented on the domain credential specifically so credential_policy_input(&item) still receives the identical object; converting to the API type first would have altered it. Gate I passes.
  • New list/collection endpoint? Does it re-check each item? (I8) — No new endpoint; this strengthens I8. The per-item check is unchanged, and the refill loop fixes a case where filtering silently truncated the collection. test_list_denied_drops_all_items still passes through the new helper.
  • Does the change let a narrow auth method be broadened by a request-supplied scope? (I5) — No.
  • Are there negative tests proving the escape is blocked? — Yes: ..._propagates_authorize_errors (failure ≠ denial) and ..._refills_past_denied_items (denied rows excluded from results while still advancing the marker), plus the pre-existing test_list_denied_drops_all_items.
  • Does the test drive ValidatedSecurityContext::new_for_scope() end-to-end? — N/A; no scope resolution is involved.

@gtema gtema left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pretty interesting spot. Few comments though:

  • PR descritption claims new tests 4 of which are not present in the diff: test_collect_authorized_page_refills_past_denied_items, test_collect_authorized_page_stops_when_backend_exhausted, test_collect_authorized_page_propagates_authorize_errors, test_collect_authorized_page_unlimited_filters_all
  • pagination for credentials was previously skipping entries that are not allowed to be returned by the policy. Now it fetches next page(s) until it reaches the wanted page size - the mentioned tests would be really necessary. It is necessary to check all corner cases like all rows returned by backend are rejected by policy, second page returns less (or exactly equal) to the requested entities (verifying proper abort). It would be eventually also useful to implement a guard that guarantees no more than something like limit*2 of entries are tried before returning to prevent a full table scan. Also please add this behavior change into the PR description since it is a significant change.

Otherwise it is really a good change

Three defects in the shared v3/v4 pagination helpers, which every
paginated collection inherits.

`PaginationQuery::limit` defaulted to `Some(20)`, so a request that
omits `?limit` still arrived with one. `Config::resolve_list_limit` only
consults the configured values when the request carries no limit, so
every per-provider and global `list_limit` was dead config, contrary to
the precedence chain ADR 0029 documents. The default is now `None`,
which also matches python keystone, whose `[DEFAULT] list_limit` is
unset.

The paginators then read `query.limit` — the raw client value — while
the handler had fetched `resolve_list_limit(...) + 1` rows. A request
for `limit=100` against a `max_list_limit` of 10 therefore returned all
11 fetched rows, exceeding the cap, and emitted no `next` link. Both
paginators now resolve the effective limit themselves from the
provider's `ListLimitConfig`, so the slice, the link, and the fetch
cannot diverge.

`build_pagination_link` rebuilt the href from the pagination controls
alone, dropping every resource filter, so following the link from `GET
/v3/policies?type=application/json` silently widened page two to the
unfiltered collection. Non-pagination query parameters are now carried
over verbatim, and the paginators take the request URI rather than only
its path.

Separately, a list endpoint that re-checks each item against the
per-item read policy (security model I8) cannot over-fetch `limit + 1`
rows just once: if any are filtered out, the surviving count can fall to
`limit` or below while more visible rows remain, which the paginator
reads as "no next page". `collect_authorized_page` keeps pulling
batches, advancing the marker over raw rows, until `limit + 1`
authorized items are collected or the backend is exhausted.

`identity/credential/list` adopts it too. `ResourceIdentifier` is
implemented on the *domain* credential so the per-item check still
receives exactly the object it received before, leaving the policy input
for that endpoint unchanged.

That refill is bounded: at most `limit * AUTHORIZED_PAGE_SCAN_FACTOR`
(2) raw rows are examined per request. Without a bound, a caller who may
read only a sparse subset of a large collection would walk the whole
table on every request, which is a cheap way to force a full scan.

BEHAVIOUR CHANGE: `GET /v3/credentials` previously dropped the entries a
caller could not read and returned a short page, ending pagination
early. It now refills from subsequent rows up to the page size. A caller
that could see 3 of 20 credentials used to receive 3 items and no `next`
link; it now receives a full page and continues paging. Callers relying
on the old short pages will see more entries per response.

When the budget stops the scan before the page fills, the page is still
short but `next` is emitted anyway, keyed on the last authorized item,
so the caller keeps paging instead of treating it as the end of the
collection. If no authorized item was found at all there is no marker
that would not disclose an object the caller may not read, so no link is
emitted and the truncation is logged.

Signed-off-by: Sihao Jiang <sihao_jiang@outlook.com>
@ShJ-code
ShJ-code force-pushed the fix/pagination-effective-limit branch from 152b681 to d7f448b Compare July 30, 2026 01:08
let calls = calls as *const std::cell::Cell<usize>;
move |marker| {
// SAFETY-free: `calls` outlives the closure inside each test body.
unsafe { &*calls }.set(unsafe { &*calls }.get() + 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you found another issue - the crate does not apply overall workspace rules which forbid use of unsafe code (I will fix that). Rc<Cell> shared between the helper and the returned closure would avoid unsafe entirely and should be simpler.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my oversight and thanks for fixing the issue. Is there anything else that I should make a change? I will check the code against the safety standard later again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants