Skip to content

feat(auth): allow consuming access tokens directly - #3642

Merged
alvarowolfx merged 15 commits into
googleapis:mainfrom
alvarowolfx:feat-auth-acess-token-direct
Nov 25, 2025
Merged

alvarowolfx merged 15 commits into
googleapis:mainfrom
alvarowolfx:feat-auth-acess-token-direct

Conversation

@alvarowolfx

@alvarowolfx alvarowolfx commented Oct 29, 2025 •

Copy link
Copy Markdown
Contributor

Fixes #2929 and #3362

@alvarowolfx

Copy link
Copy Markdown
Contributor Author

This is a WIP and is missing all documentation for the new interface.

Pros:

  • AccessTokenCredentials is compatible with Credentials type (via automatic conversion to CredentialProvider -> Credentials)
  • Reuse all current Builders and can selective pick which Credentials types would support that (we don't want API Key creds to support this)
    Cons:
  • I don't like the build_access_token_credentials() method name. Too long and not sure if is a good pattern to add another final branch to a Builder.
  • Introduces new AccessTokenCredentials which is almost the same as Credentials. Perhaps it can be simplified ?
  • AccessTokenCredentialsProvider looks really similar to TokenProvider, can this be reused ?
    • Probably not, Token is private and we need a new AccessToken type to expose that.

@codecov

codecov Bot commented Oct 29, 2025 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.03%. Comparing base (3388d78) to head (67e9d89).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/auth/src/credentials.rs 95.45% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3642   +/-   ##
=======================================
  Coverage   95.02%   95.03%           
=======================================
  Files         161      161           
  Lines        6049     6097   +48     
=======================================
+ Hits         5748     5794   +46     
- Misses        301      303    +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.

@coryan

coryan commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

I don't know what is the exact level of test coverage we want, but 46% is too low.

@alvarowolfx

Copy link
Copy Markdown
Contributor Author

I don't know what is the exact level of test coverage we want, but 46% is too low.

this is also missing tests, is just a draft for the public API.

@dbolduc dbolduc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, this approach makes a lot of sense to me. It is lamentable that we are already making backwards compatibility sacrifices in version 1.1.0 of a library, but that is where we are.

I think I messed up by making Credentials an opaque object instead of a trait. I think at the time I was afraid of all the generics that entailed... but now I realize that is just what flexible Rust looks like.

Comment thread src/auth/src/credentials.rs Resolved
Comment thread tests/integration-auth/src/lib.rs Resolved
Comment thread src/auth/src/credentials.rs Resolved
Comment thread src/auth/src/credentials.rs Resolved
@alvarowolfx
alvarowolfx marked this pull request as ready for review November 11, 2025 19:49
@alvarowolfx
alvarowolfx requested review from a team November 11, 2025 19:49
@alvarowolfx
alvarowolfx marked this pull request as draft November 11, 2025 20:10
@alvarowolfx
alvarowolfx marked this pull request as ready for review November 17, 2025 16:43
@coryan

coryan commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Marking as draft. If / when it is ready for review please let us know.

@coryan
coryan marked this pull request as draft November 20, 2025 22:26
@alvarowolfx

Copy link
Copy Markdown
Contributor Author

Marking as draft. If / when it is ready for review please let us know.

@coryan it was ready to review, but I think I'll split into smaller PRs to make it easier to review. Unless you think this is in a reasonable size.

@coryan
coryan marked this pull request as ready for review November 21, 2025 13:56
@coryan

coryan commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

@dbolduc can you take another look?

@dbolduc dbolduc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code looks really good, and I would symbolically approve the PR. I think my comments are mostly grammar nits.

I need an explicit answer on why we are abandoning Extensions / CacheableResource for the AccessToken before I can approve though.

Comment thread src/auth/src/credentials.rs Outdated
Ok(self.build_access_token_credentials()?.into())
}

/// Returns a [Credentials] instance with the configured settings.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: s/Credentials/AccessTokenCredentials/

Comment thread src/auth/src/credentials.rs Outdated
///
/// # Errors
///
/// Returns a [CredentialsError] if a unsupported credential type is provided

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: s/a unsupported/an unsupported/

Comment thread src/auth/src/credentials.rs Outdated
/// Returns a [CredentialsError] if a unsupported credential type is provided
/// or if the JSON value is either malformed
/// or missing required fields. For more information, on how to generate
/// json, consult the relevant section in the [application-default credentials] guide.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: s/json/the JSON for a credential/ ?

Ok(self.build_access_token_credentials()?.into())
}

/// Returns a [AccessTokenCredentials] instance with the configured settings.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: s/Returns a/Returns an/

Comment on lines +655 to +660
/// Returns a [BuilderError] if the `external_account_config`
/// provided to [`Builder::new`] cannot be successfully deserialized into the
/// expected format for an external account configuration. This typically happens if the
/// JSON value is malformed or missing required fields. For more information,
/// on the expected format, consult the relevant section in the
/// [external_account_credentials] guide.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: reflow?

we do not have any sort of tool. I just manully cut lines off at or before 80 characters. 🤷

Also consider a paragraph break before "For more information..."

Comment thread src/auth/src/credentials.rs Outdated
///
/// Returns a [CredentialsError] if a unsupported credential type is provided
/// or if the JSON value is either malformed
/// or missing required fields. For more information, on how to generate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

optional nit: paragraph break before "For more information..."

Comment thread src/auth/src/credentials.rs Resolved
@alvarowolfx

Copy link
Copy Markdown
Contributor Author

The code looks really good, and I would symbolically approve the PR. I think my comments are mostly grammar nits.

I need an explicit answer on why we are abandoning Extensions / CacheableResource for the AccessToken before I can approve though.

pushed some updates addressing the nits in the docs and also made some improvements so customers can mock the new trait

@alvarowolfx
alvarowolfx merged commit 37aef82 into googleapis:main Nov 25, 2025
29 checks passed
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.

Add "access token" method to credentials

3 participants