-
Notifications
You must be signed in to change notification settings - Fork 1.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
Handle sso session names with quotes/spaces #2895
Conversation
You have made a change to core without a corresponding change to the CHANGELOG.md. This change will not result in a new version and will not published unless an entry is added to CHANGELOG.md |
"Referenced by profile #{profile}" | ||
end | ||
|
||
unless sso_session['sso_region'] |
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.
sso_credentials_from_profile
is not validating sso_region existing - SSO_CREDENTIAL_PROFILE_KEYS does not require sso_region. There is a branch that checks if it exists on profile and compares it to sso_region if present. I think this refactor is maybe breaking?
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.
The SSOCredentials will fail if sso_region is unset, so this moves the failure case up to here (with hopefully slightly. more info).
def sso_session(cfg, profile, sso_session_name) | ||
sso_session = cfg["sso-session #{sso_session_name}"] | ||
|
||
if sso_session.nil? && sso_session_name.match(/\s/) |
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.
You can possible just add an || cfg["sso-session '#{sso_session_name}'"]
after cfg["sso-session #{sso_session_name}"]
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.
Yeah, we can skip the check for it containing whitespace and always use this check - I think thats reasonable and forwards compatible.
@@ -389,7 +385,7 @@ def sso_credentials_from_profile(cfg, profile) | |||
sso_role_name: prof_config['sso_role_name'], | |||
sso_session: prof_config['sso_session'], | |||
sso_region: sso_region, | |||
sso_start_url: prof_config['sso_start_url'] | |||
sso_start_url: sso_start_url |
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 change should be safe since the non-legacy flow does NOT use the sso_start_url, see: https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-core/lib/aws-sdk-core/sso_credentials.rb#L82
Thank you - that indeed solves the issue I was having in #2894 |
Fixes #2894
The
aws configure sso
command will add quotes to sso-session names with whitespace. Eg:This PR adds flexible support for loading those sso sessions (first checking for unquoted and then looking for the quoted version).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the
CHANGELOG.md
file (at corresponding gem). For the description entry, please make sure it lives in one line and starts withFeature
orIssue
in the correct format.For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md
Thank you for your contribution!