feat: service accounts - #361
Open
yuvrajjsingh0 wants to merge 1 commit into
Open
Conversation
|
Review changes with Changed Files
|
|
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 adds organisation service accounts: a new database table and migrations, Diesel schema/model additions, AuthN provider support (base and Keycloak) for creating/deleting service-account users, backend API endpoints for create/list/delete/rotate, token issuance changes to resolve service-account credentials, and dashboard UI for managing service accounts. ChangesService Accounts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServiceAccountRoute
participant AuthNProvider
participant Database
participant AuthZProvider
Client->>ServiceAccountRoute: POST /service-accounts
ServiceAccountRoute->>AuthNProvider: create_service_account_user(username, email, password)
AuthNProvider-->>ServiceAccountRoute: UserToken (refresh token as client_secret)
ServiceAccountRoute->>Database: insert ServiceAccountEntry
ServiceAccountRoute->>AuthZProvider: add org membership with role
ServiceAccountRoute-->>Client: CreateServiceAccountResponse
sequenceDiagram
participant TokenEndpoint
participant UserCredentialsTable
participant ServiceAccountsTable
participant AuthNProvider
TokenEndpoint->>UserCredentialsTable: lookup by client_id
alt found
TokenEndpoint->>TokenEndpoint: decrypt stored refresh token with client_secret
else not found
TokenEndpoint->>ServiceAccountsTable: lookup by client_id
alt found
TokenEndpoint->>TokenEndpoint: use client_secret as refresh token
else not found
TokenEndpoint-->>TokenEndpoint: return Unauthorized
end
end
TokenEndpoint->>AuthNProvider: refresh_access_token(refresh_token)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
airborne_server/src/service_account.rs (1)
278-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rotate_service_accountreuses the"create"authz action.Tagging rotate as
action = "create"(same as the actual create endpoint) makes the two operations indistinguishable in any RBAC policy or audit trail keyed on action name. A dedicated action (e.g."rotate") would allow finer-grained policy/audit control later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airborne_server/src/service_account.rs` around lines 278 - 284, The rotate endpoint is currently using the same authz action as service-account creation, which makes rotation indistinguishable in policy and audit flows. Update the authz annotation on rotate_service_account to use a dedicated action name such as "rotate" instead of reusing "create", keeping the resource and role settings intact so RBAC and audit logs can differentiate the operations.airborne_server/src/token.rs (1)
296-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCredential-resolution logic is dense; consider extracting a helper.
The PAT-vs-service-account resolution block mixes two DB round trips, decryption, and error branching inline inside
issue_token. Extracting it into a small helper (e.g.resolve_refresh_token(pool, client_id, client_secret)) would make it independently unit-testable and easier to reason about, especially as more credential sources may be added later. Note also that both failure paths correctly return the same generic "Invalid credentials" message, avoiding a credential-type oracle — good practice to keep intact in any refactor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airborne_server/src/token.rs` around lines 296 - 342, The refresh-token resolution logic inside issue_token is doing too much inline, mixing PAT lookup, service-account fallback, decryption, and error mapping. Extract this block into a helper such as resolve_refresh_token(state.db_pool, client_id, client_secret) that encapsulates the two DB checks and decrypt_string flow, and keep the existing generic Unauthorized("Invalid credentials") behavior unchanged for both failure paths. Use the existing identifiers issue_token, decrypt_string, user_credentials_table, and service_accounts_table to preserve current behavior while making the logic easier to test and extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airborne_dashboard/app/dashboard/`[orgId]/users/page.tsx:
- Around line 206-215: Prevent duplicate rotations in handleRotate by guarding
the rotate action so it can only run once at a time; since apiFetch to
/service-accounts/.../rotate is non-idempotent, a second click can create a
stale secret. Update the users/page.tsx handler around handleRotate (and any
related rotate button wiring) to disable the trigger while the request is in
flight or to early-return when a rotation is already pending, and make sure the
UI state is reset in both success and failure paths.
- Around line 169-171: Scope the service-account SWR fetch in the users page so
it only runs when the current user has read access, not just create access, and
make the cache key include org/token context instead of the static
"/service-accounts" string. Update the useSWR call in the service-account
section (and the similar usages referenced elsewhere in the page) so the GET is
gated by the read permission path and the key changes when org or token changes,
preventing stale cross-org metadata reuse and the avoidable 403 toast for
create-only users.
- Around line 306-315: The service-account Role select is hard-coded to
admin/write/read, which prevents assigning custom org roles from the UI. Update
the role picker inside the users page component in page.tsx to use the same
dynamically loaded org role list already used for user management, and bind the
options to the existing role state and change handler rather than static
SelectItem entries. Ensure the service-account creation flow reuses the org
roles source of truth so any custom role available in the organization can be
selected.
- Around line 122-132: Add accessible names to the icon-only copy actions in the
user details UI: the two Button components in the users page that call
copyToClipboard for clientId and clientSecret need aria-labels so screen readers
can distinguish them. Update the Button props in the component that renders the
Client ID and Client Secret rows, using labels that clearly describe each copy
action.
In `@airborne_server/src/organisation/user.rs`:
- Around line 72-79: The user email check in the user creation flow hardcodes
the service-account domain, which duplicates the source of truth. Update the
validation in the user handling logic to use the shared
SERVICE_ACCOUNT_EMAIL_DOMAIN from service_account.rs (make it pub(crate) if
needed) instead of the literal string, so the check stays consistent if the
domain changes.
In `@airborne_server/src/provider/authn/keycloak.rs`:
- Around line 174-232: The create_service_account_user flow leaves a Keycloak
user behind if password_login_common(..., Some("offline_access")) fails after
admin.realm_users_post succeeds. Update create_service_account_user to clean up
the newly created user on any post-creation login error, using the same
KeycloakAdmin instance and the created username as the rollback target. Keep the
existing find_user_by_username duplicate check and ensure the rollback happens
before returning the error so retries do not get blocked by an orphaned Keycloak
identity.
In `@airborne_server/src/service_account.rs`:
- Around line 312-322: The rotate flow in service_account.rs deletes the
existing OIDC user before confirming the replacement can be created, which can
leave the service account without any backing identity if
create_service_account_user fails. Update the rotate sequence in the service
account rotation logic to create the new credentials first, verify success via
authn_provider.create_service_account_user, and only then call
authn_provider.delete_user for the old account so the replacement window is
minimized.
- Around line 99-174: create_service_account has a partial-failure gap across
create_service_account_user, the DB insert, and add_organisation_user: if a
later step fails, the OIDC user is left orphaned. Update the
create_service_account flow to add compensation/rollback after failures from the
DB insert or authz step, using the existing authn_provider and authz_provider
calls plus the generated email/name/client_uid to identify and clean up the
created service account. Ensure the rollback path is best-effort and preserves
the current error propagation from create_service_account.
- Around line 245-255: The best-effort revocation in service_account.rs is
swallowing failures from remove_organisation_user and delete_user, so add error
logging around those calls in the service account deletion flow. Update the
deletion logic near the authz_provider.authn_provider calls to capture and log
any returned errors with enough context (organisation, entry.email, entry.name,
and auth.sub) while keeping the best-effort behavior. Use the existing delete
path in the service account handler as the reference point so operators can see
when identity-provider cleanup failed before the local row is removed.
---
Nitpick comments:
In `@airborne_server/src/service_account.rs`:
- Around line 278-284: The rotate endpoint is currently using the same authz
action as service-account creation, which makes rotation indistinguishable in
policy and audit flows. Update the authz annotation on rotate_service_account to
use a dedicated action name such as "rotate" instead of reusing "create",
keeping the resource and role settings intact so RBAC and audit logs can
differentiate the operations.
In `@airborne_server/src/token.rs`:
- Around line 296-342: The refresh-token resolution logic inside issue_token is
doing too much inline, mixing PAT lookup, service-account fallback, decryption,
and error mapping. Extract this block into a helper such as
resolve_refresh_token(state.db_pool, client_id, client_secret) that encapsulates
the two DB checks and decrypt_string flow, and keep the existing generic
Unauthorized("Invalid credentials") behavior unchanged for both failure paths.
Use the existing identifiers issue_token, decrypt_string,
user_credentials_table, and service_accounts_table to preserve current behavior
while making the logic easier to test and extend.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef4eec4f-9770-4e32-9230-822f7334f71b
📒 Files selected for processing (13)
airborne_dashboard/app/dashboard/[orgId]/users/page.tsxairborne_dashboard/next.config.mjsairborne_server/migrations/20260410120000_add_service_accounts/down.sqlairborne_server/migrations/20260410120000_add_service_accounts/up.sqlairborne_server/src/main.rsairborne_server/src/organisation/user.rsairborne_server/src/provider/authn.rsairborne_server/src/provider/authn/keycloak.rsairborne_server/src/service_account.rsairborne_server/src/service_account/types.rsairborne_server/src/token.rsairborne_server/src/utils/db/models.rsairborne_server/src/utils/db/schema.rs
yuvrajjsingh0
force-pushed
the
service-accounts-1
branch
from
July 6, 2026 16:11
94eb198 to
e549b53
Compare
yuvrajjsingh0
force-pushed
the
service-accounts-1
branch
from
July 6, 2026 16:11
e549b53 to
5fc2728
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes