Bug-fix batch: api-gateway CORS expose-headers, AM listing / IdP errors / conversions / child counts, resource-group PEP type#4226
Merged
Artifizer merged 7 commits intoJul 18, 2026
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>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThe changes add typed IdP duplicate and password-policy errors, transactional conversion cancellation during tenant deletion, platform-scoped cascade authorization, storage-aware OData cursors, CORS validation and exposed headers, and the canonical resource-group GTS identifier. ChangesAccount error taxonomy
Tenant deletion lifecycle
OData pagination and resource-group identity
CORS configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gears/system/account-management/account-management-sdk/src/field.rs`:
- Around line 48-53: Add a PasswordPolicy variant to ValidationReason and map it
to and from the PASSWORD_POLICY constant, preserving Unknown handling for
unrecognized values. Extend the existing ValidationReason round-trip test to
verify PASSWORD_POLICY converts to PasswordPolicy and back.
In
`@gears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rs`:
- Around line 501-537: The parent reachability gate and child count in the
affected read method must execute atomically rather than as separate queries.
Replace the preliminary `reachable` query and subsequent `allow_all` count with
one SQL statement that joins or subqueries Respect-scoped parents while counting
their direct children, preserving the existing exclusions and empty-result
behavior.
- Around line 458-480: Update the public child_count contract in
account-management-v1.yaml to document the parent-gated direct-child carve-out:
count self_managed direct children when their parent is Respect-reachable, while
excluding descendants behind an impenetrable self-managed barrier. Replace the
outdated statement that such direct children are never counted, and keep the
existing behavior for non-Respect-reachable parents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93facb16-3700-4a4b-9405-33e45d73ea20
📒 Files selected for processing (36)
gears/system/account-management/account-management-sdk/src/field.rsgears/system/account-management/account-management-sdk/src/idp_user.rsgears/system/account-management/account-management-sdk/src/lib.rsgears/system/account-management/account-management-sdk/src/tenant.rsgears/system/account-management/account-management/src/domain/error.rsgears/system/account-management/account-management/src/domain/error_tests.rsgears/system/account-management/account-management/src/domain/idp/idp_tests.rsgears/system/account-management/account-management/src/domain/idp/mod.rsgears/system/account-management/account-management/src/domain/system_actor.rsgears/system/account-management/account-management/src/domain/tenant/repo.rsgears/system/account-management/account-management/src/domain/tenant/service/mod.rsgears/system/account-management/account-management/src/domain/tenant/service/service_tests.rsgears/system/account-management/account-management/src/domain/tenant/test_support/repo.rsgears/system/account-management/account-management/src/infra/sdk_error_mapping.rsgears/system/account-management/account-management/src/infra/sdk_error_mapping_tests.rsgears/system/account-management/account-management/src/infra/storage/repo_impl/conversion.rsgears/system/account-management/account-management/src/infra/storage/repo_impl/mod.rsgears/system/account-management/account-management/src/infra/storage/repo_impl/reads.rsgears/system/account-management/account-management/src/infra/storage/repo_impl/updates.rsgears/system/account-management/account-management/tests/conversion_integration.rsgears/system/account-management/account-management/tests/lifecycle_integration.rsgears/system/account-management/account-management/tests/list_children_integration.rsgears/system/account-management/account-management/tests/metadata_integration.rsgears/system/account-management/account-management/tests/metadata_integration_pg.rsgears/system/account-management/account-management/tests/updates_integration.rsgears/system/account-management/docs/account-management-v1.yamlgears/system/api-gateway/src/config.rsgears/system/api-gateway/src/cors.rsgears/system/api-gateway/src/gear.rsgears/system/api-gateway/tests/cors_tests.rsgears/system/resource-group/resource-group-sdk/src/error_tests.rsgears/system/resource-group/resource-group-sdk/src/gts.rsgears/system/resource-group/resource-group/src/api/rest/error.rsgears/system/resource-group/resource-group/src/infra/storage/odata_mapper.rsgears/system/resource-group/resource-group/tests/domain_unit_test.rslibs/toolkit-db/src/odata/sea_orm_filter.rs
… 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>
…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>
diffora
force-pushed
the
fix/am-gateway-rg-batch
branch
from
July 18, 2026 09:20
3f105e2 to
f003271
Compare
Contributor
Author
|
All three review findings addressed and folded into their originating commits (history stays one-commit-per-fix):
Tests, fmt, clippy green on the touched crates; branch force-pushed. |
Artifizer
approved these changes
Jul 18, 2026
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
2 participants
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.
A batch of independent bug fixes, one commit per fix, spanning api-gateway, account-management (+SDK), resource-group, and toolkit-db.
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) write was impossible for a cross-origin browser client while curl/Postman hid the problem. Addscors.exposed_headers(default["ETag"], so deployments need no config change), config-load validation for wildcard+credentials combinations (previously a tower-http assert panic at startup — a crash-loop with no pointer at the offending config), and warnings for unparseable CORS list entries instead of a silentfilter_mapdrop.$orderby=statuson the tenant children listing (account-management + toolkit-db + resource-group)Sorting the tenant children listing by status returned 400
unsupported $orderby field. Root cause: the cursor codec was bound to the wireStringkind while the cursor value carries the storageSMALLINTordinal, so the token could not round-trip. AddsODataFieldMapping::cursor_kind(default = the wire kind, so no existing mapper changes behavior) and uses it on both sides of the cursor codec;encode_cursor_valuealso acceptsSmallInt/IntunderFieldKind::I64so mappers need not hand-widen narrow columns.statusbecomes orderable by its lifecycle ordinal (active<suspended<deleted). resource-group's three type fields carried the same latent wire-String/storage-SmallInt mismatch (orderable, un-overridden —$orderby=typebroke withInvalidCursoron any page overflow) and now declarecursor_kind = I64. OpenAPI updated accordingly.Typed IdP uniqueness / password-policy rejections (account-management SDK + gear)
POST /tenants/{id}/userswith a duplicate username surfaced as a redacted generic 400 (request/VALIDATION), so clients could not attribute the failure to a field; duplicate email and password-policy rejections collapsed into the same catch-all. The#[non_exhaustive]IdpUserOperationFailuregainsDuplicateUser { field: Username | Email | UsernameOrEmail }andPasswordPolicy;Rejectedstays as the fallback, so providers that never emit the new variants keep today's behavior unchanged. AM mapsDuplicateUserto 409already_existson the user resource (the stable colliding-field token asresource_name, never the caller-supplied value; the curated detail derives from the same typed field) andPasswordPolicyto 400 with apassword/PASSWORD_POLICYfield violation.UsernameOrEmailcovers Keycloak's combinedModelDuplicateExceptionconstant — on KC 26 the only 409 shapecreateUserproduces directly. Raw provider text stays digest-only in logs, same redaction posture as the existing arms. The IdP-plugin half that actually emits the classified variants lives in the consuming application; until it lands, every path still producesRejectedand nothing changes on the wire.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= the deleting actor); the idempotent already-Deleted path runs the same fenced cancel, so pre-fix orphans are healed by simply retrying the DELETE. The audit event is emitted after commit (no double-count on serialization retry, no phantom event on rollback).Platform-scope the user-groups cascade system actor (account-management)
The retention hard-delete reaper's resource-group cascade ran scoped to the tenant being deleted; the resolver's subtree grant rooted at a Deleted tenant materializes to zero rows and fails closed, silently deferring the whole hard-delete backlog forever. The cascade context is now platform-scoped (nil home tenant resolves to Global at the resolver). The resolver-side half lives in the consuming application.
Count self_managed direct children in
child_count(account-management)The public
child_countread-shape field 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 dropped out of the count while the listing (direct-child carve-out) still showed it. The count now gates on the PARENT's Respect-reachability and then counts all its direct children; theparent_idpredicate pins the count 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 could not catch this).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~. Deployed role grants are 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 on a live deployment: a grant ongts.cf.core.rg.*is denied whilegts.cf.core.resource_group.*is allowed, same caller and flow. PinsGROUP_RESOURCE_TYPEback tocf.core.rg.group.v1~, consistent with the untouched siblings (rg.group_membership.v1~,rg.type.v1~), with a comment so a future mechanical migration does not rename it again.Verification
Branch is rebased onto current
main.cargo test/cargo clippy --all-targets/cargo fmt --checkare green across the six touched crates (toolkit-db, api-gateway, account-management, account-management-sdk, resource-group, resource-group-sdk);cargo dylint --all(incl. the GTS-layer lints DE0901/DE0904) green on the resource-group crates. The AM, gateway, and resource-group fixes were additionally exercised end-to-end against a live deployment (REST-level checks for the CORS header,$orderby=statusincl. a cursor walk across ordinal groups, the duplicate/password-policy classification, the delete-cancels-conversion flow, and the self_managedchild_countagreement between the single read and the listing).Summary by CodeRabbit
New Features
Bug Fixes
Documentation