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

fix: find the chosen okta mfa option in the mfaOptions #1133

Merged
merged 8 commits into from
Sep 29, 2023

Conversation

russell
Copy link
Contributor

@russell russell commented Sep 21, 2023

find the index of the a duoMfaOption by looking in the duoMfaOptions.

the duoMfaOptions is dynamically created, so it could have any number of items, the reason that using the interactively selected mechanism works is that it's choosing based on the list of duoMfaOptions options rather than hardcoded ids. This change does something similar, but searches through duoMfaOptions to find the option passed at the CLI, if the item is missing, then an error will be emitted.

for example if I don't have passcode enabled and if i try to force that option

$ ./saml2aws login --session-duration=43200 --mfa='DUO' --duo-mfa-option='Passcode' --skip-prompt --force --role='arn:aws:iam::183823948234:role/okta/team'
Using IdP Account default to access Okta https://foobar.okta.com/app/amazon_aws/aoeusnthoaenuth/sso/saml
Authenticating as [email protected] ...
Error authenticating to IdP.: error verifying MFA: error unsupported MFA option 'Passcode'

fixes #1079 #1061

@russell russell changed the title fix: find the chosen mfa option in the mfaOptions fix: find the chosen okta mfa option in the mfaOptions Sep 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

Merging #1133 (eb8dc09) into master (391a961) will increase coverage by 1.81%.
The diff coverage is 66.66%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   37.19%   39.00%   +1.81%     
==========================================
  Files          53       53              
  Lines        7959     7963       +4     
==========================================
+ Hits         2960     3106     +146     
+ Misses       4619     4442     -177     
- Partials      380      415      +35     
Flag Coverage Δ
unittests 39.00% <66.66%> (+1.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/provider/okta/okta.go 38.14% <66.66%> (+11.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@mapkon mapkon left a comment

Choose a reason for hiding this comment

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

Hey - do you mind adding tests to this? This is going to push the coverage down and make the build to fail

mapkon and others added 3 commits September 27, 2023 13:42
* origin/mfa-options:
  chore(deps): bump github.com/tidwall/gjson from 1.16.0 to 1.17.0
  fix: find the chosen mfa option in the mfaOptions
  chore(deps): bump github.com/aws/aws-sdk-go from 1.45.7 to 1.45.12
@russell
Copy link
Contributor Author

russell commented Sep 27, 2023

Sure! here is a test. Can you give me some feedback about the fixtures, i haven't trimmed them down to only the fields that are referenced in the okta.go calls, if you like i can do that. I left them with more information rather than less in the hope that future readers would be able to improve the logic without needing access to fresh raw responses from duo. But each to their own, would you like them trimmed?

@mapkon
Copy link
Member

mapkon commented Sep 27, 2023

Sure! here is a test. Can you give me some feedback about the fixtures, i haven't trimmed them down to only the fields that are referenced in the okta.go calls, if you like i can do that. I left them with more information rather than less in the hope that future readers would be able to improve the logic without needing access to fresh raw responses from duo. But each to their own, would you like them trimmed?

I personally think we should trim them down so that the test is focused on what it is testing. The extra context can be confusing.

@gliptak thoughts?

@russell
Copy link
Contributor Author

russell commented Sep 28, 2023

I personally think we should trim them down so that the test is focused on what it is testing. The extra context can be confusing.

i'm cool with doing that, i'll update the PR later today. I'm a bit on the fence personally so it's not a big deal

@russell
Copy link
Contributor Author

russell commented Sep 28, 2023

ok, this is ready for review again, I have extended the test suite to assert all the server request's and trimmed down the fixtures

@mapkon mapkon merged commit b452ec5 into Versent:master Sep 29, 2023
8 checks passed
@mapkon
Copy link
Member

mapkon commented Oct 27, 2023

@russell This has been released

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.

panic: runtime error: index out of range [1] with length 1
3 participants