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

Use new headers crate instead of hyper-old-types #158

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mthebridge
Copy link
Contributor

@mthebridge mthebridge commented Jul 20, 2022

Signed-off-by: Mark Thebridge [email protected]

hyper-old-types is no longer maintained. Switch to the headers hyper crate instead - https://docs.rs/headers/latest/headers/authorization/index.html

This is a breaking change because swagger-rs re-exported types.

  • Rather than continue to re-export, I've made the AuthData enum variants just wrap Strings. This is slightly uglier, but the inner Basic and Bearer types in headers can't be directly constructed, only decoded from an Authorization header, and this felt like a more efficient implementation.
  • The from_headers method now returns an Option<AuthData>, thus removing any public authentication type reexports and avoiding exposing the whole swagger ecosystem to upstream breaking changes.

There were no existing tests of AuthData - not sure what would be useful to add, but will do if there's anything you think would help?

@mthebridge mthebridge requested a review from richardwhiuk July 20, 2022 12:49
src/auth.rs Outdated
password: Some(password.to_owned()),
})
let basic = Header::basic(username, password);
AuthData::Basic(basic.username().to_owned(), basic.password().to_owned())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realised this is a pretty pointless implementation - I'm cnnstucting the header for no reason. I feel like ideally, we would return https://docs.rs/headers/latest/headers/authorization/struct.Basic.html here - but that has no constructor beyond a decode from a HeaderValue. (Same applies to the Bearer variant). We could make the inner type an authorization header but that feels wrong too.

Thoughts welcome!

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.

1 participant