From 3a3f39293e0d488e81b816d3f22dd999d49e3a60 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:36:32 +0200 Subject: [PATCH 1/7] fix(api-gateway): CORS-expose the ETag response header The RBAC/AM write contract is ETag-guarded: DELETE/PATCH on role assignments and role definitions require the resource's current ETag in If-Match, and the only way to obtain it is to read the ETag header from a GET/POST response. Browsers may only read non-safelisted response headers when the server sends Access-Control-Expose-Headers, and the gateway sent none at all - so every cross-origin browser client saw ETag as absent and every ETag-guarded write dead-ended, while curl/Postman hid the problem. Add CorsConfig.exposed_headers (wired to CorsLayer::expose_headers, "*" supported like the sibling lists) defaulting to ["ETag"] so deployments get the fix without a config change. Also harden CORS config handling, both surfaced while reviewing the fix: - validate_cors_config at gear init rejects any wildcard CORS list combined with allow_credentials=true. tower-http asserts on exactly this inside Layer::layer, and axum's eager router layering turned it into a startup crash-loop with no pointer at the config; now it is a clean init error naming the offending list. - Unparseable entries in ANY cors list warn and are skipped instead of vanishing in a silent filter_map: a typo'd exposed_headers entry used to silently suppress Access-Control-Expose-Headers and re-break every ETag-guarded browser write. A shared parse_list helper replaces the four copies of the parse pattern. Covers the default and an explicitly configured list with request-level tests, plus unit tests for each validation arm. Signed-off-by: Diffora --- gears/system/api-gateway/src/config.rs | 10 ++ gears/system/api-gateway/src/cors.rs | 165 +++++++++++++++++-- gears/system/api-gateway/src/gear.rs | 4 + gears/system/api-gateway/tests/cors_tests.rs | 116 +++++++++++++ 4 files changed, 279 insertions(+), 16 deletions(-) diff --git a/gears/system/api-gateway/src/config.rs b/gears/system/api-gateway/src/config.rs index 0ca959f4f..35c6fbb4e 100644 --- a/gears/system/api-gateway/src/config.rs +++ b/gears/system/api-gateway/src/config.rs @@ -104,6 +104,15 @@ pub struct CorsConfig { pub allowed_methods: Vec, /// Allowed request headers; `["*"]` means any pub allowed_headers: Vec, + /// Response headers exposed to browser JavaScript via + /// `Access-Control-Expose-Headers`; `["*"]` means any (do not combine + /// with `allow_credentials` — the wildcard is invalid with credentials). + /// Defaults to `["ETag"]`: the optimistic-concurrency write contract + /// requires clients to read the `ETag` response header to build + /// `If-Match` guarded writes, and `ETag` is not on the CORS safelist — + /// without exposing it every ETag-guarded write is impossible from a + /// cross-origin browser client. + pub exposed_headers: Vec, /// Whether to allow credentials pub allow_credentials: bool, /// Max age for preflight caching in seconds @@ -123,6 +132,7 @@ impl Default for CorsConfig { "OPTIONS".to_owned(), ], allowed_headers: vec!["*".to_owned()], + exposed_headers: vec!["ETag".to_owned()], allow_credentials: false, max_age_seconds: 600, } diff --git a/gears/system/api-gateway/src/cors.rs b/gears/system/api-gateway/src/cors.rs index b4a81e50a..73d555c2d 100644 --- a/gears/system/api-gateway/src/cors.rs +++ b/gears/system/api-gateway/src/cors.rs @@ -2,7 +2,73 @@ use tower_http::cors::CorsLayer; use crate::config::{ApiGatewayConfig, CorsConfig}; -/// Build a CORS layer from config. +/// Validate the CORS configuration at config-load time, BEFORE any +/// `CorsLayer` is constructed. tower-http enforces the same rules with +/// an `assert!` inside `Layer::layer` — with axum's eager router +/// layering that would be a startup panic (crash-loop) with no pointer +/// at the offending config; failing `init` instead surfaces a clean, +/// actionable error. +/// +/// # Errors +/// Returns a human-readable description of the invalid combination. +pub fn validate_cors_config(cfg: &ApiGatewayConfig) -> Result<(), String> { + if !cfg.cors_enabled { + return Ok(()); + } + let cors_cfg: CorsConfig = cfg.cors.clone().unwrap_or_default(); + if !cors_cfg.allow_credentials { + return Ok(()); + } + // tower-http rejects every wildcard when credentials are allowed + // (`ensure_usable_cors_rules`): origins, methods, request headers, + // and exposed headers. + let wildcard_lists = [ + ("cors.allowed_origins", &cors_cfg.allowed_origins), + ("cors.allowed_methods", &cors_cfg.allowed_methods), + ("cors.allowed_headers", &cors_cfg.allowed_headers), + ("cors.exposed_headers", &cors_cfg.exposed_headers), + ]; + for (name, list) in wildcard_lists { + if list.iter().any(|v| v == "*") { + return Err(format!( + "invalid CORS configuration: `{name}` contains the wildcard \"*\" while \ + `cors.allow_credentials` is true; browsers forbid this combination and \ + tower-http would panic at startup — list the values explicitly or \ + disable credentials" + )); + } + } + Ok(()) +} + +/// Parse a configured string list into typed values, WARNING about (and +/// skipping) every entry that does not parse instead of dropping it +/// silently. A silently-dropped entry is how a config typo turns into a +/// hard-to-diagnose behavioral hole — e.g. an unparseable +/// `exposed_headers` entry silently suppresses `Access-Control-Expose-Headers` +/// and breaks every ETag-guarded browser write. +fn parse_list(list_name: &'static str, items: &[String]) -> Vec { + items + .iter() + .filter_map(|s| { + s.parse::() + .map_err(|_| { + tracing::warn!( + entry = %s, + list = list_name, + "api-gateway CORS config entry does not parse; ignoring it" + ); + }) + .ok() + }) + .collect() +} + +/// Build a CORS layer from config. `["*"]` in a list means "any"; +/// otherwise entries are parsed into their typed forms with a warning +/// for every unparseable entry (see [`parse_list`]). Invalid +/// wildcard+credentials combinations are rejected earlier by +/// [`validate_cors_config`] at gear init. pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { let cors_cfg: CorsConfig = cfg.cors.clone().unwrap_or_default(); @@ -11,11 +77,8 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_origins.iter().any(|o| o == "*") { layer = layer.allow_origin(tower_http::cors::Any); } else { - let origins: Vec = cors_cfg - .allowed_origins - .into_iter() - .filter_map(|s| axum::http::HeaderValue::from_str(&s).ok()) - .collect(); + let origins: Vec = + parse_list("allowed_origins", &cors_cfg.allowed_origins); if !origins.is_empty() { layer = layer.allow_origin(origins); } @@ -24,11 +87,8 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_methods.iter().any(|m| m == "*") { layer = layer.allow_methods(tower_http::cors::Any); } else { - let methods: Vec = cors_cfg - .allowed_methods - .into_iter() - .filter_map(|s| s.parse().ok()) - .collect(); + let methods: Vec = + parse_list("allowed_methods", &cors_cfg.allowed_methods); if !methods.is_empty() { layer = layer.allow_methods(methods); } @@ -37,16 +97,23 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_headers.iter().any(|h| h == "*") { layer = layer.allow_headers(tower_http::cors::Any); } else { - let headers: Vec = cors_cfg - .allowed_headers - .into_iter() - .filter_map(|s| s.parse().ok()) - .collect(); + let headers: Vec = + parse_list("allowed_headers", &cors_cfg.allowed_headers); if !headers.is_empty() { layer = layer.allow_headers(headers); } } + if cors_cfg.exposed_headers.iter().any(|h| h == "*") { + layer = layer.expose_headers(tower_http::cors::Any); + } else { + let headers: Vec = + parse_list("exposed_headers", &cors_cfg.exposed_headers); + if !headers.is_empty() { + layer = layer.expose_headers(headers); + } + } + if cors_cfg.allow_credentials { layer = layer.allow_credentials(true); } @@ -57,3 +124,69 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { layer } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::ApiGatewayConfig; + + fn cfg_with(cors: CorsConfig) -> ApiGatewayConfig { + ApiGatewayConfig { + cors_enabled: true, + cors: Some(cors), + ..Default::default() + } + } + + // The exact combination tower-http panics on at layer construction: + // any wildcard list + allow_credentials. Must be a config-load error, + // not a startup crash-loop. + #[test] + fn wildcard_exposed_headers_with_credentials_rejected_at_validation() { + // Explicit origins/methods/headers so ONLY exposed_headers carries + // the wildcard — pins the newly-added list's own validation arm. + let cfg = cfg_with(CorsConfig { + allowed_origins: vec!["https://ui.example.com".to_owned()], + allowed_methods: vec!["GET".to_owned()], + allowed_headers: vec!["Content-Type".to_owned()], + exposed_headers: vec!["*".to_owned()], + allow_credentials: true, + ..Default::default() + }); + let err = validate_cors_config(&cfg).expect_err("wildcard+credentials must be rejected"); + assert!(err.contains("exposed_headers"), "got: {err}"); + } + + #[test] + fn wildcard_origins_with_credentials_rejected_at_validation() { + let cfg = cfg_with(CorsConfig { + allow_credentials: true, + ..Default::default() // default allowed_origins/headers are ["*"] + }); + assert!(validate_cors_config(&cfg).is_err()); + } + + #[test] + fn explicit_lists_with_credentials_pass_validation() { + let cfg = cfg_with(CorsConfig { + allowed_origins: vec!["https://ui.example.com".to_owned()], + allowed_methods: vec!["GET".to_owned(), "POST".to_owned()], + allowed_headers: vec!["Content-Type".to_owned()], + exposed_headers: vec!["ETag".to_owned()], + allow_credentials: true, + ..Default::default() + }); + assert!(validate_cors_config(&cfg).is_ok()); + } + + #[test] + fn cors_disabled_skips_validation() { + let mut cfg = cfg_with(CorsConfig { + exposed_headers: vec!["*".to_owned()], + allow_credentials: true, + ..Default::default() + }); + cfg.cors_enabled = false; + assert!(validate_cors_config(&cfg).is_ok()); + } +} diff --git a/gears/system/api-gateway/src/gear.rs b/gears/system/api-gateway/src/gear.rs index 402bbe34b..7570e4566 100644 --- a/gears/system/api-gateway/src/gear.rs +++ b/gears/system/api-gateway/src/gear.rs @@ -679,6 +679,10 @@ impl ApiGateway { impl toolkit::Gear for ApiGateway { async fn init(&self, ctx: &toolkit::context::GearCtx) -> anyhow::Result<()> { let cfg = ctx.config_or_default::()?; + // Fail init on invalid CORS combinations (wildcard+credentials): + // tower-http would otherwise assert-panic during eager router + // layering — a startup crash-loop with no pointer at the config. + crate::cors::validate_cors_config(&cfg).map_err(|e| anyhow::anyhow!(e))?; self.config.store(Arc::new(cfg.clone())); debug!( diff --git a/gears/system/api-gateway/tests/cors_tests.rs b/gears/system/api-gateway/tests/cors_tests.rs index f582b88e3..59adb6d06 100644 --- a/gears/system/api-gateway/tests/cors_tests.rs +++ b/gears/system/api-gateway/tests/cors_tests.rs @@ -205,6 +205,122 @@ async fn test_cors_disabled() { // Verify router builds without CORS layer } +// Regression guard: the RBAC/AM write contract is ETag-guarded +// (`If-Match`), and browsers may only read non-safelisted response headers +// when the server sends `Access-Control-Expose-Headers`. The gateway used to +// emit no expose-headers at all, so cross-origin JS saw every `ETag` as +// absent and ETag-guarded writes were impossible from the UI. Assert the +// header is present on an actual cross-origin response — with the DEFAULT +// cors config (no explicit `cors` block), which must expose `ETag` out of +// the box. +#[tokio::test] +async fn test_cors_default_exposes_etag_header() { + use tower::ServiceExt as _; + + let api_gateway = api_gateway::ApiGateway::default(); + let ctx = create_test_gear_ctx_permissive_cors(); + api_gateway.init(&ctx).await.expect("Failed to init"); + + let gear = CorsTestGear; + let router = gear + .register_rest(&ctx, Router::new(), &api_gateway) + .expect("Failed to register routes"); + let app = api_gateway + .rest_finalize(&ctx, router) + .expect("Failed to finalize router"); + + let response = app + .oneshot( + http::Request::builder() + .method(http::Method::GET) + .uri("/tests/v1/cors/v1/cors-test") + .header(http::header::ORIGIN, "https://ui.example.com") + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + let exposed = response + .headers() + .get(http::header::ACCESS_CONTROL_EXPOSE_HEADERS) + .expect("Access-Control-Expose-Headers must be present on CORS responses") + .to_str() + .unwrap() + .to_ascii_lowercase(); + assert!( + exposed.split(',').map(str::trim).any(|h| h == "etag"), + "ETag must be CORS-exposed so browser clients can perform \ + ETag-guarded writes, got: {exposed}" + ); +} + +// Same guard for an explicitly configured `cors` block that names its own +// `exposed_headers` list — the configured values must reach the wire. +#[tokio::test] +async fn test_cors_configured_exposed_headers_reach_response() { + use tower::ServiceExt as _; + + let config = wrap_config(&serde_json::json!({ + "bind_addr": "127.0.0.1:0", + "cors_enabled": true, + "cors": { + "allowed_origins": ["https://example.com"], + "allowed_methods": ["GET", "POST"], + "allowed_headers": ["Content-Type"], + "exposed_headers": ["ETag", "X-Request-Id"], + "allow_credentials": true, + "max_age_seconds": 600 + }, + "auth_disabled": true + })); + let hub = Arc::new(toolkit::ClientHub::new()); + let ctx = GearCtx::new( + "api-gateway", + Uuid::new_v4(), + Arc::new(TestConfigProvider { config }), + hub, + tokio_util::sync::CancellationToken::new(), + ); + + let api_gateway = api_gateway::ApiGateway::default(); + api_gateway.init(&ctx).await.expect("Failed to init"); + + let gear = CorsTestGear; + let router = gear + .register_rest(&ctx, Router::new(), &api_gateway) + .expect("Failed to register routes"); + let app = api_gateway + .rest_finalize(&ctx, router) + .expect("Failed to finalize router"); + + let response = app + .oneshot( + http::Request::builder() + .method(http::Method::GET) + .uri("/tests/v1/cors/v1/cors-test") + .header(http::header::ORIGIN, "https://example.com") + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + let exposed = response + .headers() + .get(http::header::ACCESS_CONTROL_EXPOSE_HEADERS) + .expect("Access-Control-Expose-Headers must be present") + .to_str() + .unwrap() + .to_ascii_lowercase(); + for want in ["etag", "x-request-id"] { + assert!( + exposed.split(',').map(str::trim).any(|h| h == want), + "expected {want} in Access-Control-Expose-Headers, got: {exposed}" + ); + } +} + #[tokio::test] async fn test_cors_config_validation() { // Test that CORS config is properly loaded From 3524af558f8c03649e127cbaa99713d9deaafdbb Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:36:47 +0200 Subject: [PATCH 2/7] feat(account-management): support $orderby=status on the tenant children listing Clicking the Status column on the Tenants page issued GET /tenants/{id}/children?$orderby=status and got 400 'unsupported $orderby field: status': the field was flagged non-orderable because the cursor codec was hard-wired to the wire filter kind (String) while the extracted cursor value speaks the storage shape (SMALLINT) - encode/parse could not round-trip the token. Give ODataFieldMapping a per-field cursor_kind() (default: the wire field.kind(), so every existing mapper is untouched) and use it on both sides of the cursor codec. encode_cursor_value also accepts SmallInt/Int under FieldKind::I64 so mappers exposing narrow integer columns need not hand-widen to BigInt. The AM mapper overrides Status -> I64 and extracts the storage ordinal, so status becomes orderable by its lifecycle ordinal (active < suspended < deleted) with cursor pages comparing integer to SMALLINT - a numeric SQL comparison. tenant_type stays non-orderable (ordering by a derived UUIDv5 has no honest meaning). Filter comparisons on status stay membership-only (eq/ne/in). resource-group carried the same latent wire-String/storage-SMALLINT mismatch on its three type fields (Group/Hierarchy/Membership), orderable and un-overridden - $orderby=type broke with InvalidCursor on any page overflow; they now declare cursor_kind=I64. Integration tests replace the old rejection pin with a lifecycle-ordinal sort assertion and a full cursor walk across ordinal groups (bounded against a repeated-cursor hang). Codec unit tests pin the narrow-integer round-trip and the loud failure for a genuine wire/storage mismatch. OpenAPI documents $orderby=status as supported with lifecycle-ordinal semantics. Signed-off-by: Diffora --- .../account-management-sdk/src/tenant.rs | 8 +- .../src/infra/storage/repo_impl/reads.rs | 53 +++++--- .../tests/list_children_integration.rs | 120 +++++++++++++++--- .../docs/account-management-v1.yaml | 20 +-- .../src/infra/storage/odata_mapper.rs | 29 +++++ libs/toolkit-db/src/odata/sea_orm_filter.rs | 97 +++++++++++--- 6 files changed, 267 insertions(+), 60 deletions(-) diff --git a/gears/system/account-management/account-management-sdk/src/tenant.rs b/gears/system/account-management/account-management-sdk/src/tenant.rs index aa62d5fbe..17d602df0 100644 --- a/gears/system/account-management/account-management-sdk/src/tenant.rs +++ b/gears/system/account-management/account-management-sdk/src/tenant.rs @@ -240,7 +240,11 @@ pub struct TenantInfoQuery { /// (the serde rename on the SDK enum). The `OData` parser only /// validates the value is a String; arbitrary unknown values /// (including the AM-internal `"provisioning"`) reach storage and - /// are mapped to a domain error downstream. + /// are mapped to a domain error downstream. `$orderby=status` is + /// supported and sorts by the lifecycle ordinal (`active` < + /// `suspended` < `deleted`), not alphabetically — operator UIs sort + /// their Status column with it; filter comparisons stay + /// membership-only (`eq` / `ne` / `in`). #[odata(filter(kind = "String"))] pub status: String, /// Deterministic `UUIDv5` of the registered tenant-type schema id. @@ -259,7 +263,7 @@ pub struct TenantInfoQuery { /// string is mapped server-side to its deterministic `UUIDv5` and /// compared against `tenant_type_uuid`; ordered operators and /// `$orderby` are rejected (sorting by a derived UUID is - /// meaningless), mirroring `status`. + /// meaningless). #[odata(filter(kind = "String"))] pub tenant_type: String, /// `tenants.self_managed` flag. Useful to surface "boundary" diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs index 3a993e089..375671633 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs @@ -17,7 +17,7 @@ use toolkit_db::odata::sea_orm_filter::{ FieldToColumn, LimitCfg, ODataFieldMapping, PaginateOdataTryError, paginate_odata_try, }; use toolkit_db::secure::SecureEntityExt; -use toolkit_odata::filter::{FilterOp, ODataValue}; +use toolkit_odata::filter::{FieldKind, FilterField as _, FilterOp, ODataValue}; use toolkit_odata::{ODataQuery, Page, SortDir, ast}; use toolkit_security::AccessScope; use uuid::Uuid; @@ -136,23 +136,20 @@ impl FieldToColumn for TenantODataMapper { } } - /// Reject `$orderby=status` and `status` cursor keys: the column - /// is exposed as a string contract on the wire while it is - /// `SMALLINT` in storage, and there is no consistent ordering - /// across the two shapes — alphabetical (`active < deleted < - /// suspended`) versus numeric (`Active = 1 < Suspended = 2 < - /// Deleted = 3`). The framework rejects the `$orderby` clause as - /// `InvalidOrderByField` before composing the effective order, so - /// the cursor codec never sees a translated-shape value. + /// `$orderby=status` sorts by the lifecycle ordinal (`Active = 1 < + /// Suspended = 2 < Deleted = 3`), NOT alphabetically by the wire + /// string — a deliberate categorical order (healthy first, + /// tombstones last, and inverted via `desc`) that operator UIs sort + /// their Status column by. Cursor pages are safe: both + /// the effective-order comparison and `extract_cursor_value` speak + /// the storage `SMALLINT`, so the codec never sees a + /// translated-shape value. + /// + /// `$orderby=tenant_type` stays rejected: it compares via a derived + /// `UUIDv5`, and an ordered comparison on a derived UUID has no + /// honest meaning. fn is_orderable(field: TenantInfoFilterField) -> bool { - // `status` and `tenant_type` are exposed as strings on the wire but - // compared via a translated storage shape (SMALLINT / derived - // UUIDv5); neither has a consistent wire-vs-storage order, so - // `$orderby` on them is rejected before the effective order composes. - !matches!( - field, - TenantInfoFilterField::Status | TenantInfoFilterField::TenantType - ) + !matches!(field, TenantInfoFilterField::TenantType) } } @@ -184,6 +181,17 @@ impl ODataFieldMapping for TenantODataMapper { } } } + + /// `status` is a wire string but sorts (and therefore cursors) by + /// its storage lifecycle ordinal — encode/parse its cursor token as + /// an integer, not with the wire `String` kind. Every + /// other field's cursor shape matches its wire kind. + fn cursor_kind(field: TenantInfoFilterField) -> FieldKind { + match field { + TenantInfoFilterField::Status => FieldKind::I64, + other => other.kind(), + } + } } /// Pagination limits for the tenant-children listing surface. Mirrors @@ -648,4 +656,15 @@ mod tenant_type_filter_tests { Field::TenantType )); } + + // Regression guard: the Tenants-page Status column sorts via + // `$orderby=status`; the field must be orderable (lifecycle-ordinal + // order — see `is_orderable` docs), not rejected with + // `InvalidOrderByField` → 400 as it used to be. + #[test] + fn status_is_orderable() { + assert!(>::is_orderable( + Field::Status + )); + } } diff --git a/gears/system/account-management/account-management/tests/list_children_integration.rs b/gears/system/account-management/account-management/tests/list_children_integration.rs index 5af1c4e64..be82517af 100644 --- a/gears/system/account-management/account-management/tests/list_children_integration.rs +++ b/gears/system/account-management/account-management/tests/list_children_integration.rs @@ -426,33 +426,123 @@ async fn list_children_rejects_ordered_comparison_on_status() { ); } -/// `$orderby=status` is rejected by the framework before the -/// effective-order is composed: the column is exposed as a string on -/// the wire but is `SMALLINT` in storage, so the cursor codec would -/// either fail to decode the token (decoded as a String when the -/// model side returned a `SmallInt`) or compare against the wrong -/// SQL type on the next page. `TenantODataMapper::is_orderable` -/// returns `false` for `Status`, which the framework's order- -/// validation loop trips on. +/// `$orderby=status` sorts children by the lifecycle ordinal +/// (`Active = 1 < Suspended = 2 < Deleted = 3`), the order the +/// Tenants-page Status column uses. Insertion (`created_at`) order is +/// deliberately NOT the ordinal order, so a passing assertion proves +/// the sort honoured `status`, not the chronological default. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn list_children_rejects_orderby_on_status() { +async fn list_children_orderby_status_sorts_by_lifecycle_ordinal() { let h = setup_sqlite().await.expect("harness"); let root = Uuid::from_u128(ROOT_ID); seed_root(&h, root).await; + let type_a = Uuid::from_u128(0xAA); + let suspended_old = Uuid::from_u128(0x701); + let active_new = Uuid::from_u128(0x702); + let suspended_new = Uuid::from_u128(0x703); + seed_tenant_at(&h, suspended_old, root, SUSPENDED, false, type_a, ts_at(1)).await; + seed_tenant_at(&h, active_new, root, ACTIVE, false, type_a, ts_at(2)).await; + seed_tenant_at(&h, suspended_new, root, SUSPENDED, false, type_a, ts_at(3)).await; + let query = ODataQuery::default().with_order(ODataOrderBy(vec![OrderKey { field: "status".to_owned(), dir: SortDir::Asc, }])); - let err = h + let page = h .repo .list_children(&allow_all(), root, &query) .await - .expect_err("$orderby=status MUST be rejected"); - let detail = format!("{err:?}"); - assert!( - detail.contains("status") || detail.contains("InvalidOrderByField"), - "expected order-validation rejection; got {detail}" + .expect("$orderby=status must be accepted"); + + // Effective order is (status ASC, id ASC): active first, then the + // two suspended rows tie-broken by id. + assert_eq!( + ids_of(&page.items), + vec![active_new, suspended_old, suspended_new], + "`$orderby=status asc` MUST sort by lifecycle ordinal with id tiebreak" + ); +} + +/// Cursor guard: ordering by `status` must survive the cursor +/// round-trip. The cursor token carries the storage ordinal +/// (`cursor_kind = I64` on the AM mapper), so page 2+ predicates +/// compare SMALLINT-to-integer — the exact wire-vs-storage mismatch +/// that used to force `is_orderable = false` for this field. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_children_orderby_status_cursor_roundtrip() { + let h = setup_sqlite().await.expect("harness"); + let root = Uuid::from_u128(ROOT_ID); + seed_root(&h, root).await; + + let type_a = Uuid::from_u128(0xAA); + // 5 children, statuses interleaved so every page boundary lands + // inside or between ordinal groups. + let seeds: Vec<(Uuid, i16)> = vec![ + (Uuid::from_u128(0x711), SUSPENDED), + (Uuid::from_u128(0x712), ACTIVE), + (Uuid::from_u128(0x713), SUSPENDED), + (Uuid::from_u128(0x714), ACTIVE), + (Uuid::from_u128(0x715), ACTIVE), + ]; + for (i, (id, status)) in seeds.iter().enumerate() { + seed_tenant_at( + &h, + *id, + root, + *status, + false, + type_a, + ts_at(1) + Duration::seconds(i64::try_from(i).unwrap()), + ) + .await; + } + + // Expected effective order (status ASC, id ASC): the three active + // rows by id, then the two suspended rows by id. + let expected = vec![ + Uuid::from_u128(0x712), + Uuid::from_u128(0x714), + Uuid::from_u128(0x715), + Uuid::from_u128(0x711), + Uuid::from_u128(0x713), + ]; + + // Bound the walk: a mapper/codec regression that emits a repeated + // `next_cursor` would otherwise loop forever instead of failing. A + // cursor seen twice is a hard assertion, not a hang. + let mut seen_cursors = std::collections::HashSet::new(); + let mut collected = Vec::new(); + let mut cursor: Option = None; + loop { + let mut q = ODataQuery::default().with_limit(2); + if let Some(c) = cursor.take() { + assert!( + seen_cursors.insert(c.clone()), + "cursor walk returned a repeated cursor — pagination is not advancing" + ); + q = q.with_cursor(CursorV1::decode(&c).expect("decode status cursor")); + } else { + q = q.with_order(ODataOrderBy(vec![OrderKey { + field: "status".to_owned(), + dir: SortDir::Asc, + }])); + } + let page = h + .repo + .list_children(&allow_all(), root, &q) + .await + .expect("status-ordered cursor page must not fail"); + collected.extend(ids_of(&page.items)); + match page.page_info.next_cursor { + Some(next) => cursor = Some(next), + None => break, + } + } + assert_eq!( + collected, expected, + "cursor walk over `$orderby=status` MUST visit every row exactly \ + once in (status ASC, id ASC) order" ); } diff --git a/gears/system/account-management/docs/account-management-v1.yaml b/gears/system/account-management/docs/account-management-v1.yaml index 3ba313445..f29ecfc5a 100644 --- a/gears/system/account-management/docs/account-management-v1.yaml +++ b/gears/system/account-management/docs/account-management-v1.yaml @@ -750,14 +750,18 @@ components: `SMALLINT` ordinal at the repo boundary. Examples: `status eq 'active'`, `status eq 'pending'`, `target_mode eq 'self_managed'`, `initiator_side eq 'child'`. - Only `eq`, `ne`, and `in` are admissible on these columns; - ordered comparisons (`gt`/`ge`/`lt`/`le`) and `$orderby` are - rejected with `400 INVALID_FILTER` because there is no - consistent ordering across the wire (alphabetical) and - storage (numeric) shapes. Response bodies - return the same field as a string (`"pending"`, `"active"`, - etc.) for human readability — the filter is the one place - the wire surface diverges from the response shape. + Only `eq`, `ne`, and `in` are admissible in `$filter` on these + columns; ordered comparisons (`gt`/`ge`/`lt`/`le`) are rejected + with `400 INVALID_FILTER` because the wire strings carry no + meaningful order. `$orderby` on the `conversion_requests` + enum columns is likewise rejected. `$orderby=status` on the + tenant children listing IS supported and sorts by the + lifecycle ordinal (`active` < `suspended` < `deleted`), not + alphabetically — operator UIs sort their Status column with + it. Filters and response bodies use the same public string + values (`"pending"`, `"active"`, etc.); the divergence is + between the wire contract (strings) and the `SMALLINT` storage + ordinal, not between the filter and the response shape. schema: type: string ODataOrderBy: diff --git a/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs b/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs index 44fa329fc..5a36b700e 100644 --- a/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs +++ b/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs @@ -6,6 +6,7 @@ //! This gear maps from DTO-level filter fields to `SeaORM` Column types. use toolkit_db::odata::sea_orm_filter::{FieldToColumn, ODataFieldMapping}; +use toolkit_odata::filter::{FieldKind, FilterField as _}; use crate::infra::storage::entity::gts_type::{ Column as TypeColumn, Entity as TypeEntity, Model as TypeModel, @@ -77,6 +78,18 @@ impl ODataFieldMapping for GroupODataMapper { GroupFilterField::Type => sea_orm::Value::SmallInt(Some(model.gts_type_id)), } } + + /// `type` is a wire string but its cursor value is the storage + /// `SMALLINT` gts ordinal — without this override the cursor codec + /// would try to encode the `SmallInt` under the wire `String` kind + /// and fail with `InvalidCursor` on any `$orderby=type` page + /// overflow (same wire-vs-storage seam as AM's `status`). + fn cursor_kind(field: GroupFilterField) -> FieldKind { + match field { + GroupFilterField::Type => FieldKind::I64, + other => other.kind(), + } + } } /// `OData` mapper for hierarchy queries (not used for `paginate_odata`; hierarchy @@ -103,6 +116,14 @@ impl ODataFieldMapping for HierarchyODataMapper { HierarchyFilterField::Type => sea_orm::Value::SmallInt(Some(model.gts_type_id)), } } + + /// Same wire-string / storage-SMALLINT seam as `GroupODataMapper`. + fn cursor_kind(field: HierarchyFilterField) -> FieldKind { + match field { + HierarchyFilterField::Type => FieldKind::I64, + HierarchyFilterField::HierarchyDepth => field.kind(), + } + } } /// `OData` mapper for memberships. @@ -137,6 +158,14 @@ impl ODataFieldMapping for MembershipODataMapper { } } } + + /// Same wire-string / storage-SMALLINT seam as `GroupODataMapper`. + fn cursor_kind(field: MembershipFilterField) -> FieldKind { + match field { + MembershipFilterField::ResourceType => FieldKind::I64, + other => other.kind(), + } + } } #[cfg(test)] diff --git a/libs/toolkit-db/src/odata/sea_orm_filter.rs b/libs/toolkit-db/src/odata/sea_orm_filter.rs index 3615e3874..c1e303f74 100644 --- a/libs/toolkit-db/src/odata/sea_orm_filter.rs +++ b/libs/toolkit-db/src/odata/sea_orm_filter.rs @@ -89,14 +89,15 @@ pub trait FieldToColumn { /// Whether `field` is admissible in `$orderby` (and therefore as /// a key in cursor-based pagination). Default `true`. Implementors - /// override with `false` when the wire shape of the field differs - /// from its storage shape — there is no consistent ordering - /// between the two shapes (alphabetical strings vs ordinal - /// integers), and the cursor encoding path relies on - /// `field.kind()` to read tokens back, which would fail to decode - /// a storage-encoded value. The framework checks this in - /// `paginate_odata_collect` before composing the effective order - /// and rejects the `$orderby` clause as + /// override with `false` when the field has no honest ordering at + /// all (e.g. a derived-UUID comparison column). When the field IS + /// meaningfully orderable but its storage shape differs from its + /// wire shape (e.g. a SMALLINT-backed enum exposed as a string), + /// keep it orderable and override + /// [`ODataFieldMapping::cursor_kind`] instead, so the cursor codec + /// round-trips the storage-shaped token. The framework checks this + /// in `paginate_odata_collect` before composing the effective + /// order and rejects the `$orderby` clause as /// [`ODataError::InvalidOrderByField`] — keeping the diagnostic /// at the same site as unknown-field rejections. fn is_orderable(_field: F) -> bool { @@ -144,6 +145,23 @@ pub trait ODataFieldMapping: FieldToColumn { field: F, ) -> sea_orm::Value; + /// The [`FieldKind`] the cursor codec uses to encode/parse this + /// field's cursor token. Defaults to the field's declared wire + /// kind (`field.kind()`). + /// + /// Override for fields whose cursor value (what + /// [`Self::extract_cursor_value`] returns, i.e. the storage shape + /// that `$orderby` actually sorts by) differs from the wire filter + /// shape — e.g. a SMALLINT-backed enum exposed as a string on the + /// API surface but ordered by its storage ordinal: return + /// [`FieldKind::I64`] and extract the natural storage value + /// (`SmallInt` / `Int` / `BigInt` all round-trip under `I64`) so + /// `encode_cursor_value` / `parse_cursor_value` carry the ordinal + /// instead of failing on the wire/storage mismatch. + fn cursor_kind(field: F) -> FieldKind { + field.kind() + } + /// Extract cursor values for all fields in an order. /// /// This is a convenience method that can be overridden for optimization, @@ -392,6 +410,13 @@ pub fn encode_cursor_value(value: &sea_orm::Value, kind: FieldKind) -> Result = match (kind, value) { (FieldKind::String, V::String(Some(s))) => Ok(s.to_string()), (FieldKind::I64, V::BigInt(Some(i))) => Ok(i.to_string()), + // Storage-shaped integer narrower than i64 (e.g. a SMALLINT + // lifecycle ordinal a mapper exposes under `cursor_kind = I64`): + // accept the natural extracted width so mappers need not widen + // by hand. `parse_cursor_value(I64, ..)` yields BigInt, and the + // SQL comparison against the narrow column stays numeric. + (FieldKind::I64, V::SmallInt(Some(i))) => Ok(i.to_string()), + (FieldKind::I64, V::Int(Some(i))) => Ok(i.to_string()), (FieldKind::F64, V::Double(Some(f))) => Ok(ryu::Buffer::new().format(*f).to_owned()), (FieldKind::Bool, V::Bool(Some(b))) => Ok(b.to_string()), (FieldKind::Uuid, V::Uuid(Some(u))) => Ok(u.to_string()), @@ -665,14 +690,12 @@ where }; // Reject `$orderby` keys against fields the mapper has flagged as - // non-orderable (typically: filter columns whose wire shape - // differs from their storage shape — e.g. a SMALLINT-backed enum - // exposed as a string on the API surface). Without this guard the - // cursor codec would round-trip through `field.kind()` and either - // misinterpret the encoded token or hand `SeaORM` a value with - // the wrong SQL type. Tiebreakers go through the same gate so a - // caller cannot smuggle a non-orderable column in via - // `ensure_tiebreaker`. + // non-orderable (fields with no honest ordering at all, e.g. a + // derived-UUID comparison column; wire-vs-storage shape mismatches + // are instead handled by the mapper's `cursor_kind` override, which + // keeps the cursor codec on the storage shape). Tiebreakers go + // through the same gate so a caller cannot smuggle a non-orderable + // column in via `ensure_tiebreaker`. for order_key in &effective_order.0 { let field = F::from_name(&order_key.field) .ok_or_else(|| ODataError::InvalidOrderByField(order_key.field.clone()))?; @@ -837,7 +860,10 @@ where let field = F::from_name(&order_key.field) .ok_or(ODataError::InvalidOrderByField(order_key.field.clone()))?; let column = M::map_field(field); - let kind = field.kind(); + // Cursor tokens carry the storage-shaped value the mapper + // extracted, so decode with the mapper's cursor kind (which may + // differ from the wire filter kind — see `cursor_kind`). + let kind = M::cursor_kind(field); let value = parse_cursor_value(kind, key_str).map_err(|_| ODataError::InvalidCursor)?; cursor_values.push((field, column, value, order_key.dir)); } @@ -892,7 +918,9 @@ where let mut cursor_keys = Vec::new(); for (field, value) in field_values { - let kind = field.kind(); + // Encode with the mapper's cursor kind, matching what + // `extract_cursor_value` produced (see `cursor_kind`). + let kind = M::cursor_kind(field); let key_str = encode_cursor_value(&value, kind).map_err(|_| ODataError::InvalidCursor)?; cursor_keys.push(key_str); } @@ -908,3 +936,36 @@ where d: direction.to_owned(), }) } + +#[cfg(test)] +mod cursor_codec_tests { + use super::*; + + // Pin the storage-shaped integer arms: a mapper exposing a + // SMALLINT/INT column under `cursor_kind = I64` must round-trip its + // natural extracted width without hand-widening to BigInt. + #[test] + fn i64_kind_accepts_narrow_integer_values() { + for value in [ + sea_orm::Value::SmallInt(Some(2)), + sea_orm::Value::Int(Some(2)), + sea_orm::Value::BigInt(Some(2)), + ] { + let token = encode_cursor_value(&value, FieldKind::I64) + .expect("narrow integer must encode under I64"); + assert_eq!(token, "2"); + let parsed = parse_cursor_value(FieldKind::I64, &token).expect("token must parse back"); + assert!(matches!(parsed, sea_orm::Value::BigInt(Some(2)))); + } + } + + // The wire/storage mismatch that motivated `cursor_kind`: a + // SmallInt extracted under the wire String kind still fails loudly + // rather than producing a corrupt token. + #[test] + fn string_kind_rejects_integer_values() { + assert!( + encode_cursor_value(&sea_orm::Value::SmallInt(Some(2)), FieldKind::String).is_err() + ); + } +} From b04d0f4cf4f37c0d0f7faacce9a27fda9155a769 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:37:04 +0200 Subject: [PATCH 3/7] feat(account-management): classify IdP uniqueness and password-policy rejections POST /tenants/{id}/users with a duplicate username came back as the redacted generic 400 invalid_argument (field=request, reason=VALIDATION, 'identity provider rejected the user operation (detail redacted; ...)'). The client could not attribute the failure to the username field, which blocks the 'Add user dialogs - per-field and API error handling' acceptance criterion. Duplicate email and password-policy rejections collapsed into the same catch-all. SDK: extend the #[non_exhaustive] IdpUserOperationFailure with typed variants - DuplicateUser { field: IdpUserDuplicateField (Username | Email | UsernameOrEmail), detail } and PasswordPolicy { detail }; Rejected stays as the fallback for unattributable rejections, so legacy plugins keep today's behavior unchanged. UsernameOrEmail covers Keycloak's ModelDuplicateException path, which emits the combined constant 'User exists with same username or email' (on KC 26 the only 409 createUser produces directly) and must not be misattributed to username. New field/reason vocabulary: PASSWORD_FIELD, PASSWORD_POLICY. AM: map DuplicateUser to DomainError::UserAlreadyExists -> HTTP 409 already_exists on the user resource type, carrying the typed field so the canonical boundary derives both resource_name (the stable colliding -field token 'username'/'email', never the caller-supplied value) and the curated detail from one place; map PasswordPolicy to DomainError::IdpPasswordPolicy -> HTTP 400 with field_violations [{field: password, reason: PASSWORD_POLICY}] instead of the generic request/VALIDATION pair. Raw provider text stays digest-only in the am.idp log lines, same redaction posture as the existing arms. The vp-idp-plugin change that actually emits the classified variants from Keycloak 409/400 responses lands separately after the submodule bump - until then every path still produces Rejected and nothing changes on the wire. Signed-off-by: Diffora --- .../account-management-sdk/src/field.rs | 10 ++ .../account-management-sdk/src/idp_user.rs | 81 ++++++++++++++- .../account-management-sdk/src/lib.rs | 4 +- .../account-management/src/domain/error.rs | 29 ++++++ .../src/domain/error_tests.rs | 4 + .../src/domain/idp/idp_tests.rs | 98 +++++++++++++++++++ .../account-management/src/domain/idp/mod.rs | 36 +++++++ .../src/infra/sdk_error_mapping.rs | 22 +++++ .../src/infra/sdk_error_mapping_tests.rs | 71 ++++++++++++++ 9 files changed, 348 insertions(+), 7 deletions(-) diff --git a/gears/system/account-management/account-management-sdk/src/field.rs b/gears/system/account-management/account-management-sdk/src/field.rs index 01801c72d..b9c1dfe1b 100644 --- a/gears/system/account-management/account-management-sdk/src/field.rs +++ b/gears/system/account-management/account-management-sdk/src/field.rs @@ -45,6 +45,12 @@ pub const ROOT_TENANT_CANNOT_CHANGE_STATUS: &str = "ROOT_TENANT_CANNOT_CHANGE_ST /// `provisioning_metadata.realm_name`) — see [`PROVISIONING_METADATA_FIELD`]. pub const IDP_INVALID_INPUT: &str = "IDP_INVALID_INPUT"; +/// The `IdP` rejected the supplied password against its configured +/// password policy. Carried on the [`PASSWORD_FIELD`] +/// field-violation so clients can attribute the failure to the +/// password input; the raw policy text stays provider-side. +pub const PASSWORD_POLICY: &str = "PASSWORD_POLICY"; + // --------------------------------------------------------------------------- // `field_violations[].field` attribution keys. // @@ -69,6 +75,10 @@ pub const METADATA_FIELD: &str = "metadata"; /// [`ROOT_TENANT_CANNOT_CHANGE_STATUS`]). pub const TENANT_ID_FIELD: &str = "tenant_id"; +/// `password` field for `IdP` password-policy rejects (carries +/// [`PASSWORD_POLICY`]). +pub const PASSWORD_FIELD: &str = "password"; + /// Shared fallback field for [`IDP_INVALID_INPUT`] when the `IdP` /// plugin cannot localise the violation to a specific sub-key — the /// public surface every `IdP` plugin shares. diff --git a/gears/system/account-management/account-management-sdk/src/idp_user.rs b/gears/system/account-management/account-management-sdk/src/idp_user.rs index 73a508fab..8fad03aeb 100644 --- a/gears/system/account-management/account-management-sdk/src/idp_user.rs +++ b/gears/system/account-management/account-management-sdk/src/idp_user.rs @@ -711,11 +711,78 @@ pub enum IdpUserOperationFailure { /// `idp_unsupported_operation`. Providers MUST NOT silently no-op /// a mutating call. UnsupportedOperation { detail: String }, - /// Provider returned a payload-rejection category (e.g. duplicate - /// username, validation failure on email format). AM maps this to - /// the canonical validation envelope; the catalog refinement is - /// owned by `feature-errors-observability`. + /// Provider returned a payload-rejection the plugin could NOT + /// classify further. AM maps this to the canonical validation + /// envelope with the generic `request`/`VALIDATION` tokens. + /// Providers SHOULD emit the classified variants below + /// ([`Self::DuplicateUser`], [`Self::PasswordPolicy`]) whenever + /// the vendor response identifies the cause — this catch-all is + /// the fallback for genuinely unattributable rejections. Rejected { detail: String }, + /// Provider rejected the operation because a uniqueness invariant + /// on `field` is already taken (duplicate username in + /// the realm, duplicate email realm-wide). AM maps this to the + /// canonical `already_exists` envelope (HTTP 409) — the create + /// contract already declares a Conflict response and documents + /// idempotency keyed by `(tenant_id, username)`. + DuplicateUser { + field: IdpUserDuplicateField, + detail: String, + }, + /// Provider rejected the supplied password against its configured + /// password policy (length / complexity / history). AM maps this + /// to the canonical validation envelope with the structured + /// `password` / `PASSWORD_POLICY` field-violation tokens so + /// clients can attribute the failure to the password field + ///; the raw policy text stays provider-side. + PasswordPolicy { detail: String }, +} + +/// Which unique user attribute an [`IdpUserOperationFailure::DuplicateUser`] +/// rejection collided on. `#[non_exhaustive]`: providers may learn to +/// classify further unique attributes without a breaking SDK release. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum IdpUserDuplicateField { + /// Realm-scoped username collision. + Username, + /// Realm-wide email collision. + Email, + /// The provider reported a uniqueness conflict without an + /// attributable single field. Keycloak's `ModelDuplicateException` + /// path emits the combined constant "User exists with same + /// username or email" — on KC 26 that is the ONLY 409 shape + /// `createUser` produces directly — and a conflict body the plugin + /// cannot parse lands here too: on the user-create endpoint a 409 + /// has no other cause than a uniqueness collision, so the status + /// alone justifies the conflict classification even when the field + /// stays unknown. + UsernameOrEmail, +} + +impl IdpUserDuplicateField { + /// Stable wire/field token for canonical `field_violations.field` + /// / `resource_name` and metric labels. + #[must_use] + pub const fn as_field_token(self) -> &'static str { + match self { + Self::Username => "username", + Self::Email => "email", + Self::UsernameOrEmail => "username_or_email", + } + } + + /// Human phrase for curated public error details ("a user with + /// this {phrase} already exists"). Centralised here so AM's + /// canonical boundary and its tests cannot drift. + #[must_use] + pub const fn as_human_phrase(self) -> &'static str { + match self { + Self::Username => "username", + Self::Email => "email", + Self::UsernameOrEmail => "username or email", + } + } } impl IdpUserOperationFailure { @@ -731,6 +798,8 @@ impl IdpUserOperationFailure { Self::Unavailable { .. } => "unavailable", Self::UnsupportedOperation { .. } => "unsupported_operation", Self::Rejected { .. } => "rejected", + Self::DuplicateUser { .. } => "duplicate_user", + Self::PasswordPolicy { .. } => "password_policy", } } @@ -743,7 +812,9 @@ impl IdpUserOperationFailure { match self { Self::Unavailable { detail } | Self::UnsupportedOperation { detail } - | Self::Rejected { detail } => detail, + | Self::Rejected { detail } + | Self::DuplicateUser { detail, .. } + | Self::PasswordPolicy { detail } => detail, } } } diff --git a/gears/system/account-management/account-management-sdk/src/lib.rs b/gears/system/account-management/account-management-sdk/src/lib.rs index 9934196d4..ead7503bd 100644 --- a/gears/system/account-management/account-management-sdk/src/lib.rs +++ b/gears/system/account-management/account-management-sdk/src/lib.rs @@ -59,8 +59,8 @@ pub use idp::{ }; pub use idp_user::{ IdpDeprovisionUserRequest, IdpListUsersRequest, IdpNewUser, IdpProvisionUserRequest, - IdpTenantContext, IdpUser, IdpUserFilterField, IdpUserOperationFailure, IdpUserPagination, - IdpUserPaginationError, IdpUserQuery, ListUsersQuery, NewUserPassword, + IdpTenantContext, IdpUser, IdpUserDuplicateField, IdpUserFilterField, IdpUserOperationFailure, + IdpUserPagination, IdpUserPaginationError, IdpUserQuery, ListUsersQuery, NewUserPassword, }; pub use metadata::{ MetadataEntry, MetadataEntryFilterField, MetadataEntryQuery, UpsertMetadataRequest, diff --git a/gears/system/account-management/account-management/src/domain/error.rs b/gears/system/account-management/account-management/src/domain/error.rs index c1b27121e..a12d57364 100644 --- a/gears/system/account-management/account-management/src/domain/error.rs +++ b/gears/system/account-management/account-management/src/domain/error.rs @@ -151,6 +151,35 @@ pub enum DomainError { #[error("already exists: {detail}")] AlreadyExists { detail: String }, + /// The `IdP` reported a uniqueness collision on a user attribute + /// during a user operation (duplicate username in the + /// realm, duplicate email realm-wide, or KC's combined + /// "username or email" constant). Distinct from + /// [`Self::AlreadyExists`] (tenant-flavoured, DB-classifier + /// produced) so the canonical envelope rides the `user` resource + /// type. Carries the typed colliding field — the boundary mapping + /// derives both the stable `resource_name` token and the curated + /// public detail from it, so the wording lives in exactly one + /// place and no caller can pair an inconsistent detail/resource. + /// The caller-supplied value is never echoed (redaction posture of + /// the `IdP` boundary). Surfaces as HTTP 409 `already_exists`. + #[error("user already exists ({})", field.as_field_token())] + UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField, + }, + + /// The `IdP` rejected the supplied password against its configured + /// password policy. Distinct from [`Self::Validation`] + /// so the canonical envelope carries the structured + /// `password` / `PASSWORD_POLICY` field-violation tokens instead + /// of the generic `request` / `VALIDATION` pair — clients can + /// attribute the failure to the password input. The raw policy + /// text is redacted upstream (digest in `am.idp` logs); `detail` + /// is the curated public summary. Surfaces as HTTP 400 + /// `invalid_argument`. + #[error("password rejected by IdP policy: {detail}")] + IdpPasswordPolicy { detail: String }, + // ---- Aborted (HTTP 409) ---- /// Retry-budget-exhausted serialization failure surfaced by /// [`crate::infra::storage::repo_impl::helpers::with_serializable_retry`] diff --git a/gears/system/account-management/account-management/src/domain/error_tests.rs b/gears/system/account-management/account-management/src/domain/error_tests.rs index a77a7c91e..7b86c1af8 100644 --- a/gears/system/account-management/account-management/src/domain/error_tests.rs +++ b/gears/system/account-management/account-management/src/domain/error_tests.rs @@ -316,6 +316,8 @@ impl DomainError { Self::MetadataEntryNotFound { .. } => "metadata_entry_not_found", Self::MetadataVersionMismatch { .. } => "metadata_version_mismatch", Self::AlreadyExists { .. } => "already_exists", + Self::UserAlreadyExists { .. } => "user_already_exists", + Self::IdpPasswordPolicy { .. } => "idp_password_policy", Self::Aborted { .. } => "aborted", Self::TypeNotAllowed { .. } => "type_not_allowed", Self::TenantDepthExceeded { .. } => "tenant_depth_exceeded", @@ -365,12 +367,14 @@ impl DomainError { | Self::PendingExists { .. } | Self::AlreadyResolved | Self::Conflict { .. } + | Self::IdpPasswordPolicy { .. } | Self::FeatureDisabled { .. } => 400, Self::NotFound { .. } | Self::UserNotFound { .. } | Self::ConversionRequestNotFound { .. } | Self::MetadataEntryNotFound { .. } => 404, Self::AlreadyExists { .. } + | Self::UserAlreadyExists { .. } | Self::Aborted { .. } | Self::MetadataVersionMismatch { .. } => 409, Self::CrossTenantDenied { .. } => 403, diff --git a/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs b/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs index 53818cd1a..a8e5a460b 100644 --- a/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs +++ b/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs @@ -108,3 +108,101 @@ fn provision_ambiguous_chains_redacted_cause_without_leaking_raw_detail() { "DomainError::Internal::cause MUST be reachable via Error::source" ); } + +// --------------------------------------------------------------------------- +// Classified user-operation rejections. +// --------------------------------------------------------------------------- + +#[test] +fn user_duplicate_username_maps_to_user_already_exists() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::Username, + detail: "User exists with same username".into(), + } + .into_domain_error(fixture_tenant_id()); + // The typed field survives verbatim; the curated public wording is + // derived (once) at the canonical boundary, so raw provider text + // cannot leak from here by construction. + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::Username + } + ), + "expected UserAlreadyExists(Username), got {err:?}" + ); +} + +#[test] +fn user_duplicate_email_maps_to_user_already_exists() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::Email, + detail: "User exists with same email".into(), + } + .into_domain_error(fixture_tenant_id()); + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::Email + } + ), + "expected UserAlreadyExists(Email), got {err:?}" + ); +} + +// KC's ModelDuplicateException path emits the combined constant "User +// exists with same username or email" (on KC 26 it is the only 409 +// `createUser` produces directly) — the unattributable classification +// must survive to the domain error, not collapse into Username. +#[test] +fn user_duplicate_combined_maps_to_username_or_email() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::UsernameOrEmail, + detail: "User exists with same username or email".into(), + } + .into_domain_error(fixture_tenant_id()); + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::UsernameOrEmail + } + ), + "expected UserAlreadyExists(UsernameOrEmail), got {err:?}" + ); +} + +#[test] +fn user_password_policy_maps_to_structured_password_violation() { + let err = IdpUserOperationFailure::PasswordPolicy { + detail: "invalidPasswordMinLengthMessage: 12".into(), + } + .into_domain_error(fixture_tenant_id()); + let DomainError::IdpPasswordPolicy { detail } = err else { + panic!("expected IdpPasswordPolicy, got a different variant"); + }; + // Curated public summary only — the raw KC policy text stays in + // the digest-only log line. + assert!( + !detail.contains("invalidPasswordMinLengthMessage"), + "raw provider policy text leaked into public detail: {detail}" + ); +} + +// The unclassified catch-all keeps its historical shape: a provider +// rejection the plugin could not attribute still collapses to the +// redacted generic Validation (no accidental behavior change for +// legacy plugins that only emit `Rejected`). +#[test] +fn user_rejected_still_maps_to_redacted_validation() { + let err = IdpUserOperationFailure::Rejected { + detail: "something vendor-specific".into(), + } + .into_domain_error(fixture_tenant_id()); + let DomainError::Validation { detail } = err else { + panic!("expected Validation, got a different variant"); + }; + assert!(detail.contains("detail redacted"), "got: {detail}"); +} diff --git a/gears/system/account-management/account-management/src/domain/idp/mod.rs b/gears/system/account-management/account-management/src/domain/idp/mod.rs index 4632f887a..b81126256 100644 --- a/gears/system/account-management/account-management/src/domain/idp/mod.rs +++ b/gears/system/account-management/account-management/src/domain/idp/mod.rs @@ -361,6 +361,42 @@ impl UserOperationFailureExt for IdpUserOperationFailure { } // @cpt-end:cpt-cf-account-management-algo-idp-user-operations-contract-idp-contract-invocation:p1:inst-algo-ici-provider-error-return // @cpt-end:cpt-cf-account-management-algo-idp-user-operations-contract-idp-contract-invocation:p1:inst-algo-ici-provider-error + // Classified uniqueness collision: surface HTTP + // 409 `already_exists` with the stable colliding-field + // token, instead of collapsing into the redacted generic + // Validation. The provider detail is still digest-only in + // logs — the public detail is derived from the typed field + // at the canonical boundary. + Self::DuplicateUser { field, detail } => { + let (digest, len) = redact_provider_detail(&detail); + tracing::warn!( + target: "am.idp", + tenant_id = %tenant_id, + field = field.as_field_token(), + provider_detail_digest = digest, + provider_detail_len = len, + "IdP user operation DuplicateUser; surfacing already_exists, raw detail redacted" + ); + DomainError::UserAlreadyExists { field } + } + // Classified password-policy reject: surface + // the structured `password` / `PASSWORD_POLICY` violation + // instead of the generic `request` / `VALIDATION`. Raw + // policy text stays digest-only in logs. + Self::PasswordPolicy { detail } => { + let (digest, len) = redact_provider_detail(&detail); + tracing::warn!( + target: "am.idp", + tenant_id = %tenant_id, + provider_detail_digest = digest, + provider_detail_len = len, + "IdP user operation PasswordPolicy; surfacing password field violation, raw detail redacted" + ); + DomainError::IdpPasswordPolicy { + detail: "the supplied password does not meet the identity provider's password policy" + .to_owned(), + } + } // SDK enum is `#[non_exhaustive]`. A new variant added in // a future SDK release lands here until the AM-side // mapping is updated; surface as `Internal` with a loud diff --git a/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs b/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs index 818e54b85..10f69f59b 100644 --- a/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs +++ b/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs @@ -141,6 +141,17 @@ impl From for CanonicalError { account_management_sdk::field::IDP_INVALID_INPUT, ) .create(), + // IdP password-policy reject: structured + // `password` / `PASSWORD_POLICY` tokens on the `user` + // resource, so clients attribute the 400 to the password + // input instead of the generic `request` / `VALIDATION`. + DomainError::IdpPasswordPolicy { detail } => UserResource::invalid_argument() + .with_field_violation( + account_management_sdk::field::PASSWORD_FIELD, + detail, + account_management_sdk::field::PASSWORD_POLICY, + ) + .create(), // ---- NotFound (HTTP 404) — one resource per variant ---- DomainError::NotFound { detail, resource } => TenantResource::not_found(detail) @@ -184,6 +195,17 @@ impl From for CanonicalError { DomainError::AlreadyExists { detail } => TenantResource::already_exists(detail) .with_resource("tenant") .create(), + // IdP-reported user uniqueness collision: both + // the curated public detail and the stable `resource_name` + // token derive from the typed field here — the single + // source of the wording; the caller-supplied value is + // never echoed. + DomainError::UserAlreadyExists { field } => UserResource::already_exists(format!( + "a user with this {} already exists", + field.as_human_phrase() + )) + .with_resource(field.as_field_token()) + .create(), // Duplicate-on-create per AIP-193: the at-most-one-pending // invariant surfaces as HTTP 409 with the existing // `request_id` as the structural resource identifier. diff --git a/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs b/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs index b3d36a9e8..4ea241b40 100644 --- a/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs +++ b/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs @@ -267,6 +267,77 @@ fn already_exists_maps_to_409() { ); } +/// An `IdP`-reported user uniqueness collision surfaces as +/// 409 `already_exists` on the USER resource type, with the stable +/// colliding-field token as `resource_name` and a detail derived from +/// the typed field — never the caller-supplied value and never the raw +/// provider text. +#[test] +fn user_already_exists_maps_to_409_with_user_resource() { + for (field, token, phrase) in [ + ( + account_management_sdk::IdpUserDuplicateField::Username, + "username", + "a user with this username already exists", + ), + ( + account_management_sdk::IdpUserDuplicateField::Email, + "email", + "a user with this email already exists", + ), + ( + account_management_sdk::IdpUserDuplicateField::UsernameOrEmail, + "username_or_email", + "a user with this username or email already exists", + ), + ] { + let canonical = round_trip(DomainError::UserAlreadyExists { field }); + assert_eq!(canonical.status_code(), 409); + assert_eq!(canonical.resource_name(), Some(token)); + assert_eq!( + canonical.resource_type(), + Some(account_management_sdk::gts::USER_RESOURCE_TYPE) + ); + assert!( + matches!(canonical, CanonicalError::AlreadyExists { .. }), + "UserAlreadyExists MUST surface as the AlreadyExists variant" + ); + assert_eq!(canonical.detail(), phrase); + } +} + +/// An `IdP` password-policy reject carries the structured +/// `password` / `PASSWORD_POLICY` field-violation tokens (not the +/// generic `request` / `VALIDATION` pair) so clients can attribute +/// the 400 to the password input without parsing `detail`. +#[test] +fn idp_password_policy_maps_to_400_with_password_field_violation() { + let canonical = round_trip(DomainError::IdpPasswordPolicy { + detail: "the supplied password does not meet the identity provider's password policy" + .to_owned(), + }); + assert_eq!(canonical.status_code(), 400); + assert_eq!( + canonical.resource_type(), + Some(account_management_sdk::gts::USER_RESOURCE_TYPE) + ); + let CanonicalError::InvalidArgument { ctx, .. } = canonical else { + panic!("expected CanonicalError::InvalidArgument"); + }; + let InvalidArgument::FieldViolations { field_violations } = ctx else { + panic!("expected InvalidArgument::FieldViolations ctx"); + }; + assert_eq!(field_violations.len(), 1); + assert_eq!( + field_violations[0].field, + account_management_sdk::field::PASSWORD_FIELD + ); + assert_eq!( + field_violations[0].reason, + account_management_sdk::field::PASSWORD_POLICY + ); +} + // --------------------------------------------------------------------------- // Aborted (HTTP 409 with reason) // --------------------------------------------------------------------------- From 9c7ad2c97a4d62e2da764cd45ddc6bceed05a2f9 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:37:18 +0200 Subject: [PATCH 4/7] fix(account-management): cancel pending conversion in the tenant soft-delete TX DELETE /tenants/{id} tombstoned the tenant but left any pending mode-conversion request row dangling. schedule_deletion's SERIALIZABLE TX now terminal-resolves the pending conversion (cancelled_by = the deleting actor) inside the same transaction, so a committed soft-delete never leaves an orphaned conversion behind. The already-Deleted idempotent short-circuit also runs the same (Pending-fenced) cancel cascade, so tenants soft-deleted BEFORE the cascade existed are healed by the natural operator remediation - retrying the DELETE; post-fix tombstones make it a no-op. The conversion auto-cancel am.events line is emitted AFTER the SERIALIZABLE TX commits (the count rides the closure return): logging inside the closure would double-count on a 40001 retry and emit a phantom event when the TX ultimately rolled back. Signed-off-by: Diffora --- .../src/domain/tenant/repo.rs | 11 +- .../src/domain/tenant/service/mod.rs | 2 +- .../domain/tenant/service/service_tests.rs | 1 + .../src/domain/tenant/test_support/repo.rs | 7 + .../src/infra/storage/repo_impl/conversion.rs | 60 +++++++ .../src/infra/storage/repo_impl/mod.rs | 3 +- .../src/infra/storage/repo_impl/updates.rs | 51 +++++- .../tests/conversion_integration.rs | 157 ++++++++++++++++++ .../tests/lifecycle_integration.rs | 4 +- .../tests/metadata_integration.rs | 1 + .../tests/metadata_integration_pg.rs | 1 + .../tests/updates_integration.rs | 9 +- 12 files changed, 296 insertions(+), 11 deletions(-) diff --git a/gears/system/account-management/account-management/src/domain/tenant/repo.rs b/gears/system/account-management/account-management/src/domain/tenant/repo.rs index e2c710c83..2d88cd3f9 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/repo.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/repo.rs @@ -405,8 +405,14 @@ pub trait TenantRepo: Send + Sync { /// Flip the tenant from its current SDK-visible state to /// `Deleted`, stamp `deleted_at = now` (which also starts the /// retention timer — eligibility for hard-delete becomes - /// `deleted_at + retention_window`), and rewrite - /// `tenant_closure.descendant_status` in the same transaction. + /// `deleted_at + retention_window`), rewrite + /// `tenant_closure.descendant_status`, and terminal-resolve + /// (cancel) any still-pending mode-conversion request whose + /// subject is this tenant — all in the same transaction + /// (a committed soft-delete must never leave a + /// `pending` conversion row referencing the tombstone). + /// `deleted_by` is stamped as the auto-cancelled request's + /// `cancelled_by`. /// /// **Idempotent.** Calling on a row that is already in `Deleted` /// status returns the existing tombstone without re-stamping @@ -421,6 +427,7 @@ pub trait TenantRepo: Send + Sync { &self, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result; diff --git a/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs b/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs index 8d107b375..8bb6f872c 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs @@ -1877,7 +1877,7 @@ impl TenantService { }; let updated = self .repo - .schedule_deletion(&scope, tenant_id, now, retention) + .schedule_deletion(&scope, tenant_id, ctx.subject_id(), now, retention) .await?; // TODO(events): emit AM event when platform event-bus lands. tracing::info!( diff --git a/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs b/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs index a1d93070c..39500bc08 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs @@ -1924,6 +1924,7 @@ async fn hard_delete_batch_skips_rows_already_claimed_by_another_worker() { .schedule_deletion( &AccessScope::allow_all(), id, + Uuid::nil(), now, Some(StdDuration::from_secs(0)), ) diff --git a/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs b/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs index 7cf750cd6..c2b5a39b3 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs @@ -1323,10 +1323,14 @@ impl TenantRepo for FakeTenantRepo { Ok(u64::try_from(state.closure.len()).unwrap_or(u64::MAX)) } + // `_deleted_by` is unused: the in-memory fake models the `tenants` + // table only — the conversion auto-cancel cascade is a + // real-repo TX concern covered by the sqlite integration tests. async fn schedule_deletion( &self, _scope: &AccessScope, id: Uuid, + _deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { @@ -1735,6 +1739,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), parent, + Uuid::nil(), ts(1_700_000_100), Some(Duration::from_secs(0)), ) @@ -1767,6 +1772,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), leaf, + Uuid::nil(), ts(1_700_000_100), Some(Duration::from_secs(0)), ) @@ -1792,6 +1798,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), leaf, + Uuid::nil(), ts(1_700_000_500), Some(Duration::from_secs(0)), ) diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs index 24b22294a..b984dda9b 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs @@ -493,6 +493,66 @@ async fn find_pending_for_tenant( /// [`DomainError::NotFound`] from [`DomainError::AlreadyResolved`] per /// the trait contract; on `rows_affected == 1` re-reads the row to /// return the post-transition snapshot to the caller. +/// Terminal-resolve (cancel) every still-pending conversion request +/// whose subject tenant is being soft-deleted, INSIDE the caller's +/// transaction. Called by the tenant repo's +/// `schedule_deletion` SERIALIZABLE TX so the tenant flip and the +/// conversion resolution commit or roll back together — a `204` from +/// DELETE `/tenants/{id}` can never leave a `pending` row dangling on +/// a tombstoned tenant. Lives here (not in `updates.rs`) so ownership +/// of the `conversion_requests` guarded-transition shape stays with +/// the conversion repo. +/// +/// Only rows keyed by `tenant_id = :id` can exist at this point: a +/// parent with non-deleted children is rejected by the +/// `TenantHasChildren` guard before the TX, so inbound conversions of +/// live children never reach this path. +/// +/// `allow_all`: this is a structural cascade inside an +/// already-authorized delete (PDP gate at the service layer), same +/// rationale as the closure rewrite in the same TX. The +/// `status = Pending AND deleted_at IS NULL` fence makes the cascade +/// idempotent — a soft-delete retry finds no pending row and affects +/// zero rows. Returns `rows_affected`. +pub(super) async fn cancel_pending_on_tenant_delete_tx( + tx: &DbTx<'_>, + tenant_id: Uuid, + cancelled_by: Uuid, + resolved_at: OffsetDateTime, +) -> Result { + let res = conversion_requests::Entity::update_many() + .col_expr( + conversion_requests::Column::Status, + Expr::value(ConversionStatus::Cancelled.as_smallint()), + ) + .col_expr( + conversion_requests::Column::CancelledBy, + Expr::value(Some(cancelled_by)), + ) + .col_expr( + conversion_requests::Column::ResolvedAt, + Expr::value(Some(resolved_at)), + ) + .col_expr( + conversion_requests::Column::CancelledComment, + Expr::value(Some("subject tenant deleted".to_owned())), + ) + .filter( + Condition::all() + .add(conversion_requests::Column::TenantId.eq(tenant_id)) + .add( + conversion_requests::Column::Status.eq(ConversionStatus::Pending.as_smallint()), + ) + .add(conversion_requests::Column::DeletedAt.is_null()), + ) + .secure() + .scope_with(&AccessScope::allow_all()) + .exec(tx) + .await + .map_err(map_scope_to_tx)?; + Ok(res.rows_affected) +} + async fn run_guarded_transition( repo: &ConversionRepoImpl, scope: &AccessScope, diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs index 3bd62ab70..bf9f6695f 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs @@ -225,10 +225,11 @@ impl TenantRepo for TenantRepoImpl { &self, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { - updates::schedule_deletion(self, scope, id, now, retention).await + updates::schedule_deletion(self, scope, id, deleted_by, now, retention).await } async fn check_hard_delete_eligibility( diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs index 074cffeb0..fe2b2d088 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs @@ -435,6 +435,7 @@ pub(super) async fn schedule_deletion( repo: &TenantRepoImpl, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { @@ -486,7 +487,21 @@ pub(super) async fn schedule_deletion( // un-contended retry path; this guard is the // authoritative correctness boundary. if existing.status == TenantStatus::Deleted.as_smallint() { - return entity_to_model(existing).map_err(TxError::Domain); + // Heal-on-retry: tenants soft-deleted BEFORE + // the conversion cascade existed can still carry a + // pending conversion row, and retrying the DELETE is + // the natural operator remediation — so the + // (idempotent, Pending-fenced) cancel runs on this + // path too instead of returning early past it. For + // post-fix tombstones the fence matches zero rows and + // this is a no-op. + let healed = super::conversion::cancel_pending_on_tenant_delete_tx( + tx, id, deleted_by, now, + ) + .await?; + return entity_to_model(existing) + .map(|m| (m, healed)) + .map_err(TxError::Domain); } // `Provisioning` rows have no closure entries by // construction; flipping them straight to `Deleted` @@ -595,6 +610,22 @@ pub(super) async fn schedule_deletion( } // @cpt-end:cpt-cf-account-management-algo-tenant-hierarchy-management-closure-maintenance:p1:inst-algo-closmnt-repo-soft-delete-status + // A pending mode-conversion request whose + // subject is this tenant must not outlive it — resolve + // (cancel) it in the SAME transaction as the status + // flip, so a committed soft-delete can never leave a + // `pending` row referencing a tombstone. The guarded + // UPDATE's `status = Pending` fence keeps concurrent / + // repeated deletes from re-stamping resolved rows. The + // count rides the closure's return value: the + // `am.events` emission happens AFTER the TX commits — + // logging in here would double-count on a serialization + // retry and emit a phantom event if the TX ultimately + // rolled back. + let cancelled = + super::conversion::cancel_pending_on_tenant_delete_tx(tx, id, deleted_by, now) + .await?; + let fresh = tenants::Entity::find() .secure() .scope_with(&scope) @@ -606,9 +637,25 @@ pub(super) async fn schedule_deletion( diagnostic: format!("tenant {id} disappeared after schedule_deletion"), cause: None, })?; - entity_to_model(fresh).map_err(TxError::Domain) + entity_to_model(fresh) + .map(|m| (m, cancelled)) + .map_err(TxError::Domain) }) }) }) .await + .map(|(model, cancelled)| { + if cancelled > 0 { + tracing::info!( + target: "am.events", + kind = "conversionStateChanged", + tenant_id = %id, + cancelled_by = %deleted_by, + rows = cancelled, + event = "conversion_auto_cancelled_on_tenant_delete", + "am conversion request auto-cancelled by tenant soft-delete" + ); + } + model + }) } diff --git a/gears/system/account-management/account-management/tests/conversion_integration.rs b/gears/system/account-management/account-management/tests/conversion_integration.rs index 8c90797a0..fd35b87fc 100644 --- a/gears/system/account-management/account-management/tests/conversion_integration.rs +++ b/gears/system/account-management/account-management/tests/conversion_integration.rs @@ -1114,3 +1114,160 @@ async fn approve_recomputes_barrier_for_closure_rows_referencing_soft_deleted_de .expect("(mid, leaf) row"); assert_eq!(row_mid_leaf.barrier, 0); } + +// --------------------------------------------------------------------------- +// Tenant soft-delete auto-cancels the pending conversion in-TX. +// --------------------------------------------------------------------------- + +/// Seed a pending conversion for `child` and return its id. +async fn seed_pending_conversion( + conv_repo: &ConversionRepoImpl, + child: Uuid, + parent: Uuid, + now: OffsetDateTime, +) -> Uuid { + let new = NewConversionRequest { + id: Uuid::new_v4(), + tenant_id: child, + parent_id: Some(parent), + child_tenant_name: "c".into(), + initiator_side: ConversionSide::Parent, + target_mode: TargetMode::SelfManaged, + requested_by: Uuid::new_v4(), + requested_at: now, + expires_at: now + TimeDuration::days(7), + requested_comment: None, + }; + conv_repo + .insert_pending(&allow_all(), &new) + .await + .expect("seed pending conversion") + .id +} + +/// DELETE `/tenants/{id}` with an open conversion used to return 204 and +/// leave the row `pending` with a dangling FK onto the tombstone. The +/// cascade now runs inside `schedule_deletion`'s SERIALIZABLE TX: the +/// tenant flip and the cancel commit together. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_cancels_pending_conversion_in_same_tx() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9100); + let child = Uuid::from_u128(0x9101); + seed_root(&h, root).await; + create_active_child(&h, child, root, "vhp1729-c", false, 1).await; + + let now = fixed_now(); + let request_id = seed_pending_conversion(&conv_repo, child, root, now).await; + let deleter = Uuid::from_u128(0x00DE_1E7E); + + let tombstone = h + .repo + .schedule_deletion(&allow_all(), child, deleter, now, None) + .await + .expect("soft delete"); + assert_eq!(tombstone.status, TenantStatus::Deleted); + + let resolved = conv_repo + .find_by_id(&allow_all(), request_id) + .await + .expect("find") + .expect("row still exists (resolved, not removed)"); + assert_eq!( + resolved.status, + ConversionStatus::Cancelled, + "pending conversion MUST be terminal-resolved by the delete TX" + ); + assert_eq!(resolved.cancelled_by, Some(deleter)); + assert_eq!(resolved.resolved_at, Some(now)); + // No pending row survives for the tombstoned tenant. + assert!( + conv_repo + .find_pending_for_tenant(&allow_all(), child) + .await + .expect("query") + .is_none(), + "no pending conversion may reference a tombstoned tenant" + ); +} + +/// Idempotent delete retry must not re-stamp the resolved row, and an +/// already-resolved (cancelled) conversion is left untouched. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_retry_leaves_resolved_conversion_untouched() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9200); + let child = Uuid::from_u128(0x9201); + seed_root(&h, root).await; + create_active_child(&h, child, root, "vhp1729-r", false, 1).await; + + let now = fixed_now(); + let request_id = seed_pending_conversion(&conv_repo, child, root, now).await; + let first_deleter = Uuid::from_u128(0xD1); + + h.repo + .schedule_deletion(&allow_all(), child, first_deleter, now, None) + .await + .expect("first delete"); + // Retry with a DIFFERENT actor and later timestamp: the idempotent + // short-circuit + the `status = Pending` fence must preserve the + // first resolution verbatim. + let later = now + TimeDuration::hours(1); + h.repo + .schedule_deletion(&allow_all(), child, Uuid::from_u128(0xD2), later, None) + .await + .expect("idempotent retry"); + + let resolved = conv_repo + .find_by_id(&allow_all(), request_id) + .await + .expect("find") + .expect("row"); + assert_eq!(resolved.status, ConversionStatus::Cancelled); + assert_eq!( + resolved.cancelled_by, + Some(first_deleter), + "retry must not re-stamp cancelled_by" + ); + assert_eq!( + resolved.resolved_at, + Some(now), + "retry must not move resolved_at" + ); +} + +/// The cascade is keyed by `tenant_id`: deleting one child must not +/// touch a sibling's pending conversion. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_does_not_touch_other_tenants_conversions() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9300); + let doomed = Uuid::from_u128(0x9301); + let sibling = Uuid::from_u128(0x9302); + seed_root(&h, root).await; + create_active_child(&h, doomed, root, "vhp1729-d", false, 1).await; + create_active_child(&h, sibling, root, "vhp1729-s", false, 1).await; + + let now = fixed_now(); + seed_pending_conversion(&conv_repo, doomed, root, now).await; + let sibling_request = seed_pending_conversion(&conv_repo, sibling, root, now).await; + + h.repo + .schedule_deletion(&allow_all(), doomed, Uuid::nil(), now, None) + .await + .expect("delete doomed"); + + let untouched = conv_repo + .find_by_id(&allow_all(), sibling_request) + .await + .expect("find") + .expect("row"); + assert_eq!( + untouched.status, + ConversionStatus::Pending, + "sibling's pending conversion MUST survive" + ); +} diff --git a/gears/system/account-management/account-management/tests/lifecycle_integration.rs b/gears/system/account-management/account-management/tests/lifecycle_integration.rs index e8ed7f323..fe76d8fca 100644 --- a/gears/system/account-management/account-management/tests/lifecycle_integration.rs +++ b/gears/system/account-management/account-management/tests/lifecycle_integration.rs @@ -442,7 +442,7 @@ async fn schedule_deletion_rewrites_closure_to_deleted() { let now = OffsetDateTime::now_utc(); let outcome = h .repo - .schedule_deletion(&allow_all(), leaf, now, None) + .schedule_deletion(&allow_all(), leaf, Uuid::nil(), now, None) .await .expect("schedule_deletion"); assert_eq!(outcome.status, TenantStatus::Deleted); @@ -598,6 +598,7 @@ async fn hard_delete_one_cleans_tenant_and_closure_rows() { .schedule_deletion( &allow_all(), leaf, + Uuid::nil(), scheduled_at, Some(Duration::from_secs(1)), ) @@ -680,6 +681,7 @@ async fn hard_delete_one_rejects_lost_claim() { .schedule_deletion( &allow_all(), leaf, + Uuid::nil(), scheduled_at, Some(Duration::from_secs(1)), ) diff --git a/gears/system/account-management/account-management/tests/metadata_integration.rs b/gears/system/account-management/account-management/tests/metadata_integration.rs index 8b3625691..ebf4c04d0 100644 --- a/gears/system/account-management/account-management/tests/metadata_integration.rs +++ b/gears/system/account-management/account-management/tests/metadata_integration.rs @@ -468,6 +468,7 @@ async fn hard_delete_cascades_metadata_rows_for_target_tenant_only() { .schedule_deletion( &allow_all(), target, + Uuid::nil(), now, Some(Duration::ZERO.unsigned_abs()), ) diff --git a/gears/system/account-management/account-management/tests/metadata_integration_pg.rs b/gears/system/account-management/account-management/tests/metadata_integration_pg.rs index 2d1174cc4..af5ade5da 100644 --- a/gears/system/account-management/account-management/tests/metadata_integration_pg.rs +++ b/gears/system/account-management/account-management/tests/metadata_integration_pg.rs @@ -267,6 +267,7 @@ async fn pg_hard_delete_one_clears_metadata_via_combined_path() { .schedule_deletion( &allow_all(), target, + Uuid::nil(), now, Some(Duration::ZERO.unsigned_abs()), ) diff --git a/gears/system/account-management/account-management/tests/updates_integration.rs b/gears/system/account-management/account-management/tests/updates_integration.rs index 1789ce6d6..d7200798f 100644 --- a/gears/system/account-management/account-management/tests/updates_integration.rs +++ b/gears/system/account-management/account-management/tests/updates_integration.rs @@ -287,7 +287,7 @@ async fn schedule_deletion_not_found() { let h = setup_sqlite().await.expect("setup"); let err = h .repo - .schedule_deletion(&allow_all(), Uuid::new_v4(), now(), None) + .schedule_deletion(&allow_all(), Uuid::new_v4(), Uuid::nil(), now(), None) .await .expect_err("missing"); assert!(matches!(err, DomainError::NotFound { .. }), "got {err:?}"); @@ -315,7 +315,7 @@ async fn schedule_deletion_idempotent_on_already_deleted() { let out = h .repo - .schedule_deletion(&allow_all(), id, now(), None) + .schedule_deletion(&allow_all(), id, Uuid::nil(), now(), None) .await .expect("idempotent"); assert_eq!(out.status, TenantStatus::Deleted); @@ -341,7 +341,7 @@ async fn schedule_deletion_rejects_provisioning() { let err = h .repo - .schedule_deletion(&allow_all(), id, now(), None) + .schedule_deletion(&allow_all(), id, Uuid::nil(), now(), None) .await .expect_err("provisioning cannot be soft-deleted"); assert!(matches!(err, DomainError::Conflict { .. }), "got {err:?}"); @@ -361,7 +361,7 @@ async fn schedule_deletion_rejects_when_live_children_present() { let err = h .repo - .schedule_deletion(&allow_all(), parent, now(), None) + .schedule_deletion(&allow_all(), parent, Uuid::nil(), now(), None) .await .expect_err("parent with a live child cannot be soft-deleted"); assert!(matches!(err, DomainError::TenantHasChildren), "got {err:?}"); @@ -383,6 +383,7 @@ async fn schedule_deletion_stamps_deleted_at_and_retention_window() { .schedule_deletion( &allow_all(), id, + Uuid::nil(), now(), Some(std::time::Duration::from_mins(15)), ) From 384fbe51242db71f8570e01e5330661297e92258 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:37:26 +0200 Subject: [PATCH 5/7] fix(account-management): platform-scope the user-groups cascade system actor The retention hard-delete reaper's resource-group cascade ran scoped to the tenant being deleted. The resolver's subtree grant is rooted at that tenant, but the tenant row is already Deleted, so the grant materializes to zero rows and the PDP fails closed - the cascade is denied and the whole hard-delete backlog is deferred forever, silently. Platform-scope the cascade's system-actor context (a nil home tenant resolves to Global at the resolver) so the reaper can clean up resource-group state for tenants it is tearing down. The resolver-side halves land separately after the submodule bump. Signed-off-by: Diffora --- .../src/domain/system_actor.rs | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/gears/system/account-management/account-management/src/domain/system_actor.rs b/gears/system/account-management/account-management/src/domain/system_actor.rs index 92bbf9185..2be68da86 100644 --- a/gears/system/account-management/account-management/src/domain/system_actor.rs +++ b/gears/system/account-management/account-management/src/domain/system_actor.rs @@ -151,7 +151,18 @@ pub(crate) fn for_retention_sweep(tenant_id: Uuid) -> SecurityContext { /// User-groups cascade-cleanup hook — fired when a tenant is /// hard-deleted and AM must walk its user-group memberships in RG. -/// `tenant_id` is the tenant being deleted. +/// `tenant_id` is the tenant being deleted; it rides the audit log +/// line only. +/// +/// Platform-scoped, NOT scoped to `tenant_id`: the subject +/// tenant here is by definition a `Deleted` row mid-purge, and the +/// `AuthZ` resolver's system-actor grant would root a tenant-subtree +/// scope at it — deleted roots are clamped out of scope +/// materialization, the subtree resolves to zero tenants, and every +/// RG cascade call fails closed. The reaper then defers the whole +/// backlog forever. Platform scope (nil tenant → Global grant) is +/// honest for this flow: the cascade addresses explicit group ids of +/// a tenant that is being erased, not a live tenant's data. #[must_use] pub(crate) fn for_user_groups_cascade(tenant_id: Uuid) -> SecurityContext { tracing::info!( @@ -160,7 +171,7 @@ pub(crate) fn for_user_groups_cascade(tenant_id: Uuid) -> SecurityContext { tenant_id = %tenant_id, "am system actor constructed", ); - build_inner(Some(tenant_id)) + build_inner(None) } #[cfg(test)] @@ -193,7 +204,6 @@ mod tests { ("bootstrap", for_bootstrap(tenant)), ("provisioning_reaper", for_provisioning_reaper(tenant)), ("retention_sweep", for_retention_sweep(tenant)), - ("user_groups_cascade", for_user_groups_cascade(tenant)), ] { assert_eq!( ctx.subject_id(), @@ -212,4 +222,20 @@ mod tests { ); } } + + // The cascade hook's subject tenant is a Deleted row + // mid-purge — a subtree grant rooted at it materializes to zero + // tenants and fails the whole reaper closed. The factory MUST be + // platform-scoped (nil tenant → Global grant at the resolver). + #[test] + fn user_groups_cascade_is_platform_scoped() { + let ctx = for_user_groups_cascade(Uuid::from_u128(0xDEAD_BEEF_FACE_CAFE)); + assert_eq!(ctx.subject_id(), AM_SYSTEM_ACTOR_UUID); + assert_eq!(ctx.subject_type(), Some(AM_SYSTEM_SUBJECT_TYPE)); + assert_eq!( + ctx.subject_tenant_id(), + Uuid::nil(), + "cascade context must be platform-scoped, not scoped to the deleted tenant" + ); + } } From 656228487dc61ed3417814101980e71b3eb76272 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 20:37:37 +0200 Subject: [PATCH 6/7] fix(resource-group): restore rg-namespace group resource type for PEP evaluation The gts-rust v0.11.0 migration routed the group resource-type literal through the crate-name-derived SDK constant, which silently renamed the PEP-evaluated type from gts.cf.core.rg.group.v1~ to gts.cf.core.resource_group.group.v1~. Every deployed role grant is written against the documented gts.cf.core.rg.* family, so the rename 403'd all group CRUD for tenant members and failed AM's tenant-delete ownership probe closed (surfaced as 503) - verified live: a grant on gts.cf.core.rg.* is denied while gts.cf.core.resource_group.* is allowed, on the same caller and flow. Pin GROUP_RESOURCE_TYPE back to cf.core.rg.group.v1~, consistent with the untouched siblings (rg.group_membership.v1~, rg.type.v1~), and realign the #[resource_error] tags + tests that the SDK round-trip tests pin to the constant. A code comment marks the literal as the PEP-evaluated type that must NOT be crate-derived, so a future mechanical migration does not silently rename it again. Adopting crate-derived namespaces intentionally would be a separate, coordinated migration: the whole type family at once, plus installer seeds and a data migration of existing grants. Signed-off-by: Diffora --- .../resource-group-sdk/src/error_tests.rs | 4 ++-- .../resource-group/resource-group-sdk/src/gts.rs | 10 +++++++++- .../resource-group/src/api/rest/error.rs | 2 +- .../resource-group/tests/domain_unit_test.rs | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/gears/system/resource-group/resource-group-sdk/src/error_tests.rs b/gears/system/resource-group/resource-group-sdk/src/error_tests.rs index e7c81908b..3fa5c9e65 100644 --- a/gears/system/resource-group/resource-group-sdk/src/error_tests.rs +++ b/gears/system/resource-group/resource-group-sdk/src/error_tests.rs @@ -25,7 +25,7 @@ mod wire_vocabulary_round_trip { // Its literal MUST equal `gts::GROUP_RESOURCE_TYPE` — the // `gts_resource_type_round_trip` test asserts that equality (the // proc-macro cannot reference the const directly). - #[resource_error(gts_id!("cf.core.resource_group.group.v1~"))] + #[resource_error(gts_id!("cf.core.rg.group.v1~"))] struct RgScope; fn problem(err: CanonicalError) -> serde_json::Value { @@ -132,7 +132,7 @@ mod projection_tests { use crate::{field, gts, precondition, reason}; use toolkit_canonical_errors::{CanonicalError, Problem, resource_error}; - #[resource_error(gts_id!("cf.core.resource_group.group.v1~"))] + #[resource_error(gts_id!("cf.core.rg.group.v1~"))] struct RgScope; #[test] diff --git a/gears/system/resource-group/resource-group-sdk/src/gts.rs b/gears/system/resource-group/resource-group-sdk/src/gts.rs index b5d771ce4..3083729eb 100644 --- a/gears/system/resource-group/resource-group-sdk/src/gts.rs +++ b/gears/system/resource-group/resource-group-sdk/src/gts.rs @@ -63,7 +63,15 @@ pub struct ResourceGroupTypeV1 { /// Match it against the resource-scoped projection variants /// ([`crate::ResourceGroupError::NotFound`] / /// [`crate::ResourceGroupError::AlreadyExists`]). -pub const GROUP_RESOURCE_TYPE: &str = gts_id!("cf.core.resource_group.group.v1~"); +// `rg` namespace, NOT the crate-name-derived `resource_group`: this +// string is the PEP-evaluated resource type for every group CRUD gate, +// and every deployed role grant is written against the documented +// `gts.cf.core.rg.*` family (siblings: `rg.group_membership.v1~`, +// `rg.type.v1~`). The gts-rust v0.11.0 migration mechanically swapped +// the literal for a crate-derived id and silently renamed the type — +// which 403'd every existing grant (tenant members could no longer +// create groups, AM's ownership probe on tenant delete failed closed). +pub const GROUP_RESOURCE_TYPE: &str = gts_id!("cf.core.rg.group.v1~"); /// Canonical GTS resource type for a resource-group membership link. pub const GROUP_MEMBERSHIP_RESOURCE_TYPE: &str = gts_id!("cf.core.rg.group_membership.v1~"); diff --git a/gears/system/resource-group/resource-group/src/api/rest/error.rs b/gears/system/resource-group/resource-group/src/api/rest/error.rs index 171eeb814..dfefdea51 100644 --- a/gears/system/resource-group/resource-group/src/api/rest/error.rs +++ b/gears/system/resource-group/resource-group/src/api/rest/error.rs @@ -18,7 +18,7 @@ use crate::domain::error::DomainError; /// The macro literal mirrors [`resource_group_sdk::gts::GROUP_RESOURCE_TYPE`] /// (proc-macros cannot resolve a const); the SDK round-trip tests pin the /// two equal. -#[resource_error(gts_id!("cf.core.resource_group.group.v1~"))] +#[resource_error(gts_id!("cf.core.rg.group.v1~"))] pub struct RgError; /// Implement `From for CanonicalError` so `?` works in diff --git a/gears/system/resource-group/resource-group/tests/domain_unit_test.rs b/gears/system/resource-group/resource-group/tests/domain_unit_test.rs index 424a92c5b..168b31291 100644 --- a/gears/system/resource-group/resource-group/tests/domain_unit_test.rs +++ b/gears/system/resource-group/resource-group/tests/domain_unit_test.rs @@ -34,7 +34,7 @@ const PERMISSION_DENIED_TYPE: &str = const INTERNAL_TYPE: &str = gts_uri!("cf.core.errors.err.v1~cf.core.err.internal.v1~"); /// Resource-group GTS prefix (matches `RgError`'s `#[resource_error(...)]`). -const RG_GTS: &str = gts_id!("cf.core.resource_group.group.v1~"); +const RG_GTS: &str = gts_id!("cf.core.rg.group.v1~"); // ── validate_type_code ────────────────────────────────────────────────── From c793ba598b87c5b35f6b7bc4a2dcc5b33f9cadb4 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 21:22:28 +0200 Subject: [PATCH 7/7] fix(account-management): count self_managed direct children in child_count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scope-selector counter on the Tenants page (the public `child_count` read-shape field) undercounted a parent whose direct children include a `self_managed` one: a parent with two active children where one is self_managed reported `child_count = 1`, while `GET /tenants/{id}/children` correctly listed both. Operator UIs render the wrong direct-child count. Root cause: `count_children_grouped` clamped each child row by the caller's barrier-respecting scope (`id IN (closure WHERE ancestor = root AND barrier = 0)`). A self_managed tenant's inbound closure edges carry `barrier = 1` even from its own parent, so a Respect-reachable parent's self_managed direct child was dropped from the count — even though the identity-level direct-child carve-out (see `service::scope_util`) makes it visible through `list_children`. The two read paths disagreed. Gate the count on the PARENT's Respect-reachability instead of each child's own barrier-clamped visibility: resolve which requested parents the caller can Respect-reach, then count all their direct children (the `parent_id` predicate pins the count to depth 1, so nothing below a barrier leaks — a parent reachable only via the carve-out, i.e. a self_managed tenant past its barrier, is absent from the Respect read and its subtree count stays 0). `Provisioning` excluded, `Deleted` included, as before. Regression tests exercise a real Respect-barriers `InTenantSubtree` scope (the previous coverage ran under `allow_all`, with barriers off, and could not catch this): one asserts a reachable parent's count includes its self_managed direct child, one asserts a past-barrier parent's subtree stays uncounted. Signed-off-by: Diffora --- .../src/infra/storage/repo_impl/reads.rs | 64 +++++++-- .../tests/list_children_integration.rs | 133 +++++++++++++++++- 2 files changed, 187 insertions(+), 10 deletions(-) diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs index 375671633..118c12e89 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs @@ -455,18 +455,29 @@ pub(super) async fn count_children( /// different from [`count_children`], which is an internal /// delete-saga guard (`allow_all`, always counts `Provisioning`): /// -/// * **Scope-filtered** — bounded by the caller's `scope` through -/// `SecureORM`, so direct children behind a self-managed barrier the -/// caller cannot penetrate collapse to `0`. This matches the -/// visibility rule the rest of the public read surface obeys. +/// * **Gated on the PARENT's Respect-reachability, then counts every +/// direct child** — including a `self_managed` direct child. This is +/// the direct-child carve-out (see `service::scope_util`): a +/// `self_managed` tenant sits behind a `barrier = 1` closure edge +/// even from its own parent, so clamping each *child* by the caller's +/// barrier-respecting scope would silently drop a Respect-reachable +/// parent's `self_managed` direct child from its `child_count` (while +/// `list_children` still shows it — the two must agree). Instead we +/// check which requested parents the caller can Respect-reach, then +/// count all their direct children. The `parent_id` predicate keeps +/// the count strictly depth-1, so nothing below a barrier leaks: a +/// parent the caller can only reach via the identity-level carve-out +/// (i.e. a `self_managed` tenant past the barrier) is NOT +/// Respect-reachable, so its subtree count collapses to `0`. /// * **`Provisioning` excluded** — those rows have no public /// representation anywhere on the SDK boundary. `Deleted` rows are /// *included*, mirroring that they stay reachable via /// `$filter=status eq 'deleted'`. /// -/// One grouped `COUNT ... GROUP BY parent_id` for the whole batch — no -/// N+1 across the page. Parents with no matching child are absent from -/// the returned map; callers default those to `0`. +/// Two indexed queries for the whole batch (parent reachability, then +/// one grouped `COUNT ... GROUP BY parent_id`) — no N+1 across the page. +/// Parents with no matching child are absent from the returned map; +/// callers default those to `0`. pub(super) async fn count_children_grouped( repo: &TenantRepoImpl, scope: &AccessScope, @@ -477,18 +488,53 @@ pub(super) async fn count_children_grouped( parent_id: Uuid, cnt: i64, } + #[derive(FromQueryResult)] + struct IdRow { + id: Uuid, + } if parent_ids.is_empty() { return Ok(HashMap::new()); } let connection = repo.db.conn()?; - let rows = tenants::Entity::find() + // Step 1 — of the requested parents, which can the caller + // Respect-reach? This is the access-control boundary: only children + // of parents the caller can genuinely see are counted. A parent + // reachable to the caller only via the identity-level direct-child + // carve-out (a self_managed tenant past its barrier) is absent from + // this Respect-scoped read, so its children are never counted. + let reachable: Vec = tenants::Entity::find() .secure() .scope_with(scope) + .filter(Condition::all().add(tenants::Column::Id.is_in(parent_ids.iter().copied()))) + .project_all(&connection, |q| { + q.select_only() + .column(tenants::Column::Id) + .into_model::() + }) + .await + .map_err(map_scope_err)? + .into_iter() + .map(|r| r.id) + .collect(); + if reachable.is_empty() { + return Ok(HashMap::new()); + } + + // Step 2 — count every direct child of each Respect-reachable + // parent, INCLUDING self_managed direct children. The parent gate + // above is the access boundary and the `parent_id` predicate pins + // this to depth 1, so the count runs unclamped (`allow_all`) rather + // than re-applying the caller's barrier=0 subtree clamp — which + // would otherwise drop a self_managed direct child of a reachable + // parent. `Provisioning` is excluded; `Deleted` is included. + let rows = tenants::Entity::find() + .secure() + .scope_with(&AccessScope::allow_all()) .filter( Condition::all() - .add(tenants::Column::ParentId.is_in(parent_ids.iter().copied())) + .add(tenants::Column::ParentId.is_in(reachable.iter().copied())) .add(tenants::Column::Status.ne(TenantStatus::Provisioning.as_smallint())), ) .project_all(&connection, |q| { diff --git a/gears/system/account-management/account-management/tests/list_children_integration.rs b/gears/system/account-management/account-management/tests/list_children_integration.rs index be82517af..9f598ea33 100644 --- a/gears/system/account-management/account-management/tests/list_children_integration.rs +++ b/gears/system/account-management/account-management/tests/list_children_integration.rs @@ -35,6 +35,9 @@ use time::{Duration, OffsetDateTime}; use toolkit_odata::ast::{CompareOperator, Expr, Value as OdataValue}; use toolkit_odata::filter::FilterField; use toolkit_odata::{CursorV1, ODataOrderBy, ODataQuery, OrderKey, SortDir}; +use toolkit_security::{ + AccessScope, InTenantSubtreeScopeFilter, ScopeConstraint, ScopeFilter, pep_properties, +}; use uuid::Uuid; use account_management::infra::storage::entity::tenants; @@ -341,6 +344,134 @@ async fn count_children_grouped_excludes_provisioning_includes_deleted() { ); } +/// Build a Respect-barriers `InTenantSubtree` scope rooted at `root`, +/// keyed on the tenant row's own id — the exact shape AM's PDP emits +/// for a tenant read (`tenants` maps `resource_col = "id"`). +fn respect_scope_rooted_at(root: Uuid) -> AccessScope { + AccessScope::single(ScopeConstraint::new(vec![ScopeFilter::InTenantSubtree( + InTenantSubtreeScopeFilter::new(pep_properties::RESOURCE_ID, root), + )])) +} + +/// Regression: a Respect-scoped caller counting a *reachable* parent's +/// direct children MUST include a `self_managed` direct child. +/// +/// A `self_managed` tenant's inbound closure edges carry `barrier = 1` +/// even from its own parent, so clamping each child by the caller's +/// barrier-respecting scope silently dropped the `self_managed` direct +/// child from `child_count` — while `list_children` still surfaced it +/// (the direct-child carve-out). The count must gate on the PARENT's +/// reachability, then count all direct children. The existing +/// `count_children_grouped_excludes_provisioning_includes_deleted` +/// test runs under `allow_all` (barriers off) and cannot catch this. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn count_children_grouped_respect_scope_counts_self_managed_direct_child() { + let h = setup_sqlite().await.expect("harness"); + let root = Uuid::from_u128(ROOT_ID); + seed_root(&h, root).await; + let type_a = Uuid::from_u128(0xAA); + + // P: an ordinary (non-barrier) direct child of root — Respect-reachable. + let p = Uuid::from_u128(0x401); + seed_tenant_at(&h, p, root, ACTIVE, false, type_a, ts_at(1)).await; + insert_closure(&h.provider, root, p, 0, ACTIVE) + .await + .expect("(root,P) barrier 0"); + insert_closure(&h.provider, p, p, 0, ACTIVE) + .await + .expect("(P,P) self-row"); + + // c1: ordinary active direct child of P. + let c1 = Uuid::from_u128(0x402); + seed_tenant_at(&h, c1, p, ACTIVE, false, type_a, ts_at(2)).await; + insert_closure(&h.provider, root, c1, 0, ACTIVE) + .await + .expect("(root,c1)"); + insert_closure(&h.provider, p, c1, 0, ACTIVE) + .await + .expect("(P,c1)"); + insert_closure(&h.provider, c1, c1, 0, ACTIVE) + .await + .expect("(c1,c1)"); + + // c2: self_managed direct child of P — inbound edges barrier = 1 + // (even from P), self-row barrier = 0. This is the row the old + // barrier clamp dropped from P's count. + let c2 = Uuid::from_u128(0x403); + seed_tenant_at(&h, c2, p, ACTIVE, true, type_a, ts_at(3)).await; + insert_closure(&h.provider, root, c2, 1, ACTIVE) + .await + .expect("(root,c2) barrier 1"); + insert_closure(&h.provider, p, c2, 1, ACTIVE) + .await + .expect("(P,c2) barrier 1"); + insert_closure(&h.provider, c2, c2, 0, ACTIVE) + .await + .expect("(c2,c2) self-row"); + + let scope = respect_scope_rooted_at(root); + let counts = h + .repo + .count_children_grouped(&scope, &[p]) + .await + .expect("grouped count"); + assert_eq!( + counts.get(&p).copied(), + Some(2), + "P is Respect-reachable, so its child_count must include the \ + self_managed direct child c2 (active c1 + self_managed c2 = 2), \ + not drop c2 to 1 under the barrier clamp" + ); +} + +/// The parent gate must not leak a subtree below a barrier: counting the +/// children of a `self_managed` tenant the caller can only reach past +/// the barrier collapses to `0` (absent from the map). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn count_children_grouped_respect_scope_hides_past_barrier_subtree() { + let h = setup_sqlite().await.expect("harness"); + let root = Uuid::from_u128(ROOT_ID); + seed_root(&h, root).await; + let type_a = Uuid::from_u128(0xAA); + + // s: self_managed tenant directly under root — reachable to the + // caller only via the identity-level carve-out, NOT under Respect. + let s = Uuid::from_u128(0x411); + seed_tenant_at(&h, s, root, ACTIVE, true, type_a, ts_at(1)).await; + insert_closure(&h.provider, root, s, 1, ACTIVE) + .await + .expect("(root,s) barrier 1"); + insert_closure(&h.provider, s, s, 0, ACTIVE) + .await + .expect("(s,s) self-row"); + + // gc: a child of s, strictly below s's barrier. + let gc = Uuid::from_u128(0x412); + seed_tenant_at(&h, gc, s, ACTIVE, false, type_a, ts_at(2)).await; + insert_closure(&h.provider, root, gc, 1, ACTIVE) + .await + .expect("(root,gc) barrier 1"); + insert_closure(&h.provider, s, gc, 0, ACTIVE) + .await + .expect("(s,gc)"); + insert_closure(&h.provider, gc, gc, 0, ACTIVE) + .await + .expect("(gc,gc)"); + + let scope = respect_scope_rooted_at(root); + let counts = h + .repo + .count_children_grouped(&scope, &[s]) + .await + .expect("grouped count"); + assert_eq!( + counts.get(&s).copied(), + None, + "s is not Respect-reachable (past its own barrier from root), so \ + its subtree must not be counted - child_count collapses to 0" + ); +} + /// Empty input short-circuits to an empty map (no query, no panic on an /// empty `IN ()` predicate). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -519,7 +650,7 @@ async fn list_children_orderby_status_cursor_roundtrip() { if let Some(c) = cursor.take() { assert!( seen_cursors.insert(c.clone()), - "cursor walk returned a repeated cursor — pagination is not advancing" + "cursor walk returned a repeated cursor - pagination is not advancing" ); q = q.with_cursor(CursorV1::decode(&c).expect("decode status cursor")); } else {