nathan-v / aws_okta_keyman

AWS Okta Keyman (Key Manager) - An AWS + Okta CLI for generating and managing local AWS API keys
https://nathanv.com
Other
59 stars 38 forks source link

Update okta auth to support 3-number challenge when Okta Verify mobile app forces extra verification #212

Open Pashtetollo opened 1 month ago

Pashtetollo commented 1 month ago

This fixes #81 by pulling the correct number from request embedded data and displaying it in console once Okta Verify makes you pick one of 3 numbers

nathan-v commented 1 month ago

@Pashtetollo this would be great to add but this new code will need unit tests.

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 100.00%. Comparing base (2d6da19) to head (5ec808b).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #212 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 9 9 Lines 1131 1136 +5 ========================================= + Hits 1131 1136 +5 ```

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

Pashtetollo commented 1 month ago

@nathan-v Updated the test to cover the changes, not in the best way though as the request is updated with the number challenge data after you approve sign-in on your device (press Yes it's me), but that's better than nothing I suppose. Let me know if anything else is needed for this change to be included,

nathan-v commented 1 month ago

@nathan-v Updated the test to cover the changes, not in the best way though as the request is updated with the number challenge data after you approve sign-in on your device (press Yes it's me), but that's better than nothing I suppose. Let me know if anything else is needed for this change to be included,

@Pashtetollo Instead of re purposing an existing test a new test should be added that covers the alternate case of the phone number challenge. That way both potential branches are checked separately. :) After that it should be good.

Pashtetollo commented 3 weeks ago

@nathan-v Rewrote the test as a separate one to check for display of a number

Pashtetollo commented 5 days ago

@nathan-v just a reminder, please have a look once you can so that it could be merged