feat(authz): ABAC for default_config, context and experiment#910
Merged
ayushjain17 merged 4 commits intomainfrom Mar 17, 2026
Merged
feat(authz): ABAC for default_config, context and experiment#910ayushjain17 merged 4 commits intomainfrom
ayushjain17 merged 4 commits intomainfrom
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAuthorization system integration across context-aware config and experimentation handlers. Added authorization checks via AuthZ to create, update, move, delete, and bulk operations. Restructured casbin API into nested modules (policy, role, action_group). Enhanced AuthZ middleware with persistent state and authorization methods. Updated Authorizer trait signature for string reference handling. Added Deref derives to wrapper types. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler
participant AuthZ
participant Authorizer
participant Database
Client->>Handler: POST /create (with overrides)
Handler->>AuthZ: authorize_create(overrides)
AuthZ->>Authorizer: is_allowed(resource, action, attributes)
Authorizer-->>AuthZ: allowed/denied
AuthZ-->>Handler: Ok() / Forbidden
alt Authorization Success
Handler->>Database: Insert new config
Database-->>Handler: Success
Handler-->>Client: 200 OK
else Authorization Failure
Handler-->>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)
📝 Coding Plan
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: 5
🧹 Nitpick comments (1)
crates/experimentation_platform/src/api/experiments/handlers.rs (1)
136-146: Consider using slice parameter type.The function works correctly. Minor style suggestion: prefer
&[Variant]over&Vec<Variant>for the parameter type, which is more idiomatic in Rust and allows passing slices directly.♻️ Optional idiomatic fix
async fn authorize_create( auth_z: AuthZ<AuthZActionCreate>, - variants: &Vec<Variant>, + variants: &[Variant], ) -> superposition::Result<()> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/experimentation_platform/src/api/experiments/handlers.rs` around lines 136 - 146, Change the authorize_create signature to accept a slice instead of a Vec reference (i.e., replace &Vec<Variant> with &[Variant]) and update any callers to pass slices or vec.as_slice() as needed; inside the function keep all logic the same (use variants.iter()...) so no other internal changes are required—this makes the API more idiomatic and allows callers to pass either Vec or slice directly while still referencing the existing function name authorize_create and its use of Variant and VariantType.
🤖 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/context/handlers.rs`:
- Around line 193-209: authorize_update reads current overrides
(operations::get_overrides_from_identifier), calls auth_z.authorize_action with
changed_keys and then returns, creating a race where the overrides can change
before the write; change the flow so the read and authorization are bound to the
same locked write path: either perform a SELECT ... FOR UPDATE (or start the
write transaction/lock) before calling auth_z.authorize_action inside
authorize_update, or record the row version/override_id during the locked read
and re-check it immediately before committing (and reject the write if it
changed); apply the same pattern to the other authorization helpers referenced
in the review (the blocks around lines 226-264, 303-316, 619-632, 718-772) so
authorization is always based on the locked/transactional state or fails if the
version changed.
In `@crates/context_aware_config/src/api/context/helpers.rs`:
- Around line 495-516: The changed_keys function uses sentinel Value::String
markers which conflates actual string values with absence; replace the sentinel
logic by comparing Option<&Value> directly so None stays distinct from Some(..).
In changed_keys, collect the union of keys as before, but in the filter use
old_values.get(key) and new_values.get(key) (i.e., Option<&Value>) and keep keys
where those Option<&Value> differ (old_values.get(*key) !=
new_values.get(*key)); remove the unwrap_or sentinel branches so
authorize_update receives real None vs Some distinctions.
In `@crates/context_aware_config/src/api/context/operations.rs`:
- Around line 111-123: The match in get_overrides_from_identifier returns a
reference to a temporary String created by &hash(...) which will be dropped,
causing E0716; fix it by producing an owned String and then taking a reference
to it for the call to get_overrides_from_ctx_id: change the local variable
context_id to an owned String (e.g., let context_id = match ... {
Identifier::Context(context) => hash(...), Identifier::Id(id) => id.clone() })
and then call get_overrides_from_ctx_id(&context_id, schema_name, conn) so no
reference points into a temporary; reference the functions/variants
get_overrides_from_identifier, Identifier::Context, hash, and
get_overrides_from_ctx_id when making the change.
In `@crates/frontend/src/api.rs`:
- Around line 140-147: The code uses matches!(scope, AuthzScope::Admin) which
moves the non-Copy AuthzScope and makes scope unavailable for subsequent calls;
in the functions list_domain, add_domain, and delete_domain replace
matches!(scope, AuthzScope::Admin) with a borrow comparison scope ==
AuthzScope::Admin so scope is not moved and can be passed into
casbin_url_and_headers and other callers.
In `@crates/superposition_types/src/lib.rs`:
- Around line 135-144: The types Cac<T> and Exp<T> should not expose DerefMut
because that allows mutation of the inner value after validation; remove
DerefMut from the derive attributes (and any use/import of DerefMut) for both
Cac<T> and Exp<T> so they only implement immutable Deref, keeping validation
guarantees enforced by preventing mutable access to the wrapped value.
---
Nitpick comments:
In `@crates/experimentation_platform/src/api/experiments/handlers.rs`:
- Around line 136-146: Change the authorize_create signature to accept a slice
instead of a Vec reference (i.e., replace &Vec<Variant> with &[Variant]) and
update any callers to pass slices or vec.as_slice() as needed; inside the
function keep all logic the same (use variants.iter()...) so no other internal
changes are required—this makes the API more idiomatic and allows callers to
pass either Vec or slice directly while still referencing the existing function
name authorize_create and its use of Variant and VariantType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74f4375e-1a01-4a7f-9717-b306b13d84e9
📒 Files selected for processing (13)
crates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/context/helpers.rscrates/context_aware_config/src/api/context/operations.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/experimentation_platform/src/api/experiments/helpers.rscrates/frontend/src/api.rscrates/frontend/src/components/authz.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/no_auth.rscrates/superposition_types/src/lib.rs
Comment on lines
+193
to
+209
| async fn authorize_update<A: AuthZAction>( | ||
| auth_z: &AuthZ<A>, | ||
| context: &Identifier, | ||
| new_overrides: &Cac<Overrides>, | ||
| schema_name: &SchemaName, | ||
| conn: &mut DBConnection, | ||
| ) -> superposition::Result<()> { | ||
| let overrides = | ||
| operations::get_overrides_from_identifier(context, schema_name, conn)?; | ||
|
|
||
| auth_z | ||
| .authorize_action( | ||
| &AuthZActionUpdate::get(), | ||
| &changed_keys(&overrides, &new_overrides.deref()), | ||
| ) | ||
| .await | ||
| } |
Contributor
There was a problem hiding this comment.
Don't authorize against stale overrides.
These helpers read the current overrides, await AuthZ, and only then start the write transaction. Another request can update or delete the same context in between, so ABAC approves one key set while the mutation applies to another. The same race exists in bulk because the entire batch is authorized before the transaction begins.
Please bind the read + authorization decision to the same locked write path, or reject the write if the row's override_id / version changed after authorization.
Also applies to: 226-264, 303-316, 619-632, 718-772
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 193 -
209, authorize_update reads current overrides
(operations::get_overrides_from_identifier), calls auth_z.authorize_action with
changed_keys and then returns, creating a race where the overrides can change
before the write; change the flow so the read and authorization are bound to the
same locked write path: either perform a SELECT ... FOR UPDATE (or start the
write transaction/lock) before calling auth_z.authorize_action inside
authorize_update, or record the row version/override_id during the locked read
and re-check it immediately before committing (and reject the write if it
changed); apply the same pattern to the other authorization helpers referenced
in the review (the blocks around lines 226-264, 303-316, 619-632, 718-772) so
authorization is always based on the locked/transactional state or fails if the
version changed.
Contributor
There was a problem hiding this comment.
@ayushjain17, here's a concrete fix using an optimistic-lock / version-token pattern — the only approach that cleanly works when async authorization must happen outside a sync Diesel transaction closure.
The pattern
Each authorize_* helper returns the snapshot version (last_modified_at) it read when fetching overrides. Inside the write transaction, before mutating, a SELECT … FOR UPDATE re-fetches the row and rejects the write if the version changed since authorization.
Step 1 — operations.rs: add a version-aware reader
/// Returns overrides *and* the last_modified_at snapshot used to detect races.
pub fn get_overrides_and_version_from_ctx_id(
ctx_id: &str,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> result::Result<(Overrides, chrono::DateTime<chrono::Utc>)> {
use superposition_types::database::schema::contexts::dsl::*;
let ctx: Context = contexts
.filter(id.eq(ctx_id))
.schema_name(schema_name)
.get_result(conn)?;
Ok((
serde_json::from_value(ctx.override_.clone()).map_err(|e| unexpected_error!(e))?,
ctx.last_modified_at,
))
}
pub fn get_overrides_and_version_from_identifier(
identifier: &Identifier,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> result::Result<(Overrides, chrono::DateTime<chrono::Utc>)> {
let ctx_id = match identifier {
Identifier::Context(ctx) => hash(&Value::Object(ctx.clone().into_inner().into())),
Identifier::Id(id) => id.clone(),
};
get_overrides_and_version_from_ctx_id(&ctx_id, schema_name, conn)
}Step 2 — handlers.rs: return the snapshot from each auth helper
async fn authorize_update<A: AuthZAction>(
auth_z: &AuthZ<A>,
context: &Identifier,
new_overrides: &Cac<Overrides>,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> superposition::Result<chrono::DateTime<chrono::Utc>> { // <-- returns snapshot
let (overrides, snapshot_version) =
operations::get_overrides_and_version_from_identifier(context, schema_name, conn)?;
auth_z
.authorize_action(
&AuthZActionUpdate::get(),
&changed_keys(&overrides, new_overrides.deref()),
)
.await?;
Ok(snapshot_version)
}
async fn authorize_action<A: AuthZAction>( // used by move_handler
auth_z: &AuthZ<A>,
ctx_id: &str,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> superposition::Result<chrono::DateTime<chrono::Utc>> {
let (overrides, snapshot_version) =
operations::get_overrides_and_version_from_ctx_id(ctx_id, schema_name, conn)?;
auth_z
.authorize_action(
&AuthZActionMove::get(),
&overrides.keys().collect::<Vec<_>>(),
)
.await?;
Ok(snapshot_version)
}
async fn authorize_delete<A: AuthZAction>(
auth_z: &AuthZ<A>,
ctx_id: &str,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> superposition::Result<chrono::DateTime<chrono::Utc>> {
let (overrides, snapshot_version) =
operations::get_overrides_and_version_from_ctx_id(ctx_id, schema_name, conn)?;
auth_z
.authorize_action(
&AuthZActionDelete::get(),
&overrides.keys().collect::<Vec<_>>(),
)
.await?;
Ok(snapshot_version)
}For bulk, collect a map of ctx_id → snapshot_version:
async fn authorize_bulk<A: AuthZAction>(
auth_z: &AuthZ<A>,
ops: &Vec<ContextAction>,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> superposition::Result<std::collections::HashMap<String, chrono::DateTime<chrono::Utc>>> {
let mut snapshots = std::collections::HashMap::new();
for op in ops {
match op {
ContextAction::Put(put_req) => {
authorize_create(auth_z, &put_req.r#override).await?;
// No version needed for new inserts (no existing row).
}
ContextAction::Replace(update_req) => {
let ctx_id = match &update_req.context {
Identifier::Context(ctx) => {
superposition_core::helpers::hash(
&serde_json::Value::Object(ctx.clone().into_inner().into())
)
}
Identifier::Id(id) => id.clone(),
};
let v = authorize_update(
auth_z,
&update_req.context,
&update_req.override_,
schema_name,
conn,
).await?;
snapshots.insert(ctx_id, v);
}
ContextAction::Delete(ctx_id) => {
let v = authorize_delete(auth_z, ctx_id, schema_name, conn).await?;
snapshots.insert(ctx_id.clone(), v);
}
ContextAction::Move { id: ctx_id, .. } => {
let v = authorize_action(auth_z, ctx_id, schema_name, conn).await?;
snapshots.insert(ctx_id.clone(), v);
}
}
}
Ok(snapshots)
}Step 3 — Add a version-guard helper (called inside transactions)
/// Called inside a Diesel transaction. Locks the row with SELECT FOR UPDATE
/// and rejects if `last_modified_at` changed since authorization.
fn assert_context_version_unchanged(
ctx_id: &str,
authorized_version: chrono::DateTime<chrono::Utc>,
schema_name: &SchemaName,
conn: &mut DBConnection,
) -> superposition::Result<()> {
use superposition_types::database::schema::contexts::dsl::*;
use diesel::dsl::for_update;
let current: chrono::DateTime<chrono::Utc> = contexts
.filter(id.eq(ctx_id))
.schema_name(schema_name)
.for_update() // SELECT … FOR UPDATE — locks the row
.select(last_modified_at)
.get_result(conn)?;
if current != authorized_version {
return Err(superposition_macros::unexpected_error!(
"Context {} was concurrently modified; authorization may be stale. \
Please retry the request.",
ctx_id
));
}
Ok(())
}Note:
for_update()requires Diesel'slocking_clausefeature; if not already enabled, adddiesel = { features = ["postgres", "with-deprecated"] }or enablefor-update-lockingin your Diesel feature flags.
Step 4 — Thread the snapshot into each handler's transaction
update_handler:
let authorized_version = authorize_update(
&_auth_z,
&req.context,
&req.override_,
&workspace_context.schema_name,
&mut db_conn,
).await?;
let ctx_id = /* resolve identifier to string, same logic as operations.rs */;
let (override_resp, version_id) = db_conn
.transaction::<_, superposition::AppError, _>(|transaction_conn| {
assert_context_version_unchanged(
&ctx_id, authorized_version,
&workspace_context.schema_name, transaction_conn
)?;
// ... existing update logic
})?;move_handler and delete_handler: same pattern — pass authorized_version into the transaction and call assert_context_version_unchanged as the first statement.
bulk_operations_handler: pass the snapshots map in; for each Replace, Delete, and Move action call assert_context_version_unchanged using the matching entry in the map before the mutation.
Why this is correct
| Concern | How it's addressed |
|---|---|
| Async auth can't run inside sync Diesel closure | Auth runs outside; only a sync SELECT FOR UPDATE guard runs inside |
| Row mutated between auth and write | FOR UPDATE lock + version comparison detects and rejects the stale write |
Put (new insert) has no version to check |
Not vulnerable — there is no pre-existing row to be concurrently modified |
| Bulk batch authorized before transaction | Each item's snapshot is verified under lock at the start of its mutation step |
🧠 Learnings used
Learnt from: ayushjain17
Repo: juspay/superposition PR: 543
File: crates/service_utils/src/middlewares/auth_z.rs:141-152
Timestamp: 2026-01-02T20:59:01.233Z
Learning: In crates/service_utils/src/middlewares/auth_z.rs, the AuthZHandler::init function is intentionally designed to panic on startup if AUTH_Z_PROVIDER environment variable is missing or set to an unknown value. This fail-fast behavior is expected and preferred for this critical authorization configuration.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 908
File: crates/context_aware_config/src/api/secrets/handlers.rs:283-283
Timestamp: 2026-03-09T12:19:57.206Z
Learning: In crates/context_aware_config/src/api/secrets/handlers.rs (and similar handler files in the juspay/superposition repo), `declare_resource!(ResourceName)` is a file-level macro that sets the default resource for all `#[authorized]` handlers that do NOT explicitly specify a resource. When a handler uses `#[authorized(resource = SomeResource)]` with an explicit resource argument, no separate `declare_resource!(SomeResource)` is needed — the inline resource specification is sufficient for AuthZ registry registration. Do not flag missing `declare_resource!` for resources already specified inline via `#[authorized(resource = ...)]`.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 910
File: crates/frontend/src/api.rs:159-166
Timestamp: 2026-03-12T06:30:24.697Z
Learning: In `crates/frontend/src/api.rs`, using `matches!(scope, AuthzScope::Admin)` where `scope: AuthzScope` (non-Copy enum with `Clone, PartialEq, Eq`) followed by passing `scope` to subsequent calls (e.g., `casbin_url_and_headers`) is valid and compiles correctly. Do not flag `matches!` on a non-Copy enum value as a move issue when the matched variant (`Admin`) has no fields and the code path that uses `scope` afterward is only reached when the condition is false. This pattern is accepted by the Rust compiler in the `superposition` codebase.
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.
ayushjain17
force-pushed
the
auth_z_abac
branch
4 times, most recently
from
020c0af to
388342e
Compare
sauraww
reviewed
Mar 16, 2026
sauraww
reviewed
Mar 16, 2026
ayushjain17
force-pushed
the
auth_z_abac
branch
2 times, most recently
from
b211a5f to
83c7c24
Compare
sauraww
approved these changes
Mar 16, 2026
Datron
reviewed
Mar 17, 2026
Collaborator
Datron
left a comment
Datron
left a comment
There was a problem hiding this comment.
Mostly nitpicks, please take a look
| Ok(HttpResponse::Ok().body("Policy already exists")) | ||
| } | ||
| Ok(Json(ActionResponse { | ||
| success: added, |
Collaborator
There was a problem hiding this comment.
I'm guessing this says either true or false, wouldn't the 200 OK response mean success? Do we need to send this back?
Collaborator
Author
There was a problem hiding this comment.
yeah, because casbin does not give me an error when it is trying to delete something which does not exist
Collaborator
There was a problem hiding this comment.
Can we check the success boolean in code and throw the error in the handler? Return 200 and saying the request failed is bad API semantics
ayushjain17
force-pushed
the
auth_z_abac
branch
from
51d1775 to
7b7ae90
Compare
ayushjain17
force-pushed
the
auth_z_abac
branch
from
7b7ae90 to
4e11f7a
Compare
Datron
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Change log
Added ABAC support for all write APIs for default config, context and experiment - essentially for all those resources which have config as a resource in them
Summary by CodeRabbit
New Features
Improvements