Skip to content

Add MFA support#76

Merged
MisterWil merged 3 commits intoMisterWil:masterfrom
kevdliu:master
Sep 6, 2020
Merged

Add MFA support#76
MisterWil merged 3 commits intoMisterWil:masterfrom
kevdliu:master

Conversation

@kevdliu
Copy link
Contributor

@kevdliu kevdliu commented Aug 31, 2020

First attempt at adding MFA support. Overview of changes:

  • Added --mfa argument to the CLI which allows the user to provide a MFA code for login.
  • Since Abode uniquely identifies clients by a combination of the UUID and a session cookie, I modified the cache to persist session cookies to disk after successfully logging in and loading it back into requests.session during initialization.
  • Added some constants and error for MFA
  • I had to modify the CLI logic a little bit for when a MFA code is provided. I decided against adding mfa_code as an argument and a property to the Abode class since the code is very time sensitive and there's no guarantee that it'll be used for login immediately in that fashion. If the user provides a MFA code, we'll log in using the login() function first before fetching devices from Abode.

@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage increased (+0.2%) to 83.833% when pulling 2fd0aa1 on kevdliu:master into 2bed8dc on MisterWil:master.

@kevdliu kevdliu mentioned this pull request Aug 31, 2020
@MisterWil
Copy link
Owner

This looks great! Thank you so much for working on this. I really appreciate the tests too. I left a single comment, and really it's only a suggestion. If you feel like it's working well feel free to mark it ready for review and I'll merge it in and push out a new version.

@kevdliu kevdliu changed the title [DRAFT] Add MFA support Add MFA support Sep 6, 2020
@kevdliu kevdliu marked this pull request as ready for review September 6, 2020 00:46
@kevdliu
Copy link
Contributor Author

kevdliu commented Sep 6, 2020

Good call on handling unknown mfa types. I changed the PR to include that and added some related tests. I've been using this for the past week and no problems so far 👍

@MisterWil MisterWil merged commit a021091 into MisterWil:master Sep 6, 2020
@MisterWil
Copy link
Owner

Great! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants