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, and cover both the default and an explicitly configured list with request-level tests. 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': TenantODataMapper::is_orderable
rejected the field because the cursor codec was hard-wired to the wire
filter kind (String) while the extracted cursor value spoke the storage
shape (SMALLINT) - encode/parse could not round-trip the token.
Give ODataFieldMapping a per-field cursor_kind() (default: the wire
field.kind(), so every existing mapper is untouched) and use it on both
sides of the cursor codec. The AM mapper overrides Status -> I64 and
extracts the ordinal as BigInt, so status becomes orderable by its
lifecycle ordinal (active < suspended < deleted) with cursor pages
comparing integer-to-SMALLINT - a numeric SQL comparison.
tenant_type stays non-orderable (ordering by a derived UUIDv5 has no
honest meaning). Filter comparisons on status stay membership-only
(eq/ne/in). Integration tests replace the old rejection pin with a
lifecycle-ordinal sort assertion and a full cursor walk across ordinal
groups.
Signed-off-by: Diffora <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 PRD 'Add user dialogs - per-field and API error handling'
acceptance criterion. Duplicate email and password-policy rejections
collapsed into the same catch-all.
SDK: extend the #[non_exhaustive] IdpUserOperationFailure with typed
variants - DuplicateUser { field: IdpUserDuplicateField (Username |
Email), detail } and PasswordPolicy { detail }; Rejected stays as the
fallback for unattributable rejections, so legacy plugins keep today's
behavior unchanged. New field/reason vocabulary: PASSWORD_FIELD,
PASSWORD_POLICY.
AM: map DuplicateUser to the new DomainError::UserAlreadyExists ->
HTTP 409 already_exists on the user resource type, with the stable
colliding-field token ('username' / 'email') as resource_name (never
the caller-supplied value); map PasswordPolicy to
DomainError::IdpPasswordPolicy -> HTTP 400 with field_violations
[{field: password, reason: PASSWORD_POLICY}] instead of the generic
request/VALIDATION pair. Raw provider text stays digest-only in the
am.idp log lines, same redaction posture as the existing arms.
The vp-idp-plugin change that actually emits the classified variants
from Keycloak 409/400 responses lands separately in vhp-core after the
submodule bump - until then every path still produces Rejected and
nothing changes on the wire.
Signed-off-by: Diffora <ddiffora@gmail.com>
…-delete TX
DELETE /tenants/{id} on a tenant with an open child-conversion request
returned 204 and tombstoned the tenant, but the conversion row stayed
'pending' with tenant_id pointing at the tombstone - breaking the
invariant that every pending conversion references a live tenant, and
leaving LIST /child-conversions dereferencing a dead FK.
schedule_deletion's SERIALIZABLE TX now terminal-resolves the request
right after the status flip + closure rewrite: a guarded UPDATE
(status = Pending AND deleted_at IS NULL) moves it to Cancelled with
cancelled_by = the deleting actor, resolved_at = the deletion
timestamp, and a fixed 'subject tenant deleted' comment. The helper
lives with the conversion repo (cancel_pending_on_tenant_delete_tx) so
ownership of the conversion_requests transition shape stays in one
place; the tenant TX just invokes it. The Pending fence keeps
idempotent delete retries from re-stamping an already-resolved row,
and the already-Deleted short-circuit returns before the cascade
anyway. Only rows keyed by the deleted tenant can exist here: a parent
with non-deleted children is rejected by TenantHasChildren before the
TX.
schedule_deletion gains a deleted_by actor parameter (trait, impl,
in-memory fake, all call sites); the service passes ctx.subject_id().
An am.events line (conversion_auto_cancelled_on_tenant_delete) fires
when the cascade touches a row.
Integration tests: cascade resolves the row in the same TX (row is
resolved, not removed; no pending row survives for the tombstone), an
idempotent retry with a different actor/timestamp preserves the first
resolution verbatim, and a sibling tenant's pending conversion is
untouched.
Signed-off-by: Diffora <ddiffora@gmail.com>
…ting/conversion fixes Fixes from the branch code review, batched: api-gateway: - validate_cors_config at gear init rejects any wildcard CORS list combined with allow_credentials=true. tower-http asserts on exactly this inside Layer::layer, and axum's eager router layering turned it into a startup crash-loop with no pointer at the config; now it is a clean init error naming the offending list. - Unparseable entries in ANY cors list warn and are skipped instead of vanishing in a silent filter_map: a typo'd exposed_headers entry used to silently suppress Access-Control-Expose-Headers and re-break every ETag-guarded browser write. Shared parse_list helper replaces the 4th copy of the parse pattern. account-management SDK+AM: - IdpUserDuplicateField gains UsernameOrEmail: Keycloak's ModelDuplicateException path emits the combined constant 'User exists with same username or email' (on KC 26 it is the only 409 createUser produces directly), which must not be misattributed to username. - DomainError::UserAlreadyExists now carries the typed field; the canonical boundary derives both resource_name and the curated detail from it, collapsing three copies of the wording into one. account-management: - The conversion auto-cancel am.events line is emitted AFTER the SERIALIZABLE TX commits (count rides the closure return): logging inside the closure double-counted on a 40001 retry and emitted a phantom event when the TX ultimately rolled back. - The already-Deleted idempotent short-circuit now also runs the (Pending-fenced) cancel cascade, so tenants soft-deleted BEFORE the cascade existed are healed by the natural operator remediation - retrying the DELETE; post-fix tombstones make it a no-op. - OpenAPI account-management-v1.yaml: $orderby=status on the tenant children listing is now documented as supported with lifecycle-ordinal semantics (the old text still declared it rejected with 400). toolkit-db + resource-group (follow-through): - encode_cursor_value accepts SmallInt/Int under FieldKind::I64, so mappers exposing narrow integer columns need not hand-widen to BigInt (AM's status extract reverts to the natural SmallInt). - resource-group's three type fields (Group/Hierarchy/Membership) had the exact wire-String/storage-SmallInt mismatch the cursor_kind seam governs, orderable and un-overridden - $orderby=type broke with InvalidCursor on any page overflow; they now declare cursor_kind=I64. - Codec unit tests pin the narrow-integer round-trip and the loud failure for a genuine wire/storage mismatch. Signed-off-by: Diffora <ddiffora@gmail.com>
…m actor The retention hard-delete reaper could never purge a tenant: its RG cascade hook ran under a system-actor context scoped to the very tenant being hard-deleted. The authz resolver's system-actor grant roots a tenant-subtree scope at the subject's home tenant - a Deleted root is clamped out of scope materialization, the subtree resolves to zero tenants, and every cascade call fails closed, so each due tenant was deferred forever (observed on stage1: processed=64 cleaned=0 deferred=64 on every tick). Make for_user_groups_cascade platform-scoped (nil tenant -> Global grant at the resolver): honest for this flow, which addresses explicit group ids of a tenant being erased, not a live tenant's data. The deleted tenant id stays on the am.system_actor audit line. The resolver-side halves (empty-token-scopes bypass for the unforgeable system actor + Global on nil home tenant) land in vp-core's vp-authz-resolver-plugin. Signed-off-by: Diffora <ddiffora@gmail.com>
|
Warning Review limit reached
Next review available in: 17 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 Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds structured IdP failure types and canonical mappings, cascades tenant deletion to pending conversions, supports storage-shaped OData cursors for status ordering, and adds configurable CORS response-header exposure with startup validation. ChangesStructured IdP error handling
Tenant deletion cascades
Storage-shaped OData cursors
CORS header exposure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant IdP
participant UserOperationFailureExt
participant sdk_error_mapping
IdP->>UserOperationFailureExt: DuplicateUser or PasswordPolicy
UserOperationFailureExt->>sdk_error_mapping: DomainError
sdk_error_mapping-->>IdP: CanonicalError envelope
Suggested reviewers: 🚥 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 |
Fold the reasoning previously carried by internal tracker references directly into the comments so they stand on their own. Signed-off-by: Diffora <ddiffora@gmail.com>
diffora
changed the title
diffora
marked this pull request as ready for review
July 17, 2026 16:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/list_children_integration.rs`:
- Around line 511-533: The cursor pagination loop in the integration test must
be bounded so repeated next_cursor values cannot hang indefinitely. Update the
loop around cursor and page.page_info.next_cursor to track previously seen
cursors or enforce a maximum page count, and fail with a clear assertion when
the bound is exceeded while preserving normal pagination and collection
behavior.
In `@gears/system/account-management/docs/account-management-v1.yaml`:
- Around line 761-764: Update the wire/storage divergence description in the
account-management API documentation to state that filters and response bodies
both use public enum strings, while the divergence is between the wire contract
and SMALLINT database storage. Remove the claim that filters differ from the
response shape.
🪄 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 Plus
Run ID: bbd823b2-67f5-4796-b67c-025364617de0
📒 Files selected for processing (32)
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/src/infra/storage/odata_mapper.rslibs/toolkit-db/src/odata/sea_orm_filter.rs
Comment on lines
+511
to
+533
| let mut collected = Vec::new(); | ||
| let mut cursor: Option<String> = 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the cursor walk to prevent a hanging regression test.
A repeated next_cursor causes this loop to run indefinitely instead of failing with a useful assertion. Track seen cursors or cap the number of pages.
Proposed fix
+ let mut seen_cursors = std::collections::HashSet::new();
let mut collected = Vec::new();
let mut cursor: Option<String> = None;
loop {
let mut q = ODataQuery::default().with_limit(2);
if let Some(c) = cursor.take() {
+ assert!(seen_cursors.insert(c.clone()), "pagination returned a repeated cursor");
q = q.with_cursor(CursorV1::decode(&c).expect("decode status cursor"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut collected = Vec::new(); | |
| let mut cursor: Option<String> = 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, | |
| } | |
| } | |
| let mut seen_cursors = std::collections::HashSet::new(); | |
| let mut collected = Vec::new(); | |
| let mut cursor: Option<String> = None; | |
| loop { | |
| let mut q = ODataQuery::default().with_limit(2); | |
| if let Some(c) = cursor.take() { | |
| assert!(seen_cursors.insert(c.clone()), "pagination returned a repeated cursor"); | |
| 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, | |
| } | |
| } |
🤖 Prompt for 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.
In
`@gears/system/account-management/account-management/tests/list_children_integration.rs`
around lines 511 - 533, The cursor pagination loop in the integration test must
be bounded so repeated next_cursor values cannot hang indefinitely. Update the
loop around cursor and page.page_info.next_cursor to track previously seen
cursors or enforce a maximum page count, and fail with a clear assertion when
the bound is exceeded while preserving normal pagination and collection
behavior.
Comment on lines
+761
to
+764
| 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the wire/storage divergence description.
Filters and responses both use public enum strings; the divergence is between the wire contract and SMALLINT storage, not between filters and responses.
Proposed fix
- 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.
+ it. Filters and response bodies use the same public string
+ values (`"pending"`, `"active"`, etc.); only the internal
+ storage representation uses numeric ordinals.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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. | |
| it. Filters and response bodies use the same public string | |
| values (`"pending"`, `"active"`, etc.); only the internal | |
| storage representation uses numeric ordinals. |
🤖 Prompt for 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.
In `@gears/system/account-management/docs/account-management-v1.yaml` around lines
761 - 764, Update the wire/storage divergence description in the
account-management API documentation to state that filters and response bodies
both use public enum strings, while the divergence is between the wire contract
and SMALLINT database storage. Remove the claim that filters differ from the
response shape.
… evaluation The gts-rust v0.11.0 migration mechanically replaced the group gate's resource-type literal with the crate-name-derived SDK constant, which silently renamed the PEP-evaluated type from gts.cf.core.rg.group.v1~ to gts.cf.core.resource_group.group.v1~. Every deployed role grant is written against the documented gts.cf.core.rg.* family, so the rename 403'd all group CRUD for tenant members and failed AM's tenant-delete ownership probe closed (surfaced as 503) - verified live: a grant on gts.cf.core.rg.* is denied while gts.cf.core.resource_group.* is allowed, on the same caller and flow. Pin GROUP_RESOURCE_TYPE back to cf.core.rg.group.v1~, consistent with the untouched siblings (rg.group_membership.v1~, rg.type.v1~), and realign the #[resource_error] tags + tests that the SDK round-trip tests pin to the constant. Signed-off-by: Diffora <ddiffora@gmail.com>
Owner
Author
|
Superseded by #10 (clean history + branch name, CodeRabbit's two comments folded in). Closing this one; the |
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 ticket plus a post-review remediation commit.
VHP-2193 — 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.VHP-2084 —
$orderby=statuson the tenant children listing (AM + 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.VHP-2158 — typed IdP uniqueness / password-policy rejections (AM SDK + AM)
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 vhp-core.VHP-1729 — soft-delete cancels the pending conversion in-TX (AM)
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. Audit event is emitted after commit (no double-count on serialization retry).VHP-2376 — platform-scope the user-groups cascade system actor (AM)
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); resolver-side halves land in vhp-core (vp-authz-resolver-plugin).
Verification
cargo test/clippy --all-targets/fmt --checkgreen on the five touched crates (toolkit-db, api-gateway, account-management, account-management-sdk, resource-group): 1789+ tests. An 8-angle review with verifier passes ran over the branch; all confirmed findings are remediated in the final commit (KC combined-constant handling verified against Keycloak 20/26 sources).Coupling
vhp-core consumes this via the submodule pin: its VHP-2158 branch (vp-idp-plugin classification + authz-resolver fix) needs the pin bumped to this branch's head after merge.
Summary by CodeRabbit
ETagby default.