diff --git a/gears/system/account-management/account-management-sdk/src/field.rs b/gears/system/account-management/account-management-sdk/src/field.rs index 01801c72d..b9c1dfe1b 100644 --- a/gears/system/account-management/account-management-sdk/src/field.rs +++ b/gears/system/account-management/account-management-sdk/src/field.rs @@ -45,6 +45,12 @@ pub const ROOT_TENANT_CANNOT_CHANGE_STATUS: &str = "ROOT_TENANT_CANNOT_CHANGE_ST /// `provisioning_metadata.realm_name`) — see [`PROVISIONING_METADATA_FIELD`]. pub const IDP_INVALID_INPUT: &str = "IDP_INVALID_INPUT"; +/// The `IdP` rejected the supplied password against its configured +/// password policy. Carried on the [`PASSWORD_FIELD`] +/// field-violation so clients can attribute the failure to the +/// password input; the raw policy text stays provider-side. +pub const PASSWORD_POLICY: &str = "PASSWORD_POLICY"; + // --------------------------------------------------------------------------- // `field_violations[].field` attribution keys. // @@ -69,6 +75,10 @@ pub const METADATA_FIELD: &str = "metadata"; /// [`ROOT_TENANT_CANNOT_CHANGE_STATUS`]). pub const TENANT_ID_FIELD: &str = "tenant_id"; +/// `password` field for `IdP` password-policy rejects (carries +/// [`PASSWORD_POLICY`]). +pub const PASSWORD_FIELD: &str = "password"; + /// Shared fallback field for [`IDP_INVALID_INPUT`] when the `IdP` /// plugin cannot localise the violation to a specific sub-key — the /// public surface every `IdP` plugin shares. diff --git a/gears/system/account-management/account-management-sdk/src/idp_user.rs b/gears/system/account-management/account-management-sdk/src/idp_user.rs index 73a508fab..8fad03aeb 100644 --- a/gears/system/account-management/account-management-sdk/src/idp_user.rs +++ b/gears/system/account-management/account-management-sdk/src/idp_user.rs @@ -711,11 +711,78 @@ pub enum IdpUserOperationFailure { /// `idp_unsupported_operation`. Providers MUST NOT silently no-op /// a mutating call. UnsupportedOperation { detail: String }, - /// Provider returned a payload-rejection category (e.g. duplicate - /// username, validation failure on email format). AM maps this to - /// the canonical validation envelope; the catalog refinement is - /// owned by `feature-errors-observability`. + /// Provider returned a payload-rejection the plugin could NOT + /// classify further. AM maps this to the canonical validation + /// envelope with the generic `request`/`VALIDATION` tokens. + /// Providers SHOULD emit the classified variants below + /// ([`Self::DuplicateUser`], [`Self::PasswordPolicy`]) whenever + /// the vendor response identifies the cause — this catch-all is + /// the fallback for genuinely unattributable rejections. Rejected { detail: String }, + /// Provider rejected the operation because a uniqueness invariant + /// on `field` is already taken (duplicate username in + /// the realm, duplicate email realm-wide). AM maps this to the + /// canonical `already_exists` envelope (HTTP 409) — the create + /// contract already declares a Conflict response and documents + /// idempotency keyed by `(tenant_id, username)`. + DuplicateUser { + field: IdpUserDuplicateField, + detail: String, + }, + /// Provider rejected the supplied password against its configured + /// password policy (length / complexity / history). AM maps this + /// to the canonical validation envelope with the structured + /// `password` / `PASSWORD_POLICY` field-violation tokens so + /// clients can attribute the failure to the password field + ///; the raw policy text stays provider-side. + PasswordPolicy { detail: String }, +} + +/// Which unique user attribute an [`IdpUserOperationFailure::DuplicateUser`] +/// rejection collided on. `#[non_exhaustive]`: providers may learn to +/// classify further unique attributes without a breaking SDK release. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum IdpUserDuplicateField { + /// Realm-scoped username collision. + Username, + /// Realm-wide email collision. + Email, + /// The provider reported a uniqueness conflict without an + /// attributable single field. Keycloak's `ModelDuplicateException` + /// path emits the combined constant "User exists with same + /// username or email" — on KC 26 that is the ONLY 409 shape + /// `createUser` produces directly — and a conflict body the plugin + /// cannot parse lands here too: on the user-create endpoint a 409 + /// has no other cause than a uniqueness collision, so the status + /// alone justifies the conflict classification even when the field + /// stays unknown. + UsernameOrEmail, +} + +impl IdpUserDuplicateField { + /// Stable wire/field token for canonical `field_violations.field` + /// / `resource_name` and metric labels. + #[must_use] + pub const fn as_field_token(self) -> &'static str { + match self { + Self::Username => "username", + Self::Email => "email", + Self::UsernameOrEmail => "username_or_email", + } + } + + /// Human phrase for curated public error details ("a user with + /// this {phrase} already exists"). Centralised here so AM's + /// canonical boundary and its tests cannot drift. + #[must_use] + pub const fn as_human_phrase(self) -> &'static str { + match self { + Self::Username => "username", + Self::Email => "email", + Self::UsernameOrEmail => "username or email", + } + } } impl IdpUserOperationFailure { @@ -731,6 +798,8 @@ impl IdpUserOperationFailure { Self::Unavailable { .. } => "unavailable", Self::UnsupportedOperation { .. } => "unsupported_operation", Self::Rejected { .. } => "rejected", + Self::DuplicateUser { .. } => "duplicate_user", + Self::PasswordPolicy { .. } => "password_policy", } } @@ -743,7 +812,9 @@ impl IdpUserOperationFailure { match self { Self::Unavailable { detail } | Self::UnsupportedOperation { detail } - | Self::Rejected { detail } => detail, + | Self::Rejected { detail } + | Self::DuplicateUser { detail, .. } + | Self::PasswordPolicy { detail } => detail, } } } diff --git a/gears/system/account-management/account-management-sdk/src/lib.rs b/gears/system/account-management/account-management-sdk/src/lib.rs index 9934196d4..ead7503bd 100644 --- a/gears/system/account-management/account-management-sdk/src/lib.rs +++ b/gears/system/account-management/account-management-sdk/src/lib.rs @@ -59,8 +59,8 @@ pub use idp::{ }; pub use idp_user::{ IdpDeprovisionUserRequest, IdpListUsersRequest, IdpNewUser, IdpProvisionUserRequest, - IdpTenantContext, IdpUser, IdpUserFilterField, IdpUserOperationFailure, IdpUserPagination, - IdpUserPaginationError, IdpUserQuery, ListUsersQuery, NewUserPassword, + IdpTenantContext, IdpUser, IdpUserDuplicateField, IdpUserFilterField, IdpUserOperationFailure, + IdpUserPagination, IdpUserPaginationError, IdpUserQuery, ListUsersQuery, NewUserPassword, }; pub use metadata::{ MetadataEntry, MetadataEntryFilterField, MetadataEntryQuery, UpsertMetadataRequest, diff --git a/gears/system/account-management/account-management-sdk/src/tenant.rs b/gears/system/account-management/account-management-sdk/src/tenant.rs index aa62d5fbe..17d602df0 100644 --- a/gears/system/account-management/account-management-sdk/src/tenant.rs +++ b/gears/system/account-management/account-management-sdk/src/tenant.rs @@ -240,7 +240,11 @@ pub struct TenantInfoQuery { /// (the serde rename on the SDK enum). The `OData` parser only /// validates the value is a String; arbitrary unknown values /// (including the AM-internal `"provisioning"`) reach storage and - /// are mapped to a domain error downstream. + /// are mapped to a domain error downstream. `$orderby=status` is + /// supported and sorts by the lifecycle ordinal (`active` < + /// `suspended` < `deleted`), not alphabetically — operator UIs sort + /// their Status column with it; filter comparisons stay + /// membership-only (`eq` / `ne` / `in`). #[odata(filter(kind = "String"))] pub status: String, /// Deterministic `UUIDv5` of the registered tenant-type schema id. @@ -259,7 +263,7 @@ pub struct TenantInfoQuery { /// string is mapped server-side to its deterministic `UUIDv5` and /// compared against `tenant_type_uuid`; ordered operators and /// `$orderby` are rejected (sorting by a derived UUID is - /// meaningless), mirroring `status`. + /// meaningless). #[odata(filter(kind = "String"))] pub tenant_type: String, /// `tenants.self_managed` flag. Useful to surface "boundary" diff --git a/gears/system/account-management/account-management/src/domain/error.rs b/gears/system/account-management/account-management/src/domain/error.rs index c1b27121e..a12d57364 100644 --- a/gears/system/account-management/account-management/src/domain/error.rs +++ b/gears/system/account-management/account-management/src/domain/error.rs @@ -151,6 +151,35 @@ pub enum DomainError { #[error("already exists: {detail}")] AlreadyExists { detail: String }, + /// The `IdP` reported a uniqueness collision on a user attribute + /// during a user operation (duplicate username in the + /// realm, duplicate email realm-wide, or KC's combined + /// "username or email" constant). Distinct from + /// [`Self::AlreadyExists`] (tenant-flavoured, DB-classifier + /// produced) so the canonical envelope rides the `user` resource + /// type. Carries the typed colliding field — the boundary mapping + /// derives both the stable `resource_name` token and the curated + /// public detail from it, so the wording lives in exactly one + /// place and no caller can pair an inconsistent detail/resource. + /// The caller-supplied value is never echoed (redaction posture of + /// the `IdP` boundary). Surfaces as HTTP 409 `already_exists`. + #[error("user already exists ({})", field.as_field_token())] + UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField, + }, + + /// The `IdP` rejected the supplied password against its configured + /// password policy. Distinct from [`Self::Validation`] + /// so the canonical envelope carries the structured + /// `password` / `PASSWORD_POLICY` field-violation tokens instead + /// of the generic `request` / `VALIDATION` pair — clients can + /// attribute the failure to the password input. The raw policy + /// text is redacted upstream (digest in `am.idp` logs); `detail` + /// is the curated public summary. Surfaces as HTTP 400 + /// `invalid_argument`. + #[error("password rejected by IdP policy: {detail}")] + IdpPasswordPolicy { detail: String }, + // ---- Aborted (HTTP 409) ---- /// Retry-budget-exhausted serialization failure surfaced by /// [`crate::infra::storage::repo_impl::helpers::with_serializable_retry`] diff --git a/gears/system/account-management/account-management/src/domain/error_tests.rs b/gears/system/account-management/account-management/src/domain/error_tests.rs index a77a7c91e..7b86c1af8 100644 --- a/gears/system/account-management/account-management/src/domain/error_tests.rs +++ b/gears/system/account-management/account-management/src/domain/error_tests.rs @@ -316,6 +316,8 @@ impl DomainError { Self::MetadataEntryNotFound { .. } => "metadata_entry_not_found", Self::MetadataVersionMismatch { .. } => "metadata_version_mismatch", Self::AlreadyExists { .. } => "already_exists", + Self::UserAlreadyExists { .. } => "user_already_exists", + Self::IdpPasswordPolicy { .. } => "idp_password_policy", Self::Aborted { .. } => "aborted", Self::TypeNotAllowed { .. } => "type_not_allowed", Self::TenantDepthExceeded { .. } => "tenant_depth_exceeded", @@ -365,12 +367,14 @@ impl DomainError { | Self::PendingExists { .. } | Self::AlreadyResolved | Self::Conflict { .. } + | Self::IdpPasswordPolicy { .. } | Self::FeatureDisabled { .. } => 400, Self::NotFound { .. } | Self::UserNotFound { .. } | Self::ConversionRequestNotFound { .. } | Self::MetadataEntryNotFound { .. } => 404, Self::AlreadyExists { .. } + | Self::UserAlreadyExists { .. } | Self::Aborted { .. } | Self::MetadataVersionMismatch { .. } => 409, Self::CrossTenantDenied { .. } => 403, diff --git a/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs b/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs index 53818cd1a..a8e5a460b 100644 --- a/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs +++ b/gears/system/account-management/account-management/src/domain/idp/idp_tests.rs @@ -108,3 +108,101 @@ fn provision_ambiguous_chains_redacted_cause_without_leaking_raw_detail() { "DomainError::Internal::cause MUST be reachable via Error::source" ); } + +// --------------------------------------------------------------------------- +// Classified user-operation rejections. +// --------------------------------------------------------------------------- + +#[test] +fn user_duplicate_username_maps_to_user_already_exists() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::Username, + detail: "User exists with same username".into(), + } + .into_domain_error(fixture_tenant_id()); + // The typed field survives verbatim; the curated public wording is + // derived (once) at the canonical boundary, so raw provider text + // cannot leak from here by construction. + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::Username + } + ), + "expected UserAlreadyExists(Username), got {err:?}" + ); +} + +#[test] +fn user_duplicate_email_maps_to_user_already_exists() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::Email, + detail: "User exists with same email".into(), + } + .into_domain_error(fixture_tenant_id()); + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::Email + } + ), + "expected UserAlreadyExists(Email), got {err:?}" + ); +} + +// KC's ModelDuplicateException path emits the combined constant "User +// exists with same username or email" (on KC 26 it is the only 409 +// `createUser` produces directly) — the unattributable classification +// must survive to the domain error, not collapse into Username. +#[test] +fn user_duplicate_combined_maps_to_username_or_email() { + let err = IdpUserOperationFailure::DuplicateUser { + field: account_management_sdk::IdpUserDuplicateField::UsernameOrEmail, + detail: "User exists with same username or email".into(), + } + .into_domain_error(fixture_tenant_id()); + assert!( + matches!( + err, + DomainError::UserAlreadyExists { + field: account_management_sdk::IdpUserDuplicateField::UsernameOrEmail + } + ), + "expected UserAlreadyExists(UsernameOrEmail), got {err:?}" + ); +} + +#[test] +fn user_password_policy_maps_to_structured_password_violation() { + let err = IdpUserOperationFailure::PasswordPolicy { + detail: "invalidPasswordMinLengthMessage: 12".into(), + } + .into_domain_error(fixture_tenant_id()); + let DomainError::IdpPasswordPolicy { detail } = err else { + panic!("expected IdpPasswordPolicy, got a different variant"); + }; + // Curated public summary only — the raw KC policy text stays in + // the digest-only log line. + assert!( + !detail.contains("invalidPasswordMinLengthMessage"), + "raw provider policy text leaked into public detail: {detail}" + ); +} + +// The unclassified catch-all keeps its historical shape: a provider +// rejection the plugin could not attribute still collapses to the +// redacted generic Validation (no accidental behavior change for +// legacy plugins that only emit `Rejected`). +#[test] +fn user_rejected_still_maps_to_redacted_validation() { + let err = IdpUserOperationFailure::Rejected { + detail: "something vendor-specific".into(), + } + .into_domain_error(fixture_tenant_id()); + let DomainError::Validation { detail } = err else { + panic!("expected Validation, got a different variant"); + }; + assert!(detail.contains("detail redacted"), "got: {detail}"); +} diff --git a/gears/system/account-management/account-management/src/domain/idp/mod.rs b/gears/system/account-management/account-management/src/domain/idp/mod.rs index 4632f887a..b81126256 100644 --- a/gears/system/account-management/account-management/src/domain/idp/mod.rs +++ b/gears/system/account-management/account-management/src/domain/idp/mod.rs @@ -361,6 +361,42 @@ impl UserOperationFailureExt for IdpUserOperationFailure { } // @cpt-end:cpt-cf-account-management-algo-idp-user-operations-contract-idp-contract-invocation:p1:inst-algo-ici-provider-error-return // @cpt-end:cpt-cf-account-management-algo-idp-user-operations-contract-idp-contract-invocation:p1:inst-algo-ici-provider-error + // Classified uniqueness collision: surface HTTP + // 409 `already_exists` with the stable colliding-field + // token, instead of collapsing into the redacted generic + // Validation. The provider detail is still digest-only in + // logs — the public detail is derived from the typed field + // at the canonical boundary. + Self::DuplicateUser { field, detail } => { + let (digest, len) = redact_provider_detail(&detail); + tracing::warn!( + target: "am.idp", + tenant_id = %tenant_id, + field = field.as_field_token(), + provider_detail_digest = digest, + provider_detail_len = len, + "IdP user operation DuplicateUser; surfacing already_exists, raw detail redacted" + ); + DomainError::UserAlreadyExists { field } + } + // Classified password-policy reject: surface + // the structured `password` / `PASSWORD_POLICY` violation + // instead of the generic `request` / `VALIDATION`. Raw + // policy text stays digest-only in logs. + Self::PasswordPolicy { detail } => { + let (digest, len) = redact_provider_detail(&detail); + tracing::warn!( + target: "am.idp", + tenant_id = %tenant_id, + provider_detail_digest = digest, + provider_detail_len = len, + "IdP user operation PasswordPolicy; surfacing password field violation, raw detail redacted" + ); + DomainError::IdpPasswordPolicy { + detail: "the supplied password does not meet the identity provider's password policy" + .to_owned(), + } + } // SDK enum is `#[non_exhaustive]`. A new variant added in // a future SDK release lands here until the AM-side // mapping is updated; surface as `Internal` with a loud diff --git a/gears/system/account-management/account-management/src/domain/system_actor.rs b/gears/system/account-management/account-management/src/domain/system_actor.rs index 92bbf9185..2be68da86 100644 --- a/gears/system/account-management/account-management/src/domain/system_actor.rs +++ b/gears/system/account-management/account-management/src/domain/system_actor.rs @@ -151,7 +151,18 @@ pub(crate) fn for_retention_sweep(tenant_id: Uuid) -> SecurityContext { /// User-groups cascade-cleanup hook — fired when a tenant is /// hard-deleted and AM must walk its user-group memberships in RG. -/// `tenant_id` is the tenant being deleted. +/// `tenant_id` is the tenant being deleted; it rides the audit log +/// line only. +/// +/// Platform-scoped, NOT scoped to `tenant_id`: the subject +/// tenant here is by definition a `Deleted` row mid-purge, and the +/// `AuthZ` resolver's system-actor grant would root a tenant-subtree +/// scope at it — deleted roots are clamped out of scope +/// materialization, the subtree resolves to zero tenants, and every +/// RG cascade call fails closed. The reaper then defers the whole +/// backlog forever. Platform scope (nil tenant → Global grant) is +/// honest for this flow: the cascade addresses explicit group ids of +/// a tenant that is being erased, not a live tenant's data. #[must_use] pub(crate) fn for_user_groups_cascade(tenant_id: Uuid) -> SecurityContext { tracing::info!( @@ -160,7 +171,7 @@ pub(crate) fn for_user_groups_cascade(tenant_id: Uuid) -> SecurityContext { tenant_id = %tenant_id, "am system actor constructed", ); - build_inner(Some(tenant_id)) + build_inner(None) } #[cfg(test)] @@ -193,7 +204,6 @@ mod tests { ("bootstrap", for_bootstrap(tenant)), ("provisioning_reaper", for_provisioning_reaper(tenant)), ("retention_sweep", for_retention_sweep(tenant)), - ("user_groups_cascade", for_user_groups_cascade(tenant)), ] { assert_eq!( ctx.subject_id(), @@ -212,4 +222,20 @@ mod tests { ); } } + + // The cascade hook's subject tenant is a Deleted row + // mid-purge — a subtree grant rooted at it materializes to zero + // tenants and fails the whole reaper closed. The factory MUST be + // platform-scoped (nil tenant → Global grant at the resolver). + #[test] + fn user_groups_cascade_is_platform_scoped() { + let ctx = for_user_groups_cascade(Uuid::from_u128(0xDEAD_BEEF_FACE_CAFE)); + assert_eq!(ctx.subject_id(), AM_SYSTEM_ACTOR_UUID); + assert_eq!(ctx.subject_type(), Some(AM_SYSTEM_SUBJECT_TYPE)); + assert_eq!( + ctx.subject_tenant_id(), + Uuid::nil(), + "cascade context must be platform-scoped, not scoped to the deleted tenant" + ); + } } diff --git a/gears/system/account-management/account-management/src/domain/tenant/repo.rs b/gears/system/account-management/account-management/src/domain/tenant/repo.rs index e2c710c83..2d88cd3f9 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/repo.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/repo.rs @@ -405,8 +405,14 @@ pub trait TenantRepo: Send + Sync { /// Flip the tenant from its current SDK-visible state to /// `Deleted`, stamp `deleted_at = now` (which also starts the /// retention timer — eligibility for hard-delete becomes - /// `deleted_at + retention_window`), and rewrite - /// `tenant_closure.descendant_status` in the same transaction. + /// `deleted_at + retention_window`), rewrite + /// `tenant_closure.descendant_status`, and terminal-resolve + /// (cancel) any still-pending mode-conversion request whose + /// subject is this tenant — all in the same transaction + /// (a committed soft-delete must never leave a + /// `pending` conversion row referencing the tombstone). + /// `deleted_by` is stamped as the auto-cancelled request's + /// `cancelled_by`. /// /// **Idempotent.** Calling on a row that is already in `Deleted` /// status returns the existing tombstone without re-stamping @@ -421,6 +427,7 @@ pub trait TenantRepo: Send + Sync { &self, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result; diff --git a/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs b/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs index 8d107b375..8bb6f872c 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/service/mod.rs @@ -1877,7 +1877,7 @@ impl TenantService { }; let updated = self .repo - .schedule_deletion(&scope, tenant_id, now, retention) + .schedule_deletion(&scope, tenant_id, ctx.subject_id(), now, retention) .await?; // TODO(events): emit AM event when platform event-bus lands. tracing::info!( diff --git a/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs b/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs index a1d93070c..39500bc08 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/service/service_tests.rs @@ -1924,6 +1924,7 @@ async fn hard_delete_batch_skips_rows_already_claimed_by_another_worker() { .schedule_deletion( &AccessScope::allow_all(), id, + Uuid::nil(), now, Some(StdDuration::from_secs(0)), ) diff --git a/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs b/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs index 7cf750cd6..c2b5a39b3 100644 --- a/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs +++ b/gears/system/account-management/account-management/src/domain/tenant/test_support/repo.rs @@ -1323,10 +1323,14 @@ impl TenantRepo for FakeTenantRepo { Ok(u64::try_from(state.closure.len()).unwrap_or(u64::MAX)) } + // `_deleted_by` is unused: the in-memory fake models the `tenants` + // table only — the conversion auto-cancel cascade is a + // real-repo TX concern covered by the sqlite integration tests. async fn schedule_deletion( &self, _scope: &AccessScope, id: Uuid, + _deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { @@ -1735,6 +1739,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), parent, + Uuid::nil(), ts(1_700_000_100), Some(Duration::from_secs(0)), ) @@ -1767,6 +1772,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), leaf, + Uuid::nil(), ts(1_700_000_100), Some(Duration::from_secs(0)), ) @@ -1792,6 +1798,7 @@ mod repo_contract_tests { .schedule_deletion( &AccessScope::allow_all(), leaf, + Uuid::nil(), ts(1_700_000_500), Some(Duration::from_secs(0)), ) diff --git a/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs b/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs index 818e54b85..10f69f59b 100644 --- a/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs +++ b/gears/system/account-management/account-management/src/infra/sdk_error_mapping.rs @@ -141,6 +141,17 @@ impl From for CanonicalError { account_management_sdk::field::IDP_INVALID_INPUT, ) .create(), + // IdP password-policy reject: structured + // `password` / `PASSWORD_POLICY` tokens on the `user` + // resource, so clients attribute the 400 to the password + // input instead of the generic `request` / `VALIDATION`. + DomainError::IdpPasswordPolicy { detail } => UserResource::invalid_argument() + .with_field_violation( + account_management_sdk::field::PASSWORD_FIELD, + detail, + account_management_sdk::field::PASSWORD_POLICY, + ) + .create(), // ---- NotFound (HTTP 404) — one resource per variant ---- DomainError::NotFound { detail, resource } => TenantResource::not_found(detail) @@ -184,6 +195,17 @@ impl From for CanonicalError { DomainError::AlreadyExists { detail } => TenantResource::already_exists(detail) .with_resource("tenant") .create(), + // IdP-reported user uniqueness collision: both + // the curated public detail and the stable `resource_name` + // token derive from the typed field here — the single + // source of the wording; the caller-supplied value is + // never echoed. + DomainError::UserAlreadyExists { field } => UserResource::already_exists(format!( + "a user with this {} already exists", + field.as_human_phrase() + )) + .with_resource(field.as_field_token()) + .create(), // Duplicate-on-create per AIP-193: the at-most-one-pending // invariant surfaces as HTTP 409 with the existing // `request_id` as the structural resource identifier. diff --git a/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs b/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs index b3d36a9e8..4ea241b40 100644 --- a/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs +++ b/gears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rs @@ -267,6 +267,77 @@ fn already_exists_maps_to_409() { ); } +/// An `IdP`-reported user uniqueness collision surfaces as +/// 409 `already_exists` on the USER resource type, with the stable +/// colliding-field token as `resource_name` and a detail derived from +/// the typed field — never the caller-supplied value and never the raw +/// provider text. +#[test] +fn user_already_exists_maps_to_409_with_user_resource() { + for (field, token, phrase) in [ + ( + account_management_sdk::IdpUserDuplicateField::Username, + "username", + "a user with this username already exists", + ), + ( + account_management_sdk::IdpUserDuplicateField::Email, + "email", + "a user with this email already exists", + ), + ( + account_management_sdk::IdpUserDuplicateField::UsernameOrEmail, + "username_or_email", + "a user with this username or email already exists", + ), + ] { + let canonical = round_trip(DomainError::UserAlreadyExists { field }); + assert_eq!(canonical.status_code(), 409); + assert_eq!(canonical.resource_name(), Some(token)); + assert_eq!( + canonical.resource_type(), + Some(account_management_sdk::gts::USER_RESOURCE_TYPE) + ); + assert!( + matches!(canonical, CanonicalError::AlreadyExists { .. }), + "UserAlreadyExists MUST surface as the AlreadyExists variant" + ); + assert_eq!(canonical.detail(), phrase); + } +} + +/// An `IdP` password-policy reject carries the structured +/// `password` / `PASSWORD_POLICY` field-violation tokens (not the +/// generic `request` / `VALIDATION` pair) so clients can attribute +/// the 400 to the password input without parsing `detail`. +#[test] +fn idp_password_policy_maps_to_400_with_password_field_violation() { + let canonical = round_trip(DomainError::IdpPasswordPolicy { + detail: "the supplied password does not meet the identity provider's password policy" + .to_owned(), + }); + assert_eq!(canonical.status_code(), 400); + assert_eq!( + canonical.resource_type(), + Some(account_management_sdk::gts::USER_RESOURCE_TYPE) + ); + let CanonicalError::InvalidArgument { ctx, .. } = canonical else { + panic!("expected CanonicalError::InvalidArgument"); + }; + let InvalidArgument::FieldViolations { field_violations } = ctx else { + panic!("expected InvalidArgument::FieldViolations ctx"); + }; + assert_eq!(field_violations.len(), 1); + assert_eq!( + field_violations[0].field, + account_management_sdk::field::PASSWORD_FIELD + ); + assert_eq!( + field_violations[0].reason, + account_management_sdk::field::PASSWORD_POLICY + ); +} + // --------------------------------------------------------------------------- // Aborted (HTTP 409 with reason) // --------------------------------------------------------------------------- diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs index 24b22294a..b984dda9b 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rs @@ -493,6 +493,66 @@ async fn find_pending_for_tenant( /// [`DomainError::NotFound`] from [`DomainError::AlreadyResolved`] per /// the trait contract; on `rows_affected == 1` re-reads the row to /// return the post-transition snapshot to the caller. +/// Terminal-resolve (cancel) every still-pending conversion request +/// whose subject tenant is being soft-deleted, INSIDE the caller's +/// transaction. Called by the tenant repo's +/// `schedule_deletion` SERIALIZABLE TX so the tenant flip and the +/// conversion resolution commit or roll back together — a `204` from +/// DELETE `/tenants/{id}` can never leave a `pending` row dangling on +/// a tombstoned tenant. Lives here (not in `updates.rs`) so ownership +/// of the `conversion_requests` guarded-transition shape stays with +/// the conversion repo. +/// +/// Only rows keyed by `tenant_id = :id` can exist at this point: a +/// parent with non-deleted children is rejected by the +/// `TenantHasChildren` guard before the TX, so inbound conversions of +/// live children never reach this path. +/// +/// `allow_all`: this is a structural cascade inside an +/// already-authorized delete (PDP gate at the service layer), same +/// rationale as the closure rewrite in the same TX. The +/// `status = Pending AND deleted_at IS NULL` fence makes the cascade +/// idempotent — a soft-delete retry finds no pending row and affects +/// zero rows. Returns `rows_affected`. +pub(super) async fn cancel_pending_on_tenant_delete_tx( + tx: &DbTx<'_>, + tenant_id: Uuid, + cancelled_by: Uuid, + resolved_at: OffsetDateTime, +) -> Result { + let res = conversion_requests::Entity::update_many() + .col_expr( + conversion_requests::Column::Status, + Expr::value(ConversionStatus::Cancelled.as_smallint()), + ) + .col_expr( + conversion_requests::Column::CancelledBy, + Expr::value(Some(cancelled_by)), + ) + .col_expr( + conversion_requests::Column::ResolvedAt, + Expr::value(Some(resolved_at)), + ) + .col_expr( + conversion_requests::Column::CancelledComment, + Expr::value(Some("subject tenant deleted".to_owned())), + ) + .filter( + Condition::all() + .add(conversion_requests::Column::TenantId.eq(tenant_id)) + .add( + conversion_requests::Column::Status.eq(ConversionStatus::Pending.as_smallint()), + ) + .add(conversion_requests::Column::DeletedAt.is_null()), + ) + .secure() + .scope_with(&AccessScope::allow_all()) + .exec(tx) + .await + .map_err(map_scope_to_tx)?; + Ok(res.rows_affected) +} + async fn run_guarded_transition( repo: &ConversionRepoImpl, scope: &AccessScope, diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs index 3bd62ab70..bf9f6695f 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rs @@ -225,10 +225,11 @@ impl TenantRepo for TenantRepoImpl { &self, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { - updates::schedule_deletion(self, scope, id, now, retention).await + updates::schedule_deletion(self, scope, id, deleted_by, now, retention).await } async fn check_hard_delete_eligibility( diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs index 3a993e089..375671633 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs @@ -17,7 +17,7 @@ use toolkit_db::odata::sea_orm_filter::{ FieldToColumn, LimitCfg, ODataFieldMapping, PaginateOdataTryError, paginate_odata_try, }; use toolkit_db::secure::SecureEntityExt; -use toolkit_odata::filter::{FilterOp, ODataValue}; +use toolkit_odata::filter::{FieldKind, FilterField as _, FilterOp, ODataValue}; use toolkit_odata::{ODataQuery, Page, SortDir, ast}; use toolkit_security::AccessScope; use uuid::Uuid; @@ -136,23 +136,20 @@ impl FieldToColumn for TenantODataMapper { } } - /// Reject `$orderby=status` and `status` cursor keys: the column - /// is exposed as a string contract on the wire while it is - /// `SMALLINT` in storage, and there is no consistent ordering - /// across the two shapes — alphabetical (`active < deleted < - /// suspended`) versus numeric (`Active = 1 < Suspended = 2 < - /// Deleted = 3`). The framework rejects the `$orderby` clause as - /// `InvalidOrderByField` before composing the effective order, so - /// the cursor codec never sees a translated-shape value. + /// `$orderby=status` sorts by the lifecycle ordinal (`Active = 1 < + /// Suspended = 2 < Deleted = 3`), NOT alphabetically by the wire + /// string — a deliberate categorical order (healthy first, + /// tombstones last, and inverted via `desc`) that operator UIs sort + /// their Status column by. Cursor pages are safe: both + /// the effective-order comparison and `extract_cursor_value` speak + /// the storage `SMALLINT`, so the codec never sees a + /// translated-shape value. + /// + /// `$orderby=tenant_type` stays rejected: it compares via a derived + /// `UUIDv5`, and an ordered comparison on a derived UUID has no + /// honest meaning. fn is_orderable(field: TenantInfoFilterField) -> bool { - // `status` and `tenant_type` are exposed as strings on the wire but - // compared via a translated storage shape (SMALLINT / derived - // UUIDv5); neither has a consistent wire-vs-storage order, so - // `$orderby` on them is rejected before the effective order composes. - !matches!( - field, - TenantInfoFilterField::Status | TenantInfoFilterField::TenantType - ) + !matches!(field, TenantInfoFilterField::TenantType) } } @@ -184,6 +181,17 @@ impl ODataFieldMapping for TenantODataMapper { } } } + + /// `status` is a wire string but sorts (and therefore cursors) by + /// its storage lifecycle ordinal — encode/parse its cursor token as + /// an integer, not with the wire `String` kind. Every + /// other field's cursor shape matches its wire kind. + fn cursor_kind(field: TenantInfoFilterField) -> FieldKind { + match field { + TenantInfoFilterField::Status => FieldKind::I64, + other => other.kind(), + } + } } /// Pagination limits for the tenant-children listing surface. Mirrors @@ -648,4 +656,15 @@ mod tenant_type_filter_tests { Field::TenantType )); } + + // Regression guard: the Tenants-page Status column sorts via + // `$orderby=status`; the field must be orderable (lifecycle-ordinal + // order — see `is_orderable` docs), not rejected with + // `InvalidOrderByField` → 400 as it used to be. + #[test] + fn status_is_orderable() { + assert!(>::is_orderable( + Field::Status + )); + } } diff --git a/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs b/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs index 074cffeb0..fe2b2d088 100644 --- a/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs +++ b/gears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rs @@ -435,6 +435,7 @@ pub(super) async fn schedule_deletion( repo: &TenantRepoImpl, scope: &AccessScope, id: Uuid, + deleted_by: Uuid, now: OffsetDateTime, retention: Option, ) -> Result { @@ -486,7 +487,21 @@ pub(super) async fn schedule_deletion( // un-contended retry path; this guard is the // authoritative correctness boundary. if existing.status == TenantStatus::Deleted.as_smallint() { - return entity_to_model(existing).map_err(TxError::Domain); + // Heal-on-retry: tenants soft-deleted BEFORE + // the conversion cascade existed can still carry a + // pending conversion row, and retrying the DELETE is + // the natural operator remediation — so the + // (idempotent, Pending-fenced) cancel runs on this + // path too instead of returning early past it. For + // post-fix tombstones the fence matches zero rows and + // this is a no-op. + let healed = super::conversion::cancel_pending_on_tenant_delete_tx( + tx, id, deleted_by, now, + ) + .await?; + return entity_to_model(existing) + .map(|m| (m, healed)) + .map_err(TxError::Domain); } // `Provisioning` rows have no closure entries by // construction; flipping them straight to `Deleted` @@ -595,6 +610,22 @@ pub(super) async fn schedule_deletion( } // @cpt-end:cpt-cf-account-management-algo-tenant-hierarchy-management-closure-maintenance:p1:inst-algo-closmnt-repo-soft-delete-status + // A pending mode-conversion request whose + // subject is this tenant must not outlive it — resolve + // (cancel) it in the SAME transaction as the status + // flip, so a committed soft-delete can never leave a + // `pending` row referencing a tombstone. The guarded + // UPDATE's `status = Pending` fence keeps concurrent / + // repeated deletes from re-stamping resolved rows. The + // count rides the closure's return value: the + // `am.events` emission happens AFTER the TX commits — + // logging in here would double-count on a serialization + // retry and emit a phantom event if the TX ultimately + // rolled back. + let cancelled = + super::conversion::cancel_pending_on_tenant_delete_tx(tx, id, deleted_by, now) + .await?; + let fresh = tenants::Entity::find() .secure() .scope_with(&scope) @@ -606,9 +637,25 @@ pub(super) async fn schedule_deletion( diagnostic: format!("tenant {id} disappeared after schedule_deletion"), cause: None, })?; - entity_to_model(fresh).map_err(TxError::Domain) + entity_to_model(fresh) + .map(|m| (m, cancelled)) + .map_err(TxError::Domain) }) }) }) .await + .map(|(model, cancelled)| { + if cancelled > 0 { + tracing::info!( + target: "am.events", + kind = "conversionStateChanged", + tenant_id = %id, + cancelled_by = %deleted_by, + rows = cancelled, + event = "conversion_auto_cancelled_on_tenant_delete", + "am conversion request auto-cancelled by tenant soft-delete" + ); + } + model + }) } diff --git a/gears/system/account-management/account-management/tests/conversion_integration.rs b/gears/system/account-management/account-management/tests/conversion_integration.rs index 8c90797a0..fd35b87fc 100644 --- a/gears/system/account-management/account-management/tests/conversion_integration.rs +++ b/gears/system/account-management/account-management/tests/conversion_integration.rs @@ -1114,3 +1114,160 @@ async fn approve_recomputes_barrier_for_closure_rows_referencing_soft_deleted_de .expect("(mid, leaf) row"); assert_eq!(row_mid_leaf.barrier, 0); } + +// --------------------------------------------------------------------------- +// Tenant soft-delete auto-cancels the pending conversion in-TX. +// --------------------------------------------------------------------------- + +/// Seed a pending conversion for `child` and return its id. +async fn seed_pending_conversion( + conv_repo: &ConversionRepoImpl, + child: Uuid, + parent: Uuid, + now: OffsetDateTime, +) -> Uuid { + let new = NewConversionRequest { + id: Uuid::new_v4(), + tenant_id: child, + parent_id: Some(parent), + child_tenant_name: "c".into(), + initiator_side: ConversionSide::Parent, + target_mode: TargetMode::SelfManaged, + requested_by: Uuid::new_v4(), + requested_at: now, + expires_at: now + TimeDuration::days(7), + requested_comment: None, + }; + conv_repo + .insert_pending(&allow_all(), &new) + .await + .expect("seed pending conversion") + .id +} + +/// DELETE `/tenants/{id}` with an open conversion used to return 204 and +/// leave the row `pending` with a dangling FK onto the tombstone. The +/// cascade now runs inside `schedule_deletion`'s SERIALIZABLE TX: the +/// tenant flip and the cancel commit together. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_cancels_pending_conversion_in_same_tx() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9100); + let child = Uuid::from_u128(0x9101); + seed_root(&h, root).await; + create_active_child(&h, child, root, "vhp1729-c", false, 1).await; + + let now = fixed_now(); + let request_id = seed_pending_conversion(&conv_repo, child, root, now).await; + let deleter = Uuid::from_u128(0x00DE_1E7E); + + let tombstone = h + .repo + .schedule_deletion(&allow_all(), child, deleter, now, None) + .await + .expect("soft delete"); + assert_eq!(tombstone.status, TenantStatus::Deleted); + + let resolved = conv_repo + .find_by_id(&allow_all(), request_id) + .await + .expect("find") + .expect("row still exists (resolved, not removed)"); + assert_eq!( + resolved.status, + ConversionStatus::Cancelled, + "pending conversion MUST be terminal-resolved by the delete TX" + ); + assert_eq!(resolved.cancelled_by, Some(deleter)); + assert_eq!(resolved.resolved_at, Some(now)); + // No pending row survives for the tombstoned tenant. + assert!( + conv_repo + .find_pending_for_tenant(&allow_all(), child) + .await + .expect("query") + .is_none(), + "no pending conversion may reference a tombstoned tenant" + ); +} + +/// Idempotent delete retry must not re-stamp the resolved row, and an +/// already-resolved (cancelled) conversion is left untouched. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_retry_leaves_resolved_conversion_untouched() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9200); + let child = Uuid::from_u128(0x9201); + seed_root(&h, root).await; + create_active_child(&h, child, root, "vhp1729-r", false, 1).await; + + let now = fixed_now(); + let request_id = seed_pending_conversion(&conv_repo, child, root, now).await; + let first_deleter = Uuid::from_u128(0xD1); + + h.repo + .schedule_deletion(&allow_all(), child, first_deleter, now, None) + .await + .expect("first delete"); + // Retry with a DIFFERENT actor and later timestamp: the idempotent + // short-circuit + the `status = Pending` fence must preserve the + // first resolution verbatim. + let later = now + TimeDuration::hours(1); + h.repo + .schedule_deletion(&allow_all(), child, Uuid::from_u128(0xD2), later, None) + .await + .expect("idempotent retry"); + + let resolved = conv_repo + .find_by_id(&allow_all(), request_id) + .await + .expect("find") + .expect("row"); + assert_eq!(resolved.status, ConversionStatus::Cancelled); + assert_eq!( + resolved.cancelled_by, + Some(first_deleter), + "retry must not re-stamp cancelled_by" + ); + assert_eq!( + resolved.resolved_at, + Some(now), + "retry must not move resolved_at" + ); +} + +/// The cascade is keyed by `tenant_id`: deleting one child must not +/// touch a sibling's pending conversion. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn soft_delete_does_not_touch_other_tenants_conversions() { + let h = setup_sqlite().await.expect("harness"); + let conv_repo = Arc::new(ConversionRepoImpl::new(Arc::clone(&h.provider))); + let root = Uuid::from_u128(0x9300); + let doomed = Uuid::from_u128(0x9301); + let sibling = Uuid::from_u128(0x9302); + seed_root(&h, root).await; + create_active_child(&h, doomed, root, "vhp1729-d", false, 1).await; + create_active_child(&h, sibling, root, "vhp1729-s", false, 1).await; + + let now = fixed_now(); + seed_pending_conversion(&conv_repo, doomed, root, now).await; + let sibling_request = seed_pending_conversion(&conv_repo, sibling, root, now).await; + + h.repo + .schedule_deletion(&allow_all(), doomed, Uuid::nil(), now, None) + .await + .expect("delete doomed"); + + let untouched = conv_repo + .find_by_id(&allow_all(), sibling_request) + .await + .expect("find") + .expect("row"); + assert_eq!( + untouched.status, + ConversionStatus::Pending, + "sibling's pending conversion MUST survive" + ); +} diff --git a/gears/system/account-management/account-management/tests/lifecycle_integration.rs b/gears/system/account-management/account-management/tests/lifecycle_integration.rs index e8ed7f323..fe76d8fca 100644 --- a/gears/system/account-management/account-management/tests/lifecycle_integration.rs +++ b/gears/system/account-management/account-management/tests/lifecycle_integration.rs @@ -442,7 +442,7 @@ async fn schedule_deletion_rewrites_closure_to_deleted() { let now = OffsetDateTime::now_utc(); let outcome = h .repo - .schedule_deletion(&allow_all(), leaf, now, None) + .schedule_deletion(&allow_all(), leaf, Uuid::nil(), now, None) .await .expect("schedule_deletion"); assert_eq!(outcome.status, TenantStatus::Deleted); @@ -598,6 +598,7 @@ async fn hard_delete_one_cleans_tenant_and_closure_rows() { .schedule_deletion( &allow_all(), leaf, + Uuid::nil(), scheduled_at, Some(Duration::from_secs(1)), ) @@ -680,6 +681,7 @@ async fn hard_delete_one_rejects_lost_claim() { .schedule_deletion( &allow_all(), leaf, + Uuid::nil(), scheduled_at, Some(Duration::from_secs(1)), ) diff --git a/gears/system/account-management/account-management/tests/list_children_integration.rs b/gears/system/account-management/account-management/tests/list_children_integration.rs index 5af1c4e64..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,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. +/// `$orderby=status` sorts children by the lifecycle ordinal +/// (`Active = 1 < Suspended = 2 < Deleted = 3`), the order the +/// Tenants-page Status column uses. Insertion (`created_at`) order is +/// deliberately NOT the ordinal order, so a passing assertion proves +/// the sort honoured `status`, not the chronological default. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn list_children_rejects_orderby_on_status() { +async fn list_children_orderby_status_sorts_by_lifecycle_ordinal() { let h = setup_sqlite().await.expect("harness"); let root = Uuid::from_u128(ROOT_ID); seed_root(&h, root).await; + let type_a = Uuid::from_u128(0xAA); + let suspended_old = Uuid::from_u128(0x701); + let active_new = Uuid::from_u128(0x702); + let suspended_new = Uuid::from_u128(0x703); + seed_tenant_at(&h, suspended_old, root, SUSPENDED, false, type_a, ts_at(1)).await; + seed_tenant_at(&h, active_new, root, ACTIVE, false, type_a, ts_at(2)).await; + seed_tenant_at(&h, suspended_new, root, SUSPENDED, false, type_a, ts_at(3)).await; + let query = ODataQuery::default().with_order(ODataOrderBy(vec![OrderKey { field: "status".to_owned(), dir: SortDir::Asc, }])); - let err = h + let page = h .repo .list_children(&allow_all(), root, &query) .await - .expect_err("$orderby=status MUST be rejected"); - let detail = format!("{err:?}"); - assert!( - detail.contains("status") || detail.contains("InvalidOrderByField"), - "expected order-validation rejection; got {detail}" + .expect("$orderby=status must be accepted"); + + // Effective order is (status ASC, id ASC): active first, then the + // two suspended rows tie-broken by id. + assert_eq!( + ids_of(&page.items), + vec![active_new, suspended_old, suspended_new], + "`$orderby=status asc` MUST sort by lifecycle ordinal with id tiebreak" + ); +} + +/// Cursor guard: ordering by `status` must survive the cursor +/// round-trip. The cursor token carries the storage ordinal +/// (`cursor_kind = I64` on the AM mapper), so page 2+ predicates +/// compare SMALLINT-to-integer — the exact wire-vs-storage mismatch +/// that used to force `is_orderable = false` for this field. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_children_orderby_status_cursor_roundtrip() { + let h = setup_sqlite().await.expect("harness"); + let root = Uuid::from_u128(ROOT_ID); + seed_root(&h, root).await; + + let type_a = Uuid::from_u128(0xAA); + // 5 children, statuses interleaved so every page boundary lands + // inside or between ordinal groups. + let seeds: Vec<(Uuid, i16)> = vec![ + (Uuid::from_u128(0x711), SUSPENDED), + (Uuid::from_u128(0x712), ACTIVE), + (Uuid::from_u128(0x713), SUSPENDED), + (Uuid::from_u128(0x714), ACTIVE), + (Uuid::from_u128(0x715), ACTIVE), + ]; + for (i, (id, status)) in seeds.iter().enumerate() { + seed_tenant_at( + &h, + *id, + root, + *status, + false, + type_a, + ts_at(1) + Duration::seconds(i64::try_from(i).unwrap()), + ) + .await; + } + + // Expected effective order (status ASC, id ASC): the three active + // rows by id, then the two suspended rows by id. + let expected = vec![ + Uuid::from_u128(0x712), + Uuid::from_u128(0x714), + Uuid::from_u128(0x715), + Uuid::from_u128(0x711), + Uuid::from_u128(0x713), + ]; + + 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/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)), ) 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/config.rs b/gears/system/api-gateway/src/config.rs index 0ca959f4f..35c6fbb4e 100644 --- a/gears/system/api-gateway/src/config.rs +++ b/gears/system/api-gateway/src/config.rs @@ -104,6 +104,15 @@ pub struct CorsConfig { pub allowed_methods: Vec, /// Allowed request headers; `["*"]` means any pub allowed_headers: Vec, + /// Response headers exposed to browser JavaScript via + /// `Access-Control-Expose-Headers`; `["*"]` means any (do not combine + /// with `allow_credentials` — the wildcard is invalid with credentials). + /// Defaults to `["ETag"]`: the optimistic-concurrency write contract + /// requires clients to read the `ETag` response header to build + /// `If-Match` guarded writes, and `ETag` is not on the CORS safelist — + /// without exposing it every ETag-guarded write is impossible from a + /// cross-origin browser client. + pub exposed_headers: Vec, /// Whether to allow credentials pub allow_credentials: bool, /// Max age for preflight caching in seconds @@ -123,6 +132,7 @@ impl Default for CorsConfig { "OPTIONS".to_owned(), ], allowed_headers: vec!["*".to_owned()], + exposed_headers: vec!["ETag".to_owned()], allow_credentials: false, max_age_seconds: 600, } diff --git a/gears/system/api-gateway/src/cors.rs b/gears/system/api-gateway/src/cors.rs index b4a81e50a..73d555c2d 100644 --- a/gears/system/api-gateway/src/cors.rs +++ b/gears/system/api-gateway/src/cors.rs @@ -2,7 +2,73 @@ use tower_http::cors::CorsLayer; use crate::config::{ApiGatewayConfig, CorsConfig}; -/// Build a CORS layer from config. +/// Validate the CORS configuration at config-load time, BEFORE any +/// `CorsLayer` is constructed. tower-http enforces the same rules with +/// an `assert!` inside `Layer::layer` — with axum's eager router +/// layering that would be a startup panic (crash-loop) with no pointer +/// at the offending config; failing `init` instead surfaces a clean, +/// actionable error. +/// +/// # Errors +/// Returns a human-readable description of the invalid combination. +pub fn validate_cors_config(cfg: &ApiGatewayConfig) -> Result<(), String> { + if !cfg.cors_enabled { + return Ok(()); + } + let cors_cfg: CorsConfig = cfg.cors.clone().unwrap_or_default(); + if !cors_cfg.allow_credentials { + return Ok(()); + } + // tower-http rejects every wildcard when credentials are allowed + // (`ensure_usable_cors_rules`): origins, methods, request headers, + // and exposed headers. + let wildcard_lists = [ + ("cors.allowed_origins", &cors_cfg.allowed_origins), + ("cors.allowed_methods", &cors_cfg.allowed_methods), + ("cors.allowed_headers", &cors_cfg.allowed_headers), + ("cors.exposed_headers", &cors_cfg.exposed_headers), + ]; + for (name, list) in wildcard_lists { + if list.iter().any(|v| v == "*") { + return Err(format!( + "invalid CORS configuration: `{name}` contains the wildcard \"*\" while \ + `cors.allow_credentials` is true; browsers forbid this combination and \ + tower-http would panic at startup — list the values explicitly or \ + disable credentials" + )); + } + } + Ok(()) +} + +/// Parse a configured string list into typed values, WARNING about (and +/// skipping) every entry that does not parse instead of dropping it +/// silently. A silently-dropped entry is how a config typo turns into a +/// hard-to-diagnose behavioral hole — e.g. an unparseable +/// `exposed_headers` entry silently suppresses `Access-Control-Expose-Headers` +/// and breaks every ETag-guarded browser write. +fn parse_list(list_name: &'static str, items: &[String]) -> Vec { + items + .iter() + .filter_map(|s| { + s.parse::() + .map_err(|_| { + tracing::warn!( + entry = %s, + list = list_name, + "api-gateway CORS config entry does not parse; ignoring it" + ); + }) + .ok() + }) + .collect() +} + +/// Build a CORS layer from config. `["*"]` in a list means "any"; +/// otherwise entries are parsed into their typed forms with a warning +/// for every unparseable entry (see [`parse_list`]). Invalid +/// wildcard+credentials combinations are rejected earlier by +/// [`validate_cors_config`] at gear init. pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { let cors_cfg: CorsConfig = cfg.cors.clone().unwrap_or_default(); @@ -11,11 +77,8 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_origins.iter().any(|o| o == "*") { layer = layer.allow_origin(tower_http::cors::Any); } else { - let origins: Vec = cors_cfg - .allowed_origins - .into_iter() - .filter_map(|s| axum::http::HeaderValue::from_str(&s).ok()) - .collect(); + let origins: Vec = + parse_list("allowed_origins", &cors_cfg.allowed_origins); if !origins.is_empty() { layer = layer.allow_origin(origins); } @@ -24,11 +87,8 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_methods.iter().any(|m| m == "*") { layer = layer.allow_methods(tower_http::cors::Any); } else { - let methods: Vec = cors_cfg - .allowed_methods - .into_iter() - .filter_map(|s| s.parse().ok()) - .collect(); + let methods: Vec = + parse_list("allowed_methods", &cors_cfg.allowed_methods); if !methods.is_empty() { layer = layer.allow_methods(methods); } @@ -37,16 +97,23 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { if cors_cfg.allowed_headers.iter().any(|h| h == "*") { layer = layer.allow_headers(tower_http::cors::Any); } else { - let headers: Vec = cors_cfg - .allowed_headers - .into_iter() - .filter_map(|s| s.parse().ok()) - .collect(); + let headers: Vec = + parse_list("allowed_headers", &cors_cfg.allowed_headers); if !headers.is_empty() { layer = layer.allow_headers(headers); } } + if cors_cfg.exposed_headers.iter().any(|h| h == "*") { + layer = layer.expose_headers(tower_http::cors::Any); + } else { + let headers: Vec = + parse_list("exposed_headers", &cors_cfg.exposed_headers); + if !headers.is_empty() { + layer = layer.expose_headers(headers); + } + } + if cors_cfg.allow_credentials { layer = layer.allow_credentials(true); } @@ -57,3 +124,69 @@ pub fn build_cors_layer(cfg: &ApiGatewayConfig) -> CorsLayer { layer } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::ApiGatewayConfig; + + fn cfg_with(cors: CorsConfig) -> ApiGatewayConfig { + ApiGatewayConfig { + cors_enabled: true, + cors: Some(cors), + ..Default::default() + } + } + + // The exact combination tower-http panics on at layer construction: + // any wildcard list + allow_credentials. Must be a config-load error, + // not a startup crash-loop. + #[test] + fn wildcard_exposed_headers_with_credentials_rejected_at_validation() { + // Explicit origins/methods/headers so ONLY exposed_headers carries + // the wildcard — pins the newly-added list's own validation arm. + let cfg = cfg_with(CorsConfig { + allowed_origins: vec!["https://ui.example.com".to_owned()], + allowed_methods: vec!["GET".to_owned()], + allowed_headers: vec!["Content-Type".to_owned()], + exposed_headers: vec!["*".to_owned()], + allow_credentials: true, + ..Default::default() + }); + let err = validate_cors_config(&cfg).expect_err("wildcard+credentials must be rejected"); + assert!(err.contains("exposed_headers"), "got: {err}"); + } + + #[test] + fn wildcard_origins_with_credentials_rejected_at_validation() { + let cfg = cfg_with(CorsConfig { + allow_credentials: true, + ..Default::default() // default allowed_origins/headers are ["*"] + }); + assert!(validate_cors_config(&cfg).is_err()); + } + + #[test] + fn explicit_lists_with_credentials_pass_validation() { + let cfg = cfg_with(CorsConfig { + allowed_origins: vec!["https://ui.example.com".to_owned()], + allowed_methods: vec!["GET".to_owned(), "POST".to_owned()], + allowed_headers: vec!["Content-Type".to_owned()], + exposed_headers: vec!["ETag".to_owned()], + allow_credentials: true, + ..Default::default() + }); + assert!(validate_cors_config(&cfg).is_ok()); + } + + #[test] + fn cors_disabled_skips_validation() { + let mut cfg = cfg_with(CorsConfig { + exposed_headers: vec!["*".to_owned()], + allow_credentials: true, + ..Default::default() + }); + cfg.cors_enabled = false; + assert!(validate_cors_config(&cfg).is_ok()); + } +} diff --git a/gears/system/api-gateway/src/gear.rs b/gears/system/api-gateway/src/gear.rs index 402bbe34b..7570e4566 100644 --- a/gears/system/api-gateway/src/gear.rs +++ b/gears/system/api-gateway/src/gear.rs @@ -679,6 +679,10 @@ impl ApiGateway { impl toolkit::Gear for ApiGateway { async fn init(&self, ctx: &toolkit::context::GearCtx) -> anyhow::Result<()> { let cfg = ctx.config_or_default::()?; + // Fail init on invalid CORS combinations (wildcard+credentials): + // tower-http would otherwise assert-panic during eager router + // layering — a startup crash-loop with no pointer at the config. + crate::cors::validate_cors_config(&cfg).map_err(|e| anyhow::anyhow!(e))?; self.config.store(Arc::new(cfg.clone())); debug!( diff --git a/gears/system/api-gateway/tests/cors_tests.rs b/gears/system/api-gateway/tests/cors_tests.rs index f582b88e3..59adb6d06 100644 --- a/gears/system/api-gateway/tests/cors_tests.rs +++ b/gears/system/api-gateway/tests/cors_tests.rs @@ -205,6 +205,122 @@ async fn test_cors_disabled() { // Verify router builds without CORS layer } +// Regression guard: the RBAC/AM write contract is ETag-guarded +// (`If-Match`), and browsers may only read non-safelisted response headers +// when the server sends `Access-Control-Expose-Headers`. The gateway used to +// emit no expose-headers at all, so cross-origin JS saw every `ETag` as +// absent and ETag-guarded writes were impossible from the UI. Assert the +// header is present on an actual cross-origin response — with the DEFAULT +// cors config (no explicit `cors` block), which must expose `ETag` out of +// the box. +#[tokio::test] +async fn test_cors_default_exposes_etag_header() { + use tower::ServiceExt as _; + + let api_gateway = api_gateway::ApiGateway::default(); + let ctx = create_test_gear_ctx_permissive_cors(); + api_gateway.init(&ctx).await.expect("Failed to init"); + + let gear = CorsTestGear; + let router = gear + .register_rest(&ctx, Router::new(), &api_gateway) + .expect("Failed to register routes"); + let app = api_gateway + .rest_finalize(&ctx, router) + .expect("Failed to finalize router"); + + let response = app + .oneshot( + http::Request::builder() + .method(http::Method::GET) + .uri("/tests/v1/cors/v1/cors-test") + .header(http::header::ORIGIN, "https://ui.example.com") + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + let exposed = response + .headers() + .get(http::header::ACCESS_CONTROL_EXPOSE_HEADERS) + .expect("Access-Control-Expose-Headers must be present on CORS responses") + .to_str() + .unwrap() + .to_ascii_lowercase(); + assert!( + exposed.split(',').map(str::trim).any(|h| h == "etag"), + "ETag must be CORS-exposed so browser clients can perform \ + ETag-guarded writes, got: {exposed}" + ); +} + +// Same guard for an explicitly configured `cors` block that names its own +// `exposed_headers` list — the configured values must reach the wire. +#[tokio::test] +async fn test_cors_configured_exposed_headers_reach_response() { + use tower::ServiceExt as _; + + let config = wrap_config(&serde_json::json!({ + "bind_addr": "127.0.0.1:0", + "cors_enabled": true, + "cors": { + "allowed_origins": ["https://example.com"], + "allowed_methods": ["GET", "POST"], + "allowed_headers": ["Content-Type"], + "exposed_headers": ["ETag", "X-Request-Id"], + "allow_credentials": true, + "max_age_seconds": 600 + }, + "auth_disabled": true + })); + let hub = Arc::new(toolkit::ClientHub::new()); + let ctx = GearCtx::new( + "api-gateway", + Uuid::new_v4(), + Arc::new(TestConfigProvider { config }), + hub, + tokio_util::sync::CancellationToken::new(), + ); + + let api_gateway = api_gateway::ApiGateway::default(); + api_gateway.init(&ctx).await.expect("Failed to init"); + + let gear = CorsTestGear; + let router = gear + .register_rest(&ctx, Router::new(), &api_gateway) + .expect("Failed to register routes"); + let app = api_gateway + .rest_finalize(&ctx, router) + .expect("Failed to finalize router"); + + let response = app + .oneshot( + http::Request::builder() + .method(http::Method::GET) + .uri("/tests/v1/cors/v1/cors-test") + .header(http::header::ORIGIN, "https://example.com") + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + let exposed = response + .headers() + .get(http::header::ACCESS_CONTROL_EXPOSE_HEADERS) + .expect("Access-Control-Expose-Headers must be present") + .to_str() + .unwrap() + .to_ascii_lowercase(); + for want in ["etag", "x-request-id"] { + assert!( + exposed.split(',').map(str::trim).any(|h| h == want), + "expected {want} in Access-Control-Expose-Headers, got: {exposed}" + ); + } +} + #[tokio::test] async fn test_cors_config_validation() { // Test that CORS config is properly loaded 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/src/infra/storage/odata_mapper.rs b/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs index 44fa329fc..5a36b700e 100644 --- a/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs +++ b/gears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rs @@ -6,6 +6,7 @@ //! This gear maps from DTO-level filter fields to `SeaORM` Column types. use toolkit_db::odata::sea_orm_filter::{FieldToColumn, ODataFieldMapping}; +use toolkit_odata::filter::{FieldKind, FilterField as _}; use crate::infra::storage::entity::gts_type::{ Column as TypeColumn, Entity as TypeEntity, Model as TypeModel, @@ -77,6 +78,18 @@ impl ODataFieldMapping for GroupODataMapper { GroupFilterField::Type => sea_orm::Value::SmallInt(Some(model.gts_type_id)), } } + + /// `type` is a wire string but its cursor value is the storage + /// `SMALLINT` gts ordinal — without this override the cursor codec + /// would try to encode the `SmallInt` under the wire `String` kind + /// and fail with `InvalidCursor` on any `$orderby=type` page + /// overflow (same wire-vs-storage seam as AM's `status`). + fn cursor_kind(field: GroupFilterField) -> FieldKind { + match field { + GroupFilterField::Type => FieldKind::I64, + other => other.kind(), + } + } } /// `OData` mapper for hierarchy queries (not used for `paginate_odata`; hierarchy @@ -103,6 +116,14 @@ impl ODataFieldMapping for HierarchyODataMapper { HierarchyFilterField::Type => sea_orm::Value::SmallInt(Some(model.gts_type_id)), } } + + /// Same wire-string / storage-SMALLINT seam as `GroupODataMapper`. + fn cursor_kind(field: HierarchyFilterField) -> FieldKind { + match field { + HierarchyFilterField::Type => FieldKind::I64, + HierarchyFilterField::HierarchyDepth => field.kind(), + } + } } /// `OData` mapper for memberships. @@ -137,6 +158,14 @@ impl ODataFieldMapping for MembershipODataMapper { } } } + + /// Same wire-string / storage-SMALLINT seam as `GroupODataMapper`. + fn cursor_kind(field: MembershipFilterField) -> FieldKind { + match field { + MembershipFilterField::ResourceType => FieldKind::I64, + other => other.kind(), + } + } } #[cfg(test)] diff --git a/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 ────────────────────────────────────────────────── diff --git a/libs/toolkit-db/src/odata/sea_orm_filter.rs b/libs/toolkit-db/src/odata/sea_orm_filter.rs index 3615e3874..c1e303f74 100644 --- a/libs/toolkit-db/src/odata/sea_orm_filter.rs +++ b/libs/toolkit-db/src/odata/sea_orm_filter.rs @@ -89,14 +89,15 @@ pub trait FieldToColumn { /// Whether `field` is admissible in `$orderby` (and therefore as /// a key in cursor-based pagination). Default `true`. Implementors - /// override with `false` when the wire shape of the field differs - /// from its storage shape — there is no consistent ordering - /// between the two shapes (alphabetical strings vs ordinal - /// integers), and the cursor encoding path relies on - /// `field.kind()` to read tokens back, which would fail to decode - /// a storage-encoded value. The framework checks this in - /// `paginate_odata_collect` before composing the effective order - /// and rejects the `$orderby` clause as + /// override with `false` when the field has no honest ordering at + /// all (e.g. a derived-UUID comparison column). When the field IS + /// meaningfully orderable but its storage shape differs from its + /// wire shape (e.g. a SMALLINT-backed enum exposed as a string), + /// keep it orderable and override + /// [`ODataFieldMapping::cursor_kind`] instead, so the cursor codec + /// round-trips the storage-shaped token. The framework checks this + /// in `paginate_odata_collect` before composing the effective + /// order and rejects the `$orderby` clause as /// [`ODataError::InvalidOrderByField`] — keeping the diagnostic /// at the same site as unknown-field rejections. fn is_orderable(_field: F) -> bool { @@ -144,6 +145,23 @@ pub trait ODataFieldMapping: FieldToColumn { field: F, ) -> sea_orm::Value; + /// The [`FieldKind`] the cursor codec uses to encode/parse this + /// field's cursor token. Defaults to the field's declared wire + /// kind (`field.kind()`). + /// + /// Override for fields whose cursor value (what + /// [`Self::extract_cursor_value`] returns, i.e. the storage shape + /// that `$orderby` actually sorts by) differs from the wire filter + /// shape — e.g. a SMALLINT-backed enum exposed as a string on the + /// API surface but ordered by its storage ordinal: return + /// [`FieldKind::I64`] and extract the natural storage value + /// (`SmallInt` / `Int` / `BigInt` all round-trip under `I64`) so + /// `encode_cursor_value` / `parse_cursor_value` carry the ordinal + /// instead of failing on the wire/storage mismatch. + fn cursor_kind(field: F) -> FieldKind { + field.kind() + } + /// Extract cursor values for all fields in an order. /// /// This is a convenience method that can be overridden for optimization, @@ -392,6 +410,13 @@ pub fn encode_cursor_value(value: &sea_orm::Value, kind: FieldKind) -> Result = match (kind, value) { (FieldKind::String, V::String(Some(s))) => Ok(s.to_string()), (FieldKind::I64, V::BigInt(Some(i))) => Ok(i.to_string()), + // Storage-shaped integer narrower than i64 (e.g. a SMALLINT + // lifecycle ordinal a mapper exposes under `cursor_kind = I64`): + // accept the natural extracted width so mappers need not widen + // by hand. `parse_cursor_value(I64, ..)` yields BigInt, and the + // SQL comparison against the narrow column stays numeric. + (FieldKind::I64, V::SmallInt(Some(i))) => Ok(i.to_string()), + (FieldKind::I64, V::Int(Some(i))) => Ok(i.to_string()), (FieldKind::F64, V::Double(Some(f))) => Ok(ryu::Buffer::new().format(*f).to_owned()), (FieldKind::Bool, V::Bool(Some(b))) => Ok(b.to_string()), (FieldKind::Uuid, V::Uuid(Some(u))) => Ok(u.to_string()), @@ -665,14 +690,12 @@ where }; // Reject `$orderby` keys against fields the mapper has flagged as - // non-orderable (typically: filter columns whose wire shape - // differs from their storage shape — e.g. a SMALLINT-backed enum - // exposed as a string on the API surface). Without this guard the - // cursor codec would round-trip through `field.kind()` and either - // misinterpret the encoded token or hand `SeaORM` a value with - // the wrong SQL type. Tiebreakers go through the same gate so a - // caller cannot smuggle a non-orderable column in via - // `ensure_tiebreaker`. + // non-orderable (fields with no honest ordering at all, e.g. a + // derived-UUID comparison column; wire-vs-storage shape mismatches + // are instead handled by the mapper's `cursor_kind` override, which + // keeps the cursor codec on the storage shape). Tiebreakers go + // through the same gate so a caller cannot smuggle a non-orderable + // column in via `ensure_tiebreaker`. for order_key in &effective_order.0 { let field = F::from_name(&order_key.field) .ok_or_else(|| ODataError::InvalidOrderByField(order_key.field.clone()))?; @@ -837,7 +860,10 @@ where let field = F::from_name(&order_key.field) .ok_or(ODataError::InvalidOrderByField(order_key.field.clone()))?; let column = M::map_field(field); - let kind = field.kind(); + // Cursor tokens carry the storage-shaped value the mapper + // extracted, so decode with the mapper's cursor kind (which may + // differ from the wire filter kind — see `cursor_kind`). + let kind = M::cursor_kind(field); let value = parse_cursor_value(kind, key_str).map_err(|_| ODataError::InvalidCursor)?; cursor_values.push((field, column, value, order_key.dir)); } @@ -892,7 +918,9 @@ where let mut cursor_keys = Vec::new(); for (field, value) in field_values { - let kind = field.kind(); + // Encode with the mapper's cursor kind, matching what + // `extract_cursor_value` produced (see `cursor_kind`). + let kind = M::cursor_kind(field); let key_str = encode_cursor_value(&value, kind).map_err(|_| ODataError::InvalidCursor)?; cursor_keys.push(key_str); } @@ -908,3 +936,36 @@ where d: direction.to_owned(), }) } + +#[cfg(test)] +mod cursor_codec_tests { + use super::*; + + // Pin the storage-shaped integer arms: a mapper exposing a + // SMALLINT/INT column under `cursor_kind = I64` must round-trip its + // natural extracted width without hand-widening to BigInt. + #[test] + fn i64_kind_accepts_narrow_integer_values() { + for value in [ + sea_orm::Value::SmallInt(Some(2)), + sea_orm::Value::Int(Some(2)), + sea_orm::Value::BigInt(Some(2)), + ] { + let token = encode_cursor_value(&value, FieldKind::I64) + .expect("narrow integer must encode under I64"); + assert_eq!(token, "2"); + let parsed = parse_cursor_value(FieldKind::I64, &token).expect("token must parse back"); + assert!(matches!(parsed, sea_orm::Value::BigInt(Some(2)))); + } + } + + // The wire/storage mismatch that motivated `cursor_kind`: a + // SmallInt extracted under the wire String kind still fails loudly + // rather than producing a corrupt token. + #[test] + fn string_kind_rejects_integer_values() { + assert!( + encode_cursor_value(&sea_orm::Value::SmallInt(Some(2)), FieldKind::String).is_err() + ); + } +}