Custom token storage #146

djrodgerspryor · 2021-02-04T13:42:01Z

Related to: #145

Allow users to build their own token storage system by implementing the TokenStorage trait. This allows use of more secure storage mechanisms like OS keychains, encrypted files, or secret-management tools.

Custom storage providers are Box-ed to avoid adding more generics to the API — the indirection cost will only apply if using a custom store.

I've added anyhow to allow easy handling of a wide range of errors from custom storage providers.

As discussed in the linked issue, this approach uses boxing to extend the existing storage enum and minimise API changes.

The one significant API change is that Storage.set and thus Authenticator.token() now require &mut self, so caller will need to store the Authenticator with in a mutable variable.

ggriffiniii · 2021-02-04T16:03:44Z

Regarding the change to &mut self, I would be interested to hear the use case where that's useful, but it seems overly limiting to me. Prior to this change Autheticator::token could be called from multiple threads concurrently, but with this change that will no longer be the case. I suspect that api should continue to accept &self, especially since TokenStorage requires Send+Sync (which I agree it should). This will presumably require most impl's of TokenStorage use some form of interior mutability, but I would suspect they will already need to do that in order to satisfy the Send+Sync bound.

Allow users to build their own token storage system by implementing the `TokenStorage` trait. This allows use of more secure storage mechanisms like OS keychains, encrypted files, or secret-management tools. Custom storage providers are Box-ed to avoid adding more generics to the API — the indirection cost will only apply if using a custom store. I've added `anyhow` to allow easy handling of a wide range of errors from custom storage providers.

Instead, suggest using interior mutability (and RwLock in the example) to manage storage of token states. This makes it easier to share authenticators between threads.

djrodgerspryor · 2021-02-06T14:19:32Z

Yep, good call: interior mutability is a better fit. I've changed the API back to not requiring mutable references and used an RwLock in the example.

dermesser · 2021-02-16T15:21:54Z

This looks quite good to me, I asked @ggriffiniii to provide a review and then we can merge.

ggriffiniii · 2021-02-16T16:46:08Z

src/authenticator.rs

@@ -236,6 +237,14 @@ impl<C, F> AuthenticatorBuilder<C, F> {
        }
    }

