Skip to content

Comments

feat: Implement refreshCredentialsIfRequired for intermediate token r…#1583

Merged
nbayati merged 19 commits intogoogleapis:client-side-cabfrom
nbayati:refresh-if-required
Jan 2, 2025
Merged

feat: Implement refreshCredentialsIfRequired for intermediate token r…#1583
nbayati merged 19 commits intogoogleapis:client-side-cabfrom
nbayati:refresh-if-required

Conversation

@nbayati
Copy link
Contributor

@nbayati nbayati commented Nov 23, 2024

This PR implements refreshCredentialsIfRequired which will be called by generateToken(). The refresh logic was implemented based on the design doc, which takes into account the refreshMargin and minimumTokenLifetime, and decides on either async or synchronous refresh if one is needed.

Notes:

  • This PR merges the changes to a feature branch and not the main branch.
  • Unit tests to follow. 

@nbayati
feat: Implement refreshCredentialsIfRequired for intermediate token r…
eb4b793 
…efresh

Implement `refreshCredentialsIfRequired`, called by `generateToken()`, to handle token refresh. It uses `refreshMargin` and `minimumTokenLifetime` to decide on synchronous or asynchronous refresh
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Nov 23, 2024
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 23, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 3, 2024
@nbayati
Improve concurrency handling during credential refresh.
47f6319 
Introduced a refresh task to manage concurrent refresh requests, preventing redundant attempts and potential race conditions. This aligns the refresh mechanism with the pattern used in OAuth2Credentials and ensures more robust credential management.
@nbayati nbayati requested a review from lqiu96 December 10, 2024 00:45
@nbayati nbayati marked this pull request as ready for review December 10, 2024 00:49
@nbayati nbayati requested a review from a team December 10, 2024 00:49
@nbayati nbayati requested a review from a team as a code owner December 10, 2024 00:49
@nbayati nbayati requested a review from aeitzman December 10, 2024 00:50
@nbayati nbayati requested a review from lqiu96 December 11, 2024 00:45
lqiu96
lqiu96 approved these changes Dec 11, 2024
View reviewed changes
Copy link
Member

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Added a few smaller comments but the overall flow is similar to Oauth2Credentials

@huangjiahua huangjiahua mentioned this pull request Dec 12, 2024
Merged
4 tasks
aeitzman
aeitzman approved these changes Dec 12, 2024
View reviewed changes
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 19, 2024
lqiu96
lqiu96 reviewed Dec 20, 2024
View reviewed changes
.../com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java Outdated
Comment on lines 296 to 298
// After the delay, the request count should be 2, indicating that the async refresh has made
// a single request to the STS endpoint.
assertEquals(2, mockStsTransportFactory.transport.getRequestCount());
Copy link
Member

@lqiu96 lqiu96 Dec 20, 2024

Choose a reason for hiding this comment

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

qq, why is this 2 and not 4 as similar to BLOCKING?

Copy link
Member

@lqiu96 lqiu96 Dec 20, 2024

Choose a reason for hiding this comment

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

Oh wait, is it because we wait for the first async thread to finish? If so, can we add a comment that explain that

Copy link
Contributor Author

@nbayati nbayati Dec 26, 2024

Choose a reason for hiding this comment

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

Correct. The async calls will re-use the same refreshTask, so only 1 additional call to the STS is made. I've updated the comment to explain that.

@nbayati nbayati requested a review from a team as a code owner December 20, 2024 19:28
lqiu96
lqiu96 approved these changes Dec 27, 2024
View reviewed changes
Copy link
Member

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM

@nbayati nbayati merged commit 0d96dcf into googleapis:client-side-cab Jan 2, 2025
11 of 12 checks passed
lqiu96 pushed a commit that referenced this pull request Feb 4, 2025
@nbayati @huangjiahua @aeitzman
feat: Introduce Client-Side Credential Access Boundary (CAB) function…
f481123 
…ality (#1629)

* feat: Implement ClientSideCredentialAccessBoundaryFactory (#1562)

* feat: Implement ClientSideCredentialAccessBoundaryFactory.refreshCredentials()

Set up the ClientSideCredentialAccessBoundaryFactory class and module.
Implement the function to fetch and refresh intermediary tokens from STS.

* feat: Add the generated ClientSideAccessBoundaryProto class for Client-Side CAB feature. (#1571)

Change-Id: Ic7ef3cbd80b2ad778d61b9ccabf780561d3cc709

* feat: Implement refreshCredentialsIfRequired for intermediate token r… (#1583)

* feat: Implement refreshCredentialsIfRequired for intermediate token refresh

Implement `refreshCredentialsIfRequired`, called by `generateToken()`, to handle token refresh. It uses `refreshMargin` and `minimumTokenLifetime` to decide on synchronous or asynchronous refresh

* Add unit tests for the builder and refreshCredentials()

* Improve concurrency handling during credential refresh.

Introduced a refresh task to manage concurrent refresh requests, preventing redundant attempts and potential race conditions. This aligns the refresh mechanism with the pattern used in OAuth2Credentials and ensures more robust credential management.

* Update existing unit tests for compatibility and readability.

* Add unit tests for refreshCredentialsIfRequired.

* Fix a merge issue.

* Temporary add sonatype-snapshots repository and cel version to fix the build error.

* Remove duplicated code.

* Fix lint issue.

* Fix: Propagate credential refresh exceptions in blocking refresh.

* Change cel version

* Change cel version

* Add jsr305 dependency

* Fix Javadoc error

* Minor code readability enhancements.

* Revert "Fix Javadoc error"

This reverts commit 2157fdb.

* Address comments (add javadoc and use assertThrows in tests)

* Run format script

* feat: Implement Client-Side CAB token generation.  (#1598)

* feat: Implement Client-Side CAB token generation.

Change-Id: I2c217656584cf5805297f02340cbbabca471f609

* Use IllegalStateException(String, Throwable) to capture upstream exception during Tink initialization

Change-Id: I12af5b84eae4dcec5865adfdad1f9396d54c0200

* Rethrow exceptions from tink and CEL

Change-Id: If8c94c786ee39201029d9c27856fd2eafb61e51c

* Add tests for invalid keys from upstream, and rename test cases.

Change-Id: Ib41cb81c779534fc6efd74d66bf4728efd743906

* Add additional throws comment for generatToken method.

Change-Id: I9cfc589ade8a91040fc9c447740493fd49e392af

* Refactor tests for better readability.

Change-Id: Icfd0bc24c1694f220bcbffc6cde41462c59119c4

* Catch and rethrow the exception of session key not being base64 encoded.

Change-Id: I5fa0c25fe020e9612735e4ac5df2b85a2a5aab11

* Format the code using mvn com.coveo:fmt-maven-plugin:format.

Change-Id: I46572488dcd28de450a6b1b2f732bee5baa86910

* Fix a typo in the javadoc comment.

Change-Id: Icef9ef5f7c3567224ec507303543b78e61f43ec1

* chore: Update version tag in cab-token-generator pom.xml

This commit updates the version tag in the pom.xml file.

* feat: Add integration test for the client side cab

* Remove volatile keyword and use refreshLock when reading intermediateCredentials.

* Define new default values for refreshMargin and minimumTokenLifetime.

* Update version in pom.xml

* Run formatter to resolve lint errors

* add missing dependency

* Swap the assertEquals parameters so the expected value is first.

* Docs: Added javadocs

Improvements: Cleaned up code, resolved readability enhancements

---------

Co-authored-by: Jiahua Huang <jh@jiahuah.com>
Co-authored-by: aeitzman <12433791+aeitzman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lqiu96 lqiu96 lqiu96 approved these changes

+1 more reviewer

@aeitzman aeitzman aeitzman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size: xl Pull request size is extra large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nbayati @lqiu96 @aeitzman