ref #2659 public HTTP Basic credentials extraction

[canni]

Expose HTTP Basic credentials extraction to public API, it is helpful also outside of HttpBasicAuthenticationPolicy

Also try first logic from WebOb before doing everything manually

[mmerickel]

I don't see the point in this fallback. It's trying to reproduce all of the logic, but likely with bugs and inconsistencies from the original request.authorization code. This looks like a maintenance nightmare.

[mmerickel]

I thought you wanted to support custom headers. This obviously does not. In fact, adding request.authorization from webob means you have no chance.

Regardless, I see no point in supporting custom headers myself. Parsing an HTTP BASIC header is completely trivial (base64 decode, convert to latin-1 or utf8, split on colon). If you're using this encoding and it's not in the standard Authorization header then it's really not our problem and not worth us maintaining just for you.

I'm fine merging this if the comment above is addressed about the fallbacks. But adding in custom header support is something I'm against.

[canni]

@mmerickel yes, I also came to the conclusion, that adding custom header support is out of the framework scope, anyway this code is still useful outside of authentication policy code, I'll remove fallback

[digitalresistor]

I wonder if this or something like it shouldn't live instead within WebOb instead of part of Pyramid?

[canni]

Is there anything else I should do to get this merged? Or are we just rejecting this?

[mmerickel]

This crosses 79 chars, but I will fix it in the merge.

[mmerickel]

It's debatable whether this belongs in webob. It's not in there right now because webob began in paste and paste had the helper methods in paste.authentication. Webob was then split out and paste was the lib with all the wsgi helpers. Now paste is much less popular than it once was and so placing this type of logic in there is less desirable.

[mmerickel]

@canni Can you add your name to contributors.txt please.

[canni]

Can we re-run checks? the failure was a connection error from intersphinx

[stevepiercy]

@canni you will need to pull in this change on master:
https://github.com/Pylons/pyramid/pull/2702/files

[canni]

@stevepiercy done

[stevepiercy]

@mmerickel OK to merge? Backport needed as well?

[tseaver]

@stevepiercy I'd say no to a backport: it is a new feature.

[digitalresistor]

Sorry, thought @mmerickel had already provided a +1 as well.

This LGTM.

[mmerickel]

Are we making this a public API? I'd like to be clear on the point of this PR.

[mmerickel]

Right now, all the tests are a side-effect of its usage in the auth policy, obviously. Certainly it'd be nice if that were fixed but not critical.

[mmerickel]

If this is a public API then it needs to be added to the docs as such in docs/api/authentication.rst.

[canni]

I'll update docs (if we're decided on making this a public API?) extracting tests shouldn't be a problem also

[mmerickel]

@canni I don't have a problem making it public I guess. Below is just to make sure we're all on the same page and happy with what we're doing before we ship a public api that we get to maintain since it's changed in scope and doesn't solve your original issue #2659.

There's 3 separate algorithms required to identify basic credentials.

1. Where are the credentials? Header? Somewhere else?
2. How to parse the credential envelope?
3. How to get the username/password from the basic-serialized content?

1 is pretty arbitrary. Obviously it's usually the Authorization header. Could be something else for some reason.
2 depends on the source. If it's the Authorization header. Then you split check the auth type of the header, check for Basic and send the second part to step 3.
Finally part 3 is standard, using the b64decode/split on colon algorithm.

I'd argue that everything is up to the user (re: auth policy) except part 3 which is standard.

This api you want to expose combines all 3 parts and thus is only useable in one very specific scenario... The credentials passed via Authorization header with Basic auth type. This is fine because it's like 99% of the use cases for basic auth, but you came in here originally asking for the 1% in #2659.

So anyway, if you want to make the changes to the docs and the tests then I have no problem merging it.

[canni]

@mmerickel yes I came with < 1% use-cases, and my use case is quite complex :)

Short answer: Yes this solves my case

Long answer:

One of legacy systems has been designed (don't ask me why) in such a way that credentials are coming from HTTP Basic auth, but with renamed header (I can work-around that easily) but after successful auth the sessions are used instead (HTTP Basic is just a credentials delivery on 1st request, then session cookie)

So I'm trying to mix'n'match Session based auth policy, and HTTP Basic one, into single thing.

Current implementation, with _get_credentials "private" instance method prevents me from (relatively) easy re-use of credentials extraction logic already in place.

So by extracting get_http_basic_credentials my Auth Policy becomes swift and simple subclass of Session Auth Policy

[canni]

@mmerickel I added new fn to docs, and (re-)created test cases. (I was wondering if I should mock usage of this helper fn for policy, but in the end the policy tests are still valid, as standalone...)

[mmerickel]

LGTM, thank you @canni !

[canni]

I also changed the return value to be a namedtuple gives a nicer API, with B/C