From fe7d2bbb8c097bf88438c363486fd2967c72cbac Mon Sep 17 00:00:00 2001 From: Sihao Jiang Date: Tue, 28 Jul 2026 17:35:03 -0400 Subject: [PATCH] fix(api): Fix pagination limit, filters, and rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. The `batched_fetch` test helper shares its call counter through an `Rc>` rather than casting a `&Cell` to a raw pointer. The returned closure owns its handle, so there is no borrow lifetime for the `impl FnMut` return type to name, and no `unsafe` block is needed. Signed-off-by: Sihao Jiang --- crates/api-types/src/lib.rs | 12 +- crates/core/src/api.rs | 23 +- crates/keystone/src/api/common.rs | 618 +++++++++++++++++- crates/keystone/src/api/mod.rs | 2 +- crates/keystone/src/api/v3/credential/list.rs | 91 ++- .../keystone/src/api/v3/credential/types.rs | 15 + crates/keystone/src/api/v3/domain/list.rs | 8 +- crates/keystone/src/api/v3/endpoint/list.rs | 9 +- crates/keystone/src/api/v3/group/list.rs | 8 +- crates/keystone/src/api/v3/project/list.rs | 8 +- crates/keystone/src/api/v3/region/list.rs | 8 +- crates/keystone/src/api/v3/role/list.rs | 8 +- .../src/api/v3/role_assignment/list.rs | 9 +- crates/keystone/src/api/v3/service/list.rs | 8 +- crates/keystone/src/api/v3/user/list.rs | 8 +- crates/keystone/src/api/v4/api_key/list.rs | 9 +- .../src/api/v4/mapping/ruleset/list.rs | 9 +- .../src/api/v4/oauth2/clients/list.rs | 9 +- crates/keystone/src/api/v4/scim_realm/list.rs | 9 +- .../src/api/v4/token/restriction/list.rs | 3 +- crates/keystone/src/api/v4/user/list.rs | 8 +- .../federation/api/identity_provider/list.rs | 113 +++- .../src/k8s_auth/api/instance/list.rs | 9 +- 23 files changed, 915 insertions(+), 89 deletions(-) diff --git a/crates/api-types/src/lib.rs b/crates/api-types/src/lib.rs index 4e0e4680e..1fe7d47ef 100644 --- a/crates/api-types/src/lib.rs +++ b/crates/api-types/src/lib.rs @@ -64,9 +64,17 @@ pub fn default_true() -> bool { true } -/// Default page size applied when the client does not supply `limit`. +/// Default `limit` when the client does not supply one: **`None`**. +/// +/// Deliberately not a hard-coded page size. ADR 0029's precedence chain is +/// "user limit -> per-resource `list_limit` -> global `[DEFAULT] list_limit` +/// -> `max_db_limit`", and `Config::resolve_list_limit` only consults the +/// configured values when the request carries no limit. Returning `Some(20)` +/// here made `requested` always populated, which silently rendered every +/// `list_limit` setting dead config. `None` also matches python keystone, +/// whose `[DEFAULT] list_limit` is unset (no truncation) out of the box. pub fn default_list_limit() -> Option { - Some(20) + None } /// Shared pagination query parameters, reused by every v3/v4 list endpoint. diff --git a/crates/core/src/api.rs b/crates/core/src/api.rs index 91b6a9c1d..d185b43c4 100644 --- a/crates/core/src/api.rs +++ b/crates/core/src/api.rs @@ -188,6 +188,27 @@ pub mod tests { provider_builder: ProviderBuilder, policy_allow: bool, policy_allow_see_other_domains: Option, + ) -> ServiceState { + get_mocked_state_with_config( + provider_builder, + policy_allow, + policy_allow_see_other_domains, + Config::default(), + ) + .await + } + + /// Like [`get_mocked_state`], but with a caller-supplied [`Config`]. + /// + /// Needed to exercise anything the configuration governs — notably the + /// `list_limit`/`max_db_limit` pagination chain, which `Config::default()` + /// leaves entirely unset and therefore cannot distinguish a resolved + /// limit from the raw request value. + pub async fn get_mocked_state_with_config( + provider_builder: ProviderBuilder, + policy_allow: bool, + policy_allow_see_other_domains: Option, + config: Config, ) -> ServiceState { let provider = provider_builder.build().unwrap(); @@ -213,7 +234,7 @@ pub mod tests { Arc::new( Service::new( - ConfigManager::not_watched(Config::default()), + ConfigManager::not_watched(config), DatabaseConnection::default(), provider, Arc::new(policy_enforcer_mock), diff --git a/crates/keystone/src/api/common.rs b/crates/keystone/src/api/common.rs index 4f647ab12..c02565838 100644 --- a/crates/keystone/src/api/common.rs +++ b/crates/keystone/src/api/common.rs @@ -12,13 +12,14 @@ // // SPDX-License-Identifier: Apache-2.0 //! # Common API helpers +use std::future::Future; use std::net::SocketAddr; -use axum::{extract::FromRequestParts, http::request::Parts}; -use url::Url; +use axum::{extract::FromRequestParts, http::Uri, http::request::Parts}; +use url::{Url, form_urlencoded}; use openstack_keystone_api_types::{Link, PaginationQuery}; -use openstack_keystone_config::Config; +use openstack_keystone_config::{Config, ListLimitConfig}; use openstack_keystone_core::net::public_ingress_peer_addr; use openstack_keystone_core_types::resource::Domain; @@ -160,12 +161,20 @@ pub trait ResourceIdentifier { fn get_id(&self) -> String; } +/// Pagination controls, which a generated link replaces rather than inherits. +const PAGINATION_PARAMS: [&str; 3] = ["limit", "marker", "page_reverse"]; + /// Build a single pagination `Link`, pointing `collection_url` at a new /// `marker`/`page_reverse` combination derived from `query`. +/// +/// Every **non-pagination** query parameter on `collection_url` is carried +/// over verbatim, so a filtered collection stays filtered across pages. Losing +/// them would silently widen the next page from, say, +/// `GET /v3/policies?type=application/json` to the unfiltered collection. fn build_pagination_link( config: &Config, - query: &PaginationQuery, - collection_url: &str, + limit: u64, + collection_url: &Uri, rel: &str, marker: String, page_reverse: bool, @@ -175,14 +184,41 @@ fn build_pagination_link( } else { Url::parse("http://localhost")? }; - url.set_path(collection_url); + url.set_path(collection_url.path()); let new_query = PaginationQuery { - limit: query.limit, + limit: Some(limit), marker: Some(marker), page_reverse, }; - url.set_query(Some(&serde_urlencoded::to_string(&new_query)?)); + + // Resource filters first, then the (authoritative) pagination controls. + let mut serialized = String::new(); + for (key, value) in collection_url + .query() + .map(|q| form_urlencoded::parse(q.as_bytes()).collect::>()) + .unwrap_or_default() + { + if PAGINATION_PARAMS.contains(&key.as_ref()) { + continue; + } + if !serialized.is_empty() { + serialized.push('&'); + } + serialized.push_str( + &form_urlencoded::Serializer::new(String::new()) + .append_pair(&key, &value) + .finish(), + ); + } + let pagination = serde_urlencoded::to_string(&new_query)?; + if !pagination.is_empty() { + if !serialized.is_empty() { + serialized.push('&'); + } + serialized.push_str(&pagination); + } + url.set_query(Some(&serialized)); let href = format!( "{}{}", @@ -195,6 +231,183 @@ fn build_pagination_link( }) } +/// How many raw backend rows [`collect_authorized_page`] may examine per +/// request, as a multiple of the requested page size. +/// +/// Bounds the cost of the per-item authorization refill loop. `2` means a +/// request for 10 items never inspects more than 20 rows, so a caller with +/// sparse visibility cannot turn one request into a full table scan. +pub const AUTHORIZED_PAGE_SCAN_FACTOR: u64 = 2; + +/// Outcome of [`collect_authorized_page`]. +#[derive(Debug)] +pub struct AuthorizedPage { + /// The authorized items, up to `limit + 1` of them. + pub items: Vec, + /// `true` when the scan stopped because the examine budget ran out, rather + /// than because the page filled or the backend was exhausted. More visible + /// rows may remain even when `items` is shorter than the page size. + pub scan_budget_exhausted: bool, +} + +/// Fill a page with items the caller is actually allowed to see. +/// +/// A list endpoint that re-checks every item against the per-item read policy +/// (security model I8) cannot just over-fetch `limit + 1` rows once: if any of +/// them are filtered out, the surviving count can drop to `limit` or below +/// while more *visible* rows still exist. [`paginate_forward`] then reads that +/// as "no next page" and pagination stops early, silently hiding the tail of +/// the collection. +/// +/// This keeps pulling backend batches — advancing the marker past the last +/// **raw** row of each batch, authorized or not — until `limit + 1` authorized +/// items are collected, the backend runs out, or the scan budget is spent. The +/// `limit + 1`th item is the over-fetch sentinel [`paginate_forward`] expects. +/// +/// # Scan budget +/// +/// Refilling is bounded: at most `limit * `[`AUTHORIZED_PAGE_SCAN_FACTOR`] 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 entire table on every +/// request — a cheap way to force a full scan. When the budget is spent before +/// the page fills, [`AuthorizedPage::scan_budget_exhausted`] is set so the +/// caller can still advertise a `next` link (see +/// [`paginate_forward_filtered`]) instead of reporting a short page as the end +/// of the collection. +/// +/// `authorize` takes ownership and returns `Ok(None)` to drop an item; any +/// `Err` is propagated, so a policy-engine failure fails the request instead of +/// being mistaken for a denial. +pub async fn collect_authorized_page( + limit: Option, + initial_marker: Option, + mut fetch: Fetch, + mut authorize: Authorize, +) -> Result, KeystoneApiError> +where + T: ResourceIdentifier, + Fetch: FnMut(Option) -> FetchFut, + FetchFut: Future, KeystoneApiError>>, + Authorize: FnMut(T) -> AuthorizeFut, + AuthorizeFut: Future, KeystoneApiError>>, +{ + let Some(limit) = limit else { + // Unpaginated: a single batch is the whole collection, so there is no + // page to refill and no budget to spend. + let raw = fetch(initial_marker).await?; + let mut items = Vec::with_capacity(raw.len()); + for item in raw { + if let Some(item) = authorize(item).await? { + items.push(item); + } + } + return Ok(AuthorizedPage { + items, + scan_budget_exhausted: false, + }); + }; + + // One past `limit` so `paginate_forward` can tell "there is a next page" + // exactly rather than guessing. + let wanted = limit as usize + 1; + // Never below `wanted`, or a single batch could not even be consumed. + let budget = limit + .saturating_mul(AUTHORIZED_PAGE_SCAN_FACTOR) + .max(wanted as u64) as usize; + + let mut items: Vec = Vec::with_capacity(wanted); + let mut examined = 0usize; + let mut marker = initial_marker; + + loop { + let raw = fetch(marker.clone()).await?; + let fetched = raw.len(); + let Some(last_raw_id) = raw.last().map(ResourceIdentifier::get_id) else { + break; + }; + + for item in raw { + examined += 1; + if let Some(item) = authorize(item).await? { + items.push(item); + if items.len() >= wanted { + return Ok(AuthorizedPage { + items, + scan_budget_exhausted: false, + }); + } + } + if examined >= budget { + // Out of budget with the page unfilled: report it so the + // caller advertises a `next` link rather than presenting a + // short page as the end of the collection. + return Ok(AuthorizedPage { + items, + scan_budget_exhausted: true, + }); + } + } + + // The backend also over-fetches by one, so a batch smaller than + // `wanted` means it has nothing left behind this one. + if fetched < wanted { + break; + } + marker = Some(last_raw_id); + } + + Ok(AuthorizedPage { + items, + scan_budget_exhausted: false, + }) +} + +/// [`paginate_forward`] for a page produced by [`collect_authorized_page`]. +/// +/// Adds one thing the plain paginator cannot know: when the refill loop stopped +/// on its scan budget, the page can be *shorter* than the limit while more +/// visible rows still exist. Reporting that as the end of the collection is the +/// very truncation bug the refill loop exists to fix, so a `next` link is +/// emitted anyway, keyed on the last authorized item. +/// +/// If the budget ran out before a single authorized item was found there is no +/// safe marker to hand back — the last row examined belongs to an object the +/// caller may not read, and putting its ID in a link would disclose it — so no +/// link is emitted and the truncation is logged. A caller in that position is +/// one whose visible rows are sparser than the entire budget window. +pub fn paginate_forward_filtered( + config: &Config, + provider_limit: &ListLimitConfig, + page: AuthorizedPage, + query: &PaginationQuery, + collection_url: &Uri, +) -> Result<(Vec, Option>), KeystoneApiError> { + let AuthorizedPage { + items, + scan_budget_exhausted, + } = page; + + let (items, links) = paginate_forward(config, provider_limit, items, query, collection_url)?; + + if !scan_budget_exhausted || links.is_some() { + return Ok((items, links)); + } + + let Some(last) = items.last() else { + tracing::warn!( + "per-item authorization scan budget exhausted with no authorized \ + rows; the response may omit visible entries beyond this point" + ); + return Ok((items, None)); + }; + + let limit = config + .resolve_list_limit(provider_limit, query.limit) + .unwrap_or_default(); + let link = build_pagination_link(config, limit, collection_url, "next", last.get_id(), false)?; + Ok((items, Some(vec![link]))) +} + /// Paginate a forward-only (v3, python-keystone compatible) list response. /// /// The backend is expected to have over-fetched by one row (`limit + 1`) so @@ -207,11 +420,17 @@ fn build_pagination_link( /// python-keystone behavior (its `previous` is always `null`). pub fn paginate_forward( config: &Config, + provider_limit: &ListLimitConfig, mut items: Vec, query: &PaginationQuery, - collection_url: &str, + collection_url: &Uri, ) -> Result<(Vec, Option>), KeystoneApiError> { - let Some(limit) = query.limit else { + // Resolve the limit here rather than trusting `query.limit`: the handler + // fetched `effective + 1` rows, so slicing and link generation must use + // that same effective value. Reading the raw client value instead let a + // request over `max_list_limit` return more rows than the cap allows and + // suppressed its `next` link. + let Some(limit) = config.resolve_list_limit(provider_limit, query.limit) else { return Ok((items, None)); }; @@ -224,7 +443,7 @@ pub fn paginate_forward( items .last() .map(|last| { - build_pagination_link(config, query, collection_url, "next", last.get_id(), false) + build_pagination_link(config, limit, collection_url, "next", last.get_id(), false) }) .transpose()? .map(|link| vec![link]) @@ -250,11 +469,13 @@ pub fn paginate_forward( /// the original `marker` with `page_reverse: false` — no extra lookup needed. pub fn paginate_bidirectional( config: &Config, + provider_limit: &ListLimitConfig, mut items: Vec, query: &PaginationQuery, - collection_url: &str, + collection_url: &Uri, ) -> Result<(Vec, Option>), KeystoneApiError> { - let Some(limit) = query.limit else { + // See `paginate_forward`: the effective limit, not the raw client value. + let Some(limit) = config.resolve_list_limit(provider_limit, query.limit) else { return Ok((items, None)); }; @@ -269,7 +490,7 @@ pub fn paginate_bidirectional( if has_previous && let Some(first) = items.first() { links.push(build_pagination_link( config, - query, + limit, collection_url, "previous", first.get_id(), @@ -281,7 +502,7 @@ pub fn paginate_bidirectional( if let Some(marker) = &query.marker { links.push(build_pagination_link( config, - query, + limit, collection_url, "next", marker.clone(), @@ -296,7 +517,7 @@ pub fn paginate_bidirectional( if has_next && let Some(last) = items.last() { links.push(build_pagination_link( config, - query, + limit, collection_url, "next", last.get_id(), @@ -309,7 +530,7 @@ pub fn paginate_bidirectional( { links.push(build_pagination_link( config, - query, + limit, collection_url, "previous", first.get_id(), @@ -323,6 +544,9 @@ pub fn paginate_bidirectional( #[cfg(test)] mod tests { + use std::cell::Cell; + use std::rc::Rc; + use rstest::rstest; use openstack_keystone_config::Config; @@ -393,6 +617,346 @@ mod tests { } } + /// Fetch `all[start..]` in fixed-size batches, counting calls. + /// + /// The counter is shared through an `Rc` so the returned closure can *own* + /// its handle. Taking `&Cell` instead would tie the closure to that + /// borrow's lifetime, which the `impl FnMut` return type cannot name. + fn batched_fetch( + all: &[&'static str], + batch: usize, + calls: Rc>, + ) -> impl FnMut(Option) -> std::future::Ready, KeystoneApiError>> + { + let all: Vec = all.iter().map(|s| (*s).to_string()).collect(); + move |marker| { + calls.set(calls.get() + 1); + let start = match &marker { + None => 0, + Some(m) => all.iter().position(|id| id == m).map_or(0, |i| i + 1), + }; + let rows = all + .iter() + .skip(start) + .take(batch) + .map(|id| FakeResource { id: id.clone() }) + .collect(); + std::future::ready(Ok(rows)) + } + } + + fn allow_except( + denied: &'static [&'static str], + ) -> impl FnMut(FakeResource) -> std::future::Ready, KeystoneApiError>> + { + move |item: FakeResource| { + std::future::ready(Ok(if denied.contains(&item.id.as_str()) { + None + } else { + Some(item) + })) + } + } + + /// `collect_authorized_page` keeps pulling batches until it has `limit + 1` + /// *authorized* items, so denied rows cannot truncate pagination. Without + /// the refill loop this returned a short page and no `next` link, stranding + /// the visible rows behind the denied ones. + #[tokio::test] + async fn test_collect_authorized_page_refills_past_denied_items() { + const ALL: &[&str] = &["1", "2", "3", "4", "5", "6", "7", "8", "9"]; + let calls = Rc::new(Cell::new(0usize)); + + // limit 2 -> wants 3, budget 4. Row "1" is denied, so the first batch + // of 3 yields only 2 authorized items and a second batch is needed. + let page = collect_authorized_page( + Some(2), + None, + batched_fetch(ALL, 3, calls.clone()), + allow_except(&["1"]), + ) + .await + .unwrap(); + + assert_eq!( + page.items.iter().map(|i| i.id.as_str()).collect::>(), + vec!["2", "3", "4"], + "must refill to limit + 1 authorized items across batches" + ); + assert!(!page.scan_budget_exhausted); + assert_eq!(calls.get(), 2, "the denied row forced exactly one refill"); + } + + /// Reviewer corner case: *every* row the backend returns is rejected. The + /// page is empty and the scan is reported as budget-limited rather than as + /// a genuine end-of-collection. + #[tokio::test] + async fn test_collect_authorized_page_all_rows_denied() { + const ALL: &[&str] = &["1", "2", "3", "4", "5", "6", "7", "8"]; + let calls = Rc::new(Cell::new(0usize)); + + let page = collect_authorized_page( + Some(2), + None, + batched_fetch(ALL, 3, calls.clone()), + allow_except(&["1", "2", "3", "4", "5", "6", "7", "8"]), + ) + .await + .unwrap(); + + assert!(page.items.is_empty(), "nothing is authorized"); + assert!( + page.scan_budget_exhausted, + "an all-denied scan must not look like the end of the collection" + ); + } + + /// Reviewer corner case: the second batch comes back *shorter* than + /// requested, which means the backend is exhausted — the loop must abort + /// rather than keep re-fetching. + #[tokio::test] + async fn test_collect_authorized_page_aborts_when_second_batch_is_short() { + const ALL: &[&str] = &["1", "2", "3", "4"]; + let calls = Rc::new(Cell::new(0usize)); + + // limit 3 -> wants 4, batch 4: first batch returns 4 (== wanted, so + // "maybe more"), second returns 0 and ends the loop. + let page = collect_authorized_page( + Some(3), + None, + batched_fetch(ALL, 4, calls.clone()), + allow_except(&["1"]), + ) + .await + .unwrap(); + + assert_eq!( + page.items.iter().map(|i| i.id.as_str()).collect::>(), + vec!["2", "3", "4"], + "only the authorized rows, and fewer than the page size" + ); + assert!( + !page.scan_budget_exhausted, + "the backend really is exhausted, so this is a true final page" + ); + assert_eq!(calls.get(), 2, "one refill, then the empty batch stops it"); + } + + /// Reviewer corner case: every batch comes back *exactly* `limit + 1` long, + /// so "maybe more" is always true. The budget — not the data — must end the + /// loop, and it must do so after a bounded number of rows. + #[tokio::test] + async fn test_collect_authorized_page_budget_stops_endless_refill() { + const ALL: &[&str] = &[ + "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", + ]; + let calls = Rc::new(Cell::new(0usize)); + let examined = Cell::new(0usize); + + let limit = 3u64; + let page = collect_authorized_page( + Some(limit), + None, + batched_fetch(ALL, limit as usize + 1, calls.clone()), + |_item: FakeResource| { + examined.set(examined.get() + 1); + // Deny everything, so only the budget can end the loop. + std::future::ready(Ok(None)) + }, + ) + .await + .unwrap(); + + assert!(page.items.is_empty()); + assert!(page.scan_budget_exhausted, "the budget must stop the loop"); + let budget = (limit * AUTHORIZED_PAGE_SCAN_FACTOR) as usize; + assert_eq!( + examined.get(), + budget, + "must examine exactly the budget, never the whole table" + ); + assert!( + examined.get() < ALL.len(), + "a full table scan is what the budget prevents" + ); + } + + /// A budget-truncated short page still advertises a `next` link, keyed on + /// the last authorized item — otherwise the caller would treat the short + /// page as the end of the collection. + #[test] + fn test_paginate_forward_filtered_links_after_budget_truncation() { + let page = AuthorizedPage { + items: fake_items(2), + scan_budget_exhausted: true, + }; + let (items, links) = paginate_forward_filtered( + &Config::default(), + &ListLimitConfig { + list_limit: None, + max_list_limit: Some(10), + }, + page, + &PaginationQuery { + limit: Some(10), + ..Default::default() + }, + &"/v3/credentials?type=ec2&limit=10".parse::().unwrap(), + ) + .unwrap(); + + assert_eq!(items.len(), 2, "the short page is returned as-is"); + let href = &links.expect("a next link is required")[0].href; + // `fake_items` is 0-indexed, so the last of two items is "1". + assert!( + href.contains("marker=1"), + "keyed on the last authorized item: {href}" + ); + assert!(href.contains("type=ec2"), "filter preserved: {href}"); + } + + /// With no authorized item there is no marker that does not disclose an + /// object the caller may not read, so no link is emitted. + #[test] + fn test_paginate_forward_filtered_no_link_without_authorized_items() { + let page: AuthorizedPage = AuthorizedPage { + items: Vec::new(), + scan_budget_exhausted: true, + }; + let (items, links) = paginate_forward_filtered( + &Config::default(), + &ListLimitConfig::default(), + page, + &PaginationQuery { + limit: Some(5), + ..Default::default() + }, + &"/v3/credentials".parse::().unwrap(), + ) + .unwrap(); + + assert!(items.is_empty()); + assert_eq!(links, None, "no marker can be published safely"); + } + + /// With no `?limit` and no configuration, nothing is truncated and no + /// links are emitted — matching python keystone, whose `[DEFAULT] + /// list_limit` is unset out of the box. + #[test] + fn test_paginate_forward_unlimited_by_default() { + let query = PaginationQuery::default(); + assert_eq!( + query.limit, None, + "the serde default must not inject a page size" + ); + + let (items, links) = paginate_forward( + &Config::default(), + &ListLimitConfig::default(), + fake_items(50), + &query, + &"/foo/bar".parse::().unwrap(), + ) + .unwrap(); + + assert_eq!(items.len(), 50); + assert_eq!(links, None); + } + + /// A per-provider `list_limit` governs the page size when the client + /// omits `limit`. Before the effective-limit fix the serde default of + /// `Some(20)` shadowed `requested`, so this setting was dead config. + #[test] + fn test_paginate_forward_uses_provider_default_limit() { + let provider_limit = ListLimitConfig { + list_limit: Some(2), + max_list_limit: None, + }; + // The backend over-fetches `limit + 1`. + let (items, links) = paginate_forward( + &Config::default(), + &provider_limit, + fake_items(3), + &PaginationQuery::default(), + &"/foo/bar".parse::().unwrap(), + ) + .unwrap(); + + assert_eq!(items.len(), 2, "page must honour the configured list_limit"); + assert!(links.is_some(), "a next link is required"); + } + + /// A client `limit` above `max_list_limit` is clamped, and the clamp + /// governs both the slice and the link. Reading the raw client value + /// returned more rows than the cap and suppressed the `next` link. + #[test] + fn test_paginate_forward_clamps_to_max_list_limit() { + let provider_limit = ListLimitConfig { + list_limit: None, + max_list_limit: Some(10), + }; + let query = PaginationQuery { + limit: Some(100), + ..Default::default() + }; + + // The handler resolved the same effective limit (10) and fetched 11. + let (items, links) = paginate_forward( + &Config::default(), + &provider_limit, + fake_items(11), + &query, + &"/foo/bar".parse::().unwrap(), + ) + .unwrap(); + + assert_eq!(items.len(), 10, "must not exceed max_list_limit"); + let links = links.expect("a next link is required when rows remain"); + assert!( + links[0].href.contains("limit=10"), + "the link must advertise the clamped limit, not the request's 100: {}", + links[0].href + ); + } + + /// Resource filters survive into the next link *and* coexist with the + /// clamped pagination controls. + #[test] + fn test_paginate_forward_link_keeps_filters_with_effective_limit() { + let provider_limit = ListLimitConfig { + list_limit: None, + max_list_limit: Some(2), + }; + let query = PaginationQuery { + limit: Some(50), + ..Default::default() + }; + + let (_, links) = paginate_forward( + &Config::default(), + &provider_limit, + fake_items(3), + &query, + &"/v3/policies?type=application%2Fjson&limit=50" + .parse::() + .unwrap(), + ) + .unwrap(); + + let href = &links.expect("next link")[0].href; + assert!( + href.contains("type=application%2Fjson"), + "filter kept: {href}" + ); + assert!(href.contains("limit=2"), "clamped limit: {href}"); + assert_eq!( + href.matches("limit=").count(), + 1, + "no duplicate limit: {href}" + ); + assert!(href.contains("marker="), "marker present: {href}"); + } + /// Fake resource for pagination testing. struct FakeResource { pub id: String, @@ -443,8 +1007,14 @@ mod tests { #[case] expected_len: usize, #[case] expected_links: Option>, ) { - let (items, links) = - paginate_forward(&Config::default(), fake_items(cnt), &query, "foo/bar").unwrap(); + let (items, links) = paginate_forward( + &Config::default(), + &ListLimitConfig::default(), + fake_items(cnt), + &query, + &"/foo/bar".parse::().unwrap(), + ) + .unwrap(); assert_eq!(items.len(), expected_len); assert_eq!(links, expected_links); } @@ -478,8 +1048,14 @@ mod tests { #[case] expected_len: usize, #[case] expected_links: Option>, ) { - let (items, links) = - paginate_bidirectional(&Config::default(), fake_items(cnt), &query, "foo/bar").unwrap(); + let (items, links) = paginate_bidirectional( + &Config::default(), + &ListLimitConfig::default(), + fake_items(cnt), + &query, + &"/foo/bar".parse::().unwrap(), + ) + .unwrap(); assert_eq!(items.len(), expected_len); assert_eq!(links, expected_links); } diff --git a/crates/keystone/src/api/mod.rs b/crates/keystone/src/api/mod.rs index 1699d85f6..670cee528 100644 --- a/crates/keystone/src/api/mod.rs +++ b/crates/keystone/src/api/mod.rs @@ -122,7 +122,7 @@ async fn version( pub(crate) mod tests { pub use openstack_keystone_core::api::policy_contract; pub use openstack_keystone_core::api::tests::{ - get_capturing_state, get_mocked_state, test_fixture_scoped, + get_capturing_state, get_mocked_state, get_mocked_state_with_config, test_fixture_scoped, }; use std::net::SocketAddr; diff --git a/crates/keystone/src/api/v3/credential/list.rs b/crates/keystone/src/api/v3/credential/list.rs index deec33d07..2049de8df 100644 --- a/crates/keystone/src/api/v3/credential/list.rs +++ b/crates/keystone/src/api/v3/credential/list.rs @@ -25,7 +25,7 @@ use openstack_keystone_core_types::ListPagination; use super::types::{Credential, CredentialList, CredentialListParameters}; use crate::api::auth::Auth; -use crate::api::common::paginate_forward; +use crate::api::common::{AuthorizedPage, collect_authorized_page, paginate_forward_filtered}; use crate::api::error::KeystoneApiError; use crate::keystone::ServiceState; use openstack_keystone_core::auth::ExecutionContext; @@ -71,43 +71,64 @@ pub(super) async fn list( .await?; let config = state.config_manager.config.read().await; - let mut provider_params = + let base_params = openstack_keystone_core_types::credential::CredentialListParameters::from(query); - provider_params.pagination = ListPagination { - limit: config.resolve_list_limit(&config.credential.list_limit, pagination.limit), - marker: pagination.marker.clone(), - page_reverse: false, + let limit = config.resolve_list_limit(&config.credential.list_limit, pagination.limit); + + // Bind by reference: `collect_authorized_page` takes `FnMut`, so the + // per-batch future must capture a `Copy` handle rather than move the + // context out of the closure. + let provider = state.provider.get_credential_provider(); + let exec_ctx = ExecutionContext::from_auth(&state, &user_auth); + let exec = &exec_ctx; + let enforcer = &state.policy_enforcer; + let auth = &user_auth; + + let page = collect_authorized_page( + limit, + pagination.marker.clone(), + |marker| { + let mut params = base_params.clone(); + params.pagination = ListPagination { + limit, + marker, + page_reverse: false, + }; + async move { Ok(provider.list_credentials(exec, ¶ms).await?) } + }, + |item: openstack_keystone_core_types::credential::Credential| async move { + match enforcer + .enforce( + "identity/credential/show", + auth, + serde_json::Value::Null, + Some(super::credential_policy_input(&item)), + ) + .await + { + Ok(_) => Ok(Some(item)), + Err(PolicyError::Forbidden(_)) => Ok(None), + Err(err) => Err(err.into()), + } + }, + ) + .await?; + let page = AuthorizedPage { + items: page + .items + .into_iter() + .map(Credential::from) + .collect::>(), + scan_budget_exhausted: page.scan_budget_exhausted, }; - let raw = state - .provider - .get_credential_provider() - .list_credentials( - &ExecutionContext::from_auth(&state, &user_auth), - &provider_params, - ) - .await?; - - let mut credentials = Vec::with_capacity(raw.len()); - for item in raw { - match state - .policy_enforcer - .enforce( - "identity/credential/show", - &user_auth, - serde_json::Value::Null, - Some(super::credential_policy_input(&item)), - ) - .await - { - Ok(_) => credentials.push(Credential::from(item)), - Err(PolicyError::Forbidden(_)) => continue, - Err(err) => return Err(err.into()), - } - } - - let (credentials, links) = - paginate_forward(&config, credentials, &pagination, original_url.path())?; + let (credentials, links) = paginate_forward_filtered( + &config, + &config.credential.list_limit, + page, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(CredentialList { credentials, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/credential/types.rs b/crates/keystone/src/api/v3/credential/types.rs index 79d9f38f9..4cad5f5c7 100644 --- a/crates/keystone/src/api/v3/credential/types.rs +++ b/crates/keystone/src/api/v3/credential/types.rs @@ -19,3 +19,18 @@ impl crate::api::common::ResourceIdentifier for Credential { self.id.clone() } } + +/// Marker for the *domain* credential so `collect_authorized_page` can advance +/// its marker over raw backend rows. +/// +/// Deliberately implemented on the domain type rather than converting rows to +/// the API type before the per-item check: `credential_policy_input` must keep +/// receiving exactly the object it received before, so adopting the shared +/// page-filler changes no policy input. +impl crate::api::common::ResourceIdentifier + for openstack_keystone_core_types::credential::Credential +{ + fn get_id(&self) -> String { + self.id.clone() + } +} diff --git a/crates/keystone/src/api/v3/domain/list.rs b/crates/keystone/src/api/v3/domain/list.rs index f35cd7d73..bcd6fd765 100644 --- a/crates/keystone/src/api/v3/domain/list.rs +++ b/crates/keystone/src/api/v3/domain/list.rs @@ -84,7 +84,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (domains, links) = paginate_forward(&config, domains, &pagination, original_url.path())?; + let (domains, links) = paginate_forward( + &config, + &config.resource.list_limit, + domains, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(DomainList { domains, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/endpoint/list.rs b/crates/keystone/src/api/v3/endpoint/list.rs index c5e0ee52a..3201ed412 100644 --- a/crates/keystone/src/api/v3/endpoint/list.rs +++ b/crates/keystone/src/api/v3/endpoint/list.rs @@ -81,8 +81,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (endpoints, links) = - paginate_forward(&config, endpoints, &pagination, original_url.path())?; + let (endpoints, links) = paginate_forward( + &config, + &config.catalog.list_limit, + endpoints, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(EndpointList { endpoints, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/group/list.rs b/crates/keystone/src/api/v3/group/list.rs index 9009d5c78..726ade85b 100644 --- a/crates/keystone/src/api/v3/group/list.rs +++ b/crates/keystone/src/api/v3/group/list.rs @@ -81,7 +81,13 @@ pub async fn list( .map(Into::into) .collect(); - let (groups, links) = paginate_forward(&config, groups, &pagination, original_url.path())?; + let (groups, links) = paginate_forward( + &config, + &config.identity.list_limit, + groups, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(GroupList { groups, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/project/list.rs b/crates/keystone/src/api/v3/project/list.rs index 89cf7092c..c63e2ff5b 100644 --- a/crates/keystone/src/api/v3/project/list.rs +++ b/crates/keystone/src/api/v3/project/list.rs @@ -84,7 +84,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (projects, links) = paginate_forward(&config, projects, &pagination, original_url.path())?; + let (projects, links) = paginate_forward( + &config, + &config.resource.list_limit, + projects, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(ProjectShortList { projects, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/region/list.rs b/crates/keystone/src/api/v3/region/list.rs index 766386e94..068442234 100644 --- a/crates/keystone/src/api/v3/region/list.rs +++ b/crates/keystone/src/api/v3/region/list.rs @@ -81,7 +81,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (regions, links) = paginate_forward(&config, regions, &pagination, original_url.path())?; + let (regions, links) = paginate_forward( + &config, + &config.catalog.list_limit, + regions, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(RegionList { regions, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/role/list.rs b/crates/keystone/src/api/v3/role/list.rs index 445931557..77553a591 100644 --- a/crates/keystone/src/api/v3/role/list.rs +++ b/crates/keystone/src/api/v3/role/list.rs @@ -80,7 +80,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (roles, links) = paginate_forward(&config, roles, &pagination, original_url.path())?; + let (roles, links) = paginate_forward( + &config, + &config.role.list_limit, + roles, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(RoleList { roles, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/role_assignment/list.rs b/crates/keystone/src/api/v3/role_assignment/list.rs index 9de489aa3..fd3672840 100644 --- a/crates/keystone/src/api/v3/role_assignment/list.rs +++ b/crates/keystone/src/api/v3/role_assignment/list.rs @@ -85,8 +85,13 @@ pub(super) async fn list( ) .await?; - let (raw_assignments, links) = - paginate_forward(&config, raw_assignments, &pagination, original_url.path())?; + let (raw_assignments, links) = paginate_forward( + &config, + &config.assignment.list_limit, + raw_assignments, + &pagination, + &original_url, + )?; let assignments: Result, _> = raw_assignments.into_iter().map(TryInto::try_into).collect(); diff --git a/crates/keystone/src/api/v3/service/list.rs b/crates/keystone/src/api/v3/service/list.rs index 2f9535253..599895ce3 100644 --- a/crates/keystone/src/api/v3/service/list.rs +++ b/crates/keystone/src/api/v3/service/list.rs @@ -81,7 +81,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (services, links) = paginate_forward(&config, services, &pagination, original_url.path())?; + let (services, links) = paginate_forward( + &config, + &config.catalog.list_limit, + services, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(ServiceList { services, links })).into_response()) } diff --git a/crates/keystone/src/api/v3/user/list.rs b/crates/keystone/src/api/v3/user/list.rs index 5ec4cdf75..01abfda93 100644 --- a/crates/keystone/src/api/v3/user/list.rs +++ b/crates/keystone/src/api/v3/user/list.rs @@ -84,7 +84,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (users, links) = paginate_forward(&config, users, &pagination, original_url.path())?; + let (users, links) = paginate_forward( + &config, + &config.identity.list_limit, + users, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(UserList { users, links })).into_response()) } diff --git a/crates/keystone/src/api/v4/api_key/list.rs b/crates/keystone/src/api/v4/api_key/list.rs index 90153f049..7cbcf1514 100644 --- a/crates/keystone/src/api/v4/api_key/list.rs +++ b/crates/keystone/src/api/v4/api_key/list.rs @@ -92,8 +92,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (api_keys, links) = - paginate_bidirectional(&config, keys, &pagination, original_url.path())?; + let (api_keys, links) = paginate_bidirectional( + &config, + &config.api_key.list_limit, + keys, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(ApiKeyList { api_keys, links })).into_response()) } diff --git a/crates/keystone/src/api/v4/mapping/ruleset/list.rs b/crates/keystone/src/api/v4/mapping/ruleset/list.rs index 44d8ac539..0d8f80880 100644 --- a/crates/keystone/src/api/v4/mapping/ruleset/list.rs +++ b/crates/keystone/src/api/v4/mapping/ruleset/list.rs @@ -101,8 +101,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (mappings, links) = - paginate_bidirectional(&config, rulesets, &pagination, original_url.path())?; + let (mappings, links) = paginate_bidirectional( + &config, + &config.mapping.list_limit, + rulesets, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(MappingRuleSetList { mappings, links })).into_response()) } diff --git a/crates/keystone/src/api/v4/oauth2/clients/list.rs b/crates/keystone/src/api/v4/oauth2/clients/list.rs index 903ce6400..ad27cc825 100644 --- a/crates/keystone/src/api/v4/oauth2/clients/list.rs +++ b/crates/keystone/src/api/v4/oauth2/clients/list.rs @@ -98,8 +98,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (oauth2_clients, links) = - paginate_bidirectional(&config, clients, &pagination, original_url.path())?; + let (oauth2_clients, links) = paginate_bidirectional( + &config, + &config.oauth2.list_limit, + clients, + &pagination, + &original_url, + )?; Ok(( StatusCode::OK, diff --git a/crates/keystone/src/api/v4/scim_realm/list.rs b/crates/keystone/src/api/v4/scim_realm/list.rs index 5822f7b69..34211fbc1 100644 --- a/crates/keystone/src/api/v4/scim_realm/list.rs +++ b/crates/keystone/src/api/v4/scim_realm/list.rs @@ -91,8 +91,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (scim_realms, links) = - paginate_bidirectional(&config, realms, &pagination, original_url.path())?; + let (scim_realms, links) = paginate_bidirectional( + &config, + &config.scim_realm.list_limit, + realms, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(ScimRealmList { scim_realms, links })).into_response()) } diff --git a/crates/keystone/src/api/v4/token/restriction/list.rs b/crates/keystone/src/api/v4/token/restriction/list.rs index f8bddfdef..b31d33957 100644 --- a/crates/keystone/src/api/v4/token/restriction/list.rs +++ b/crates/keystone/src/api/v4/token/restriction/list.rs @@ -100,9 +100,10 @@ pub(super) async fn list( let (restrictions, links) = paginate_bidirectional( &config, + &config.token_restriction.list_limit, token_restrictions, &pagination, - original_url.path(), + &original_url, )?; Ok(( diff --git a/crates/keystone/src/api/v4/user/list.rs b/crates/keystone/src/api/v4/user/list.rs index 116ea24a5..f71059e18 100644 --- a/crates/keystone/src/api/v4/user/list.rs +++ b/crates/keystone/src/api/v4/user/list.rs @@ -86,7 +86,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (users, links) = paginate_bidirectional(&config, users, &pagination, original_url.path())?; + let (users, links) = paginate_bidirectional( + &config, + &config.identity.list_limit, + users, + &pagination, + &original_url, + )?; Ok((StatusCode::OK, Json(UserList { users, links })).into_response()) } diff --git a/crates/keystone/src/federation/api/identity_provider/list.rs b/crates/keystone/src/federation/api/identity_provider/list.rs index 9048cb53e..8c3b4c725 100644 --- a/crates/keystone/src/federation/api/identity_provider/list.rs +++ b/crates/keystone/src/federation/api/identity_provider/list.rs @@ -96,10 +96,21 @@ pub(super) async fn list( Some(HashSet::from([query.domain_id.clone()])) }; + // Resolve the effective limit *before* querying, so the backend fetches a + // bounded page. Passing the raw `pagination.limit` here let a request with + // no `?limit` read the whole table and only truncate afterwards, whenever + // a global `[DEFAULT] list_limit` was configured. + // + // The federation provider has no `list_limit` section of its own, so the + // global `[DEFAULT] list_limit`/`max_db_limit` fallbacks apply. + let config = state.config_manager.config.read().await; + let provider_limit = openstack_keystone_config::ListLimitConfig::default(); + let limit = config.resolve_list_limit(&provider_limit, pagination.limit); + let mut provider_list_params = ProviderIdentityProviderListParameters::from(query.clone()); provider_list_params.domain_ids = domain_ids; provider_list_params.pagination = ListPagination { - limit: pagination.limit, + limit, marker: pagination.marker.clone(), page_reverse: pagination.page_reverse, }; @@ -116,12 +127,12 @@ pub(super) async fn list( .map(Into::into) .collect(); - let config = state.config_manager.config.read().await; let (identity_providers, links) = paginate_bidirectional( &config, + &provider_limit, identity_providers, &pagination, - original_url.path(), + &original_url, )?; Ok(( StatusCode::OK, @@ -149,7 +160,7 @@ mod tests { use openstack_keystone_core_types::federation as provider_types; use super::{super::openapi_router, *}; - use crate::api::tests::{get_mocked_state, test_fixture_scoped}; + use crate::api::tests::{get_mocked_state, get_mocked_state_with_config, test_fixture_scoped}; use crate::federation::MockFederationProvider; use crate::provider::Provider; @@ -287,7 +298,7 @@ mod tests { name: Some("name".into()), domain_ids: Some(HashSet::from([None, Some("domain_id".into())])), pagination: ListPagination { - limit: Some(20), + limit: None, marker: None, page_reverse: false, }, @@ -344,7 +355,7 @@ mod tests { name: Some("name".into()), domain_ids: Some(HashSet::from([None, Some("domain_id".into())])), pagination: ListPagination { - limit: Some(20), + limit: None, marker: None, page_reverse: false, }, @@ -519,4 +530,94 @@ mod tests { assert_eq!(res.links, None); assert_eq!(res.identity_providers.len(), 1); } + + /// The backend must receive the *resolved* limit, not the raw request. + /// With a global `[DEFAULT] list_limit` and no `?limit`, passing + /// `pagination.limit` straight through made the driver read the whole + /// table and only truncate afterwards. + #[traced_test] + #[tokio::test] + async fn test_list_backend_gets_global_default_limit() { + let mut federation_mock = MockFederationProvider::default(); + federation_mock + .expect_list_identity_providers() + .withf(|_, qp: &provider_types::IdentityProviderListParameters| { + qp.pagination.limit == Some(7) + }) + .returning(|_, _| Ok(Vec::new())); + + let mut config = openstack_keystone_config::Config::default(); + config.default.list_limit = Some(7); + + let vsc = test_fixture_scoped(); + let state = get_mocked_state_with_config( + Provider::mocked_builder().mock_federation(federation_mock), + true, + Some(true), + config, + ) + .await; + + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .uri("/") + .extension(vsc) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::OK); + } + + /// A client `limit` above the global `max_db_limit` is clamped before the + /// query, so the driver never over-reads. + #[traced_test] + #[tokio::test] + async fn test_list_backend_limit_clamped_to_max_db_limit() { + let mut federation_mock = MockFederationProvider::default(); + federation_mock + .expect_list_identity_providers() + .withf(|_, qp: &provider_types::IdentityProviderListParameters| { + qp.pagination.limit == Some(3) + }) + .returning(|_, _| Ok(Vec::new())); + + let mut config = openstack_keystone_config::Config::default(); + config.default.max_db_limit = Some(3); + + let vsc = test_fixture_scoped(); + let state = get_mocked_state_with_config( + Provider::mocked_builder().mock_federation(federation_mock), + true, + Some(true), + config, + ) + .await; + + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .uri("/?limit=100") + .extension(vsc) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::OK); + } } diff --git a/crates/keystone/src/k8s_auth/api/instance/list.rs b/crates/keystone/src/k8s_auth/api/instance/list.rs index 240c945c1..98e134003 100644 --- a/crates/keystone/src/k8s_auth/api/instance/list.rs +++ b/crates/keystone/src/k8s_auth/api/instance/list.rs @@ -103,8 +103,13 @@ pub(super) async fn list( .map(Into::into) .collect(); - let (instances, links) = - paginate_bidirectional(&config, instances, &pagination, original_url.path())?; + let (instances, links) = paginate_bidirectional( + &config, + &config.k8s_auth.list_limit, + instances, + &pagination, + &original_url, + )?; Ok(( StatusCode::OK, Json(K8sAuthInstanceList { instances, links }),