-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[v2] SSOTokenLoader Expiration Check #9356
base: v2
Are you sure you want to change the base?
Conversation
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.
- I think it's still worth a changelog entry explaining the change in behavior.
- The Windows test failures look valid.
- Can you add a test case asserting that the expected error was thrown and that no calls were made to
GetRoleCredentials
?
…ase of an expired cached legacy token.
expiration = dateutil.parser.parse(token_dict['expiresAt']) | ||
remaining = total_seconds(expiration - self._time_fetcher()) | ||
if remaining <= 0: | ||
raise UnauthorizedSSOTokenError() |
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.
For reviewer:
On one hand, I think it's unfair to claim the token is "Unauthorized" without the server stating this, since the server is the source of truth on this.
On the other hand, if we use any other exception (e.g. a new ExpiredTokenException), then that would theoretically be a breaking change, since we're changing the exception in a code path that always (?) throws an exception.
Description of changes:
SSOTokenLoader
to check if the cached legacy token is expired according to the local clock. If expired, it will raise anUnauthorizedSSOTokenError
instead of sending an expired token to Identity Center'sGetRoleCredentials
API.UnauthorizedSSOTokenError
is raised.Description of tests:
aws sso login
.aws s3 ls --profile SSOProfile --debug
)UnauthorizedSSOTokenError
was raised, and checked the stacktrace (pasted below) to verify this exception was raised withoutGetRoleCredentials
being called.Stacktrace from manual workflow on this branch
Stacktrace from manual workflow on v2
Notice the above exception is raised due to a returned
UnauthorizedException
fromGetRoleCredentials
, while this is not the case in the previous stacktrace.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.