26.7 release-bug batch: api-gateway CORS, AM listing / IdP errors / conversions, authz system-actor#10
Closed
diffora wants to merge 7 commits into
Closed
26.7 release-bug batch: api-gateway CORS, AM listing / IdP errors / conversions, authz system-actor#10diffora wants to merge 7 commits into
diffora wants to merge 7 commits into
Conversation
The RBAC/AM write contract is ETag-guarded: DELETE/PATCH on role assignments and role definitions require the resource's current ETag in If-Match, and the only way to obtain it is to read the ETag header from a GET/POST response. Browsers may only read non-safelisted response headers when the server sends Access-Control-Expose-Headers, and the gateway sent none at all - so every cross-origin browser client saw ETag as absent and every ETag-guarded write dead-ended, while curl/Postman hid the problem. Add CorsConfig.exposed_headers (wired to CorsLayer::expose_headers, "*" supported like the sibling lists) defaulting to ["ETag"] so deployments get the fix without a config change. Also harden CORS config handling, both surfaced while reviewing the fix: - validate_cors_config at gear init rejects any wildcard CORS list combined with allow_credentials=true. tower-http asserts on exactly this inside Layer::layer, and axum's eager router layering turned it into a startup crash-loop with no pointer at the config; now it is a clean init error naming the offending list. - Unparseable entries in ANY cors list warn and are skipped instead of vanishing in a silent filter_map: a typo'd exposed_headers entry used to silently suppress Access-Control-Expose-Headers and re-break every ETag-guarded browser write. A shared parse_list helper replaces the four copies of the parse pattern. Covers the default and an explicitly configured list with request-level tests, plus unit tests for each validation arm. Signed-off-by: Diffora <ddiffora@gmail.com>
…ren listing
Clicking the Status column on the Tenants page issued
GET /tenants/{id}/children?$orderby=status and got 400
'unsupported $orderby field: status': the field was flagged
non-orderable because the cursor codec was hard-wired to the wire
filter kind (String) while the extracted cursor value speaks the
storage shape (SMALLINT) - encode/parse could not round-trip the token.
Give ODataFieldMapping a per-field cursor_kind() (default: the wire
field.kind(), so every existing mapper is untouched) and use it on both
sides of the cursor codec. encode_cursor_value also accepts SmallInt/Int
under FieldKind::I64 so mappers exposing narrow integer columns need not
hand-widen to BigInt. The AM mapper overrides Status -> I64 and extracts
the storage ordinal, so status becomes orderable by its lifecycle ordinal
(active < suspended < deleted) with cursor pages comparing integer to
SMALLINT - a numeric SQL comparison.
tenant_type stays non-orderable (ordering by a derived UUIDv5 has no
honest meaning). Filter comparisons on status stay membership-only
(eq/ne/in).
resource-group carried the same latent wire-String/storage-SMALLINT
mismatch on its three type fields (Group/Hierarchy/Membership), orderable
and un-overridden - $orderby=type broke with InvalidCursor on any page
overflow; they now declare cursor_kind=I64.
Integration tests replace the old rejection pin with a lifecycle-ordinal
sort assertion and a full cursor walk across ordinal groups (bounded
against a repeated-cursor hang). Codec unit tests pin the narrow-integer
round-trip and the loud failure for a genuine wire/storage mismatch.
OpenAPI documents $orderby=status as supported with lifecycle-ordinal
semantics.
Signed-off-by: Diffora <ddiffora@gmail.com>
… rejections
POST /tenants/{id}/users with a duplicate username came back as the
redacted generic 400 invalid_argument (field=request, reason=VALIDATION,
'identity provider rejected the user operation (detail redacted; ...)').
The client could not attribute the failure to the username field, which
blocks the 'Add user dialogs - per-field and API error handling'
acceptance criterion. Duplicate email and password-policy rejections
collapsed into the same catch-all.
SDK: extend the #[non_exhaustive] IdpUserOperationFailure with typed
variants - DuplicateUser { field: IdpUserDuplicateField (Username |
Email | UsernameOrEmail), detail } and PasswordPolicy { detail };
Rejected stays as the fallback for unattributable rejections, so legacy
plugins keep today's behavior unchanged. UsernameOrEmail covers
Keycloak's ModelDuplicateException path, which emits the combined
constant 'User exists with same username or email' (on KC 26 the only
409 createUser produces directly) and must not be misattributed to
username. New field/reason vocabulary: PASSWORD_FIELD, PASSWORD_POLICY.
AM: map DuplicateUser to DomainError::UserAlreadyExists -> HTTP 409
already_exists on the user resource type, carrying the typed field so
the canonical boundary derives both resource_name (the stable colliding
-field token 'username'/'email', never the caller-supplied value) and
the curated detail from one place; map PasswordPolicy to
DomainError::IdpPasswordPolicy -> HTTP 400 with field_violations
[{field: password, reason: PASSWORD_POLICY}] instead of the generic
request/VALIDATION pair. Raw provider text stays digest-only in the
am.idp log lines, same redaction posture as the existing arms.
The vp-idp-plugin change that actually emits the classified variants
from Keycloak 409/400 responses lands separately after the submodule
bump - until then every path still produces Rejected and nothing
changes on the wire.
Signed-off-by: Diffora <ddiffora@gmail.com>
…-delete TX
DELETE /tenants/{id} tombstoned the tenant but left any pending
mode-conversion request row dangling. schedule_deletion's SERIALIZABLE
TX now terminal-resolves the pending conversion (cancelled_by = the
deleting actor) inside the same transaction, so a committed soft-delete
never leaves an orphaned conversion behind.
The already-Deleted idempotent short-circuit also runs the same
(Pending-fenced) cancel cascade, so tenants soft-deleted BEFORE the
cascade existed are healed by the natural operator remediation -
retrying the DELETE; post-fix tombstones make it a no-op.
The conversion auto-cancel am.events line is emitted AFTER the
SERIALIZABLE TX commits (the count rides the closure return): logging
inside the closure would double-count on a 40001 retry and emit a
phantom event when the TX ultimately rolled back.
Signed-off-by: Diffora <ddiffora@gmail.com>
…m actor The retention hard-delete reaper's resource-group cascade ran scoped to the tenant being deleted. The resolver's subtree grant is rooted at that tenant, but the tenant row is already Deleted, so the grant materializes to zero rows and the PDP fails closed - the cascade is denied and the whole hard-delete backlog is deferred forever, silently. Platform-scope the cascade's system-actor context (a nil home tenant resolves to Global at the resolver) so the reaper can clean up resource-group state for tenants it is tearing down. The resolver-side halves land separately after the submodule bump. Signed-off-by: Diffora <ddiffora@gmail.com>
… evaluation The gts-rust v0.11.0 migration routed the group resource-type literal through the crate-name-derived SDK constant, which silently renamed the PEP-evaluated type from gts.cf.core.rg.group.v1~ to gts.cf.core.resource_group.group.v1~. Every deployed role grant is written against the documented gts.cf.core.rg.* family, so the rename 403'd all group CRUD for tenant members and failed AM's tenant-delete ownership probe closed (surfaced as 503) - verified live: a grant on gts.cf.core.rg.* is denied while gts.cf.core.resource_group.* is allowed, on the same caller and flow. Pin GROUP_RESOURCE_TYPE back to cf.core.rg.group.v1~, consistent with the untouched siblings (rg.group_membership.v1~, rg.type.v1~), and realign the #[resource_error] tags + tests that the SDK round-trip tests pin to the constant. A code comment marks the literal as the PEP-evaluated type that must NOT be crate-derived, so a future mechanical migration does not silently rename it again. Adopting crate-derived namespaces intentionally would be a separate, coordinated migration: the whole type family at once, plus installer seeds and a data migration of existing grants. Signed-off-by: Diffora <ddiffora@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…count
The scope-selector counter on the Tenants page (the public `child_count`
read-shape field) undercounted a parent whose direct children include a
`self_managed` one: a parent with two active children where one is
self_managed reported `child_count = 1`, while `GET /tenants/{id}/children`
correctly listed both. Operator UIs render the wrong direct-child count.
Root cause: `count_children_grouped` clamped each child row by the
caller's barrier-respecting scope (`id IN (closure WHERE ancestor = root
AND barrier = 0)`). A self_managed tenant's inbound closure edges carry
`barrier = 1` even from its own parent, so a Respect-reachable parent's
self_managed direct child was dropped from the count — even though the
identity-level direct-child carve-out (see `service::scope_util`) makes
it visible through `list_children`. The two read paths disagreed.
Gate the count on the PARENT's Respect-reachability instead of each
child's own barrier-clamped visibility: resolve which requested parents
the caller can Respect-reach, then count all their direct children
(the `parent_id` predicate pins the count to depth 1, so nothing below a
barrier leaks — a parent reachable only via the carve-out, i.e. a
self_managed tenant past its barrier, is absent from the Respect read
and its subtree count stays 0). `Provisioning` excluded, `Deleted`
included, as before.
Regression tests exercise a real Respect-barriers `InTenantSubtree`
scope (the previous coverage ran under `allow_all`, with barriers off,
and could not catch this): one asserts a reachable parent's count
includes its self_managed direct child, one asserts a past-barrier
parent's subtree stays uncounted.
Signed-off-by: Diffora <ddiffora@gmail.com>
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Superseded by the upstream submission constructorfabric#4226 (same fixes, rebased onto current upstream main; CodeRabbit's two findings from #10 are folded in). Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Batch of 26.7 release-bug fixes, one commit per fix.
CORS-expose the ETag response header (api-gateway)
Browsers may only read non-safelisted response headers when the server sends
Access-Control-Expose-Headers; the gateway sent none, so every ETag-guarded (If-Match) RBAC/AM write was impossible cross-origin. Addscors.exposed_headers(default["ETag"]), config-load validation for wildcard+credentials combos (previously a tower-http startup panic), and warnings for unparseable list entries.$orderby=statuson the tenant children listing (account-management + toolkit-db + resource-group)Status-column sorting returned 400. Root cause: the cursor codec was bound to the wire
Stringkind while the cursor value is the storageSMALLINT. AddsODataFieldMapping::cursor_kind(default = wire kind; no existing mapper affected), enables lifecycle-ordinal ordering forstatus, sweeps resource-group's three type fields carrying the same latent mismatch, and updates the OpenAPI doc.Typed IdP uniqueness / password-policy rejections (account-management SDK + gear)
IdpUserOperationFailuregainsDuplicateUser { Username | Email | UsernameOrEmail }andPasswordPolicy; AM maps them to 409already_existson the user resource (stable colliding-field token asresource_name, curated detail derived in one place) and 400 with apassword/PASSWORD_POLICYfield violation.UsernameOrEmailcovers Keycloak's combinedModelDuplicateExceptionconstant (the only direct 409 shape on KC 26). The plugin half that emits these lands in the platform repo.Soft-delete cancels the pending conversion in-TX (account-management)
DELETE /tenants/{id}tombstoned the tenant but left its pending mode-conversion row dangling.schedule_deletion's SERIALIZABLE TX now terminal-resolves it (cancelled_by = deleting actor); the idempotent already-Deleted path runs the same fenced cancel, so pre-fix orphans are healed by retrying the DELETE. The audit event is emitted after commit (no double-count on serialization retry).Platform-scope the user-groups cascade system actor (account-management)
The retention hard-delete reaper's RG cascade ran scoped to the tenant being deleted; the resolver's subtree grant rooted at a Deleted tenant materializes to zero and fails closed, deferring the whole backlog forever. The cascade context is now platform-scoped (nil tenant → Global at the resolver); the resolver-side halves land in the platform repo.
Count self_managed direct children in
child_count(account-management)The scope-selector counter (
child_count) undercounted a parent whose direct children include aself_managedone — a parent with two active children where one is self_managed reportedchild_count = 1whileGET /tenants/{id}/childrenlisted both. Root cause:count_children_groupedclamped each child row by the caller's barrier-respecting scope, and a self_managed tenant's inbound closure edges carrybarrier = 1even from its own parent, so the self_managed direct child was dropped from the count (while the listing still showed it — the direct-child carve-out). Fix gates the count on the PARENT's Respect-reachability, then counts all its direct children (theparent_idpredicate pins it to depth 1, so nothing below a barrier leaks). Regression tests run under a real Respect-barriers scope (prior coverage ran underallow_all, barriers off, and missed it).Restore rg-namespace group resource type for PEP evaluation (resource-group)
The gts-rust v0.11.0 migration routed the group resource-type literal through the crate-name-derived SDK constant, silently renaming the PEP-evaluated type from
gts.cf.core.rg.group.v1~togts.cf.core.resource_group.group.v1~. Every deployed role grant is written against the documentedgts.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. PinsGROUP_RESOURCE_TYPEback tocf.core.rg.group.v1~, consistent with the untouched siblings, with a comment so a future mechanical migration will not rename it again.Verification
cargo test/clippy --all-targets/fmt --checkgreen on the touched crates (toolkit-db, api-gateway, account-management, account-management-sdk, resource-group).cargo dylint --all(incl. the GTS-layer lints DE0901/DE0904) green on the two resource-group crates.Coupling
The platform repo consumes this via its submodule pin; the pin needs bumping to this branch's head after merge (the plugin-side IdP classification and the authz-resolver cascade fix depend on the SDK/AM changes here).