Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

decko
Copy link
Member

@decko decko commented Sep 20, 2024

Closes #5819

@lubosmj
Copy link
Member

lubosmj commented Sep 23, 2024

Did you correctly reference the issue number from the commit description?

@decko
Copy link
Member Author

decko commented Sep 23, 2024

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.

@lubosmj
Copy link
Member

lubosmj commented Sep 23, 2024

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"
Copy link
Member

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

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@dkliban
Copy link
Member

dkliban commented Sep 24, 2024

We decided to add this in the pulp_service plugin.

@dkliban dkliban closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSONHeaderRemoteAuthentication auth class usage is returning 403 instead of 401
3 participants