Let domain members view all content via core.view_content - #1465
Conversation
Reviewer's GuideThe PR adds a centralized access-policy overlay for all BaseContentViewSet subclasses: list and retrieve are gated by domain-scoped core.view_content and repository queryset scoping is removed, while writes, creation hooks, wildcard grants, non-content policies, and the original policy remain intact. The behavior is configured and tested but only activates when the RBAC permission class is enabled. Sequence diagram for RBAC content visibilitysequenceDiagram
participant Client
participant ContentViewSet as BaseContentViewSet
participant Policy as PulpServiceAccessPolicy
participant Settings as ACCESS_POLICIES
participant Domain as DomainPermissions
Client->>ContentViewSet: list or retrieve
ContentViewSet->>Policy: get_access_policy(view)
Policy->>Settings: read ACCESS_POLICIES[content]
Policy-->>ContentViewSet: read policy with queryset_scoping=None
ContentViewSet->>Domain: has_domain_perms:core.view_content
alt permission granted
Domain-->>ContentViewSet: allow
ContentViewSet-->>Client: all content in domain
else permission denied
Domain-->>ContentViewSet: deny
ContentViewSet-->>Client: 403
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/access_policy.py" line_range="82-99" />
<code_context>
+ read_actions = {"list", "retrieve"}
+ merged = deepcopy(policy)
+
+ kept_statements = []
+ for statement in merged.get("statements", []):
+ if statement.get("effect") != "allow":
+ kept_statements.append(statement)
+ continue
+ action = statement.get("action")
</code_context>
<issue_to_address>
**issue (broader_impact):** The merge removes every non-wildcard `allow` statement containing `list` or `retrieve`, then appends a single content read statement; this discards existing read grants with distinct conditions such as object-level permissions, role-specific principals, or special content access rules rather than preserving them. Users who previously qualified through those grants lose access unless they also have the domain-scoped `core.view_content` permission.
**Triggers:** When a content viewset has an existing list/retrieve allow statement whose access condition is broader or different from `has_domain_perms:core.view_content`.
**Suggested fix:** Preserve existing read grants that must remain valid, or explicitly combine the new domain permission with the existing read conditions instead of deleting them.
```suggestion
kept_statements = merged.get("statements", [])
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and when RBAC is active, this changes the authorization policy for every content viewset: a domain member with core.view_content can list and retrieve all content in the domain, without repository scoping. Reverting would stop future access but could not undo content already exposed, and the access decision is wrong for everyone immediately if the policy is wrong.
Blocking findings: pulp_service/pulp_service/app/access_policy.py:99
0380d8a to
68ca218
Compare
Under RBAC, content is scoped by repository membership, so content a user pushed but hasn't added to a repo (orphan content) is invisible to them -- read-after-push 404s on the list-all content endpoint. Make PulpServiceAccessPolicy inherit from pulpcore's AccessPolicyFromSettings so each viewset's policy is read from settings.ACCESS_POLICIES[<urlpattern>] (falling back to the viewset's DEFAULT_ACCESS_POLICY) instead of the DB. The "content" entry overrides pulpcore's ListContentViewSet to gate the list action on the domain-scoped core.view_content permission and drop repository-based queryset scoping, so a domain member can list all content in their domain. This replaces the earlier custom get_access_policy override with configuration only. The change is staged: DomainBasedPermission is still the default permission class, so this only takes effect once RBAC is re-enabled. Signed-off-by: Bryan ramos <[email protected]> Co-Authored-By: Claude Opus 4.8 <[email protected]>
68ca218 to
68e0700
Compare
Under RBAC, content is scoped by repository membership, so content a user pushed but hasn't added to a repo (orphan content) is invisible to them, read-after-push 404s.
Add an access-policy override so that, for any content viewset, list/retrieve is gated on the domain-scoped
core.view_contentpermission (already granted to domain roles) and repository-based queryset scoping is dropped. A domain member can then see all content in their domain. Write statements and creation_hooks are preserved, so uploads are unaffected, and keying onBaseContentViewSetcovers every content type in one place.The change is staged:
DomainBasedPermissionis still the default permission class, so this only takes effect once RBAC is re-enabled.Summary by Sourcery
Enable domain-scoped content visibility through RBAC while preserving existing write and staged-permission behavior.
New Features:
core.view_contentpermission to list all content in their domain, including orphan content.Bug Fixes:
Enhancements:
Tests: