feat: bind role authoring and assignment to the caller's own permissions - #1022
Open
rohan-chaturvedi wants to merge 3 commits into
Open
rohan-chaturvedi wants to merge 3 commits into
rohan-chaturvedi wants to merge 3 commits into
Conversation
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
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.
🔍 Overview
Role authoring and role assignment were not bound by the acting user's own role. A user who could manage roles could define a role with any permission set, including permissions their own role does not hold. A user who could assign roles could then attach that role, or any other existing role, to another member, an invite, or a service account, and act through that principal.
This PR makes both operations bounded by the caller's own permissions. The rule is a single invariant applied everywhere a role is written or attached to a principal:
Owners and Admins (roles with global access) are exempt and keep their existing delegation abilities.
💡 Proposed Changes
Shared comparator (
backend/api/utils/access/permissions.py)get_role_effective_policyresolves a role's real policy. Managed roles resolve from their template (their DB JSON is empty by design), custom roles from stored JSON, and custom roles never have effective global access.role_grant_violations(actor_role, target_policy)returns the permissions in the target that exceed the actor, as<scope>:<resource>:<action>strings. Missing keys grant nothing, unknown resources and malformed legacy shapes are reported as violations rather than coerced to empty, and a missing actor role fails closed.role_update_grant_violations(actor_role, current_role, new_policy)reports only what an edit adds, so existing roles stay editable.roles_grant_violations/role_assignment_errorapply the same comparison across several actor roles with union semantics, for team-scoped callers.Authoring is bounded by the author
createCustomRole/updateCustomRoleand RESTPOST /v1/roles/andPUT /v1/roles/{id}now reject a payload carrying permissions the caller does not hold, naming each one.select_for_update()insidetransaction.atomic(), so the ceiling is computed against the row being written.Assignment is bounded by the assigner
updateOrganisationMemberRoleand RESTPUT /v1/members/{id}.bulkInviteOrganisationMembersand RESTPOST /v1/members/invites. Bulk invite now validates every invite in the batch before it creates or emails any of them, so a partially rejected batch creates nothing.createServiceAccount/updateServiceAccountand REST create andPUT /v1/service-accounts/{id}.Service account key material is verified
The assignment ceiling is only meaningful if a caller cannot simply act as an existing, stronger service account. Caller-supplied key material is now verified rather than trusted:
enableServiceAccountServerSideKeyManagement, and service account creation when server keys are supplied) and before any mint path uses them (GraphQL, REST, and the AWS and Azure identity exchanges).Managed role change
Managergains app-levelEnvironments: delete(template version 3). Without it the defaultServicerole is not a subset ofManager, and Managers could no longer create service accounts with the stock role.Console
AssignableRoleOptionrenders roles the viewer cannot grant as disabled, with a lock icon and an explanation.isHandledGraphQLErrorhelper, instead of being swallowed by empty catch blocks.Fix picked up along the way
serviceAccountHandlersraised a 500 for the whole organisation if any custom role stored a null or non-dict permissions map, which the validator allows.🖼️ Screenshots or Demo
📝 Release Notes
Role permissions are now bounded by your own role
403 Forbiddenwith an error naming each permission you do not hold.Managed role change
Managerrole now includesdeleteon app environments, in addition toread,createandupdate. This applies to all existing Managers on upgrade. No migration is required.Possible impact on narrow custom roles
Servicerole can no longer assign that role. An Owner or Admin can widen the custom role, or create the service account.Service accounts
createServiceAccountTokenmutation takes a new requiredsignatureargument. The GraphQL API is session authenticated and console-facing. CLI and SDK clients use the REST API and are unaffected.🧪 Testing
Full backend suite and full frontend suite run in the dev containers.
tsc --noEmitclean, ESLint clean.New test files:
backend/tests/graphene/mutations/test_service_account_roles.pyandtest_member_invite_ceiling.pyfor the assignment ceiling.backend/tests/graphene/mutations/test_service_account_token_signature.pyandtest_service_account_ssk_keyring.pyfor signature and keyring binding, using real Ed25519 keys.backend/tests/api/views/test_service_account_token_keyring.pyandbackend/tests/api/utils/identity/test_common.pyfor the REST and identity mint paths.frontend/tests/utils/access/roleGrant.test.tsandfrontend/tests/utils/crypto/serviceAccountTokens.test.ts.Extended:
test_permissions.py,test_access_roles.py,test_roles_api.py,test_members_api.py,test_update_member_role.py,test_service_accounts_api.py,queries/test_service_accounts.py,utils/test_environments.py.Each wired site has a rejection test, an in-ceiling acceptance test, a global-access exemption test, and an unchanged-role test that asserts the comparator is not called. Rejection tests assert the exact violation strings so an earlier guard cannot be what rejects. The exemption tests use a permission the Admin template genuinely lacks, so they fail if the exemption is removed.
Known gaps: the new
403branches in the AWS and Azure identity views have no view-level test, because those modules are rate-limit flaky in the dev container. The underlying refusal is covered by a unit test onmint_service_account_token.🎯 Reviewer Focus
backend/api/utils/access/permissions.pyis the whole comparison model. Everything else calls into it.backend/backend/graphene/mutations/access.py,organisation.py,service_accounts.py, andbackend/api/views/roles.py,members.py,service_accounts.py.backend/api/utils/service_accounts.pyandbackend/api/utils/crypto.pyfor the key binding and signature verification, then the frontend counterpart infrontend/utils/crypto/service-accounts.tsto confirm the signed message is byte identical on both sides.backend/api/utils/access/roles.pyfor theManagertemplate change.➕ Additional Context
rbac-updatesbranch of the docs repo: the managed role tables now match the templates, and the grant rule is documented on the roles, users, organisation, service accounts, and teams pages, and on the roles, members, invites and service accounts API pages.Managerchange takes effect on deploy.✨ How to Test the Changes Locally