Conversation
…179) Add support for unauthenticated GET/HEAD/OPTIONS requests to API endpoints for domains with names starting with "public-" (case-insensitive). This allows public read-only access to repository content and metadata while maintaining security for private domains and write operations. Changes: - Add PublicDomainReadOnlyPermission and PublicDomainOrAuthenticatedPermission classes to authorization.py - Update default permission class in clowdapp.yaml to apply system-wide - Update PyPI endpoints patch to use new permission class - Add comprehensive tests for public domain access, private domain restrictions, and case-insensitive matching - Document the feature in ARCHITECTURE.md with security considerations Security: - Only safe HTTP methods (GET, HEAD, OPTIONS) allowed without auth - POST/PUT/PATCH/DELETE still require authentication on public domains - Case-insensitive prefix matching (public-, Public-, PUBLIC-) - All unauthenticated access is logged for audit purposes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements domain-based public read-only access by introducing new permission classes that allow unauthenticated GET/HEAD/OPTIONS requests only for domains prefixed with public-, wiring this into the global DRF permission configuration, updating PyPI endpoint patches to use the new permission, and adding focused tests and documentation around the new behavior and security constraints. Sequence diagram for request authorization with PublicDomainOrAuthenticatedPermissionsequenceDiagram
actor Client
participant DRFView
participant PublicDomainOrAuthenticatedPermission as PublicDomainOrAuth
participant PublicDomainReadOnlyPermission as PublicReadOnly
participant DomainBasedPermission as DomainPerm
participant Domain
Client->>DRFView: HTTP request
DRFView->>PublicDomainOrAuth: has_permission(request, view)
activate PublicDomainOrAuth
PublicDomainOrAuth->>PublicReadOnly: has_permission(request, view)
activate PublicReadOnly
alt method not safe
PublicReadOnly-->>PublicDomainOrAuth: False
else method is safe
PublicReadOnly->>Domain: get(pk=get_domain_pk())
alt domain name startswith public- (case-insensitive)
Domain-->>PublicReadOnly: Domain instance
PublicReadOnly-->>PublicDomainOrAuth: True
else domain missing or not public- or error
Domain-->>PublicReadOnly: not found or error
PublicReadOnly-->>PublicDomainOrAuth: False
end
end
deactivate PublicReadOnly
alt PublicReadOnly returned True
PublicDomainOrAuth-->>DRFView: True
DRFView-->>Client: 2xx response (unauthenticated allowed)
else PublicReadOnly returned False
PublicDomainOrAuth->>DomainPerm: has_permission(request, view)
activate DomainPerm
DomainPerm-->>PublicDomainOrAuth: True or False
deactivate DomainPerm
PublicDomainOrAuth-->>DRFView: True or False
DRFView-->>Client: 2xx or 401/403 based on result
end
deactivate PublicDomainOrAuth
Class diagram for new public domain permission classesclassDiagram
class BasePermission {
<<abstract>>
+has_permission(request, view) bool
}
class Domain {
+pk
+name
}
class DomainBasedPermission {
+has_permission(request, view) bool
}
class PublicDomainReadOnlyPermission {
+has_permission(request, view) bool
-get_domain_pk() int
-SAFE_METHODS
-_logger
}
class PublicDomainOrAuthenticatedPermission {
+has_permission(request, view) bool
-PublicDomainReadOnlyPermission public_check
-DomainBasedPermission domain_check
}
BasePermission <|-- DomainBasedPermission
BasePermission <|-- PublicDomainReadOnlyPermission
BasePermission <|-- PublicDomainOrAuthenticatedPermission
Domain "1" <-- PublicDomainReadOnlyPermission : uses
PublicDomainOrAuthenticatedPermission --> PublicDomainReadOnlyPermission : delegates
PublicDomainOrAuthenticatedPermission --> DomainBasedPermission : delegates
Flow diagram for PublicDomainReadOnlyPermission decision logicflowchart TD
A[Start has_permission] --> B{Is request.method in SAFE_METHODS?}
B -- No --> Z[Return False]
B -- Yes --> C[domain_pk = get_domain_pk]
C --> D{domain_pk exists?}
D -- No --> Z
D -- Yes --> E[Fetch Domain by pk]
E --> F{Domain found and domain.name.lower startswith public-?}
F -- Yes --> G[Log allowing unauthenticated access]
G --> H[Return True]
F -- No --> Z
E -->|Domain.DoesNotExist| I[Log domain not found]
I --> Z
E -->|Exception| J[Log warning about error]
J --> Z
Z --> K[End has_permission]
H --> K
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
PublicDomainReadOnlyPermissionperforms aDomain.objects.getand logs on every allowed public request; consider caching the domain lookup for the lifetime of the request and downgrading or structuring the log level to avoid excessive overhead/volume on high-traffic public domains. - The setup logic for creating domains, repositories, distributions, and building URLs is duplicated across the new authentication tests; extracting a small helper or fixture to encapsulate this would make the tests shorter and easier to maintain.
- The Dockerfile change adding and applying
0042-Add-contains-filter-python-content-endpoint.patchseems orthogonal to the public-domain permission work described in the PR; consider moving this patch into a separate, focused PR to keep changes scoped and reviewable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `PublicDomainReadOnlyPermission` performs a `Domain.objects.get` and logs on every allowed public request; consider caching the domain lookup for the lifetime of the request and downgrading or structuring the log level to avoid excessive overhead/volume on high-traffic public domains.
- The setup logic for creating domains, repositories, distributions, and building URLs is duplicated across the new authentication tests; extracting a small helper or fixture to encapsulate this would make the tests shorter and easier to maintain.
- The Dockerfile change adding and applying `0042-Add-contains-filter-python-content-endpoint.patch` seems orthogonal to the public-domain permission work described in the PR; consider moving this patch into a separate, focused PR to keep changes scoped and reviewable.
## Individual Comments
### Comment 1
<location> `pulp_service/pulp_service/app/authorization.py:135-144` </location>
<code_context>
+class PublicDomainReadOnlyPermission(BasePermission):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider tightening the broad `except Exception` and logging with traceback for easier debugging.
At this level, a broad catch makes it difficult to separate expected operational errors (e.g. transient DB issues) from genuine bugs, and the current logging loses the traceback. Please either narrow the exception type to the specific errors you expect (e.g. `DatabaseError`) or, if you must keep it broad, log with `exc_info=True` (e.g. `logger.warning("Error checking domain for public access", exc_info=True)`) so the traceback is preserved and issues aren’t silently masked.
Suggested implementation:
```python
except Exception:
# Preserve full traceback for easier debugging while still denying access on error
logger.warning("Error checking domain for public access", exc_info=True)
return False
```
Because I only see part of the file, please also:
1. Verify that the above `except Exception` block is inside `PublicDomainReadOnlyPermission.has_permission`; if the log message is slightly different, adjust the SEARCH text to match the existing message exactly.
2. If your codebase defines more specific operational error types (e.g., `DatabaseError`, `OperationalError` or a custom domain check exception), consider replacing `Exception` with a tuple of those specific types while keeping `exc_info=True`:
- `except (DatabaseError, OperationalError) as exc:`
3. Ensure that `logger` is defined in this module, typically via:
- `import logging`
- `logger = logging.getLogger(__name__)`
If the logger variable or import names differ, adapt the code to the existing logging conventions.
</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
+135
to
+144
| class PublicDomainReadOnlyPermission(BasePermission): | ||
| """ | ||
| A Permission Class that grants permission to make GET/HEAD/OPTIONS requests | ||
| without authentication ONLY to domains with 'public-' prefix. | ||
| """ | ||
|
|
||
| def has_permission(self, request, view): | ||
| # Only allow safe methods | ||
| if request.method not in SAFE_METHODS: | ||
| return False |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider tightening the broad except Exception and logging with traceback for easier debugging.
At this level, a broad catch makes it difficult to separate expected operational errors (e.g. transient DB issues) from genuine bugs, and the current logging loses the traceback. Please either narrow the exception type to the specific errors you expect (e.g. DatabaseError) or, if you must keep it broad, log with exc_info=True (e.g. logger.warning("Error checking domain for public access", exc_info=True)) so the traceback is preserved and issues aren’t silently masked.
Suggested implementation:
except Exception:
# Preserve full traceback for easier debugging while still denying access on error
logger.warning("Error checking domain for public access", exc_info=True)
return FalseBecause I only see part of the file, please also:
- Verify that the above
except Exceptionblock is insidePublicDomainReadOnlyPermission.has_permission; if the log message is slightly different, adjust the SEARCH text to match the existing message exactly. - If your codebase defines more specific operational error types (e.g.,
DatabaseError,OperationalErroror a custom domain check exception), consider replacingExceptionwith a tuple of those specific types while keepingexc_info=True:except (DatabaseError, OperationalError) as exc:
- Ensure that
loggeris defined in this module, typically via:import logginglogger = logging.getLogger(__name__)
If the logger variable or import names differ, adapt the code to the existing logging conventions.
Member
Author
|
decko
changed the title
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
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.
Summary
Implements PULP-1179 to enable unauthenticated GET/HEAD/OPTIONS requests to any API endpoint for domains with names starting with
public-(case-insensitive).Changes
Core Implementation
pulp_service/pulp_service/app/authorization.py):PublicDomainReadOnlyPermission: Allows unauthenticated GET/HEAD/OPTIONS to public- domainsPublicDomainOrAuthenticatedPermission: Combined permission class (public domain access OR authenticated access)Configuration
deploy/clowdapp.yaml):PULP_REST_FRAMEWORK__DEFAULT_PERMISSION_CLASSESto usePublicDomainOrAuthenticatedPermissionPatches
images/assets/patches/0038-readonly-pypi-endpoints.patch):PublicDomainOrAuthenticatedPermissionclass for consistencyTesting
pulp_service/pulp_service/tests/functional/test_authentication.py):test_get_requests_to_public_domains_without_auth: Verifies GET/HEAD/OPTIONS work without auth on public- domainstest_get_requests_to_private_domains_require_auth: Verifies private domains require authtest_case_insensitive_public_domain_matching: Tests case-insensitive matching (public-, Public-, PUBLIC-)Documentation
Security Features
✅ Prefix-only matching: Uses
startswith("public-")to prevent accidental matches✅ Case-insensitive: Handles public-, Public-, PUBLIC-, etc.
✅ Safe methods only: Only GET/HEAD/OPTIONS allowed without auth
✅ Write operations protected: POST/PUT/PATCH/DELETE still require authentication
✅ Fail-safe: Returns False on any exceptions
✅ Audit logging: All public access is logged with domain name
Behavior Changes
Before:
AllowUnauthPull)After:
Testing Instructions
Unit Tests
Manual Testing
Deployment Notes
"Allowing unauthenticated {method} to public domain: {name}"PULP_REST_FRAMEWORK__DEFAULT_PERMISSION_CLASSEStoDomainBasedPermissionif neededFiles Changed
pulp_service/pulp_service/app/authorization.py(+50 lines)deploy/clowdapp.yaml(+2/-2 lines)images/assets/patches/0038-readonly-pypi-endpoints.patch(+7/-7 lines)pulp_service/pulp_service/tests/functional/test_authentication.py(+189/-56 lines)docs/ARCHITECTURE.md(+27 lines)Total: 284 lines added, 56 lines removed
🤖 Generated with Claude Code
Summary by Sourcery
Restrict unauthenticated API access to read-only requests on explicitly public domains while updating permissions, configuration, and tests accordingly.
New Features:
Enhancements:
Tests: