Skip to content

Allow an auth parameter when EnvironBuilder building#1809

Merged
davidism merged 1 commit into
pallets:masterfrom
pgjones:test_auth
Oct 16, 2020
Merged

Allow an auth parameter when EnvironBuilder building#1809
davidism merged 1 commit into
pallets:masterfrom
pgjones:test_auth

Conversation

@pgjones
Copy link
Copy Markdown
Member

@pgjones pgjones commented May 18, 2020

This allows a shorthand for testing basic auth in Werkzeug (and
Flask). Specifically it allows

test_client.get("/", auth=(username, password))

as a shorthand for

credentials = b64encode(b":".join((username.encode(), password.encode())))
headers = {"Authorization": "Basic {}".format(credentials.decode())}
test_client.get("/", headers=headers)

I'm not sure if the auth param should also allow the Authenticate datastructures (alongside the tuple in this PR). This would allow more than just basic auth, but would require the ability to write these headers via the datastructure. @jab do you have a view?

@jab
Copy link
Copy Markdown
Contributor

jab commented May 20, 2020

Thanks for the ping, @pgjones! Indeed, I opened #1769 out of concern that werkzeug.datastructures.WWWAuthenticate currently only supports Basic and Digest auth, and for similar reasons would advocate that the new auth param added here should work with other auth schemes (at least eventually, maybe even documenting that intention?), or alternatively just rename this param basic_auth if you only think this should be used for Basic auth.

Likewise, accepting either a tuple or a werkzeug.datastructures.Authorization instance here seems like better design, but again, if for some reason you expect the vast majority use case to be for testing Basic auth, then the current tuple-only shorthand seems fine.

What's your thinking on this? Do you expect to not need similar API to facilitate testing other auth schemes for some reason?

As I mentioned in #1769, I implemented lightweight WSGI middleware for my (Bearer and Negotiate) authentication, and didn't even need Werkzeug for that, so this doesn't really affect me. If that's representative of most users, then we can take the concerns above with a grain of salt. But it'd be helpful to hear more about how other users may actually be affected by Werkzeug's current limitation to Basic and Digest auth to make a more informed decision here.

@davidism
Copy link
Copy Markdown
Member

We should accept an Authorization object as well.

@pgjones
Copy link
Copy Markdown
Member Author

pgjones commented Oct 15, 2020

@jab & @davidism updated to accept Authorization arguments.

This allows a shorthand for testing basic auth in Werkzeug (and
Flask). Specifically it allows

    test_client.get("/", auth=(username, password))

as a shorthand for

    credentials = b64encode(b":".join((username.encode(), password.encode())))
    headers = {"Authorization": "Basic {}".format(credentials.decode())}
    test_client.get("/", headers=headers)
@davidism
Copy link
Copy Markdown
Member

I shuffled things around a bit in anticipation of eventually supporting more auth types. It didn't make a lot of sense to me to have a specialty header function when all it does is take an object that could have a method. Instead of a http.dump_authorization_header() function, moved it to the Authorization.to_header() method. This mirrors the WWWAuthenticate object.

This would probably make sense for the CSP header object as well.

@davidism davidism merged commit ad3ac73 into pallets:master Oct 16, 2020
@davidism davidism added this to the 2.0.0 milestone Oct 16, 2020
@pgjones pgjones deleted the test_auth branch October 17, 2020 09:02
@pgjones
Copy link
Copy Markdown
Member Author

pgjones commented Oct 17, 2020

Thanks.

This would probably make sense for the CSP header object as well.

Happy to adopt this (I want to add COOP support as well).

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants