Okta + Duo: Allow passcodes from mobile app

wlonkly commented 9 months ago

Any Duo mobile app registered with Duo, usually for Duo Push, also has the capability to produce HOTP passcodes:

Duo Mobile app on iOS, showing HOTP code. Yes, I burned that code.

In the case where Duo Push is administratively disabled and Yubikeys (etc) are not provided, HOTP might be the only way for a user to perform Duo MFA. This is the case for us, which is preventing our non-Yubikey users from using saml2aws at all.

This change adds Passcode to the list of MFA possibilities whenever a phone1 is registered with Duo, to support using HOTP.

I debated between adding it like this, or adding another conditional to the option[value="token"] case -- let me know if that would be preferable. I also checked the other Duo-enabled providers, but none of the others use this kind of logic to limit the MFA options presented to the user.

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 41.92%. Comparing base (9a4c5b6) to head (22e706d). Report is 32 commits behind head on master.

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

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1222 +/- ## ========================================== + Coverage 40.44% 41.92% +1.48% ========================================== Files 54 54 Lines 8276 6373 -1903 ========================================== - Hits 3347 2672 -675 + Misses 4491 3265 -1226 + Partials 438 436 -2 ``` | [Flag](https://app.codecov.io/gh/Versent/saml2aws/pull/1222/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/Versent/saml2aws/pull/1222/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | `41.92% <100.00%> (+1.48%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gliptak commented 9 months ago

any related unit test updates?

wlonkly commented 9 months ago

any related unit test updates?

There was no test coverage of the option selection/presentation today so I didn't update the tests, but happy to add coverage (I think... I don't work in Go often and the current tests in TestVerifyMfa_Duo all focus on the request flow and simulating a push, so I could use a pointer in the right direction if possible!)

gliptak commented 9 months ago

@wlonky yes, there seems to be coverage the modified code https://app.codecov.io/gh/Versent/saml2aws/blob/master/pkg%2Fprovider%2Fokta%2Fokta.go#L974

consider creating a copy of https://github.com/Versent/saml2aws/blob/14c45abf5f8dcee4d39a269cf1a55dcf28006643/pkg/provider/okta/okta_test.go#L268 and updating for 1Passcode` path (some processing could be shared between these two flows)