-
Notifications
You must be signed in to change notification settings - Fork 15
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
[pr] Okta Challenge Preferences #93
base: master
Are you sure you want to change the base?
Conversation
As I had issues with Clisso not selecting push notifications but require me to enter codes instead of using push, even though I only had okta push turned on, I added this functionality, so we prefer push when possible but make it configurable for whoever is into doing it. Worked well here so far in different scenarios.
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.
✅ A review job has been created and sent to the PullRequest network.
@sandrom you can click here to see the review status or cancel the code review job.
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.
} `json:"_embedded"` | ||
} | ||
|
||
type factor struct { |
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.
@@ -43,6 +44,7 @@ func init() { | |||
|
|||
// Okta | |||
cmdProvidersCreateOkta.Flags().StringVar(&baseURL, "base-url", "", "Okta base URL") | |||
cmdProvidersCreateOkta.Flags().StringArrayVar(&challenges, "challenges", []string{"push", "token:software:totp"}, "Challenges to be used with preference") |
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.
@@ -23,6 +23,7 @@ var providerDuration int | |||
|
|||
// Okta |
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.
@@ -163,3 +173,34 @@ func Get(app, provider string, duration int64) (*aws.Credentials, error) { | |||
|
|||
return creds, err | |||
} | |||
|
|||
func typeMapOf(factors []factor) (m map[string]factor) { |
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 code all looks good, but I don't see any accompanying unit tests. I strongly recommend adding these to protect the code against future changes that could unintentionally break things; and it may also be a good clean place to start with that if you haven't already as there aren't many dependencies.
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.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard
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 pull request should be reviewed by PullRequest because the title contains "[pr]"; however since review has been cancelled, it can only be re-sent manually from the PullRequest dashboard.
|
As I had issues with Clisso not selecting push notifications but require me to enter codes instead of using push, even though I only had okta push turned on, I added this functionality, so we prefer push when possible but make it configurable for whoever is into doing it. Worked well here so far in different scenarios.