Skip to content

feat(BA-5681): auto-sync user-scope entries on role assign/unassign#10990

Open
fregataa wants to merge 6 commits intomainfrom
feature/BA-5681-auto-sync-user-scope
Open

feat(BA-5681): auto-sync user-scope entries on role assign/unassign#10990
fregataa wants to merge 6 commits intomainfrom
feature/BA-5681-auto-sync-user-scope

Conversation

@fregataa
Copy link
Copy Markdown
Member

@fregataa fregataa commented Apr 12, 2026

Summary

  • Auto-sync user-scope membership entries in association_scopes_entities when roles are assigned or revoked
  • On assign: INSERT user-scope entries for all scopes bound to the role (ON CONFLICT DO NOTHING)
  • On revoke: DELETE user-scope entries only when no other role covers the same scope (correlated NOT EXISTS check)
  • Scope-type-agnostic — works for project, domain, and any future scope types
  • Integrated into assign_role(), revoke_role(), bulk_assign_role(), and bulk_revoke_role()

Test plan

  • Unit tests: assign creates user-scope entries (single scope, multi-scope, no-scope, idempotent)
  • Unit tests: revoke removes entries (only role, overlapping roles, multi-scope partial, no-scope)
  • Existing role assignment tests still pass

Resolves BA-5681

Copilot AI review requested due to automatic review settings April 12, 2026 13:05
@github-actions github-actions bot added size:XL 500~ LoC comp:manager Related to Manager component labels Apr 12, 2026
fregataa added a commit that referenced this pull request Apr 12, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds automatic synchronization of scope→user membership edges in association_scopes_entities when roles are assigned or revoked, so RBAC scope membership stays consistent with role assignments.

Changes:

  • Add sync_user_scope_on_assign() / sync_user_scope_on_revoke() helpers to maintain scope→user edges based on role-scope bindings.
  • Integrate the sync logic into single and bulk role assign/revoke flows.
  • Add unit tests validating scope entry creation/removal behavior for assign/revoke scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py Adds scope-sync helpers and wires them into assign/revoke (including bulk) operations.
tests/unit/manager/repositories/permission_controller/test_user_scope_sync.py New unit tests covering user-scope sync behavior for role assign/revoke cases.
Comments suppressed due to low confidence (1)

src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py:1202

  • The new user-scope sync behavior is integrated into bulk_assign_role() / bulk_revoke_role(), but the added unit tests only cover the single-user assign_role() / revoke_role() paths. Adding coverage for the bulk paths would help prevent regressions (e.g., partial-success batches, idempotency, and ensuring scope entries are synced for each successful row).
    async def bulk_assign_role(
        self, bulk_creator: BulkCreator[UserRoleRow]
    ) -> BulkCreatorResultWithFailures[UserRoleRow]:
        async with self._db.begin_session() as db_session:
            result = await execute_bulk_creator_partial(db_session, bulk_creator)
            for row in result.successes:
                await sync_user_scope_on_assign(
                    db_session, user_id=row.user_id, role_id=row.role_id
                )
            return result

    async def bulk_revoke_role(
        self, data: BulkUserRoleRevocationInput
    ) -> BulkRoleRevocationResultData:
        successes: list[UserRoleRevocationData] = []
        failures: list[BulkRoleRevocationFailure] = []

        async with self._db.begin_session() as db_session:
            for user_id in data.user_ids:
                try:
                    async with db_session.begin_nested():
                        stmt = (
                            sa.select(UserRoleRow)
                            .where(UserRoleRow.user_id == user_id)
                            .where(UserRoleRow.role_id == data.role_id)
                        )
                        user_role_row = await db_session.scalar(stmt)
                        if user_role_row is None:
                            raise RoleNotAssigned(
                                f"Role {data.role_id} is not assigned to user {user_id}."
                            )
                        user_role_id = user_role_row.id
                        await db_session.delete(user_role_row)
                        await db_session.flush()
                        await sync_user_scope_on_revoke(
                            db_session, user_id=user_id, role_id=data.role_id
                        )
                        successes.append(
                            UserRoleRevocationData(
                                user_role_id=user_role_id,
                                user_id=user_id,
                                role_id=data.role_id,
                            )
                        )
                except Exception as e:
                    log.warning(
                        "Failed to revoke role {} from user {}: {}",
                        data.role_id,
                        user_id,
                        str(e),
                    )
                    failures.append(BulkRoleRevocationFailure(user_id=user_id, message=str(e)))

        return BulkRoleRevocationResultData(successes=successes, failures=failures)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py Outdated
