-
Notifications
You must be signed in to change notification settings - Fork 15
Allow unauthenticated GET requests to simple API #847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR introduces a new DRF permission class that allows unauthenticated GET requests to the simple API by replacing the previous domain-based public access logic with a simple method check, and wires this behavior into the container build via an updated patch file. Sequence diagram for unauthenticated GET request using AllowUnauthPullsequenceDiagram
actor Client
participant DRFView
participant AllowUnauthPull
Client->>DRFView: HTTP GET /simple/
DRFView->>AllowUnauthPull: has_permission(request, view)
AllowUnauthPull-->>DRFView: True (method is GET)
DRFView-->>Client: 200 OK
Client->>DRFView: HTTP POST /simple/
DRFView->>AllowUnauthPull: has_permission(request, view)
AllowUnauthPull-->>DRFView: False (method is not GET)
DRFView-->>Client: 403 Forbidden
Class diagram for updated DRF permission class allowing unauthenticated GETclassDiagram
class BasePermission {
<<abstract>>
+has_permission(request, view) bool
}
class PublicDomainPermission {
<<removed>>
+has_permission(request, view) bool
}
class AllowUnauthPull {
+has_permission(request, view) bool
}
BasePermission <|-- PublicDomainPermission
BasePermission <|-- AllowUnauthPull
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The new
AllowUnauthPullpermission currently allows any unauthenticated GET to the view; if this is meant to be limited to specific endpoints, consider scoping it more narrowly (e.g. only for certain views or URL patterns) rather than applying it generically. - Instead of hardcoding
request.method == "GET", consider usingrequest.method in SAFE_METHODSfrom DRF if you also want to support other read-only verbs like HEAD/OPTIONS, or explicitly document why only GET is allowed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `AllowUnauthPull` permission currently allows any unauthenticated GET to the view; if this is meant to be limited to specific endpoints, consider scoping it more narrowly (e.g. only for certain views or URL patterns) rather than applying it generically.
- Instead of hardcoding `request.method == "GET"`, consider using `request.method in SAFE_METHODS` from DRF if you also want to support other read-only verbs like HEAD/OPTIONS, or explicitly document why only GET is allowed.
## Individual Comments
### Comment 1
<location> `pulp_service/pulp_service/app/authorization.py:123-131` </location>
<code_context>
-class PublicDomainPermission(BasePermission):
+class AllowUnauthPull(BasePermission):
"""
- A Permission Class that grants permission to Domains with public- prefix
+ A Permission Class that grants permission to make GET requests without authN/authZ
"""
def has_permission(self, request, view):
# Only allow GET requests
- 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
</code_context>
<issue_to_address>
**🚨 issue (security):** Broadening from public domains to all GET requests may introduce unintended data exposure.
This change removes the `public-` domain constraint and effectively allows unauthenticated access to any GET endpoint using this permission. Please verify that every view using `AllowUnauthPull` only returns data that is safe for anonymous users, and that there’s no indirect data leak via query params, path-based tenant selection, or similar mechanisms.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0b982d8 to
b5f588e
Compare
decko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
b5f588e to
d9e2c9a
Compare
d9e2c9a to
ffac9ce
Compare
Summary by Sourcery
Enhancements: