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

Expose _get_credentials of BasicAuthAuthenticationPolicy #2659

Closed
canni opened this issue Jun 30, 2016 · 5 comments
Closed

Expose _get_credentials of BasicAuthAuthenticationPolicy #2659

canni opened this issue Jun 30, 2016 · 5 comments

Comments

@canni
Copy link
Contributor

canni commented Jun 30, 2016

Currently the _get_credentials method of BasicAuthAuthenticationPolicy is an instance method, witch prevents from using this well-tested bit of functionality outside of AuthPolicy scope.

Backward-compatible propositions:

  • Make _get_credentials an exposed staticmethod: get_credentials (as it does not require self or cls to operate)
  • Extract this bit into Request object method

Reasoning:

In my case I have to support legacy quirks, where some non-authorization headers contain HTTP Basic authinfo, so currently my options are:

  • copy-paste the code from BasicAuthPolicy & it's tests - ugly and does not promote code re-use
  • create a temporary BasicAuthPolicy object, just to invoke this "private" method - as ugly as previous one

Before I jump to hacking a PR, maybe some discussion on best/other possibilities?

@mmerickel
Copy link
Member

I was surprised to discover that this _get_credentials method even exists considering webob already has a parser via request.authorization and webob.descriptor.parse_auth (which probably isn't a public api). However, this only handles splitting up the <authtype> <payload> format. The base64 decode/split of the username/password is not handled by webob.

It feels like a couple changes could be made to make this more helpful. Instead of a _get_credentials method we could add pyramid.authentication.get_basic_credentials or something that would return the username/password tuple or raise a ValueError.

@canni
Copy link
Contributor Author

canni commented Jul 1, 2016

For my case the best would be to export this functionality in a way that I could call it with any header, not only HTTP-Authorization, probably the header from where to extract from could be just a kwarg with Authorization as a default value for it.

@ztane
Copy link
Contributor

ztane commented Jul 3, 2016

Where do you need it? I am thinking that the Authn/z is too complicated already; I am not sure Pyramid core should have anything about passwords really.

@ztane
Copy link
Contributor

ztane commented Jul 3, 2016

Sorry, I read the commit now, this is a sane change of course...

@mmerickel
Copy link
Member

Closed via #2662.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants