feat(BA-2937): Migrate VFolder invitations to RBAC DB #7128
Conversation
github-actions
bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR implements a database migration to migrate VFolder invitations from the legacy system to the new RBAC (Role-Based Access Control) permission groups system. The migration aims to preserve existing vfolder access permissions by creating corresponding permission groups in the RBAC database.
Key Changes:
- Adds Alembic migration script to query existing vfolder_permissions and create permission groups
- Implements temporary unique constraint management during migration to prevent duplicates
- Creates USER-scoped permission groups for users who have vfolder permissions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| input = cls._invitiee_user_to_permission_group_input( | ||
| role_id=row.role_id, | ||
| user_id=row.user_id, | ||
| ) | ||
| permission_group_inputs.append(input) |
There was a problem hiding this comment.
The variable name input shadows the built-in Python function. Consider using a more descriptive name like permission_input or pg_input to avoid shadowing the built-in.
| input = cls._invitiee_user_to_permission_group_input( | |
| role_id=row.role_id, | |
| user_id=row.user_id, | |
| ) | |
| permission_group_inputs.append(input) | |
| permission_group_input = cls._invitiee_user_to_permission_group_input( | |
| role_id=row.role_id, | |
| user_id=row.user_id, | |
| ) | |
| permission_group_inputs.append(permission_group_input) |
| stmt = ( | ||
| sa.select( | ||
| vfolder_permissions_table.c.user.label("user_id"), | ||
| roles_table.c.id.label("role_id"), | ||
| ) | ||
| .select_from( | ||
| sa.join( | ||
| vfolder_permissions_table, | ||
| users_table, | ||
| vfolder_permissions_table.c.user == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| user_roles_table, | ||
| user_roles_table.c.user_id == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| roles_table, | ||
| roles_table.c.id == user_roles_table.c.role_id, | ||
| ) | ||
| ) | ||
| .where(roles_table.c.source == RoleSource.SYSTEM) | ||
| .offset(offset) | ||
| .limit(page_size) | ||
| ) | ||
| return db_conn.execute(stmt).all() |
There was a problem hiding this comment.
The vfolder_permissions table includes a permission column that stores different permission levels (READ_ONLY, READ_WRITE, RW_DELETE), but this migration ignores these permission levels when creating permission groups. All users with vfolder permissions will receive the same permission group regardless of their original permission level. This loses important access control information. The migration should either preserve the permission levels or document why they are being discarded.
| @classmethod | ||
| def _invitiee_user_to_permission_group_input( | ||
| cls, | ||
| role_id: uuid.UUID, | ||
| user_id: uuid.UUID, | ||
| ) -> PermissionGroupCreateInput: | ||
| permission_group_input = PermissionGroupCreateInput( | ||
| role_id=role_id, | ||
| scope_type=ScopeType.USER, | ||
| scope_id=str(user_id), | ||
| ) | ||
| return permission_group_input |
There was a problem hiding this comment.
The _invitiee_user_to_permission_group_input method name suggests it's converting invitee user data to permission group input, but there's no documentation explaining what this conversion does or why it's creating a USER-scoped permission group. Adding a docstring would help clarify the intended behavior and migration strategy.
| stmt = ( | ||
| sa.select( | ||
| vfolder_permissions_table.c.user.label("user_id"), | ||
| roles_table.c.id.label("role_id"), | ||
| ) | ||
| .select_from( | ||
| sa.join( | ||
| vfolder_permissions_table, | ||
| users_table, | ||
| vfolder_permissions_table.c.user == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| user_roles_table, | ||
| user_roles_table.c.user_id == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| roles_table, | ||
| roles_table.c.id == user_roles_table.c.role_id, | ||
| ) | ||
| ) | ||
| .where(roles_table.c.source == RoleSource.SYSTEM) | ||
| .offset(offset) | ||
| .limit(page_size) | ||
| ) | ||
| return db_conn.execute(stmt).all() |
There was a problem hiding this comment.
The query selects users from vfolder_permissions and joins with their system roles, but it doesn't include the vfolder column from vfolder_permissions. This means the migration creates permission groups at the USER scope level regardless of which vfolder the permission was for. This appears to be granting users a role-based permission to all vfolders, rather than preserving the specific vfolder invitations. If the intent is to migrate specific vfolder invitations to permission groups, the query should include the vfolder ID and create permission groups or object permissions that are scoped to specific vfolders.
| PermissionCreator.add_unique_constraint_to_permission_groups_role_id_scope_id() | ||
| conn = op.get_bind() | ||
| PermissionCreator.add_vfolder_permissions_as_permission_groups(conn) | ||
| PermissionCreator.drop_unique_constraint_from_permission_groups_role_id_scope_id() |
There was a problem hiding this comment.
The unique constraint on permission_groups is added before inserting data and then dropped afterward. This pattern is unusual - typically, a unique constraint is either permanently present or not. If the constraint is needed to prevent duplicate insertions during migration, the insert_skip_on_conflict function already handles conflicts. If duplicate permission groups should be prevented permanently, the constraint should remain. The current approach of adding and then removing the constraint suggests unclear intent and could lead to duplicate permission groups being created after the migration completes.
| PermissionCreator.drop_unique_constraint_from_permission_groups_role_id_scope_id() |
| stmt = ( | ||
| sa.select( | ||
| vfolder_permissions_table.c.user.label("user_id"), | ||
| roles_table.c.id.label("role_id"), | ||
| ) | ||
| .select_from( | ||
| sa.join( | ||
| vfolder_permissions_table, | ||
| users_table, | ||
| vfolder_permissions_table.c.user == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| user_roles_table, | ||
| user_roles_table.c.user_id == users_table.c.uuid, | ||
| ) | ||
| .join( | ||
| roles_table, | ||
| roles_table.c.id == user_roles_table.c.role_id, | ||
| ) | ||
| ) | ||
| .where(roles_table.c.source == RoleSource.SYSTEM) | ||
| .offset(offset) | ||
| .limit(page_size) | ||
| ) | ||
| return db_conn.execute(stmt).all() |
There was a problem hiding this comment.
The query doesn't filter or exclude any rows, which means it will attempt to create permission groups for all users who have vfolder permissions. This includes potentially creating duplicate permission groups for the same user if they have permissions on multiple vfolders. While insert_skip_on_conflict handles conflicts, the migration may still create multiple permission groups with identical role_id, scope_type, and scope_id if a user has permissions on multiple vfolders. Consider adding a DISTINCT clause or grouping by (user_id, role_id) to prevent processing the same user-role combination multiple times.
|
|
||
|
|
||
| def downgrade() -> None: | ||
| pass |
There was a problem hiding this comment.
The downgrade() function is empty, which means this migration cannot be rolled back. If the migration needs to be reversed, all permission groups created by this migration would remain in the database. Consider implementing a downgrade that either removes the migrated permission groups or documents why rollback is not supported.
| pass | |
| """ | |
| Remove permission groups created by this migration. | |
| """ | |
| conn = op.get_bind() | |
| permission_groups_table = get_permission_groups_table() | |
| # Delete permission groups with scope_type=USER that were created for vfolder permissions | |
| delete_stmt = ( | |
| permission_groups_table.delete() | |
| .where(permission_groups_table.c.scope_type == ScopeType.USER) | |
| ) | |
| conn.execute(delete_stmt) |
| PermissionCreator.drop_unique_constraint_from_permission_groups_role_id_scope_id() | ||
|
|
||
|
|
||
| def downgrade() -> None: |
There was a problem hiding this comment.
Is this change an irreversible migration?
There was a problem hiding this comment.
If that’s the case, the PR prefix should be marked as ‘breaking’, or the PR should more explicitly indicate that it is non-reversible.
There was a problem hiding this comment.
it is not breaking because the RBAC DB is not used in any API yet. This is a preparation step for RBAC implementation
| @classmethod | ||
| def add_unique_constraint_to_permission_groups_role_id_scope_id(cls) -> None: | ||
| permission_groups_table = get_permission_groups_table() | ||
| op.create_unique_constraint( | ||
| "uq_permission_groups_role_id_scope_id", | ||
| permission_groups_table.name, | ||
| ["role_id", "scope_id", "scope_type"], | ||
| ) | ||
|
|
||
| @classmethod | ||
| def drop_unique_constraint_from_permission_groups_role_id_scope_id(cls) -> None: | ||
| permission_groups_table = get_permission_groups_table() | ||
| op.drop_constraint( | ||
| "uq_permission_groups_role_id_scope_id", | ||
| permission_groups_table.name, | ||
| type_="unique", | ||
| ) |
There was a problem hiding this comment.
In my personal opinion, having these as separate methods actually hurts readability. The methods are very simple, but their names are too long.
resolves #6609 (BA-2937)
Checklist: (if applicable)
ai.backend.testdocsdirectory