Skip to content

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

Closed
wants to merge 18 commits into from

Conversation

harkamaljot
Copy link
Collaborator

@harkamaljot harkamaljot commented Jun 21, 2025

As part of allowing the user to provide their own subject token provider, this PR makes following changes:

  1. Make SubjectTokenProvider trait public.
  2. A new builder ProgrammaticSourcedCredentials is created to utilize this trait and a corresponding test case has been added.

In future PR, more fields will be added to the builder for ProgrammaticSourcedCredentials

@harkamaljot harkamaljot requested a review from a team as a code owner June 21, 2025 03:02
@harkamaljot harkamaljot marked this pull request as draft June 21, 2025 03:02
Copy link

codecov bot commented Jun 21, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.48%. Comparing base (c4f3439) to head (cab006f).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
src/auth/src/credentials/external_account.rs 90.32% 3 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@harkamaljot harkamaljot changed the title draft(auth): make subject token provider public feat(auth): expose SubjectTokenProvider Trait Jun 21, 2025
@harkamaljot harkamaljot marked this pull request as ready for review June 21, 2025 06:28
@harkamaljot harkamaljot marked this pull request as draft June 23, 2025 01:03
@harkamaljot harkamaljot marked this pull request as ready for review June 23, 2025 02:56
@harkamaljot harkamaljot changed the title feat(auth): expose SubjectTokenProvider Trait feat(auth): update SubjectTokenProvider Trait Jun 23, 2025
@harkamaljot harkamaljot marked this pull request as draft June 23, 2025 17:48
@harkamaljot harkamaljot changed the title feat(auth): update SubjectTokenProvider Trait feat(auth): Add ProgrammaticSourcedCredentials Jun 24, 2025
@harkamaljot harkamaljot requested a review from sai-sunder-s June 24, 2025 06:05
@harkamaljot harkamaljot marked this pull request as ready for review June 24, 2025 06:05
/// [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,
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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>,
Copy link
Contributor

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.

Copy link
Contributor

@sai-sunder-s sai-sunder-s Jun 25, 2025

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.

Copy link
Collaborator Author

@harkamaljot harkamaljot Jun 25, 2025

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

  1. create a new CredentialSource i.e. CredentialSource::Programmatic
  2. 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
  3. ProgrammaticBuilder::Build method now calls the make_credentials rather than calling the make_credentials_from_source method?

@sai-sunder-s sai-sunder-s requested a review from alvarowolfx June 25, 2025 00:50
@harkamaljot harkamaljot requested a review from sai-sunder-s June 25, 2025 17:52
@harkamaljot harkamaljot marked this pull request as draft June 26, 2025 01:21
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.

2 participants