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

introduce Authorization::bearer helper method #121

Merged
merged 7 commits into from
Nov 18, 2022

Conversation

miraclx
Copy link
Collaborator

@miraclx miraclx commented Nov 17, 2022

Resolves #118.

Introduces the Authorization::bearer method that adds an equivalent Authorization: Bearer ... entry into the request headers.

This function does no input validation beyond that it's a header value, not that it's a valid JWT.

We offload the guarantee of it being a valid utf-8 string to the caller.

Base automatically changed from miraclx/non-debuggable-api-keys to master November 17, 2022 14:34
@miraclx miraclx force-pushed the miraclx/add-authorization-header-helper branch from a05b002 to 2d73544 Compare November 17, 2022 14:42
Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides one thing

///
/// See the [`auth`](self) module documentation for more information.
pub fn bearer<T: AsRef<str>>(token: T) -> Result<Self, InvalidHeaderValue> {
HeaderValue::from_bytes(&[b"Bearer ", token.as_ref().as_bytes()].concat()).map(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come we're concatenating with bytes here? Also, from_bytes allow ASCII characters outside the standard visible characters which might not be what we want from authorization. from_str would only allow the visible ASCII characters

Suggested change
HeaderValue::from_bytes(&[b"Bearer ", token.as_ref().as_bytes()].concat()).map(
HeaderValue::from_str(&["Bearer ", token.as_ref()].concat()).map(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature T: AsRef<str> already enforces this guarantee at the caller level. So both of these are pretty much equivalent, but we avoid constructing an owned String when concat-ing.

PS: the logic of from_str doesn't use any str-specific logic. It literally just casts it back to a slice.

https://github.com/hyperium/http/blob/34a9d6bdab027948d6dea3b36d994f9cbaf96f75/src/header/value.rs#L125

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature T: AsRef already enforces this guarantee at the caller level. So both of these are pretty much equivalent, but we avoid constructing an owned String when concat-ing.

ahh, ignore my suggestion then

PS: the logic of from_str doesn't use any str-specific logic. It literally just casts it back to a slice.

interesting and weird how the docs say that it accepts visible ASCII octets only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting and weird how the docs say that it accepts visible ASCII octets only?

Yeah, it does validation at a later point, but by then it doesn't matter what the input was, it validates a byte slice eventually. https://github.com/hyperium/http/blob/34a9d6bdab027948d6dea3b36d994f9cbaf96f75/src/header/value.rs#L226

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: thanks for looking through

@miraclx miraclx merged commit 18b998b into master Nov 18, 2022
@miraclx miraclx deleted the miraclx/add-authorization-header-helper branch November 18, 2022 01:17
@miraclx miraclx mentioned this pull request Feb 24, 2023
@frol frol mentioned this pull request Jun 2, 2023
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

Successfully merging this pull request may close these issues.

Support JWT auth
2 participants