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 }),