Configure python SimpleView access for public domains #832
Conversation
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
Contributor
Reviewer's GuideAdds a new PublicDomainPermission for unauthenticated GET access to public- prefixed domains, wires additional authentication into default REST framework settings, extends Tekton CI with pulp-service functional tests, and introduces functional tests plus fixtures to validate x-rh-identity handling and public vs private PyPI Simple endpoint behavior. Sequence diagram for public domain GET permission evaluationsequenceDiagram
actor User
participant DjangoApp
participant PublicDomainPermission
participant DomainModel
User->>DjangoApp: GET /pypi/simple/
DjangoApp->>PublicDomainPermission: has_permission(request, view)
PublicDomainPermission->>PublicDomainPermission: check request.method == GET
alt method_is_not_GET
PublicDomainPermission-->>DjangoApp: return False
DjangoApp-->>User: 403 Forbidden
else method_is_GET
PublicDomainPermission->>PublicDomainPermission: domain_pk = get_domain_pk()
PublicDomainPermission->>DomainModel: Domain.objects.get(pk=domain_pk)
alt domain_name_starts_with_public_prefix
DomainModel-->>PublicDomainPermission: domain(name=public-example)
PublicDomainPermission-->>DjangoApp: return True
DjangoApp-->>User: 200 OK
else not_public_or_error
DomainModel-->>PublicDomainPermission: domain(name=private-example) or exception
PublicDomainPermission-->>DjangoApp: return False
DjangoApp-->>User: 403 Forbidden
end
end
Class diagram for new PublicDomainPermission and related typesclassDiagram
class BasePermission {
}
class PublicDomainPermission {
+has_permission(request, view) bool
}
class Domain {
+pk int
+name string
+objects DomainManager
}
class DomainManager {
+get(pk int) Domain
}
class Utilities {
+get_domain_pk() int
}
PublicDomainPermission --|> BasePermission
Domain o-- DomainManager
PublicDomainPermission ..> Domain : uses
PublicDomainPermission ..> Utilities : calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
git-hyagi
force-pushed
the
pypi-public-domain-access
branch
7 times, most recently
from
da71b22 to
a0e7e35
Compare
git-hyagi
force-pushed
the
pypi-public-domain-access
branch
from
a0e7e35 to
0b6f3d7
Compare
![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
PublicDomainPermission.has_permission, consider narrowing the broadexcept Exceptionto specific errors likeDomain.DoesNotExist(and potentiallyValueError/TypeErrorfromget_domain_pk()), so unexpected failures don’t get silently turned into authorization denials and are easier to debug. - The
PublicDomainPermissioncurrently only allowsGETrequests; if you want to support typical read-only semantics, you may want to explicitly handleHEAD(and possiblyOPTIONS) as allowed methods for public domains rather than denying them. - The
cleanup_auth_headersfixture and the explicit header cleanup at the end oftest_pypi_simple_endpoint_private_domain_without_auth_blockedpartially duplicate behavior; consider consolidating this cleanup logic into the fixture (e.g., usingyieldand always running after the test) to avoid header-leakage bugs and make it easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PublicDomainPermission.has_permission`, consider narrowing the broad `except Exception` to specific errors like `Domain.DoesNotExist` (and potentially `ValueError`/`TypeError` from `get_domain_pk()`), so unexpected failures don’t get silently turned into authorization denials and are easier to debug.
- The `PublicDomainPermission` currently only allows `GET` requests; if you want to support typical read-only semantics, you may want to explicitly handle `HEAD` (and possibly `OPTIONS`) as allowed methods for public domains rather than denying them.
- The `cleanup_auth_headers` fixture and the explicit header cleanup at the end of `test_pypi_simple_endpoint_private_domain_without_auth_blocked` partially duplicate behavior; consider consolidating this cleanup logic into the fixture (e.g., using `yield` and always running after the test) to avoid header-leakage bugs and make it easier to maintain.
## Individual Comments
### Comment 1
<location> `pulp_service/pulp_service/app/authorization.py:133-138` </location>
<code_context>
+ if request.method != "GET":
+ return False
+
+ try:
+ domain_pk = get_domain_pk()
+ domain = Domain.objects.get(pk=domain_pk)
+ if domain.name.startswith("public-"):
+ return True
+ except Exception:
+ # If we can't get the domain, deny access
+ return False
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid catching bare `Exception` and narrow to expected error types.
Catching bare `Exception` here can mask real bugs (e.g. programming or DB errors) and make them harder to detect. Prefer catching only the expected cases, e.g. `Domain.DoesNotExist` and the specific exceptions `get_domain_pk()` can raise (such as `ValueError`/`TypeError`), and let anything else surface instead of being converted into a generic 403/denial.
</issue_to_address>
### Comment 2
<location> `pulp_service/pulp_service/tests/functional/test_authentication.py:345-354` </location>
<code_context>
+ api_host = python_bindings.DistributionsPypiApi.api_client.configuration.host
+ simple_url = f"{api_host}/api/pypi/{domain_name}/{base_path}/simple/"
+
+ # This should be blocked for non-public domains
+ # 401 = Unauthorized, 403 = Forbidden
+ response = requests.get(simple_url)
+ assert response.status_code in [401, 403]
+
+ # some bindings has scope=session, so we need to remove the headers to avoid
+ # affecting the other tests
+ pulpcore_bindings.DomainsApi.api_client.default_headers.pop("x-rh-identity", None)
+ python_bindings.RepositoriesPythonApi.api_client.default_headers.pop("x-rh-identity", None)
+ python_bindings.DistributionsPypiApi.api_client.default_headers.pop("x-rh-identity", None)
</code_context>
<issue_to_address>
**suggestion (testing):** Rely on a centralized header-cleanup mechanism instead of manually popping headers in this test.
Here you manually clear `x-rh-identity` from three bindings. Since we already have a `cleanup_auth_headers` fixture for preventing auth header leakage between tests, it would be better to rely on a shared mechanism instead of ad-hoc `pop` calls.
Consider either extending `cleanup_auth_headers` (e.g. via `yield` or `request.addfinalizer`) so it always runs after tests and covers all relevant bindings, then drop the manual cleanup here, or extracting this logic into a reusable helper/fixture used by all tests that modify `default_headers`. This will improve test isolation and reduce the risk of flakiness from shared client state.
Suggested implementation:
```python
# This should be blocked for non-public domains
# 401 = Unauthorized, 403 = Forbidden
response = requests.get(simple_url)
assert response.status_code in [401, 403]
# Cleanup of auth-related headers is handled by the shared cleanup_auth_headers fixture.
```
1. Ensure this test function uses the `cleanup_auth_headers` fixture by adding it to the test function's parameter list, for example:
- Change `def test_xxx(...):` to `def test_xxx(..., cleanup_auth_headers):`.
2. If `cleanup_auth_headers` currently only targets a subset of bindings, extend it so that it also clears `x-rh-identity` from:
- `pulpcore_bindings.DomainsApi.api_client.default_headers`
- `python_bindings.RepositoriesPythonApi.api_client.default_headers`
- `python_bindings.DistributionsPypiApi.api_client.default_headers`
3. If the fixture is not in this file, confirm it is imported/available via `conftest.py` so that adding it as a parameter correctly triggers the centralized cleanup.
</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
+133
to
+138
| try: | ||
| domain_pk = get_domain_pk() | ||
| domain = Domain.objects.get(pk=domain_pk) | ||
| if domain.name.startswith("public-"): | ||
| return True | ||
| except Exception: |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Avoid catching bare Exception and narrow to expected error types.
Catching bare Exception here can mask real bugs (e.g. programming or DB errors) and make them harder to detect. Prefer catching only the expected cases, e.g. Domain.DoesNotExist and the specific exceptions get_domain_pk() can raise (such as ValueError/TypeError), and let anything else surface instead of being converted into a generic 403/denial.
Comment on lines
+345
to
+354
| # This should be blocked for non-public domains | ||
| # 401 = Unauthorized, 403 = Forbidden | ||
| response = requests.get(simple_url) | ||
| assert response.status_code in [401, 403] | ||
|
|
||
| # some bindings has scope=session, so we need to remove the headers to avoid | ||
| # affecting the other tests | ||
| pulpcore_bindings.DomainsApi.api_client.default_headers.pop("x-rh-identity", None) | ||
| python_bindings.RepositoriesPythonApi.api_client.default_headers.pop("x-rh-identity", None) | ||
| python_bindings.DistributionsPypiApi.api_client.default_headers.pop("x-rh-identity", None) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Rely on a centralized header-cleanup mechanism instead of manually popping headers in this test.
Here you manually clear x-rh-identity from three bindings. Since we already have a cleanup_auth_headers fixture for preventing auth header leakage between tests, it would be better to rely on a shared mechanism instead of ad-hoc pop calls.
Consider either extending cleanup_auth_headers (e.g. via yield or request.addfinalizer) so it always runs after tests and covers all relevant bindings, then drop the manual cleanup here, or extracting this logic into a reusable helper/fixture used by all tests that modify default_headers. This will improve test isolation and reduce the risk of flakiness from shared client state.
Suggested implementation:
# This should be blocked for non-public domains
# 401 = Unauthorized, 403 = Forbidden
response = requests.get(simple_url)
assert response.status_code in [401, 403]
# Cleanup of auth-related headers is handled by the shared cleanup_auth_headers fixture.- Ensure this test function uses the
cleanup_auth_headersfixture by adding it to the test function's parameter list, for example:- Change
def test_xxx(...):todef test_xxx(..., cleanup_auth_headers):.
- Change
- If
cleanup_auth_headerscurrently only targets a subset of bindings, extend it so that it also clearsx-rh-identityfrom:pulpcore_bindings.DomainsApi.api_client.default_headerspython_bindings.RepositoriesPythonApi.api_client.default_headerspython_bindings.DistributionsPypiApi.api_client.default_headers
- If the fixture is not in this file, confirm it is imported/available via
conftest.pyso that adding it as a parameter correctly triggers the centralized cleanup.
git-hyagi
commented
Jan 9, 2026
Comment on lines
+53
to
+55
| - authentication_classes = [] | ||
| - permission_classes = [] | ||
| + permission_classes = [PublicDomainPermission|DomainBasedPermission] |
Collaborator
Author
There was a problem hiding this comment.
- we still need to authenticate because the
permission_classeswill use therequest.userto validate permissions (so we cannot setauthentication_classes=[]) - the permission classes work in a composed way, so in this patch it will check if
PublicDomainPermissionORDomainBasedPermissionpassed - if no authentication class succeeds,
request.userdefaults to anAnonymousUserinstance,request.authis set to None, and drf will not return 401 to the client yet, it will now check thepermission_classes(for public domains, the authentication will fail and drf will check thePublicDomainPermission)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
ref: https://issues.redhat.com/browse/PULP-1065
Summary by Sourcery
Allow unauthenticated read-only access to the PyPI SimpleView for Python distributions in explicitly marked public domains while strengthening authentication configuration and coverage.
New Features:
Bug Fixes:
CI:
Tests: