Skip to content

Add a configurable option to set the duo mfa option - #259

Merged
wolfeidau merged 3 commits into
Versent:masterfrom
srizzling:duo-mfa-option
Feb 6, 2019
Merged

Add a configurable option to set the duo mfa option#259
wolfeidau merged 3 commits into
Versent:masterfrom
srizzling:duo-mfa-option

Conversation

@srizzling

Copy link
Copy Markdown
Contributor

This change adds a configurable option so that users can set a flag to configure their prefered duo option (Push or Passcode). This is useful for users, who setup aliases, and want a swift and easy approach to login into the accounts.

@pwmcintyre pwmcintyre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very happy that this is a feature don't is working on, but I do have concerns and some feedback, please continue!

Comment thread README.md Outdated
Comment thread pkg/creds/creds.go
Password string
MFAToken string
DuoMFAOption string
URL string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like to think this could be just MFAOption, leaving it open to be implemented on other providers?

Comment thread pkg/provider/okta/okta.go
duoMfaOption := prompter.Choose("Select a DUO MFA Option", duoMfaOptions)
duoMfaOption := 0

if loginDetails.DuoMFAOption == "Duo Push" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is safe, I don't think there's any certainty of the order - you may need to iterate over each and compare

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I assumed it was OK, since the code previously assumed an order (and since the array is declared just above)

I could iterate to be make it safer however.

@wolfeidau wolfeidau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am OK with the solution in the current state.

Really need to refactor that entire provider.

@wolfeidau
wolfeidau merged commit 8701093 into Versent:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants