feat: AuthZ setup #543
feat: AuthZ setup #543
Conversation
ayushjain17
force-pushed
the
auth_z
branch
5 times, most recently
from
6c52611 to
4928220
Compare
Datron
force-pushed
the
main
branch
8 times, most recently
from
a858147 to
3a83502
Compare
Datron
force-pushed
the
main
branch
10 times, most recently
from
04b8065 to
c899522
Compare
Datron
force-pushed
the
main
branch
3 times, most recently
from
e6a8db3 to
74222bf
Compare
ayushjain17
force-pushed
the
auth_z
branch
2 times, most recently
from
243bffa to
13545ac
Compare
ayushjain17
force-pushed
the
auth_z
branch
2 times, most recently
from
1b5d05e to
fa3117f
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive authorization framework and standardizes handler naming conventions across the backend and frontend. It adds authentication/authorization middleware, procedural macros, and refactors API handlers to consistently use generic names with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as Actix Middleware
participant AuthZ as AuthZHandler
participant Resource as Resource Middleware
participant Authorizer
participant Handler as Request Handler
Client->>Middleware: HTTP Request
Middleware->>AuthZ: Extract/Initialize AuthZHandler
Middleware->>Resource: Inject Resource into Extensions
Middleware->>Middleware: Pass to Next Layer
rect rgb(200, 220, 255)
note over Handler: @[auth_action] Handler
Handler->>Handler: Extract AuthZ<Action> from Request
Handler->>Authorizer: is_allowed(WorkspaceContext, User, Resource, Action)
alt Authorization Allowed
Authorizer-->>Handler: Ok(true)
Handler->>Handler: Process Request
Handler-->>Client: 200 OK / Response
else Authorization Denied
Authorizer-->>Handler: Ok(false)
Handler-->>Client: 403 Forbidden
else Missing Context
Handler-->>Client: 401 Unauthorized
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/frontend/src/utils.rs (1)
240-251: Avoid sending an invalid cookie header when no cookie exists.When
cookieisNone, the code sends"No Cookie found"as the cookie header value. This sends a malformed/invalid cookie to the server, which could cause unexpected behavior or confuse logging/debugging.🔎 Proposed fix
let ssr_headers = use_context::<Option<SsrSharedHttpRequestHeaders>>().flatten(); let cookie = ssr_headers.and_then(|h| h.cookie.clone()); let mut request_builder = HTTP_CLIENT.request(method.clone(), url).headers(headers); request_builder = match (method, body) { (reqwest::Method::GET | reqwest::Method::DELETE, _) => request_builder, (_, Some(data)) => request_builder.json(&data), _ => request_builder, }; - let response = request_builder - .header("cookie", cookie.unwrap_or("No Cookie found".to_string())) - .send() - .await - .map_err(|err| err.to_string())?; + let mut request_builder = request_builder; + if let Some(cookie_value) = cookie { + request_builder = request_builder.header("cookie", cookie_value); + } + + let response = request_builder + .send() + .await + .map_err(|err| err.to_string())?;crates/superposition/src/organisation/handlers.rs (1)
53-59: Inconsistent user identifier usage:get_username()vsget_email().In
create_handler,created_byandupdated_byuseuser.get_username()(lines 53, 59), but inupdate_handler,updated_byusesuser.get_email()(line 86). This inconsistency may cause confusion when auditing who modified records.Proposed fix - use consistent identifier
// In update_handler (line 86): - .set((req, updated_at.eq(now), updated_by.eq(user.get_email()))) + .set((req, updated_at.eq(now), updated_by.eq(user.get_username())))Or alternatively, update
create_handlerto useget_email()if that's the preferred identifier across the codebase.Also applies to: 86-86
crates/experimentation_platform/src/api/experiment_groups/handlers.rs (1)
450-455: Blockingthread::sleepinside async context and transaction.Using
std::thread::sleepblocks the current thread, which is problematic in an async runtime. Additionally, sleeping inside a database transaction holds the transaction open longer, potentially causing lock contention. Consider usingtokio::time::sleepoutside the transaction loop or removing the delay entirely if it's only for rate limiting during backfill.🔎 Suggested approach
If the delay is for rate limiting, consider processing in batches outside the transaction:
- for experiment in experiments { + for experiment in experiments.iter() { // ... existing logic ... - std::thread::sleep(std::time::Duration::from_millis(delay)); } + Ok(results) + })?; + + // If delay is needed, apply between separate transactions per experimentAlternatively, if you must keep the delay, use
tokio::time::sleepbut note this still extends transaction duration:// Outside transaction, or restructure to commit per experiment tokio::time::sleep(std::time::Duration::from_millis(delay)).await;
🧹 Nitpick comments (8)
crates/service_utils/src/db/utils.rs (1)
36-42: Consider validating the env_prefix parameter to prevent unintended behavior.The
env_prefixparameter enables multi-tenant database configuration, but there's an edge case: if a caller passesSome("")(empty string), line 41 will produce"_"as the prefix, resulting in environment variable names like_DB_USERand_DB_HOST.Consider either:
- Documenting that empty strings will produce an underscore prefix
- Filtering out empty strings:
env_prefix.filter(|s| !s.is_empty()).map(...)- Validating and returning an error for empty strings
🔎 Proposed fix to filter empty strings
- let env_prefix = env_prefix.map(|s| format!("{s}_")).unwrap_or_default(); + let env_prefix = env_prefix + .filter(|s| !s.is_empty()) + .map(|s| format!("{s}_")) + .unwrap_or_default();crates/service_utils/src/middlewares/workspace_context.rs (1)
109-111: Extract the hardcoded "test" workspace_id into a constant for clarity.When
enable_workspace_idisfalse, the code defaults toWorkspaceId(String::from("test")). This is intentional for administrative endpoints like/workspacesand/superposition/organisationswhere workspace tracking is disabled. However, the magic string should be defined as a constant (e.g.,const DEFAULT_DISABLED_WORKSPACE: &str = "test") to improve maintainability and signal intent clearly.crates/superposition_derives/src/lib.rs (1)
380-387: Consider visibility and uniqueness of generated struct.The generated
AuthZAction*struct is private (nopubmodifier). If the handler function is public and used across module boundaries, this could cause visibility issues. Additionally, if two handlers in the same module derive the same action name, there will be a naming conflict.Consider:
- Adding
#[allow(non_camel_case_types)]to suppress potential warnings.- Including the function name or a unique suffix in the struct name to prevent conflicts.
🔎 Suggested improvement
quote! { + #[allow(non_camel_case_types)] struct #struct_name; impl service_utils::middlewares::auth_z::Action for #struct_name { fn get() -> String { #action_name.to_string() } } #input_fn }crates/superposition_types/src/result.rs (1)
24-25: Consider clarifying the error message terminology.The error message displays "unauthorized" but the variant name is
Forbiddenand returns HTTP 403. In HTTP semantics, 401 Unauthorized means "not authenticated" while 403 Forbidden means "authenticated but not authorized." Consider changing the message to "forbidden" for consistency.🔎 Proposed fix
- #[error("unauthorized ( `{0}` )")] + #[error("forbidden ( `{0}` )")] Forbidden(String),crates/service_utils/src/middlewares/auth_z.rs (2)
15-15: Unused import:aws_sdk_kms::Client.This import is only used in commented-out Casbin code. Consider removing it until Casbin support is enabled to avoid dead code warnings.
170-183: Duplicate environment variable reading logic.
AuthZManager::initduplicates the sameget_from_env_unsafe("AUTH_Z_PROVIDER").unwrap()pattern asAuthZHandler::init. This creates maintenance burden and inconsistent error handling risk.Consider extracting a shared helper or consolidating initialization logic.
crates/superposition/src/workspace/handlers.rs (1)
161-161: Minor: Prefer consistent string formatting style.Line 161 uses string concatenation (
org_id.clone().0 + "_" + workspace_name.as_str()) while line 104 usesformat!macro. Consider usingformat!for consistency with the rest of the codebase.🔎 Suggested change
- let schema_name = SchemaName(org_id.clone().0 + "_" + workspace_name.as_str()); + let schema_name = SchemaName(format!("{}_{}", &org_id.0, &workspace_name));crates/experimentation_platform/src/api/experiment_groups/handlers.rs (1)
398-460: Consider the comment's instruction to remove after backfilling.The comment on line 398 states "Remove this after backfilling experiment groups". Ensure this endpoint is tracked for removal or is behind a feature flag to prevent accidental use in production after backfill is complete.
Would you like me to open an issue to track the removal of this backfill endpoint after migration is complete?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (65)
.env.examplecrates/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/type_templates/handlers.rscrates/context_aware_config/src/api/variables/handlers.rscrates/experimentation_platform/Cargo.tomlcrates/experimentation_platform/src/api/experiment_groups/handlers.rscrates/experimentation_platform/src/api/experiments/cac_api.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/frontend/src/api.rscrates/frontend/src/components/default_config_form.rscrates/frontend/src/components/default_config_form/utils.rscrates/frontend/src/components/dimension_form.rscrates/frontend/src/components/experiment_conclude_form/utils.rscrates/frontend/src/components/experiment_form.rscrates/frontend/src/components/experiment_form/utils.rscrates/frontend/src/components/experiment_ramp_form/utils.rscrates/frontend/src/components/function_form/utils.rscrates/frontend/src/components/type_template_form/utils.rscrates/frontend/src/pages/compare_overrides.rscrates/frontend/src/pages/config_version.rscrates/frontend/src/pages/config_version_list.rscrates/frontend/src/pages/context_override.rscrates/frontend/src/pages/context_override/utils.rscrates/frontend/src/pages/default_config_list.rscrates/frontend/src/pages/dimensions.rscrates/frontend/src/pages/experiment.rscrates/frontend/src/pages/experiment_group_listing.rscrates/frontend/src/pages/experiment_list.rscrates/frontend/src/pages/function.rscrates/frontend/src/pages/function/function_list.rscrates/frontend/src/pages/function/publish_form.rscrates/frontend/src/pages/function/utils.rscrates/frontend/src/pages/home.rscrates/frontend/src/pages/type_templates.rscrates/frontend/src/pages/webhooks.rscrates/frontend/src/types.rscrates/frontend/src/utils.rscrates/service_utils/Cargo.tomlcrates/service_utils/src/db/utils.rscrates/service_utils/src/middlewares.rscrates/service_utils/src/middlewares/auth_n/authentication.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/middlewares/resource.rscrates/service_utils/src/middlewares/workspace_context.rscrates/service_utils/src/service/types.rscrates/superposition/src/main.rscrates/superposition/src/organisation.rscrates/superposition/src/organisation/handlers.rscrates/superposition/src/resolve/handlers.rscrates/superposition/src/webhooks/handlers.rscrates/superposition/src/workspace/handlers.rscrates/superposition_derives/src/lib.rscrates/superposition_macros/src/lib.rscrates/superposition_types/src/api.rscrates/superposition_types/src/api/organisation.rscrates/superposition_types/src/api/workspace.rscrates/superposition_types/src/result.rs
💤 Files with no reviewable changes (1)
- crates/superposition/src/organisation.rs
🧰 Additional context used
🧬 Code graph analysis (38)
crates/frontend/src/components/type_template_form/utils.rs (1)
crates/frontend/src/utils.rs (1)
use_host_server(60-69)
crates/frontend/src/pages/function.rs (1)
crates/frontend/src/api.rs (1)
fetch_function(307-324)
crates/frontend/src/components/function_form/utils.rs (1)
crates/frontend/src/utils.rs (1)
use_host_server(60-69)
crates/service_utils/src/middlewares/auth_z/authorization.rs (1)
crates/service_utils/src/middlewares/auth_z/no_auth.rs (1)
is_allowed(11-20)
crates/frontend/src/components/default_config_form/utils.rs (1)
crates/frontend/src/utils.rs (1)
use_host_server(60-69)
crates/frontend/src/components/experiment_form/utils.rs (1)
crates/frontend/src/utils.rs (1)
use_host_server(60-69)
crates/frontend/src/pages/config_version_list.rs (2)
crates/frontend/src/pages/workspace.rs (1)
workspace(28-285)crates/frontend/src/api.rs (3)
fetch_all(72-89)fetch_all(448-464)fetch_all(989-1012)
crates/frontend/src/pages/context_override.rs (2)
crates/frontend/src/api.rs (6)
fetch_context(351-375)fetch(91-108)fetch(120-137)fetch(868-890)fetch(1014-1028)fetch(1163-1186)crates/frontend/src/pages/dimensions.rs (1)
dimensions(25-145)
crates/service_utils/src/middlewares/auth_z/no_auth.rs (1)
crates/service_utils/src/middlewares/auth_z/authorization.rs (1)
is_allowed(13-20)
crates/service_utils/src/middlewares/auth_z.rs (2)
crates/service_utils/src/middlewares/auth_z/authorization.rs (1)
is_allowed(13-20)crates/service_utils/src/middlewares/auth_z/no_auth.rs (1)
is_allowed(11-20)
crates/superposition_macros/src/lib.rs (1)
crates/experimentation_client/src/interface.rs (1)
to_string(45-50)
crates/frontend/src/components/experiment_ramp_form/utils.rs (1)
crates/frontend/src/utils.rs (4)
construct_request_headers(202-217)parse_json_response(219-228)request(292-302)use_host_server(60-69)
crates/frontend/src/components/experiment_conclude_form/utils.rs (1)
crates/frontend/src/utils.rs (1)
use_host_server(60-69)
crates/superposition_types/src/api/organisation.rs (2)
crates/superposition/src/organisation/handlers.rs (1)
organisations(100-102)crates/superposition/src/workspace/handlers.rs (1)
organisations(97-99)
crates/frontend/src/components/dimension_form.rs (2)
crates/frontend/src/pages/dimensions.rs (1)
dimensions(25-145)crates/frontend/src/api.rs (23)
None(57-57)None(83-83)None(102-102)None(131-131)None(150-150)None(227-227)None(247-247)None(274-274)None(299-299)None(318-318)None(343-343)None(369-369)None(389-389)None(408-408)None(421-421)None(602-602)fetch(91-108)fetch(120-137)fetch(868-890)fetch(1014-1028)fetch(1163-1186)fetch_functions(282-305)fetch_types(426-443)
crates/superposition_derives/src/lib.rs (1)
crates/service_utils/src/middlewares/auth_z.rs (1)
get(28-28)
crates/frontend/src/pages/webhooks.rs (1)
crates/frontend/src/api.rs (1)
fetch_webhooks(591-608)
crates/service_utils/src/middlewares/resource.rs (2)
crates/service_utils/src/middlewares/workspace_context.rs (4)
new(25-30)new_transform(45-51)call(72-140)req(78-78)crates/service_utils/src/middlewares/auth_z.rs (6)
new_transform(156-161)call(119-122)req(41-41)req(52-52)req(63-63)req(74-74)
crates/frontend/src/components/experiment_form.rs (2)
crates/frontend/src/api.rs (1)
fetch_experiment(378-395)crates/experimentation_platform/src/api/experiments/helpers.rs (1)
fetch_experiment(806-818)
crates/superposition/src/resolve/handlers.rs (1)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)
crates/context_aware_config/src/api/context/handlers.rs (3)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)crates/service_utils/src/middlewares/auth_n/authentication.rs (1)
routes(36-36)crates/service_utils/src/middlewares/auth_z.rs (1)
get(28-28)
crates/service_utils/src/db/utils.rs (1)
crates/service_utils/src/aws/kms.rs (1)
decrypt(5-27)
crates/context_aware_config/src/api/audit_log/handlers.rs (1)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)
crates/frontend/src/pages/context_override/utils.rs (1)
crates/frontend/src/utils.rs (4)
construct_request_headers(202-217)parse_json_response(219-228)request(292-302)use_host_server(60-69)
crates/superposition/src/webhooks/handlers.rs (2)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)crates/service_utils/src/middlewares/auth_z.rs (1)
get(28-28)
crates/context_aware_config/src/api/config/handlers.rs (1)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)
crates/frontend/src/utils.rs (1)
crates/frontend/src/api.rs (1)
execute_value_compute_function(763-803)
crates/experimentation_platform/src/api/experiments/cac_api.rs (5)
crates/context_aware_config/src/helpers.rs (4)
state(287-289)state(291-293)state(301-303)state(306-308)crates/superposition_sdk/src/operation/get_resolved_config_with_identifier/_get_resolved_config_with_identifier_input.rs (2)
context_id(56-58)context_id(181-184)crates/superposition_sdk/src/operation/get_resolved_config_with_identifier/builders.rs (1)
context_id(189-192)crates/superposition_sdk/src/operation/get_resolved_config/_get_resolved_config_input.rs (2)
context_id(54-56)context_id(174-177)crates/superposition_sdk/src/operation/get_resolved_config/builders.rs (1)
context_id(189-192)
crates/context_aware_config/src/api/functions/handlers.rs (1)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)
crates/superposition_types/src/api/workspace.rs (2)
clients/javascript/sdk/src/models/models_0.ts (2)
WorkspaceStatus(1539-1542)WorkspaceStatus(1546-1546)clients/python/sdk/superposition_sdk/models.py (1)
WorkspaceStatus(6615-6617)
crates/frontend/src/pages/experiment.rs (1)
crates/frontend/src/api.rs (7)
fetch_experiment(378-395)fetch(91-108)fetch(120-137)fetch(868-890)fetch(1014-1028)fetch(1163-1186)fetch_default_config(40-63)
crates/frontend/src/pages/compare_overrides.rs (3)
crates/frontend/src/pages/dimensions.rs (1)
dimensions(25-145)crates/frontend/src/api.rs (5)
fetch(91-108)fetch(120-137)fetch(868-890)fetch(1014-1028)fetch(1163-1186)crates/superposition_types/src/custom_query.rs (1)
all_entries(245-251)
crates/context_aware_config/src/api/type_templates/handlers.rs (3)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)crates/service_utils/src/middlewares/auth_z.rs (1)
get(28-28)crates/service_utils/src/middlewares/auth_n/authentication.rs (1)
routes(36-36)
crates/frontend/src/pages/type_templates.rs (2)
crates/frontend/src/pages/workspace.rs (1)
workspace(28-285)crates/frontend/src/api.rs (1)
fetch_types(426-443)
crates/experimentation_platform/src/api/experiment_groups/handlers.rs (3)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)crates/service_utils/src/middlewares/workspace_context.rs (1)
req(78-78)crates/experimentation_platform/src/api/experiment_groups/helpers.rs (2)
fetch_experiment_group(442-453)member_experiments(98-104)
crates/frontend/src/pages/dimensions.rs (1)
crates/frontend/src/api.rs (5)
fetch(91-108)fetch(120-137)fetch(868-890)fetch(1014-1028)fetch(1163-1186)
crates/context_aware_config/src/api/variables/handlers.rs (1)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)
crates/context_aware_config/src/api/dimension/handlers.rs (4)
crates/superposition_derives/src/lib.rs (1)
auth_action(349-392)crates/service_utils/src/middlewares/auth_z.rs (1)
get(28-28)crates/service_utils/src/middlewares/auth_n/authentication.rs (1)
routes(36-36)crates/context_aware_config/src/api/dimension/utils.rs (2)
dimensions(26-28)dimensions(148-166)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 38-38: [UnorderedKey] The AUTH_Z_PROVIDER key should go before the AWS_ACCESS_KEY_ID key
(UnorderedKey)
[warning] 40-40: [UnorderedKey] The ENCRYPTED_KEYS key should go before the HOSTNAME key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Provider Tests (py)
- GitHub Check: Provider Tests (rust)
- GitHub Check: Provider Tests (kotlin)
- GitHub Check: Provider Tests (js)
- GitHub Check: Testing
- GitHub Check: Check formatting
- GitHub Check: checks (aarch64-darwin)
🔇 Additional comments (128)
crates/superposition_types/src/api/workspace.rs (1)
6-9: LGTM!The conditional import for
workspacesschema is correctly feature-gated underdiesel_derives, which aligns with its usage in theUpdateWorkspaceRequeststruct's#[diesel(table_name = workspaces)]attribute on line 79. The import organization is clean and follows the pattern established elsewhere in the file.crates/service_utils/Cargo.toml (1)
30-30: Dependency correctly added and properly used. The workspace definessuperposition_macros = { path = "crates/superposition_macros", version = "0.96.1" }, the entry is correctly formatted and alphabetically ordered, and it's actively used incrates/service_utils/src/middlewares/auth_z.rsfor theforbiddenandunexpected_errormacros.crates/frontend/src/pages/function/function_list.rs (2)
53-53: LGTM: Improved naming consistency.The parameter rename from
current_tenanttoworkspacealigns well with the context variable defined on line 29, improving code readability.
55-55: Function signature is compatible with the call.The
fetch_functionssignature incrates/frontend/src/api.rsaccepts&strparameters fortenantandorg_id, and the call at line 55 passes&workspaceand&orgas borrowed String references, which correctly coerce to&strthrough Rust's Deref mechanism. No issues found.crates/service_utils/src/db/utils.rs (4)
2-5: LGTM: Import reformatting improves readability.The multi-line diesel import formatting is a cosmetic improvement with no functional impact.
43-56: LGTM: Consistent env_prefix usage across database configuration.The
env_prefixparameter is applied consistently across all database configuration variables (DB_USER,DB_PASSWORD,DB_HOST,DB_NAME). The URL-encoding of the decrypted password (line 51) is appropriate for use in PostgreSQL connection strings.
59-70: LGTM: Backward compatibility maintained.Passing
Nonefor theenv_prefixparameter preserves the original behavior ofinit_pool_manager, ensuring backward compatibility for existing initialization code.
20-20: No action needed—removal of URL-encoding is correct for these use cases.The
SUPERPOSITION_TOKENandOIDC_CLIENT_SECRETare used in contexts where URL-encoding is not required and would be incorrect:
superposition_tokenis used in HTTP Authorization headers and string comparisons, not in URL syntaxclient_secretis passed to the OIDC library which expects the raw decrypted value for OAuth token exchangeURL-encoding is appropriately retained only for the database password (line 51), which is embedded in a PostgreSQL connection string URL where special characters must be encoded.
crates/frontend/src/pages/function.rs (1)
110-111: LGTM! Parameter passing updated correctly.The change to pass
&workspaceand&org_idby reference correctly aligns with thefetch_functionsignature that expects&strparameters. The destructured ownedStringvalues are appropriately borrowed here.crates/context_aware_config/Cargo.toml (1)
29-29: LGTM! Dependency added to support authorization framework.The
superposition_derivesworkspace dependency correctly enables the#[auth_action]attribute macro for the authorization framework introduced in this PR.crates/frontend/src/pages/function/publish_form.rs (1)
35-36: LGTM! Consistent parameter borrowing.The change to pass
&workspaceand&orgby reference is consistent with the broader frontend refactor and correctly borrows the ownedStringvalues extracted on lines 29-30.crates/superposition_types/src/api.rs (1)
21-21: LGTM! Public module added for organisation API.The new
organisationmodule is correctly declared as public, expanding the API surface to support organization-level operations referenced in the PR objectives.crates/frontend/src/pages/webhooks.rs (1)
50-51: LGTM! Consistent workspace terminology and parameter borrowing.The renaming from
current_tenanttoworkspaceimproves terminology consistency, and the parameter borrowing correctly matches thefetch_webhookssignature expecting&strreferences..env.example (2)
38-38: LGTM! Authorization provider configuration added.The
AUTH_Z_PROVIDER=DISABLEDenvironment variable correctly documents the new authorization framework. The defaultDISABLEDvalue is appropriate for development environments.
41-41: LGTM! Encrypted keys example clarified.The updated comment format better illustrates the expected comma-separated format for the
ENCRYPTED_KEYSvariable.crates/experimentation_platform/src/api/experiments/cac_api.rs (1)
226-226: LGTM! Explicit string reference conversion.Using
context_id.as_ref()instead of&context_idis more explicit about the&strconversion for URL concatenation. Both approaches are functionally equivalent for aStringparameter.crates/frontend/src/components/function_form/utils.rs (4)
12-14: LGTM! Centralized host resolution.The import change correctly replaces
get_hostwithuse_host_server, aligning with the frontend refactor that centralizes host resolution logic with SSR support.
36-36: LGTM! Consistent host resolution in create_function.
66-66: LGTM! Consistent host resolution in update_function.
86-86: LGTM! Consistent host resolution in test_function.All three functions (
create_function,update_function,test_function) now consistently useuse_host_server()for host resolution, which properly handles both SSR and non-SSR scenarios.crates/frontend/src/pages/type_templates.rs (1)
43-47: LGTM!The closure parameter rename from
ttoworkspaceand the updatedfetch_typescall with references (&workspace,&org_id) correctly align with the API signature inapi.rswhich expects&strparameters. This follows the consistent workspace-centric refactoring pattern across the frontend.crates/service_utils/src/middlewares/workspace_context.rs (1)
121-133: LGTM!Using
SchemaName::default()instead of hardcoding"public"is cleaner and ensures consistency with the type'sDefaultimplementation. The clone/move pattern forworkspace_idcorrectly handles insertion into both the individual extension and theWorkspaceContextstruct.crates/frontend/src/pages/config_version_list.rs (1)
39-44: LGTM!The closure parameter rename to
workspaceand the updatedfetch_allcall with references correctly align with the API signature inapi.rs. The async/await pattern withunwrap_or_default()is consistent with the refactoring approach used across other frontend pages.crates/frontend/src/components/dimension_form.rs (1)
95-125: LGTM!The refactoring correctly updates the closure to use
workspaceandorg_idparameters, passing them as references todimensions::fetch,fetch_functions, andfetch_types. This aligns with the API signatures that expect&strparameters. The concurrent fetch pattern usingjoin!and error handling withunwrap_or_default()remain consistent.crates/frontend/src/pages/default_config_list.rs (1)
84-89: LGTM!The closure parameter rename and the
fetch_default_configcall update follow the consistent workspace-centric refactoring pattern. Reference passing (&workspace,&org_id) aligns with API expectations.crates/frontend/src/types.rs (1)
17-21: LGTM!The new
SsrSharedHttpRequestHeadersstruct is appropriately minimal for SSR header propagation. TheOption<String>for the cookie field allows for flexible handling when cookies may not be present.crates/frontend/src/components/default_config_form.rs (1)
102-116: LGTM!The refactoring correctly updates the closure to receive
workspaceandorg_id, passing them as references tofetch_functionsandfetch_types. This is consistent with the API signatures and the broader workspace-centric refactoring pattern across the frontend.crates/superposition/src/webhooks/handlers.rs (7)
21-29: LGTM!The
endpoints()registration correctly references the renamed handlers (create_handler,list_handler, etc.). This aligns with the#[auth_action]macro requirement that function names end with_handlerto automatically derive action names.
31-73: LGTM!The
create_handlercorrectly applies#[auth_action]and follows the_handlernaming convention. The authorization layer is cleanly integrated without modifying the core creation logic.
75-108: LGTM!The
update_handlercorrectly integrates authorization while preserving the update logic including workspace settings validation and event validation.
110-120: LGTM!The
get_handlercorrectly applies the#[auth_action]attribute for read authorization.
122-159: LGTM!The
list_handlercorrectly integrates authorization while maintaining the pagination logic.
186-200: LGTM!The
get_by_event_handlercorrectly applies authorization for event-based webhook lookup.
161-184: This pattern is intentional and justified by the database audit trigger.The webhook schema includes a
webhooks_audittrigger that automatically invokesevent_logger()on every INSERT, UPDATE, and DELETE operation. The UPDATE call before deletion ensures thatlast_modified_atandlast_modified_byare captured by the audit trigger before the record is deleted, creating a complete audit trail. The subsequent DELETE operation also triggers the audit mechanism. This is not redundant—it provides traceability of who made the final modification and when.Likely an incorrect or invalid review comment.
crates/superposition_derives/src/lib.rs (1)
394-404: LGTM!The
pascal_casehelper correctly handles splitting on both-and_, capitalizes the first character of each word, and handles empty segments gracefully.crates/frontend/src/pages/function/utils.rs (1)
38-57: LGTM!Good refactor to use borrowed
&strfortenantandorg_idparameters, avoiding unnecessary allocations. The switch touse_host_server()and centralizedrequest()helper aligns with the broader PR refactor for consistent host resolution and SSR support.crates/frontend/src/utils.rs (2)
349-361: LGTM!The rename from
tenanttoworkspaceimproves clarity and aligns with the naming convention used byexecute_value_compute_function. The function call parameters are correctly passed.
71-72: LGTM!Making
get_host()private and cfg-gating it to non-SSR builds is appropriate since it's now only used internally byuse_host_server(). This prevents external code from bypassing the centralized host resolution.crates/frontend/src/components/experiment_ramp_form/utils.rs (1)
6-8: LGTM!Clean refactor to use
use_host_server()for centralized host resolution, consistent with the broader frontend changes in this PR.Also applies to: 24-24
crates/frontend/src/components/type_template_form/utils.rs (1)
10-10: LGTM!Consistent application of
use_host_server()across all type template operations (create_type,update_type,delete_type), aligning with the centralized host resolution pattern.Also applies to: 27-27, 57-57, 75-75
crates/frontend/src/components/experiment_conclude_form/utils.rs (1)
11-14: LGTM!Consistent refactor to use
use_host_server()for host resolution in the experiment conclude flow.Also applies to: 30-30
crates/frontend/src/components/default_config_form/utils.rs (1)
10-12: LGTM!Consistent application of
use_host_server()in bothcreate_default_configandupdate_default_configfunctions, following the established pattern.Also applies to: 35-35, 73-73
crates/service_utils/src/middlewares/resource.rs (1)
1-65: LGTM!Well-structured Actix Web middleware that follows established patterns (similar to
OrgWorkspaceMiddleware). The implementation correctly:
- Uses
Rc<S>for the inner service to enable cloning in async contexts- Delegates readiness via
forward_ready!- Inserts the
Resourceinto request extensions before forwarding to the inner serviceThe design is clean and integrates well with the AuthZ middleware that retrieves the
Resourcefrom extensions (as shown inauth_z.rs:51).crates/frontend/src/components/experiment_form.rs (1)
389-393: LGTM: Ownership refactor aligns with API signature.The change from passing owned values to borrowed references (
&workspace,&org) correctly matches thefetch_experimentsignature that expects&strparameters, as shown in the relevant code snippets.crates/frontend/src/components/experiment_form/utils.rs (3)
19-21: LGTM: Import updated for SSR support.Adding
use_host_serverto imports enables the SSR-compatible host resolution pattern used below.
57-57: LGTM: Host resolution updated for SSR.The change from
get_host()touse_host_server()enables server-side rendering by conditionally routing to localhost when in SSR mode, as shown in the relevant code snippets.
96-96: LGTM: Consistent host resolution for SSR.Matches the pattern established in
create_experimentfor SSR compatibility.crates/frontend/src/pages/compare_overrides.rs (1)
130-136: LGTM: Borrowed references align with API signature.The dimension resource correctly uses borrowed references (
&workspace,&org) matching thedimensions::fetchsignature. Theasync moveclosure withunwrap_or_default()provides appropriate error handling.crates/frontend/src/pages/context_override/utils.rs (3)
9-11: LGTM: Import updated for SSR support.Consistent with the SSR-enabling pattern across the frontend.
40-40: LGTM: Host resolution updated for SSR.The
use_host_server()call enables SSR compatibility by conditionally routing to localhost when appropriate.
74-74: LGTM: Consistent host resolution for SSR.Matches the SSR pattern established in
create_context.crates/frontend/src/pages/config_version.rs (1)
18-22: LGTM: Borrowed references for snapshot fetch.The resource correctly passes borrowed references to
fetch(), consistent with the API signature and the broader refactoring pattern.crates/frontend/src/pages/home.rs (1)
169-176: LGTM: Workspace-based fetch with borrowed references.The dimension resource correctly uses workspace naming and borrowed references, aligning with the broader frontend refactoring. The
async movewithunwrap_or_default()provides safe defaults.crates/experimentation_platform/Cargo.toml (1)
24-24: Workspace dependency is properly defined.
superposition_derivesis correctly configured in the workspace rootCargo.toml(line 79) with path"crates/superposition_derives"and version"0.96.1", matching the workspace package version.crates/service_utils/src/middlewares.rs (1)
1-5: All new middleware modules are properly present and the tenant module has been successfully removed.The module declarations in
middlewares.rsare correctly matched by their corresponding files:auth_z.rs,resource.rs, andworkspace_context.rsall exist in the filesystem. Theauth_zmodule also includes a subdirectory with additional implementation files (authorization.rs,no_auth.rs). The removal oftenant.rsconfirms the migration is complete.crates/frontend/src/pages/dimensions.rs (1)
36-38: LGTM! Workspace-scoped refactor is consistent.The change from owned
current_tenantto borrowed&workspaceimproves memory efficiency and aligns with SSR support patterns across the frontend.crates/frontend/src/pages/experiment_list.rs (1)
82-100: LGTM! Consistent refactor to borrowed workspace references.All concurrent fetch operations correctly use borrowed references (
&workspace,&org_id), which is memory-efficient and consistent with the broader frontend refactor for SSR support.crates/frontend/src/pages/context_override.rs (1)
429-446: LGTM! Workspace references properly propagated.The refactor consistently uses borrowed references across all three fetch operations (context, dimensions, default_config), aligning with the workspace-scoped data fetching pattern.
crates/service_utils/src/middlewares/auth_z/authorization.rs (1)
1-38: LGTM! Authorization trait is well-structured.The
Authorizertrait provides a clean async interface for authorization checks usingLocalBoxFuture. TheSync + Sendbounds are appropriate for middleware usage across async contexts.The commented-out methods (lines 7-11, 22-38) suggest planned expansion—consider tracking these as follow-up tasks if not already documented.
crates/service_utils/src/middlewares/auth_z/no_auth.rs (1)
1-21: LGTM! Permissive authorizer implementation is appropriate.The
NoAuthimplementation correctly provides a pass-through authorizer that always grants access. This is suitable for testing environments or when authorization is disabled.crates/frontend/src/pages/experiment.rs (1)
64-76: LGTM! Consistent workspace refactor.The resource closure correctly uses borrowed workspace references across all three concurrent fetch operations, maintaining consistency with the broader frontend refactor.
crates/frontend/src/pages/experiment_group_listing.rs (1)
251-266: LGTM!The parameter renaming from
tenanttoworkspacealigns with the PR-wide refactoring, and the reference-based passing tofetch_allanddimensions::fetchis correct.crates/superposition_types/src/api/organisation.rs (2)
1-7: LGTM!The conditional imports for Diesel's
AsChangesetand schema are correctly feature-gated underdiesel_derives.
9-28: LGTM!The struct renames (
CreateRequest,UpdateRequest) and theAsChangesetderive withdiesel(table_name = organisations)enable direct Diesel update operations when the feature is enabled.crates/superposition/src/main.rs (5)
38-47: LGTM!The
use_request_headersfunction correctly extracts the Cookie header for SSR context propagation to Leptos routes.
129-130: LGTM!AuthZ handler and manager initialization follows the same pattern as the existing AuthN setup.
152-155: LGTM!Providing the request headers context to Leptos routes enables SSR components to access authentication cookies.
254-260: LGTM!The
/authscope is correctly configured withResourceMiddlewareFactory, workspace middleware, and theauth_z_manageris properly shared viaapp_data.
274-275: Verify middleware execution order.Actix middleware wraps execute in reverse order of registration. With
auth_zregistered beforeauth_n,auth_nwill execute first (outer), thenauth_z(inner). Confirm this is the intended order—typically authentication should precede authorization.crates/context_aware_config/src/api/audit_log/handlers.rs (1)
13-24: LGTM!The handler rename to
list_handlerand#[auth_action]annotation correctly integrate with the authorization framework. The macro will derive the action name "list" from the function name.crates/service_utils/src/service/types.rs (3)
71-89: LGTM!The
Resourceenum comprehensively covers API resources with consistentsnake_caseserialization via both Strum and Serde attributes.
91-97: LGTM!The
workspace_formethod correctly scopes authorization:WorkspaceandAuthresources useorganisation_id, while other resources useschema_name(workspace-level).
148-152: LGTM!The
Defaultimplementation forSchemaNamereturning "superposition" is consistent with theOrganisationIddefault.crates/superposition/src/resolve/handlers.rs (1)
23-41: LGTM!The handler rename and
#[auth_action]integration follow the established pattern. Theclippy::too_many_argumentsallowance is reasonable given the authorization parameter injection by the macro.crates/context_aware_config/src/api/context/handlers.rs (9)
59-71: LGTM!The endpoint registrations correctly reference all renamed handlers with consistent
_handlersuffix naming.
73-136: LGTM!The
create_handleris correctly decorated with#[auth_action]and maintains the existing transactional logic with config versioning.
138-191: LGTM!The
update_handlerwith routes for PUT/PATCH on/overridescorrectly integrates authorization while preserving the override update logic.
193-257: LGTM!The
move_handlercorrectly applies#[auth_action]with the existing move operation logic intact.
259-297: LGTM!Both
get_from_condition_handlerandget_handlerare correctly authorized and follow the naming convention.
299-421: LGTM!The
list_handlerwith complex filtering and pagination logic is correctly integrated with authorization.
423-470: LGTM!The
delete_handlercorrectly applies authorization while maintaining transactional deletion with config versioning.
472-672: LGTM!The
bulk_operations_handlerhandles the complex transactional bulk operations with proper authorization integration.
674-780: LGTM!Both
weight_recompute_handlerandvalidate_handlerare correctly decorated with#[auth_action]following the established pattern.crates/context_aware_config/src/api/variables/handlers.rs (6)
17-24: LGTM!The endpoint registrations correctly reference all renamed handlers with the
_handlersuffix convention.
26-92: LGTM!The
list_handlercorrectly integrates authorization while maintaining the filtering, sorting, and pagination logic.
94-126: LGTM!The
create_handlercorrectly applies authorization and creates variables with proper audit fields.
128-145: LGTM!The
get_handleris correctly authorized with schema-scoped variable retrieval.
147-169: LGTM!The
update_handlercorrectly applies authorization and maintains audit trail withlast_modified_atandlast_modified_by.
171-197: LGTM!The
delete_handlercorrectly updates the audit fields before deletion, which is good practice for maintaining an audit trail of who deleted the variable.crates/service_utils/src/middlewares/auth_z.rs (1)
27-101: Authorization framework implementation looks correct.The
Actiontrait,AuthZ<A>wrapper, andFromRequestimplementation correctly:
- Extract required extensions from the request
- Handle missing extensions with appropriate error types
- Call the authorizer and return forbidden on unauthorized access
crates/context_aware_config/src/api/config/handlers.rs (2)
62-72: Endpoint wiring and handler renames look correct.The endpoints function correctly registers all renamed handlers, and the naming convention with
_handlersuffix aligns with theauth_actionmacro requirements.
433-435: Authorization attributes consistently applied to all handlers.All handlers (
reduce_handler,get_fast_handler,get_handler,resolve_handler,list_version_handler,get_version_handler) correctly have#[auth_action]applied before the route macros, which is the expected order for procedural macro expansion.Also applies to: 476-478, 565-569, 622-627, 672-674, 715-717
crates/context_aware_config/src/api/default_config/handlers.rs (2)
50-57: Handler wiring and renames correctly implemented.The endpoints function properly registers all renamed handlers with consistent naming convention.
59-68: Authorization attributes properly applied to all default config handlers.All CRUD handlers have
#[auth_action]correctly positioned before route macros. The#[allow(clippy::too_many_arguments)]onupdate_handleris appropriately placed.Also applies to: 166-176, 178-191, 359-366, 429-438
crates/experimentation_platform/src/api/experiments/handlers.rs (3)
102-114: Endpoint wiring correctly updated.All handlers are properly registered with the new naming convention. Note that
conclude_handler,discard_handler,pause_handler, andresume_handlerretain their names (they already followed the_handlerconvention or are being kept as-is).
126-128: Authorization consistently applied across experiment handlers.All experiment lifecycle handlers (create, conclude, discard, ramp, update, pause, resume, list, get, get_applicable_variants) have
#[auth_action]properly applied.Also applies to: 416-418, 701-703, 924-929, 988-990, 1151-1153, 1174-1176, 1368-1372, 1727-1729, 1828-1830
251-251: Minor improvement in variant ID construction.Using
variant.id.as_ref()instead of&variant.idfor string concatenation is a minor stylistic change that's equivalent in behavior.crates/context_aware_config/src/api/dimension/handlers.rs (3)
50-57: Endpoint wiring correctly implemented for dimension handlers.All handlers properly registered with consistent
_handlernaming convention.
59-68: Authorization and annotations correctly applied.All dimension CRUD handlers have
#[auth_action]properly positioned. The#[allow(clippy::too_many_arguments)]onupdate_handleris appropriately placed before other attributes.Also applies to: 234-240, 257-270, 437-443, 494-503
7-7: Import adjustment for diesel::delete.Removing
deletefrom the direct imports and usingdiesel::deleteat line 550 is a valid approach that avoids potential naming conflicts within the transaction closure.Also applies to: 550-550
crates/context_aware_config/src/api/functions/handlers.rs (3)
32-41: Endpoint wiring correctly updated for function handlers.All seven handlers properly registered including the function-specific
test_handlerandpublish_handler.
43-50: Authorization correctly applied to all function handlers.All function management handlers have
#[auth_action]properly positioned before route macros.Also applies to: 122-130, 166-172, 180-187, 216-223, 260-267, 301-309
7-7: Consistent import pattern with diesel::delete.Same approach as dimension handlers - using fully qualified
diesel::deletepath to avoid import conflicts.Also applies to: 248-248
crates/context_aware_config/src/api/type_templates/handlers.rs (2)
26-33: Endpoint wiring correctly updated for type template handlers.All handlers properly registered with consistent
_handlernaming.
35-42: Authorization correctly applied to all type template handlers.All CRUD handlers have
#[auth_action]properly positioned.Also applies to: 72-78, 89-99, 156-163, 182-188
crates/superposition/src/organisation/handlers.rs (3)
24-30: Endpoint wiring correctly updated for organisation handlers.All handlers properly registered with consistent
_handlernaming.
32-34: Authorization added to organisation handlers despite PR description.The PR description states "AuthZ for org level APIs not supported in this", yet
#[auth_action]is applied to all organisation handlers. This may be intentional scaffolding for future enforcement, but please verify this is the intended behavior.Also applies to: 69-73, 92-94, 107-109
41-43: Organisation ID generation uses snowflake-style ID.Using
IdInstance::next_id()with an "orgid" prefix creates human-readable, sortable identifiers. This is a good pattern for external-facing IDs.crates/superposition/src/workspace/handlers.rs (6)
62-69: LGTM!The endpoint wiring correctly references the renamed handlers. The service registration pattern is consistent with the handler naming convention adopted across the codebase.
71-86: LGTM!The
get_handlercorrectly applies the#[auth_action]attribute and the implementation properly filters byorg_idensuring proper workspace isolation.
88-145: LGTM!The
create_handlerproperly integrates auth via#[auth_action]and uses theUserextractor for audit trail (created_by,last_modified_by). The transaction-based workspace creation with schema setup is well-structured.
147-193: LGTM on auth integration.The
update_handlercorrectly applies#[auth_action]and properly tracks user modifications. The#[routes]macro for PUT/PATCH is appropriate for update operations.
195-250: LGTM!The
list_handlerproperly applies auth action and filters workspaces byorg_id. Pagination logic is correctly implemented with proper handling of theallflag.
280-302: LGTM!The
migrate_schema_handlercorrectly applies auth action. The transaction ensures atomicity of the schema migration operation.crates/experimentation_platform/src/api/experiment_groups/handlers.rs (8)
55-65: LGTM!The endpoint wiring correctly references all renamed handlers including the new
backfill_handler.
67-145: LGTM!The
create_handlerproperly integrates auth via#[auth_action]and correctly usesworkspace_request.schema_namethroughout. The transaction ensures atomicity of group creation and member assignment.
147-189: LGTM!The
update_handlercorrectly usesworkspace_request.schema_namefor both fetching the experiment group and for the update query. The guard against modifying system-generated groups is appropriate.
191-239: LGTM!The
add_members_handlercorrectly threadsworkspace_request.schema_namethrough all helper function calls.
241-284: LGTM!The
remove_members_handlercorrectly usesworkspace_request.schema_nameconsistently.
349-363: Same pattern aslist_handler- usesSchemaNamedirectly.The
get_handleralso usesSchemaNameinstead ofWorkspaceContext. This appears to be intentional for read-only operations that don't need the full workspace context.
365-396: LGTM!The
delete_handlercorrectly usesSchemaNameand properly validates that the group has no members before deletion. The update-then-delete pattern within a transaction ensures proper audit trail.
286-347: The difference in parameter usage is intentional. Whilecreate_handler,update_handler,add_members_handler, andremove_members_handleruseWorkspaceContext, thelist_handlercorrectly uses onlySchemaName. This pattern is consistent across the codebase: read-only list handlers uniformly useSchemaNamefor query scoping, while write operations use the fullerWorkspaceContext. The middleware properly supports both patterns.crates/frontend/src/api.rs (6)
35-37: LGTM!The import of
construct_request_headers,parse_json_response,request, anduse_host_servercentralizes the request handling utilities, reducing code duplication across API functions.
40-63: LGTM!The
fetch_default_configfunction correctly uses the new request pattern with borrowed parameters and proper header construction.
763-803: LGTM!The
execute_value_compute_functionfunction correctly handles the function execution request with proper parameter passing and response parsing.
980-1154: LGTM!The
experiment_groupsmodule is consistently refactored with the new request pattern. All CRUD operations properly use borrowed tenant/org parameters and the centralized request helper.
1156-1186: LGTM!The
audit_logmodule correctly implements the fetch function with the new request pattern.
416-424: Empty headers for organisation listing are intentional and correctly aligned with the backend.The
/organisationsendpoint is configured withOrgWorkspaceMiddlewareFactory::new(false, false), which disables both org_id and workspace_id header validation. The backend handler only requires authentication (via#[auth_action]) but not tenant or organisation context, making this a pre-auth operation that lists all organisations without being scoped to a specific tenant/org. The emptyHeaderMap::new()in the frontend is the correct implementation.
ayushjain17
force-pushed
the
auth_z
branch
2 times, most recently
from
c88c74b to
01dfaee
Compare
| org_id: String, | ||
| ) -> Result<PaginatedResponse<DefaultConfig>, ServerFnError> { | ||
| let client = reqwest::Client::new(); | ||
| tenant: &str, |
There was a problem hiding this comment.
let us change to workspace - we seem to be doing it in other places.
There was a problem hiding this comment.
will raise a separate PR for this cleanup
AuthZ for org level APIs not supported in this
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.