-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: adjust IAM token expiration time #268
Conversation
auth/token-managers/token-manager.ts
Outdated
@@ -229,7 +229,7 @@ export class TokenManager { | |||
* Checks if currently-stored token should be refreshed | |||
* i.e. past the window to request a new token | |||
*/ | |||
private tokenNeedsRefresh(): boolean { | |||
protected tokenNeedsRefresh(): boolean { |
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.
technically, I didn't need to override tokenNeedsRefresh()
but it didn't seem right leaving it as the only private function :)
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.
On looking at it, I'm of the mind that we should only expose things when necessary to prevent expanding our public maintenance burden - even if the private function is lonely 🙂 But I'm fine with this going either way.
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.
Looks good but I want to check on the isTokenExpired
implementation in the VPC Instance TM before approving.
auth/token-managers/token-manager.ts
Outdated
@@ -229,7 +229,7 @@ export class TokenManager { | |||
* Checks if currently-stored token should be refreshed | |||
* i.e. past the window to request a new token | |||
*/ | |||
private tokenNeedsRefresh(): boolean { | |||
protected tokenNeedsRefresh(): boolean { |
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.
On looking at it, I'm of the mind that we should only expose things when necessary to prevent expanding our public maintenance burden - even if the private function is lonely 🙂 But I'm fine with this going either way.
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.
LGTM!
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.
Looks good! 👍
This commit changes the IAM, Container and VPC Instance authenticators slightly so that an IAM access token will be viewed as "expired" when the current time is within 10 seconds of the official expiration time. IOW, we'll expire the access token 10 secs earlier than the IAM server-computed expiration time. We're doing this to avoid a scenario where an IBM Cloud service receives a request along with an "almost expired" access token and then uses that token to perform downstream requests in a somewhat longer-running transaction and then the access token expires while that transaction is still active. Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
## [4.2.4](v4.2.3...v4.2.4) (2024-02-28) ### Bug Fixes * adjust IAM token expiration time ([#268](#268)) ([9b975e0](9b975e0))
🎉 This PR is included in version 4.2.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This commit changes the IAM, Container and VPC Instance authenticators slightly so that an IAM access token will be viewed as "expired" when the current time is within 10 seconds of the official expiration time. IOW, we'll expire the access token 10 secs earlier than the IAM server-computed expiration time.
We're doing this to avoid a scenario where
an IBM Cloud service receives a request along
with an "almost expired" access token and then uses that token to perform downstream requests in a
somewhat longer-running transaction and then the
access token expires while that transaction is
still active.
Checklist
npm test
passes (tip:npm run lint-fix
can correct most style issues)