Skip to content

Group-based RBAC for integrations, enforced at the runtime proxy - #89

Merged
abskrj merged 2 commits into
mainfrom
devin/1787823920-rbac-user-groups
Aug 27, 2026
Merged

abskrj merged 2 commits into
mainfrom
devin/1787823920-rbac-user-groups

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Integrations can now be restricted to tenant user groups, and the restriction is enforced where it actually matters — the /v1/proxy path a snippet uses at runtime — not just in list endpoints.

Model (migration 029_rbac_user_groups.sql): user_groups, user_group_members, integration_group_grants(group_id, credential_profile_id), nullable api_keys.user_id, tenants.rbac_strict_mode, and widened role CHECKs (owner, integration_manager, member, viewer alongside admin/manage/invoke). IDs are TEXT to match the existing schema convention rather than the uuid in the request.

Visibility rule (handlers/access.go), chosen so enabling RBAC changes nothing retroactively:

profile has zero grants        -> visible to whole tenant
profile has >=1 grant          -> visible to admin/owner + members of a granted group
api key with user_id           -> inherits that user's groups
api key with user_id = NULL    -> legacy tenant-wide, sees all unless tenant.rbac_strict_mode

Runtime enforcement is the load-bearing part. The proxy's only trust input today is X-Velane-Tenant, so a plain VELANE_CALLER_GROUPS env var would be trivially forgeable from inside the sandbox. Instead the scheduler mints an HMAC-signed, 1h-expiry token over the caller's claims and the proxy verifies it:

// scheduler.injectProxyEnv(env, tenantID, caller)
env[callerid.EnvVar] = callerid.Sign(s.encKey, callerid.Claims{TenantID, UserID, Role, GroupIDs}, ttl)

// handlers: Proxy -> callerMayUseConnection
claims := verify(X-Velane-Caller)            // nil when absent/forged/tenant-mismatch
if claims == nil  { return !restricted || !tenant.rbac_strict_mode }
if isAdminOrOwner(claims.Role) || !restricted { return true }
return store.CredentialProfileGrantedToGroups(profileID, claims.GroupIDs)   // else 403

VELANE_CALLER_GROUPS is still exported for introspection but is never trusted. The token rides in SecretEnvVars, so it survives the Redis queue into async/streaming workers. Both platform libraries forward it as X-Velane-Caller on every proxy call.

Roles map onto existing scopes in roleHasScope (via models.RoleGrantsScope) so RequireScope keeps working unchanged; group management uses a new RequireRole (admin/owner; grants also allow integration_manager).

Notes / deliberate gaps:

  • viewer currently grants no scope, so viewers cannot hit routes that require invoke. Read-only scope semantics need a follow-up decision.
  • rbac_strict_mode has store accessors and enforcement but no management endpoint yet — it is opt-in per tenant via SetTenantRBACStrictMode.
  • The caller token is signed and expiring but not bound to a specific invocation record, so replay within the TTL by a caller that already had the token is possible.
  • Event-triggered invocations (worker/integration_events.go) carry no caller identity and therefore behave as legacy tenant-wide callers; they would be denied under strict mode.

Testing

go test ./... (with TEST_DATABASE_URL, -p 1), go build ./..., make openapi-check, make test-platform-libs, npx tsc --noEmit in apps/admin — all pass. New coverage: store group/grant lifecycle and tenant scoping, API tests for granted vs. ungranted member visibility and admin see-all, proxy 200/403 for granted/ungranted/admin/legacy/strict-mode, and callerid signature, tamper, foreign-key and expiry rejection.

