-
Notifications
You must be signed in to change notification settings - Fork 116
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
Add an authenticate_header method to JSONHeaderRemoteAuthentication. #5820
Conversation
a5c20c8
to
52148ee
Compare
52148ee
to
5c17943
Compare
Did you correctly reference the issue number from the commit description? |
Thanks for eye catch it man. I've corrected it in the commit message but not in the description. Fixed now. |
We want to get rid of the "no-issue" label. I guess you need to use a lowercase in "Closes" or update the plugin template: https://github.com/pulp/plugin_template/blob/c4cd2b8f981eabec717616c7c47036a922a86624/.ci/scripts/pr_labels.py#L19. Which path are you choosing? |
@@ -72,3 +72,6 @@ def authenticate(self, request): | |||
|
|||
_logger.debug(_("User {user} authenticated.").format(user=remote_user)) | |||
return (user, None) | |||
|
|||
def authenticate_header(self, request): | |||
return "JSONHeaderRemoteAuthentication" |
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.
From reading the docs[0], it looks like this string should be a name of a header that a client can send to authenticate. However, when looking through docs outside of DRF, it seems like the string that is sent with the 401 is supposed describe what type of credentials should be sent[1,2]. I believe in this case this string just needs to be "Bearer"
[0] https://www.django-rest-framework.org/api-guide/authentication/#custom-authentication
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate
[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401
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.
However, 'Bearer' may not be an appropriate value in all cases. Perhaps we should not make this adjustment and just adjust our test to expect a 403?
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.
However, 'Bearer' may not be an appropriate value in all cases. Perhaps we should not make this adjustment and just adjust our test to expect a 403?
Could be, but it's strange anyway. When I see a 403 I've presumed that the user is authenticated and then it got his access denied. Different from 401.
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.
Also, this changes as we change the sequence of authentication classes.
We decided to add this in the pulp_service plugin. |
Closes #5819