Skip to content
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

Support npm's 2FA (via OTP) #5

Closed
nfischer opened this issue Jan 12, 2018 · 4 comments
Closed

Support npm's 2FA (via OTP) #5

nfischer opened this issue Jan 12, 2018 · 4 comments
Assignees

Comments

@nfischer
Copy link
Member

I ran into this when pushing shelljs v0.8. Now that NPM supports 2FA (and I've enabled it), we really need to find a way to support this in the script.

Current behavior is to do most things (bump the version, push the tags) but it fails during npm publish. First reaction might be to re-run the command, but this would re-bump the version and push a second tag (and would still fail).

I see a few solutions:

  1. Add a --otp flag. If present, then the otp value is forwarded to npm publish's --otp flag. But if the user forgets to add this (I inevitably will forget), then we have the same issue where it's not safe to re-run the command.
  2. Always prompt the user for OTP, and pass this to npm publish --otp=. This isn't perfect, since the user may not have 2FA (yet we still prompt them). And if they pass the wrong thing, we need to remember to re-prompt them.
  3. Run npm publish in a way that it accepts user input. This doesn't work with shell.exec(), but does with child_process.exec(). Users are only prompted when they use 2FA. But if the user supplies a bad OTP, or hits ctrl-c, then the project is still in a bad state (git tags are pushed, but it isn't published).

Official docs for 2FA are here.

@nfischer
Copy link
Member Author

@freitagbr FYI

@nfischer
Copy link
Member Author

I tried a local patch along the lines of suggestion 1, and I don't think it's a suitable approach. Even if an OTP is fresh (remaining lifetime ~30 seconds) when the user types it on the commandline, shelljs-release does several (slow) steps before getting to npm publish, by which time the OTP may be nearly expired.

I think suggestion 3 is a better option.

@nfischer nfischer self-assigned this Jan 23, 2018
@freitagbr
Copy link

Publish the tags only after npm publish succeeds?

@nfischer
Copy link
Member Author

Reordering might help. Current order looks like:

  1. npm whoami
  2. npm access ls-collaborators
  3. npm owner ls
  4. npm version (bump version, commit, make tag)
  5. git push
  6. npm publish

Steps 1-3 exist to see if the user is authorized to run npm publish (and give descriptive output if not). This is mostly redundant: npm publish will fail if the user is unauthorized. I think we could rearrange to something like:

  1. npm version (bump version, commit, make tag)
  2. npm publish
  3. if npm publish succeeded, push commit+tag
  4. if npm publish failed, undo commit+tag and provide error message

This puts npm publish close to the front (good for OTP). We'll trigger step 4 if the user is (1) not logged in, (2) they're not allowed to publish, or (3) they gave a bad OTP. I don't think we really need the extra error messages (e.g. npm owner ls) we had before, I assume npm publish is descriptive enough.


This seems like a good refactor, regardless of 2FA. This would also open the door for us to implement a blend of suggestions 1 & 3, which is exactly how npm publish itself actually works (prefer --otp if present, but prompt user otherwise).

nfischer added a commit that referenced this issue Jan 30, 2018
This completely changes the order of steps we follow for publishing a
package. The goal here is to move `npm publish` as close as possible to
the beginning so that we can error-out early if it fails (e.g. 2FA
errors).

At a high level, our new steps are:

 * npm version (bump version, commit, make tag)
 * echo npm publish
   * if successful, push commit & tag
   * if failed, undo commit & tag

In the case of failure, we still provide the same granularity of error
output: we check to see if the user is logged in, if they're
owner/contributor, etc. In addition, we also provide some error output
if the user is outside of a git repo (the usual cause for `npm version`
failures).

While this refactor is done to enable supporting 2FA, this does not
itself provide OTP support. This refactor also adds TODOs for other
bugs, which will be resolved in follow-up PRs.

Issue #5
nfischer added a commit that referenced this issue Jan 30, 2018
This completely changes the order of steps we follow for publishing a
package. The goal here is to move `npm publish` as close as possible to
the beginning so that we can error-out early if it fails (e.g. 2FA
errors).

At a high level, our new steps are:

 * npm version (bump version, commit, make tag)
 * echo npm publish
   * if successful, push commit & tag
   * if failed, undo commit & tag

In the case of failure, we still provide the same granularity of error
output: we check to see if the user is logged in, if they're
owner/contributor, etc. In addition, we also provide some error output
if the user is outside of a git repo (the usual cause for `npm version`
failures).

While this refactor is done to enable supporting 2FA, this does not
itself provide OTP support. This refactor also adds TODOs for other
bugs, which will be resolved in follow-up PRs.

Issue #5
nfischer added a commit that referenced this issue Jan 31, 2018
This adds the `--otp` flag to support 2FA.

Fixes #5
nfischer added a commit that referenced this issue Feb 7, 2018
This adds the `--otp` flag to support 2FA.

Fixes #5
nfischer added a commit that referenced this issue May 8, 2018
This documents our support for npm's 2FA via the --otp flag.

Issue #5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants