-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for Software Token MFA. #144
Conversation
Would be great if you could update the readme with an example. (optional, can be also an future PR) Please fix all lint issues + black issues. Thanks |
fix all lint issues + black issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review, but I think I've found a logic issue. @pvizeli @Taikono-Himazin
if not (bool(sms_mfa) or bool(software_token_mfa)): | ||
# Disable MFA | ||
pass | ||
if preferred == "SMS": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter doc says "However, preferred
is not needed only if both of the previous arguments are False." So I think this supposed to be elif
?
""" | ||
Verify the value generated by TOTP to complete the registration of Software Token MFA. | ||
:param code: The value generated by TOTP | ||
:param device_name: Device name to register (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter is declared as optional, but doesn't have a default value.
:param code: software token MFA code. | ||
:type code: string | ||
:param code: mfa_token stored in MFAChallengeException. Not required if you have not regenerated the Cognito instance. | ||
:type code: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name is mfa_tokens
, but the doc calls it code
. And it's some collection, not a string, right?
:param code: SMS MFA code. | ||
:type code: string | ||
:param code: mfa_token stored in MFAChallengeException. Not required if you have not regenerated the Cognito instance. | ||
:type code: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
|
||
def respond_to_sms_mfa_challenge(self, code, mfa_tokens=None): | ||
""" | ||
Respons challenge to SMS MFA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "Sends a response to an SMS MFA challenge."
I was looking for an easy way to add Software Token MFA from Python. I think adding functionality to this library is the easiest solution.
I think this will solve what has remained as an issue since the warrant era.
capless#169
However, the function is still incomplete. We will continue to add more if necessary.
Corrections are welcome. Please point out if the function layout or naming convention is incorrect.
Also, I would like to mention it in the document if necessary, but please tell me how to do it.
thank you.
Since it is Google Translate, there may be something wrong. I'm sorry.