Repository: nathan-v/aws_okta_keyman

Issue:
Support for 3-number challenge when Okta Verify mobile app forces extra verification
**Is your feature request related to a problem? Please describe.**
When attempting to use aws_okta_keyman, Okta Verify for Android determines that it is an "unusual" login attempt, and forces an additional layer of verification with the 3-number challenge. Unfortunately, this prevents further use of aws_okta_keyman

**Describe the solution you'd like**
The CLI tool should be able to integrate with this 3-number challenge verification step, and report back the correct number to select in the Okta Verify mobile app so that users can get past this stage and be able to use aws_okta_keyman successfully when additional verification is required.

Discussion:
nathan-v: @Pashtetollo this would be great to add but this new code will need unit tests.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/pull/212?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 100.00%. Comparing base [(`2d6da19`)](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/commit/2d6da191a1ecb9b53fb7f5b29c7b890f798fe88f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V) to head [(`71272a1`)](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/commit/71272a107f1cf77d842bfadd95c87ab1ebc3ad05?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V).

<details><summary>Additional details and impacted files</summary>


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

| [Flag](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/pull/212/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V) | Coverage Δ | |
|---|---|---|
| [](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/pull/212/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V) | `100.00% <100.00%> (?)` | |

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=Nathan+V#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/nathan-v/aws_okta_keyman/pull/212?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nathan+V).

Pashtetollo: @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: > @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: @nathan-v  Rewrote the test as a separate one to check for display of a number
Pashtetollo: @nathan-v  just a reminder, please have a look once you can so that it could be merged

Patch:
diff --git a/aws_okta_keyman/okta.py b/aws_okta_keyman/okta.py
index fa55911..a5762d5 100644
--- a/aws_okta_keyman/okta.py
+++ b/aws_okta_keyman/okta.py
@@ -378,10 +378,24 @@ class Okta:
             otherwise None
         """
         try:
+            message = "Waiting for MFA success..."
             while ret["status"] != "SUCCESS":
-                LOG.info("Waiting for MFA success...")
+                LOG.info(message)
                 time.sleep(sleep)
 
+                if ret.get("factorResult", "WAITING") == "WAITING":
+                    correct_number = (
+                        ret.get("_embedded", {})
+                        .get("factor", {})
+                        .get("_embedded", {})
+                        .get("challenge", {})
+                        .get("correctAnswer", None)
+                    )
+                    # Okta issued MFA 3-number challenge
+                    if correct_number:
+                        message = (
+                            f"Pick a following number on your device: {correct_number}"
+                        )
                 if ret.get("factorResult", "REJECTED") == "REJECTED":
                     LOG.error("Duo Push REJECTED")
                     return None


Test patch:
diff --git a/aws_okta_keyman/test/okta_test.py b/aws_okta_keyman/test/okta_test.py
index 1109ade..c11ddac 100644
--- a/aws_okta_keyman/test/okta_test.py
+++ b/aws_okta_keyman/test/okta_test.py
@@ -161,6 +161,28 @@ MFA_WAITING_RESPONSE = {
         },
     },
 }
+MFA_NUMBER_CHALLENGE_ANSWER = 23
+MFA_NUMBER_CHALLENGE_WAITING_RESPONSE = {
+    "status": "MFA_CHALLENGE",
+    "factorResult": "WAITING",
+    "challengeType": "FACTOR",
+    "_links": {
+        "next": {
+            "href": "https://foobar.okta.com/api/v1/authn/factors/X/verify",
+        },
+    },
+    "stateToken": "token",
+    "_embedded": {
+        "factor": {
+            "factorType": "push",
+            "provider": "OKTA",
+            "id": "abcd",
+            "_embedded": {
+                "challenge": {"correctAnswer": MFA_NUMBER_CHALLENGE_ANSWER},
+            },
+        },
+    },
+}
 MFA_REJECTED_RESPONSE = {
     "status": "MFA_CHALLENGE",
     "factorResult": "REJECTED",
@@ -796,6 +818,25 @@ class OktaTest(unittest.TestCase):
         }
         self.assertEqual(ret, expected)
 
+    @mock.patch("aws_okta_keyman.okta.LOG")
+    def test_mfa_wait_loop_number_challenge_display(self, logger_mock):
+        client = okta.Okta("organization", "username", "password")
+        client._request = mock.MagicMock(name="_request")
+        client._request.side_effect = [
+            MFA_WAITING_RESPONSE,
+            MFA_NUMBER_CHALLENGE_WAITING_RESPONSE,
+            MFA_NUMBER_CHALLENGE_WAITING_RESPONSE,
+            SUCCESS_RESPONSE,
+        ]
+        data = {"fid": "123", "stateToken": "token"}
+
+        client.mfa_wait_loop(MFA_WAITING_RESPONSE, data, sleep=0)
+
+        expected_message = (
+            f"Pick a following number on your device: {MFA_NUMBER_CHALLENGE_ANSWER}"
+        )
+        logger_mock.info.assert_any_call(expected_message)
+
     def test_mfa_wait_loop_rejected(self):
         client = okta.Okta("organization", "username", "password")
         client._request = mock.MagicMock(name="_request")