Comment thread src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py Outdated
@fregataa fregataa marked this pull request as draft April 13, 2026 05:13
@fregataa fregataa marked this pull request as draft April 13, 2026 05:13
fregataa added a commit that referenced this pull request Apr 14, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fregataa fregataa force-pushed the feature/BA-5681-auto-sync-user-scope branch from 95e54db to 636fffc Compare April 14, 2026 11:10
fregataa added a commit that referenced this pull request Apr 14, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fregataa fregataa force-pushed the feature/BA-5681-auto-sync-user-scope branch from 636fffc to 2a0026b Compare April 14, 2026 14:55
fregataa added a commit that referenced this pull request Apr 17, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fregataa fregataa force-pushed the feature/BA-5681-auto-sync-user-scope branch from 2a0026b to 547552c Compare April 17, 2026 19:12
fregataa added a commit that referenced this pull request Apr 18, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fregataa added a commit that referenced this pull request Apr 18, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fregataa added a commit that referenced this pull request Apr 18, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fregataa added a commit that referenced this pull request Apr 18, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fregataa added a commit that referenced this pull request Apr 18, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fregataa fregataa force-pushed the feature/BA-5681-auto-sync-user-scope branch from d4450c5 to e3b9bd1 Compare April 18, 2026 10:55
@fregataa fregataa added this to the 26.5 milestone Apr 18, 2026
@fregataa fregataa marked this pull request as ready for review April 18, 2026 19:49
@fregataa fregataa requested review from a team and removed request for a team April 18, 2026 19:50
@fregataa fregataa marked this pull request as draft April 19, 2026 04:18
When a role is assigned or revoked, automatically sync the user's
membership entries in association_scopes_entities based on the role's
bound scopes. This ensures the scope chain CTE can discover user-scope
paths for permission checks.

- assign: INSERT ON CONFLICT DO NOTHING (idempotent)
- revoke: DELETE only when no other role covers the same scope
- Works for all scope types (project, domain, etc.)
- Supports single and bulk assign/revoke operations

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fregataa fregataa force-pushed the feature/BA-5681-auto-sync-user-scope branch from e3b9bd1 to 3fa81e0 Compare April 19, 2026 05:27
@fregataa fregataa marked this pull request as ready for review April 19, 2026 05:29
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread src/ai/backend/manager/repositories/permission_controller/db_source/db_source.py Outdated
fregataa and others added 4 commits April 19, 2026 22:16
entity_id is a String(64) column that could theoretically hold
non-UUID values. Log a warning and skip the row instead of crashing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t queries

- Remove UserScopeBinding, _get_role_bound_scopes, _build_bindings
- _sync_user_scopes_on_assign: single INSERT...SELECT joining user_roles
  to resolve scopes at execution time (no pre-fetch TOCTOU gap)
- _sync_user_scopes_on_revoke: single DELETE with NOT EXISTS, no longer
  scoped to a specific role — cleans up any orphaned user-scope entries
- Both methods accept user_ids only (role_id removed)
- bulk_revoke_role calls sync once after loop instead of per-user
- Add TODO comments for association_groups_users migration deprecation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ssign

Filter user_ids directly on user_roles JOIN instead of using a
separate unnest CROSS JOIN. The user_roles table already provides
the user_id column, making the subquery unnecessary.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:manager Related to Manager component size:XL 500~ LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants