Skip to content

Conversation

alvarowolfx
Copy link
Collaborator

@alvarowolfx alvarowolfx marked this pull request as ready for review October 14, 2025 20:31
@alvarowolfx alvarowolfx requested review from a team as code owners October 14, 2025 20:31
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 95.06173% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.67%. Comparing base (5bf695d) to head (3381285).
⚠️ Report is 44 commits behind head on main.

Files with missing lines Patch % Lines
src/auth/src/credentials/idtoken.rs 92.30% 3 Missing ⚠️
src/auth/src/credentials/internal/jwk_client.rs 97.61% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3516      +/-   ##
==========================================
- Coverage   95.78%   95.67%   -0.11%     
==========================================
  Files         120      125       +5     
  Lines        4820     4999     +179     
==========================================
+ Hits         4617     4783     +166     
- Misses        203      216      +13     

☔ 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.


impl Verifier {
/// Sets the audience for the token verification.
pub fn with_audience<S: Into<String>>(mut self, audience: S) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a bit odd that I can set the audience, but not clear the audience.

Should we separate this to a builder and once built we disable all functions to change the audience, email or clock skew?

Comment on lines +109 to +113
let response = client
.get(jwks_url)
.send()
.await
.map_err(|e| crate::errors::from_http_error(e, "failed to fetch JWK set"))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a retry loop? Do we need to cache the reqwest client (not if these calls are rare)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, adding retries is a good call, but I was planning to add retries to all id token related features later, to reduce the amount of things to be reviewed first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then file a bug (maybe a sub-issue) and add a comment like // TODO(#...) - add a retry loop.

Copy link
Collaborator Author

@alvarowolfx alvarowolfx left a comment

Choose a reason for hiding this comment

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

addressed some of the review comments.

Comment on lines +109 to +113
let response = client
.get(jwks_url)
.send()
.await
.map_err(|e| crate::errors::from_http_error(e, "failed to fetch JWK set"))?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, adding retries is a good call, but I was planning to add retries to all id token related features later, to reduce the amount of things to be reviewed first.

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.

3 participants