Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[pr] Okta Challenge Preferences #93

wants to merge 1 commit into from

Conversation

sandrom
Copy link

@sandrom sandrom commented Dec 29, 2019

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.

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.
@ghost ghost changed the title Okta Challenge Preferences [pr] Okta Challenge Preferences Mar 5, 2020
Copy link

@pullrequest pullrequest bot left a 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.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

These changes look good. I've left a few comments inline.


Was this helpful? Yes | No

} `json:"_embedded"`
}

type factor struct {
Copy link

Choose a reason for hiding this comment

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

This type of cleanup is always good.

@@ -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")
Copy link

Choose a reason for hiding this comment

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

This line might be better if split to reduce line length.

@@ -23,6 +23,7 @@ var providerDuration int

// Okta
Copy link

Choose a reason for hiding this comment

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

Although not precisely the same, could the OktaProviderConfig from config/config.go be used here, and the OneLoginProviderConfig be used above?

@@ -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) {
Copy link

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.

Copy link

@pullrequest pullrequest bot left a 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

Copy link

@pullrequest pullrequest bot left a 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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 participants