feat: gate PyPI access via distribution content guards instead of hardcoded feature check#1316
Open
CryptoRodeo wants to merge 5 commits into
Open
feat: gate PyPI access via distribution content guards instead of hardcoded feature check#1316CryptoRodeo wants to merge 5 commits into
CryptoRodeo wants to merge 5 commits into
Conversation
Contributor
Reviewer's GuideRefactors DomainBasedPermission’s PyPI SAFE_METHOD access logic to use each distribution’s content_guard (with DomainOrg bypass and strict error/logging behavior) instead of a hardcoded lightwell-network feature check, and updates unit/functional tests and fixtures to match the new content-guard-driven, domain-agnostic model. Sequence diagram for content-guard-based PyPI SAFE_METHOD accesssequenceDiagram
actor Client
participant PyPIView as PyPIView
participant DomainBasedPermission as DomainBasedPermission
participant Distribution as Distribution
participant Guard as ContentGuard
Client->>PyPIView: SAFE_METHOD request
PyPIView->>DomainBasedPermission: has_permission(request, view)
DomainBasedPermission->>DomainBasedPermission: _check_pypi_safe_method_access(request, view, domain)
alt view is not PyPIMixin
DomainBasedPermission-->>DomainBasedPermission: return None
else view is PyPIMixin
DomainBasedPermission->>PyPIView: access distribution
alt Http404
DomainBasedPermission-->>DomainBasedPermission: return True
else other exception
DomainBasedPermission->>DomainBasedPermission: log exception
DomainBasedPermission-->>DomainBasedPermission: return False
else distribution resolved
PyPIView-->>DomainBasedPermission: distribution
DomainBasedPermission->>Distribution: read content_guard
alt no content_guard
DomainBasedPermission-->>DomainBasedPermission: return True
else has content_guard
DomainBasedPermission->>DomainBasedPermission: get_decoded_identity_header(request)
DomainBasedPermission->>DomainBasedPermission: get_org_id(decoded_header)
DomainBasedPermission->>DomainBasedPermission: _has_domain_access(domain_pk, org_id, user)
alt DomainOrg match
DomainBasedPermission->>DomainBasedPermission: log GRANTED via DomainOrg
DomainBasedPermission-->>DomainBasedPermission: return True
else no DomainOrg match
DomainBasedPermission->>Guard: cast()
alt cast error
DomainBasedPermission->>DomainBasedPermission: log exception
DomainBasedPermission-->>DomainBasedPermission: return False
else cast ok
Guard-->>DomainBasedPermission: casted_guard
DomainBasedPermission->>Guard: permit(request)
alt PermissionError
DomainBasedPermission->>DomainBasedPermission: log DENIED via content guard
DomainBasedPermission-->>DomainBasedPermission: return False
else other exception
DomainBasedPermission->>DomainBasedPermission: log exception
DomainBasedPermission-->>DomainBasedPermission: return False
else permit ok
DomainBasedPermission->>DomainBasedPermission: log GRANTED via content guard
DomainBasedPermission-->>DomainBasedPermission: return True
end
end
end
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Architectural Review FindingsM1. Bare
|
…dcoded feature check DomainBasedPermission no longer hardcodes the "lightwell" domain name or the "lightwell-network" feature. Instead, _check_pypi_safe_method_access reads the distribution's content_guard: if one is configured, it delegates to guard.cast().permit(request). if none, SAFE_METHOD access is open (preserving the existing default). DomainOrg owners still bypass the guard. This makes PyPI access control domain-name-agnostic, any distribution can be protected by attaching a content guard, with no code changes needed. Signed-off-by: Bryan Ramos <bramos@redhat.com>
CryptoRodeo
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the logging-focused tests you switched from
any(...)toall(...)when asserting oncaplog.records(e.g.assert all("GRANTED via DomainOrg" in record.message ... for record in caplog.records)), which makes them brittle if any unrelated log lines are present; consider reverting toany(...)or scopingcaplogwith a stricter logger/level filter to avoid false negatives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the logging-focused tests you switched from `any(...)` to `all(...)` when asserting on `caplog.records` (e.g. `assert all("GRANTED via DomainOrg" in record.message ... for record in caplog.records)`), which makes them brittle if any unrelated log lines are present; consider reverting to `any(...)` or scoping `caplog` with a stricter logger/level filter to avoid false negatives.
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/authorization.py" line_range="100-82" />
<code_context>
- same cache/lookup mechanism as FeatureContentGuard.
- """
- guard = FeatureContentGuard(features=[LIGHTWELL_NETWORK_FEATURE], pulp_domain=None)
- try:
- return guard.check_feature(org_id)
- except PermissionError:
- return False
-
def _has_lightwell_readonly_group_access(self, user):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wrap `casted_guard.permit(request)` in a broader exception handler to avoid unexpected 500s.
Right now only `PermissionError` from `casted_guard.permit(request)` is handled; any other exception will bubble up as a 500 instead of failing closed like the other guard/distribution paths. Consider wrapping `permit` in a broad `try/except Exception` that logs and returns `False`, with a specific `except PermissionError` for expected denials, so unexpected guard errors don’t turn into user-visible 500s.
Suggested implementation:
```python
casted_guard = typing.cast(PulpDomainGuard, guard)
try:
return casted_guard.permit(request)
except PermissionError:
# Expected denial from the guard; fail closed
return False
except Exception:
# Unexpected guard error; log and fail closed rather than raising a 500
logger.exception("Unexpected error while evaluating guard permit", extra={"view": getattr(view, "__class__", type(view)).__name__})
return False
```
If a `logger` is not already defined in this module, you should add at the top of `authorization.py`:
- `import logging`
- `logger = logging.getLogger(__name__)`
Also, if the actual guard type or variable names differ (e.g. `casted_guard` or `PulpDomainGuard` are named differently in your code), adjust the SEARCH/REPLACE block accordingly so it matches the existing `try/except PermissionError` around `permit(request)`.
</issue_to_address>
### Comment 2
<location path="pulp_service/pulp_service/tests/unit/test_domain_based_permission.py" line_range="285" />
<code_context>
- assert permission.has_permission(request, view) is False
-
- assert any("DENIED: no org_id" in record.message for record in caplog.records)
+ assert all("GRANTED via DomainOrg" in record.message and "12345" in record.message for record in caplog.records)
- @patch("pulp_service.app.authorization.DomainBasedPermission._has_lightwell_network_feature")
</code_context>
<issue_to_address>
**suggestion (testing):** Using `all(...)` for log assertions makes the test brittle; consider `any(...)` instead.
This asserts that every log record contains both strings, which is fragile if other messages are logged on the same logger. Using `any(...)` keeps the test focused on verifying that at least one matching log entry is produced, e.g.:
```python
assert any(
"GRANTED via DomainOrg" in record.message and "12345" in record.message
for record in caplog.records
)
```
Please apply the same pattern to the other content-guard logging tests as well.
Suggested implementation:
```python
assert any(
"GRANTED via DomainOrg" in record.message and "12345" in record.message
for record in caplog.records
)
```
To fully apply your comment, similar changes should be made to any other tests in `test_domain_based_permission.py` that:
- Use `all(...)` on `caplog.records` to assert log contents for content-guard checks.
Those occurrences should be updated to use the same `any(...)` pattern, e.g.:
```python
assert any(
"EXPECTED MESSAGE FRAGMENT" in record.message and "OTHER_FRAGMENT" in record.message
for record in caplog.records
)
```
This will keep all logging-related tests focused on the existence of at least one matching log entry instead of requiring every log entry to match.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Catch unexpected exceptions from casted_guard.permit() (not just PermissionError) to prevent 500s — log and fail closed instead. Addresses Sourcery review comment on PR pulp#1316. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ertions Changed 5 assertions in test_domain_based_permission.py to use any() instead of all() for checking log records. Using all() is brittle: if any unrelated log line is present, the assertion fails even though the expected log was emitted. Using any() correctly asserts that at least one matching record exists, regardless of other log lines. Fixes review feedback from PR pulp#1316. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_check_pypi_safe_method_access, the log messages use the stringPyPiwhile the rest of the code and tests refer toPyPI; consider standardizing the capitalization to avoid confusion when searching logs. - Some new tests (e.g.
test_anonymous_no_header_with_guard_denied) describe header-driven behavior that isn’t actually encoded in the_make_content_guardhelper (which unconditionally raisesPermissionErrorwhenpermits=False); updating the docstrings or making the helper reflect the described condition would make the tests clearer to future readers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_check_pypi_safe_method_access`, the log messages use the string `PyPi` while the rest of the code and tests refer to `PyPI`; consider standardizing the capitalization to avoid confusion when searching logs.
- Some new tests (e.g. `test_anonymous_no_header_with_guard_denied`) describe header-driven behavior that isn’t actually encoded in the `_make_content_guard` helper (which unconditionally raises `PermissionError` when `permits=False`); updating the docstrings or making the helper reflect the described condition would make the tests clearer to future readers.
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/authorization.py" line_range="102-111" />
<code_context>
+
+ if user.is_authenticated and self._has_domain_access(domain_pk, org_id, user):
+ _logger.info(
+ "Content-guarded PyPi access GRANTED via DomainOrg: user=%s org_id=%s",
+ user,
+ org_id,
+ )
+ return True
+
+ try:
+ casted_guard = guard.cast()
+ except Exception:
+ _logger.exception("Failed to resolve content guard type for distribution")
+ return False
+
+ try:
+ casted_guard.permit(request)
+ _logger.info(
+ "Content-guarded PyPi access GRANTED via content guard: org_id=%s user=%s",
+ org_id,
+ user,
+ )
+ return True
+ except PermissionError:
+ _logger.info(
+ "Content-guarded PyPi access DENIED via content guard: org_id=%s user=%s",
+ org_id,
+ user,
</code_context>
<issue_to_address>
**nitpick (typo):** Minor typo/inconsistency in log message: use "PyPI" instead of "PyPi".
These log lines currently use "PyPi". Please change them to "PyPI" to match the canonical spelling and keep log search/aggregation consistent with the rest of the ecosystem.
Suggested implementation:
```python
_logger.info(
"Content-guarded PyPI access GRANTED via DomainOrg: user=%s org_id=%s",
user,
org_id,
)
```
```python
_logger.info(
"Content-guarded PyPI access GRANTED via content guard: org_id=%s user=%s",
org_id,
user,
)
```
```python
_logger.info(
"Content-guarded PyPI access DENIED via content guard: org_id=%s user=%s",
org_id,
user,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+102
to
+111
| "Content-guarded PyPi access GRANTED via DomainOrg: user=%s org_id=%s", | ||
| user, | ||
| org_id, | ||
| ) | ||
| return True | ||
|
|
||
| try: | ||
| casted_guard = guard.cast() | ||
| except Exception: | ||
| _logger.exception("Failed to resolve content guard type for distribution") |
Contributor
There was a problem hiding this comment.
nitpick (typo): Minor typo/inconsistency in log message: use "PyPI" instead of "PyPi".
These log lines currently use "PyPi". Please change them to "PyPI" to match the canonical spelling and keep log search/aggregation consistent with the rest of the ecosystem.
Suggested implementation:
_logger.info(
"Content-guarded PyPI access GRANTED via DomainOrg: user=%s org_id=%s",
user,
org_id,
) _logger.info(
"Content-guarded PyPI access GRANTED via content guard: org_id=%s user=%s",
org_id,
user,
) _logger.info(
"Content-guarded PyPI access DENIED via content guard: org_id=%s user=%s",
org_id,
user,
)PyPi -> PyPI Signed-off-by: Bryan Ramos <bramos@redhat.com>
better reflects described condition. Signed-off-by: Bryan Ramos <bramos@redhat.com>
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_check_pypi_safe_method_accessmethod has grown quite complex with multiple early returns and nested error handling; consider extracting the DomainOrg bypass and content-guard resolution/permit logic into dedicated helpers to make the permission flow easier to follow and maintain. - In
_check_pypi_safe_method_access,Http404fromview.distributionis treated as an unconditional allow for SAFE_METHODs; you may want to reconsider or clearly centralize this behavior (e.g., via a helper or explicit configuration) so that unexpected distribution lookup patterns don’t silently bypass content guards.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_check_pypi_safe_method_access` method has grown quite complex with multiple early returns and nested error handling; consider extracting the DomainOrg bypass and content-guard resolution/permit logic into dedicated helpers to make the permission flow easier to follow and maintain.
- In `_check_pypi_safe_method_access`, `Http404` from `view.distribution` is treated as an unconditional allow for SAFE_METHODs; you may want to reconsider or clearly centralize this behavior (e.g., via a helper or explicit configuration) so that unexpected distribution lookup patterns don’t silently bypass content guards.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DomainBasedPermission no longer hardcodes the "lightwell" domain name or the "lightwell-network" feature.
Instead, _check_pypi_safe_method_access reads the distribution's content_guard:
DomainOrg owners still bypass the guard.
This makes PyPI access control domain-name-agnostic, any distribution can be protected by attaching a content guard, with no code changes needed.
JIRA: PULP-1993
Summary by Sourcery
Gate PyPI SAFE_METHOD access for content-guarded distributions via their content guard, while preserving default-allow behavior for unguarded distributions and DomainOrg-based bypass.
Enhancements:
Tests:
Summary by Sourcery
Gate PyPI SAFE_METHOD access using distribution content guards instead of a hardcoded feature flag, while preserving default-allow behavior for unguarded distributions and DomainOrg-based bypass.
Enhancements:
Tests: