-
Notifications
You must be signed in to change notification settings - Fork 15
Disable authentication using ServiceAccount #916
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 changes the deprecated ServiceAccount-based authentication path so that any attempt to authenticate via the Sequence diagram for deprecated ServiceAccount authentication behavior changesequenceDiagram
actor ApiClient
participant PulpAPI
participant AuthenticationBackend
ApiClient->>PulpAPI: HTTP request with remote_user header
PulpAPI->>AuthenticationBackend: authenticate(request)
AuthenticationBackend->>AuthenticationBackend: Parse request JSON
alt Invalid JSON
AuthenticationBackend-->>PulpAPI: raise AuthenticationFailed Access denied. Invalid JSON.
PulpAPI-->>ApiClient: 401 Unauthorized
else Valid JSON and remote_user present
AuthenticationBackend-->>PulpAPI: raise AuthenticationFailed Deprecated c.rh.c SA authentication method attempted by user
PulpAPI-->>ApiClient: 401 Unauthorized
else No remote_user
AuthenticationBackend-->>PulpAPI: return None (no authentication)
PulpAPI-->>ApiClient: Process as unauthenticated request
end
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:
- Consider preserving the warning log when rejecting deprecated SA authentication (log then raise) so that operational visibility of these attempts is not lost.
- Including the
remote_uservalue in the authentication failure message may leak user identifiers to clients; consider logging it server-side only and returning a more generic error to the caller.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider preserving the warning log when rejecting deprecated SA authentication (log then raise) so that operational visibility of these attempts is not lost.
- Including the `remote_user` value in the authentication failure message may leak user identifiers to clients; consider logging it server-side only and returning a more generic error to the caller.
## Individual Comments
### Comment 1
<location> `pulp_service/pulp_service/app/authentication.py:99-102` </location>
<code_context>
- _logger.warning(f"Deprecated c.rh.c SA authentication method attempted by user: {remote_user}")
- return None
+ raise AuthenticationFailed(
+ _(f"Deprecated c.rh.c SA authentication method attempted by user: {remote_user}")
+ )
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding f-strings inside the translation function.
Using an f-string inside `_()` ties the translation key to the specific value, which makes catalog management harder. Prefer placeholders instead, e.g. `_("Deprecated c.rh.c SA authentication method attempted by user: %(user)s") % {"user": remote_user}`.
```suggestion
if remote_user:
raise AuthenticationFailed(
_(
"Deprecated c.rh.c SA authentication method "
"attempted by user: %(user)s"
)
% {"user": remote_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.
328f410 to
179aab2
Compare
ref: https://issues.redhat.com/browse/PULP-1064
Summary by Sourcery
Bug Fixes: