From ed75b2b20f02c4f1639b5b76adcbd790e7a1d470 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 13:32:03 +0200 Subject: [PATCH 1/8] 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, and cover both the default and an explicitly configured list with request-level tests. Signed-off-by: Diffora --- gears/system/api-gateway/src/config.rs | 10 ++ gears/system/api-gateway/src/cors.rs | 13 +++ gears/system/api-gateway/tests/cors_tests.rs | 116 +++++++++++++++++++ 3 files changed, 139 insertions(+) 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..d82b81747 100644 --- a/gears/system/api-gateway/src/cors.rs +++ b/gears/system/api-gateway/src/cors.rs @@ -47,6 +47,19 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { } } + if cors_cfg.exposed_headers.iter().any(|h| h == "*") { + layer = layer.expose_headers(tower_http::cors::Any); + } else { + let headers: Vec = cors_cfg + .exposed_headers + .into_iter() + .filter_map(|s| s.parse().ok()) + .collect(); + if !headers.is_empty() { + layer = layer.expose_headers(headers); + } + } + if cors_cfg.allow_credentials { layer = layer.allow_credentials(true); } diff --git a/gears/system/api-gateway/tests/cors_tests.rs b/gears/system/api-gateway/tests/cors_tests.rs index f582b88e3..9cc7701d4 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 } +// VHP-2193 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 0e1b0eae726e5e3b5df2e68d9910f5307f0596a4 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 13:40:16 +0200 Subject: [PATCH 2/8] 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': TenantODataMapper::is_orderable rejected the field because the cursor codec was hard-wired to the wire filter kind (String) while the extracted cursor value spoke 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. The AM mapper overrides Status -> I64 and extracts the ordinal as BigInt, 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). Integration tests replace the old rejection pin with a lifecycle-ordinal sort assertion and a full cursor walk across ordinal groups. Signed-off-by: Diffora --- .../account-management-sdk/src/tenant.rs | 8 +- .../src/infra/storage/repo_impl/reads.rs | 61 +++++++--- .../tests/list_children_integration.rs | 112 +++++++++++++++--- libs/toolkit-db/src/odata/sea_orm_filter.rs | 56 ++++++--- 4 files changed, 184 insertions(+), 53 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..f37301f8a 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 (VHP-2084); 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..8a17b6ce9 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 (VHP-2084). 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) } } @@ -168,7 +165,13 @@ impl ODataFieldMapping for TenantODataMapper { TenantInfoFilterField::Name => { sea_orm::Value::String(Some(Box::new(model.name.clone()))) } - TenantInfoFilterField::Status => sea_orm::Value::SmallInt(Some(model.status)), + // `BigInt` (not the raw `SmallInt`) so the token round-trips + // through the cursor codec under the `cursor_kind = I64` + // override below; SQL comparison against the SMALLINT column + // stays numeric either way. + TenantInfoFilterField::Status => { + sea_orm::Value::BigInt(Some(i64::from(model.status))) + } // `TenantType` is filter-only (not orderable), so it never reaches // the cursor path; map it identically to the raw-UUID field for // exhaustiveness. @@ -184,6 +187,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 (VHP-2084). 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 +662,15 @@ mod tenant_type_filter_tests { Field::TenantType )); } + + // VHP-2084 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..6113ffd65 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,115 @@ 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. +/// VHP-2084: `$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 (VHP-2084)"); + + // 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" + ); +} + +/// VHP-2084 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), + ]; + + 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() { + 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/libs/toolkit-db/src/odata/sea_orm_filter.rs b/libs/toolkit-db/src/odata/sea_orm_filter.rs index 3615e3874..6a3b655ac 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,22 @@ 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 lifecycle enum exposed as a + /// string on the API surface but ordered by its storage ordinal: + /// return [`FieldKind::I64`] and extract `sea_orm::Value::BigInt` + /// so `encode_cursor_value` / `parse_cursor_value` round-trip 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, @@ -665,14 +682,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 +852,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 +910,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); } From 81958df231d3d0aa5007f5e8c4463917f8711a78 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 15:24:04 +0200 Subject: [PATCH 3/8] 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 PRD '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), detail } and PasswordPolicy { detail }; Rejected stays as the fallback for unattributable rejections, so legacy plugins keep today's behavior unchanged. New field/reason vocabulary: PASSWORD_FIELD, PASSWORD_POLICY. AM: map DuplicateUser to the new DomainError::UserAlreadyExists -> HTTP 409 already_exists on the user resource type, with the stable colliding-field token ('username' / 'email') as resource_name (never the caller-supplied value); 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 in vhp-core 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 | 58 +++++++++++++-- .../account-management-sdk/src/lib.rs | 4 +- .../account-management/src/domain/error.rs | 25 +++++++ .../src/domain/error_tests.rs | 4 ++ .../src/domain/idp/idp_tests.rs | 70 +++++++++++++++++++ .../account-management/src/domain/idp/mod.rs | 39 +++++++++++ .../src/infra/sdk_error_mapping.rs | 19 +++++ .../src/infra/sdk_error_mapping_tests.rs | 54 ++++++++++++++ .../src/infra/storage/repo_impl/reads.rs | 4 +- 10 files changed, 277 insertions(+), 10 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..8b8515290 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 (VHP-2158). 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..ef4638503 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,55 @@ 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 (VHP-2158: 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 + /// (VHP-2158); 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, +} + +impl IdpUserDuplicateField { + /// Stable wire/field token for canonical `field_violations.field` + /// and metric labels. + #[must_use] + pub const fn as_field_token(self) -> &'static str { + match self { + Self::Username => "username", + Self::Email => "email", + } + } } impl IdpUserOperationFailure { @@ -731,6 +775,8 @@ impl IdpUserOperationFailure { Self::Unavailable { .. } => "unavailable", Self::UnsupportedOperation { .. } => "unsupported_operation", Self::Rejected { .. } => "rejected", + Self::DuplicateUser { .. } => "duplicate_user", + Self::PasswordPolicy { .. } => "password_policy", } } @@ -743,7 +789,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..a99f1f19e 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,31 @@ pub enum DomainError { #[error("already exists: {detail}")] AlreadyExists { detail: String }, + /// The `IdP` reported a uniqueness collision on a user attribute + /// during a user operation (VHP-2158: duplicate username in the + /// realm, duplicate email realm-wide). Distinct from + /// [`Self::AlreadyExists`] (tenant-flavoured, DB-classifier + /// produced) so the canonical envelope rides the `user` resource + /// type. `resource` carries the stable colliding-field token + /// (`"username"` / `"email"`), NOT the caller-supplied value — + /// the redaction posture of the `IdP` boundary keeps provider + /// echoes out of public envelopes. Surfaces as HTTP 409 + /// `already_exists`. + #[error("user already exists: {detail}")] + UserAlreadyExists { detail: String, resource: String }, + + /// The `IdP` rejected the supplied password against its configured + /// password policy (VHP-2158). 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..829a303fd 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,73 @@ fn provision_ambiguous_chains_redacted_cause_without_leaking_raw_detail() { "DomainError::Internal::cause MUST be reachable via Error::source" ); } + +// --------------------------------------------------------------------------- +// VHP-2158: 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()); + let DomainError::UserAlreadyExists { detail, resource } = err else { + panic!("expected UserAlreadyExists, got a different variant"); + }; + assert_eq!(resource, "username"); + // Public detail is a curated fixed string carrying the field + // token; the raw provider text MUST NOT leak. + assert!(detail.contains("username"), "field token missing: {detail}"); + assert!( + !detail.contains("User exists"), + "raw provider string leaked into public detail: {detail}" + ); +} + +#[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()); + let DomainError::UserAlreadyExists { resource, .. } = err else { + panic!("expected UserAlreadyExists, got a different variant"); + }; + assert_eq!(resource, "email"); +} + +#[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..93ae91228 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,45 @@ 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 (VHP-2158): 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 a curated fixed string. + Self::DuplicateUser { field, detail } => { + let (digest, len) = redact_provider_detail(&detail); + let field_token = field.as_field_token(); + tracing::warn!( + target: "am.idp", + tenant_id = %tenant_id, + field = field_token, + provider_detail_digest = digest, + provider_detail_len = len, + "IdP user operation DuplicateUser; surfacing already_exists, raw detail redacted" + ); + DomainError::UserAlreadyExists { + detail: format!("a user with this {field_token} already exists"), + resource: field_token.to_owned(), + } + } + // Classified password-policy reject (VHP-2158): 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..9fcc1db18 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 (VHP-2158): 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,14 @@ impl From for CanonicalError { DomainError::AlreadyExists { detail } => TenantResource::already_exists(detail) .with_resource("tenant") .create(), + // IdP-reported user uniqueness collision (VHP-2158): + // `resource` is the stable colliding-field token + // ("username" / "email"), never the caller-supplied value. + DomainError::UserAlreadyExists { detail, resource } => { + UserResource::already_exists(detail) + .with_resource(resource) + .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..9b907ebf3 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,60 @@ fn already_exists_maps_to_409() { ); } +/// VHP-2158: 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` — never the +/// caller-supplied value and never the raw provider text. +#[test] +fn user_already_exists_maps_to_409_with_user_resource() { + let canonical = round_trip(DomainError::UserAlreadyExists { + detail: "a user with this username already exists".to_owned(), + resource: "username".to_owned(), + }); + assert_eq!(canonical.status_code(), 409); + assert_eq!(canonical.resource_name(), Some("username")); + 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" + ); +} + +/// VHP-2158: 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) // --------------------------------------------------------------------------- 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 8a17b6ce9..a50052d4b 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 @@ -169,9 +169,7 @@ impl ODataFieldMapping for TenantODataMapper { // through the cursor codec under the `cursor_kind = I64` // override below; SQL comparison against the SMALLINT column // stays numeric either way. - TenantInfoFilterField::Status => { - sea_orm::Value::BigInt(Some(i64::from(model.status))) - } + TenantInfoFilterField::Status => sea_orm::Value::BigInt(Some(i64::from(model.status))), // `TenantType` is filter-only (not orderable), so it never reaches // the cursor path; map it identically to the raw-UUID field for // exhaustiveness. From 722dd44c90844972e5fd419b063cd860e3eb877d Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 15:45:52 +0200 Subject: [PATCH 4/8] fix(account-management): cancel pending conversion in the tenant soft-delete TX DELETE /tenants/{id} on a tenant with an open child-conversion request returned 204 and tombstoned the tenant, but the conversion row stayed 'pending' with tenant_id pointing at the tombstone - breaking the invariant that every pending conversion references a live tenant, and leaving LIST /child-conversions dereferencing a dead FK. schedule_deletion's SERIALIZABLE TX now terminal-resolves the request right after the status flip + closure rewrite: a guarded UPDATE (status = Pending AND deleted_at IS NULL) moves it to Cancelled with cancelled_by = the deleting actor, resolved_at = the deletion timestamp, and a fixed 'subject tenant deleted' comment. The helper lives with the conversion repo (cancel_pending_on_tenant_delete_tx) so ownership of the conversion_requests transition shape stays in one place; the tenant TX just invokes it. The Pending fence keeps idempotent delete retries from re-stamping an already-resolved row, and the already-Deleted short-circuit returns before the cascade anyway. Only rows keyed by the deleted tenant can exist here: a parent with non-deleted children is rejected by TenantHasChildren before the TX. schedule_deletion gains a deleted_by actor parameter (trait, impl, in-memory fake, all call sites); the service passes ctx.subject_id(). An am.events line (conversion_auto_cancelled_on_tenant_delete) fires when the cascade touches a row. Integration tests: cascade resolves the row in the same TX (row is resolved, not removed; no pending row survives for the tombstone), an idempotent retry with a different actor/timestamp preserves the first resolution verbatim, and a sibling tenant's pending conversion is untouched. 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 | 25 +++ .../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, 272 insertions(+), 9 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..c348948ca 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 + /// (VHP-1729: 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..4f2a2ab13 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 (VHP-1729) 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..b0b9dccea 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 (VHP-1729). 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..e637654b4 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 { @@ -595,6 +596,30 @@ 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 + // VHP-1729: 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 idempotent + // delete retries from re-stamping resolved rows (the + // already-Deleted short-circuit above returns before + // reaching here anyway). + let cancelled = + super::conversion::cancel_pending_on_tenant_delete_tx(tx, id, deleted_by, now) + .await?; + 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" + ); + } + let fresh = tenants::Entity::find() .secure() .scope_with(&scope) 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..68b6f0197 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); } + +// --------------------------------------------------------------------------- +// VHP-1729: 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 1eefd22bfaf399833c543675091ac1dbab46d8a5 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 16:34:16 +0200 Subject: [PATCH 5/8] tech(account-management): post-review remediation for the gateway/listing/conversion fixes Fixes from the branch code review, batched: api-gateway: - 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. Shared parse_list helper replaces the 4th copy of the parse pattern. account-management SDK+AM: - IdpUserDuplicateField gains UsernameOrEmail: Keycloak'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), which must not be misattributed to username. - DomainError::UserAlreadyExists now carries the typed field; the canonical boundary derives both resource_name and the curated detail from it, collapsing three copies of the wording into one. account-management: - The conversion auto-cancel am.events line is emitted AFTER the SERIALIZABLE TX commits (count rides the closure return): logging inside the closure double-counted on a 40001 retry and emitted a phantom event when the TX ultimately rolled back. - The already-Deleted idempotent short-circuit now also runs the (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. - OpenAPI account-management-v1.yaml: $orderby=status on the tenant children listing is now documented as supported with lifecycle-ordinal semantics (the old text still declared it rejected with 400). toolkit-db + resource-group (follow-through): - encode_cursor_value accepts SmallInt/Int under FieldKind::I64, so mappers exposing narrow integer columns need not hand-widen to BigInt (AM's status extract reverts to the natural SmallInt). - resource-group's three type fields (Group/Hierarchy/Membership) had the exact wire-String/storage-SmallInt mismatch the cursor_kind seam governs, orderable and un-overridden - $orderby=type broke with InvalidCursor on any page overflow; they now declare cursor_kind=I64. - Codec unit tests pin the narrow-integer round-trip and the loud failure for a genuine wire/storage mismatch. Signed-off-by: Diffora --- .../account-management-sdk/src/idp_user.rs | 25 ++- .../account-management/src/domain/error.rs | 20 ++- .../src/domain/idp/idp_tests.rs | 54 ++++-- .../account-management/src/domain/idp/mod.rs | 11 +- .../src/infra/sdk_error_mapping.rs | 19 +- .../src/infra/sdk_error_mapping_tests.rs | 49 ++++-- .../src/infra/storage/repo_impl/reads.rs | 6 +- .../src/infra/storage/repo_impl/updates.rs | 56 ++++-- .../docs/account-management-v1.yaml | 20 ++- gears/system/api-gateway/src/cors.rs | 162 +++++++++++++++--- gears/system/api-gateway/src/gear.rs | 4 + .../src/infra/storage/odata_mapper.rs | 29 ++++ libs/toolkit-db/src/odata/sea_orm_filter.rs | 51 +++++- 13 files changed, 397 insertions(+), 109 deletions(-) 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 ef4638503..ab36c29a9 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 @@ -748,16 +748,39 @@ pub enum IdpUserDuplicateField { 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` - /// and metric labels. + /// / `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", } } } 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 a99f1f19e..811b6f01f 100644 --- a/gears/system/account-management/account-management/src/domain/error.rs +++ b/gears/system/account-management/account-management/src/domain/error.rs @@ -153,16 +153,20 @@ pub enum DomainError { /// The `IdP` reported a uniqueness collision on a user attribute /// during a user operation (VHP-2158: duplicate username in the - /// realm, duplicate email realm-wide). Distinct from + /// 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. `resource` carries the stable colliding-field token - /// (`"username"` / `"email"`), NOT the caller-supplied value — - /// the redaction posture of the `IdP` boundary keeps provider - /// echoes out of public envelopes. Surfaces as HTTP 409 - /// `already_exists`. - #[error("user already exists: {detail}")] - UserAlreadyExists { detail: String, resource: String }, + /// 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 (VHP-2158). Distinct from [`Self::Validation`] 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 829a303fd..4f186e1ca 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 @@ -120,16 +120,17 @@ fn user_duplicate_username_maps_to_user_already_exists() { detail: "User exists with same username".into(), } .into_domain_error(fixture_tenant_id()); - let DomainError::UserAlreadyExists { detail, resource } = err else { - panic!("expected UserAlreadyExists, got a different variant"); - }; - assert_eq!(resource, "username"); - // Public detail is a curated fixed string carrying the field - // token; the raw provider text MUST NOT leak. - assert!(detail.contains("username"), "field token missing: {detail}"); + // 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!( - !detail.contains("User exists"), - "raw provider string leaked into public detail: {detail}" + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::Username + } + ), + "expected UserAlreadyExists(Username), got {err:?}" ); } @@ -140,10 +141,37 @@ fn user_duplicate_email_maps_to_user_already_exists() { detail: "User exists with same email".into(), } .into_domain_error(fixture_tenant_id()); - let DomainError::UserAlreadyExists { resource, .. } = err else { - panic!("expected UserAlreadyExists, got a different variant"); - }; - assert_eq!(resource, "email"); + 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] 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 93ae91228..bbd2b425c 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 @@ -365,22 +365,19 @@ impl UserOperationFailureExt for IdpUserOperationFailure { // 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 a curated fixed string. + // 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); - let field_token = field.as_field_token(); tracing::warn!( target: "am.idp", tenant_id = %tenant_id, - field = field_token, + 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 { - detail: format!("a user with this {field_token} already exists"), - resource: field_token.to_owned(), - } + DomainError::UserAlreadyExists { field } } // Classified password-policy reject (VHP-2158): surface // the structured `password` / `PASSWORD_POLICY` violation 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 9fcc1db18..df0142d2d 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 @@ -195,14 +195,17 @@ impl From for CanonicalError { DomainError::AlreadyExists { detail } => TenantResource::already_exists(detail) .with_resource("tenant") .create(), - // IdP-reported user uniqueness collision (VHP-2158): - // `resource` is the stable colliding-field token - // ("username" / "email"), never the caller-supplied value. - DomainError::UserAlreadyExists { detail, resource } => { - UserResource::already_exists(detail) - .with_resource(resource) - .create() - } + // IdP-reported user uniqueness collision (VHP-2158): 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 9b907ebf3..d7c7dcbf8 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 @@ -269,24 +269,41 @@ fn already_exists_maps_to_409() { /// VHP-2158: 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` — never the -/// caller-supplied value and never the raw provider text. +/// 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() { - let canonical = round_trip(DomainError::UserAlreadyExists { - detail: "a user with this username already exists".to_owned(), - resource: "username".to_owned(), - }); - assert_eq!(canonical.status_code(), 409); - assert_eq!(canonical.resource_name(), Some("username")); - 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" - ); + 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); + } } /// VHP-2158: an `IdP` password-policy reject carries the structured 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 a50052d4b..0b856d8a1 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 @@ -165,11 +165,7 @@ impl ODataFieldMapping for TenantODataMapper { TenantInfoFilterField::Name => { sea_orm::Value::String(Some(Box::new(model.name.clone()))) } - // `BigInt` (not the raw `SmallInt`) so the token round-trips - // through the cursor codec under the `cursor_kind = I64` - // override below; SQL comparison against the SMALLINT column - // stays numeric either way. - TenantInfoFilterField::Status => sea_orm::Value::BigInt(Some(i64::from(model.status))), + TenantInfoFilterField::Status => sea_orm::Value::SmallInt(Some(model.status)), // `TenantType` is filter-only (not orderable), so it never reaches // the cursor path; map it identically to the raw-UUID field for // exhaustiveness. 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 e637654b4..5f2177615 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 @@ -487,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); + // VHP-1729 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` @@ -601,24 +615,16 @@ pub(super) async fn schedule_deletion( // (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 idempotent - // delete retries from re-stamping resolved rows (the - // already-Deleted short-circuit above returns before - // reaching here anyway). + // 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?; - 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" - ); - } let fresh = tenants::Entity::find() .secure() @@ -631,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/docs/account-management-v1.yaml b/gears/system/account-management/docs/account-management-v1.yaml index 3ba313445..4be275dd0 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. 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. schema: type: string ODataOrderBy: diff --git a/gears/system/api-gateway/src/cors.rs b/gears/system/api-gateway/src/cors.rs index d82b81747..0f7eaab05 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 (VHP-2193). +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,11 +97,8 @@ 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); } @@ -50,11 +107,8 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.exposed_headers.iter().any(|h| h == "*") { layer = layer.expose_headers(tower_http::cors::Any); } else { - let headers: Vec = cors_cfg - .exposed_headers - .into_iter() - .filter_map(|s| s.parse().ok()) - .collect(); + let headers: Vec = + parse_list("exposed_headers", &cors_cfg.exposed_headers); if !headers.is_empty() { layer = layer.expose_headers(headers); } @@ -70,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/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..3116b1405 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`, VHP-2084). + 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 6a3b655ac..c1e303f74 100644 --- a/libs/toolkit-db/src/odata/sea_orm_filter.rs +++ b/libs/toolkit-db/src/odata/sea_orm_filter.rs @@ -152,11 +152,12 @@ pub trait ODataFieldMapping: FieldToColumn { /// 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 lifecycle enum exposed as a - /// string on the API surface but ordered by its storage ordinal: - /// return [`FieldKind::I64`] and extract `sea_orm::Value::BigInt` - /// so `encode_cursor_value` / `parse_cursor_value` round-trip the - /// ordinal instead of failing on the wire/storage mismatch. + /// 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() } @@ -409,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()), @@ -928,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 f1eb10caf148b3b26a9c8a78120dfbc629647224 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 17:05:10 +0200 Subject: [PATCH 6/8] fix(account-management): platform-scope the user-groups cascade system actor The retention hard-delete reaper could never purge a tenant: its RG cascade hook ran under a system-actor context scoped to the very tenant being hard-deleted. The authz resolver's system-actor grant roots a tenant-subtree scope at the subject's home tenant - a Deleted root is clamped out of scope materialization, the subtree resolves to zero tenants, and every cascade call fails closed, so each due tenant was deferred forever (observed on stage1: processed=64 cleaned=0 deferred=64 on every tick). Make for_user_groups_cascade platform-scoped (nil tenant -> Global grant at the resolver): honest for this flow, which addresses explicit group ids of a tenant being erased, not a live tenant's data. The deleted tenant id stays on the am.system_actor audit line. The resolver-side halves (empty-token-scopes bypass for the unforgeable system actor + Global on nil home tenant) land in vp-core's vp-authz-resolver-plugin. 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..3d76f9522 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 (VHP-2376), 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 { ); } } + + // VHP-2376: 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 8cb09bf9e673d7c782484d478a7984ad8b7f0c4a Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 18:14:27 +0200 Subject: [PATCH 7/8] cleanup: make change-rationale comments self-contained Fold the reasoning previously carried by internal tracker references directly into the comments so they stand on their own. Signed-off-by: Diffora --- .../account-management/account-management-sdk/src/field.rs | 2 +- .../account-management-sdk/src/idp_user.rs | 4 ++-- .../account-management/account-management-sdk/src/tenant.rs | 2 +- .../account-management/src/domain/error.rs | 4 ++-- .../account-management/src/domain/idp/idp_tests.rs | 2 +- .../account-management/src/domain/idp/mod.rs | 4 ++-- .../account-management/src/domain/system_actor.rs | 4 ++-- .../account-management/src/domain/tenant/repo.rs | 2 +- .../src/domain/tenant/test_support/repo.rs | 2 +- .../account-management/src/infra/sdk_error_mapping.rs | 4 ++-- .../account-management/src/infra/sdk_error_mapping_tests.rs | 4 ++-- .../src/infra/storage/repo_impl/conversion.rs | 2 +- .../account-management/src/infra/storage/repo_impl/reads.rs | 6 +++--- .../src/infra/storage/repo_impl/updates.rs | 4 ++-- .../account-management/tests/conversion_integration.rs | 2 +- .../account-management/tests/list_children_integration.rs | 6 +++--- gears/system/api-gateway/src/cors.rs | 2 +- gears/system/api-gateway/tests/cors_tests.rs | 2 +- .../resource-group/src/infra/storage/odata_mapper.rs | 2 +- 19 files changed, 30 insertions(+), 30 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 8b8515290..b9c1dfe1b 100644 --- a/gears/system/account-management/account-management-sdk/src/field.rs +++ b/gears/system/account-management/account-management-sdk/src/field.rs @@ -46,7 +46,7 @@ pub const ROOT_TENANT_CANNOT_CHANGE_STATUS: &str = "ROOT_TENANT_CANNOT_CHANGE_ST pub const IDP_INVALID_INPUT: &str = "IDP_INVALID_INPUT"; /// The `IdP` rejected the supplied password against its configured -/// password policy (VHP-2158). Carried on the [`PASSWORD_FIELD`] +/// 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"; 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 ab36c29a9..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 @@ -720,7 +720,7 @@ pub enum IdpUserOperationFailure { /// the fallback for genuinely unattributable rejections. Rejected { detail: String }, /// Provider rejected the operation because a uniqueness invariant - /// on `field` is already taken (VHP-2158: duplicate username in + /// 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 @@ -734,7 +734,7 @@ pub enum IdpUserOperationFailure { /// to the canonical validation envelope with the structured /// `password` / `PASSWORD_POLICY` field-violation tokens so /// clients can attribute the failure to the password field - /// (VHP-2158); the raw policy text stays provider-side. + ///; the raw policy text stays provider-side. PasswordPolicy { detail: String }, } 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 f37301f8a..17d602df0 100644 --- a/gears/system/account-management/account-management-sdk/src/tenant.rs +++ b/gears/system/account-management/account-management-sdk/src/tenant.rs @@ -243,7 +243,7 @@ pub struct TenantInfoQuery { /// 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 (VHP-2084); filter comparisons stay + /// their Status column with it; filter comparisons stay /// membership-only (`eq` / `ne` / `in`). #[odata(filter(kind = "String"))] pub status: String, 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 811b6f01f..a12d57364 100644 --- a/gears/system/account-management/account-management/src/domain/error.rs +++ b/gears/system/account-management/account-management/src/domain/error.rs @@ -152,7 +152,7 @@ pub enum DomainError { AlreadyExists { detail: String }, /// The `IdP` reported a uniqueness collision on a user attribute - /// during a user operation (VHP-2158: duplicate username in the + /// 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 @@ -169,7 +169,7 @@ pub enum DomainError { }, /// The `IdP` rejected the supplied password against its configured - /// password policy (VHP-2158). Distinct from [`Self::Validation`] + /// 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 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 4f186e1ca..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 @@ -110,7 +110,7 @@ fn provision_ambiguous_chains_redacted_cause_without_leaking_raw_detail() { } // --------------------------------------------------------------------------- -// VHP-2158: classified user-operation rejections. +// Classified user-operation rejections. // --------------------------------------------------------------------------- #[test] 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 bbd2b425c..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,7 +361,7 @@ 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 (VHP-2158): surface HTTP + // 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 @@ -379,7 +379,7 @@ impl UserOperationFailureExt for IdpUserOperationFailure { ); DomainError::UserAlreadyExists { field } } - // Classified password-policy reject (VHP-2158): surface + // 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. 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 3d76f9522..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 @@ -154,7 +154,7 @@ pub(crate) fn for_retention_sweep(tenant_id: Uuid) -> SecurityContext { /// `tenant_id` is the tenant being deleted; it rides the audit log /// line only. /// -/// Platform-scoped (VHP-2376), NOT scoped to `tenant_id`: the subject +/// 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 @@ -223,7 +223,7 @@ mod tests { } } - // VHP-2376: the cascade hook's subject tenant is a Deleted row + // 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). 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 c348948ca..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 @@ -409,7 +409,7 @@ pub trait TenantRepo: Send + Sync { /// `tenant_closure.descendant_status`, and terminal-resolve /// (cancel) any still-pending mode-conversion request whose /// subject is this tenant — all in the same transaction - /// (VHP-1729: a committed soft-delete must never leave a + /// (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`. 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 4f2a2ab13..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 @@ -1324,7 +1324,7 @@ impl TenantRepo for FakeTenantRepo { } // `_deleted_by` is unused: the in-memory fake models the `tenants` - // table only — the conversion auto-cancel cascade (VHP-1729) is a + // table only — the conversion auto-cancel cascade is a // real-repo TX concern covered by the sqlite integration tests. async fn schedule_deletion( &self, 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 df0142d2d..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,7 +141,7 @@ impl From for CanonicalError { account_management_sdk::field::IDP_INVALID_INPUT, ) .create(), - // IdP password-policy reject (VHP-2158): structured + // 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`. @@ -195,7 +195,7 @@ impl From for CanonicalError { DomainError::AlreadyExists { detail } => TenantResource::already_exists(detail) .with_resource("tenant") .create(), - // IdP-reported user uniqueness collision (VHP-2158): both + // 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 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 d7c7dcbf8..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,7 +267,7 @@ fn already_exists_maps_to_409() { ); } -/// VHP-2158: an `IdP`-reported user uniqueness collision surfaces as +/// 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 @@ -306,7 +306,7 @@ fn user_already_exists_maps_to_409_with_user_resource() { } } -/// VHP-2158: an `IdP` password-policy reject carries the structured +/// 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`. 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 b0b9dccea..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 @@ -495,7 +495,7 @@ async fn find_pending_for_tenant( /// 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 (VHP-1729). Called by the tenant repo'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 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 0b856d8a1..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 @@ -140,7 +140,7 @@ impl FieldToColumn for TenantODataMapper { /// 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 (VHP-2084). Cursor pages are safe: both + /// 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. @@ -184,7 +184,7 @@ 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 (VHP-2084). Every + /// 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 { @@ -657,7 +657,7 @@ mod tenant_type_filter_tests { )); } - // VHP-2084 regression guard: the Tenants-page Status column sorts via + // 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. 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 5f2177615..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 @@ -487,7 +487,7 @@ pub(super) async fn schedule_deletion( // un-contended retry path; this guard is the // authoritative correctness boundary. if existing.status == TenantStatus::Deleted.as_smallint() { - // VHP-1729 heal-on-retry: tenants soft-deleted BEFORE + // 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 @@ -610,7 +610,7 @@ 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 - // VHP-1729: a pending mode-conversion request whose + // 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 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 68b6f0197..fd35b87fc 100644 --- a/gears/system/account-management/account-management/tests/conversion_integration.rs +++ b/gears/system/account-management/account-management/tests/conversion_integration.rs @@ -1116,7 +1116,7 @@ async fn approve_recomputes_barrier_for_closure_rows_referencing_soft_deleted_de } // --------------------------------------------------------------------------- -// VHP-1729: tenant soft-delete auto-cancels the pending conversion in-TX. +// Tenant soft-delete auto-cancels the pending conversion in-TX. // --------------------------------------------------------------------------- /// Seed a pending conversion for `child` and return its id. 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 6113ffd65..3b8b72b06 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,7 +426,7 @@ async fn list_children_rejects_ordered_comparison_on_status() { ); } -/// VHP-2084: `$orderby=status` sorts children by the lifecycle ordinal +/// `$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 @@ -453,7 +453,7 @@ async fn list_children_orderby_status_sorts_by_lifecycle_ordinal() { .repo .list_children(&allow_all(), root, &query) .await - .expect("$orderby=status must be accepted (VHP-2084)"); + .expect("$orderby=status must be accepted"); // Effective order is (status ASC, id ASC): active first, then the // two suspended rows tie-broken by id. @@ -464,7 +464,7 @@ async fn list_children_orderby_status_sorts_by_lifecycle_ordinal() { ); } -/// VHP-2084 cursor guard: ordering by `status` must survive the cursor +/// 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 diff --git a/gears/system/api-gateway/src/cors.rs b/gears/system/api-gateway/src/cors.rs index 0f7eaab05..73d555c2d 100644 --- a/gears/system/api-gateway/src/cors.rs +++ b/gears/system/api-gateway/src/cors.rs @@ -46,7 +46,7 @@ pub fn validate_cors_config(cfg: &ApiGatewayConfig) -> Result<(), String> { /// 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 (VHP-2193). +/// and breaks every ETag-guarded browser write. fn parse_list(list_name: &'static str, items: &[String]) -> Vec { items .iter() diff --git a/gears/system/api-gateway/tests/cors_tests.rs b/gears/system/api-gateway/tests/cors_tests.rs index 9cc7701d4..59adb6d06 100644 --- a/gears/system/api-gateway/tests/cors_tests.rs +++ b/gears/system/api-gateway/tests/cors_tests.rs @@ -205,7 +205,7 @@ async fn test_cors_disabled() { // Verify router builds without CORS layer } -// VHP-2193 regression guard: the RBAC/AM write contract is ETag-guarded +// 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 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 3116b1405..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 @@ -83,7 +83,7 @@ impl ODataFieldMapping for GroupODataMapper { /// `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`, VHP-2084). + /// overflow (same wire-vs-storage seam as AM's `status`). fn cursor_kind(field: GroupFilterField) -> FieldKind { match field { GroupFilterField::Type => FieldKind::I64, From 222d78dd71b99b6501ab570957fadfc246338025 Mon Sep 17 00:00:00 2001 From: Diffora Date: Fri, 17 Jul 2026 19:22:56 +0200 Subject: [PATCH 8/8] fix(resource-group): restore rg-namespace group resource type for PEP evaluation The gts-rust v0.11.0 migration mechanically replaced the group gate's resource-type literal with 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. 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 ──────────────────────────────────────────────────