IFC-1750 Fix default PC Reviewer role permission set #7038
Conversation
WalkthroughRefactors permission initialization to use a new get_or_create_global_permission helper, adds human-readable permission descriptions, introduces an OBJECTPERMISSION for ProposedChange updates, updates role bindings accordingly, exports the new helper, and adjusts an e2e test to target the first matching UI elements. Changes
Sequence Diagram(s)sequenceDiagram
actor Init as Initialization
participant DB as Database
participant Perm as permissions.globals
Init->>Perm: get_or_create_global_permission(db, EDIT_DEFAULT_BRANCH)
Perm->>DB: Query CoreGlobalPermission by action
alt exists
Perm-->>Init: return existing permission
else
Perm->>DB: Create CoreGlobalPermission(action, ALLOW_ALL, description)
Perm-->>Init: return created permission
end
Init->>DB: Create OBJECTPERMISSION (ProposedChange, UPDATE, ALLOW_ALL)
Init->>DB: Update Role: add [edit_default_branch, review_proposed_change, proposed_change_update]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
gmazoyer
changed the title
github-actions
bot
added
the
group/backend
CodSpeed Performance ReportMerging #7038 will not alter performanceComparing Summary
|
github-actions
bot
added
the
group/frontend
gmazoyer
force-pushed
the
gma-20250814-ifc1750
branch
from
6068602 to
fea2411
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
backend/infrahub/permissions/globals.py (1)
27-45: Add a docstring and make description lookup resilient.
- Missing function docstring; our guidelines require docstrings for all Python functions.
- Accessing GLOBAL_PERMISSION_DESCRIPTION[permission] will raise a KeyError for any enum not present in the map (e.g., OVERRIDE_CONTEXT). Add a safe fallback.
Apply this diff:
async def get_or_create_global_permission(db: InfrahubDatabase, permission: GlobalPermissions) -> CoreGlobalPermission: + """Create or fetch a CoreGlobalPermission for the given GlobalPermissions. + + Ensures idempotency by looking up an existing permission by action and, if absent, + creating it with an ALLOW_ALL decision and a human-readable description. + + Args: + db: Database connection. + permission: Global permission to ensure exists. + + Returns: + The existing or newly created CoreGlobalPermission node. + """ permissions = await NodeManager.query( db=db, schema=CoreGlobalPermission, filters={"action__value": permission.value}, limit=1 ) if permissions: return permissions[0] p = await Node.init(db=db, schema=CoreGlobalPermission) await p.new( db=db, action=permission.value, decision=PermissionDecision.ALLOW_ALL.value, - description=GLOBAL_PERMISSION_DESCRIPTION[permission], + description=GLOBAL_PERMISSION_DESCRIPTION.get( + permission, f"Allow a user to {permission.value.replace('_', ' ')}" + ), ) await p.save(db=db) return p
🧹 Nitpick comments (1)
frontend/app/tests/e2e/role-management/read.spec.ts (1)
28-29: Use exact text matching to avoid accidental partial matches.To ensure we don’t match a superset text (e.g., embedded in a longer string), consider enabling exact matching.
- await expect(page.getByText("Infrahub Users").first()).toBeVisible(); - await expect(page.getByText("global:edit_default_branch:").first()).toBeVisible(); + await expect(page.getByText("Infrahub Users", { exact: true }).first()).toBeVisible(); + await expect(page.getByText("global:edit_default_branch:", { exact: true }).first()).toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/infrahub/core/initialization.py(3 hunks)backend/infrahub/permissions/__init__.py(2 hunks)backend/infrahub/permissions/constants.py(1 hunks)backend/infrahub/permissions/globals.py(2 hunks)frontend/app/tests/e2e/role-management/read.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/permissions/constants.pybackend/infrahub/permissions/__init__.pybackend/infrahub/permissions/globals.pybackend/infrahub/core/initialization.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/permissions/constants.pybackend/infrahub/permissions/__init__.pybackend/infrahub/permissions/globals.pybackend/infrahub/core/initialization.py
🧬 Code Graph Analysis (3)
backend/infrahub/permissions/constants.py (1)
backend/infrahub/core/constants/__init__.py (1)
GlobalPermissions(91-101)
backend/infrahub/permissions/__init__.py (1)
backend/infrahub/permissions/globals.py (2)
define_global_permission_from_branch(18-24)get_or_create_global_permission(27-44)
backend/infrahub/core/initialization.py (4)
backend/infrahub/permissions/globals.py (1)
get_or_create_global_permission(27-44)backend/infrahub/core/constants/__init__.py (3)
GlobalPermissions(91-101)PermissionAction(104-109)PermissionDecision(112-116)backend/infrahub/core/node/standard.py (1)
save(88-94)backend/infrahub/core/protocols.py (1)
CoreAccountRole(255-258)
🔇 Additional comments (5)
frontend/app/tests/e2e/role-management/read.spec.ts (1)
28-29: Targeting the first match reduces test flakiness.Chaining .first() is a pragmatic way to disambiguate multiple matches and should make these assertions more stable.
backend/infrahub/permissions/__init__.py (1)
2-2: Re-export of get_or_create_global_permission looks correct.Import path and all exposure align with current module layout.
Also applies to: 15-15
backend/infrahub/permissions/globals.py (1)
27-45: Ensure uniqueness to prevent duplicate permission creation under concurrency.If two coroutines race, both could create the permission before either sees the other’s write. Please confirm there’s a uniqueness constraint or index on CoreGlobalPermission.action to enforce singletons. If not, consider adding one or handling unique-violation errors during save.
backend/infrahub/core/initialization.py (2)
40-40: Importing get_or_create_global_permission aligns with new helper API.Good step towards idempotent permission initialization.
444-448: Confirmed — GraphQL permission pipeline requires BOTH EDIT_DEFAULT_BRANCH (global) AND the object-level UPDATE for ProposedChange.Short summary: Default-branch and object-permission checks run in the GraphQL permission checker chain, so granting EDIT_DEFAULT_BRANCH alone does not bypass object-level UPDATE checks for ProposedChange.
Key locations (evidence):
- backend/infrahub/core/initialization.py — create_proposed_change_reviewer_role (role created with edit_default_branch_permission + reviewer_permission + proposed_change_update_permission). (lines ~423–448)
- backend/infrahub/graphql/auth/query_permission_checker/default_branch_checker.py — enforces GlobalPermissions.EDIT_DEFAULT_BRANCH on default-branch mutations via active_permissions.raise_for_permission().
- backend/infrahub/graphql/auth/query_permission_checker/object_permission_checker.py — builds required ObjectPermission(s) (including UPDATE for kinds) and calls active_permissions.raise_for_permissions(...).
- backend/infrahub/graphql/auth/query_permission_checker/checker.py — executes the checker chain (sub_checkers) so both default-branch and object checks are applied.
- backend/infrahub/permissions/manager.py — report_object_permission / resolve_object_permission implement object-level decision logic used by raise_for_permissions/has_permission.
Note / caveat:
- Some mutations also perform explicit global-permission checks inside resolvers (e.g., MERGE/REVIEW checks in backend/infrahub/graphql/mutations/proposed_change.py) — these are additional safeguards and do not replace object-level checks.
- If there are non‑GraphQL code paths or external APIs that perform only a global EDIT_DEFAULT_BRANCH check, review those paths separately. I can scan for such code paths on request.
This change adds a couple of permissions to the "Proposed Change Reviewer" role that we ship when Infrahub is started for the first time.
Summary by CodeRabbit
New Features
Refactor
Tests