-
Notifications
You must be signed in to change notification settings - Fork 60
feat(auth): Add ProgrammaticSourcedCredentials #2489
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
- Coverage 95.51% 95.48% -0.03%
==========================================
Files 79 80 +1
Lines 3185 3214 +29
==========================================
+ Hits 3042 3069 +27
- Misses 143 145 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/// [external_account_credentials]: https://google.aip.dev/auth/4117#configuration-file-generation-and-usage | ||
pub fn new_with_external_account_config<S: Into<Value>>( | ||
subject_token_provider: T, | ||
external_account_config: S, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since they are already providing part of the config through code, we should provide them a way to build the full credential through code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I plan to do that in the next PR, as mentioned in the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add all the required builder methods as part of this PR. Lets do the method for loading from a config later. We can add that from config one if we get customer feedback that it is needed.
@@ -76,7 +72,7 @@ struct ExternalAccountFile { | |||
client_id: Option<String>, | |||
client_secret: Option<String>, | |||
scopes: Option<Vec<String>>, | |||
credential_source: CredentialSourceFile, | |||
credential_source: Option<CredentialSourceFile>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of making optional here, it would be better to create another struct just for programmatic and do not include credential_source in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a ProgrammaticExternalAccountFile
struct which doesn't have credential_source, then you can combine that with the custom subject token provider and build the ExternalAccountConfig.
I believe that will make the code nicer, you do not have to pass source separately to make_credentials and pull it out of ExternalAccountConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't follow this comment do you mean to say do the following
- create a new CredentialSource i.e. CredentialSource::Programmatic
- Now we do not remove CredentialSource from the ExternalAccountConfig but rather, when ProgrammaticExternalAccount.into() is called it automatically converts to ExternalAccountConfig with source set as CredentialSource::Programmatic
- ProgrammaticBuilder::Build method now calls the make_credentials rather than calling the make_credentials_from_source method?
As part of allowing the user to provide their own subject token provider, this PR makes following changes:
In future PR, more fields will be added to the builder for ProgrammaticSourcedCredentials