feat: enable OTP and Push MFA for Okta #72
Conversation
crystall-bitquill
force-pushed
the
feat/okta-2fa
branch
7 times, most recently
from
e908d8b to
b6570ce
Compare
ColinKYuen
left a comment
ColinKYuen
left a comment
There was a problem hiding this comment.
Do you also have a screenshot of what the user will see when they are prompted for both the TOTP & Push challenge (not in terms of the phone but what the user will see while the driver is waiting for a response)?
& for manual testing, was it done for Windows, Linux & Mac?
| std::string VerifyTOTPChallenge(const std::string &verify_url, const std::string &state_token, const std::shared_ptr<Aws::Http::HttpClient>& http_client); | ||
|
|
||
| std::string VerifyPushChallenge(const std::string &verify_url, const std::string &state_token, const std::shared_ptr<Aws::Http::HttpClient> &http_client); |
There was a problem hiding this comment.
Can these methods not access the http_client from the class already? Why do we need to pass them in again
| #include "okta_auth_plugin.h" | ||
|
|
||
| #include <unordered_set> | ||
| #include <random> |
There was a problem hiding this comment.
Random doesn't seem used
***Moving this to the web servers util would make it more clear. Can also help by making that class into a namespace so when the function GenerateState() is called, we can tell its not coming from the okta class but the web server util
| //////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| inline AddrInformation::~AddrInformation() | ||
| { | ||
| freeaddrinfo(addrinfo_); | ||
| } |
There was a problem hiding this comment.
Couple of these throughout the header files inside the http folder. Can we move these inline constructors, deconstructors, & functions into the .cpp file (& creating one if it doesn't already exist)? Let's keep our .h as lightweight as possible
|
|
||
| #if (defined(_WIN32) || defined(_WIN64)) | ||
|
|
||
| #define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers |
There was a problem hiding this comment.
We should be able to remove this define WIN32_LEAN_AND_MEAN since it we do define it in our cmake
crystall-bitquill
force-pushed
the
feat/okta-2fa
branch
from
b6570ce to
ee1908d
Compare
crystall-bitquill
force-pushed
the
feat/okta-2fa
branch
from
ee1908d to
5ad5ba9
Compare
For Push challenges, there's no visible prompt on screen from the driver. For TOTP, if there's a browser available, the following window should appear: Testing was on Windows, Linux, and Mac, but the Linux machine didn't have an available browser to test TOTP with. |
2 participants
Summary
feat: enable OTP and Push MFA for Okta
Description
KEY_IAM_IDP_ARNkey, which is a duplicate ofKEY_IDP_SAML_ARNUpdated GUI:
Testing
Manual testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.