+    /// Use the provided token storage mechanism
+    pub fn with_storage(self, storage_type: StorageType) -> Self {


I would consider if we would prefer to accept a Box<dyn TokenStorage> here rather than StorageType. There is already a way to choose a Disk storage with persist_tokens_to_disk and Memory is the default behavior so there isn't much to be gained by exposing StorageType at this time unless we feel like there's a potential for more variants in the future and we want to provide this more general purpose method now to eliminate the need for continuing to expand the number of methods for each variant.

I personally think it's probably unlikely that we'll need many more variants especially since the one being added here allows the utmost flexibility.

+1 - take the Box and wrap it into StorageType::Custom()

ggriffiniii · 2021-02-16T16:59:02Z

src/types.rs

@@ -56,7 +56,7 @@ impl From<TokenInfo> for AccessToken {
 /// It authenticates certain operations, and must be refreshed once
 /// it reached it's expiry date.
 #[derive(Clone, PartialEq, Debug, Deserialize, Serialize)]
-pub(crate) struct TokenInfo {
+pub struct TokenInfo {
    /// used when authenticating calls to oauth2 enabled services.
    pub(crate) access_token: String,


It probably doesn't make a lot of sense to keep these attributes private to the crate if this struct is now exposed. That just unnecessarily limits users to rely on the serde serialization.

I agree, public TokenInfo fields helped me debug things before, if the struct is public, let's make the fields public too :)

src/error.rs

ggriffiniii · 2021-02-16T17:05:19Z

src/storage.rs

+    /// Store a token for the given set of scopes so that it can be retrieved later by get()
+    /// ScopeSet implements Hash so that you can easily serialize and store it.
+    /// TokenInfo can be serialized with serde.
+    async fn set(&self, scopes: ScopeSet<'_, &str>, token: TokenInfo) -> anyhow::Result<()>;


I don't think I would make ScopeSet public, instead opting to just provide scopes: &[&str] for both get and set. ScopeSet is really designed to make the current implementation of Disk and Memory efficient and doesn't seem very widely applicable outside those contexts.

ggriffiniii · 2021-02-16T18:12:40Z

src/storage.rs

+            Storage::Custom(custom_storage) => {
+                let str_scopes: Vec<_> = scopes.scopes.iter().map(|scope| scope.as_ref()).collect();
+
+                (*custom_storage)


I think the deref of custom_storage is unnecessary.

ggriffiniii · 2021-02-16T18:12:45Z

src/storage.rs

+            Storage::Custom(custom_storage) => {
+                let str_scopes: Vec<_> = scopes.scopes.iter().map(|scope| scope.as_ref()).collect();
+
+                (*custom_storage)


dermesser · 2021-02-19T09:41:40Z

It seems that what's left here are relatively easily addressed comments, so I hope it will be ready for merge soon!

dermesser · 2021-02-28T06:46:37Z

@djrodgerspryor Hey Daniel, are you tired of all the nitpicking, or do you still have this PR in mind? :) Like I said, it's a really important feature, so I'd be inconsolable to see us having driven you away!

djrodgerspryor · 2021-03-01T09:13:05Z

The nitpicking is great 😄

I should have time to come back to this in a few days

chrisabruce · 2021-03-24T15:49:44Z

What's left to get this ready to merge?

Just pass `&[&str]` into custom storage providers. The scopeset struct has a range of unnecessary internal features. It's now also part of the interface for custom storage providers that the given scopes will be both unique and sorted. The only slightly awkward thing is that there's no conventient way to expose a `scopes_covered_by` helper method (which almost all custom storage engines will need), but it's still included in the example code.

For easier debugging, and for implementing custom storage solutions which might not use serde.

By only allowing a custom storage. To use one of the built-in storage mechanism, there is already a special-purpose `persist_tokens_to_disk` method available.

djrodgerspryor · 2021-03-25T10:22:38Z

@dermesser and @ggriffiniii all suggestions implemented: please take a look!

ggriffiniii

Overall, this looks good to me. Just a single nitpick about sort/uniq within get/set and a larger question for @dermesser that would require a public api change to avoid the unfortunate allocations/copies of the scopes slice when using custom token storage.

src/storage.rs

ggriffiniii · 2021-03-25T14:56:59Z

src/storage.rs

+                    .iter()
+                    .map(|scope| scope.as_ref())
+                    .sorted()
+                    .unique()


I wouldn't bother with sorting or unique here. Many implementations will not benefit from sort/uniq and those that do can do it within their TokenStorage::{get,set} without it being much more expensive than what's being done here.

I disagree: without this, the interface isn't well defined and it'll be easy for implementers to mess-up.

Eg. can get(['read', 'write']) return a different result from get(['write', 'read'])? What about get(['read', 'write', 'read'])?

Actually, my preference would probably be to give a Scopes struct which takes this invariance into account in its Eq and Hash implementations and could implement a helpful .covers method like the scopes_covered_by method in the example.

That won't help the allocation problem though.

giving unique keys from yup-oauth2 to custom storage providres seems nice towards implementers, at a small cost. So I'm ok with how it is at the moment :)

ggriffiniii · 2021-03-25T15:02:25Z

src/storage.rs

+                    .map(|scope| scope.as_ref())
+                    .sorted()
+                    .unique()
+                    .collect();


The allocation here is unfortunate. Comment for @dermesser: It may be worth considering changing the public API so that scope lists are passed as &[&str] rather than generic &[T] where T: AsRef<str>. I liked the latter when it was possible to do it without unnecessary overhead, but if it's causing the implementation to copy the scope list on every call to get a new token (as it now will when using a custom storage token) it's probably not worth the usability benefit.

well, let's be realistic: We are dealing with -- sometimes intercontinental -- network calls here, over a HTTP2 connection with JSON. And then possibly seconds for an API response that is tens to hundreds of times bigger than the scope strings.

A couple allocations for short strings are acceptable here, IMHO, given the nice properties of &[T] where T: AsRef<str>.

ggriffiniii · 2021-03-25T15:02:53Z

src/storage.rs

+                    .map(|scope| scope.as_ref())
+                    .sorted()
+                    .unique()
+                    .collect();


same comments from get() apply here

dermesser · 2021-04-01T20:53:29Z

I'm good with this and will merge it as beta -- if there are more improvements, we can deal with it in other PRs.

What do you think, is this ok as a sub-version bump (5.2), as it only extends enums as far as I can see -- or should it be v6.0?

djrodgerspryor mentioned this pull request Feb 4, 2021

Pluggable Storage #145

Open

djrodgerspryor added 2 commits February 6, 2021 22:59

Revert mutable authenticator interface change

384963e

Instead, suggest using interior mutability (and RwLock in the example) to manage storage of token states. This makes it easier to share authenticators between threads.

djrodgerspryor force-pushed the custom_storage_options branch from 1a76032 to 384963e Compare February 6, 2021 11:59

dermesser requested a review from ggriffiniii February 16, 2021 15:21

ggriffiniii reviewed Feb 16, 2021

View reviewed changes

dermesser mentioned this pull request Mar 4, 2021

Consider making Authenticator a Clone #133

Closed

djrodgerspryor added 4 commits March 25, 2021 21:08

Expose TokenInfo fields publically

fbf6c17

For easier debugging, and for implementing custom storage solutions which might not use serde.

Remove unnecessary deref

089543f

Simplify with_storage interface

6e4503f

By only allowing a custom storage. To use one of the built-in storage mechanism, there is already a special-purpose `persist_tokens_to_disk` method available.

ggriffiniii reviewed Mar 25, 2021

View reviewed changes

Remove now irrelevant comment about ScopeSets

e9facae

dermesser merged commit e63aa4b into dermesser:master Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom token storage #146

Custom token storage #146

djrodgerspryor commented Feb 4, 2021 •

edited

ggriffiniii commented Feb 4, 2021

djrodgerspryor commented Feb 6, 2021

dermesser commented Feb 16, 2021

ggriffiniii Feb 16, 2021

dermesser Feb 19, 2021

ggriffiniii Feb 16, 2021

dermesser Feb 19, 2021

ggriffiniii Feb 16, 2021

dermesser Feb 19, 2021

ggriffiniii Feb 16, 2021

ggriffiniii Feb 16, 2021

dermesser commented Feb 19, 2021

dermesser commented Feb 28, 2021

djrodgerspryor commented Mar 1, 2021

chrisabruce commented Mar 24, 2021

djrodgerspryor commented Mar 25, 2021

ggriffiniii left a comment

ggriffiniii Mar 25, 2021

djrodgerspryor Mar 26, 2021 •

edited

dermesser Apr 1, 2021

ggriffiniii Mar 25, 2021

dermesser Apr 1, 2021

ggriffiniii Mar 25, 2021

dermesser commented Apr 1, 2021

Custom token storage #146

Custom token storage #146

Conversation

djrodgerspryor commented Feb 4, 2021 • edited

ggriffiniii commented Feb 4, 2021

djrodgerspryor commented Feb 6, 2021

dermesser commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dermesser commented Feb 19, 2021

dermesser commented Feb 28, 2021

djrodgerspryor commented Mar 1, 2021

chrisabruce commented Mar 24, 2021

djrodgerspryor commented Mar 25, 2021

ggriffiniii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrodgerspryor Mar 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dermesser commented Apr 1, 2021

djrodgerspryor commented Feb 4, 2021 •

edited

djrodgerspryor Mar 26, 2021 •

edited