Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add token refresh support to SSOCredentialProvider #1903
Add token refresh support to SSOCredentialProvider #1903
Changes from all commits
8d09ba9
fce3a5f
5eb9d79
edb4d43
db6bfac
6118eb4
ca45169
fe74417
02d654c
6d34685
cf28002
c210422
7607b30
a6d628f
ef79153
f374d28
54f2645
a4c2dd8
f4fb486
2ae06ad
3a82c79
2f0c3b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
optFns
does not seem to be used anywhere in this 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.
looks like this should of bee used as input for
ssoidc.NewFromConfig
?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 believe these are function options to be passed to the token provider constructor function actually.
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.
This is a good catch. Because
optFns
are*ssocreds.SSOTokenProviderOptions
, it cannot be passed into thessooidc
constructor and must be passed into the token provider constructor. This means that instead of adding a new field to the ssoOptions
and passing thessoidc
client as an option into thesso
constructor:I must make the added field a
*SSOTokenProvider
and pass that as an option into thesso
constructorIll push a new commit to this PR with that change.
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.
nit: put
oidcClient := ssooidc.NewFromConfig(cfgCopy)
at the top of this comment block to keep with other validation done forTokenClient
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.
hmm im a bit confused at this requested change.
this validation:
isnt operating on any input related to
oidcClient := ssooidc.NewFromConfig(cfgCopy)
additionally, the cached path conditional checks to see if the existing token is valid before creating an oidc client. if the existing token is not valid, then there is not reason to create an oidc client, so it should happen after
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.
Make sure this function that was removed is not referenced anywhere else.
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.
Since this is package private there is no concern for this being removed. If something else requires it within the package, then it wouldn't compile or pass tests.
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.
note: validation in general has become stricter for both the legacy and current format.
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.
Can both exist within the same profile, and if so is there precedence? As that would change how you would want this validation to work.
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.
Yes - both could exist in one profile (its unlikely, but someone who is migrating from legacy to new could have such a setup). If so - the new format should be used, and the sso_start_url and sso_region MUST match (ie, if there is a mismatch between the profile's sso_region and the sso-session's, then we should raise an error).
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.
@skmcgrail yes both can exist in the same profile. i intentionally check to see if the new configuration is present and valid. if it is valid, then it should be used and if it is not valid, then it should error (even if the legacy configuration is present and valid). is there another scenario in mind you might be thinking about thats not covered here?
also, regarding @alextwoods comment about matching
sso_start_url
andsso_region
, i do that additional check here https://github.com/aws/aws-sdk-go-v2/pull/1903/files#diff-ad732dd8295a0ce04a495056007062e13a140b7f97f173004d0d6523545805d2R1120There 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.
nit: add back newline