Support Number Challenge for Okta MFA

[muramasa64]

Support Number Challenge for Okta MFA

https://help.okta.com/oie/en-us/Content/Topics/identity-engine/authenticators/configure-okta-verify-options.htm

[amasses]

@wolfeidau could we please have this (or one of the others) merged in and released? I'd really like to be able to enforce the number challenge for risk based session decisions, but until this is in place it makes it impossible to use.

[bdemirtas]

@wolfeidau could we please have this (or one of the others) merged in and released? I'd really like to be able to enforce the number challenge for risk based session decisions, but until this is in place it makes it impossible to use.

Definitely, this is something needed. Trying to bring some attention to this PR.

[ddavison-lucid]

this repo has been pretty active since this PR was raised with other merges and enhancements. Is there some objection to this PR @wolfeidau ? Would at least help to know what that is. Quite a few looking to make use of this - ideally without maintaining a fork :)

[codecov-commenter]

Codecov Report

Merging #877 (c17b42a) into master (7f8e7d4) will decrease coverage by 0.03%. The diff coverage is 0.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
- Coverage   36.20%   36.18%   -0.03%     
==========================================
  Files          53       53              
  Lines        7951     7956       +5     
==========================================
  Hits         2879     2879              
- Misses       4703     4708       +5     
  Partials      369      369              

Flag	Coverage Δ
unittests	36.18% <0.00%> (-0.03%)	:arrow_down:

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

Files Changed	Coverage Δ
pkg/provider/okta/okta.go	22.10% <0.00%> (-0.09%)	:arrow_down:

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

[silver886]

This PR looks like #1115, I added unit test for this one as well. https://github.com/silver886/saml2aws/blob/feature/okta-number-challenge/pkg/provider/okta/okta_test.go#L197

Because I don't have Okta MFA, I'm not able to verify if the format of JSON response in the unit test is still valid. In the document it contains both _embedded.factors._embedded.challenge.correctAnswer and _embedded.factor._embedded.challenge.correctAnswer in similar responses which confuses me.

Ref. https://developer.okta.com/docs/reference/api/authn/#response-example-waiting-for-3-number-verification-challenge-response

[amasses]

@silver886 I had a quick look and the JSON content looks correct.

For reference I've captured the full payload and redacted sensitive data, and attached it - here the correctAnswer is 42. You are welcome to use some or all of this if you like.

number-challenge-example.json.txt

[silver886]

@amasses Thank you for the sample data, the JSON path is the same as the unit test and there is no need to modify it. :)

@mapkon Should I open another PR or add this unit test in #1115?

[mapkon]

@amasses Thank you for the sample data, the JSON path is the same as the unit test and there is no need to modify it. :)

@mapkon Should I open another PR or add this unit test in #1115?

Another PR would suffice. Thank you 🙏🏿

[mapkon]

@amasses With #1115 is this still required?

[amasses]

@mapkon #1115 seems to be for Ping Identity, I don't see anything in that PR to reference Okta

[mapkon]

@amasses Got you and thanks. If you could help me fix the coverage (this PR bumps it down by 0.3%), I am happy to merge this.

[silver886]

#1116 is submitted and should increase the coverage for this PR.

[silver886]

I think we might be able to close this PR since #1116 is merged.

Also, could we have this be released?

[muramasa64]

Sorry for leaving this pull request for so long.

@silver886 Thanks for writing the test.

This Pull Request will be closed.