Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

respect 2fa settings #396

Merged
merged 2 commits into from
Feb 6, 2019
Merged

respect 2fa settings #396

merged 2 commits into from
Feb 6, 2019

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Feb 1, 2019

Now forces users that have 2FA enabled to supply a 2FA code to get a valid token. This corresponds to an unversioned server change (https://github.com/zapier/zapier/pull/23125) and will affect all CLI users on all versions, so we have to think about 4 use cases:

  • new CLI user w/ 2fa - catches the initial error, supplies the 2FA code, is all set
  • new CLI user w/o 2fa - doesn't see the 2FA error, logs in as normal
  • User with old version of CLI w/ 2FA - won't be able to successfully login. Existing keys will continue to work, but they'll need the newer version of CLI (presumably 8.0.0) to get access to their data. This shouldn't be a big impact, as most users who have the CLI already don't ever need to log in after the initial setup. This class of users is really only impacted if they installed a version, didn't log in, and then try to log in after the server PR is merged
  • user w/ old version of CLI w/o 2FA - no problem, log in like normal

To help the users in group 3 above, the server error message specifically mentions updating CLI (and includes the command to do so). The only folks seeing that message are those with old versions, as this PR eats that message and prompts for the CLI code.

A potential UX improvement we could include is prompting for 2FA a second time in the case of a failed token (instead of making the user re-type email/pw just for missing the 30 sec window). It makes the try/catch logic a little deeper, but it's manageable if we think that'll be a common thing.

@xavdid xavdid requested a review from eliangcs February 1, 2019 21:32
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it works! I have a minor suggestion, but you have the final call.

src/commands/login.js Outdated Show resolved Hide resolved
Co-Authored-By: xavdid <beamneocube@gmail.com>
@xavdid xavdid merged commit a11c832 into master Feb 6, 2019
@xavdid xavdid deleted the respect-2fa branch February 6, 2019 01:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants