Make the public API more modular #49

djc · 2022-01-24T12:04:24Z

This does way with all top-level functions except init(), while making AuthenticationManager::new() (effectively the same as init()) and the CustomServiceAccount type public. The latter can be constructed directly with from_env(), from_file() and from_json() callers, and can easily be converted to an AuthenticationManager. It also gives access to some of its inner credentials which I think will be useful.

I've also switch the logging provider from log to tracing and gave CustomServiceAccount callers access to its signer.

@flub any thoughts?

…tionManager

…:new()

Given that this library makes use of async code, it's useful to use spans.

flub

The new API to let you create structs directly feels natural, though it is weird to single out the CustomServiceAccount as the only public one. Maybe all should be made public? That way someone could also say use only the DefaultServiceAccount or something like that.

flub · 2022-01-25T21:43:28Z

src/authentication_manager.rs

@@ -21,10 +25,66 @@ pub struct AuthenticationManager {
 }

 impl AuthenticationManager {
-    pub(crate) fn new(client: HyperClient, service_account: Box<dyn ServiceAccount>) -> Self {
+    /// Create an `AuthenticationManager` directly from a custom service account
+    pub fn from_custom_service_account(service_account: CustomServiceAccount) -> Self {


is it customary to provide this as well as a From impl? Not that I have any problem with this, just wondering if there is a Rust-wide consensus on this.

Yeah, I was maybe overdoing it on the ergonomics. I've found that often From implementations aren't very easily discoverable, and in my mind gcp_auth is about making this complicated stuff really easy; but maybe we should double down on There's Only One Way To Do It and we should ditch this method in favor of the From impl.

The discoverability is fair, but easily solvable by showing it in the struct docs, maybe even with an example.

That's fair. I'm interested what @hrvolapeter thinks.

I think both arguments valid. I was trying to find if some other library is having from trait and function as well but wasn't able to find anything. Adding public function is easier then removing it so I would probably just start with From trait and if people are confused we can consider improving docs / making function explicit

flub · 2022-01-25T21:48:09Z

src/lib.rs

 /// Initialize GCP authentication
 ///
-/// Returns `AuthenticationManager` which can be used to obtain tokens
+/// This is a convenience wrapper around [`AuthenticationManager::new()`].


If you don't mind breaking the API why not completely remove this init and rely only on AuthenticationManager::new?

I personally like the shortcut of being able to do gcp_auth::init() without doing any imports, especially in helper CLIs. Seems like a cheap way to get a slicely nicer UX.

I have no strong opinions and recognise this is entirely bikeshedding, so if you like to keep both that's perfectly fine. Though in rust I did get very used to the SomeStruct::constructor() way over an init shortcut and when I have the option (I think tracing_subscriber::fmt() is another example) I will still choose the constructor with the struct name in it as it is more descriptive to me to see the struct name being created.

Interesting data point! I think I rather like the import-less initialization aspect to me but I will recognize it's less idiomatic then calling a type constructor.

Again, since @hrvolapeter came up with this API initially I'll let them weigh in before I make any further changes.

the whole init think is probably just a habit from university, we've been taught that constructor shouldn't have side effects due to weird error handling in C++ constructors which is not really a valid point in rust anymore. init for me is basically constructor with side effects, I think similar approach is quite common in OOP although it would be constructed in 2 steps. I agree that it's probably not the most rust native way.

flub · 2022-01-25T21:59:27Z

src/custom_service_account.rs

+    }
+
+    /// The project ID as found in the credentials
+    pub fn project_id(&self) -> Option<&str> {


it seems odd to me to provide two public functions with the same name on the same struct. I realise this is a slightly different signature that the ServiceAccount trait, but this seems hardly worth it?

The ServiceAccount trait is pub(crate), so that implementation is actually not public.

flub · 2022-01-25T21:59:38Z

src/custom_service_account.rs

            tokens: RwLock::new(HashMap::new()),
        })
    }
+
+    /// The RSA PKCS1 SHA256 [`Signer`] used to sign JWT tokens
+    pub fn signer(&self) -> &Signer {


why make the signer public?

So there's a use case here: ThouCheese/cloud-storage-rs#92. This is for the https://cloud.google.com/storage/docs/access-control/signed-urls feature, which uses the same encryption mechanism as far as I can tell.

More broadly, this is basically code that we already have in gcp_auth, so we might as well expose it to users as an easy API -- they might also want to produce JWTs.

djc · 2022-01-25T23:08:10Z

The new API to let you create structs directly feels natural, though it is weird to single out the CustomServiceAccount as the only public one. Maybe all should be made public? That way someone could also say use only the DefaultServiceAccount or something like that.

We could definitely do the others. Personally I find the CustomServiceAccount implementation more interesting because it can provide access to the Signer.

hrvolapeter

The proposed changes look good. All open discussions seems reasonable in both ways

…of documentation

djc · 2022-02-01T09:38:45Z

I removed init() and AuthenticationManager::from_custom_service_account() for now, we could always add them back later. @hrvolapeter shall I publish 0.7 to crates.io?

hrvolapeter · 2022-02-01T11:48:29Z

Yes please do

djc added 19 commits January 24, 2022 11:33

Clean up unintentionally added .DS_Store file

cf0ffba

Simplify parsing CustomServiceAccount setup

148eb79

Remove unused Error variants

a0df627

Move initialization of AuthenticationManager into associated function

d5c543a

Make GCloudAuthorizedUser::new() synchronous

a73537c

Use matching to avoid unwrapping errors

0e6fd5a

Allow non-UTF-8 paths in GOOGLE_APPLICATION_CREDENTIALS

ee892c3

Extract initialization of HyperClient into function

8976520

Clarify internal API for turning CustomServiceAccount into Authentica…

4240558

…tionManager

Move checking environment for credentials into AuthenticationManager:…

9da6f85

…:new()

Rename AuthenticationManager::select() to new()

8076fdb

Make AuthenticationManager::new() public and document it

11c1d9e

Make CustomServiceAccount public

f95113c

Add methods to access custom service account credentials

ed6203b

Rename TLSError variant to TlsError

a220fad

Deserialize user credentials directly from file

a4da35a

Add code to support project_id() in GCloud tokens

467c076

Rewrite code to be easier to follow

b7c0fd5

Bump version for API changes

8b8c07e

djc mentioned this pull request Jan 24, 2022

Add from_credentials_json function. #48

Merged

djc added 9 commits January 24, 2022 13:51

Inline decode_rsa_key() function into JwtSigner::new()

4c688ca

Sort dependencies

c29a7df

Use ring instead of rustls wrappers for signing

6c2c460

Move JWT-specific parts of signing into Claims method

c0dc296

Move Signer from jwt into types

562795d

Reuse signer for the lifetime of the CustomServiceAccount

88ae957

Expose API access to the CustomServiceAccount's Signer

b62d18b

Switch from log to tracing

3963a98

Given that this library makes use of async code, it's useful to use spans.

Instrument core code paths with tracing spans

b312228

djc force-pushed the decouple branch from f70af7a to b312228 Compare January 24, 2022 13:43

flub reviewed Jan 25, 2022

View reviewed changes

djc mentioned this pull request Jan 25, 2022

Consider deferring authentication to another crate ThouCheese/cloud-storage-rs#92

Open

hrvolapeter approved these changes Jan 31, 2022

View reviewed changes

djc added 2 commits February 1, 2022 10:28

Remove init() wrapper

7e202ee

Remove AuthenticationManager::from_custom_service_account() in favor …

3c6185d

…of documentation

djc merged commit a3edad5 into master Feb 1, 2022

djc deleted the decouple branch February 1, 2022 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the public API more modular #49

Make the public API more modular #49

djc commented Jan 24, 2022 •

edited

Loading

flub left a comment

flub Jan 25, 2022

djc Jan 25, 2022

flub Jan 31, 2022

djc Jan 31, 2022

hrvolapeter Jan 31, 2022

flub Jan 25, 2022

djc Jan 25, 2022

flub Jan 31, 2022

djc Jan 31, 2022

hrvolapeter Jan 31, 2022 •

edited

Loading

flub Jan 25, 2022

djc Jan 25, 2022

flub Jan 25, 2022

djc Jan 25, 2022

djc commented Jan 25, 2022

hrvolapeter left a comment

djc commented Feb 1, 2022

hrvolapeter commented Feb 1, 2022

Make the public API more modular #49

Make the public API more modular #49

Conversation

djc commented Jan 24, 2022 • edited Loading

flub left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrvolapeter Jan 31, 2022 • edited Loading

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

djc commented Jan 25, 2022

hrvolapeter left a comment

Choose a reason for hiding this comment

djc commented Feb 1, 2022

hrvolapeter commented Feb 1, 2022

djc commented Jan 24, 2022 •

edited

Loading

hrvolapeter Jan 31, 2022 •

edited

Loading