[WAIT] Refactor with an otp being given in cli to mean that otp doesn't need…#82
[WAIT] Refactor with an otp being given in cli to mean that otp doesn't need…#82gkhaburzaniya-onelogin wants to merge 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the MFA verification flow to allow OTP tokens provided via CLI to bypass push notification attempts. The key change extracts OTP verification logic into a dedicated verify() function and restructures the flow to prioritize CLI-provided OTP tokens.
Key changes:
- Extracted OTP verification logic into a new
verify()function - Modified control flow to check for CLI-provided OTP tokens before attempting push notifications
- Wrapped
get_saml_response()in a newget_result()function to standardize return format
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return False | ||
|
|
||
|
|
||
| def verify(client, app_id, device_id, state_token, otp_token, username_or_email, password, onelogin_subdomain, ip, mfa_verify_info, devices, device_type): |
There was a problem hiding this comment.
The verify() function has 12 parameters, which makes it difficult to maintain and prone to argument ordering errors. Consider grouping related parameters into configuration objects (e.g., auth_config, device_config, mfa_config) to improve readability and maintainability.
| saml_endpoint_response = verify( | ||
| client, app_id, device_id, state_token, otp_token, username_or_email, password, onelogin_subdomain, ip, mfa_verify_info, devices, device_type) |
There was a problem hiding this comment.
The verify() function returns a tuple (saml_endpoint_response, mfa_verify_info), but here it's being assigned to a single variable saml_endpoint_response. This will cause the variable to contain the tuple instead of just the response object, leading to incorrect behavior. Use tuple unpacking: saml_endpoint_response, mfa_verify_info = verify(...)
| if not verified: | ||
| print("Enter the OTP Token for %s: " % device_type) | ||
| otp_token = sys.stdin.readline().strip() | ||
| saml_endpoint_response = verify( |
There was a problem hiding this comment.
The verify() function returns a tuple (saml_endpoint_response, mfa_verify_info), but here it's being assigned to a single variable saml_endpoint_response. This will cause the variable to contain the tuple instead of just the response object, leading to incorrect behavior. Use tuple unpacking: saml_endpoint_response, mfa_verify_info = verify(...)
| saml_endpoint_response = verify( | |
| saml_endpoint_response, mfa_verify_info = verify( |
| 'username_or_email': username_or_email, | ||
| 'password': password, | ||
| 'onelogin_subdomain': onelogin_subdomain | ||
| } |
There was a problem hiding this comment.
The return result statement has been removed from the get_result() function, but the function needs to return the result dictionary. Add return result after line 394.
| } | |
| } | |
| return result |
… push.