feat: Add casbin as AuthZ provider option#908
Merged
ayushjain17 merged 9 commits intomainfrom Mar 11, 2026
Merged
Conversation
Contributor
WalkthroughIntroduces a Casbin-based authorization system: adds policy engine, DB schema and bootstrap, HTTP endpoints, derive-macro resource declarations and action registry, workspace/org/admin routing, frontend UI/components for policy management, and makes workspace_admin_email optional across APIs and SDKs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Frontend
participant AuthZAPI as AuthZ API (/casbin)
participant CasbinEngine
participant Database
Client->>Frontend: Open AuthzEditor
Frontend->>AuthZAPI: GET /policy (scope)
AuthZAPI->>CasbinEngine: enforcer.list_filtered(domain)
CasbinEngine->>Database: SELECT casbin_rule...
Database-->>CasbinEngine: policy rows
CasbinEngine-->>AuthZAPI: policies
AuthZAPI-->>Frontend: policies
Frontend->>AuthZAPI: POST /policy (add)
AuthZAPI->>CasbinEngine: enforcer.add_policy(...)
CasbinEngine->>Database: INSERT casbin_rule
CasbinEngine-->>AuthZAPI: ok
AuthZAPI-->>Frontend: 200 OK
sequenceDiagram
participant Client
participant WorkspaceCtx as WorkspaceContext MW
participant AuthZ as AuthZ MW
participant CasbinEngine
participant Handler
Client->>WorkspaceCtx: request /config (with workspace)
WorkspaceCtx->>WorkspaceCtx: compute SchemaName + AuthZDomain
WorkspaceCtx-->>AuthZ: request (extensions contain domain)
AuthZ->>CasbinEngine: is_allowed(domain, user, resource, action)
CasbinEngine-->>AuthZ: allowed? (true/false)
alt allowed
AuthZ-->>Handler: call handler
Handler-->>Client: 200 OK
else denied
AuthZ-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/superposition_types/src/api/workspace.rs (1)
75-89:⚠️ Potential issue | 🟠 MajorPotential database constraint violation with
Option<T>fields andAsChangeset.When fields like
workspace_admin_email,workspace_status, orallow_experiment_self_approvalareNone(e.g., client omits the field), Diesel'sAsChangesetdefault behavior generatesSET column = NULL. However, the database schema defines these as NOT NULL columns, causing runtime constraint violations.To skip updating columns when the value is
None, add thetreat_none_as_nullattribute:#[derive(Debug, Deserialize, Serialize)] #[cfg_attr(feature = "diesel_derives", derive(AsChangeset))] +#[cfg_attr(feature = "diesel_derives", diesel(treat_none_as_null = false))] #[cfg_attr(feature = "diesel_derives", diesel(table_name = workspaces))] pub struct UpdateWorkspaceRequest {Additionally,
auto_populate_controlshould include#[serde(default = "default_true")]to default totruewhen omitted from the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/api/workspace.rs` around lines 75 - 89, The UpdateWorkspaceRequest AsChangeset will set columns to NULL when Option<T> fields are None which conflicts with NOT NULL DB columns; add #[diesel(treat_none_as_null = "false")] (or the appropriate treat_none_as_null attribute) to the struct/fields so None values are skipped instead of generating SET column = NULL for fields like workspace_admin_email, workspace_status, allow_experiment_self_approval, auto_populate_control, enable_context_validation, and enable_change_reason_validation, and add a serde default for auto_populate_control (e.g., #[serde(default = "default_true")] on auto_populate_control plus a default_true() -> bool helper) so omitted requests default to true; update the UpdateWorkspaceRequest definition accordingly.crates/context_aware_config/src/api/type_templates/handlers.rs (1)
33-35:⚠️ Potential issue | 🟠 MajorWrap the exposed delete flow in a transaction.
delete_handlerupdates audit fields and deletes the row in two separate statements. If the delete fails after the update, the type template is left modified but not removed, and concurrent writers can interleave between the two calls. Please make this path atomic before exposing it publicly.Also applies to: 153-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/type_templates/handlers.rs` around lines 33 - 35, The delete_handler currently issues the audit-field update and the delete as two separate statements; wrap these operations in a single database transaction so they execute atomically: open a transaction using the same DB connection/context, perform the audit update and then the delete inside that transaction, and commit at the end (rollback on any error) to prevent partial updates and interleaving; apply the same transactional change to the same delete flow referenced at lines 153-175. Ensure you use the existing DB/ORM transaction APIs so both statements run on the same connection and propagate errors to return the appropriate failure response.crates/context_aware_config/src/api/variables/handlers.rs (1)
24-27:⚠️ Potential issue | 🟠 MajorMake the newly exposed delete path atomic.
delete_handlerupdates audit fields and deletes the row in separate statements. If the second call fails, the variable remains in place with a partially applied mutation, and concurrent writers can race between the two queries. Wrap both statements in one transaction before exposing this route.Also applies to: 203-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/api/variables/handlers.rs` around lines 24 - 27, The delete_handler currently performs two separate DB calls—updating audit fields and then deleting the variable—which can leave partial mutations on failure; modify delete_handler (and the same pattern in the other handler block around lines 203-215) to run both the audit update and the delete inside a single database transaction/atomic unit using your DB client's transaction API (start/commit/rollback or a provided transaction closure), so that either both statements succeed or the transaction is rolled back on error and no partial state is visible to concurrent writers.crates/superposition/src/workspace/handlers.rs (1)
258-266:⚠️ Potential issue | 🟠 MajorThis endpoint still leaks workspace metadata across permission boundaries.
The TODO is describing a real auth gap: both the query and count paths return every workspace in the org before any per-workspace permission filter is applied. Under the new AuthZ model that exposes names and pagination counts for workspaces the caller should not be able to discover.
If useful, I can help sketch the query/AuthZ wiring for a follow-up.
🟡 Minor comments (5)
crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTypo: "SYTEM_MAINTAINED" should be "SYSTEM_MAINTAINED".
✏️ Proposed fix
-# SYTEM_MAINTAINED (resource - action - action_group) +# SYSTEM_MAINTAINED (resource - action - action_group)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf` at line 10, The comment string "SYTEM_MAINTAINED (resource - action - action_group)" contains a typo; update the symbol "SYTEM_MAINTAINED" to the correct "SYSTEM_MAINTAINED" in the casbin model file so the maintained marker is spelled correctly and any tooling or humans reading the marker see the proper constant.crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs-468-494 (1)
468-494:⚠️ Potential issue | 🟡 MinorRoute collision: Both backfill handlers use
POST /backfill.
backfill_orgs_handler(line 469) andbackfill_workspaces_handler(line 497) both register atPOST /backfill. Since they're added to different scopes (admin_endpointsvsorg_endpoints), they won't collide at runtime, but this is confusing and error-prone if scope registration changes.Also applies to: 496-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs` around lines 468 - 494, Two handlers share the same POST path which is confusing: change the route for one (or both) so they are unique (e.g., backfill_orgs_handler -> POST "/backfill/orgs" and backfill_workspaces_handler -> POST "/backfill/workspaces") or explicitly add scope-specific prefixes when registering; update the route attribute on the functions backfill_orgs_handler and backfill_workspaces_handler and any tests or docs referencing "/backfill" to the new distinct paths to avoid accidental collisions if scopes change.crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs-83-123 (1)
83-123:⚠️ Potential issue | 🟡 Minor
add_policy_handlervalidation returns internal error for bad input.The TODO at line 96 notes this should return a bad request. Currently, an invalid resource-action pair returns a 500 error via
unexpected_error!. This should be a 400 Bad Request.Suggested fix
+use superposition_macros::bad_argument; + // TODO: change this to return bad request action_map .get(&body.obj) .and_then(|actions| actions.contains(&body.act).then_some(())) - .ok_or_else(|| { - format!("Invalid resource-action pair: {}:{}", body.obj, body.act) - })?; + .ok_or_else(|| bad_argument!( + "Invalid resource-action pair: {}:{}", + body.obj, + body.act + ))?;Note: This requires adjusting the error handling since
bad_argument!likely returns anAppErrorrather thanString.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs` around lines 83 - 123, The handler currently turns an invalid resource-action pair into a String error, which becomes a 500 via unexpected_error!; change the validation branch in add_policy_handler so it returns a proper bad-request AppError (use bad_argument!(...)) instead of a String and make the enforcer closure return Result<bool, AppError> so types align; specifically, replace the ok_or_else(|| format!(...))? with an early Err(bad_argument!(...)) (or use .ok_or_else(|| bad_argument!(...))?), and convert enforcer.add_policy errors to AppError (e.g., .map_err(|e| unexpected_error!(e))) so the overall data.enforcer(async |enforcer| { ... }) returns Result<bool, AppError> and the outer .map_err(...) call remains valid.crates/service_utils/tests/action_registry_test.rs-8-40 (1)
8-40:⚠️ Potential issue | 🟡 MinorAssert the registry is non-empty before iterating.
Both tests pass when inventory submission/linking is broken, because the loops are skipped on an empty registry. That makes the new coverage ineffective for the exact regression this feature can introduce.
Suggested test hardening
#[test] fn test_actions_are_registered() { let actions = ActionRegistry::get_all(); + assert!( + !actions.is_empty(), + "Expected at least one registered action" + ); // Verify structure is correct (basic smoke test) for action in &actions { assert!( !action.action_name.is_empty(), @@ #[test] fn test_grouping_by_resource() { let grouped = ActionRegistry::group_by_resource(); + assert!( + !grouped.is_empty(), + "Expected at least one resource group" + ); // Verify grouping is correct for (resource, resource_actions) in &grouped { for action in resource_actions { assert_eq!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/tests/action_registry_test.rs` around lines 8 - 40, The tests test_actions_are_registered and test_grouping_by_resource iterate over ActionRegistry::get_all() and ActionRegistry::group_by_resource() without asserting the registry contains entries, so they silently pass when the registry is empty; add explicit non-empty assertions (e.g., assert!(!actions.is_empty()) for the vector from ActionRegistry::get_all() and assert!(!grouped.is_empty()) for the map from ActionRegistry::group_by_resource()) at the start of each test before the for-loops to ensure the registry is populated and the subsequent checks actually run.crates/service_utils/src/registry.rs-7-18 (1)
7-18:⚠️ Potential issue | 🟡 MinorRemove unnecessary
Deserializederive.The
ActionRegistrystruct derivesDeserialize, but this is never used in the codebase. All instances are created at compile-time viaActionRegistry::new(), and the struct is only serialized to JSON (ingenerate_actions_registry.rsandlist_actions.rs). SinceDeserializeon fields with&'static strlifetimes would fail at runtime if ever attempted, remove the unused derive:Remove Deserialize
-#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Clone, Debug, Serialize)] pub struct ActionRegistry {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/registry.rs` around lines 7 - 18, The ActionRegistry struct unnecessarily derives Deserialize; remove Deserialize from the derive list on struct ActionRegistry (leaving Clone, Debug, Serialize) and remove any now-unused serde::Deserialize import, making sure uses that serialize ActionRegistry (e.g., in generate_actions_registry.rs and list_actions.rs) still work; locate the struct definition named ActionRegistry and update its derive attribute and imports accordingly (no code changes to ActionRegistry::new() are required).
🧹 Nitpick comments (10)
.env.example (1)
3-3: Consider removing unused CASBIN_DATABASE_URL.This commented variable is marked as unused and doesn't match the actual Casbin database configuration pattern used in the codebase. The code reads individual components (
CASBIN_DB_USER,CASBIN_DB_HOST, etc.) viaget_database_url()with the "CASBIN" prefix, not a combinedCASBIN_DATABASE_URL. This could cause confusion for users setting up Casbin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 3, Remove the commented-out CASBIN_DATABASE_URL line from .env.example to avoid confusion; update the example to reflect the actual Casbin configuration pattern used by get_database_url("CASBIN") which reads individual env vars like CASBIN_DB_USER, CASBIN_DB_PASSWORD, CASBIN_DB_HOST, CASBIN_DB_PORT and CASBIN_DB_NAME, and ensure any README or comment points users to those CASBIN_DB_* variables instead of a combined CASBIN_DATABASE_URL.crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf (1)
80-102: Clarify the purpose of embedded test data.This file contains hardcoded user names (
ayush_test,ayush,nata,boaz) and workspace identifiers (o1_w1,o1_w2,o2_w1) that appear to be test/development data. Since the Dockerfile copiesmodel.conf(notmodel_with_data.conf), this won't affect production, but having this committed could cause confusion.Consider:
- Moving this to a
test/orexamples/directory- Renaming to
model_example.conformodel_test.conf- Adding a comment at the top clarifying this is for development/testing only
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf` around lines 80 - 102, This file contains hardcoded test entries such as group subjects (g lines) and policies (p lines) referencing ayush_test, ayush, nata, boaz and workspace domains o1_w1, o1_w2, o2_w1 which are development-only; move the contents out of the production config by either relocating this file to a test or examples folder (e.g., test/model_test.conf or examples/model_example.conf) or renaming it to model_test.conf/model_example.conf, and add a clear top-of-file comment like "DEV/TEST ONLY: example policies and subjects" so callers know it is not production data; when moving/renaming ensure any references or CI/Docker build steps that expect model.conf continue to use the production file and update docs or scripts to point to the new example/test file if needed.crates/frontend/src/components/side_nav.rs (3)
386-448: Significant code duplication betweenOrgNavComponentandNavComponent.
OrgNavComponent(lines 386-448) duplicates most of the rendering logic fromNavComponent(lines 280-344). The main difference is the absence ofWorkspaceSelector. Consider extracting shared navigation rendering logic into a common helper component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 386 - 448, OrgNavComponent duplicates most of NavComponent's rendering (only lacking WorkspaceSelector), so extract the shared nav UI into a new helper component (e.g., SharedNavBody or NavBody) that encapsulates the <nav>...<ul> rendering and uses props for app_routes, collapsed_rws, location/base, and an optional children slot for WorkspaceSelector; then update NavComponent to render WorkspaceSelector + SharedNavBody and OrgNavComponent to render SharedNavBody alone (keep NavItem usage intact) so both components reuse the same implementation.
22-24:use_side_nav_collapsed()will panic if context is missing.The
unwrap()call will panic at runtime if this hook is called outside aSideNavCollapsedProvider. Consider usingexpect()with a descriptive message or returning anOptionto fail gracefully.Suggested improvement
pub fn use_side_nav_collapsed() -> RwSignal<NavState> { - use_context::<RwSignal<NavState>>().unwrap() + use_context::<RwSignal<NavState>>() + .expect("use_side_nav_collapsed must be called within a SideNavCollapsedProvider") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 22 - 24, The current use_side_nav_collapsed() calls use_context::<RwSignal<NavState>>().unwrap() which will panic if the component is used outside a SideNavCollapsedProvider; change it to either return an Option by removing unwrap (e.g., return use_context::<RwSignal<NavState>>() so callers handle None) or replace unwrap() with expect("<descriptive message>") to provide a clear runtime error referencing SideNavCollapsedProvider and use_side_nav_collapsed so missing context is obvious; update call sites accordingly if you choose the Option approach.
107-113: Address TODO: Conditionally render Authorization Settings route.Both route lists include TODO comments about showing the "Authorization Settings" route only when AuthZ is enabled. This could leak navigation UI to users when the feature is disabled.
Would you like me to help implement conditional rendering based on the AuthZ provider configuration? This could involve passing an
authz_enabledflag to the route creation functions.Also applies to: 376-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 107 - 113, The Authorization Settings AppRoute is always added; update the route-creation functions that build the admin route lists (where AppRoute { key: format!("{base}/admin/{org}/{workspace}/authz") ... } appears) to accept an authz_enabled: bool parameter and only push/create that AppRoute when authz_enabled is true; modify both places referenced (the current block and the other occurrence around lines 376-382) to guard creation of the "Authorization Settings" route by checking authz_enabled so the UI doesn't surface the route when the AuthZ provider is disabled.crates/service_utils/src/middlewares/auth_z/casbin.rs (1)
203-217: Duplicate policy refresh logic betweenenforcer()andis_allowed().Policy refresh is checked both in
enforcer()(lines 48-63) andis_allowed()(lines 205-217). Sinceis_allowed()uses a read lock directly rather than callingenforcer(), this is intentional but creates maintenance burden. Consider extracting the refresh check into a shared helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin.rs` around lines 203 - 217, The policy refresh check is duplicated between enforcer() and is_allowed(); extract the shared logic into a private async helper (e.g., ensure_policies_fresh or refresh_if_needed) that captures the Utc::now(), reads last_policy_load_time, compares against policy_refresh_interval, and calls self.refresh_policies().await on need, and replace the duplicated blocks in both enforcer() and is_allowed() with a call to that helper so both paths use the same refresh behavior and locking semantics.crates/frontend/src/components/authz.rs (1)
181-212: Consider clearing input fields after successful policy addition.After a successful
add_policycall, the input fields retain their values. Based on learnings for this codebase, usecreate_node_refwith imperative clearing to reset form fields after submission:♻️ Example pattern for input clearing
let subject_ref = create_node_ref::<html::Input>(); // In the action success path: if let Some(input) = subject_ref.get() { input.set_value(""); } p_sub.set(String::new());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/authz.rs` around lines 181 - 212, The add_policy_action currently leaves form inputs populated after a successful casbin::add_policy; update the success path in add_policy_action to imperatively clear both the state signals (p_sub, p_act, p_attr, p_obj) and any DOM input refs created via create_node_ref (e.g., subject_ref, action_ref, attr_ref, obj_ref) by calling their .get() and set_value("") if present, then set the corresponding signals to empty strings (or "*" for attr if desired) before calling policies_resource.refetch(), resource_action_map_resource.refetch(), and enqueue_alert; ensure you add and use create_node_ref declarations for the inputs if not already present and perform the clearing only after the awaited casbin::add_policy returns successfully.crates/service_utils/src/registry.rs (1)
81-87: Test assertion is a placeholder.The
test_grouping_by_resourcetest has a placeholder assertion that always passes. Consider adding meaningful assertions or marking this as a TODO:♻️ Improve test or add TODO
#[test] fn test_grouping_by_resource() { - // This test will work after actions are registered via the macro let grouped = ActionRegistry::group_by_resource(); - // Just verify the function executes without error - assert!(grouped.is_empty() || !grouped.is_empty()); // Placeholder assertion + // Note: In unit test context, no actions are registered via inventory + // This test verifies the function executes without panicking + // Integration tests should verify actual registration + let _ = grouped; // Explicitly acknowledge unused value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/registry.rs` around lines 81 - 87, The test test_grouping_by_resource currently contains a no-op placeholder assertion; replace it with a meaningful check: either register known actions (using the same macro used by the crate) before calling ActionRegistry::group_by_resource and assert the returned map contains the expected resource keys and action counts (e.g., assert_eq!(grouped.get("expected_resource").unwrap().len(), expected_count)), or if registration isn't feasible in this unit test, mark the test as TODO/ignored (#[ignore] plus a comment) or assert a concrete expected empty state (e.g., assert!(grouped.is_empty())) so the test fails if behavior changes unexpectedly. Ensure the change touches test_grouping_by_resource and references ActionRegistry::group_by_resource so the intent is clear.crates/frontend/src/pages/authz.rs (1)
32-32: Consider defensive error handling for missing context.The
.unwrap()calls onuse_contextwill panic at runtime if the context is not available. While routing typically ensures these contexts are provided, defensive handling could improve robustness:🛡️ Optional: Add fallback or error view
pub fn OrganisationAuthz() -> impl IntoView { - let org = use_context::<Signal<OrganisationId>>().unwrap(); + let Some(org) = use_context::<Signal<OrganisationId>>() else { + return view! { <div class="text-red-600">"Organisation context not available"</div> }.into_view(); + };Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/authz.rs` at line 32, The current calls to use_context::<Signal<OrganisationId>>().unwrap() (and the similar use_context calls at lines ~45-46) can panic if the context is missing; replace the unwraps with defensive handling: call use_context::<Signal<OrganisationId>>() and pattern-match the Option (if let Some / match) and provide a safe fallback such as returning an error/loading view or early-returning from the component when the context is None, so the component renders a graceful message instead of panicking; apply the same change to the other use_context::<Signal<...>>() invocations referenced in the review.crates/frontend/src/api.rs (1)
125-144: Reusecasbin_url_and_headersfor admin action groups.These two functions duplicate the same admin URL/header assembly already encoded in
casbin_url_and_headers. Reusing the helper keeps future route/header changes in one place.Small DRY cleanup
pub async fn list_action_groups() -> Result<Vec<Vec<String>>, String> { - let host = use_host_server(); - - let url = format!("{host}/authz/admin/casbin/action-groups"); - let headers = construct_request_headers(&[])?; + let (url, headers) = + casbin_url_and_headers("action-groups", AuthzScope::Admin)?; let response = request(url, reqwest::Method::GET, None::<()>, headers).await?; parse_json_response(response).await } pub async fn add_action_group( payload: ActionGroupPolicyRequest, ) -> Result<(), String> { - let host = use_host_server(); - - let url = format!("{host}/authz/admin/casbin/action-groups"); - let headers = construct_request_headers(&[])?; + let (url, headers) = + casbin_url_and_headers("action-groups", AuthzScope::Admin)?; request(url, reqwest::Method::POST, Some(payload), headers).await?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/api.rs` around lines 125 - 144, Both list_action_groups and add_action_group duplicate building the admin casbin URL and headers; replace the manual host/format/construct_request_headers steps by calling the existing casbin_url_and_headers helper to obtain the URL and headers used for admin action-groups, then call request as before (e.g., use the returned (url, headers) from casbin_url_and_headers in list_action_groups and add_action_group), removing the duplicated use_host_server/format/construct_request_headers code so future route/header changes stay centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/context_aware_config/src/api/secrets/handlers.rs`:
- Line 283: The file uses #[authorized(resource = MasterEncryptionKey)] but
never registers that resource; add a declare_resource!(MasterEncryptionKey);
declaration near the top of the file following the existing
declare_resource!(Secret); pattern so the MasterEncryptionKey resource is
registered with the AuthZ registry and appears in policy-management UI; locate
the existing Secret declaration and insert the MasterEncryptionKey declaration
adjacent to it.
In `@crates/frontend/src/api.rs`:
- Around line 106-123: Both list_domain_action_groups and
add_domain_action_group currently allow AuthzScope::Admin which yields an
unsupported /authz/admin/casbin/domain-action-groups path; update both functions
to explicitly reject Admin scope before calling casbin_url_and_headers (e.g., if
scope == AuthzScope::Admin return Err(...)) so only org/workspace scopes are
accepted; reference the functions list_domain_action_groups,
add_domain_action_group and the helper casbin_url_and_headers when making the
check and returning a clear error string.
In `@crates/frontend/src/app.rs`:
- Around line 142-145: Route collision: /admin/:org_id/settings/authz conflicts
with /admin/:org_id/:workspace/authz when a workspace is named "settings".
Update the routes to avoid the ambiguous slug (e.g., rename or namespace the
admin route like "org-settings" or move workspace-level routes under an explicit
"workspace" segment) by changing the Route that currently has
path="settings/authz" and view=OrganisationAuthz (or alternatively changing the
workspace Route path pattern) so the admin and workspace authz routes are
distinct; additionally add a server-side check to reserve the "settings" slug in
workspace name validation to prevent future collisions.
In `@crates/frontend/src/components/authz.rs`:
- Line 46: Rows passed to the Table use an empty string for key_column which
prevents Leptos from reconciling DOM updates; update the code that builds the
rows for the Table (the rows value passed to the Table component and the Table
invocation with key_column="") to provide a stable unique key for each
row—either add an ID field to the row objects returned from the API, or compute
a composite key from existing fields (e.g., subject + domain + object for policy
rows), or fall back to using the row index only if ordering is stable; ensure
the Table invocation sets key_column to the name of that unique field (instead
of "") and adjust any row construction helpers to include the chosen unique
identifier.
In `@crates/frontend/src/pages/organisations.rs`:
- Around line 156-161: The "Authorization Settings" ButtonAnchor is currently
always rendered; wrap it in a conditional that checks the actual AuthZ
enablement/provider flag (e.g., authz_enabled or has_authz_provider) from the
page's settings/state and only render the ButtonAnchor when that flag is true.
Locate the Organisations page render function (where ButtonAnchor is used) and
read the AuthZ toggle from the existing settings/state struct passed into the
view (or thread-local app state), add the guard around the ButtonAnchor usage,
and ensure the flag is propagated into this component if not already available.
In `@crates/service_utils/src/middlewares/auth_z.rs`:
- Around line 254-263: The method try_get_casbin_policy_engine on AuthZManager
is currently declared with restrictive visibility pub(self) which prevents
access from the casbin submodule; change its visibility to pub(super) so child
modules (e.g., the casbin handlers) can call it, leaving the function body and
return type (superposition::Result<Arc<CasbinPolicyEngine>>) unchanged and
preserving the existing match on AuthZManager::Casbin / AuthZManager::NoAuth.
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 42-73: The enforcer() helper always calls enforcer.save_policy(),
causing writes for read-only closures; modify enforcer() to avoid saving when no
mutation occurred by either adding a boolean parameter (e.g., mutable: bool) or
splitting into two methods (enforcer_read and enforcer_write). Keep the existing
policy refresh logic (last_policy_load_time, policy_refresh_interval,
enforcer.load_policy()) but only call enforcer.save_policy() if mutable==true or
in the write-specific method; ensure callers like list_policy_handler and
list_roles_handler use the read variant/flag while mutating handlers use the
write variant/flag.
In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql`:
- Line 30: The policy line that grants ('p', '*', '*', 'workspace', 'read', '*',
'') makes workspace read access global and overrides the intended
workspace_reader rule; remove or restrict that wildcard permission so it no
longer grants all subjects read access to all workspaces—either delete this
tuple or change its subject/object to a specific role (e.g., use the explicit
workspace_reader role or narrow to a list-only action) so the existing
workspace_reader policy (the explicit policy near the later tuple) controls who
can read/list workspaces.
- Around line 1-106: The casbin.sql is not executed so the casbin_rule table and
seed rows are missing, causing DieselAdapter/Enforcer::new and subsequent
enforcer.load_policy() to fail; fix by converting
crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql into a proper
Diesel migration under crates/superposition_types/migrations/ (follow existing
migration filename convention), include the CREATE TABLE and INSERT ... ON
CONFLICT seed statements in the up migration (and a corresponding down to drop
casbin_rule), and ensure the migration is committed so the standard migration
run creates the casbin_rule table before DieselAdapter/Enforcer::new is invoked.
In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs`:
- Around line 157-178: The read-only handler list_policy_handler currently calls
data.enforcer(...) which will unconditionally persist policies; change it to use
the read-only variant (e.g. enforcer_read or enforcer_ro) exposed by
AuthZManager so the closure does not trigger a save: get the policy engine via
try_get_casbin_policy_engine()? then call its read-only enforcer method (same
async closure body that calls enforcer.get_policy() and filters by
domain_matcher) and preserve the existing error mapping (map_err(|e|
unexpected_error!(e))) and return Ok(Json(policies)); apply the same replacement
for the other list_* handlers in this file.
In `@crates/service_utils/src/middlewares/auth_z/casbin/model_ext.conf`:
- Around line 17-34: The model currently contains hardcoded subjects and
wildcard grants which won't match real requests because is_allowed() uses
user.get_email() as the Casbin subject (see is_allowed in casbin.rs), and the
global wildcard grant (e.g., "g, boaz, admin, *" and broad policy "p, admin, *,
*, *") bakes an unintended repo-wide admin. Remove or replace hardcoded
principals (kat, ayush, nata, boaz) and any "*" domain/wildcard policy entries;
instead provide either commented example templates or use domain-scoped
placeholders that align with user.get_email() values and rely on runtime policy
provisioning. Ensure no persistent global admin rule remains in model_ext.conf
so authorization reflects actual identities created at runtime.
- Around line 7-15: The matcher and role definitions must match the existing
Casbin schema: restore the missing g2 role_definition (g2 = _, _, _) and change
g3 back to two-arity (g3 = _, _) instead of three-arity; then update the matcher
to perform action-grouping using r.obj + ":" + r.act and call g3 with that pair
(e.g. g3(r.obj + ":" + r.act, p.act)) instead of g3(r.obj, r.act, p.act); apply
the same corrections for the similar block at lines 32-34 so stored action-group
mappings remain compatible with the current schema.
- Around line 1-34: The model in model_ext.conf is not used because
CasbinPolicyEngine still loads model.conf; update CasbinPolicyEngine to accept
and use the extended model (or a configurable model path) when constructing the
enforcer so enforce() uses model_ext.conf: modify the code that builds the
enforcer in casbin.rs (symbol: CasbinPolicyEngine and its enforcer construction)
to read the provided model_ext.conf (or a new config option) instead of always
loading model.conf, and ensure the enforcer initialization and reload logic
references the new model path so the matchers/policies in model_ext.conf take
effect.
In `@crates/superposition/examples/generate_actions_registry.rs`:
- Around line 8-11: The example currently force-links only context_aware_config
and experimentation_platform, so inventory misses handlers defined in the
superposition crate; add an explicit force-link for the superposition library by
adding an extern crate reference for "superposition" (similar to the existing
extern crate lines) in the generate_actions_registry.rs example so the
#[authorized] handlers in crates/superposition (workspace modules: organisation,
resolve, webhooks, etc.) are pulled into the binary and included in the
generated JSON registry.
In `@crates/superposition/src/organisation/handlers.rs`:
- Around line 65-73: The organisation row is committed before calling
authz_handler.on_org_creation (diesel::insert_into(...).get_result(&mut conn)),
so if the hook fails the DB and AuthZ become inconsistent; wrap the DB insert
and the AuthZ provisioning in a single atomic operation or implement a
transactional outbox/reconciliation flow. Concretely: perform the insert and the
call to authz_handler.on_org_creation inside a DB transaction (use the
connection's transaction() scope) so the insert is rolled back on hook failure,
or instead persist the org in a “pending” state and write an outbox/event row in
the same transaction, then have a separate reconciler pick up the outbox and
call authz_handler.on_org_creation, marking the outbox done on success; ensure
you reference the existing diesel::insert_into(organisations::table) /
get_result and authz_handler.on_org_creation paths when applying the change.
In `@crates/superposition/src/workspace/handlers.rs`:
- Around line 172-179: The code currently calls
authz_handler.on_workspace_creation(...) and on_workspace_admin_update(...)
after the DB transaction commits (using created_workspace and
workspace_schema_name), so if the AuthZ call fails the workspace change is
already persisted; instead, make the side-effect retryable by either (A) moving
the AuthZ notification into the same transaction boundary (call or enqueue a
durable event inside the transaction) or (B) persist a reconciliation event
record (e.g., workspace_reconciliation_events) referencing created_workspace.id
and workspace_schema_name inside the transaction and return success immediately;
then have the background processor call
authz_handler.on_workspace_creation/on_workspace_admin_update and mark the event
succeeded/failed. Update the calls in handlers.rs to write the event (or
enqueue) using the transaction context rather than awaiting authz_handler after
commit, and remove the immediate .await/map_err(...) failure path so API
responses are not returned as error after commit.
- Around line 214-221: The query that fetches old_admin_email uses the wrong
column: it filters on workspaces::workspace_schema_name but should filter by the
plain workspace name like the update does; in the block that reads
request.workspace_admin_email and calls
workspaces::dsl::workspaces.select(workspaces::workspace_admin_email).get_result(transaction_conn)
replace the workspace_schema_name predicate with the workspace_name predicate
(i.e., use workspaces::workspace_name.eq(&workspace_name) alongside
workspaces::organisation_id.eq(&org_id.0)) so the lookup matches the same row as
the subsequent update.
In `@openfga/docker-compose.yaml`:
- Around line 8-10: The docker-compose.yaml currently hardcodes database
credentials (POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB); change those values
to environment interpolation (e.g., use ${POSTGRES_USER}, ${POSTGRES_PASSWORD},
${POSTGRES_DB}) so credentials are supplied from the environment, add
corresponding keys to a .env.example with safe placeholders, and ensure .env is
listed in .gitignore so secrets are not checked into the repo; update any
startup docs to instruct users to populate their own .env before running
compose.
- Line 27: The Docker Compose port mapping "8080:8181" is incorrect for
OpenFGA's REST API; update the ports entry that currently reads "8080:8181" to
"8080:8080" so the host port 8080 maps to the container's REST port 8080 (change
the ports value in the docker-compose.yaml where the REST API mapping is
defined).
---
Outside diff comments:
In `@crates/context_aware_config/src/api/type_templates/handlers.rs`:
- Around line 33-35: The delete_handler currently issues the audit-field update
and the delete as two separate statements; wrap these operations in a single
database transaction so they execute atomically: open a transaction using the
same DB connection/context, perform the audit update and then the delete inside
that transaction, and commit at the end (rollback on any error) to prevent
partial updates and interleaving; apply the same transactional change to the
same delete flow referenced at lines 153-175. Ensure you use the existing DB/ORM
transaction APIs so both statements run on the same connection and propagate
errors to return the appropriate failure response.
In `@crates/context_aware_config/src/api/variables/handlers.rs`:
- Around line 24-27: The delete_handler currently performs two separate DB
calls—updating audit fields and then deleting the variable—which can leave
partial mutations on failure; modify delete_handler (and the same pattern in the
other handler block around lines 203-215) to run both the audit update and the
delete inside a single database transaction/atomic unit using your DB client's
transaction API (start/commit/rollback or a provided transaction closure), so
that either both statements succeed or the transaction is rolled back on error
and no partial state is visible to concurrent writers.
In `@crates/superposition_types/src/api/workspace.rs`:
- Around line 75-89: The UpdateWorkspaceRequest AsChangeset will set columns to
NULL when Option<T> fields are None which conflicts with NOT NULL DB columns;
add #[diesel(treat_none_as_null = "false")] (or the appropriate
treat_none_as_null attribute) to the struct/fields so None values are skipped
instead of generating SET column = NULL for fields like workspace_admin_email,
workspace_status, allow_experiment_self_approval, auto_populate_control,
enable_context_validation, and enable_change_reason_validation, and add a serde
default for auto_populate_control (e.g., #[serde(default = "default_true")] on
auto_populate_control plus a default_true() -> bool helper) so omitted requests
default to true; update the UpdateWorkspaceRequest definition accordingly.
---
Minor comments:
In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs`:
- Around line 468-494: Two handlers share the same POST path which is confusing:
change the route for one (or both) so they are unique (e.g.,
backfill_orgs_handler -> POST "/backfill/orgs" and backfill_workspaces_handler
-> POST "/backfill/workspaces") or explicitly add scope-specific prefixes when
registering; update the route attribute on the functions backfill_orgs_handler
and backfill_workspaces_handler and any tests or docs referencing "/backfill" to
the new distinct paths to avoid accidental collisions if scopes change.
- Around line 83-123: The handler currently turns an invalid resource-action
pair into a String error, which becomes a 500 via unexpected_error!; change the
validation branch in add_policy_handler so it returns a proper bad-request
AppError (use bad_argument!(...)) instead of a String and make the enforcer
closure return Result<bool, AppError> so types align; specifically, replace the
ok_or_else(|| format!(...))? with an early Err(bad_argument!(...)) (or use
.ok_or_else(|| bad_argument!(...))?), and convert enforcer.add_policy errors to
AppError (e.g., .map_err(|e| unexpected_error!(e))) so the overall
data.enforcer(async |enforcer| { ... }) returns Result<bool, AppError> and the
outer .map_err(...) call remains valid.
In `@crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf`:
- Line 10: The comment string "SYTEM_MAINTAINED (resource - action -
action_group)" contains a typo; update the symbol "SYTEM_MAINTAINED" to the
correct "SYSTEM_MAINTAINED" in the casbin model file so the maintained marker is
spelled correctly and any tooling or humans reading the marker see the proper
constant.
In `@crates/service_utils/src/registry.rs`:
- Around line 7-18: The ActionRegistry struct unnecessarily derives Deserialize;
remove Deserialize from the derive list on struct ActionRegistry (leaving Clone,
Debug, Serialize) and remove any now-unused serde::Deserialize import, making
sure uses that serialize ActionRegistry (e.g., in generate_actions_registry.rs
and list_actions.rs) still work; locate the struct definition named
ActionRegistry and update its derive attribute and imports accordingly (no code
changes to ActionRegistry::new() are required).
In `@crates/service_utils/tests/action_registry_test.rs`:
- Around line 8-40: The tests test_actions_are_registered and
test_grouping_by_resource iterate over ActionRegistry::get_all() and
ActionRegistry::group_by_resource() without asserting the registry contains
entries, so they silently pass when the registry is empty; add explicit
non-empty assertions (e.g., assert!(!actions.is_empty()) for the vector from
ActionRegistry::get_all() and assert!(!grouped.is_empty()) for the map from
ActionRegistry::group_by_resource()) at the start of each test before the
for-loops to ensure the registry is populated and the subsequent checks actually
run.
---
Nitpick comments:
In @.env.example:
- Line 3: Remove the commented-out CASBIN_DATABASE_URL line from .env.example to
avoid confusion; update the example to reflect the actual Casbin configuration
pattern used by get_database_url("CASBIN") which reads individual env vars like
CASBIN_DB_USER, CASBIN_DB_PASSWORD, CASBIN_DB_HOST, CASBIN_DB_PORT and
CASBIN_DB_NAME, and ensure any README or comment points users to those
CASBIN_DB_* variables instead of a combined CASBIN_DATABASE_URL.
In `@crates/frontend/src/api.rs`:
- Around line 125-144: Both list_action_groups and add_action_group duplicate
building the admin casbin URL and headers; replace the manual
host/format/construct_request_headers steps by calling the existing
casbin_url_and_headers helper to obtain the URL and headers used for admin
action-groups, then call request as before (e.g., use the returned (url,
headers) from casbin_url_and_headers in list_action_groups and
add_action_group), removing the duplicated
use_host_server/format/construct_request_headers code so future route/header
changes stay centralized.
In `@crates/frontend/src/components/authz.rs`:
- Around line 181-212: The add_policy_action currently leaves form inputs
populated after a successful casbin::add_policy; update the success path in
add_policy_action to imperatively clear both the state signals (p_sub, p_act,
p_attr, p_obj) and any DOM input refs created via create_node_ref (e.g.,
subject_ref, action_ref, attr_ref, obj_ref) by calling their .get() and
set_value("") if present, then set the corresponding signals to empty strings
(or "*" for attr if desired) before calling policies_resource.refetch(),
resource_action_map_resource.refetch(), and enqueue_alert; ensure you add and
use create_node_ref declarations for the inputs if not already present and
perform the clearing only after the awaited casbin::add_policy returns
successfully.
In `@crates/frontend/src/components/side_nav.rs`:
- Around line 386-448: OrgNavComponent duplicates most of NavComponent's
rendering (only lacking WorkspaceSelector), so extract the shared nav UI into a
new helper component (e.g., SharedNavBody or NavBody) that encapsulates the
<nav>...<ul> rendering and uses props for app_routes, collapsed_rws,
location/base, and an optional children slot for WorkspaceSelector; then update
NavComponent to render WorkspaceSelector + SharedNavBody and OrgNavComponent to
render SharedNavBody alone (keep NavItem usage intact) so both components reuse
the same implementation.
- Around line 22-24: The current use_side_nav_collapsed() calls
use_context::<RwSignal<NavState>>().unwrap() which will panic if the component
is used outside a SideNavCollapsedProvider; change it to either return an Option
by removing unwrap (e.g., return use_context::<RwSignal<NavState>>() so callers
handle None) or replace unwrap() with expect("<descriptive message>") to provide
a clear runtime error referencing SideNavCollapsedProvider and
use_side_nav_collapsed so missing context is obvious; update call sites
accordingly if you choose the Option approach.
- Around line 107-113: The Authorization Settings AppRoute is always added;
update the route-creation functions that build the admin route lists (where
AppRoute { key: format!("{base}/admin/{org}/{workspace}/authz") ... } appears)
to accept an authz_enabled: bool parameter and only push/create that AppRoute
when authz_enabled is true; modify both places referenced (the current block and
the other occurrence around lines 376-382) to guard creation of the
"Authorization Settings" route by checking authz_enabled so the UI doesn't
surface the route when the AuthZ provider is disabled.
In `@crates/frontend/src/pages/authz.rs`:
- Line 32: The current calls to use_context::<Signal<OrganisationId>>().unwrap()
(and the similar use_context calls at lines ~45-46) can panic if the context is
missing; replace the unwraps with defensive handling: call
use_context::<Signal<OrganisationId>>() and pattern-match the Option (if let
Some / match) and provide a safe fallback such as returning an error/loading
view or early-returning from the component when the context is None, so the
component renders a graceful message instead of panicking; apply the same change
to the other use_context::<Signal<...>>() invocations referenced in the review.
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 203-217: The policy refresh check is duplicated between enforcer()
and is_allowed(); extract the shared logic into a private async helper (e.g.,
ensure_policies_fresh or refresh_if_needed) that captures the Utc::now(), reads
last_policy_load_time, compares against policy_refresh_interval, and calls
self.refresh_policies().await on need, and replace the duplicated blocks in both
enforcer() and is_allowed() with a call to that helper so both paths use the
same refresh behavior and locking semantics.
In `@crates/service_utils/src/middlewares/auth_z/casbin/model_with_data.conf`:
- Around line 80-102: This file contains hardcoded test entries such as group
subjects (g lines) and policies (p lines) referencing ayush_test, ayush, nata,
boaz and workspace domains o1_w1, o1_w2, o2_w1 which are development-only; move
the contents out of the production config by either relocating this file to a
test or examples folder (e.g., test/model_test.conf or
examples/model_example.conf) or renaming it to
model_test.conf/model_example.conf, and add a clear top-of-file comment like
"DEV/TEST ONLY: example policies and subjects" so callers know it is not
production data; when moving/renaming ensure any references or CI/Docker build
steps that expect model.conf continue to use the production file and update docs
or scripts to point to the new example/test file if needed.
In `@crates/service_utils/src/registry.rs`:
- Around line 81-87: The test test_grouping_by_resource currently contains a
no-op placeholder assertion; replace it with a meaningful check: either register
known actions (using the same macro used by the crate) before calling
ActionRegistry::group_by_resource and assert the returned map contains the
expected resource keys and action counts (e.g.,
assert_eq!(grouped.get("expected_resource").unwrap().len(), expected_count)), or
if registration isn't feasible in this unit test, mark the test as TODO/ignored
(#[ignore] plus a comment) or assert a concrete expected empty state (e.g.,
assert!(grouped.is_empty())) so the test fails if behavior changes unexpectedly.
Ensure the change touches test_grouping_by_resource and references
ActionRegistry::group_by_resource so the intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecfff052-40d2-4c70-ae20-b4010caef43d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.env.exampleCargo.tomlDockerfilecrates/cac_client/Cargo.tomlcrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/audit_log/handlers.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/experimentation_client/Cargo.tomlcrates/experimentation_platform/Cargo.tomlcrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/authz.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/hoc/layout.rscrates/frontend/src/pages.rscrates/frontend/src/pages/authz.rscrates/frontend/src/pages/organisations.rscrates/service_utils/Cargo.tomlcrates/service_utils/examples/list_actions.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/casbin.rscrates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlcrates/service_utils/src/middlewares/auth_z/casbin/handlers.rscrates/service_utils/src/middlewares/auth_z/casbin/model.confcrates/service_utils/src/middlewares/auth_z/casbin/model_ext.confcrates/service_utils/src/middlewares/auth_z/casbin/model_with_data.confcrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/registry.rscrates/service_utils/tests/action_registry_test.rscrates/superposition/Cargo.tomlcrates/superposition/examples/generate_actions_registry.rscrates/superposition/src/main.rscrates/superposition/src/organisation/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_core/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_derives/src/lib.rscrates/superposition_provider/Cargo.tomlcrates/superposition_types/src/api.rscrates/superposition_types/src/api/authz.rscrates/superposition_types/src/api/authz/casbin.rscrates/superposition_types/src/api/workspace.rscrates/superposition_types/src/lib.rsdocs/api_examples.mdopenfga/docker-compose.yamlsmithy/models/workspace.smithy
💤 Files with no reviewable changes (1)
- smithy/models/workspace.smithy
ayushjain17
force-pushed
the
auth_z_ui
branch
3 times, most recently
from
f03a297 to
ca64e6e
Compare
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (5)
crates/superposition/src/organisation/handlers.rs (1)
64-76:⚠️ Potential issue | 🔴 CriticalDon’t return success when AuthZ bootstrap was skipped.
Line 64 persists the organisation, and Lines 69-76 then discard both
Err(_)andOk(false)fromon_org_creation(). Incrates/service_utils/src/middlewares/auth_z/casbin.rs:114-143,Ok(false)means the org policy was not added and the admin grouping step never ran, so this path can return 200 for an organisation that nobody can administer. Please make the DB write and AuthZ bootstrap recoverable as one unit here—e.g. an outbox/reconciliation flow or explicit compensation on both sides—instead of log-and-continuing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/organisation/handlers.rs` around lines 64 - 76, The code currently persists new_org via diesel::insert_into then calls authz_handler.on_org_creation(new_org.id, new_org.admin_email) but ignores both Err(_) and Ok(false), allowing a DB record without successful AuthZ bootstrap; change this so the DB write and AuthZ bootstrap are atomic: perform the insert and the on_org_creation call inside a recoverable flow (e.g., a DB transaction or an explicit outbox/compensation flow) so that if authz_handler.on_org_creation returns Err or Ok(false) you either roll back the DB insert (abort the transaction and return an error) or enqueue an outbox event and return an error status until reconciliation succeeds; locate the logic around new_org insertion and authz_handler.on_org_creation and make failure pathways return an error instead of just logging.crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql (1)
1-106:⚠️ Potential issue | 🔴 CriticalWire this bootstrap SQL into migrations or startup before Casbin initializes.
This file is still dead code from the runtime’s perspective:
CasbinPolicyEngine::newincrates/service_utils/src/middlewares/auth_z/casbin.rs:282-318builds theDieselAdapter/Enforcerdirectly and never executescasbin.sql. On a fresh database,casbin_ruleand the seed policies will still be missing when the enforcer starts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql` around lines 1 - 106, Casbin bootstrap SQL (casbin.sql) is never executed before the enforcer is created, so CasbinPolicyEngine::new (the function that builds DieselAdapter and Enforcer) may start against a fresh DB without the casbin_rule table or seed policies; update startup/migration wiring so the SQL runs before creating the adapter: either (A) execute the casbin.sql contents (CREATE TABLE + INSERT ... ON CONFLICT DO NOTHING) from CasbinPolicyEngine::new before constructing DieselAdapter/Enforcer, or (B) move the idempotent table creation and seed logic into a migration step that is guaranteed to run prior to CasbinPolicyEngine::new; reference CasbinPolicyEngine::new, DieselAdapter, Enforcer and casbin_rule when making the change so the table and seed policies exist on first startup.crates/superposition/src/workspace/handlers.rs (2)
224-228:⚠️ Potential issue | 🔴 CriticalFix the old-admin lookup predicate.
Line 226 compares
workspace_schema_nameagainst the plain{workspace_name}path value. Stored rows use<org>_<workspace>there, so any request that setsworkspace_admin_emailwill fail before the update runs.Possible fix
let email = workspaces::dsl::workspaces .filter(workspaces::organisation_id.eq(&org_id.0)) - .filter(workspaces::workspace_schema_name.eq(&workspace_name)) + .filter(workspaces::workspace_name.eq(&workspace_name)) .select(workspaces::workspace_admin_email) .get_result(transaction_conn)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/workspace/handlers.rs` around lines 224 - 228, The query is comparing workspaces::workspace_schema_name to the plain workspace_name, but stored rows use the "<org>_<workspace>" schema name; build the correct schema string (e.g. let schema = format!("{}_{}", org_id.0, workspace_name)) and use workspace_schema_name.eq(&schema) in the filter for the workspaces::dsl::workspaces query that selects workspaces::workspace_admin_email and calls get_result(transaction_conn).
175-184:⚠️ Potential issue | 🔴 CriticalMake the AuthZ sync durable, not best-effort.
These hooks run after commit and only log failures. If the AuthZ write fails here, the workspace row is already persisted but Casbin state is stale, so the new workspace/admin can be left with incorrect permissions until manual repair. Please persist a retryable reconciliation/outbox record inside the transaction, or move the policy mutation behind a durable worker instead of fire-and-forget logging.
Also applies to: 251-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/workspace/handlers.rs` around lines 175 - 184, The current fire-and-forget call to authz_handler.on_workspace_creation (invoked with workspace_schema_name and created_workspace.workspace_admin_email) is brittle because failures are only logged; instead make the AuthZ sync durable by recording a retryable outbox/reconciliation record inside the same transaction that creates the workspace (e.g., create a WorkspaceAuthzOutbox entry referencing workspace id/schema and admin email) or enqueue a durable job/message that guaranteed-delivery workers will process, and remove the best-effort .await.map_err logging pattern; ensure the transaction commits both the workspace row and the outbox record (or pushes the durable job) so failed AuthZ mutations can be retried reliably by a worker rather than being dropped.crates/frontend/src/components/authz.rs (1)
42-47:⚠️ Potential issue | 🟠 MajorUse a stable row key for
Table.Line 46 still passes
key_column="". Because the row maps built in the viewers do not carry a unique key, every refetch ends up reconciling rows with the same empty key, which can produce stale/misplaced DOM updates. Add a synthetic key field when building each row and pointkey_columnat that field instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/authz.rs` around lines 42 - 47, The Table is being rendered with key_column="" which causes unstable reconciliation; when constructing the row maps used by the viewers, add a synthetic unique key field (e.g., generate a UUID or incrementing counter) on each row entry and then pass that field name as the key_column prop to the Table instead of an empty string; update the place inside the resource.get().map(...) where rows are created and the view! { <Table ... /> } call (referenced by resource.get(), Table, and columns.get_value()) so the Table receives a stable per-row key field.
🧹 Nitpick comments (11)
openfga/docker-compose.yaml (2)
13-14: Consider removing external postgres port exposure.Port 5432 is exposed to the host, but if only the openfga service needs database access, this exposure is unnecessary and increases attack surface. Remove if external access isn't required.
Suggested fix
volumes: - openfga_pg_data:/var/lib/postgresql/data - ports: - - "5432:5432"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openfga/docker-compose.yaml` around lines 13 - 14, The postgres service is unnecessarily exposing port mapping "5432:5432" via the ports block; remove the "ports" entry (or at least the "5432:5432" mapping) from the docker-compose service so Postgres is only accessible on the Docker network (used by openfga) and not bound to the host, ensuring the openfga service uses the internal network connection; update any documentation or environment variables if they reference host:5432.
17-17: Pin the OpenFGA image to a specific version.Using
latesttag can lead to unexpected behavior when the upstream image is updated, breaking reproducibility and potentially introducing breaking changes silently.Suggested fix
- image: openfga/openfga:latest + image: openfga/openfga:v1.11.6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openfga/docker-compose.yaml` at line 17, Replace the floating image tag "openfga/openfga:latest" with a pinned semantic version (e.g., openfga/openfga:vX.Y.Z) to ensure reproducible deployments; update the image value in the docker-compose service that currently references openfga/openfga:latest and choose a specific release tag (or use a project-wide variable like OPENFGA_VERSION) and document the chosen version in the compose file or README so upgrades are explicit and controlled.crates/frontend/src/components/side_nav.rs (2)
358-361: Prefer deref coercion over reaching into.0here.These wrappers already coerce to
&str, so using&org.get()/&workspace.get()avoids coupling the nav code to the tuple-struct layout.♻️ Small cleanup
- let app_routes = - Signal::derive(move || create_routes(&org.get().0, &workspace.get().0)); + let app_routes = + Signal::derive(move || create_routes(&org.get(), &workspace.get()));- let app_routes = Signal::derive(move || create_org_routes(&org.get().0)); + let app_routes = Signal::derive(move || create_org_routes(&org.get()));Based on learnings:
WorkspaceandOrganisationIdimplementDeref, so functions expecting&strshould take&workspace/&orgdirectly instead of&workspace.0/&org.0.Also applies to: 464-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 358 - 361, The code is indexing into tuple structs with `.0` when building routes; instead rely on the Deref coercion of Workspace and OrganisationId instead of reaching into their internal tuple fields. Update the Signal::derive call that constructs app_routes (and the analogous usage at lines 464-466) to pass `&org.get()` and `&workspace.get()` (or `org.get()`/`workspace.get()` as appropriate for the create_routes signature) to create_routes instead of `&org.get().0` / `&workspace.get().0`, keeping the use_context::<Signal<Workspace>>() and use_context::<Signal<OrganisationId>>() lookups unchanged.
398-459: Extract the shared nav shell before these two components drift.
OrgNavComponentnow duplicates the header, collapse toggle, container classes, and route list rendering fromNavComponentalmost verbatim. Any future fix to collapse behavior, active-route matching, or accessibility has to be patched twice. A shared shell component with an optional workspace-selector slot would keep them aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 398 - 459, OrgNavComponent duplicates the header, collapse toggle, container classes, and route-list rendering from NavComponent; extract a shared shell component (e.g., NavShell) that accepts is_placeholder, app_routes: Signal<Vec<AppRoute>>, collapsed_rws: RwSignal<NavState>>, base/use_location (or reads location internally), and an optional workspace-selector slot so both OrgNavComponent and NavComponent can render only their differences. Move the header block (including the A title, the collapse toggle with on:click updating collapsed_rws), the nav container class logic, and the route mapping that creates <NavItem ...> (keeping the is_active logic using pathname.contains(&route.path)) into NavShell; then refactor OrgNavComponent and NavComponent to call NavShell and pass/slot any workspace selector UI, leaving NavItem and route rendering behavior unchanged.Dockerfile (1)
75-75: Don't keep the Casbin model on a repo-relative runtime path.This copy only works while the process starts from
/appandMODEL_CONF_PATHincrates/service_utils/src/middlewares/auth_z/casbin.rsstays hard-coded tocrates/service_utils/src/middlewares/auth_z/casbin/model.conf. Any different working directory or non-Docker packaging will fail to initialize the enforcer. Prefer a stable runtime path like/app/config/model.confor embed the model into the binary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 75, The Dockerfile currently copies the Casbin model to a repo-relative runtime path which breaks if the process working directory changes; instead copy it to a stable runtime location (e.g. /app/config/model.conf) and update the code that loads it (the MODEL_CONF_PATH constant in crates/service_utils/src/middlewares/auth_z/casbin.rs) to reference that stable path or embed the model in the binary; modify the Dockerfile COPY target to /app/config/model.conf and change MODEL_CONF_PATH (or the enforcer initialization code that reads the model) to use that absolute path (or load from embedded asset) so the enforcer initialization is no longer working-directory dependent.crates/superposition_types/src/api/authz/casbin.rs (1)
5-24: Consider addingDebugandClonederives for better observability.These request structs would benefit from
Debugfor logging/debugging andClonefor flexibility in handlers.♻️ Suggested improvement
-#[derive(Deserialize, Serialize)] +#[derive(Debug, Clone, Deserialize, Serialize)] pub struct PolicyRequest { pub sub: String, pub obj: Resource, pub act: String, pub attr: Option<String>, } -#[derive(Deserialize, Serialize)] +#[derive(Debug, Clone, Deserialize, Serialize)] pub struct GroupingPolicyRequest { pub user: String, pub role: String, } -#[derive(Deserialize, Serialize)] +#[derive(Debug, Clone, Deserialize, Serialize)] pub struct ActionGroupPolicyRequest { pub resource: Resource, pub action: String, pub action_group: String, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/api/authz/casbin.rs` around lines 5 - 24, Add Debug and Clone derives to the request structs to improve observability and handler flexibility: update the derive attributes on PolicyRequest, GroupingPolicyRequest, and ActionGroupPolicyRequest to include Debug and Clone (in addition to Deserialize and Serialize) so these types can be logged and cheaply cloned where needed.crates/superposition_derives/src/lib.rs (1)
507-517: Consider improving the error message whendeclare_resource!is missing.When
#[authorized]is used without an explicitresourceparameter anddeclare_resource!()was not called in the module, the compilation will fail with an unclear error about__DECLARED_RESOURCE__not being found. Consider adding acompile_error!wrapper or documentation to guide developers.The current fallback silently references
__DECLARED_RESOURCE__, which will cause a confusing compile error if the developer forgets to calldeclare_resource!(). The documentation (lines 432-440) helps, but an explicit compile-time check could improve DX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_derives/src/lib.rs` around lines 507 - 517, The current fallback for missing explicit resource emits code that references __DECLARED_RESOURCE__ and produces a confusing compile error; change the fallback in the branch that currently returns the two quote! blocks so that instead of emitting a raw reference to __DECLARED_RESOURCE__ you emit a clear compile-time failure via compile_error! with a message like "declare_resource!() not found in this module — either add declare_resource!() or pass resource = ... to #[authorized]". Update the generation for the resource() function and the expression fallback (the two quote! outputs returned in that else-branch) to use this compile_error! message so consumers of the generated resource() (and the expression used) get a friendly, actionable compile-time error rather than an undefined symbol error for __DECLARED_RESOURCE__.crates/frontend/src/types.rs (1)
51-56: Makeauth_zbackward-compatible for deserialization.
use_env()deserializes__APP_ENVSwithserde_json::from_str. If an older HTML payload or a fixture omits this new field, frontend bootstrap will panic. Adding a serde default here makes the rollout tolerant of missing values.Possible fix
#[derive(Serialize, Deserialize, Clone, Debug)] pub struct Envs { pub host: String, pub service_prefix: &'static str, + #[serde(default)] pub auth_z: bool, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/types.rs` around lines 51 - 56, The Envs struct's new auth_z field can be missing in older serialized payloads and will panic on deserialize; make it backward-compatible by providing a default for deserialization: annotate the auth_z field in the Envs struct with serde's default (so missing values become false) or implement/derive Default for Envs and use #[serde(default)] on the struct; update the auth_z default to false to preserve current behavior and ensure use_env()'s serde_json::from_str succeeds when the field is absent.crates/frontend/src/pages/authz.rs (2)
43-55: Same reactivity consideration applies here.
WorkspaceAuthzalso usesget_untracked()for bothorgandworkspacesignals. The same consideration about navigation and component remounting applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/authz.rs` around lines 43 - 55, WorkspaceAuthz currently calls get_untracked() on the org and workspace signals which prevents reactivity; replace those get_untracked() calls with tracked access so the AuthzEditor updates when the signals change. Specifically, in WorkspaceAuthz use org.get() and workspace.get() (or create a create_memo that derives the tuple) and pass that tracked value into the authz_scope prop (AuthzScope::Workspace(...)) or alternatively pass the signals themselves into AuthzEditor so it can react; update the references to org.get_untracked() and workspace.get_untracked() accordingly.
30-41: Consider reactivity implications ofget_untracked().Using
get_untracked()means theauthz_scopewon't update if the user navigates between organizations without a full page reload. If such navigation is possible within the SPA, theAuthzEditorwould display stale authorization data.If org/workspace navigation triggers a full remount of these components (via route change), this is fine. Otherwise, consider using
get()and makingauthz_scopereactive, or verify that navigation always causes component remount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/authz.rs` around lines 30 - 41, The component OrganisationAuthz currently reads the organisation id with org.get_untracked() and passes that to AuthzEditor as authz_scope, which prevents updates when the org Signal changes; change the code to use the reactive value (org.get()) or otherwise pass a reactive handle so AuthzEditor receives updates on org navigation—locate OrganisationAuthz, the org Signal<OrganisationId> usage, and the AuthzEditor prop authz_scope and replace get_untracked() with a reactive getter (or propagate the Signal) so the editor updates when the organisation changes.crates/frontend/src/app.rs (1)
130-140: Consider adding "settings" to reserved workspace names.The current workspace validation (
validate_workspace_name) only enforces length (≤25 characters) and alphanumeric format (^[a-zA-Z0-9]+$), with no reserved names list. This means a workspace named "settings" could be created, which could cause confusion given the/admin/settings/authzroute exists. While the routes won't collide (admin routes are separate from workspace routes), adding "settings" to a reserved names list on the backend would prevent this potential source of confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/app.rs` around lines 130 - 140, Add "settings" (and any other admin-reserved words) to a reserved-names check inside the workspace name validation so users cannot create ambiguous workspace names; update the validate_workspace_name function to consult a small list/set of reserved names (include "settings") and return a validation error when the proposed name matches any reserved entry, ensuring this check runs alongside the existing length and alphanumeric checks; reference the validate_workspace_name function and the admin route /admin/settings/authz (or AdminAuthz component) when implementing so the reserved list covers that conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/frontend/src/api.rs`:
- Around line 109-115: The matches! call is moving ownership of scope; change
the pattern to match a borrowed value so scope isn't moved — e.g. replace
matches!(scope, AuthzScope::Admin) with matches!(scope, &AuthzScope::Admin) (or
matches!(&scope, AuthzScope::Admin)) so scope remains usable for the subsequent
casbin_url_and_headers(..., scope) call; apply the same borrow fix to the other
matches! occurrence in add_domain_action_group.
In `@crates/frontend/src/components/authz.rs`:
- Around line 744-750: The current resource_action_map_resource uses
casbin::get_resource_action_map(...).await.unwrap_or_default(), which swallows
backend/network errors and turns failures into an empty action map; change the
closure passed to create_blocking_resource to return the Result instead of
calling unwrap_or_default (i.e., remove unwrap_or_default on
casbin::get_resource_action_map(authz_scope.get_value()).await), then update
consumers of resource_action_map_resource (or handle within the resource) to
surface errors (e.g., set an error state or show an alert/toast) when the Result
is Err so the UI displays a failure instead of an empty dropdown; ensure any
code expecting the map is adjusted to handle Result or use .map_err/.map to
convert errors into a visible error state.
In `@crates/frontend/src/components/side_nav.rs`:
- Around line 408-421: The placeholder branch currently only changes opacity but
keeps interactive elements in the accessibility tree; when is_placeholder is
true, replace the rendered org nav subtree with a non-interactive spacer or mark
the entire nav as inert/aria-hidden and remove pointer/focusability. Locate the
is_placeholder conditional and the nav rendered in the view! macro (referencing
placeholder_class and collapsed_rws) and either: 1) render a simple spacer
element with equivalent sizing and no interactive children, or 2) add attributes
to the nav subtree (aria-hidden="true", inert or pointer-events-none and
tabindex={-1} on interactive descendants) to remove it from the accessibility
tree and tab order on small screens; apply the same change to the other
placeholder branch around lines 468-474.
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 356-357: The MODEL_CONF_PATH constant is currently a relative path
and will fail when the binary is run outside the repo root; change its
definition to use a compile-time absolute path or embed the file: either build
the path with env!("CARGO_MANIFEST_DIR") combined with the relative path (update
the MODEL_CONF_PATH constant reference) or replace the external file usage by
embedding the model with include_str! and adjust functions that read the file
(e.g., wherever MODEL_CONF_PATH is passed into Casbin initialization) to accept
the embedded string instead of a file path.
- Around line 42-72: In enforcer_mut, ensure any in-memory mutations that fail
or don't persist are rolled back by reloading the persisted policies: before
calling f(&mut enforcer) take a snapshot/mark that allows rollback (e.g., clone
the enforcer or otherwise capture current policy state), then if f returns Err
or if enforcer.save_policy().await fails, call enforcer.load_policy().await to
restore the persisted policies and update last_policy_load_time accordingly;
only return the original error after the reload completes and log any errors
from the reload. Target symbols: enforcer_mut, f(&mut enforcer),
enforcer.save_policy(), enforcer.load_policy(), and last_policy_load_time.
In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs`:
- Around line 96-116: The validation that checks
action_map.get(&body.obj)...ok_or_else(...) currently produces a generic string
error that is later wrapped by unexpected_error! and returns a 500; change that
so invalid resource/action pairs produce a 4xx (bad request) instead of being
treated as unexpected server errors: replace the ok_or_else(...) string error
with an early Err that constructs a BadRequest (use your project's bad_request!
or equivalent error type) so the enforcer.add_policy(...) await chain does not
get mapped into unexpected_error! for this validation failure; ensure the error
originates from the validation block (the action_map check referencing body.obj
and body.act) and is not converted by the final .map_err(|e|
unexpected_error!(e)) so clients receive a proper 4xx.
In `@crates/service_utils/src/middlewares/auth_z/casbin/model.conf`:
- Line 19: The matcher 'm' currently contains a special-case branch (p.sub ==
"*" && r.obj == "workspace" && g3(r.obj + ":" + r.act, "read") == true) that
allows any policy with p.sub == "*" to bypass subject checks for workspace
reads; remove that branch from the matcher expression so workspace read
authorization must follow the normal subject-role checks (e.g., g(r.sub, p.sub,
p.dom) or g(r.sub, p.sub, r.dom)) and keep the existing object/action/group
functions (g2/g3) intact; update the matcher 'm' in model.conf by deleting the
p.sub == "*" && r.obj == "workspace" && g3(...) clause so wildcard policies no
longer grant global workspace discovery.
In `@crates/service_utils/src/registry.rs`:
- Around line 81-87: The test test_grouping_by_resource currently uses a
tautological assertion; replace it with a deterministic check: either assert
that ActionRegistry::group_by_resource() returns an empty grouping when no
actions are registered (e.g.,
assert_eq!(ActionRegistry::group_by_resource().len(), 0)) or, better, register a
known dummy action via the same macro used for real actions and then assert that
ActionRegistry::group_by_resource() contains the expected resource key and
action entry (use ActionRegistry::group_by_resource() and assert on its
keys/values). If you cannot register dummy actions in unit tests yet, remove the
test until an integration test can exercise real inventory entries.
In `@crates/service_utils/tests/action_registry_test.rs`:
- Around line 8-26: The test test_actions_are_registered currently iterates
ActionRegistry::get_all() without asserting it's non-empty, allowing false
positives; update the test to assert that ActionRegistry::get_all() returns a
non-empty Vec (or assert presence of a specific known action from service_utils)
before the per-item checks, i.e., add an assert!(!actions.is_empty(), "...") or
an assert that actions.iter().any(|a| a.action_name == "<known_action>") prior
to the for loop (also apply the same non-empty/presence assertion to the other
similar test covering lines 29-41).
In `@crates/superposition/src/workspace/handlers.rs`:
- Around line 232-245: The update currently uses .execute(transaction_conn) and
then reads the workspace after committing, which allows a race; change the
diesel::update(workspaces::table)...set((request,
workspaces::last_modified_by.eq(...), workspaces::last_modified_at.eq(...)))
call to use .get_result::<Workspace>(transaction_conn) so the updated Workspace
row is returned inside the same transaction, and modify the function to return
that updated Workspace along with old_admin_email; apply the same replacement
for the second update path around the workspaces update at lines 251-257 so both
branches return the freshly updated row from the transaction rather than reading
after commit.
In `@docs/api_examples.md`:
- Line 29: The docs currently mention key_match3 and imply path-style resource
matching; update the text to describe the actual matcher in model.conf
(globMatch combined with exact Resource.obj equality or wildcard '*' semantics)
instead of referencing key_match3. Edit the sentence that mentions `key_match3`
to describe `globMatch` and the `obj` equality/`*` behavior, clarify that
Resource.obj is matched via glob patterns not path-segment key matching, and
provide a brief example showing a glob pattern and a literal `*` match for
Resource.obj to reflect the real model.
In `@openfga/docker-compose.yaml`:
- Around line 19-20: The compose uses depends_on: - postgres which only orders
startup but doesn't guarantee Postgres is ready; add a Docker HEALTHCHECK to the
postgres service (e.g., using pg_isready) and update the OpenFGA service's
depends_on to wait for postgres' health (use depends_on: postgres: condition:
service_healthy or the equivalent for your compose version) so OpenFGA only
attempts connection after postgres is healthy; update symbols: postgres service,
healthcheck, depends_on, service_healthy, and OpenFGA service.
---
Duplicate comments:
In `@crates/frontend/src/components/authz.rs`:
- Around line 42-47: The Table is being rendered with key_column="" which causes
unstable reconciliation; when constructing the row maps used by the viewers, add
a synthetic unique key field (e.g., generate a UUID or incrementing counter) on
each row entry and then pass that field name as the key_column prop to the Table
instead of an empty string; update the place inside the resource.get().map(...)
where rows are created and the view! { <Table ... /> } call (referenced by
resource.get(), Table, and columns.get_value()) so the Table receives a stable
per-row key field.
In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql`:
- Around line 1-106: Casbin bootstrap SQL (casbin.sql) is never executed before
the enforcer is created, so CasbinPolicyEngine::new (the function that builds
DieselAdapter and Enforcer) may start against a fresh DB without the casbin_rule
table or seed policies; update startup/migration wiring so the SQL runs before
creating the adapter: either (A) execute the casbin.sql contents (CREATE TABLE +
INSERT ... ON CONFLICT DO NOTHING) from CasbinPolicyEngine::new before
constructing DieselAdapter/Enforcer, or (B) move the idempotent table creation
and seed logic into a migration step that is guaranteed to run prior to
CasbinPolicyEngine::new; reference CasbinPolicyEngine::new, DieselAdapter,
Enforcer and casbin_rule when making the change so the table and seed policies
exist on first startup.
In `@crates/superposition/src/organisation/handlers.rs`:
- Around line 64-76: The code currently persists new_org via diesel::insert_into
then calls authz_handler.on_org_creation(new_org.id, new_org.admin_email) but
ignores both Err(_) and Ok(false), allowing a DB record without successful AuthZ
bootstrap; change this so the DB write and AuthZ bootstrap are atomic: perform
the insert and the on_org_creation call inside a recoverable flow (e.g., a DB
transaction or an explicit outbox/compensation flow) so that if
authz_handler.on_org_creation returns Err or Ok(false) you either roll back the
DB insert (abort the transaction and return an error) or enqueue an outbox event
and return an error status until reconciliation succeeds; locate the logic
around new_org insertion and authz_handler.on_org_creation and make failure
pathways return an error instead of just logging.
In `@crates/superposition/src/workspace/handlers.rs`:
- Around line 224-228: The query is comparing workspaces::workspace_schema_name
to the plain workspace_name, but stored rows use the "<org>_<workspace>" schema
name; build the correct schema string (e.g. let schema = format!("{}_{}",
org_id.0, workspace_name)) and use workspace_schema_name.eq(&schema) in the
filter for the workspaces::dsl::workspaces query that selects
workspaces::workspace_admin_email and calls get_result(transaction_conn).
- Around line 175-184: The current fire-and-forget call to
authz_handler.on_workspace_creation (invoked with workspace_schema_name and
created_workspace.workspace_admin_email) is brittle because failures are only
logged; instead make the AuthZ sync durable by recording a retryable
outbox/reconciliation record inside the same transaction that creates the
workspace (e.g., create a WorkspaceAuthzOutbox entry referencing workspace
id/schema and admin email) or enqueue a durable job/message that
guaranteed-delivery workers will process, and remove the best-effort
.await.map_err logging pattern; ensure the transaction commits both the
workspace row and the outbox record (or pushes the durable job) so failed AuthZ
mutations can be retried reliably by a worker rather than being dropped.
---
Nitpick comments:
In `@crates/frontend/src/app.rs`:
- Around line 130-140: Add "settings" (and any other admin-reserved words) to a
reserved-names check inside the workspace name validation so users cannot create
ambiguous workspace names; update the validate_workspace_name function to
consult a small list/set of reserved names (include "settings") and return a
validation error when the proposed name matches any reserved entry, ensuring
this check runs alongside the existing length and alphanumeric checks; reference
the validate_workspace_name function and the admin route /admin/settings/authz
(or AdminAuthz component) when implementing so the reserved list covers that
conflict.
In `@crates/frontend/src/components/side_nav.rs`:
- Around line 358-361: The code is indexing into tuple structs with `.0` when
building routes; instead rely on the Deref coercion of Workspace and
OrganisationId instead of reaching into their internal tuple fields. Update the
Signal::derive call that constructs app_routes (and the analogous usage at lines
464-466) to pass `&org.get()` and `&workspace.get()` (or
`org.get()`/`workspace.get()` as appropriate for the create_routes signature) to
create_routes instead of `&org.get().0` / `&workspace.get().0`, keeping the
use_context::<Signal<Workspace>>() and use_context::<Signal<OrganisationId>>()
lookups unchanged.
- Around line 398-459: OrgNavComponent duplicates the header, collapse toggle,
container classes, and route-list rendering from NavComponent; extract a shared
shell component (e.g., NavShell) that accepts is_placeholder, app_routes:
Signal<Vec<AppRoute>>, collapsed_rws: RwSignal<NavState>>, base/use_location (or
reads location internally), and an optional workspace-selector slot so both
OrgNavComponent and NavComponent can render only their differences. Move the
header block (including the A title, the collapse toggle with on:click updating
collapsed_rws), the nav container class logic, and the route mapping that
creates <NavItem ...> (keeping the is_active logic using
pathname.contains(&route.path)) into NavShell; then refactor OrgNavComponent and
NavComponent to call NavShell and pass/slot any workspace selector UI, leaving
NavItem and route rendering behavior unchanged.
In `@crates/frontend/src/pages/authz.rs`:
- Around line 43-55: WorkspaceAuthz currently calls get_untracked() on the org
and workspace signals which prevents reactivity; replace those get_untracked()
calls with tracked access so the AuthzEditor updates when the signals change.
Specifically, in WorkspaceAuthz use org.get() and workspace.get() (or create a
create_memo that derives the tuple) and pass that tracked value into the
authz_scope prop (AuthzScope::Workspace(...)) or alternatively pass the signals
themselves into AuthzEditor so it can react; update the references to
org.get_untracked() and workspace.get_untracked() accordingly.
- Around line 30-41: The component OrganisationAuthz currently reads the
organisation id with org.get_untracked() and passes that to AuthzEditor as
authz_scope, which prevents updates when the org Signal changes; change the code
to use the reactive value (org.get()) or otherwise pass a reactive handle so
AuthzEditor receives updates on org navigation—locate OrganisationAuthz, the org
Signal<OrganisationId> usage, and the AuthzEditor prop authz_scope and replace
get_untracked() with a reactive getter (or propagate the Signal) so the editor
updates when the organisation changes.
In `@crates/frontend/src/types.rs`:
- Around line 51-56: The Envs struct's new auth_z field can be missing in older
serialized payloads and will panic on deserialize; make it backward-compatible
by providing a default for deserialization: annotate the auth_z field in the
Envs struct with serde's default (so missing values become false) or
implement/derive Default for Envs and use #[serde(default)] on the struct;
update the auth_z default to false to preserve current behavior and ensure
use_env()'s serde_json::from_str succeeds when the field is absent.
In `@crates/superposition_derives/src/lib.rs`:
- Around line 507-517: The current fallback for missing explicit resource emits
code that references __DECLARED_RESOURCE__ and produces a confusing compile
error; change the fallback in the branch that currently returns the two quote!
blocks so that instead of emitting a raw reference to __DECLARED_RESOURCE__ you
emit a clear compile-time failure via compile_error! with a message like
"declare_resource!() not found in this module — either add declare_resource!()
or pass resource = ... to #[authorized]". Update the generation for the
resource() function and the expression fallback (the two quote! outputs returned
in that else-branch) to use this compile_error! message so consumers of the
generated resource() (and the expression used) get a friendly, actionable
compile-time error rather than an undefined symbol error for
__DECLARED_RESOURCE__.
In `@crates/superposition_types/src/api/authz/casbin.rs`:
- Around line 5-24: Add Debug and Clone derives to the request structs to
improve observability and handler flexibility: update the derive attributes on
PolicyRequest, GroupingPolicyRequest, and ActionGroupPolicyRequest to include
Debug and Clone (in addition to Deserialize and Serialize) so these types can be
logged and cheaply cloned where needed.
In `@Dockerfile`:
- Line 75: The Dockerfile currently copies the Casbin model to a repo-relative
runtime path which breaks if the process working directory changes; instead copy
it to a stable runtime location (e.g. /app/config/model.conf) and update the
code that loads it (the MODEL_CONF_PATH constant in
crates/service_utils/src/middlewares/auth_z/casbin.rs) to reference that stable
path or embed the model in the binary; modify the Dockerfile COPY target to
/app/config/model.conf and change MODEL_CONF_PATH (or the enforcer
initialization code that reads the model) to use that absolute path (or load
from embedded asset) so the enforcer initialization is no longer
working-directory dependent.
In `@openfga/docker-compose.yaml`:
- Around line 13-14: The postgres service is unnecessarily exposing port mapping
"5432:5432" via the ports block; remove the "ports" entry (or at least the
"5432:5432" mapping) from the docker-compose service so Postgres is only
accessible on the Docker network (used by openfga) and not bound to the host,
ensuring the openfga service uses the internal network connection; update any
documentation or environment variables if they reference host:5432.
- Line 17: Replace the floating image tag "openfga/openfga:latest" with a pinned
semantic version (e.g., openfga/openfga:vX.Y.Z) to ensure reproducible
deployments; update the image value in the docker-compose service that currently
references openfga/openfga:latest and choose a specific release tag (or use a
project-wide variable like OPENFGA_VERSION) and document the chosen version in
the compose file or README so upgrades are explicit and controlled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 173cf507-e1b9-4a98-b13b-24ba8f9f5496
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.env.exampleCargo.tomlDockerfilecrates/cac_client/Cargo.tomlcrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/audit_log/handlers.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/experimentation_client/Cargo.tomlcrates/experimentation_platform/Cargo.tomlcrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/authz.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/hoc/layout.rscrates/frontend/src/pages.rscrates/frontend/src/pages/authz.rscrates/frontend/src/pages/organisations.rscrates/frontend/src/types.rscrates/service_utils/Cargo.tomlcrates/service_utils/examples/list_actions.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/casbin.rscrates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlcrates/service_utils/src/middlewares/auth_z/casbin/handlers.rscrates/service_utils/src/middlewares/auth_z/casbin/model.confcrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/registry.rscrates/service_utils/tests/action_registry_test.rscrates/superposition/Cargo.tomlcrates/superposition/examples/generate_actions_registry.rscrates/superposition/src/main.rscrates/superposition/src/organisation/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_core/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_derives/src/lib.rscrates/superposition_provider/Cargo.tomlcrates/superposition_types/src/api.rscrates/superposition_types/src/api/authz.rscrates/superposition_types/src/api/authz/casbin.rscrates/superposition_types/src/api/workspace.rscrates/superposition_types/src/lib.rsdocs/api_examples.mdopenfga/docker-compose.yamlsmithy/models/workspace.smithy
💤 Files with no reviewable changes (1)
- smithy/models/workspace.smithy
Comment on lines
+42
to
+72
| pub async fn enforcer_mut<T, F>(&self, f: F) -> Result<T, String> | ||
| where | ||
| F: AsyncFnOnce(&mut Enforcer) -> Result<T, String>, | ||
| { | ||
| let mut enforcer = self.enforcer.write().await; | ||
|
|
||
| // Check if refresh is needed while holding the write lock | ||
| let now = Utc::now(); | ||
| let last_load_time = self.last_policy_load_time.read().await; | ||
| let needs_refresh = now.signed_duration_since(*last_load_time).num_seconds() | ||
| >= self.policy_refresh_interval as i64; | ||
| drop(last_load_time); | ||
|
|
||
| if needs_refresh { | ||
| enforcer.load_policy().await.map_err(|e| { | ||
| log::error!("Failed to refresh Casbin policies: {}", e); | ||
| e.to_string() | ||
| })?; | ||
|
|
||
| let mut last_load_time = self.last_policy_load_time.write().await; | ||
| *last_load_time = Utc::now(); | ||
| } | ||
|
|
||
| let resp = f(&mut enforcer).await?; | ||
|
|
||
| enforcer.save_policy().await.map_err(|e| { | ||
| log::error!("Failed to save Casbin policies: {}", e); | ||
| e.to_string() | ||
| })?; | ||
|
|
||
| Ok(resp) |
Contributor
There was a problem hiding this comment.
Reload persisted policies on every mutation failure path.
f(&mut enforcer) mutates the live enforcer in place. If the closure returns Err after a partial update, or if save_policy() fails, this method exits with the mutated in-memory policy set still active on this node even though it never reached the database. That can temporarily grant/deny permissions incorrectly until the next refresh.
🔁 Suggested rollback pattern
pub async fn enforcer_mut<T, F>(&self, f: F) -> Result<T, String>
where
F: AsyncFnOnce(&mut Enforcer) -> Result<T, String>,
{
let mut enforcer = self.enforcer.write().await;
@@
- let resp = f(&mut enforcer).await?;
-
- enforcer.save_policy().await.map_err(|e| {
- log::error!("Failed to save Casbin policies: {}", e);
- e.to_string()
- })?;
+ let resp = match f(&mut enforcer).await {
+ Ok(resp) => resp,
+ Err(e) => {
+ if let Err(reload_err) = enforcer.load_policy().await {
+ log::error!(
+ "Failed to reload Casbin policies after mutation error: {}",
+ reload_err
+ );
+ }
+ return Err(e);
+ }
+ };
+
+ if let Err(e) = enforcer.save_policy().await {
+ log::error!("Failed to save Casbin policies: {}", e);
+ if let Err(reload_err) = enforcer.load_policy().await {
+ log::error!(
+ "Failed to reload Casbin policies after save failure: {}",
+ reload_err
+ );
+ }
+ return Err(e.to_string());
+ }
Ok(resp)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs` around lines 42 - 72,
In enforcer_mut, ensure any in-memory mutations that fail or don't persist are
rolled back by reloading the persisted policies: before calling f(&mut enforcer)
take a snapshot/mark that allows rollback (e.g., clone the enforcer or otherwise
capture current policy state), then if f returns Err or if
enforcer.save_policy().await fails, call enforcer.load_policy().await to restore
the persisted policies and update last_policy_load_time accordingly; only return
the original error after the reload completes and log any errors from the
reload. Target symbols: enforcer_mut, f(&mut enforcer), enforcer.save_policy(),
enforcer.load_policy(), and last_policy_load_time.
Comment on lines
+232
to
+245
| diesel::update(workspaces::table) | ||
| .filter(workspaces::organisation_id.eq(&org_id.0)) | ||
| .filter(workspaces::workspace_name.eq(workspace_name)) | ||
| .set(( | ||
| request, | ||
| workspaces::last_modified_by.eq(user.get_email()), | ||
| workspaces::last_modified_at.eq(timestamp), | ||
| )) | ||
| .execute(transaction_conn) | ||
| .map_err(|err| { | ||
| log::error!("failed to update workspace with error: {}", err); | ||
| err | ||
| })?; | ||
| Ok(old_admin_email) |
Contributor
There was a problem hiding this comment.
Return the updated workspace from the transaction.
This path updates the row, commits, and only then derives the response/AuthZ payload from a fresh read. A concurrent update in that gap can make this handler return and sync a different admin/value than the one this request wrote. Use get_result::<Workspace>(transaction_conn) and return the updated row together with old_admin_email from the transaction.
Suggested shape
- let old_email =
- conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {
+ let (old_email, workspace) =
+ conn.transaction::<_, superposition::AppError, _>(|transaction_conn| {
let old_admin_email = match &request.workspace_admin_email {
None => None,
Some(_) => {
let email = workspaces::dsl::workspaces
.filter(workspaces::organisation_id.eq(&org_id.0))
- .filter(workspaces::workspace_schema_name.eq(&workspace_name))
+ .filter(workspaces::workspace_name.eq(&workspace_name))
.select(workspaces::workspace_admin_email)
.get_result(transaction_conn)?;
Some(email)
}
};
- diesel::update(workspaces::table)
+ let workspace = diesel::update(workspaces::table)
.filter(workspaces::organisation_id.eq(&org_id.0))
- .filter(workspaces::workspace_name.eq(workspace_name))
+ .filter(workspaces::workspace_name.eq(&workspace_name))
.set((
request,
workspaces::last_modified_by.eq(user.get_email()),
workspaces::last_modified_at.eq(timestamp),
))
- .execute(transaction_conn)
+ .get_result::<Workspace>(transaction_conn)
.map_err(|err| {
log::error!("failed to update workspace with error: {}", err);
err
})?;
- Ok(old_admin_email)
+ Ok((old_admin_email, workspace))
})?;
-
- let workspace = get_workspace(&schema_name, &mut conn)?;Also applies to: 251-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/superposition/src/workspace/handlers.rs` around lines 232 - 245, The
update currently uses .execute(transaction_conn) and then reads the workspace
after committing, which allows a race; change the
diesel::update(workspaces::table)...set((request,
workspaces::last_modified_by.eq(...), workspaces::last_modified_at.eq(...)))
call to use .get_result::<Workspace>(transaction_conn) so the updated Workspace
row is returned inside the same transaction, and modify the function to return
that updated Workspace along with old_admin_email; apply the same replacement
for the second update path around the workspaces update at lines 251-257 so both
branches return the freshly updated row from the transaction rather than reading
after commit.
Datron
reviewed
Mar 10, 2026
Comment on lines
+40
to
+48
| ### Env(s) for Casbin AuthZ | ||
| # AUTH_Z_PROVIDER=CASBIN | ||
| # CASBIN_DB_USER=postgres | ||
| # CASBIN_DB_PASSWORD=docker | ||
| # CASBIN_DB_HOST=localhost:5432 | ||
| # CASBIN_DB_NAME=config | ||
| # CASBIN_DB_POOL_SIZE=3 | ||
| # CASBIN_POLICY_REFRESH_INTERVAL=60 # in seconds, default is 60 seconds | ||
| # ROOT_ADMIN_EMAIL=user@superposition.io |
Collaborator
There was a problem hiding this comment.
Need a follow up PR documenting the casbin setup in docusaurus
Datron
reviewed
Mar 10, 2026
| } | ||
|
|
||
| // Register action in inventory for runtime introspection | ||
| inventory::submit! { |
Comment on lines
+4
to
+19
| CREATE TABLE casbin_rule ( | ||
| id SERIAL PRIMARY KEY, | ||
| ptype VARCHAR NOT NULL, | ||
| v0 VARCHAR NOT NULL, | ||
| v1 VARCHAR NOT NULL, | ||
| v2 VARCHAR NOT NULL, | ||
| v3 VARCHAR NOT NULL, | ||
| v4 VARCHAR NOT NULL, | ||
| v5 VARCHAR NOT NULL, | ||
| CONSTRAINT unique_key_diesel_adapter UNIQUE(ptype, v0, v1, v2, v3, v4, v5) | ||
| ); | ||
| EXCEPTION | ||
| WHEN duplicate_table THEN | ||
| -- Table already exists; do not insert the root admin mapping. | ||
| NULL; | ||
| END; |
Collaborator
There was a problem hiding this comment.
Would CREATE TABLE IF NOT EXISTS work here? Also should we index few columns like ptype and v0
Collaborator
Author
There was a problem hiding this comment.
here I had copied whatever casbin's diesel-adapter is doing already internally
if the DB user access to create table's and if the table is not already there then the plugin auto creates using the above query itself - I can update it for our case to use CREATE TABLE IF NOT EXISTS
but for indexing it does make sense, because the casbin crate does all filtering and other operations in memory it does not do any DB level query
| body.sub, | ||
| domain.to_string(), | ||
| body.obj.to_string(), | ||
| body.act.get_name().to_string(), |
| body.sub, | ||
| domain.to_string(), | ||
| body.obj.to_string(), | ||
| body.act.get_name().to_string(), |
| let data = data.try_get_casbin_policy_engine()?; | ||
| let body = body.into_inner(); | ||
| let removed = data | ||
| .enforcer_mut(async |enforcer| { |
Collaborator
There was a problem hiding this comment.
nitpick: We should see if modifying enforcer to return the deleted type can be done so be can maintain our current API behaviour in these handlers as well
Collaborator
Author
There was a problem hiding this comment.
@Datron casbin's delete functions only return true/false for the operation
Datron
approved these changes
Mar 10, 2026
sauraww
reviewed
Mar 10, 2026
| >= self.policy_refresh_interval as i64 | ||
| }; | ||
|
|
||
| if needs_refresh { |
Collaborator
There was a problem hiding this comment.
Redundant DB calls can go at the time of check :
Time ──────────────────────────────────────────────────────────────────►
Task A Task B
────── ──────
reads last_policy_load_time
→ needs_refresh = true ✅
reads last_policy_load_time
→ needs_refresh = true ✅
acquires write lock 🔒
loads policies from DB waiting for write lock... ⏳
updates last_load_time
runs f(&enforcer)
releases write lock 🔓
acquires write lock 🔒
loads policies from DB AGAIN 🔄 ← redundant!
updates last_load_time
runs f(&enforcer)
releases write lock 🔓
we can add a re-check inside if refresh to avoid this:
// maybe another task already refreshed while we waited
let last_load_time = self.last_policy_load_time.read().await;
let still_needs_refresh = Utc::now()
.signed_duration_since(*last_load_time)
.num_seconds() >= self.policy_refresh_interval as i64;
drop(last_load_time);
Collaborator
Author
There was a problem hiding this comment.
check this resolved comment from code rabbit:
#908 (comment)
sauraww
reviewed
Mar 10, 2026
| // Notify the AuthZHandler about the new organisation creation | ||
| let _ = authz_handler | ||
| .on_org_creation(new_org.id.clone(), new_org.admin_email.clone()) | ||
| .await |
Collaborator
There was a problem hiding this comment.
Is the clone required here ?
Collaborator
Author
There was a problem hiding this comment.
yes, new_org is returned as response
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs
Dismissed
Show dismissed
Hide dismissed
crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs
Dismissed
Show dismissed
Hide dismissed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/src/experiment_groups.test.ts (3)
895-905:⚠️ Potential issue | 🟠 MajorMissing
awaiton rejection assertion.🐛 Proposed fix
- expect( + await expect( superpositionClient.send( new DeleteExperimentGroupCommand({ workspace_id: ENV.workspace_id, org_id: ENV.org_id, id: "22", }), ), ).rejects.toThrow(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 895 - 905, The test assertion is missing an await when asserting a rejected promise from superpositionClient.send(new DeleteExperimentGroupCommand(...)); update the assertion to await the expect(...) so the test properly waits for the rejection (i.e., use await expect(superpositionClient.send(...)).rejects.toThrow("No records found. Please refine or correct your search parameters") ), or alternatively return the promise from the test; locate the call to superpositionClient.send and the DeleteExperimentGroupCommand invocation in tests/src/experiment_groups.test.ts to apply the change.
584-594:⚠️ Potential issue | 🟠 MajorMissing
awaiton rejection assertion.🐛 Proposed fix
- expect( + await expect( superpositionClient.send( new GetExperimentGroupCommand({ workspace_id: ENV.workspace_id, org_id: ENV.org_id, id: "123", }), ), ).rejects.toThrow(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 584 - 594, The test is asserting a rejected promise but missing an await on the rejection assertion; update the test so the rejection matcher is awaited — add await before expect(superpositionClient.send(new GetExperimentGroupCommand({...}))).rejects.toThrow(...) — referencing the superpositionClient.send call and GetExperimentGroupCommand used in the assertion to ensure the promise rejection is correctly awaited.
881-891:⚠️ Potential issue | 🟠 MajorMissing
awaiton rejection assertion.🐛 Proposed fix
- expect( + await expect( superpositionClient.send( new DeleteExperimentGroupCommand({ workspace_id: ENV.workspace_id, org_id: ENV.org_id, id: tempGroupId, }), ), ).rejects.toThrow(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 881 - 891, The rejection assertion is missing an await. Change the line using expect(superpositionClient.send(new DeleteExperimentGroupCommand({ workspace_id: ENV.workspace_id, org_id: ENV.org_id, id: tempGroupId }))).rejects.toThrow(...) to be awaited: await expect(superpositionClient.send(...)).rejects.toThrow(...); also ensure the enclosing test function is declared async so the await is valid. This references DeleteExperimentGroupCommand, tempGroupId, and superpositionClient.send in the failing test.tests/src/context.test.ts (2)
253-269:⚠️ Potential issue | 🟠 MajorOnly clean up dimensions this suite actually created.
The duplicate-key path updates an existing dimension and still pushes it into
createdDimensions.afterAllwill then delete that pre-existing dimension from the shared workspace, which can break other suites or developers using the same env.Suggested fix
if (e.message && e.message.includes("duplicate key")) { console.log( `Dimension ${dim.dimension} already exists, updating dimension`, ); const cmd = new UpdateDimensionCommand({ workspace_id: testWorkspaceId, org_id: testOrgId, dimension: dim.dimension, schema: dim.schema, position: dim.position, description: dim.description, change_reason: `Update ${dim.dimension} dimension for tests`, }); await client.send(cmd); - // Track created dimension - createdDimensions.push(dim.dimension); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/context.test.ts` around lines 253 - 269, The test currently pushes dimension names into createdDimensions even when the duplicate-key branch updates an existing dimension; change the logic so createdDimensions only includes dimensions that this suite actually created: in the code handling create vs duplicate (the block using UpdateDimensionCommand and checking e.message.includes("duplicate key")), do not push dim.dimension into createdDimensions on the duplicate-key path—only push when the initial creation path succeeds (the successful create operation) or explicitly detect creation success before adding to the createdDimensions array; update the try/catch flow around the create call and the use of createdDimensions to ensure afterAll only deletes dimensions this test created.
490-509:⚠️ Potential issue | 🟠 MajorAwait the rejection assertions.
These negative-path tests are flaky due to un-awaited rejection handlers. Tests with un-awaited
.rejects.toThrow(...)assertions allow Bun to finish before the matcher runs, causing false passes. The try/catch pattern at lines 490-509 silently passes if the request unexpectedly succeeds.Suggested fix
For lines 490-509:
- test("should reject empty context objects", async () => { - try { - const unwrappedCmd = new CreateContextCommand({ - workspace_id: testWorkspaceId, - org_id: testOrgId, - request: { - override: { - key1: "empty-context-value", - }, - context: {}, - description: "Empty context", - change_reason: "Testing empty context validation", - }, - }); - await client.send(unwrappedCmd); - } catch (err: any) { - expect(err.$response.body).toMatch( - /Context should not be empty/i, - ); - } - }); + test("should reject empty context objects", async () => { + const unwrappedCmd = new CreateContextCommand({ + workspace_id: testWorkspaceId, + org_id: testOrgId, + request: { + override: { + key1: "empty-context-value", + }, + context: {}, + description: "Empty context", + change_reason: "Testing empty context validation", + }, + }); + + await expect(client.send(unwrappedCmd)).rejects.toThrow( + /Context should not be empty/i, + ); + });For lines 563-565, 585-587, 609-611, 1045-1047, 1094-1096, 1235-1237, add
await:- expect(client.send(cmd)).rejects.toThrow( + await expect(client.send(cmd)).rejects.toThrow(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/context.test.ts` around lines 490 - 509, The failing tests are not awaiting their rejection assertions, causing flaky passes; update the negative-path tests (e.g., the "should reject empty context objects" test that constructs a CreateContextCommand and calls client.send) to await the rejection matcher instead of relying on try/catch — e.g. replace the try/catch with await expect(client.send(unwrappedCmd)).rejects.toMatch(/Context should not be empty/i) or, if keeping the try/catch style, ensure the thrown promise is awaited and the expect runs after await; also add the missing await before any other .rejects.* assertions referenced (the other occurrences called out in the comment) so all client.send rejection assertions are awaited.docs/docs/api/Superposition.openapi.json (1)
5606-5613:⚠️ Potential issue | 🟠 MajorKeep the
UpdateWorkspacebody required.Making individual fields optional is fine, but dropping
requestBody.requiredchanges the contract from "partial update payload" to "no payload at all". The backend handler bindsJson<UpdateWorkspaceRequest>(non-optional), which requires a request body. Removingrequired: truecreates a mismatch: generated SDKs will allow omitting the body, but the server rejects such requests.Suggested fix
"requestBody": { "content": { "application/json": { "schema": { "$ref": "#/components/schemas/UpdateWorkspaceRequestContent" } } - } + }, + "required": true },Also applies to: 11271-11303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/api/Superposition.openapi.json` around lines 5606 - 5613, The OpenAPI operation for UpdateWorkspace currently omits requestBody.required, which lets generated clients omit the body while the server expects Json<UpdateWorkspaceRequest>; set requestBody.required: true for the operation that references the schema "#/components/schemas/UpdateWorkspaceRequestContent" so the contract requires a body, and apply the same change to the other occurrence mentioned (the block around lines 11271-11303) to keep client/server behavior consistent with the backend's Json<UpdateWorkspaceRequest> binding.
♻️ Duplicate comments (4)
crates/superposition/src/organisation/handlers.rs (1)
64-76:⚠️ Potential issue | 🔴 CriticalDon't return success when org AuthZ provisioning fails.
The row is already committed before
on_org_creationruns, and this code drops any error from that call. That means the API can return 200 with an organisation that has no corresponding AuthZ state, leaving the new org admin unable to access it. Please make provisioning part of a durable workflow instead of swallowing the failure—e.g. queue/reconcile it in the same transaction, or keep the org in a provisioning state until the hook succeeds.crates/superposition/src/workspace/handlers.rs (2)
219-249:⚠️ Potential issue | 🟠 MajorReturn the updated workspace from the transaction.
This branch commits with
.execute()and then does a freshget_workspace()read. A concurrent patch in that gap can make the response andon_workspace_admin_updatepick up aworkspace_admin_emailthat this request never wrote. ReturnWorkspacefrom the update query inside the transaction and reuse that value after commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/workspace/handlers.rs` around lines 219 - 249, The transaction currently runs diesel::update(...).execute(...) then separately calls get_workspace(), which allows a race—modify the transaction closure (the conn.transaction block) to perform the update and return the updated Workspace directly by replacing the execute() call with an update that uses get_result::<Workspace>(transaction_conn) (or the equivalent RETURNING get_result call) so the transaction returns Ok(updated_workspace) and assign that result to workspace (instead of calling get_workspace()); then use that workspace for put_workspace_in_redis and subsequent logic so the post-commit value is the one actually written within the transaction.
175-184:⚠️ Potential issue | 🔴 CriticalMake the AuthZ sync retryable instead of best-effort.
Both hooks run after the transaction and drop failures to logs. If
on_workspace_admin_updatemisses, the DB points at the new admin while Casbin can still authorize the old one; ifon_workspace_creationmisses, the new workspace admin never gets seeded. Persist a reconciliation/outbox record inside the transaction, or move this sync behind a durable retry path.Also applies to: 251-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/workspace/handlers.rs` around lines 175 - 184, The current post-transaction calls to authz_handler.on_workspace_creation(...) and authz_handler.on_workspace_admin_update(...) are best-effort and just log failures; instead make these operations durable by persisting an outbox/reconciliation record inside the same transaction (e.g., create a "pending_authz_sync" row referencing created_workspace.id and the workspace_admin_email/schema) or by enqueuing a durable retry job before committing, and have the worker/process pick up and call authz_handler until success; modify the handlers in handlers.rs to stop swallowing errors—either return an error from the workspace create/update function when enqueueing fails, or persist the outbox record and remove the current log-only .map_err(...) drop so the retry mechanism can handle retries for on_workspace_creation and on_workspace_admin_update.crates/service_utils/src/middlewares/auth_z/casbin.rs (1)
45-72:⚠️ Potential issue | 🔴 CriticalRollback the live enforcer on mutation/save failure.
f(&mut enforcer)mutates the in-memoryEnforcerbeforesave_policy(). If the closure errors after a partial change, orsave_policy()fails, this node keeps enforcing state that never reached the DB. Reload persisted policy on both failure paths before returning.Possible fix shape
- let resp = f(&mut enforcer).await?; - - enforcer - .save_policy() - .await - .map_err(|e| unexpected_error!("Failed to save Casbin policies: {}", e))?; + let resp = match f(&mut enforcer).await { + Ok(resp) => resp, + Err(e) => { + let _ = enforcer.load_policy().await; + return Err(e); + } + }; + + if let Err(e) = enforcer.save_policy().await { + let _ = enforcer.load_policy().await; + return Err(unexpected_error!("Failed to save Casbin policies: {}", e)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin.rs` around lines 45 - 72, The enforcer_mut function mutates the in-memory Enforcer and saves its policy, but if the closure f or save_policy() fails, it leaves the Enforcer state inconsistent with the persisted policy. To fix this, modify enforcer_mut to reload the policy from the database by calling load_policy() on the enforcer before returning an error when either f() or save_policy() returns an error, thereby rolling back any in-memory changes on failure.
🧹 Nitpick comments (5)
crates/superposition_types/src/database/models.rs (1)
371-379: Prefer standard string conversion traits over a bespokeinner()API.
into_inner()is fine, butinner()adds a custom accessor name where Rust code usually exposesas_str()and/orAsRef<str>. That composes better with generic callers and keeps the public API closer to convention.♻️ Possible cleanup
impl NonEmptyString { - pub fn inner(&self) -> &str { + pub fn as_str(&self) -> &str { &self.0 } pub fn into_inner(self) -> String { self.0 } } + +impl AsRef<str> for NonEmptyString { + fn as_ref(&self) -> &str { + self.as_str() + } +} + +impl From<NonEmptyString> for String { + fn from(value: NonEmptyString) -> Self { + value.0 + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/database/models.rs` around lines 371 - 379, The custom accessor inner() on NonEmptyString should follow Rust conventions: replace or rename it to as_str() and implement AsRef<str> for NonEmptyString so callers can use generic APIs; keep the existing into_inner() method. Concretely, remove or deprecate inner(), add pub fn as_str(&self) -> &str that returns &self.0, and add impl AsRef<str> for NonEmptyString { fn as_ref(&self) -> &str { &self.0 } }; update any callers of inner() to use as_str() or AsRef where applicable.tests/src/experiment_groups.test.ts (1)
214-236: Unusedresponsevariables in helper functions.Both
addMandatoryDimensionandremoveMandatoryDimensioncapture the response but never use it. Either remove the assignment or use the response for verification.♻️ Proposed fix
async function addMandatoryDimension(client: SuperpositionClient) { const input = { org_id: ENV.org_id, workspace_name: ENV.workspace_id, workspace_status: WorkspaceStatus.ENABLED, mandatory_dimensions: ["clientId"], }; const cmd = new UpdateWorkspaceCommand(input); - const response = await client.send(cmd); + await client.send(cmd); } async function removeMandatoryDimension(client: SuperpositionClient) { const input = { org_id: ENV.org_id, workspace_name: ENV.workspace_id, workspace_status: WorkspaceStatus.ENABLED, mandatory_dimensions: [], }; const cmd = new UpdateWorkspaceCommand(input); - const response = await client.send(cmd); + await client.send(cmd); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 214 - 236, In addMandatoryDimension and removeMandatoryDimension the local variable response captures the result of client.send(cmd) but is never used; fix by removing the unused assignment and simply await client.send(cmd) (or, if a verification is intended, replace the unused variable with an assertion using the returned value from UpdateWorkspaceCommand), updating both helper functions (addMandatoryDimension, removeMandatoryDimension) accordingly.crates/superposition/src/main.rs (1)
117-117: Prefer a helper for the UI AuthZ toggle.This re-encodes provider semantics in
main.rs. Exposing something likeis_auth_z_enabled()next toget_auth_z_provider()would keep the UI flag coupled to the same source of truth as backend initialization.Based on learnings,
AuthZHandler::initis intentionally designed to panic on startup ifAUTH_Z_PROVIDERenvironment variable is missing or set to an unknown value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/main.rs` at line 117, The UI toggle is re-encoding provider semantics by checking get_auth_z_provider().as_str() != "DISABLED" in main.rs; add a helper is_auth_z_enabled() alongside get_auth_z_provider() that returns a bool by consulting the same provider logic (i.e., uses get_auth_z_provider() and compares to the DISABLED variant) and use is_auth_z_enabled() to set auth_z in main, ensuring the UI flag is coupled to the same source of truth as AuthZHandler::init (which will still panic for missing/unknown providers).crates/superposition/examples/generate_actions_registry.rs (1)
26-30: Consider exiting with a non-zero code on serialization failure.The current error handling silently succeeds (exit code 0) even when JSON generation fails. For CI/scripting use cases, a non-zero exit would be more appropriate.
Suggested improvement
if let Ok(output) = serde_json::to_string_pretty(&json) { println!("{}", output); } else { eprintln!("Failed to generate JSON"); + std::process::exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/examples/generate_actions_registry.rs` around lines 26 - 30, The JSON serialization failure branch (where serde_json::to_string_pretty(&json) returns Err and currently calls eprintln!("Failed to generate JSON")) should cause the process to exit with a non-zero status so CI/scripts detect failure; modify the else branch that contains eprintln! to include exiting with a non-zero code (e.g., call std::process::exit(1) or propagate the error) and optionally include the actual error message alongside the eprintln! to aid debugging.crates/frontend/src/components/side_nav.rs (1)
287-354: Consider extracting shared logic betweenNavComponentandOrgNavComponent.Both components share significant structural similarity (placeholder handling, collapsed state, nav rendering pattern). While they serve different purposes (workspace vs org navigation), the duplicated logic for collapse toggling, CSS class construction, and route rendering could be extracted into a shared base component or helper functions.
This is a nice-to-have refactor that could reduce maintenance burden if these components evolve together.
Also applies to: 401-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/side_nav.rs` around lines 287 - 354, NavComponent duplicates structural and behavioral logic with OrgNavComponent (placeholder handling, collapsed state toggle, CSS class composition, and route mapping); extract the shared parts into a reusable helper or base component: create a shared function (e.g., build_nav_classes) to compute the placeholder_class string from is_placeholder and collapsed_rws, a small NavToggle component or helper to render the menu header and handle collapsed_rws.update, and a generic RoutesList component that takes app_routes, location.pathname, and a renderer (or uses NavItem) to render the route list; then replace the duplicated blocks in NavComponent and OrgNavComponent to call these shared helpers/components (keep WorkspaceSelector and specific links where they differ).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/frontend/src/components/authz.rs`:
- Around line 810-817: The Org/Workspace action-group UI is intentionally never
rendered because the Show wrapper uses a hardcoded false predicate; update the
predicate or remove the Show so DomainResourceActionGroupViewer becomes
reachable for AuthzScope::Org and AuthzScope::Workspace. Specifically, locate
the match arm handling AuthzScope::Org(_) | AuthzScope::Workspace(_, _) that
renders <Show when=move || false> and replace the predicate with the proper
visibility condition (e.g., a boolean indicating the current user can manage
domain action groups) or render DomainResourceActionGroupViewer directly
(removing the Show) so the domain-action-group endpoints are reachable from the
org/workspace UI.
- Around line 732-740: The "Action or Action Group" selector is currently
filtering out groups because ActionDropdown defaults allow_group to false;
update the JSX where ActionDropdown is used (the instance passing
resource_action_map_resource, action_rws=g3_action, resource_rs=g3_resource) to
pass allow_group=true so group selections are included (i.e., add the prop
allow_group=true to that ActionDropdown component).
In `@crates/frontend/src/pages/authz.rs`:
- Around line 32-39: The AuthzEditor is being given a frozen org id via
org.get_untracked(), which prevents it from updating when route params change;
instead pass a reactive Signal (the org Signal returned by
use_context::<Signal<OrganisationId>>() or use_org()/use_workspace()) to
AuthzEditor (e.g. authz_scope should accept a Signal<OrganisationId> or similar)
or alternatively wrap AuthzEditor with a key derived from the current
org/workspace id so it is remounted on navigation; update the prop usage (and
AuthzEditor’s prop type) to accept the Signal or add a key using the live
org/workspace id so resources re-fetch on route change, and apply the same
change for the other occurrences noted (lines 45-53).
In `@crates/frontend/src/types.rs`:
- Line 55: The Envs struct's auth_z field can be missing in server JSON, causing
serde_json::from_str::<Envs>() to fail in use_env(); add #[serde(default)] above
the pub auth_z: bool field in the Envs definition so it defaults to false when
absent (no behavior change when present) and keep the field type as bool; ensure
the Envs struct is already deriving Deserialize so #[serde(default)] will be
applied correctly.
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 123-145: The current logic returns early if
enforcer.add_policy(...) yields false, preventing
enforcer.add_grouping_policy(...) from running; change add_org_policy so it
always attempts both enforcer.add_policy(...) and
enforcer.add_grouping_policy(...) (using the same org_schema and org_admin_email
values), collect each result (and propagate any errors from either call), and
then return Ok(true) if at least one of the two operations actually added a row
(i.e., policy_added || grouping_added), otherwise Ok(false).
In `@crates/superposition_derives/Cargo.toml`:
- Around line 19-20: Remove the unnecessary inventory dependency from the
proc-macro crate by deleting the inventory = { workspace = true } entry in
crates/superposition_derives Cargo.toml; the proc-macro does not directly use
inventory at compile time (the generated quote! output emits inventory::submit!
into consuming crates), so leave proc-macro-crate = "3.1" intact and ensure any
consuming crate (e.g., crates/superposition) depends on inventory itself.
In `@crates/superposition_types/src/api/authz.rs`:
- Around line 11-15: ResourceActionType::get_name currently collapses
Action("x") and Group("x"); change get_name to be strictly display-only and add
a new method (e.g., machine_key(&self) -> String) that returns a unique
machine-facing identifier including the variant (for example "action:read" and
"group:read"); update all places that persist or use the value as a unique key
to call machine_key instead of get_name (search for usages in authz-related
handlers and frontend option keys) so stored/serialized values preserve the
variant.
In `@tests/src/experiment_groups.test.ts`:
- Around line 530-536: The test assertion that a call to
superpositionClient.send(new CreateExperimentGroupCommand(input)) rejects is
missing an await, so the rejection matcher won't run correctly; fix by awaiting
the promise expectation—i.e., prefix the expect(...).rejects.toThrow(...) with
await so the test properly awaits the rejection of the
CreateExperimentGroupCommand sent via superpositionClient.send.
- Around line 718-722: The rejection assertion is missing an await and may not
properly assert the thrown error; update the test so the Promise returned by
superpositionClient.send(new AddMembersToGroupCommand(input)) is awaited with
Jest's rejects matcher (i.e., use await expect(...).rejects.toThrow(...)) and
confirm the test function is declared async so AddMembersToGroupCommand /
superpositionClient.send rejection is correctly asserted.
- Around line 514-520: The test is missing await on the promise assertion so the
spec can pass before the rejection is observed; update the assertions that use
expect(...).rejects.toThrow (e.g., the call to superpositionClient.send(new
CreateExperimentGroupCommand(input))) to prepend await to the expect expression
so the test waits for the promise to settle, and sweep the file
tests/src/experiment_groups.test.ts to add the same await to other
expect(...).rejects.* usages to avoid false positives.
- Around line 777-783: The test is missing an await on the Jest rejection
assertion: await the promise returned by superpositionClient.send(...) when
using .rejects.toThrow so the assertion runs correctly; update the test around
the RemoveMembersFromGroupCommand invocation (the call to
superpositionClient.send with removeInput) to use await
expect(...).rejects.toThrow(...) so the rejection is properly awaited and
asserted.
- Around line 560-566: The test's async rejection assertion is missing an await
so the promise rejection isn't properly awaited; update the assertion that calls
expect(superpositionClient.send(new
CreateExperimentGroupCommand(input))).rejects.toThrow(...) to be awaited (use
await expect(...).rejects.toThrow(...)) so the test correctly waits for the
superpositionClient.send(CreateExperimentGroupCommand) promise rejection and
asserts the thrown JSON parse error.
- Around line 703-707: The test assertion is missing an await on the rejection
matcher: when calling superpositionClient.send(new
AddMembersToGroupCommand(input)) you must await the expectation so Jest waits
for the promise to reject; update the assertion to use await
expect(superpositionClient.send(new
AddMembersToGroupCommand(input))).rejects.toThrow(... ) (or return the expect
promise) so the test properly asserts the thrown error.
- Around line 545-549: The test is missing an await for the rejection assertion:
change the assertion around superpositionClient.send(new
CreateExperimentGroupCommand(input)) to be awaited (use await
expect(...).rejects.toThrow()) so the test actually awaits the promise rejection
from CreateExperimentGroupCommand via superpositionClient.send before
completing.
- Around line 794-800: The test assertion is missing an await on the promise
rejection check; change the line that calls expect(superpositionClient.send(new
RemoveMembersFromGroupCommand(input))).rejects.toThrow(...) to await
expect(superpositionClient.send(new
RemoveMembersFromGroupCommand(input))).rejects.toThrow("No records found. Please
refine or correct your search parameters"); ensuring the test awaits the
.rejects matcher so the rejection is properly asserted for
RemoveMembersFromGroupCommand via superpositionClient.send.
- Around line 651-657: The test assertion is missing awaiting the promise
rejection: when calling superpositionClient.send(new
UpdateExperimentGroupCommand(input)) you must await the rejection matcher so
Jest correctly handles the async error; update the test to use await
expect(superpositionClient.send(new
UpdateExperimentGroupCommand(input))).rejects.toThrow("No records found. Please
refine or correct your search parameters") (or alternatively return the promise)
so the UpdateExperimentGroupCommand rejection is asserted properly.
In `@tests/src/experiments.test.ts`:
- Around line 618-620: The test assertions using the Jest-style ".rejects"
matcher are not awaited, so change each instance like
expect(superpositionClient.send(createFailingTest)).rejects.toThrow() to await
the expectation (or return it) so the test runner waits for the Promise; search
for usages of ".rejects" (notably calls around superpositionClient.send(...),
createFailingTest, and similar failing test payloads at the reported ranges) and
prepend "await" to each expect(...) .rejects.* assertion (or return the
expect(...) expression) so the rejection matcher is actually executed.
---
Outside diff comments:
In `@docs/docs/api/Superposition.openapi.json`:
- Around line 5606-5613: The OpenAPI operation for UpdateWorkspace currently
omits requestBody.required, which lets generated clients omit the body while the
server expects Json<UpdateWorkspaceRequest>; set requestBody.required: true for
the operation that references the schema
"#/components/schemas/UpdateWorkspaceRequestContent" so the contract requires a
body, and apply the same change to the other occurrence mentioned (the block
around lines 11271-11303) to keep client/server behavior consistent with the
backend's Json<UpdateWorkspaceRequest> binding.
In `@tests/src/context.test.ts`:
- Around line 253-269: The test currently pushes dimension names into
createdDimensions even when the duplicate-key branch updates an existing
dimension; change the logic so createdDimensions only includes dimensions that
this suite actually created: in the code handling create vs duplicate (the block
using UpdateDimensionCommand and checking e.message.includes("duplicate key")),
do not push dim.dimension into createdDimensions on the duplicate-key path—only
push when the initial creation path succeeds (the successful create operation)
or explicitly detect creation success before adding to the createdDimensions
array; update the try/catch flow around the create call and the use of
createdDimensions to ensure afterAll only deletes dimensions this test created.
- Around line 490-509: The failing tests are not awaiting their rejection
assertions, causing flaky passes; update the negative-path tests (e.g., the
"should reject empty context objects" test that constructs a
CreateContextCommand and calls client.send) to await the rejection matcher
instead of relying on try/catch — e.g. replace the try/catch with await
expect(client.send(unwrappedCmd)).rejects.toMatch(/Context should not be
empty/i) or, if keeping the try/catch style, ensure the thrown promise is
awaited and the expect runs after await; also add the missing await before any
other .rejects.* assertions referenced (the other occurrences called out in the
comment) so all client.send rejection assertions are awaited.
In `@tests/src/experiment_groups.test.ts`:
- Around line 895-905: The test assertion is missing an await when asserting a
rejected promise from superpositionClient.send(new
DeleteExperimentGroupCommand(...)); update the assertion to await the
expect(...) so the test properly waits for the rejection (i.e., use await
expect(superpositionClient.send(...)).rejects.toThrow("No records found. Please
refine or correct your search parameters") ), or alternatively return the
promise from the test; locate the call to superpositionClient.send and the
DeleteExperimentGroupCommand invocation in tests/src/experiment_groups.test.ts
to apply the change.
- Around line 584-594: The test is asserting a rejected promise but missing an
await on the rejection assertion; update the test so the rejection matcher is
awaited — add await before expect(superpositionClient.send(new
GetExperimentGroupCommand({...}))).rejects.toThrow(...) — referencing the
superpositionClient.send call and GetExperimentGroupCommand used in the
assertion to ensure the promise rejection is correctly awaited.
- Around line 881-891: The rejection assertion is missing an await. Change the
line using expect(superpositionClient.send(new DeleteExperimentGroupCommand({
workspace_id: ENV.workspace_id, org_id: ENV.org_id, id: tempGroupId
}))).rejects.toThrow(...) to be awaited: await
expect(superpositionClient.send(...)).rejects.toThrow(...); also ensure the
enclosing test function is declared async so the await is valid. This references
DeleteExperimentGroupCommand, tempGroupId, and superpositionClient.send in the
failing test.
---
Duplicate comments:
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 45-72: The enforcer_mut function mutates the in-memory Enforcer
and saves its policy, but if the closure f or save_policy() fails, it leaves the
Enforcer state inconsistent with the persisted policy. To fix this, modify
enforcer_mut to reload the policy from the database by calling load_policy() on
the enforcer before returning an error when either f() or save_policy() returns
an error, thereby rolling back any in-memory changes on failure.
In `@crates/superposition/src/workspace/handlers.rs`:
- Around line 219-249: The transaction currently runs
diesel::update(...).execute(...) then separately calls get_workspace(), which
allows a race—modify the transaction closure (the conn.transaction block) to
perform the update and return the updated Workspace directly by replacing the
execute() call with an update that uses
get_result::<Workspace>(transaction_conn) (or the equivalent RETURNING
get_result call) so the transaction returns Ok(updated_workspace) and assign
that result to workspace (instead of calling get_workspace()); then use that
workspace for put_workspace_in_redis and subsequent logic so the post-commit
value is the one actually written within the transaction.
- Around line 175-184: The current post-transaction calls to
authz_handler.on_workspace_creation(...) and
authz_handler.on_workspace_admin_update(...) are best-effort and just log
failures; instead make these operations durable by persisting an
outbox/reconciliation record inside the same transaction (e.g., create a
"pending_authz_sync" row referencing created_workspace.id and the
workspace_admin_email/schema) or by enqueuing a durable retry job before
committing, and have the worker/process pick up and call authz_handler until
success; modify the handlers in handlers.rs to stop swallowing errors—either
return an error from the workspace create/update function when enqueueing fails,
or persist the outbox record and remove the current log-only .map_err(...) drop
so the retry mechanism can handle retries for on_workspace_creation and
on_workspace_admin_update.
---
Nitpick comments:
In `@crates/frontend/src/components/side_nav.rs`:
- Around line 287-354: NavComponent duplicates structural and behavioral logic
with OrgNavComponent (placeholder handling, collapsed state toggle, CSS class
composition, and route mapping); extract the shared parts into a reusable helper
or base component: create a shared function (e.g., build_nav_classes) to compute
the placeholder_class string from is_placeholder and collapsed_rws, a small
NavToggle component or helper to render the menu header and handle
collapsed_rws.update, and a generic RoutesList component that takes app_routes,
location.pathname, and a renderer (or uses NavItem) to render the route list;
then replace the duplicated blocks in NavComponent and OrgNavComponent to call
these shared helpers/components (keep WorkspaceSelector and specific links where
they differ).
In `@crates/superposition_types/src/database/models.rs`:
- Around line 371-379: The custom accessor inner() on NonEmptyString should
follow Rust conventions: replace or rename it to as_str() and implement
AsRef<str> for NonEmptyString so callers can use generic APIs; keep the existing
into_inner() method. Concretely, remove or deprecate inner(), add pub fn
as_str(&self) -> &str that returns &self.0, and add impl AsRef<str> for
NonEmptyString { fn as_ref(&self) -> &str { &self.0 } }; update any callers of
inner() to use as_str() or AsRef where applicable.
In `@crates/superposition/examples/generate_actions_registry.rs`:
- Around line 26-30: The JSON serialization failure branch (where
serde_json::to_string_pretty(&json) returns Err and currently calls
eprintln!("Failed to generate JSON")) should cause the process to exit with a
non-zero status so CI/scripts detect failure; modify the else branch that
contains eprintln! to include exiting with a non-zero code (e.g., call
std::process::exit(1) or propagate the error) and optionally include the actual
error message alongside the eprintln! to aid debugging.
In `@crates/superposition/src/main.rs`:
- Line 117: The UI toggle is re-encoding provider semantics by checking
get_auth_z_provider().as_str() != "DISABLED" in main.rs; add a helper
is_auth_z_enabled() alongside get_auth_z_provider() that returns a bool by
consulting the same provider logic (i.e., uses get_auth_z_provider() and
compares to the DISABLED variant) and use is_auth_z_enabled() to set auth_z in
main, ensuring the UI flag is coupled to the same source of truth as
AuthZHandler::init (which will still panic for missing/unknown providers).
In `@tests/src/experiment_groups.test.ts`:
- Around line 214-236: In addMandatoryDimension and removeMandatoryDimension the
local variable response captures the result of client.send(cmd) but is never
used; fix by removing the unused assignment and simply await client.send(cmd)
(or, if a verification is intended, replace the unused variable with an
assertion using the returned value from UpdateWorkspaceCommand), updating both
helper functions (addMandatoryDimension, removeMandatoryDimension) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12a5f4ad-d8a0-456b-9c62-fdbbb432fbc6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.env.exampleCargo.tomlclients/haskell/sdk/Io/Superposition/Model/UpdateWorkspaceInput.hsclients/java/sdk/src/main/java/io/juspay/superposition/model/UpdateWorkspaceInput.javaclients/javascript/sdk/src/commands/UpdateWorkspaceCommand.tsclients/javascript/sdk/src/models/models_0.tsclients/python/sdk/superposition_sdk/_private/schemas.pycrates/cac_client/Cargo.tomlcrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/audit_log/handlers.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/experimentation_client/Cargo.tomlcrates/experimentation_platform/Cargo.tomlcrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/authz.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/hoc/layout.rscrates/frontend/src/pages.rscrates/frontend/src/pages/authz.rscrates/frontend/src/pages/organisations.rscrates/frontend/src/types.rscrates/service_utils/Cargo.tomlcrates/service_utils/examples/list_actions.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/auth_n.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/casbin.rscrates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlcrates/service_utils/src/middlewares/auth_z/casbin/handlers.rscrates/service_utils/src/middlewares/auth_z/casbin/model.confcrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/registry.rscrates/service_utils/tests/action_registry_test.rscrates/superposition/Cargo.tomlcrates/superposition/examples/generate_actions_registry.rscrates/superposition/src/main.rscrates/superposition/src/organisation/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_core/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_derives/src/lib.rscrates/superposition_provider/Cargo.tomlcrates/superposition_sdk/src/client/update_workspace.rscrates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rscrates/superposition_types/src/api.rscrates/superposition_types/src/api/authz.rscrates/superposition_types/src/api/authz/casbin.rscrates/superposition_types/src/api/workspace.rscrates/superposition_types/src/database/models.rscrates/superposition_types/src/lib.rsdocs/docs/api/Superposition.openapi.jsondocs/docs/api/update-workspace.api.mdxsmithy/models/workspace.smithytests/src/config.test.tstests/src/context.test.tstests/src/experiment_groups.test.tstests/src/experiments.test.tstests/src/workspace.test.ts
💤 Files with no reviewable changes (4)
- crates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rs
- clients/python/sdk/superposition_sdk/_private/schemas.py
- tests/src/config.test.ts
- smithy/models/workspace.smithy
Comment on lines
+810
to
+817
| AuthzScope::Org(_) | AuthzScope::Workspace(_, _) => { | ||
| view! { | ||
| <Show when=move || false> | ||
| <DomainResourceActionGroupViewer | ||
| resource_action_map_resource | ||
| authz_scope | ||
| /> | ||
| </Show> |
Contributor
There was a problem hiding this comment.
Org/workspace action-group management is permanently disabled.
Show when=move || false guarantees DomainResourceActionGroupViewer never renders, so the domain-action-group endpoints added in this PR are not reachable from the org/workspace UI.
Possible fix
- view! {
- <Show when=move || false>
- <DomainResourceActionGroupViewer
- resource_action_map_resource
- authz_scope
- />
- </Show>
- }
+ view! {
+ <DomainResourceActionGroupViewer
+ resource_action_map_resource
+ authz_scope
+ />
+ }📝 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
| AuthzScope::Org(_) | AuthzScope::Workspace(_, _) => { | |
| view! { | |
| <Show when=move || false> | |
| <DomainResourceActionGroupViewer | |
| resource_action_map_resource | |
| authz_scope | |
| /> | |
| </Show> | |
| AuthzScope::Org(_) | AuthzScope::Workspace(_, _) => { | |
| view! { | |
| <DomainResourceActionGroupViewer | |
| resource_action_map_resource | |
| authz_scope | |
| /> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/frontend/src/components/authz.rs` around lines 810 - 817, The
Org/Workspace action-group UI is intentionally never rendered because the Show
wrapper uses a hardcoded false predicate; update the predicate or remove the
Show so DomainResourceActionGroupViewer becomes reachable for AuthzScope::Org
and AuthzScope::Workspace. Specifically, locate the match arm handling
AuthzScope::Org(_) | AuthzScope::Workspace(_, _) that renders <Show when=move ||
false> and replace the predicate with the proper visibility condition (e.g., a
boolean indicating the current user can manage domain action groups) or render
DomainResourceActionGroupViewer directly (removing the Show) so the
domain-action-group endpoints are reachable from the org/workspace UI.
08c0045 to
f33e020
Compare
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
sauraww
approved these changes
Mar 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/src/experiment_groups.test.ts (4)
710-722:⚠️ Potential issue | 🟡 MinorThis case doesn't actually test a non-existent group.
Because
member_experiment_idsis[], the request fails input validation before the service ever checksid: "123". The current assertion only covers the empty-member error. Pass at least one experiment id here so the missing-group path is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 710 - 722, The test "should fail to add members to a non-existent group" currently sets AddMembersToGroupCommandInput.member_experiment_ids = [], which triggers input validation before the service checks the group id; update the test so AddMembersToGroupCommandInput.member_experiment_ids contains at least one valid experiment id (e.g. ["exp-1"]) and then assert that superpositionClient.send(new AddMembersToGroupCommand(input)) rejects with the missing-group error (the error message/shape returned when id "123" does not exist) instead of the empty-member validation error.
244-279:⚠️ Potential issue | 🟠 MajorThe setup assumes the first 200 rows is the full resource set.
Lines 248 and 268 cap the list calls at 200, but
ensureDimensionExists/ensureDefaultConfigExiststhen treat those arrays as authoritative. In a long-lived workspace with more than 200 dimensions or configs, this can recreate existing keys or choose an already-occupied dimension position. Please page until exhaustion or switch to a lookup keyed by name/key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 244 - 279, The test currently assumes ListDimensionsCommand and ListDefaultConfigsCommand return the full set by passing count: 200 and then treating existingDimensions/existingConfigs as authoritative, which breaks for workspaces >200; update the setup to fully enumerate resources (either paginate ListDimensionsCommand and ListDefaultConfigsCommand until no continuation token/next_token and merge pages into existingDimensions/existingConfigs, or replace the array-check logic in ensureDimensionExists and ensureDefaultConfigExists with a lookup by name/key that queries for the specific item) and keep removeMandatoryDimension and the subsequent ensure* calls unchanged except that they should use the fully-merged paged results or the keyed lookups instead of a single 200-item page.
214-235:⚠️ Potential issue | 🟠 MajorPreserve the original
mandatory_dimensionsinstead of hard-coding them.Line 219 always restores
["clientId"], while Line 231 clears the list unconditionally. That means this suite can leave the shared workspace in a different state than it found it if the original mandatory-dimension set was anything else. Please snapshot the current workspace config inbeforeAlland restore that exact value inafterAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 214 - 235, The tests currently hard-code mandatory_dimensions in addMandatoryDimension and removeMandatoryDimension which can leave the shared workspace mutated; snapshot the workspace's current mandatory_dimensions in beforeAll (call the API to fetch the workspace and store its mandatory_dimensions) and then in afterAll restore that exact array. Update addMandatoryDimension/removeMandatoryDimension (or replace them) to accept/use the captured snapshot (e.g., originalMandatoryDimensions) when constructing the UpdateWorkspaceCommand instead of using ["clientId"] or [] so the workspace is returned to its original state by afterAll.
368-433:⚠️ Potential issue | 🟠 Major
afterAllis still leaking experiment groups.This teardown only discards experiments, dimensions, and default configs. The groups created on Line 457, Line 499, and Line 875 are never deleted, so repeated runs leave persistent group records behind and can skew later list/filter assertions in the shared workspace. Please track created group ids and clean them up here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/experiment_groups.test.ts` around lines 368 - 433, The afterAll teardown currently only removes experiments, dimensions, and default configs but omits experiment groups, causing leaked group records; add tracking for createdGroupIds (push group ids when tests create groups) and update the afterAll block to iterate over createdGroupIds and call the appropriate cleanup command via superpositionClient.send (e.g., a Discard/Delete Experiment Group command) with workspace_id and org_id (include a change_reason if applicable), and catch/ignore ResourceNotFound/ExperimentGroupNotFound errors just like the existing createdExperimentIds cleanup logic.
♻️ Duplicate comments (2)
crates/service_utils/src/middlewares/auth_z/casbin.rs (1)
45-75:⚠️ Potential issue | 🔴 CriticalReload persisted policy after failed mutations.
f(&mut enforcer)mutates the live enforcer before persistence. If the closure returnsErrafter a partial change, orsave_policy()fails, this node keeps serving an uncommitted policy set until the next refresh.🔁 Roll back the in-memory state on both failure paths
- let resp = f(&mut enforcer).await?; - - enforcer - .save_policy() - .await - .map_err(|e| unexpected_error!("Failed to save Casbin policies: {}", e))?; + let resp = match f(&mut enforcer).await { + Ok(resp) => resp, + Err(e) => { + let _ = enforcer.load_policy().await; + return Err(e); + } + }; + + if let Err(e) = enforcer.save_policy().await { + let _ = enforcer.load_policy().await; + return Err(unexpected_error!( + "Failed to save Casbin policies: {}", + e + )); + }Also bump
last_policy_load_timeif the rollbackload_policy()succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin.rs` around lines 45 - 75, enforcer_mut currently applies mutations via f(&mut enforcer) and then saves, but if f returns Err or save_policy() fails the in-memory enforcer stays mutated; change enforcer_mut so that on either failure path you call enforcer.load_policy().await to reload persisted policies (while still holding the write lock) and, if that reload succeeds, update last_policy_load_time accordingly; return the original error (or wrap it) to the caller but ensure the rollback attempt is performed and its success is used to bump last_policy_load_time; reference enforcer_mut, f(&mut enforcer), enforcer.load_policy(), enforcer.save_policy(), and last_policy_load_time when making the edits.crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs (1)
93-120:⚠️ Potential issue | 🟠 MajorKeep
bad_argument!as a 400 instead of rewrapping it as a 500.The closure now returns
bad_argument!for invalid resource/action pairs, but Line 120 converts everyenforcer_muterror back intounexpected_error!, so this still responds with 500 instead of 400.🛠️ Minimal fix
- .await - .map_err(|e| unexpected_error!(e))?; + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs` around lines 93 - 120, The closure is returning bad_argument! for invalid resource/action pairs but the final .map_err(|e| unexpected_error!(e)) on the enforcer_mut result hides it and turns 400s into 500s; change the map_err on the enforcer_mut/.await result to inspect the error and preserve bad_argument! errors (return them unchanged) while only wrapping other errors with unexpected_error! so that bad_argument! responses remain HTTP 400; target the enforcer_mut call and the final map_err invocation (and reference bad_argument! and unexpected_error!) when implementing this conditional mapping.
🧹 Nitpick comments (12)
tests/src/workspace.test.ts (1)
145-204: Add a case that omitsworkspace_admin_emailinUpdateWorkspace.This PR changes that field from required to optional, but the suite still only exercises the legacy path where a value is provided. Please add one update call without
workspace_admin_emailand assert the intended behavior for that omission path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/workspace.test.ts` around lines 145 - 204, Add an additional UpdateWorkspace call that omits workspace_admin_email to exercise the optional-path: in the existing "UpdateWorkspace" test create a second input object that includes org_id, workspace_name, workspace_status, mandatory_dimensions, etc., but does NOT include workspace_admin_email, send it with new UpdateWorkspaceCommand, and assert the intended omission behavior by checking response.workspace_admin_email (e.g., that it remains unchanged from the previous value or is undefined depending on the intended contract); locate this logic around the existing UpdateWorkspace test and the usage of UpdateWorkspaceCommand and response.workspace_admin_email to implement the new assertion.crates/service_utils/src/middlewares/auth_z/casbin/model.conf (1)
18-19: Document the intentional workspace-discovery exception inline.The wildcard
workspaceread branch on Line 19 is easy to misread as an accidental bypass. A short comment above the matcher would make this file much easier to maintain and should prevent the same review churn from coming back later.💡 Suggested comment
[matchers] +# Intentionally allow authenticated users to discover/list workspaces +# before workspace-specific roles are assigned. m = (r.dom == p.dom || p.dom == "*" || globMatch(r.dom, p.dom)) && (r.sub == p.sub || (p.sub == "*" && r.obj == "workspace" && g3(r.obj + ":" + r.act, "read") == true) || g(r.sub, p.sub, p.dom) || g(r.sub, p.sub, r.dom)) && (r.obj == p.obj || p.obj == "*") && (r.act == p.act || p.act == "*" || g2(r.obj + ":" + r.act, r.dom, p.act) == true || g2(r.obj + ":" + r.act, p.dom, p.act) == true || g3(r.obj + ":" + r.act, p.act) == true) && (r.attr == "*" || p.attr == "*" || globMatch(r.attr, p.attr))Based on learnings: the wildcard workspace-read policy in
crates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlis intentionally global for workspace discovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/middlewares/auth_z/casbin/model.conf` around lines 18 - 19, Add an inline comment above the matcher named "m" in the [matchers] section explaining that the branch (p.sub == "*" && r.obj == "workspace" && g3(r.obj + ":" + r.act, "read") == true) is an intentional global wildcard to allow workspace discovery (as defined by the workspace-read policy in casbin.sql) and not an accidental bypass; place the comment directly above the long m = ... line and mention it applies only to workspace discovery/read checks to avoid future confusion during reviews.crates/frontend/src/pages/organisations.rs (1)
79-79: Use a defensive pattern for the optionalauth_zflag.The
Envscontext is provided at the application root, so theunwrap()is currently safe. However, sinceauth_zis explicitly optional (note#[serde(default)]on the field), treating it defensively aligns with the defensive patterns used elsewhere in the codebase (e.g.,utils.rs) and improves maintainability.💡 Suggested fix
- let envs = use_context::<Envs>().unwrap(); + let auth_z_enabled = use_context::<Envs>() + .map(|envs| envs.auth_z) + .unwrap_or(false);- <Show when=move || envs.auth_z> + <Show when=move || auth_z_enabled>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/organisations.rs` at line 79, Replace the direct unwrap on the Envs context with a defensive fallback so missing context won't panic; use use_context::<Envs>() and handle the Option (e.g., if let Some(envs) = use_context::<Envs>() { ... } else { let envs = Envs::default(); /* or construct minimal Envs with auth_z = None and log/warn */ }) or use unwrap_or_else(|| Envs::default()), ensuring you reference the Envs type and its optional auth_z field and follow the same defensive pattern used in utils.rs.crates/superposition_derives/src/lib.rs (1)
508-517: Improve error clarity when__DECLARED_RESOURCE__is missing.When a handler uses
#[authorized]without an explicitresourceparameter and nodeclare_resource!is present in the module, the generated code references__DECLARED_RESOURCE__which will produce a cryptic "not found in this scope" compiler error.Consider adding a compile-time hint or wrapping the reference to produce a clearer error message.
💡 Suggested improvement for better error messages
} else { // Use __DECLARED_RESOURCE__ from file-level declare_resource! macro ( quote! { fn resource() -> `#superposition_types_path`::Resource { - __DECLARED_RESOURCE__ + { + #[allow(unused)] + const _: () = { + // If this fails, add `declare_resource!(YourResource);` at the top of this file + // or use `#[authorized(resource = YourResource)]` on this handler. + let _ = __DECLARED_RESOURCE__; + }; + __DECLARED_RESOURCE__ + } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_derives/src/lib.rs` around lines 508 - 517, Replace the raw reference to __DECLARED_RESOURCE__ in the generated resource() function and expression with a macro call like expect_declared_resource!(); then add a fallback macro_rules! expect_declared_resource { () => { compile_error!("No declare_resource! found in this module: either add declare_resource!(...) or pass resource=\"...\" to #[authorized]"); } } in this crate, and update the file-level declare_resource! macro to define/override expect_declared_resource! to expand to the actual declared resource (so resource() -> expect_declared_resource!() and the expression -> expect_declared_resource!())—this yields a clear compile-time message when __DECLARED_RESOURCE__ is missing.crates/service_utils/examples/list_actions.rs (2)
1-7: Example will show empty results without linked handler crates.Running this example as documented (
cargo run --example list_actions -p service_utils) won't collect any handlers since the service_utils crate doesn't link crates containing#[authorized]handlers. Consider adding a note that this example is primarily for demonstrating the API shape, or reference thegenerate_actions_registryexample in the superposition crate for a complete registry.📝 Suggested doc update
//! Example: List all registered actions //! //! This example demonstrates how to use the action registry to collect //! and display all actions registered via the #[authorized] macro. //! //! Run with: cargo run --example list_actions -p service_utils +//! +//! Note: This example shows the API usage but will display an empty registry +//! since service_utils doesn't link handler crates. For a complete registry, +//! see the `generate_actions_registry` example in the superposition crate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/examples/list_actions.rs` around lines 1 - 7, Update the example doc comments in list_actions.rs to warn that running cargo run --example list_actions -p service_utils will produce empty output unless crates that register #[authorized] handlers are linked; explicitly state the example demonstrates the API shape only and add a pointer to the complete registry example (generate_actions_registry) in the superposition crate for a working end-to-end demonstration.
41-44: Consider surfacing JSON serialization errors.Similar to the
generate_actions_registryexample, usingunwrap_or_default()silently produces an empty string on failure. For a diagnostic tool, surfacing the error helps troubleshooting.♻️ Proposed improvement
- println!( - "{}", - serde_json::to_string(&ActionRegistry::group_by_resource()).unwrap_or_default() - ); + match serde_json::to_string(&ActionRegistry::group_by_resource()) { + Ok(json) => println!("{}", json), + Err(e) => eprintln!("Failed to serialize registry: {}", e), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/examples/list_actions.rs` around lines 41 - 44, Replace the silent failure caused by serde_json::to_string(...).unwrap_or_default() so JSON serialization errors are surfaced: call serde_json::to_string(&ActionRegistry::group_by_resource()) and handle the Result (e.g., use expect with a descriptive message, propagate the error from main, or match and print to stderr) instead of using unwrap_or_default() so any serialization error is visible for diagnostics.crates/frontend/src/pages/authz.rs (1)
32-32: Context dependency assumes proper layout hierarchy.The
unwrap()onuse_context::<Signal<OrganisationId>>()will panic ifOrganisationAuthzis rendered outside of a layout that provides org context. This is acceptable if routing guarantees correct usage, but consider usingexpect()with a descriptive message for clearer debugging.♻️ Suggested improvement
- let org = use_context::<Signal<OrganisationId>>().unwrap(); + let org = use_context::<Signal<OrganisationId>>() + .expect("OrganisationAuthz must be rendered within OrgLayout");Also applies to lines 45-46 for
WorkspaceAuthz.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/authz.rs` at line 32, The code calls use_context::<Signal<OrganisationId>>().unwrap() (and similarly for use_context::<Signal<WorkspaceId>>() in WorkspaceAuthz), which will panic with a generic message if the context is missing; replace unwrap() with expect() and provide a descriptive message (e.g., "Organisation context missing: OrganisationAuthz must be rendered inside the layout that provides OrganisationId signal") so failures are clearer, and apply the same change to the WorkspaceAuthz context retrieval to reference the Workspace layout in its expect message.crates/superposition/examples/generate_actions_registry.rs (1)
26-30: Consider including the error details in the failure message.The current error message loses diagnostic context. Since this is a developer-facing CLI tool, surfacing the underlying error would help troubleshoot issues.
♻️ Proposed improvement
- if let Ok(output) = serde_json::to_string_pretty(&json) { - println!("{}", output); - } else { - eprintln!("Failed to generate JSON"); - } + match serde_json::to_string_pretty(&json) { + Ok(output) => println!("{}", output), + Err(e) => eprintln!("Failed to generate JSON: {}", e), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/examples/generate_actions_registry.rs` around lines 26 - 30, The failure branch currently discards the serde_json::to_string_pretty error; change the conditional to capture the error (e.g., use match or if let Err(e) = serde_json::to_string_pretty(&json)) and include the error details in the eprintln so the CLI prints a diagnostic like "Failed to generate JSON: {error}" instead of a generic message; reference the call to serde_json::to_string_pretty and the existing println/eprintln locations when making this change.crates/superposition_types/src/api/authz/casbin.rs (1)
5-24: Consider addingDebugandClonederives for better developer experience.These DTOs may benefit from
Debugfor logging/troubleshooting andClonefor flexibility in handlers. This is optional but commonly useful for API request types.♻️ Suggested improvement
-#[derive(Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub struct PolicyRequest { pub sub: NonEmptyString, pub obj: Resource, pub act: ResourceActionType, pub attr: Option<NonEmptyString>, } -#[derive(Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub struct GroupingPolicyRequest { pub user: NonEmptyString, pub role: NonEmptyString, } -#[derive(Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub struct ActionGroupPolicyRequest { pub resource: Resource, pub action: NonEmptyString, pub action_group: NonEmptyString, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition_types/src/api/authz/casbin.rs` around lines 5 - 24, Add Debug and Clone derives to the request DTOs to improve logging and handler flexibility: update the derive attributes for PolicyRequest, GroupingPolicyRequest, and ActionGroupPolicyRequest to include Debug and Clone in addition to Deserialize and Serialize (i.e., derive(Deserialize, Serialize, Debug, Clone)). Ensure these derives are imported or available for NonEmptyString, Resource, and ResourceActionType usages where needed.crates/service_utils/src/registry.rs (1)
43-48: Consider using iterator filter directly instead of collecting twice.
get_by_resourceallocates a fullVecviaget_all()then filters. For large registries, filtering the iterator directly avoids the intermediate allocation.♻️ Suggested optimization
pub fn get_by_resource(resource: Resource) -> Vec<Self> { - Self::get_all() - .into_iter() + inventory::iter::<Self>() + .cloned() .filter(|action| action.resource_type == resource) .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/service_utils/src/registry.rs` around lines 43 - 48, get_by_resource currently calls get_all() which materializes a full Vec before filtering; instead expose or add a get_all_iter() (or change get_all to return an iterator) and have get_by_resource call get_all_iter().filter(|action| action.resource_type == resource).cloned().collect() (or collect into Vec by cloning/moving as needed) so you avoid the intermediate Vec allocation; update references to use get_all_iter() and ensure ownership/borrowing (clone/move) semantics match the returned Vec<Self>.crates/superposition/src/workspace/handlers.rs (1)
300-300: Acknowledged: ABAC-based filtering for workspace listing is planned.The TODO notes that workspace listing currently returns all workspaces in the org without permission filtering. This is a known limitation to address in follow-up work.
Would you like me to open an issue to track implementing ABAC-based filtering for the workspace list endpoint?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/superposition/src/workspace/handlers.rs` at line 300, The workspace listing handler currently returns all workspaces for an org; update the handler (e.g., the list_org_workspaces / list_workspaces function in handlers.rs) to perform ABAC filtering: extract the caller's identity/claims from the request context (AuthenticatedUser / Claims), pass them to the ABAC policy evaluator (e.g., abac::authorize or Ability::filter_workspaces) to compute allowed workspace IDs or a predicate, and apply that predicate to the DB query (repository method such as workspace_repo.find_by_org_filtered or modify the existing find_by_org call to accept a permission filter) so only permitted workspaces are returned; add unit/integration tests covering allowed and denied workspaces and ensure errors fallback to a 403 on unauthorized access.crates/frontend/src/api.rs (1)
682-705: Exposeworkspace_admin_emailas optional in this helper.The request field is optional now, but this helper still forces every caller to provide and send a value. If partial updates are supposed to support “leave unchanged”, take
Option<String>here and forward it directly.♻️ Suggested shape
- pub fn try_update_payload( - workspace_admin_email: String, + pub fn try_update_payload( + workspace_admin_email: Option<String>, @@ - workspace_admin_email: Some(workspace_admin_email), + workspace_admin_email,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/api.rs` around lines 682 - 705, The helper try_update_payload currently requires a String for workspace_admin_email and always sets UpdateWorkspaceRequest.workspace_admin_email to Some(...); change the parameter type to Option<String> and forward it directly into the UpdateWorkspaceRequest (i.e., assign workspace_admin_email: workspace_admin_email) so callers can pass None for “leave unchanged”; update any call sites to pass Some(...) or None as appropriate and adjust type signatures/imports if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 3: Remove the unused CASBIN_DATABASE_URL entry from the example env file
to avoid confusion; update .env.example by deleting the commented
CASBIN_DATABASE_URL line and ensure consumers rely on the individual vars
CASBIN_DB_USER, CASBIN_DB_PASSWORD, CASBIN_DB_HOST, and CASBIN_DB_NAME (as used
by get_database_url() in crates/service_utils/src/db/utils.rs).
In `@Cargo.toml`:
- Line 44: Update the Cargo.toml dependency versions to eliminate the tokio
RUSTSEC-2025-0023 issue: change the tokio dependency from "1.29.1" to at least
"1.44.2" (preferably the current 1.50.0), and consider updating actix-http from
"3.3.1" to "3.12.0" to stay current; ensure inventory = "0.3" remains as-is (it
allows 0.3.22). Locate and modify the tokio and actix-http entries in Cargo.toml
(symbols: tokio, actix-http, inventory) and run cargo update / cargo check to
verify compatibility and resolve any required downstream version bumps.
In `@crates/service_utils/src/middlewares/auth_n.rs`:
- Around line 53-57: The current substring checks (org_request,
admin_authz_request, admin_settings_request) misclassify routes because they
match anywhere in the path; replace them with segment-aware checks: split the
request.path() into path segments (ignore empty segments) and set org_request
true only if any segment equals "organisations"; set admin_authz_request true
only if there is a consecutive segment pair "authz" followed by "admin"; set
admin_settings_request true only if there is a consecutive segment pair "admin"
followed by "settings"; keep the global_request = org_request ||
admin_authz_request || admin_settings_request and preserve the Login::Global
classification logic using these new segment-aware checks so only exact route
segments trigger the global auth flow.
In `@crates/service_utils/src/middlewares/auth_z.rs`:
- Around line 135-145: Current code creates independent CasbinPolicyEngine
instances (e.g., in init -> CasbinPolicyEngine::new) causing separate enforcer
caches; instead initialize a single shared Arc<CasbinPolicyEngine> and reuse it
for both the middleware Authorizer and the management/AuthZManager path.
Concretely: create a single initialization point (e.g., a static
OnceCell/OnceLock or a shared factory function) that calls
CasbinPolicyEngine::new(...) once and stores Arc<CasbinPolicyEngine>, then
change init (and the analogous code at the other location) to obtain
Arc::clone(&shared_casbin) and wrap that in Arc<dyn Authorizer> (or hand the
same Arc to AuthZManager) so both paths share the same enforcer instance.
In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql`:
- Around line 74-77: The casbin bootstrap lacks policies for the write actions
variable:create, variable:update, and variable:delete (see existing entries for
'variable:get' and 'variable:list' and the role 'variable_reader' in
casbin.sql), so non-admin users will be denied; either confirm admin-only intent
or add corresponding policies/aliases granting write access (e.g., add group
mappings for 'variable:create'/'variable:update'/'variable:delete' or create a
'variable_writer' policy role and a policy line like the existing 'p' for
variable_reader) so the #[authorized] guards in the variable handlers will
permit intended users.
In `@tests/src/dimension.test.ts`:
- Around line 215-220: The teardown log messages reference the wrong label:
update the success and error console messages that mention "value validation
function" to instead reference the compute function being deleted (use "value
compute function" or similar) so they accurately reflect the resource identified
by valueComputeFunctionName; locate the block around the try/catch that logs
with valueComputeFunctionName and replace both the success message and the error
message text accordingly.
- Around line 1197-1199: Await the async assertion and record the created
dimension for cleanup: change the assertion to await the promise returned by
superpositionClient.send(new CreateDimensionCommand(input)) (i.e., use await
expect(...).resolves.toBeDefined() or await the send call and then expect on the
result) and, when the dimension is successfully created, push its identifier
into the createdDimensions array so the test runner can teardown state; locate
this logic around the CreateDimensionCommand invocation and the
createdDimensions test-scope variable to implement these changes.
---
Outside diff comments:
In `@tests/src/experiment_groups.test.ts`:
- Around line 710-722: The test "should fail to add members to a non-existent
group" currently sets AddMembersToGroupCommandInput.member_experiment_ids = [],
which triggers input validation before the service checks the group id; update
the test so AddMembersToGroupCommandInput.member_experiment_ids contains at
least one valid experiment id (e.g. ["exp-1"]) and then assert that
superpositionClient.send(new AddMembersToGroupCommand(input)) rejects with the
missing-group error (the error message/shape returned when id "123" does not
exist) instead of the empty-member validation error.
- Around line 244-279: The test currently assumes ListDimensionsCommand and
ListDefaultConfigsCommand return the full set by passing count: 200 and then
treating existingDimensions/existingConfigs as authoritative, which breaks for
workspaces >200; update the setup to fully enumerate resources (either paginate
ListDimensionsCommand and ListDefaultConfigsCommand until no continuation
token/next_token and merge pages into existingDimensions/existingConfigs, or
replace the array-check logic in ensureDimensionExists and
ensureDefaultConfigExists with a lookup by name/key that queries for the
specific item) and keep removeMandatoryDimension and the subsequent ensure*
calls unchanged except that they should use the fully-merged paged results or
the keyed lookups instead of a single 200-item page.
- Around line 214-235: The tests currently hard-code mandatory_dimensions in
addMandatoryDimension and removeMandatoryDimension which can leave the shared
workspace mutated; snapshot the workspace's current mandatory_dimensions in
beforeAll (call the API to fetch the workspace and store its
mandatory_dimensions) and then in afterAll restore that exact array. Update
addMandatoryDimension/removeMandatoryDimension (or replace them) to accept/use
the captured snapshot (e.g., originalMandatoryDimensions) when constructing the
UpdateWorkspaceCommand instead of using ["clientId"] or [] so the workspace is
returned to its original state by afterAll.
- Around line 368-433: The afterAll teardown currently only removes experiments,
dimensions, and default configs but omits experiment groups, causing leaked
group records; add tracking for createdGroupIds (push group ids when tests
create groups) and update the afterAll block to iterate over createdGroupIds and
call the appropriate cleanup command via superpositionClient.send (e.g., a
Discard/Delete Experiment Group command) with workspace_id and org_id (include a
change_reason if applicable), and catch/ignore
ResourceNotFound/ExperimentGroupNotFound errors just like the existing
createdExperimentIds cleanup logic.
---
Duplicate comments:
In `@crates/service_utils/src/middlewares/auth_z/casbin.rs`:
- Around line 45-75: enforcer_mut currently applies mutations via f(&mut
enforcer) and then saves, but if f returns Err or save_policy() fails the
in-memory enforcer stays mutated; change enforcer_mut so that on either failure
path you call enforcer.load_policy().await to reload persisted policies (while
still holding the write lock) and, if that reload succeeds, update
last_policy_load_time accordingly; return the original error (or wrap it) to the
caller but ensure the rollback attempt is performed and its success is used to
bump last_policy_load_time; reference enforcer_mut, f(&mut enforcer),
enforcer.load_policy(), enforcer.save_policy(), and last_policy_load_time when
making the edits.
In `@crates/service_utils/src/middlewares/auth_z/casbin/handlers.rs`:
- Around line 93-120: The closure is returning bad_argument! for invalid
resource/action pairs but the final .map_err(|e| unexpected_error!(e)) on the
enforcer_mut result hides it and turns 400s into 500s; change the map_err on the
enforcer_mut/.await result to inspect the error and preserve bad_argument!
errors (return them unchanged) while only wrapping other errors with
unexpected_error! so that bad_argument! responses remain HTTP 400; target the
enforcer_mut call and the final map_err invocation (and reference bad_argument!
and unexpected_error!) when implementing this conditional mapping.
---
Nitpick comments:
In `@crates/frontend/src/api.rs`:
- Around line 682-705: The helper try_update_payload currently requires a String
for workspace_admin_email and always sets
UpdateWorkspaceRequest.workspace_admin_email to Some(...); change the parameter
type to Option<String> and forward it directly into the UpdateWorkspaceRequest
(i.e., assign workspace_admin_email: workspace_admin_email) so callers can pass
None for “leave unchanged”; update any call sites to pass Some(...) or None as
appropriate and adjust type signatures/imports if necessary.
In `@crates/frontend/src/pages/authz.rs`:
- Line 32: The code calls use_context::<Signal<OrganisationId>>().unwrap() (and
similarly for use_context::<Signal<WorkspaceId>>() in WorkspaceAuthz), which
will panic with a generic message if the context is missing; replace unwrap()
with expect() and provide a descriptive message (e.g., "Organisation context
missing: OrganisationAuthz must be rendered inside the layout that provides
OrganisationId signal") so failures are clearer, and apply the same change to
the WorkspaceAuthz context retrieval to reference the Workspace layout in its
expect message.
In `@crates/frontend/src/pages/organisations.rs`:
- Line 79: Replace the direct unwrap on the Envs context with a defensive
fallback so missing context won't panic; use use_context::<Envs>() and handle
the Option (e.g., if let Some(envs) = use_context::<Envs>() { ... } else { let
envs = Envs::default(); /* or construct minimal Envs with auth_z = None and
log/warn */ }) or use unwrap_or_else(|| Envs::default()), ensuring you reference
the Envs type and its optional auth_z field and follow the same defensive
pattern used in utils.rs.
In `@crates/service_utils/examples/list_actions.rs`:
- Around line 1-7: Update the example doc comments in list_actions.rs to warn
that running cargo run --example list_actions -p service_utils will produce
empty output unless crates that register #[authorized] handlers are linked;
explicitly state the example demonstrates the API shape only and add a pointer
to the complete registry example (generate_actions_registry) in the
superposition crate for a working end-to-end demonstration.
- Around line 41-44: Replace the silent failure caused by
serde_json::to_string(...).unwrap_or_default() so JSON serialization errors are
surfaced: call serde_json::to_string(&ActionRegistry::group_by_resource()) and
handle the Result (e.g., use expect with a descriptive message, propagate the
error from main, or match and print to stderr) instead of using
unwrap_or_default() so any serialization error is visible for diagnostics.
In `@crates/service_utils/src/middlewares/auth_z/casbin/model.conf`:
- Around line 18-19: Add an inline comment above the matcher named "m" in the
[matchers] section explaining that the branch (p.sub == "*" && r.obj ==
"workspace" && g3(r.obj + ":" + r.act, "read") == true) is an intentional global
wildcard to allow workspace discovery (as defined by the workspace-read policy
in casbin.sql) and not an accidental bypass; place the comment directly above
the long m = ... line and mention it applies only to workspace discovery/read
checks to avoid future confusion during reviews.
In `@crates/service_utils/src/registry.rs`:
- Around line 43-48: get_by_resource currently calls get_all() which
materializes a full Vec before filtering; instead expose or add a get_all_iter()
(or change get_all to return an iterator) and have get_by_resource call
get_all_iter().filter(|action| action.resource_type ==
resource).cloned().collect() (or collect into Vec by cloning/moving as needed)
so you avoid the intermediate Vec allocation; update references to use
get_all_iter() and ensure ownership/borrowing (clone/move) semantics match the
returned Vec<Self>.
In `@crates/superposition_derives/src/lib.rs`:
- Around line 508-517: Replace the raw reference to __DECLARED_RESOURCE__ in the
generated resource() function and expression with a macro call like
expect_declared_resource!(); then add a fallback macro_rules!
expect_declared_resource { () => { compile_error!("No declare_resource! found in
this module: either add declare_resource!(...) or pass resource=\"...\" to
#[authorized]"); } } in this crate, and update the file-level declare_resource!
macro to define/override expect_declared_resource! to expand to the actual
declared resource (so resource() -> expect_declared_resource!() and the
expression -> expect_declared_resource!())—this yields a clear compile-time
message when __DECLARED_RESOURCE__ is missing.
In `@crates/superposition_types/src/api/authz/casbin.rs`:
- Around line 5-24: Add Debug and Clone derives to the request DTOs to improve
logging and handler flexibility: update the derive attributes for PolicyRequest,
GroupingPolicyRequest, and ActionGroupPolicyRequest to include Debug and Clone
in addition to Deserialize and Serialize (i.e., derive(Deserialize, Serialize,
Debug, Clone)). Ensure these derives are imported or available for
NonEmptyString, Resource, and ResourceActionType usages where needed.
In `@crates/superposition/examples/generate_actions_registry.rs`:
- Around line 26-30: The failure branch currently discards the
serde_json::to_string_pretty error; change the conditional to capture the error
(e.g., use match or if let Err(e) = serde_json::to_string_pretty(&json)) and
include the error details in the eprintln so the CLI prints a diagnostic like
"Failed to generate JSON: {error}" instead of a generic message; reference the
call to serde_json::to_string_pretty and the existing println/eprintln locations
when making this change.
In `@crates/superposition/src/workspace/handlers.rs`:
- Line 300: The workspace listing handler currently returns all workspaces for
an org; update the handler (e.g., the list_org_workspaces / list_workspaces
function in handlers.rs) to perform ABAC filtering: extract the caller's
identity/claims from the request context (AuthenticatedUser / Claims), pass them
to the ABAC policy evaluator (e.g., abac::authorize or
Ability::filter_workspaces) to compute allowed workspace IDs or a predicate, and
apply that predicate to the DB query (repository method such as
workspace_repo.find_by_org_filtered or modify the existing find_by_org call to
accept a permission filter) so only permitted workspaces are returned; add
unit/integration tests covering allowed and denied workspaces and ensure errors
fallback to a 403 on unauthorized access.
In `@tests/src/workspace.test.ts`:
- Around line 145-204: Add an additional UpdateWorkspace call that omits
workspace_admin_email to exercise the optional-path: in the existing
"UpdateWorkspace" test create a second input object that includes org_id,
workspace_name, workspace_status, mandatory_dimensions, etc., but does NOT
include workspace_admin_email, send it with new UpdateWorkspaceCommand, and
assert the intended omission behavior by checking response.workspace_admin_email
(e.g., that it remains unchanged from the previous value or is undefined
depending on the intended contract); locate this logic around the existing
UpdateWorkspace test and the usage of UpdateWorkspaceCommand and
response.workspace_admin_email to implement the new assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6138e3d6-b36b-491f-b064-484d731b2a51
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockclients/javascript/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (80)
.env.exampleCargo.tomlclients/haskell/sdk/Io/Superposition/Model/UpdateWorkspaceInput.hsclients/java/sdk/src/main/java/io/juspay/superposition/model/UpdateWorkspaceInput.javaclients/javascript/sdk/src/commands/UpdateWorkspaceCommand.tsclients/javascript/sdk/src/models/models_0.tsclients/python/sdk/superposition_sdk/_private/schemas.pycrates/cac_client/Cargo.tomlcrates/context_aware_config/Cargo.tomlcrates/context_aware_config/src/api/audit_log/handlers.rscrates/context_aware_config/src/api/config/handlers.rscrates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/api/functions/handlers.rscrates/context_aware_config/src/api/secrets/handlers.rscrates/context_aware_config/src/api/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/experimentation_client/Cargo.tomlcrates/experimentation_platform/Cargo.tomlcrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/frontend/src/api.rscrates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/authz.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/hoc/layout.rscrates/frontend/src/pages.rscrates/frontend/src/pages/authz.rscrates/frontend/src/pages/organisations.rscrates/frontend/src/types.rscrates/service_utils/Cargo.tomlcrates/service_utils/examples/list_actions.rscrates/service_utils/src/lib.rscrates/service_utils/src/middlewares/auth_n.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/casbin.rscrates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlcrates/service_utils/src/middlewares/auth_z/casbin/handlers.rscrates/service_utils/src/middlewares/auth_z/casbin/model.confcrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/registry.rscrates/service_utils/tests/action_registry_test.rscrates/superposition/Cargo.tomlcrates/superposition/examples/generate_actions_registry.rscrates/superposition/src/main.rscrates/superposition/src/organisation/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_core/Cargo.tomlcrates/superposition_derives/Cargo.tomlcrates/superposition_derives/src/lib.rscrates/superposition_provider/Cargo.tomlcrates/superposition_sdk/src/client/update_workspace.rscrates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rscrates/superposition_types/src/api.rscrates/superposition_types/src/api/authz.rscrates/superposition_types/src/api/authz/casbin.rscrates/superposition_types/src/api/workspace.rscrates/superposition_types/src/database/models.rscrates/superposition_types/src/lib.rsdocs/docs/api/Superposition.openapi.jsondocs/docs/api/update-workspace.api.mdxnix/rust.nixsmithy/models/workspace.smithytests/src/config.test.tstests/src/context.test.tstests/src/default_config.test.tstests/src/dimension.test.tstests/src/experiment_groups.test.tstests/src/experiments.test.tstests/src/functions.test.tstests/src/org.test.tstests/src/secrets.test.tstests/src/variables.test.tstests/src/workspace.test.ts
💤 Files with no reviewable changes (4)
- smithy/models/workspace.smithy
- crates/superposition_sdk/src/operation/update_workspace/_update_workspace_input.rs
- clients/python/sdk/superposition_sdk/_private/schemas.py
- tests/src/config.test.ts
| @@ -1,5 +1,6 @@ | |||
| DATABASE_URL=postgres://postgres:docker@localhost:5432/config?sslmode=disable # Sample format (unused ENV variable) | |||
| PORT=8080 | |||
| # CASBIN_DATABASE_URL=postgres://postgres:docker@localhost:5432/config?sslmode=disable # Sample format (unused ENV variable) | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
-g '!**/node_modules/**' \
-g '!**/dist/**' \
-g '!**/target/**' \
'\bCASBIN_DATABASE_URL\b|\bCASBIN_DB_(USER|PASSWORD|HOST|NAME|POOL_SIZE)\b|\bCASBIN_POLICY_REFRESH_INTERVAL\b|\bROOT_ADMIN_EMAIL\b'Repository: juspay/superposition
Length of output: 1032
🏁 Script executed:
sed -n '375,450p' crates/service_utils/src/middlewares/auth_z/casbin.rsRepository: juspay/superposition
Length of output: 2608
🏁 Script executed:
rg -n 'fn get_database_url' crates/service_utils/src/middlewares/auth_z/Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
rg -n 'fn get_database_url' --type rustRepository: juspay/superposition
Length of output: 134
🏁 Script executed:
sed -n '36,100p' crates/service_utils/src/db/utils.rsRepository: juspay/superposition
Length of output: 1516
Remove CASBIN_DATABASE_URL from line 3.
The code uses individual CASBIN_DB_USER, CASBIN_DB_PASSWORD, CASBIN_DB_HOST, and CASBIN_DB_NAME environment variables (constructed via get_database_url() in crates/service_utils/src/db/utils.rs). CASBIN_DATABASE_URL has no consumers in the codebase and is already labeled as unused; remove it to prevent deployment confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 3, Remove the unused CASBIN_DATABASE_URL entry from the
example env file to avoid confusion; update .env.example by deleting the
commented CASBIN_DATABASE_URL line and ensure consumers rely on the individual
vars CASBIN_DB_USER, CASBIN_DB_PASSWORD, CASBIN_DB_HOST, and CASBIN_DB_NAME (as
used by get_database_url() in crates/service_utils/src/db/utils.rs).
| assets-dir = "crates/frontend/assets" | ||
|
|
||
| [workspace.dependencies] | ||
| actix-http = "3.3.1" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions and security advisories for the added dependencies
echo "=== Checking latest crate versions ==="
for crate in actix-http inventory tokio; do
echo "Latest $crate version:"
curl -s "https://crates.io/api/v1/crates/$crate" | jq -r '.crate.newest_version'
done
echo ""
echo "=== Checking for security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: RUST, package: "tokio") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: juspay/superposition
Length of output: 401
🏁 Script executed:
cat Cargo.toml | sed -n '40,100p'Repository: juspay/superposition
Length of output: 2227
🌐 Web query:
tokio 1.29.1 security vulnerabilities CVE
💡 Result:
For the tokio crate v1.29.1 (June 2023), the RustSec advisories map out like this:
Affects tokio 1.29.1
- RUSTSEC-2025-0023 (INFO/Unsoundness): broadcast channel may call
clonein parallel without requiringSync(memory-corruption class unsoundness). Alias:GHSA-rr8g-9fpq-6wmg(no CVE listed). Fixed in:>=1.44.2(and also in select earlier maintained lines like1.38.2+,1.42.1+,1.43.1+). (rustsec.org)
CVEs in tokio advisories (but NOT affecting tokio 1.29.1)
These are patched well before 1.29.1, so 1.29.1 is not in the affected ranges:
- CVE-2023-22466 (RUSTSEC-2023-0001): Windows named-pipe
reject_remote_clientsconfiguration corruption; patched in>=1.23.1. (rustsec.org) - CVE-2021-45710 (RUSTSEC-2021-0124): oneshot close/send race → possible memory corruption; patched in
>=1.13.1. (rustsec.org) - CVE-2021-38191 (RUSTSEC-2021-0072):
LocalSetabort drop on wrong thread; patched in>=1.8.1. (rustsec.org)
(These are all listed on RustSec’s tokio package advisory page.) (rustsec.org)
Citations:
- 1: https://rustsec.org/advisories/RUSTSEC-2025-0023.html
- 2: https://rustsec.org/advisories/RUSTSEC-2023-0001.html
- 3: https://rustsec.org/advisories/RUSTSEC-2021-0124.html
- 4: https://rustsec.org/advisories/RUSTSEC-2021-0072.html
- 5: https://rustsec.org/packages/tokio.html
🌐 Web query:
actix-http 3.3.1 security advisories
💡 Result:
As of March 10, 2026, RustSec lists two security advisories for actix-http, and actix-http 3.3.1 is not affected by either (it is newer than the patched versions). [1]
- RUSTSEC-2020-0048 / CVE-2020-35901 (HIGH) – use-after-free in
BodyStream; patched in>= 2.0.0-alpha.1, so3.3.1is not affected. [2] - RUSTSEC-2021-0081 / CVE-2021-38512 (HIGH) – potential HTTP/1 request smuggling due to lack of input validation; patched in
>= 3.0.0-beta.9(and^2.2.1for 2.x), so3.3.1is not affected. [3]
Sources: RustSec package page for actix-http and the two advisories. [1][2][3]
Upgrade tokio to at least 1.44.2 to fix critical memory safety issue.
tokio = "1.29.1" is affected by RUSTSEC-2025-0023 — a memory-corruption unsoundness in the broadcast channel implementation where clone may be called in parallel without requiring Sync. This is fixed in tokio >= 1.44.2. Upgrade immediately.
Additionally, the pinned versions are significantly outdated:
- tokio 1.29.1 is from June 2023; current version is 1.50.0
- actix-http 3.3.1 lags behind 3.12.0, though it has no known security advisories
inventory = "0.3" is acceptable (range spec, allows 0.3.22).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 44, Update the Cargo.toml dependency versions to
eliminate the tokio RUSTSEC-2025-0023 issue: change the tokio dependency from
"1.29.1" to at least "1.44.2" (preferably the current 1.50.0), and consider
updating actix-http from "3.3.1" to "3.12.0" to stay current; ensure inventory =
"0.3" remains as-is (it allows 0.3.22). Locate and modify the tokio and
actix-http entries in Cargo.toml (symbols: tokio, actix-http, inventory) and run
cargo update / cargo check to verify compatibility and resolve any required
downstream version bumps.
Comment on lines
53
to
+57
| let org_request = request.path().matches("/organisations").count() > 0; | ||
| let admin_authz_request = request.path().matches("/authz/admin").count() > 0; | ||
| let admin_settings_request = | ||
| request.path().matches("/admin/settings").count() > 0; | ||
| let global_request = org_request || admin_authz_request || admin_settings_request; |
Contributor
There was a problem hiding this comment.
Use segment-aware route checks for login classification.
These matches(...).count() > 0 checks treat any path containing the substring as a global route. That means unrelated endpoints like /authz/administrator or /admin/settings-old would also be classified as Login::Global, which can send requests through the wrong auth flow.
🔧 Suggested fix
- let org_request = request.path().matches("/organisations").count() > 0;
- let admin_authz_request = request.path().matches("/authz/admin").count() > 0;
- let admin_settings_request =
- request.path().matches("/admin/settings").count() > 0;
+ let path = request.path();
+ let org_request =
+ path == "/organisations" || path.starts_with("/organisations/");
+ let admin_authz_request =
+ path == "/authz/admin" || path.starts_with("/authz/admin/");
+ let admin_settings_request =
+ path == "/admin/settings" || path.starts_with("/admin/settings/");📝 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 org_request = request.path().matches("/organisations").count() > 0; | |
| let admin_authz_request = request.path().matches("/authz/admin").count() > 0; | |
| let admin_settings_request = | |
| request.path().matches("/admin/settings").count() > 0; | |
| let global_request = org_request || admin_authz_request || admin_settings_request; | |
| let path = request.path(); | |
| let org_request = | |
| path == "/organisations" || path.starts_with("/organisations/"); | |
| let admin_authz_request = | |
| path == "/authz/admin" || path.starts_with("/authz/admin/"); | |
| let admin_settings_request = | |
| path == "/admin/settings" || path.starts_with("/admin/settings/"); | |
| let global_request = org_request || admin_authz_request || admin_settings_request; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/middlewares/auth_n.rs` around lines 53 - 57, The
current substring checks (org_request, admin_authz_request,
admin_settings_request) misclassify routes because they match anywhere in the
path; replace them with segment-aware checks: split the request.path() into path
segments (ignore empty segments) and set org_request true only if any segment
equals "organisations"; set admin_authz_request true only if there is a
consecutive segment pair "authz" followed by "admin"; set admin_settings_request
true only if there is a consecutive segment pair "admin" followed by "settings";
keep the global_request = org_request || admin_authz_request ||
admin_settings_request and preserve the Login::Global classification logic using
these new segment-aware checks so only exact route segments trigger the global
auth flow.
Comment on lines
+135
to
145
| pub async fn init(kms_client: &Option<Client>, app_env: &AppEnv) -> Self { | ||
| let ap: Arc<dyn Authorizer> = match get_auth_z_provider().as_str() { | ||
| // "CASBIN" => Arc::new( | ||
| // CasbinPolicyEngine::new(kms_client, app_env, None) | ||
| // .await | ||
| // .unwrap(), | ||
| // ), | ||
| "CASBIN" => Arc::new( | ||
| CasbinPolicyEngine::new(kms_client, app_env, None) | ||
| .await | ||
| .unwrap(), | ||
| ), | ||
| "DISABLED" => Arc::new(NoAuth), | ||
| _ => panic!("Missing/Unknown authorizer."), | ||
| }; | ||
| Self(ap) |
Contributor
There was a problem hiding this comment.
Share one CasbinPolicyEngine between middleware and management paths.
These initializers create independent enforcer caches. A policy mutation through AuthZManager can therefore stay invisible to request-time authorization until the next refresh window, so grants/revocations behave inconsistently on the same node.
Also applies to: 225-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/middlewares/auth_z.rs` around lines 135 - 145,
Current code creates independent CasbinPolicyEngine instances (e.g., in init ->
CasbinPolicyEngine::new) causing separate enforcer caches; instead initialize a
single shared Arc<CasbinPolicyEngine> and reuse it for both the middleware
Authorizer and the management/AuthZManager path. Concretely: create a single
initialization point (e.g., a static OnceCell/OnceLock or a shared factory
function) that calls CasbinPolicyEngine::new(...) once and stores
Arc<CasbinPolicyEngine>, then change init (and the analogous code at the other
location) to obtain Arc::clone(&shared_casbin) and wrap that in Arc<dyn
Authorizer> (or hand the same Arc to AuthZManager) so both paths share the same
enforcer instance.
Comment on lines
+74
to
+77
| -- variable | ||
| ('g3', 'variable:get', 'read', '', '', '', ''), | ||
| ('g3', 'variable:list', 'read', '', '', '', ''), | ||
| ('p', 'variable_reader', '*', 'variable', 'read', '*', ''), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Variable handlers exposed through Actix ==="
sed -n '100,220p' crates/context_aware_config/src/api/variables/handlers.rs
echo
echo "=== Variable aliases/policies seeded in casbin.sql ==="
rg -n -C1 "variable:(get|list|create|update|delete)|'variable'|\\bread\\b|\\bwrite\\b" \
crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql
echo
echo "=== Casbin/action-registry code that may register write aliases elsewhere ==="
rg -n -C2 "variable:(create|update|delete)|ActionRegistry|g3|write" crates --glob '!**/target/**'Repository: juspay/superposition
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Authorized macro implementation ==="
rg -l "macro.*authorized|#\[authorized\]" crates/context_aware_config --type rs | head -5
echo
echo "=== Check for create/update/delete action aliases in casbin.sql ==="
cat crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql | grep -E "variable.*(create|update|delete|write)" || echo "No variable write actions found"
echo
echo "=== Look for any write action patterns seeded for similar resources ==="
cat crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql | grep -E "\(.*write.*\)" || echo "No write actions seeded at all"
echo
echo "=== Check if routes are behind workspace context (implies admin) ==="
rg -A2 "variable.*create_handler|variable.*update_handler|variable.*delete_handler" crates/context_aware_config --type rs | head -20Repository: juspay/superposition
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for create/update/delete action aliases in casbin.sql ==="
cat crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql | grep -E "variable.*(create|update|delete|write)" || echo "No variable write actions found"
echo
echo "=== Look for ANY write action patterns seeded in casbin.sql ==="
cat crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql | grep -E "write" || echo "No write actions seeded at all"
echo
echo "=== Check variable handlers route configuration in handlers.rs ==="
rg -A5 "variable_routes|\.service.*variable" crates/context_aware_config/src --max-count=10
echo
echo "=== Look for authorized macro definition ==="
rg -B2 -A10 "macro.*authorized" crates --max-count=5Repository: juspay/superposition
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find authorized macro definition ==="
find crates -name "*.rs" -type f -exec grep -l "macro.*authorized\|#\[authorized\]" {} \; | head -5
echo
echo "=== Check crates/service_utils for auth/authorization logic ==="
ls -la crates/service_utils/src/middlewares/auth_z/ 2>/dev/null || echo "Directory not found"
echo
echo "=== Search for authorized macro across all crates ==="
rg "macro\s+authorized" crates --max-count=5
echo
echo "=== Check main.rs or lib.rs in context_aware_config for macro imports ==="
head -50 crates/context_aware_config/src/main.rs | grep -E "use|macro|authorized" || echo "Not found in main"
echo
echo "=== Full casbin.sql content to understand policy structure ==="
wc -l crates/service_utils/src/middlewares/auth_z/casbin/casbin.sqlRepository: juspay/superposition
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read authorization.rs (likely contains authorized macro) ==="
cat crates/service_utils/src/middlewares/auth_z/authorization.rs
echo
echo "=== Read first 100 lines of casbin.rs ==="
head -100 crates/service_utils/src/middlewares/auth_z/casbin.rs
echo
echo "=== Check no_auth.rs to understand fallback ==="
cat crates/service_utils/src/middlewares/auth_z/no_auth.rsRepository: juspay/superposition
Length of output: 6969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find superposition_macros crate ==="
find crates -name "superposition_macros" -type d
echo
echo "=== Search for authorized macro definition ==="
rg -A20 "pub.*macro.*authorized" crates/superposition_macros --max-count=3 2>/dev/null || echo "Not found with pub macro"
echo
echo "=== Look for authorized in lib.rs of macros ==="
grep -n "authorized" crates/superposition_macros/src/lib.rs 2>/dev/null | head -5
echo
echo "=== Continue reading casbin.rs - enforcement logic ==="
sed -n '150,250p' crates/service_utils/src/middlewares/auth_z/casbin.rsRepository: juspay/superposition
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List superposition_macros source files ==="
ls -la crates/superposition_macros/src/
echo
echo "=== Read superposition_macros lib.rs ==="
cat crates/superposition_macros/src/lib.rs
echo
echo "=== Continue casbin.rs enforcement (lines 150-250) ==="
sed -n '150,250p' crates/service_utils/src/middlewares/auth_z/casbin.rsRepository: juspay/superposition
Length of output: 5534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for authorized attribute macro in Cargo.toml dependencies ==="
rg "authorized" crates/context_aware_config/Cargo.toml || echo "Not found in Cargo.toml"
echo
echo "=== Search for proc-macro or derive in all Cargo.toml ==="
grep -r "proc-macro\|#\[proc_macro\]" crates --include="Cargo.toml" | head -3
echo
echo "=== Search for #[authorized] in actix/handler files ==="
rg "#\[authorized\]" crates/context_aware_config --type rust -B1 -A5 | head -30
echo
echo "=== Get complete is_allowed enforcement logic (lines 220-280) ==="
sed -n '220,280p' crates/service_utils/src/middlewares/auth_z/casbin.rsRepository: juspay/superposition
Length of output: 4798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List superposition_derives source files ==="
find crates/superposition_derives/src -type f -name "*.rs" | head -20
echo
echo "=== Search for authorized in superposition_derives ==="
rg -n "authorized" crates/superposition_derives/src --max-count=10
echo
echo "=== Read main lib.rs of superposition_derives ==="
head -150 crates/superposition_derives/src/lib.rsRepository: juspay/superposition
Length of output: 7350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read authorized macro implementation (lines 443-550) ==="
sed -n '443,550p' crates/superposition_derives/src/lib.rs
echo
echo "=== Continue reading (lines 550-650) ==="
sed -n '550,650p' crates/superposition_derives/src/lib.rsRepository: juspay/superposition
Length of output: 5009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find AuthZ struct definition ==="
rg -n "struct AuthZ|impl.*AuthZ" crates/service_utils/src/middlewares/auth_z --max-count=5
echo
echo "=== Read middleware/extractor implementation for AuthZ ==="
find crates/service_utils/src/middlewares/auth_z -name "*.rs" -type f | xargs ls -la
echo
echo "=== Check handlers.rs in auth_z ==="
head -100 crates/service_utils/src/middlewares/auth_z/handlers.rsRepository: juspay/superposition
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Broader search for AuthZ struct ==="
rg -n "struct AuthZ<" crates --max-count=10
echo
echo "=== Search for AuthZ in all files ==="
rg -l "AuthZ" crates/service_utils/src/middlewares/auth_z/
echo
echo "=== Check handlers.rs in auth_z ==="
cat crates/service_utils/src/middlewares/auth_z/handlers.rsRepository: juspay/superposition
Length of output: 597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read auth_z.rs (main authorization module) ==="
cat crates/service_utils/src/middlewares/auth_z.rsRepository: juspay/superposition
Length of output: 7798
Variable write actions lack bootstrapped policies.
The variable:create, variable:update, and variable:delete actions are not seeded in casbin.sql. Although the handlers are exposed with #[authorized] guards in crates/context_aware_config/src/api/variables/handlers.rs, non-admin users will receive 403 Forbidden when attempting mutations since no matching policies exist. Confirm this is intentional (admin-only) or add write action aliases if intended for non-admin roles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/middlewares/auth_z/casbin/casbin.sql` around lines
74 - 77, The casbin bootstrap lacks policies for the write actions
variable:create, variable:update, and variable:delete (see existing entries for
'variable:get' and 'variable:list' and the role 'variable_reader' in
casbin.sql), so non-admin users will be denied; either confirm admin-only intent
or add corresponding policies/aliases granting write access (e.g., add group
mappings for 'variable:create'/'variable:update'/'variable:delete' or create a
'variable_writer' policy role and a policy line like the existing 'p' for
variable_reader) so the #[authorized] guards in the variable handlers will
permit intended users.
Comment on lines
215
to
+220
| console.log( | ||
| `Cleaned up value validation function: ${valueComputeFunctionName}` | ||
| `Cleaned up value validation function: ${valueComputeFunctionName}`, | ||
| ); | ||
| } catch (e: any) { | ||
| console.error( | ||
| `Failed to clean up value validation function: ${e.message}` | ||
| `Failed to clean up value validation function: ${e.message}`, |
Contributor
There was a problem hiding this comment.
Fix the teardown labels for the compute function.
These log lines are deleting valueComputeFunctionName, but both messages still say "value validation function", which makes cleanup failures misleading.
Suggested fix
await superpositionClient.send(deleteCmd);
console.log(
- `Cleaned up value validation function: ${valueComputeFunctionName}`,
+ `Cleaned up value compute function: ${valueComputeFunctionName}`,
);
} catch (e: any) {
console.error(
- `Failed to clean up value validation function: ${e.message}`,
+ `Failed to clean up value compute function: ${e.message}`,
);
}📝 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
| console.log( | |
| `Cleaned up value validation function: ${valueComputeFunctionName}` | |
| `Cleaned up value validation function: ${valueComputeFunctionName}`, | |
| ); | |
| } catch (e: any) { | |
| console.error( | |
| `Failed to clean up value validation function: ${e.message}` | |
| `Failed to clean up value validation function: ${e.message}`, | |
| console.log( | |
| `Cleaned up value compute function: ${valueComputeFunctionName}`, | |
| ); | |
| } catch (e: any) { | |
| console.error( | |
| `Failed to clean up value compute function: ${e.message}`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/dimension.test.ts` around lines 215 - 220, The teardown log
messages reference the wrong label: update the success and error console
messages that mention "value validation function" to instead reference the
compute function being deleted (use "value compute function" or similar) so they
accurately reflect the resource identified by valueComputeFunctionName; locate
the block around the try/catch that logs with valueComputeFunctionName and
replace both the success message and the error message text accordingly.
Comment on lines
1197
to
1199
| expect( | ||
| superpositionClient.send(new CreateDimensionCommand(input)) | ||
| superpositionClient.send(new CreateDimensionCommand(input)), | ||
| ).resolves.toBeDefined(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In bun:test, when an async test uses expect(promise).resolvesorexpect(promise).rejects, must that assertion be awaited or returned for the test runner to wait for it?
💡 Result:
Yes. In bun:test, the .resolves / .rejects chains are async (they produce a Promise), so you must await them (or return them from a non-async test) for the runner to wait for the assertion to complete; otherwise the test can finish before the promise settles. Bun’s own docs show this pattern (e.g. await expect(...).rejects...). [1] (This matches Jest’s expectation semantics as well.) [2]
Examples
import { test, expect } from "bun:test";
test("resolves", async () => {
await expect(Promise.resolve(42)).resolves.toBe(42);
});
test("rejects (non-async test)", () => {
return expect(Promise.reject(new Error("nope"))).rejects.toThrow("nope");
});Sources: Bun test docs [1], Jest async testing docs [2].
🏁 Script executed:
# Check the test file at the mentioned lines
sed -n '1197,1199p' tests/src/dimension.test.tsRepository: juspay/superposition
Length of output: 186
🏁 Script executed:
# Also check the broader context around this test and the created dimension handling
sed -n '1180,1210p' tests/src/dimension.test.tsRepository: juspay/superposition
Length of output: 1079
🏁 Script executed:
# Check lines 215-220 for the logging issue mentioned in the scratchpad
sed -n '215,220p' tests/src/dimension.test.tsRepository: juspay/superposition
Length of output: 344
Await this async assertion and register the created dimension for teardown.
Lines 1197–1199 use expect(promise).resolves without await, which allows the test to complete before the assertion settles. In bun:test, .resolves and .rejects produce promises that must be awaited (or returned) for the test runner to wait for them. Additionally, the created dimension is never added to createdDimensions, so test state leaks on successful runs.
Suggested fix
- expect(
- superpositionClient.send(new CreateDimensionCommand(input)),
- ).resolves.toBeDefined();
+ const response = await superpositionClient.send(
+ new CreateDimensionCommand(input),
+ );
+ createdDimensions.push(response.dimension ?? cohortData.dimension);
+ expect(response).toBeDefined();📝 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
| expect( | |
| superpositionClient.send(new CreateDimensionCommand(input)) | |
| superpositionClient.send(new CreateDimensionCommand(input)), | |
| ).resolves.toBeDefined(); | |
| const response = await superpositionClient.send( | |
| new CreateDimensionCommand(input), | |
| ); | |
| createdDimensions.push(response.dimension ?? cohortData.dimension); | |
| expect(response).toBeDefined(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/dimension.test.ts` around lines 1197 - 1199, Await the async
assertion and record the created dimension for cleanup: change the assertion to
await the promise returned by superpositionClient.send(new
CreateDimensionCommand(input)) (i.e., use await
expect(...).resolves.toBeDefined() or await the send call and then expect on the
result) and, when the dimension is successfully created, push its identifier
into the createdDimensions array so the test runner can teardown state; locate
this logic around the CreateDimensionCommand invocation and the
createdDimensions test-scope variable to implement these changes.
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
4 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.
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit
New Features
Docs