feat(BA-6886): register APP_CONFIG_FRAGMENT as an RBAC resource type#12856
Open
jopemachine wants to merge 4 commits into
Open
feat(BA-6886): register APP_CONFIG_FRAGMENT as an RBAC resource type#12856jopemachine wants to merge 4 commits into
jopemachine wants to merge 4 commits into
Conversation
Add APP_CONFIG_FRAGMENT to EntityType._resource_types() and to the RBAC migration enum, and backfill its write permissions onto existing write-capable roles (a user's own user-scope role and domain admins; member roles excluded). New roles get these permissions at role creation, so the migration only covers roles that predate the entity type (BEP-1052). Split out of BA-6859 as its base: registration and the backfill are independent of the per-fragment scope binding. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Registers app config fragments as RBAC resources and backfills permissions for existing roles.
Changes:
- Extends resource type registries and tests.
- Adds an Alembic permission backfill.
- Adds a changelog fragment.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/unit/manager/rbac/test_permission_types.py |
Updates resource-type expectations. |
tests/unit/manager/actions/test_action_types.py |
Updates action type tests. |
src/ai/backend/manager/models/rbac_models/migration/enums.py |
Synchronizes migration enums. |
src/ai/backend/manager/models/alembic/versions/b3d9f7a2c184_add_app_config_fragment_permissions_to_rbac.py |
Backfills RBAC permissions. |
src/ai/backend/common/data/permission/types.py |
Registers the resource type. |
changes/12856.feature.md |
Documents the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Jul 15, 2026
jopemachine
added a commit
that referenced
this pull request
Jul 15, 2026
The resource-type registration and the permission backfill migration moved to BA-6886 (#12856), so this PR's fragment no longer claims them. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
fregataa
reviewed
Jul 15, 2026
Comment on lines
+91
to
+95
| db_conn = op.get_bind() | ||
| db_conn.execute( | ||
| sa.text("DELETE FROM permissions WHERE entity_type = :entity_type"), | ||
| {"entity_type": EntityType.APP_CONFIG_FRAGMENT.value}, | ||
| ) |
Member
There was a problem hiding this comment.
This is dangerous, let's not delete during downgrade migration
Member
Author
There was a problem hiding this comment.
I see in other PRs that added permission records that those records were removed during the downgrade. Are you suggesting that we leave downgrade as pass here instead?
Is there a particular reason for that?
…role name The backfill selected owner roles negatively, via `NOT (role_name LIKE '%member')`. Role names carry no enforced convention — `CreateRoleInput.name` is a free string and the caller picks the scope — so any domain-scoped role not named `%member` matched. The `domain-viewer` preset (read-only, VFOLDER and SESSION READ) is exactly that shape and would have received `hard-delete` and `grant:all` on fragments across the whole domain. Those grant rows are not inert: the live check matches on `permissions.operation`, and the bitmask is the half that is currently unread. Select admins positively instead, by the capability each domain admin role provably holds: * domain — require `domain_admin_page` at the same domain. Every domain admin role holds it under both naming schemes by the time this runs (3b6297b1bd75, f2b9a4c7e103/a3c1d8e5b294 are all ancestors), so the `roles` join drops out entirely. * user — kept broad. A user-scope grant only reaches that user's own scope, and writing an own-scope fragment is deliberately not admin-only. `public` (ScopeType.GLOBAL) is granted to nobody; superadmin reaches it via the `is_superadmin` short-circuit in the RBAC validators, which needs no rows. `role_superadmin` drops out for the same reason, matching 3b6297b1bd75. Also make `downgrade` a no-op, as f2b9a4c7e103 does: a DELETE by entity_type cannot tell backfilled rows from those role creation grants natively. Verified against a local DB per models/alembic/AGENTS.md: domain admin roles under both naming schemes receive the grant; the read-only custom role, member roles, and `role_superadmin` receive nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Drop the revision-by-revision argument for why every domain admin role holds ``domain_admin_page``, the ``permission`` bitmask provenance, and the bullet structure. Keep only what the SQL cannot show: why the bits are inlined, why admins are selected by capability instead of role name, and why ``role_superadmin`` drops out. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
jopemachine
added a commit
that referenced
this pull request
Jul 15, 2026
The resource-type registration and the permission backfill migration moved to BA-6886 (#12856), so this PR's fragment no longer claims them. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
jopemachine
added a commit
that referenced
this pull request
Jul 15, 2026
The resource-type registration and the permission backfill migration moved to BA-6886 (#12856), so this PR's fragment no longer claims them. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.
Summary
Register
APP_CONFIG_FRAGMENTas an RBAC resource type and backfill its write permissions onto existing write-capable roles, so later PRs in the stack can authorize fragment writes through RBAC (BEP-1052).Split out of #12826 as its base: the entity-type registration and the permission backfill are independent of the per-fragment scope binding, and land together because a backfilled permission is meaningless unless the type is a resource type.
Changes
EntityType._resource_types()— addAPP_CONFIG_FRAGMENT, so new roles receive its permissions at role creation (10 → 11 types)rbac_models/migration/enums.py— same registration for the migration-local enumb3d9f7a2c184— backfill the permissions onto roles that predate the entity typeWhat the migration backfills
Only
permissionsrows, and only where a fragment can actually live:useranddomainonly — a fragment lives at no other scope.publicmaps toScopeType.GLOBALand is granted to nobody: superadmin reaches it through theis_superadminshort-circuit in the RBAC validators, which needs no rowsuser-scope role, anddomainadmins selected by capability — the role must holddomain_admin_pageat that same domain (see below)permissioncarries the matching bitmask (c6648c039bd4shape), grant operations store 0association_scopes_entities) — they are per-fragment and written at fragment-creation time (#12826). The feature is unreleased, so no fragments and no associations exist yetON CONFLICT DO NOTHING, so re-running grants nothing twice.downgradeis a no-op, followingf2b9a4c7e103: aDELETEbyentity_typecannot tell backfilled rows from the ones role creation now grants natively, so it would strip permissions that roles created since this revision legitimately hold.Selecting admins by capability, not by name
The sibling backfills (
f1a2b3c4d5e6,30c8308738ee, …) select owner roles negatively, viaNOT (role_name LIKE '%member'). That is unsafe: role names carry no enforced convention —CreateRoleInput.nameis a free string and the caller picks the scope — so any domain-scoped role not named%membermatches. Thedomain-viewerpreset intest_create_preset_roles.py(read-only:VFOLDER:READ,SESSION:READ) is exactly that shape and would have receivedhard-deleteandgrant:allon fragments across the whole domain.Those grant rows are not inert: the live check matches on
permissions.operation(db_source.py), and the bitmask is the half that is currently unread.So this migration selects admins positively instead:
domain— every domain admin role holdsdomain_admin_pageat its own domain by the time this runs, under both naming schemes:3b6297b1bd75coversrole_domain_%_admin,f2b9a4c7e103/a3c1d8e5b294coverdomain-%-admin, and all three are ancestors of this revision. So no name matching is needed and therolesjoin drops out entirely. Matchingscope_idtoo keeps an admin of domain A from being granted on domain B.user— kept broad. A user-scope grant only ever reaches that user's own scope, and writing an own-scope fragment is deliberately not admin-only (seeAppConfigScopeType.to_rbac_scope_type).role_superadminholds domain-scoped rows but nodomain_admin_page, so it drops out. That is correct and matches3b6297b1bd75: superadmin authority comes from theis_superadminshort-circuit, never from these rows.Verified against a local DB (per
models/alembic/AGENTS.md, data migrations must be checked against a real DB):domain-testdom-admin(runtime naming)role_domain_default_admin(legacy naming)domain-auditors-readonly(read-only custom, the escalation)role_domain_default_memberrole_superadminBackport
Not needed for 26.4 — the feature does not exist there:
main26.4app_config_fragmentpathscreate_app_config_fragments_tablemigrationEntityType.APP_CONFIG_FRAGMENT26.4's
84d5c6daf8ccsays it directly: the replacement tables "are introduced in a follow-up migration on top of this one". Backporting would fail to importEntityType.APP_CONFIG_FRAGMENT, and would grant permissions on a table that does not exist.📚 Stacked PRs
Part of the AppConfigFragment / AppConfig stack under BEP-1052 (epic BA-5781). Merge in order:
feat(BA-6552): app_config_fragments DB model and Alembic migrationfeat(BA-6553): repository layerrefactor(BA-6619): consolidate AppConfigScopeType into common.datarefactor(BA-6620): ExistsQuerier + AppConfigAllowList.existsfeat(BA-6554): AppConfigFragment service layerfeat(BA-6702): move fragment rank to the allow list with scope defaultsfeat(BA-6701): expose allow-list rank on the v2 API surfacefeat(BA-6704): cascade app config subtree deletion from the definitionfeat(BA-6626): app_config_fragment bulk repository layerfeat(BA-6618): app_config_fragment bulk CRUD service layerfeat(BA-6810): AppConfigFragment visible-fragments query (repository layer)feat(BA-6886): registerAPP_CONFIG_FRAGMENTas an RBAC resource type + permission backfill ← you are herefeat(BA-6859): bind fragments to their RBAC scope on write (repository layer)fix(BA-6872): seedAPP_CONFIG_FRAGMENTpermissions into the RBAC role fixturechore(BA-6873): drop the dead APP_CONFIG RBAC entity typefeat(BA-6555): AppConfig merge engine + resolve service (service layer)feat(BA-6556): AppConfig REST v2 APIfeat(BA-6860): fragment write API routes + bulk write authzJira: https://lablup.atlassian.net/browse/BA-6886
🤖 Generated with Claude Code