feat: Service accounts#306
Conversation
|
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:
WalkthroughThis PR implements a major architectural shift from Keycloak-centric to a pluggable OIDC-based authentication provider system combined with Casbin-backed authorization. Changes include refactored authentication middleware, new provider abstractions, new authorization endpoints and permission enforcement, service account management, dashboard permission-based UI gating, database schema extensions for authorization, and configuration/documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard as Dashboard/Client
participant AuthEndpoint as Auth Endpoints
participant AuthN as AuthN Provider
participant AuthZ as AuthZ Provider
participant Backend as Backend Services
rect rgba(100, 150, 255, 0.5)
Note over User,Backend: OIDC Login Flow
User->>Dashboard: Click "Sign in"
Dashboard->>AuthEndpoint: GET /users/oauth/url?idp=google
AuthEndpoint->>AuthN: get_oauth_url(state, offline=false, idp="google")
AuthN->>AuthN: Discover OIDC metadata
AuthN-->>AuthEndpoint: OAuth authorization URL
AuthEndpoint-->>Dashboard: URL
Dashboard->>AuthN: Redirect user to OIDC provider
AuthN->>AuthN: User authenticates at provider
AuthN-->>Dashboard: Redirect with auth code
Dashboard->>AuthEndpoint: POST /users/oauth/login with code
AuthEndpoint->>AuthN: exchange_code_for_token(code, state)
AuthN->>AuthN: Exchange code for tokens
AuthN-->>AuthEndpoint: Access/refresh tokens
AuthEndpoint->>AuthN: verify_access_token(access_token)
AuthN->>AuthN: Verify JWT signature, validate claims
AuthN-->>AuthEndpoint: AuthnTokenClaims
AuthEndpoint->>AuthZ: subject_from_claims(claims)
AuthZ-->>AuthEndpoint: Normalized subject
AuthEndpoint->>AuthZ: get_user_access_summary(subject)
AuthZ-->>AuthEndpoint: User organizations/applications/roles
AuthEndpoint-->>Dashboard: Login response with token
end
rect rgba(100, 255, 150, 0.5)
Note over User,Backend: Permission Check Flow
User->>Dashboard: View release page
Dashboard->>Dashboard: usePagePermissions({read_release, update_release})
Dashboard->>AuthEndpoint: POST /authz/me/enforce-batch
AuthEndpoint->>AuthZ: enforce_permissions_batch(subject, checks)
AuthZ->>AuthZ: Evaluate Casbin policies
AuthZ-->>AuthEndpoint: [allowed_release_read, allowed_release_update]
AuthEndpoint-->>Dashboard: Permission results
Dashboard->>Dashboard: Render UI based on permissions
end
rect rgba(255, 200, 100, 0.5)
Note over User,Backend: Authenticated Request Flow
User->>Dashboard: Create release
Dashboard->>Backend: POST /release (with Bearer token)
Backend->>AuthEndpoint: Verify token (middleware)
AuthEndpoint->>AuthN: verify_access_token(token)
AuthN-->>AuthEndpoint: AuthnTokenClaims
AuthEndpoint->>AuthZ: access_for_request(claims, org, app)
AuthZ-->>AuthEndpoint: Access context
Backend->>Backend: Handler with #[authz(...)] macro
Backend->>AuthZ: enforce_permission(subject, resource, action)
AuthZ->>AuthZ: Check Casbin policy
AuthZ-->>Backend: Permission decision
Backend->>Backend: Execute business logic
Backend-->>Dashboard: Success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #[authz( | ||
| resource = "service_account", | ||
| action = "create", | ||
| org_roles = ["owner", "admin"], | ||
| app_roles = [] | ||
| )] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
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 (2)
airborne_dashboard/app/dashboard/[orgId]/[appId]/releases/page.tsx (1)
56-74:⚠️ Potential issue | 🟡 MinorMissing
notFound()guard forread_releasespermission.Other pages in this PR (e.g.,
[appId]/page.tsx,packages/create/page.tsx) callnotFound()when the user lacks the required read permission. This page fetches releases data but doesn't block rendering whenread_releasesis denied—the user sees the full page structure while the API returns 403.For consistency, consider adding the same guard pattern:
Suggested fix
const { token, org, app } = useAppContext(); const permissions = usePagePermissions(PAGE_AUTHZ); + + if (permissions.isReady && !permissions.can("read_releases")) { + notFound(); + }You'll also need to import
notFoundfromnext/navigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/releases/page.tsx around lines 56 - 74, This page does not guard against missing read_releases permission, so users without that permission still see the UI while the API returns 403; update the component to call notFound() (imported from next/navigation) when usePagePermissions(PAGE_AUTHZ).can("read_releases") is false—e.g., after const permissions = usePagePermissions(PAGE_AUTHZ) check permissions.can("read_releases") and invoke notFound() to short-circuit rendering before useSWR/apiFetch (ensure this runs before the SWR call that uses token/org/appId to avoid fetching forbidden data).airborne_dashboard/app/dashboard/[orgId]/[appId]/releases/[releaseId]/edit/page.tsx (1)
19-45:⚠️ Potential issue | 🟠 MajorHandle permission lookup errors before
notFound().As written, any
usePagePermissionsfailure makeshasAccessfalse and turns the edit page into a 404. That hides real authz outages and makes troubleshooting hard. Checkpermissions.errorseparately, then callnotFound()only on an actual deny.Suggested guard
if (!permissions.isReady) { return ( <div className="p-6 flex items-center justify-center min-h-screen"> <p className="text-muted-foreground">Checking access...</p> </div> ); } + if (permissions.error) { + return ( + <div className="p-6 flex items-center justify-center min-h-screen"> + <p className="text-destructive">Failed to verify access.</p> + </div> + ); + } + const hasAccess = permissions.can("read_release") && permissions.can("update_release");Based on learnings from
airborne_dashboard/hooks/use-page-permissions.ts:1-70,isReadybecomestruewhen the permission request errors, andcan()falls back tofalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/releases/[releaseId]/edit/page.tsx around lines 19 - 45, The current logic treats any permissions lookup failure as a deny and calls notFound(); instead, check permissions.error first and surface it (e.g., throw or render an error) before treating the result as a deny: after calling usePagePermissions use permissions.isReady and if permissions.error is set return/render an error (or rethrow) so authz outages are visible, then compute hasAccess using permissions.can(...) only when there is no permissions.error and permissions.isReady, and call notFound() only when the permission checks explicitly deny access.
🟡 Minor comments (6)
airborne_server/Project.md-323-328 (1)
323-328:⚠️ Potential issue | 🟡 MinorMove OIDC configuration variables to backend documentation section.
The environment variables
OIDC_ISSUER_URLandOIDC_CLIENT_IDdocumented in lines 326-327 are not used by the Next.js frontend (airborne_dashboard). These are backend configuration variables required by airborne_server for OIDC authentication flows (provider metadata discovery, token verification, OAuth flows). Since the frontend retrieves OIDC configuration from the backend API endpoint (/dashboard/configuration), these variables belong in backend environment documentation, not the frontend "Environment Configuration" section. Either move them to the backend documentation or remove them from this section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/Project.md` around lines 323 - 328, The OIDC vars OIDC_ISSUER_URL and OIDC_CLIENT_ID belong to the backend, not the frontend, so remove them from the frontend "Environment Configuration" block in Project.md and either relocate them into the backend/server documentation for airborne_server or delete them if already documented there; reference airborne_dashboard (frontend) which uses /dashboard/configuration to fetch OIDC settings and airborne_server which requires OIDC_ISSUER_URL and OIDC_CLIENT_ID for provider discovery/token verification, and update the Project.md section accordingly to avoid duplicating backend-only env vars.README.md-325-325 (1)
325-325:⚠️ Potential issue | 🟡 MinorCapitalize
GitHubin the IdP example.Line 325 uses
github, but the platform name isGitHub.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 325, Update the example value for the `OIDC_ENABLED_IDPS` table entry so the provider name is capitalized correctly: change the comma-separated IdP hint from `google,github` to `google,GitHub` in the `OIDC_ENABLED_IDPS` row of README.md.airborne_server/src/organisation/application/user/types.rs-4-8 (1)
4-8:⚠️ Potential issue | 🟡 MinorBreaking change:
accessfield type changed from enum to String.The
UserRequest.accessfield changed from a typedAccessLvlenum to a plainString. This is a breaking change for API consumers who may have been relying on compile-time validation of access levels. Ensure this is documented in migration notes or API changelog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/organisation/application/user/types.rs` around lines 4 - 8, UserRequest's access field was changed from the typed AccessLvl enum to a plain String (UserRequest.access), which is a breaking API change; restore type-safety by reverting access back to the AccessLvl enum (use AccessLvl for UserRequest.access and derive/implement serde Deserialize for AccessLvl) or, if String is required, add runtime validation/deserialization that maps/validates the String into AccessLvl (e.g., TryFrom or custom Deserialize for AccessLvl) and update the API changelog/migration notes to document the breaking change and accepted values.API_DOCUMENTATION.md-179-179 (1)
179-179:⚠️ Potential issue | 🟡 MinorMinor: Capitalize "GitHub".
The official name uses a capital "H".
📝 Proposed fix
-- `idp` (optional, string): OIDC provider hint (for Keycloak this maps to `kc_idp_hint`, for example `google` or `github`). +- `idp` (optional, string): OIDC provider hint (for Keycloak this maps to `kc_idp_hint`, for example `google` or `GitHub`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@API_DOCUMENTATION.md` at line 179, Update the API docs string for the `idp` OIDC provider hint to use the correct capitalization for GitHub; change the example text from "github" to "GitHub" (the `idp` field and the inline example showing `google` or `github` should be updated so `github` is capitalized as `GitHub`).airborne_dashboard/components/user-management.tsx-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorVerify role key validation matches backend.
The
isValidRoleKeyregex/^[a-z_]+$/allows only lowercase letters and underscores. Verify this matches the backend validation incasbin.rs:// From casbin.rs line 2229-2233 fn is_valid_custom_role_name(role: &str) -> bool { !role.is_empty() && role.len() <= 64 && role.chars().all(|ch| ch.is_ascii_lowercase() || ch == '_') }The frontend validation is missing the 64-character length limit. Consider adding:
-const isValidRoleKey = (value: string): boolean => /^[a-z_]+$/.test(value); +const isValidRoleKey = (value: string): boolean => /^[a-z_]+$/.test(value) && value.length <= 64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/components/user-management.tsx` at line 107, Update the frontend role-key validator to match backend rules: modify isValidRoleKey to enforce non-empty, maximum length 64, and allow only ASCII lowercase letters or underscores (same as casbin.rs is_valid_custom_role_name); locate the isValidRoleKey function and add a length check (value.length > 0 && value.length <= 64) in addition to the existing /^[a-z_]+$/ test so the validation behavior matches the backend precisely.airborne_server/src/user.rs-255-262 (1)
255-262:⚠️ Potential issue | 🟡 MinorInconsistent error types for token verification failure.
oauth_login(line 261) maps token verification failure toABError::BadRequest("Invalid token"), whileoauth_signup(line 298) maps the same error toABError::Unauthorized("Invalid token"). For consistency and correct HTTP semantics (401 = authentication failure), both should useUnauthorized:Proposed fix
// In oauth_login (lines 259-262) .map_err(|e| { info!("[OAUTH_LOGIN] Token decode failed: {:?}", e); - ABError::BadRequest("Invalid token".to_string()) + ABError::Unauthorized("Invalid token".to_string()) })?;Also applies to: 294-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/user.rs` around lines 255 - 262, The token verification error mapping is inconsistent: in oauth_login the verify_access_token call (state.authn_provider.verify_access_token) maps failures to ABError::BadRequest while oauth_signup maps to ABError::Unauthorized; change the mapping closure used after verify_access_token in both oauth_login and oauth_signup to return ABError::Unauthorized("Invalid token".to_string()) so both functions consistently return a 401 for authentication failures.
🧹 Nitpick comments (18)
airborne_dashboard/app/dashboard/[orgId]/[appId]/page.tsx (1)
28-37: Consider gating the API fetch on permission readiness.The SWR fetch starts before
permissions.isReady, which may trigger unnecessary 403 responses. The existingErrorName.Forbiddenfallback at lines 32-34 handles this gracefully, but you could reduce redundant network calls by addingpermissions.isReady && permissions.can("read_releases")to the SWR key condition.Current approach works correctly due to the dual fallback, so this is optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/page.tsx around lines 28 - 37, The SWR request is initiated before permissions are ready, causing unnecessary 403s; update the useSWR key condition so the fetch only runs when permissions.isReady and permissions.can("read_releases") are true (in addition to token, org, and appId). Specifically, modify the useSWR call in page.tsx (the one invoking useSWR and apiFetch) to include permissions.isReady && permissions.can("read_releases") in the conditional key, while keeping the existing error handling that checks error.name === ErrorName.Forbidden and the notFound() fallbacks unchanged.airborne_dashboard/app/dashboard/[orgId]/[appId]/remote-configs/page.tsx (1)
57-61: MissingfetchSchemain useEffect dependencies.The
fetchSchemafunction referencescanReadSchema,token,org, andappfrom the closure. While the explicit dependencies cover the trigger conditions,fetchSchemaitself should either be in the dependency array or wrapped inuseCallbackto avoid stale closure issues if its captured variables change mid-render.Suggested fix using useCallback
+ import { useCallback } from "react"; // ... - const fetchSchema = async () => { + const fetchSchema = useCallback(async () => { if (!canReadSchema) return; // ... rest of function - }; + }, [canReadSchema, token, org, app]); useEffect(() => { if (token && orgId && appId && permissions.isReady && canReadSchema) { fetchSchema(); } - }, [token, orgId, appId, permissions.isReady, canReadSchema]); + }, [token, orgId, appId, permissions.isReady, canReadSchema, fetchSchema]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/remote-configs/page.tsx around lines 57 - 61, The useEffect depends on values used inside fetchSchema but omits fetchSchema itself, risking stale closures; update the code so fetchSchema is stable (wrap the fetchSchema function in useCallback with token, orgId, appId, canReadSchema, and any other captured values as its dependencies) and then include fetchSchema in the useEffect dependency array (or alternatively include fetchSchema directly in the deps if you keep it as a plain function). Ensure the unique symbols mentioned (fetchSchema and the useEffect that currently lists [token, orgId, appId, permissions.isReady, canReadSchema]) are updated so the effect always runs when the actual fetch logic or its captured values change.airborne_dashboard/app/dashboard/[orgId]/[appId]/files/page.tsx (1)
159-160: Potential duplicate error toast.Per project conventions,
apiFetchalready displays an error toast on API failures by default. The manualtoastErrorcall here may result in duplicate user-visible error notifications. Consider removing the manual toast or disablingshowErrorToaston theapiFetchcall if custom error messaging is needed.♻️ Remove duplicate toast
} catch (error) { - toastError("Failed to update tag", error instanceof Error ? error.message : "Unknown error"); + // apiFetch already shows error toast; only handle non-UI cleanup here } finally {Based on learnings: "When using
apiFetchfromairborne_dashboard/lib/api.ts, note that it already shows an error toast on API failures by default... do not manually trigger toast/notification UI for API errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/files/page.tsx around lines 159 - 160, The catch block for the tag update is producing a manual toast via toastError which duplicates apiFetch's built-in error toast; remove the manual toastError call in the catch (or alternatively call apiFetch with showErrorToast: false if you want a custom message) and let apiFetch handle error toasts, updating the catch in the updateTag/handleUpdateTag flow accordingly (referencing the catch block around the update tag operation and the apiFetch/showErrorToast option).airborne_dashboard/app/dashboard/[orgId]/[appId]/dimensions/page.tsx (2)
127-133: Potential duplicate error toast in handleCreate.Similar to the files page,
apiFetchalready displays error toasts by default. The manual toast in the catch block may result in duplicate notifications.♻️ Remove duplicate toast
} catch (error) { console.error("Failed to create dimension:", error); - toast({ - title: "Error", - description: "Failed to create dimension. Please try again.", - variant: "destructive", - }); + // apiFetch already shows error toast } finally {Based on learnings: "When using
apiFetch... do not manually trigger toast/notification UI for API errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/dimensions/page.tsx around lines 127 - 133, In handleCreate, remove the manual toast call in the catch block to avoid duplicate notifications since apiFetch already shows error toasts; keep or adjust the console.error/logging of the caught error (e.g., the existing console.error("Failed to create dimension:", error)) but delete the toast({...}) invocation so only apiFetch controls user-facing error messages.
151-157: Same duplicate toast pattern in movePriority.The manual error toast here also duplicates
apiFetch's built-in error notification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/dimensions/page.tsx around lines 151 - 157, The catch block in movePriority duplicates apiFetch's built-in error toast; in the movePriority function remove the manual toast and console.error so errors are allowed to be handled by apiFetch's notification (or rethrow if upstream needs it), keeping only any necessary cleanup/return logic—locate the catch inside movePriority that currently logs "Failed to update priority:" and remove the toast() and console.error calls there.airborne_server/scripts/init-keycloak.sh (1)
1-1: Shebang declares POSIXshbut script useslocalkeyword.The script uses
#!/bin/shbut employs thelocalkeyword (lines 106, 111-114, 146-147, 185, 242) which is undefined in POSIX sh. While most systems link/bin/shto a shell that supportslocal, this isn't guaranteed and could fail on strict POSIX environments.Consider changing the shebang to
#!/bin/bashor#!/usr/bin/env bashif bash features are acceptable, or replacelocalwith function-scoped variables using subshells.♻️ Proposed fix
-#!/bin/sh +#!/usr/bin/env bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/scripts/init-keycloak.sh` at line 1, The script declares a POSIX sh shebang but uses the Bash-only keyword "local" in several functions, which can break on strict /bin/sh implementations; update the shebang to a Bash-compatible interpreter (e.g., change "#!/bin/sh" to "#!/usr/bin/env bash") or remove all "local" usages by rewriting those functions to use POSIX-compatible patterns (e.g., use temporary subshells or uniquely named variables) — search for the "local" occurrences in init-keycloak.sh (the functions where "local" is used at the noted occurrences) and apply the chosen fix consistently across those functions.airborne_dashboard/hooks/use-page-permissions.ts (1)
53-56: Consider documenting the permissive default when no permissions are defined.When
aliases.length === 0,can()returnstrue. This is a permissive default that grants access when no permissions are configured. While this may be intentional for backward compatibility, it could be surprising behavior. Consider adding a brief comment explaining this design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/hooks/use-page-permissions.ts` around lines 53 - 56, The can function currently returns true when aliases.length === 0, a permissive default that grants access if no permissions are configured; add a brief inline comment above the can function explaining this intentional fallback (e.g., "permissive default for backward compatibility: allow access when no permission aliases are defined") so future readers understand the design choice and potential implications; reference the can function and the aliases/decisions variables when adding the comment.airborne_authz_macros/src/lib.rs (1)
99-128: Silent fallback to "UNKNOWN" method may mask misconfiguration.When no HTTP method attribute (
get,post, etc.) is found, the function returns"UNKNOWN"as the method (line 127). This silently allows the macro to compile but may cause issues at runtime when the endpoint binding is registered with an invalid method.Consider emitting a compile-time warning or error when no HTTP method attribute is detected.
♻️ Suggested improvement
fn parse_method_and_path(attrs: &[Attribute]) -> (String, String) { for attr in attrs { // ... existing parsing logic ... } - ("UNKNOWN".to_string(), String::new()) + // This will be caught at runtime, but ideally we'd warn at compile time + // Consider: eprintln!("cargo:warning=No HTTP method attribute found on #[authz] decorated function"); + ("UNKNOWN".to_string(), String::new()) }airborne_server/src/organisation.rs (1)
183-192: Consider fetching actual user access from authz provider instead of hardcoding.The response returns a hardcoded access array
["owner", "admin", "write", "read"]. While this happens to be correct (since the creator receives theownerrole viacreate_organisationin the authz provider, andorg_role_display_expansion("owner")expands to all four roles), this creates an implicit contract that isn't validated. If the authz logic changes to grant a different initial role, the hardcoded response would become incorrect and inconsistent withlist_organisations, which fetches actual access viaorg_role_display_expansion().For consistency and maintainability, consider computing the access array from the actual role granted by the authz provider rather than hardcoding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/organisation.rs` around lines 183 - 192, Instead of returning the hardcoded access array in the Organisation JSON, call the authz provider to determine the creator's actual organisation role and derive the displayed access via the same helper used in list_organisations (e.g., org_role_display_expansion). Replace the literal vec!["owner","admin","write","read"] in the Json(Organisation { ... }) return with a computed access list by fetching the creator's role from the authz logic used by create_organisation (or the authz client/function that returns the granted role) and then pass that role into org_role_display_expansion (or the equivalent helper) so the returned access is consistent with the real authz state.airborne_dashboard/app/dashboard/[orgId]/users/page.tsx (1)
499-502: Hardcoded service account email domain filter.The filter uses a hardcoded domain
@service-account.airborne.juspay.in. Consider extracting this to a constant or deriving it from configuration for maintainability.♻️ Suggested improvement
+const SERVICE_ACCOUNT_EMAIL_SUFFIX = "@service-account.airborne.juspay.in"; + // Filter out service account users from the regular users list const regularUsers = (data?.users || []).filter( - (user) => !user.username.endsWith("@service-account.airborne.juspay.in") + (user) => !user.username.endsWith(SERVICE_ACCOUNT_EMAIL_SUFFIX) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_dashboard/app/dashboard/`[orgId]/users/page.tsx around lines 499 - 502, The filter that builds regularUsers currently hardcodes the service account domain in the expression user.username.endsWith("@service-account.airborne.juspay.in"); extract that literal into a named constant (e.g., SERVICE_ACCOUNT_DOMAIN) or pull it from configuration/env (e.g., getConfig().serviceAccountDomain) and replace the direct string usage so the filter becomes user.username.endsWith(SERVICE_ACCOUNT_DOMAIN), ensuring the constant/config is exported/loaded where page.tsx can access it and add a brief comment explaining why it’s configurable.airborne_server/src/service_account.rs (2)
314-322: Rotation silently ignores deletion failure.If
delete_userfails at Line 314-317 butcreate_service_account_usersucceeds, the old OIDC user might still exist with the previous credentials. Consider logging the deletion result.♻️ Proposed logging
// Delete and recreate the OIDC user with new credentials let password = generate_random_password().await?; - let _ = state + if let Err(e) = state .authn_provider .delete_user(state.get_ref(), &entry.name) - .await; + .await + { + log::warn!("[ROTATE SERVICE ACCOUNT] Failed to delete old OIDC user: {}", e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/service_account.rs` around lines 314 - 322, The call to state.authn_provider.delete_user(...) is currently ignored, which can leave the old OIDC user in place if deletion fails; update the rotation flow in the function containing the delete_user and create_service_account_user calls to capture the Result/Err from state.authn_provider.delete_user(state.get_ref(), &entry.name).await, and log a warning/error (including the error details and entry.name) when deletion fails before proceeding to call state.authn_provider.create_service_account_user(...); keep the create_service_account_user call behavior but ensure the deletion failure is not silently dropped—use the existing logging facility (process logger or state logger) to emit contextual logs.
245-255: Best-effort cleanup may leave orphaned resources.The
let _ =pattern silently ignores failures when removing AuthZ memberships or deleting the OIDC user. If the OIDC user deletion fails, the user remains in Keycloak but the DB entry is deleted, potentially leaving orphaned credentials.Consider logging these failures at minimum for audit/debugging purposes.
♻️ Proposed logging for cleanup failures
// Remove from AuthZ memberships - let _ = state + if let Err(e) = state .authz_provider .remove_organisation_user(state.get_ref(), &auth.sub, &organisation, &entry.email) - .await; + .await + { + log::warn!("[DELETE SERVICE ACCOUNT] Failed to remove AuthZ membership: {}", e); + } // Delete user from OIDC provider (best effort — invalidates all tokens) - let _ = state + if let Err(e) = state .authn_provider .delete_user(state.get_ref(), &entry.name) - .await; + .await + { + log::warn!("[DELETE SERVICE ACCOUNT] Failed to delete OIDC user: {}", e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/service_account.rs` around lines 245 - 255, The cleanup currently swallows errors with `let _ =` for state.authz_provider.remove_organisation_user and state.authn_provider.delete_user; change these to capture the Result and log failures (e.g., call the service/state logger with an error level) including contextual info (auth.sub, organisation, entry.email or entry.name) so you can audit when AuthZ removal or OIDC deletion fails — update the calls around remove_organisation_user and delete_user to handle .await -> match/if let Err(err) and log the error with the identifying fields.airborne_server/src/provider/authn/keycloak.rs (1)
138-141: Consider reusingreqwest::Clientinstances.Creating a new
reqwest::Clienton each call involves connection pool setup overhead. While not critical, consider passing a shared client viaAppStateor reusing a single instance within multiple calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/provider/authn/keycloak.rs` around lines 138 - 141, The code creates new reqwest::Client instances for get_token and KeycloakAdmin::new which wastes connection pools; update the code and AppState to hold a shared reqwest::Client (e.g., add an http_client field to the AppState struct) and use that shared client when calling get_token(...) and KeycloakAdmin::new(...) (replace Client::new() with &state.http_client or state.http_client.clone() as appropriate), ensuring get_token and KeycloakAdmin constructors accept a Client reference/clone if needed.airborne_server/src/provider/authz/casbin.rs (1)
556-567: Full-table delete inrefresh_membership_cache_from_casbinis safe only during bootstrap.The
diesel::delete(authz_memberships::table).execute(&mut conn)?on line 559 removes all membership rows before re-inserting from Casbin policies. This is acceptable duringbootstrap()initialization, but if this method were called at runtime (e.g., during policy reload), it would create a brief window where membership queries return no results.Consider adding a doc comment to clarify this method is intended for bootstrap-only use, or wrap the delete+insert in a transaction with explicit isolation level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/provider/authz/casbin.rs` around lines 556 - 567, The current implementation in refresh_membership_cache_from_casbin deletes all rows from authz_memberships::table then re-inserts, which is only safe during bootstrap; either document this intent or make the operation atomic: add a doc comment on refresh_membership_cache_from_casbin stating it is bootstrap-only (referencing bootstrap()), or wrap the delete+insert block in an explicit DB transaction (use the connection's transaction API and set an appropriate isolation level) so delete and subsequent inserts occur atomically and do not expose an empty-membership window at runtime.airborne_server/src/organisation/user.rs (1)
65-65: Unusedauth_responseparameter in handlers.The
auth_response: ReqData<AuthResponse>parameter is injected by the#[authz(...)]macro for permission enforcement, but each handler then callsget_org_context(&req)which re-extractsAuthResponsefrom request extensions. Consider using theauth_responsedirectly to avoid the redundant extraction:-async fn get_org_context(req: &HttpRequest) -> airborne_types::Result<(String, AuthResponse)> { - let auth = req - .extensions() - .get::<AuthResponse>() - .cloned() - .ok_or_else(|| ABError::Unauthorized("Missing auth context".to_string()))?; - - let org_name = require_scope_name(auth.organisation.clone(), "organisation")?; - Ok((org_name, auth)) -} +fn get_org_name(auth: &AuthResponse) -> airborne_types::Result<String> { + require_scope_name(auth.organisation.clone(), "organisation") +}Then in handlers:
- let (organisation, auth) = get_org_context(&req).await?; + let auth = auth_response.into_inner(); + let organisation = get_org_name(&auth)?;Also applies to: 112-112, 151-151, 179-179, 210-210, 237-237, 275-275, 306-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/organisation/user.rs` at line 65, The handlers currently accept the injected parameter auth_response: ReqData<AuthResponse> from the #[authz(...)] macro but ignore it and call get_org_context(&req) which re-extracts AuthResponse from request extensions; update each handler to use the provided auth_response instead of re-extracting (either pass auth_response.into_inner()/auth_response.0 to get_org_context after changing its signature to accept an AuthResponse, or remove the redundant get_org_context call and derive org context directly from the auth_response), and update all affected handler functions that declare auth_response: ReqData<AuthResponse> (the locations noted in the comment) so they consume the injected value rather than re-reading from req.airborne_server/src/provider/authz/permission.rs (1)
48-50: Minor format inconsistency in error message.The
scoped_permissionfunction formats permissions as"{scope}:{resource}.{action}", but the error message on line 103-104 uses"{resource}.{action}"without the scope prefix. This inconsistency is benign for user-facing errors but could cause confusion during debugging.Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/provider/authz/permission.rs` around lines 48 - 50, The error message currently prints permissions as "{resource}.{action}" which mismatches scoped_permission(scope, resource, action) that formats "{scope}:{resource}.{action}"; update the error(s) that construct or log the permission string (where the message omits the scope) to use the same format—either call scoped_permission(scope, resource, action) or replicate "{scope}:{resource}.{action}"—so logs and errors consistently include the scope prefix (check places that build the permission string in the permission error/path handling).airborne_server/src/middleware/auth.rs (1)
70-76: Consider documenting the purpose ofauthn_*fields.The
#[allow(dead_code)]attributes onauthn_sub,authn_iss, andauthn_emailsuggest these fields are reserved for future use. A brief doc comment explaining their intended purpose (e.g., audit logging, debugging, or future multi-provider scenarios) would help maintainers understand why they're preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/middleware/auth.rs` around lines 70 - 76, The three fields authn_sub, authn_iss, and authn_email are marked with #[allow(dead_code)] but lack documentation; add short doc-comments above each field in the struct (or a doc-block for the group) explaining their intended purpose (e.g., to hold the original authentication subject, issuer, and email for audit/logging, debugging, or future multi-provider support) so maintainers know why they are retained and when they should be used; reference the fields authn_sub, authn_iss, and authn_email when adding these comments.airborne_server/src/utils/keycloak.rs (1)
81-108: Consider documenting therealm_users_getparameter positions.The call to
realm_users_getuses many positionalNoneparameters (lines 89-101), which can be fragile if the Keycloak crate's API changes. A brief inline comment indicating which parameter isenabled(line 92) and which issearch(line 102) would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@airborne_server/src/utils/keycloak.rs` around lines 81 - 108, In search_users, annotate the positional None arguments passed to KeycloakAdmin::realm_users_get so future readers know which slots are which (e.g., mark the argument currently set to Some(true) as the "enabled" parameter and the final Some(search_term.to_string()) as the "search" parameter); locate this call inside the async fn search_users and add short inline comments after the relevant arguments to label "enabled" and "search" (or replace with named builder-style params if the crate supports it) to improve maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5dacfc6-d38a-4c47-a623-e543cd546af6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (100)
API_DOCUMENTATION.mdCargo.tomlMakefileREADME.mdairborne_authz_macros/Cargo.tomlairborne_authz_macros/src/lib.rsairborne_dashboard/app/dashboard/[orgId]/[appId]/cohorts/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/dimensions/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/files/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/packages/create/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/packages/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/releases/[releaseId]/clone/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/releases/[releaseId]/edit/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/releases/[releaseId]/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/releases/create/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/releases/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/remote-configs/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/token/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/users/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/views/page.tsxairborne_dashboard/app/dashboard/[orgId]/page.tsxairborne_dashboard/app/dashboard/[orgId]/users/page.tsxairborne_dashboard/app/dashboard/onboarding/page.tsxairborne_dashboard/app/dashboard/page.tsxairborne_dashboard/app/login/page.tsxairborne_dashboard/app/oauth/callback/page.tsxairborne_dashboard/app/page.tsxairborne_dashboard/app/register/page.tsxairborne_dashboard/components/release/steps/PackageSelectionStep.tsxairborne_dashboard/components/shared-layout.tsxairborne_dashboard/components/user-management.tsxairborne_dashboard/hooks/use-page-permissions.tsairborne_dashboard/lib/authz.tsairborne_dashboard/lib/oidc-providers.tsxairborne_dashboard/lib/page-permissions.tsairborne_dashboard/next.config.mjsairborne_dashboard/providers/app-context.tsxairborne_server/.env.exampleairborne_server/Cargo.tomlairborne_server/Project.mdairborne_server/README.mdairborne_server/migrations/20260401120000_add_casbin_rule_and_workspace_uniqueness/down.sqlairborne_server/migrations/20260401120000_add_casbin_rule_and_workspace_uniqueness/up.sqlairborne_server/migrations/20260409193000_add_authz_bindings_and_memberships/down.sqlairborne_server/migrations/20260409193000_add_authz_bindings_and_memberships/up.sqlairborne_server/migrations/20260410120000_add_service_accounts/down.sqlairborne_server/migrations/20260410120000_add_service_accounts/up.sqlairborne_server/scripts/encrypt-envs.shairborne_server/scripts/init-keycloak.shairborne_server/scripts/init-localstack.shairborne_server/src/authz.rsairborne_server/src/authz/types.rsairborne_server/src/config.rsairborne_server/src/dashboard/configuration.rsairborne_server/src/file.rsairborne_server/src/file/groups.rsairborne_server/src/main.rsairborne_server/src/middleware/auth.rsairborne_server/src/organisation.rsairborne_server/src/organisation/application.rsairborne_server/src/organisation/application/config.rsairborne_server/src/organisation/application/dimension.rsairborne_server/src/organisation/application/dimension/cohort.rsairborne_server/src/organisation/application/properties.rsairborne_server/src/organisation/application/types.rsairborne_server/src/organisation/application/user.rsairborne_server/src/organisation/application/user/transaction.rsairborne_server/src/organisation/application/user/types.rsairborne_server/src/organisation/application/user/utils.rsairborne_server/src/organisation/transaction.rsairborne_server/src/organisation/types.rsairborne_server/src/organisation/user.rsairborne_server/src/organisation/user/transaction.rsairborne_server/src/organisation/user/types.rsairborne_server/src/organisation/user/utils.rsairborne_server/src/package.rsairborne_server/src/provider.rsairborne_server/src/provider/authn.rsairborne_server/src/provider/authn/auth0.rsairborne_server/src/provider/authn/keycloak.rsairborne_server/src/provider/authn/oidc.rsairborne_server/src/provider/authn/okta.rsairborne_server/src/provider/authz.rsairborne_server/src/provider/authz/casbin.rsairborne_server/src/provider/authz/migration.rsairborne_server/src/provider/authz/permission.rsairborne_server/src/release.rsairborne_server/src/service_account.rsairborne_server/src/service_account/types.rsairborne_server/src/token.rsairborne_server/src/types.rsairborne_server/src/user.rsairborne_server/src/user/types.rsairborne_server/src/utils.rsairborne_server/src/utils/db/models.rsairborne_server/src/utils/db/schema.rsairborne_server/src/utils/keycloak.rsairborne_server/src/utils/transaction_manager.rsmemory-bank/techContext.md
💤 Files with no reviewable changes (9)
- airborne_server/src/utils.rs
- airborne_server/src/organisation/application/types.rs
- airborne_server/src/organisation/types.rs
- airborne_server/src/organisation/transaction.rs
- airborne_server/src/organisation/application/user/transaction.rs
- airborne_server/src/organisation/application/user/utils.rs
- airborne_server/src/utils/transaction_manager.rs
- airborne_server/src/organisation/user/utils.rs
- airborne_server/src/organisation/user/transaction.rs
| const { token, org, app } = useAppContext(); | ||
| const permissions = usePagePermissions(PAGE_AUTHZ); | ||
| const { toast } = useToast(); | ||
| const canManageCohorts = | ||
| permissions.can("update_cohort") || | ||
| permissions.can("create_cohort_group") || | ||
| permissions.can("update_cohort_group"); |
There was a problem hiding this comment.
Split read, checkpoint, and group capabilities instead of using one canManageCohorts flag.
This boolean now gates checkpoint creation, group creation, and group-priority editing even though those actions map to different permissions, so users can see controls that will only 403. The new read permissions are also never used to stop the loaders, so callers without read access still fire the endpoints and can fall into the misleading empty-state path. Please model canRead*, canCreateCheckpoint, canCreateGroup, and canUpdateGroupPriority separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/cohorts/page.tsx around
lines 71 - 77, The current single canManageCohorts boolean (derived from
permissions.can in the page using usePagePermissions(PAGE_AUTHZ)) conflates
distinct permissions and leads to showing controls and firing loaders for
actions the user cannot perform; replace it with separate flags such as
canReadCohorts/canReadCheckpoints (for gating data loaders and preventing API
calls when read access is absent), canCreateCheckpoint (for showing/enabling
checkpoint creation UI), canCreateGroup (for group creation UI), and
canUpdateGroupPriority (for editing group priority). Update all places that
reference canManageCohorts in this file (including the loaders, conditional UI
rendering, and action handlers) to use the appropriate new boolean so reads
block loaders when absent and each control is only visible/enabled when its
specific permission (via permissions.can("...")) is true.
| const PAGE_AUTHZ = definePagePermissions({ | ||
| read_packages: permission("package", "read", "app"), | ||
| create_package: permission("package", "create", "app"), | ||
| }); | ||
|
|
||
| export default function PackagesPage() { | ||
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const [page, setPage] = useState(1); | ||
| const count = 10; | ||
| const debouncedSearchQuery = useDebouncedValue(searchQuery, 500); | ||
| const { token, org, app, getAppAccess, getOrgAccess } = useAppContext(); | ||
| const { token, org, app } = useAppContext(); | ||
| const permissions = usePagePermissions(PAGE_AUTHZ); | ||
| const canCreatePackage = permissions.can("create_package"); |
There was a problem hiding this comment.
read_packages currently has no effect.
This page only consumes create_package, so users without package read access still navigate here and fire /packages/list. If the API rejects them, the screen falls through to the normal empty-state flow instead of a denial. Gate the page on read_packages before starting SWR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/packages/page.tsx around
lines 35 - 47, The page registers a read_packages permission but never uses it,
so data fetching (SWR) can run for users without read access; update
PackagesPage to check permissions.can("read_packages") (from
usePagePermissions(PAGE_AUTHZ)) before initiating any SWR/fetch hooks or
rendering fetch-dependent UI—if the check fails, return an
access-denied/unauthorized UI or redirect immediately (short-circuit the
component) so /packages/list is never called for unauthorized users while
keeping canCreatePackage logic intact.
| if (!permissions.isReady) { | ||
| return ( | ||
| <div className="p-6 flex items-center justify-center min-h-screen"> | ||
| <p className="text-muted-foreground">Checking access...</p> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const hasAccess = hasAppAccess(getOrgAccess(org), getAppAccess(org, params.appId), "write"); | ||
| const hasAccess = permissions.can("read_release") && permissions.can("create_release"); |
There was a problem hiding this comment.
Gate the release fetch on the permission verdict.
useSWR is initialized before this guard runs, so users without read_release/create_release can still hit /releases/:id while permissions resolve and only then fall into notFound(). Fold permissions.isReady && hasAccess into shouldFetch so the data request only starts after authz succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@airborne_dashboard/app/dashboard/`[orgId]/[appId]/releases/[releaseId]/clone/page.tsx
around lines 33 - 41, The SWR fetch is starting before the permission verdict,
allowing unauthorized requests; update the useSWR initialization to include
permissions.isReady && hasAccess as part of its shouldFetch condition (i.e.,
only call useSWR or pass a non-null key when permissions.isReady && hasAccess is
true). Locate the permissions.isReady / hasAccess logic and the useSWR call in
page.tsx and fold the combined check (permissions.isReady && hasAccess) into the
SWR key/shouldFetch so the release fetch only fires after authorization; keep
the existing notFound() behavior for when access is denied.
| const PAGE_AUTHZ = definePagePermissions({ | ||
| read_release: permission("release", "read", "app"), | ||
| update_release: permission("release", "update", "app"), | ||
| }); |
There was a problem hiding this comment.
Split Clone Release out from the update_release gate.
The clone CTA routes into a release-creation flow, but this page never requests create_release. Update-only users now get a create entry point, and create-only users lose clone.
Minimal direction
const PAGE_AUTHZ = definePagePermissions({
read_release: permission("release", "read", "app"),
update_release: permission("release", "update", "app"),
+ create_release: permission("release", "create", "app"),
});Then gate the clone button with permissions.can("create_release") separately from the update-only lifecycle actions.
Also applies to: 333-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@airborne_dashboard/app/dashboard/`[orgId]/[appId]/releases/[releaseId]/page.tsx
around lines 51 - 54, PAGE_AUTHZ currently only defines read_release and
update_release, but the Clone Release CTA needs create_release and must be gated
separately; add create_release to definePagePermissions (e.g.,
permission("release","create","app")) and update the UI logic so the Clone
button checks permissions.can("create_release") independently from the
update_release gates used for update lifecycle actions (look for the Clone
button render and the update-related action handlers).
| const { token, org, app, setOrg, setApp } = useAppContext(); | ||
| const permissions = usePagePermissions(PAGE_AUTHZ); |
There was a problem hiding this comment.
Block on authz resolution before rendering the release body.
This page only denies access after permissions.isReady, but it still falls through to toServeReleaseConfig(data) and the rest of the UI while the permission request is unresolved. A failed permission lookup is also treated as notFound() instead of an error state.
Suggested guard
- if (isLoading) return <div>Loading...</div>;
- if (permissions.isReady && !permissions.can("read_release")) {
+ if (isLoading || !permissions.isReady) return <div>Loading...</div>;
+ if (permissions.error) {
+ return <div>Failed to verify access.</div>;
+ }
+ if (!permissions.can("read_release")) {
notFound();
}Based on learnings from airborne_dashboard/hooks/use-page-permissions.ts:1-70, isReady becomes true when the permission request errors, and can() falls back to false.
Also applies to: 156-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@airborne_dashboard/app/dashboard/`[orgId]/[appId]/releases/[releaseId]/page.tsx
around lines 126 - 127, The page currently proceeds to compute and render
release UI before permission resolution; update the component to wait for
usePagePermissions(PAGE_AUTHZ) to finish by checking permissions.isReady before
calling toServeReleaseConfig(data) or rendering the release body, and handle
permission fetch errors separately (show an error state when the permission
request errored rather than treating it as notFound); use permissions.can(...)
only after isReady is true to decide access, and if isReady is true but can(...)
is false call notFound() (or render an access-denied UI) so failed lookups
(where isReady becomes true with an error) surface as an error state instead of
silently proceeding.
| useEffect(() => { | ||
| if (token) router.replace("/dashboard"); | ||
| }, [token]); | ||
| if (!config) return; | ||
| if (!registrationEnabled) router.replace("/login"); | ||
| }, [token, config, registrationEnabled, router]); |
There was a problem hiding this comment.
Return after sending authenticated users to /dashboard.
When token is set and registration is disabled, this effect calls router.replace("/dashboard") and then immediately calls router.replace("/login"). Logged-in users can end up bounced back to the login page.
Suggested fix
useEffect(() => {
- if (token) router.replace("/dashboard");
+ if (token) {
+ router.replace("/dashboard");
+ return;
+ }
if (!config) return;
if (!registrationEnabled) router.replace("/login");
}, [token, config, registrationEnabled, router]);📝 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.
| useEffect(() => { | |
| if (token) router.replace("/dashboard"); | |
| }, [token]); | |
| if (!config) return; | |
| if (!registrationEnabled) router.replace("/login"); | |
| }, [token, config, registrationEnabled, router]); | |
| useEffect(() => { | |
| if (token) { | |
| router.replace("/dashboard"); | |
| return; | |
| } | |
| if (!config) return; | |
| if (!registrationEnabled) router.replace("/login"); | |
| }, [token, config, registrationEnabled, router]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airborne_dashboard/app/register/page.tsx` around lines 43 - 47, The effect
currently calls router.replace("/dashboard") when token exists but continues and
may call router.replace("/login") if registrationEnabled is false; update the
useEffect in page.tsx so that after detecting token and calling
router.replace("/dashboard") you immediately return (or otherwise short-circuit)
to prevent subsequent checks; locate the useEffect that references token,
config, registrationEnabled and router and add the early return after the
dashboard redirect to stop falling through to the login redirect.
| CREATE TABLE IF NOT EXISTS hyperotaserver.authz_role_bindings ( | ||
| scope TEXT NOT NULL CHECK (scope IN ('org', 'app')), | ||
| role_key TEXT NOT NULL, | ||
| resource TEXT NOT NULL, | ||
| action TEXT NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| PRIMARY KEY (scope, role_key, resource, action) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS authz_role_bindings_scope_resource_action_idx | ||
| ON hyperotaserver.authz_role_bindings (scope, resource, action); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS authz_role_bindings_scope_role_key_idx | ||
| ON hyperotaserver.authz_role_bindings (scope, role_key); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS hyperotaserver.authz_memberships ( | ||
| subject TEXT NOT NULL, | ||
| scope TEXT NOT NULL CHECK (scope IN ('org', 'app')), | ||
| organisation TEXT NOT NULL, | ||
| application TEXT NOT NULL, | ||
| role_key TEXT NOT NULL, | ||
| role_level INT4 NOT NULL DEFAULT 0, | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| PRIMARY KEY (subject, scope, organisation, application) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS authz_memberships_scope_org_app_idx | ||
| ON hyperotaserver.authz_memberships (scope, organisation, application); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS authz_memberships_subject_scope_org_idx | ||
| ON hyperotaserver.authz_memberships (subject, scope, organisation); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS authz_memberships_scope_org_role_idx | ||
| ON hyperotaserver.authz_memberships (scope, organisation, role_key); |
There was a problem hiding this comment.
Avoid IF NOT EXISTS in versioned migrations.
If these tables or indexes already exist with the wrong definition, this migration will silently pass and leave schema drift that no longer matches airborne_server/src/utils/db/models.rs:195-239. Versioned migrations should usually fail loudly here, or explicitly assert the expected shape, instead of skipping creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@airborne_server/migrations/20260409193000_add_authz_bindings_and_memberships/up.sql`
around lines 1 - 34, Remove the silent "IF NOT EXISTS" behavior and make the
migration fail if objects already exist or assert their exact shape: replace
CREATE TABLE IF NOT EXISTS hyperotaserver.authz_role_bindings and CREATE TABLE
IF NOT EXISTS hyperotaserver.authz_memberships with plain CREATE TABLE so
Postgres raises an error if the table exists, and do the same for the three
CREATE INDEX IF NOT EXISTS statements
(authz_role_bindings_scope_resource_action_idx,
authz_role_bindings_scope_role_key_idx, authz_memberships_scope_org_app_idx,
authz_memberships_subject_scope_org_idx, authz_memberships_scope_org_role_idx);
alternatively, if you must support existing objects, add explicit runtime checks
that validate column names, types, constraints and index definitions against the
expected model (the schema implied by authz_role_bindings and authz_memberships
and referenced in airborne_server/src/utils/db/models.rs) and fail the migration
with a clear error if they differ.
|
|
||
| state | ||
| .authz_provider | ||
| .create_application(state.get_ref(), &organisation, &application, sub) |
There was a problem hiding this comment.
Use the canonical authz subject here, not the raw JWT sub.
create_application() seeds the creator’s initial application policy. Passing the raw sub claim here breaks the authz subject contract and can leave the creator without access to the app they just created. Please pass the same email-based subject the authz layer uses for policy lookups. Based on learnings: In airborne_server/src/provider/authz.rs, the subject_from_claims() default implementation intentionally uses the email claim as the canonical authorization subject for Casbin policy lookups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airborne_server/src/organisation/application.rs` at line 166, The call to
create_application(...) is passing the raw JWT sub claim which breaks the authz
subject contract; instead obtain and pass the canonical email-based subject used
by the authz layer (the same value returned by authz::subject_from_claims or its
default subject_from_claims implementation) so the creator’s initial application
policy is seeded under the correct subject for Casbin lookups; replace use of
the raw sub variable with the result of subject_from_claims(claims) (or the
equivalent email-based subject extraction) when calling create_application.
| let mut conn = state.db_pool.get()?; | ||
|
|
||
| // Get Keycloak Admin Token | ||
| let client = reqwest::Client::new(); | ||
| let admin_token = get_token(state.env.clone(), client).await.map_err(|e| { | ||
| info!("Error retrieving Keycloak admin token: {:?}", e); | ||
| ABError::Unauthorized(format!("Error retrieving Keycloak admin token: {}", e)) | ||
| })?; | ||
| info!("Admin token retrieved successfully."); | ||
| let client = reqwest::Client::new(); | ||
| let admin = KeycloakAdmin::new(&state.env.keycloak_url.clone(), admin_token, client); | ||
| let realm = state.env.realm.clone(); | ||
|
|
||
| let groups = admin | ||
| .realm_groups_get( | ||
| &realm, | ||
| None, | ||
| Some(true), // Exact Match | ||
| None, | ||
| Some(2), // Check only one group; Should be 5xx if more than 1 | ||
| Some(false), | ||
| None, | ||
| Some(organisation.clone()), | ||
| ) | ||
| let new_workspace_name = NewWorkspaceName { | ||
| organization_id: &organisation, | ||
| application_id: &application, | ||
| workspace_name: "pending", | ||
| }; | ||
|
|
||
| let superposition_org_id_from_env = state.env.superposition_org_id.clone(); | ||
| let mut inserted_workspace: WorkspaceName = diesel::insert_into(workspace_names::table) | ||
| .values(&new_workspace_name) | ||
| .get_result(&mut conn) | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!("Failed to store workspace name: {}", e)) | ||
| })?; | ||
|
|
||
| let generated_id = inserted_workspace.id; | ||
| let generated_workspace_name = format!("workspace{}", generated_id); | ||
| inserted_workspace.workspace_name = generated_workspace_name.clone(); | ||
|
|
||
| diesel::update(workspace_names::table.filter(workspace_names::id.eq(generated_id))) | ||
| .set(workspace_names::workspace_name.eq(&generated_workspace_name)) | ||
| .execute(&mut conn) | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!("Failed to update workspace name: {}", e)) | ||
| })?; | ||
|
|
||
| state | ||
| .superposition_client | ||
| .create_workspace() | ||
| .org_id(superposition_org_id_from_env.clone()) | ||
| .workspace_name(generated_workspace_name.clone()) | ||
| .workspace_status(WorkspaceStatus::Enabled) | ||
| .allow_experiment_self_approval(true) | ||
| .workspace_admin_email("pp-sdk@juspay.in".to_string()) | ||
| .send() | ||
| .await | ||
| .map_err(|e| ABError::InternalServerError(format!("{}", e)))?; | ||
|
|
||
| if groups.is_empty() { | ||
| Err(ABError::NotFound(format!( | ||
| "Organisation '{}' not found in Keycloak", | ||
| organisation | ||
| ))) | ||
| } | ||
| // It is possible that application group comes up in this query; Change to path | ||
| // else if groups.len() != 1 { | ||
| // return Err(error::ErrorInternalServerError(Json(json!({"Error" : "Inconsistant database entries"})))); | ||
| // } | ||
| else { | ||
| // Reject if application already exists | ||
| if groups[0] | ||
| .sub_groups | ||
| .clone() | ||
| .unwrap_or_default() | ||
| .iter() | ||
| .any(|g| g.name == Some(application.clone())) | ||
| { | ||
| return Err(ABError::BadRequest(format!( | ||
| "Application '{}' already exists in organisation '{}'", | ||
| application, organisation | ||
| ))); | ||
| } | ||
|
|
||
| // Step 1: Create application group in Keycloak | ||
| let parent_group_id = match admin | ||
| .realm_groups_with_group_id_children_post( | ||
| &realm, | ||
| &groups[0].id.clone().unwrap_or_default().clone(), | ||
| GroupRepresentation { | ||
| name: Some(application.clone()), | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(id) => { | ||
| let group_id = id.unwrap_or_default(); | ||
| // Record this resource in the transaction | ||
| transaction.add_keycloak_group(&group_id); | ||
| info!("Created application group with ID: {}", group_id); | ||
| group_id | ||
| } | ||
| Err(e) => { | ||
| // No rollback needed yet - this is the first operation | ||
| return Err(ABError::InternalServerError(format!( | ||
| "Failed to create application group: {}", | ||
| e | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| // Step 2: Create role groups and add user to them | ||
| let roles = ["read", "write", "admin"]; | ||
| for role in roles { | ||
| match admin | ||
| .realm_groups_with_group_id_children_post( | ||
| &realm, | ||
| &parent_group_id, | ||
| GroupRepresentation { | ||
| name: Some(role.to_string()), | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(id) => { | ||
| let role_group_id = id.unwrap_or_default(); | ||
| // Record this resource in the transaction | ||
| transaction.add_keycloak_group(&role_group_id); | ||
| info!("Created role group {} with ID: {}", role, role_group_id); | ||
|
|
||
| // Add the user to the role-specific group | ||
| match admin | ||
| .realm_users_with_user_id_groups_with_group_id_put( | ||
| &realm, | ||
| sub, | ||
| &role_group_id, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(_) => { | ||
| // Record this user-group relationship in the transaction | ||
| transaction.add_keycloak_resource( | ||
| "user_group_membership", | ||
| &format!("{}:{}", sub, role_group_id), | ||
| ); | ||
| info!("Added user to role group: {}", role); | ||
| } | ||
| Err(e) => { | ||
| // Handle rollback and return error | ||
| if let Err(rollback_err) = transaction | ||
| .handle_rollback_if_needed(&admin, &realm, &state) | ||
| .await | ||
| { | ||
| info!("Rollback failed: {}", rollback_err); | ||
| } | ||
|
|
||
| return Err(ABError::InternalServerError(format!( | ||
| "Failed to add user to role group {}: {}", | ||
| role, e | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| // Handle rollback and return error | ||
| if let Err(rollback_err) = transaction | ||
| .handle_rollback_if_needed(&admin, &realm, &state) | ||
| .await | ||
| { | ||
| info!("Rollback failed: {}", rollback_err); | ||
| } | ||
|
|
||
| return Err(ABError::InternalServerError(format!( | ||
| "Failed to create role group {}: {}", | ||
| role, e | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Store workspace name in our database with a placeholder, then update to "workspace{id}" | ||
| let new_workspace_name = NewWorkspaceName { | ||
| organization_id: &organisation, | ||
| application_id: &application, | ||
| workspace_name: "pending", | ||
| }; | ||
|
|
||
| let superposition_org_id_from_env = state.env.superposition_org_id.clone(); | ||
| info!( | ||
| "Using Superposition Org ID from environment: {}", | ||
| superposition_org_id_from_env | ||
| ); | ||
| // Insert and get the inserted row (to get the id) | ||
| let mut inserted_workspace: WorkspaceName = diesel::insert_into(workspace_names::table) | ||
| .values(&new_workspace_name) | ||
| .get_result(&mut conn) | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!("Failed to store workspace name: {}", e)) | ||
| })?; | ||
|
|
||
| let generated_id = inserted_workspace.id; | ||
| let generated_workspace_name = format!("workspace{}", generated_id); | ||
| inserted_workspace.workspace_name = generated_workspace_name.clone(); | ||
|
|
||
| // Update the workspace_name to "workspace{id}" | ||
| diesel::update(workspace_names::table.filter(workspace_names::id.eq(generated_id))) | ||
| .set(workspace_names::workspace_name.eq(&generated_workspace_name)) | ||
| .execute(&mut conn) | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!("Failed to update workspace name: {}", e)) | ||
| })?; | ||
|
|
||
| // Step 4: Create workspace in Superposition | ||
|
|
||
| match state | ||
| .superposition_client | ||
| .create_workspace() | ||
| .org_id(superposition_org_id_from_env.clone()) | ||
| .workspace_name(generated_workspace_name.clone()) | ||
| .workspace_status(WorkspaceStatus::Enabled) | ||
| .allow_experiment_self_approval(true) | ||
| .workspace_admin_email("pp-sdk@juspay.in".to_string()) | ||
| .send() | ||
| .await | ||
| { | ||
| Ok(workspace) => { | ||
| // Record Superposition resource using workspace name as the ID | ||
| transaction.set_superposition_resource(&workspace.workspace_name); | ||
| info!("Created workspace in Superposition: {:?}", workspace); | ||
| workspace | ||
| } | ||
| Err(e) => { | ||
| // Handle rollback and return error | ||
| if let Err(rollback_err) = transaction | ||
| .handle_rollback_if_needed(&admin, &realm, &state) | ||
| .await | ||
| { | ||
| info!("Rollback failed: {}", rollback_err); | ||
| } | ||
|
|
||
| return Err(ABError::InternalServerError(format!( | ||
| "Failed to create workspace in Superposition: {}", | ||
| e | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| // Helper function to create default config with error handling | ||
| async fn create_config_with_tx<E>( | ||
| create_fn: impl futures::Future<Output = Result<(), E>>, | ||
| key: &str, | ||
| transaction: &TransactionManager, | ||
| admin: &KeycloakAdmin, | ||
| realm: &str, | ||
| state: &web::Data<AppState>, | ||
| ) -> Result<(), ABError> | ||
| where | ||
| E: std::fmt::Display, | ||
| { | ||
| match create_fn.await { | ||
| Ok(result) => { | ||
| info!("Created configuration for key: {}", key); | ||
| Ok(result) | ||
| } | ||
| Err(e) => { | ||
| // Handle rollback | ||
| if let Err(rollback_err) = transaction | ||
| .handle_rollback_if_needed(admin, realm, state) | ||
| .await | ||
| { | ||
| info!("Rollback failed: {}", rollback_err); | ||
| } | ||
|
|
||
| Err(ABError::InternalServerError(format!( | ||
| "Failed to create configuration for {}: {}", | ||
| key, e | ||
| ))) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| create_config_with_tx( | ||
| async { | ||
| migrate_superposition_workspace( | ||
| &inserted_workspace, | ||
| &state, | ||
| &SuperpositionMigrationStrategy::Patch, | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!("Workspace migration error: {}", e)) | ||
| }) | ||
| }, | ||
| "migrate_superposition_workspace", | ||
| &transaction, | ||
| &admin, | ||
| &realm, | ||
| &state, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Mark transaction as complete since all operations have succeeded | ||
| transaction.set_database_inserted(); | ||
|
|
||
| Ok(Json(Application { | ||
| application, | ||
| organisation, | ||
| access: roles.iter().map(|&s| s.to_string()).collect(), | ||
| })) | ||
| } | ||
| .map_err(|e| { | ||
| ABError::InternalServerError(format!( | ||
| "Failed to create workspace in Superposition: {}", | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| migrate_superposition_workspace( | ||
| &inserted_workspace, | ||
| &state, | ||
| &SuperpositionMigrationStrategy::Patch, | ||
| ) | ||
| .await | ||
| .map_err(|e| ABError::InternalServerError(format!("Workspace migration error: {}", e)))?; |
There was a problem hiding this comment.
Handle partial provisioning failures before returning.
After the authz provider call succeeds, this path mutates workspace_names, Superposition workspace creation, and workspace migration with no compensation. If any later step fails, retries can hit an already-created app in authz while the DB row or workspace is missing/half-initialized. Please add best-effort rollback or persist a recoverable failed state instead of leaving the systems diverged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airborne_server/src/organisation/application.rs` around lines 169 - 218, The
code creates a DB row (inserted_workspace), updates it, calls
state.superposition_client.create_workspace() and then
migrate_superposition_workspace() with no compensating actions on later failure;
add a best-effort rollback or durable failed marker: if create_workspace() or
migrate_superposition_workspace(...) returns an error, attempt to delete or mark
the workspace_names row (workspace_names::table filtered by
workspace_names::id.eq(generated_id)) and also attempt to delete the
Superposition workspace via state.superposition_client (using the same org_id
and generated_workspace_name) before returning the ABError; if compensating
deletes fail, persist a recoverable failed state on the DB row (e.g. set a
workspace_status/failure_reason column or set workspace_name to a "failed-..."
sentinel) so retries can detect/infer partial provisioning, and ensure all calls
reference inserted_workspace, generated_id, generated_workspace_name,
state.superposition_client.create_workspace(), and
migrate_superposition_workspace() so the rollback paths target the same
resources.
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init; \ | ||
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init; \ |
There was a problem hiding this comment.
Restore init dependency order in run target.
Line 488 currently runs localstack-init before keycloak-init, but airborne_server/scripts/init-localstack.sh reads auth secrets from .env.generated produced by airborne_server/scripts/init-keycloak.sh. This can encrypt empty/stale values and break auth bootstrapping.
🔧 Suggested fix
USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) superposition-init; \
- USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init; \
- USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init; \
+ USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init; \
+ USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init; \📝 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.
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init; \ | |
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init; \ | |
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init; \ | |
| USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init; \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 488 - 489, The run target in the Makefile runs
localstack-init before keycloak-init which causes localstack to read stale/empty
auth secrets; change the invocation order so
USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) keycloak-init runs before
USE_ENCRYPTED_SECRETS=$$ENCRYPTION_MODE $(MAKE) localstack-init (i.e., swap the
two recipe invocations under the run target) so
airborne_server/scripts/init-keycloak.sh generates .env.generated before
airborne_server/scripts/init-localstack.sh consumes it.
feat: update to use oidc crate fix: google signin issues fix: PKCE
fix: casbin rbac fix: leeway config for clock drift fix: user profile in keycloak signup fix: lint fix: refactor fix: isues macro based enforcement frontend authz checks fix: frontend lint fix: added crate description fix: lint fix: gated unbounded vec length fix: coderabbit fix: lint feat: keycloak to casbin driven by env fix: lint fix: db pool
yuvrajjsingh0
force-pushed
the
service-accounts
branch
from
8838b07 to
7898867
Compare
2 participants
Summary by CodeRabbit
New Features
API Changes
first_name,last_name, andemailfieldsUI Updates