Link to Devin session: https://app.devin.ai/sessions/ed1ea4614fc74dc1bb804634fcb75d86
Open in Devin Desktop: https://app.devin.ai/desktop/session/ed1ea4614fc74dc1bb804634fcb75d86?variant=devin
Requested by: @abskrj

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot added the enhancement New feature or request label Aug 27, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +147 to +170
func (s *Scheduler) injectProxyEnv(env map[string]string, tenantID string, caller models.CallerIdentity) {
if s.internalProxyURL != "" {
env["VELANE_PROXY_URL"] = s.internalProxyURL
}
env["VELANE_TENANT_ID"] = tenantID

caller.TenantID = tenantID
if len(caller.GroupIDs) > 0 {
env["VELANE_CALLER_GROUPS"] = strings.Join(caller.GroupIDs, ",")
}
if caller.UserID == "" && caller.Role == "" && len(caller.GroupIDs) == 0 {
// Nothing to attest — legacy tenant-wide invocation.
return
}
token, err := callerid.Sign(s.encKey, callerid.Claims{
TenantID: caller.TenantID,
UserID: caller.UserID,
Role: caller.Role,
GroupIDs: caller.GroupIDs,
}, callerid.DefaultTTL)
if err != nil {
return
}
env[callerid.EnvVar] = token

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Non-interactive invocations lose access to restricted integrations

Caller is populated only in InvokeByToken. Replay, git-webhook, and event-triggered invocations build the request with no caller, so injectProxyEnv mints no token and the proxy treats them as unidentified: they can reach only ungranted profiles, and are denied all restricted ones once strict mode is on.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and intended for now — it is called out in the PR description. Replay, git-webhook and event-triggered invocations have no human principal to attest, so they run as legacy tenant-wide callers: unaffected while rbac_strict_mode is off, denied restricted profiles once it is on. Giving them identity needs a product decision (persist the original invoker on the invocation record and re-mint from it, or grant system-triggered runs an explicit service principal per trigger). I'd rather make that call with @abskrj than pick one here.

Comment on lines +161 to +166
token, err := callerid.Sign(s.encKey, callerid.Claims{
TenantID: caller.TenantID,
UserID: caller.UserID,
Role: caller.Role,
GroupIDs: caller.GroupIDs,
}, callerid.DefaultTTL)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Caller token can expire before a queued job runs

The token minted by injectProxyEnv carries a 1h TTL and travels through Redis into async/streaming workers. A job that begins executing more than an hour after enqueue presents an expired token, and the proxy then treats it as an unidentified caller and denies restricted integrations.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True for jobs that sit in the queue longer than the 1h TTL. Raising the TTL trades that off against how long a leaked token stays usable, so the durable fix is to stop carrying a pre-minted token through Redis and instead persist the caller identity on the invocation record and mint the token in the worker at execution time — which also gives the invocation binding the token currently lacks. Flagging rather than changing it here since it reshapes the job schema; happy to do it as a follow-up.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

End-to-end UI verification of group-based RBAC

Tested against a local docker compose stack built from this branch (admin :8092, control-plane :8080), with two concurrent sessions: bootstrap admin and a non-admin tenant member (manage role). Two GitHub credential profiles (restricted, shared) were created with dummy credentials; no OAuth connect was performed.

Visibility enforcement (the core of this PR)

With github · restricted granted to group support-engineers and the member not in the group, the member's Integrations list drops to shared only; after being added to the group, restricted reappears.

Non-member (restricted hidden) Group member (restricted visible)
member without grant member with grant

Admin keeps seeing both profiles throughout (owner/admin bypass):

admin sees both

Group CRUD, membership and persistence

Create group → add member → grant integration → F5: members and grants persist server-side.

group card after reload

Removing the member re-hides the profile; revoking the grant makes it tenant-wide visible again (zero grants = unrestricted); deleting the group returns the page to No groups yet.

no groups yet

Regression checks

API Keys, Team and Integrations pages all render; Team role dropdown exposes all 7 roles; no uncaught console errors during the run.

team page

Not verified (reported rather than faked): real Nango OAuth connection flows (Nango is intentionally not browser-reachable), the runtime proxy caller-token 403 path (needs a live third-party connection in a sandbox run), and API-key group inheritance / rbac_strict_mode enforcement at call time. Those paths are covered by Go tests instead.

@abskrj
abskrj merged commit 14a9520 into main Aug 27, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant