Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

feat: enable prompt if role not provided #165

Merged
merged 1 commit into from
Sep 24, 2019
Merged

feat: enable prompt if role not provided #165

merged 1 commit into from
Sep 24, 2019

Conversation

switj
Copy link
Contributor

@switj switj commented Jun 19, 2019

If the role_arn is provided, attempt to assume that role. No change to
current behaviour.

If role_arn is empty or not present in the profile and there is more than one
role then display the full list of roles the user can assume and prompt
them to choose the role to assume. The choice will be sticky for the
rest of the session for that profile. Whne the session ends they will be
prompted again to choose a role.

@switj
Copy link
Contributor Author

switj commented Jun 19, 2019

Hello aws-okta Admins,

this was a change that we needed and wanted to submit an upstream PR to see if this would be a useful change for everyone.

the new exec flow would look something like this:

$ aws-okta exec okta -- aws help
INFO[0010] Requesting MFA. Please complete two-factor authentication with your second device
INFO[0010] Select a MFA from the following list
INFO[0010] 0: GOOGLE (token:software:totp)
INFO[0010] 1: FIDO (u2f)
Select MFA method: 0

Enter MFA Code: <mfa_code>

0 - arn:aws:iam::<acct_id>:role/role1
1 - arn:aws:iam::<acct_id>:role/role2
2 - arn:aws:iam::<acct_id>:role/role3
3 - arn:aws:iam::<acct_id>:role/role4
Select Role to Assume: 1

This is a change from:

$ aws-okta exec okta -- aws help
Source profile must provide `role_arn`

In both cases I'm using the default okta profile that is created during setup. The existing behaviour of having a profile that includes role_arn is unchanged.

nickatsegment
nickatsegment suggested changes Jul 2, 2019
View reviewed changes
Copy link
Contributor

@nickatsegment nickatsegment left a comment

Choose a reason for hiding this comment

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

Looks sensible, except for the bonus refactoring :P

lib/util/prompt.go Outdated
@@ -1,4 +1,4 @@
package lib
package util
Copy link
Contributor

Choose a reason for hiding this comment

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

This refactoring seems orthogonal to the problem at hand? And while I appreciate the effort, it'd be best to leave it out, for compatibility's sake.

We really should refactor all this stuff into an internal package so we don't have to worry about compatibility.

lib/saml/roles.go Outdated Show resolved Hide resolved
lib/provider.go Outdated Show resolved Hide resolved
lib/saml/struct.go Outdated Show resolved Hide resolved
lib/saml/roles.go Outdated
}
return roleList.Roles[factorIdx].Role, roleList.Roles[factorIdx].Principal, nil
}
func (resp *Response) GetAssumableRolesFromSAML() (AssumableRoles, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry about doing this refactor here; it makes the actual purpose of the PR a little unclear.

@bazimov
Copy link

bazimov commented Aug 21, 2019

Love this feature, if it can be implemented. ❤️

@aadityasondhi
Copy link
Contributor

Looks sensible, except for the bonus refactoring :P

Fixed up the commit to remove the extra refactoring. Let me know what you think.

@nickatsegment

@aadityasondhi aadityasondhi mentioned this pull request Aug 29, 2019
Closed
@Shivang44
Copy link

@nickatsegment Mind merging this? Or are you waiting on more approvals? 🙂

@nickatsegment
Copy link
Contributor

@nickatsegment Mind merging this? Or are you waiting on more approvals? 🙂

Thanks for pinging me. I do need a second pair of eyes. Looks harmless but this codebase is getting hairy and hard to follow.

@nickatsegment
Copy link
Contributor

@switj I merged your other PR first because I feel more sure about it. Please rebase :)

feat: enable prompt if role not provided
373d137 
If the `role_arn` is provided, attempt to assume that role. No change to
current behaviour.

If `role_arn` is empty or not present in the profile and there is more than one
role then display the full list of roles the user can assume and prompt
them to choose the role to assume. The choice will be sticky for the
rest of the session for that profile. Whne the session ends they will be
prompted again to choose a role.
@switj
Copy link
Contributor Author

switj commented Sep 24, 2019

@nickatsegment rebase done. thanks for merging the other PR

Fauzyy
Fauzyy approved these changes Sep 24, 2019
View reviewed changes
Copy link
Member

@Fauzyy Fauzyy left a comment

Choose a reason for hiding this comment

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

LGTM!

@nickatsegment nickatsegment merged commit ee10242 into segmentio:master Sep 24, 2019
@switj switj deleted the switj/list-roles branch September 24, 2019 18:31
@Shivang44
Copy link

@nickatsegment @Fauzyy @switj Thanks so much for the quick response! What's usually the process for getting a new version into homebrew? Do we update this? https://github.com/Homebrew/homebrew-core/blob/master/Formula/aws-okta.rb

@nickatsegment
Copy link
Contributor

@nickatsegment @Fauzyy @switj Thanks so much for the quick response! What's usually the process for getting a new version into homebrew? Do we update this? Homebrew/homebrew-core:Formula/aws-okta.rb@master

Usually we just cut a release and a magical fairy does the rest :D I bet they'd accept a PR from you

I'll cut a release now

@nickatsegment nickatsegment changed the title enable prompt if role not provided feat: enable prompt if role not provided Sep 24, 2019
@Shivang44 Shivang44 mentioned this pull request Sep 25, 2019
Closed
@jacobwgillespie jacobwgillespie mentioned this pull request Oct 14, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nickatsegment nickatsegment nickatsegment approved these changes

@Fauzyy Fauzyy Fauzyy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@switj @bazimov @aadityasondhi @Shivang44 @nickatsegment @Fauzyy