Repository navigation
feat: remove the legacy DomainOrg authorization system (PULP-2124) - #1555
Conversation
Reviewer's GuideCompletes the migration to RBAC by deleting the DomainOrg model, authorization class, dual-write and reporting tooling, while preserving domain-owner PyPI content-guard bypasses through the equivalent core.view_content grant and keeping historical migrations self-contained. Sequence diagram for RBAC domain creation and PyPI accesssequenceDiagram
participant Client
participant AccessPolicy
participant DomainCreate
participant Signal
participant RBAC
participant PyPI
participant ContentGuard
Client->>AccessPolicy: has_permission(request, view)
AccessPolicy->>AccessPolicy: set_domain_create_context(request)
Client->>DomainCreate: create domain
DomainCreate->>Signal: post_create_domain(sender, ...)
Signal->>RBAC: _assign_domain_roles(user_or_group, domain)
Signal->>RBAC: _assign_domain_roles(org_group, domain)
Client->>PyPI: SAFE method request
PyPI->>AccessPolicy: has_permission(request, view)
AccessPolicy->>RBAC: user.has_perm(core.view_content, domain)
alt RBAC grant exists
RBAC-->>AccessPolicy: permission granted
AccessPolicy-->>PyPI: allow content-guard bypass
else no RBAC grant
AccessPolicy->>ContentGuard: permit(request)
ContentGuard-->>AccessPolicy: allow or deny
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
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="206" />
<code_context>
+ # service.domain_admin and service.domain_viewer, covering every principal with a role on the
+ # domain.
+ check_domain = domain or Domain.objects.filter(pk=get_domain_pk()).first()
+ if user.is_authenticated and check_domain is not None and user.has_perm("core.view_content", obj=check_domain):
_logger.info(
- "Content-guarded PyPI access GRANTED via DomainOrg: user=%s org_id=%s",
</code_context>
<issue_to_address>
**Read-only members bypass PyPI feature guards**
When an unentitled user belongs to a group granted `core.view_content` on a domain that has a guarded PyPI distribution, the `core.view_content` check accepts a principal with that permission on the domain before `_evaluate_content_guard` runs, so an unentitled member of the lightwell read-only group can fetch guarded PyPI content. The existing read-only-group test expects those requests to remain denied.
Restrict the bypass to the domain-owner principals that previously had a DomainOrg association, rather than every principal with `core.view_content`.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/app/migrations/0024_delete_domainorg.py" line_range="10" />
<code_context>
+ ]
+
+ operations = [
+ migrations.DeleteModel(
+ name="DomainOrg",
+ ),
</code_context>
<issue_to_address>
**Old workers fail after table removal**
When the migration runs while old-version pods or workers are still serving requests, `PulpServiceAccessPolicy`’s DomainOrg-based PyPI access checks query the dropped table, and the old `post_create_domain` worker inserts into it. Those requests raise database errors, so guarded reads fail and domain creation fails or rolls back.
Defer dropping the table until all old-version pods and workers have drained.
Also at `pulp_service/pulp_service/app/migrations/0024_delete_domainorg.py:11`.
</issue_to_address>
### Comment 3
<location path="docs/pypi-yank-monitor.md" line_range="170" />
<code_context>
| `pypi_yank_monitor/{id}/check/` | `POST` | Trigger an on-demand yank check (returns 202 with task) |
| `pypi_yank_monitor/{id}/report/` | `GET` | Retrieve the latest yank report (returns 404 if no report exists) |
-All endpoints require **DomainOrg membership** for the target domain.
+All endpoints require **an RBAC role on the target domain**.
</code_context>
<issue_to_address>
**Monitor access docs overstate role requirement**
When an authenticated caller without a role on the target domain lists monitors, the monitor list policy allows any authenticated principal, so a caller without a role on the target domain can still call `GET pypi_yank_monitor/` and receive a response; the guide incorrectly says every endpoint requires that role.
Clarify that listing is available to authenticated users and document the role checks and scope of each other action.
</issue_to_address>
### Comment 4
<location path="pulp_service/pulp_service/app/models.py" line_range="22" />
<code_context>
_logger = logging.getLogger(__name__)
-class DomainOrg(models.Model):
- """
- One-to-many relationship between org ids and Domains.
</code_context>
<issue_to_address>
**Contributor guidance names removed authorization**
When contributors use the repository’s architecture or agent guidance after this removal, cLAUDE.md and the Pulp developer agent guide still describe DomainOrg and DomainBasedPermission as current, so contributors following them work from an authorization design that no longer exists.
Update the contributor guides to describe the RBAC-based authorization flow and remove references to the deleted model and permission class.
Also at `pulp_service/pulp_service/app/authorization.py:70`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and the 0024 migration permanently deletes the DomainOrg authorization mappings, so reverting the code would not restore the table or its data. Authorization and content-guard bypasses also switch to RBAC grants; if those grants are incomplete or overly broad, access could be denied or exposed across domains with no automatic signal.
Blocking findings: pulp_service/pulp_service/app/access_policy.py:206, pulp_service/pulp_service/app/migrations/0024_delete_domainorg.py:10
CryptoRodeo
changed the title
9c6c6a2 to
fec5715
Compare
|
Thanks for the review. All four findings addressed in the amended commit:
|
|
@dkliban Hey, this should be good for review now |
de7bfc2 to
73ce546
Compare
|
Rebased onto main to get latest changes |
73ce546 to
72c5954
Compare
|
|
72c5954 to
80aac69
Compare
|
Rebased onto main. Lets see if CI passes now. |
|
looks like rebasing helped |
|
nope, same issue.... |
| # held by core.domain_owner / service.domain_admin but not by the view-only roles | ||
| # (core.domain_viewer / service.domain_viewer), so read-only members do not bypass the guard. | ||
| check_domain = domain or Domain.objects.filter(pk=get_domain_pk()).first() | ||
| if user.is_authenticated and check_domain is not None and user.has_perm("core.change_domain", obj=check_domain): |
There was a problem hiding this comment.
The permission checked here (core.change_domain) is inconsistent with the stated intent. The PR description and the test docstrings (e.g. test_domain_member_bypasses_content_guard) both say this bypass checks core.view_content, but the code checks core.change_domain. These are not equivalent — they grant the bypass to different principals.
Looking at the role model:
| Role | Assigned at create | change_domain |
view_content |
|---|---|---|---|
core.domain_owner (object-level) |
yes | ✅ | ❌ |
service.domain_admin (domain-scoped, = all plugin perms incl. core.*) |
yes | ✅ | ✅ |
service.domain_viewer (domain-scoped, = view_* perms) |
no | ❌ | ✅ |
So the comment's factual claim is correct — change_domain is held by owner/admin but not the viewer role. But that means the only case where change_domain and view_content disagree is a read-only domain member (service.domain_viewer): change_domain denies them the bypass, view_content allows them. Every principal created by _assign_domain_roles holds both owner + admin, so the happy-path test passes under either permission and doesn't pin the semantics down.
This needs an explicit decision:
- Owners/writers only bypass the guard → keep
change_domain, and fix the PR description + test docstrings that sayview_content. - Any content reader (incl. read-only members) bypasses → switch to
view_content. Noteview_contentis granted via domain-scoped roles, not object-level on the Domain, sohas_perm("core.view_content", obj=domain)won't resolve the way the current object-levelchange_domaincheck does — you'd want the domain-scoped form (has_domain_perms:core.view_content, as used elsewhere insettings.py).
Either way, worth adding a service.domain_viewer test case (asserted denied or allowed per the decision) so the intended behavior is locked in rather than incidental.
There was a problem hiding this comment.
Good catch on the code/docs mismatch. Decision: kept core.change_domain, fixed the docs.
Rationale: the bypass this replaced (DomainOrg._has_domain_access) granted to the creator, team group, and org members, who under RBAC all hold core.domain_owner + service.domain_admin (both have change_domain). Read-only members (service.domain_viewer) are an RBAC-only concept that never existed under DomainOrg, so excluding them from the bypass is what preserves the old behavior. view_content would have expanded the bypass to read-only members.
Changes pushed:
- Kept
change_domainin the bypass check (no code change). - Fixed the stale
view_contentwording in the commit message,CHANGES/2124.removal, and thetest_domain_member_bypasses_content_guarddocstring to saychange_domain. - Added
test_domain_viewer_does_not_bypass_content_guard: aservice.domain_viewermember (hasview_content, lackschange_domain, no feature) gets 403 on the guarded PyPI simple API, pinning the semantics.
Left the content list/retrieve view_content references (scope_queryset / settings access policies) untouched; those are the separate gating mechanism and are correct.
80aac69 to
61b3be4
Compare
|
Minor test-coverage gap — This is the one content-guard error branch left without a dedicated unit test. The deleted The functional Suggest a one-line sibling mirroring the PermissionError test: def test_content_guard_permit_authentication_error_denies(self, caplog):
# guard.permit() raises AuthenticationError -> _evaluate_content_guard returns False,
# and it is NOT logged as "unexpected error evaluating content guard".Low severity — just restoring parity with the other two error branches. |
RBAC (PulpServiceAccessPolicy) has been the default authorization in production for a while, with DomainOrg kept alongside it only as a dual-write safety net during the switchover. With that proven out, this removes the old system for good: the DomainOrg model and table, the now-dead DomainBasedPermission class (its last consumer, the registry re-root patch, was dropped in pulp#1535), the domain-create dual-write, and the DomainOrg backfill/report tooling and debug endpoints. The one place DomainOrg was still read on the request path, the domain-owner bypass for content-guarded PyPI reads, now checks the equivalent RBAC grant (core.change_domain on the domain) that domain creation already assigns to the creator, team group, and org group, so the behavior is preserved. core.change_domain is held by core.domain_owner / service.domain_admin but not by the view-only roles (core.domain_viewer / service.domain_viewer), so read-only members do not bypass the guard, matching the old DomainOrg behavior. A migration drops the table, and two historical data migrations that imported the model or its backfill helpers are made self-contained so the migration graph still loads after the model is gone. Note: org_id handling in the create-domain flow stays. It feeds the rh-org-<org_id> role grant, which is RBAC, not DomainOrg. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Bryan ramos <bramos@redhat.com>
61b3be4 to
1b7831a
Compare
|
Added |
RBAC (PulpServiceAccessPolicy) has been the default authorization in production for a while, with DomainOrg kept alongside it only as a dual-write safety net during the switchover. With that proven out, this removes the old system for good: the DomainOrg model and table, the now-dead DomainBasedPermission class (its last consumer, the registry re-root patch, was dropped in #1535), the domain-create dual-write, and the DomainOrg backfill/report tooling and debug endpoints.
The one place DomainOrg was still read on the request path, the domain-owner bypass for content-guarded PyPI reads, now checks the equivalent RBAC grant (core.view_content on the domain) that domain creation already assigns, so the behavior is preserved. A migration drops the table, and two historical data migrations that imported the model or its backfill helpers are made self-contained so the migration graph still loads after the model is gone.
Note: org_id handling in the create-domain flow stays. It feeds the rh-org-<org_id> role grant, which is RBAC, not DomainOrg.
Summary by Sourcery
Remove the legacy DomainOrg authorization system and complete the transition to RBAC-based domain access.
Bug Fixes:
Enhancements:
Documentation:
Tests: