Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Factor token provider out into a separate trait #15

Merged
merged 4 commits into from
Dec 17, 2020

Conversation

djc
Copy link
Contributor

@djc djc commented Nov 9, 2020

I ran into dermesser/yup-oauth2#137. In general, making the authentication/authorization process more explicit in the type system seems like a good idea (in a future PR, the YupAuthorizer and dependency could become optional).

@djc
Copy link
Contributor Author

djc commented Nov 11, 2020

My work project is now using a different Authorizer impl instead, based on the gcp_auth crate. It has a more minimal API, which makes it easier to use for our purposes. I could add my (equally optional) gcp_auth-based implementation to this crate as well, if you think that makes sense.

Copy link

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

It makes me scratch my head that the feature flag actually being evaluated inside of src/ is yup-oauth2, because distinguishing between yup-oauth2 and the newly added yup-authorizer flag seems like a distinction that does more harm than good. hyper-rustls is necessary for code enabled with cfg(feature = "yup-oauth2") to compile, and the compilation failures don't give great diagnostics. This all would get obviated by just checking cfg(feature = yup-authorizer) instead.

Did you have a concrete benefit in mind for using flags this way that I'm not seeing?

@djc
Copy link
Contributor Author

djc commented Nov 13, 2020

Did you have a concrete benefit in mind for using flags this way that I'm not seeing?

Not really, I tend to want to use the "smallest" feature guard so that if you add more complex feature dependencies later on, stuff is a little easier. I changed all of the feature guards to yup-authorizer for now.

@djc
Copy link
Contributor Author

djc commented Dec 17, 2020

Rebased this on top of current master.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Dec 17, 2020

👏 Nicely done!

I could add my (equally optional) gcp_auth-based implementation to this crate as well, if you think that makes sense.

Yes, I think that makes a lot of sense. It might even be appropriate to make it the default authorizer?

@djc
Copy link
Contributor Author

djc commented Dec 17, 2020

Yes, I think that makes a lot of sense. It might even be appropriate to make it the default authorizer?

I'd certainly prefer it!

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Dec 17, 2020

Great, let's do that in a follow up PR. I'd like to get this one merged. As for whether or not it should be the default, I'd like to make that choice after I see what data is required to build that authorizer

@djc
Copy link
Contributor Author

djc commented Dec 17, 2020

Sounds good to me.

@Xaeroxe Xaeroxe merged commit 61b2198 into vivint-smarthome:master Dec 17, 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