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

feat: Support pure web authentication for commands #5098

Merged
merged 11 commits into from
Jul 20, 2022

Conversation

jumoel
Copy link
Contributor

@jumoel jumoel commented Jun 29, 2022

What

Some operations, such as publish, might require verification.

This adds support for URL-based JSON payloads from the registry, that specify URLs to open and URLs to poll for the verification status.

Why

We want to allow the users to skip having to paste an OTP into the CLI when performing operations that require verification.
Currently, this is enabled for publish on the registry.

Adding this change in otplease allows all operations that require verification to support web based flows in the future.

Note to reviewers

To make this PR as easy as possible to review, the commits should be able to be reviewed individually.

@jumoel jumoel force-pushed the jumoel/support-web-publish branch 2 times, most recently from 7450283 to d140e6f Compare June 29, 2022 09:15
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jun 29, 2022

found 1 benchmarks with statistically significant performance regressions

  • app-medium: no-cache
timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 48.223 ±3.63 23.402 ±0.36 21.281 ±0.19 25.423 ±2.27 3.618 ±0.10 3.786 ±0.33 2.899 ±0.12 15.227 ±0.12 2.773 ±0.01 3.991 ±0.01
#5098 46.452 ±3.09 24.140 ±0.22 21.673 ±0.33 25.890 ±1.29 3.641 ±0.04 3.637 ±0.10 2.912 ±0.08 15.631 ±0.29 2.911 ±0.00 4.250 ±0.16
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 32.212 ±1.54 17.409 ±0.06 15.749 ±0.34 16.660 ±0.40 3.257 ±0.13 3.150 ±0.01 2.886 ±0.00 10.755 ±0.24 2.694 ±0.01 3.856 ±0.05
#5098 33.004 ±0.46 18.509 ±0.15 16.424 ±0.31 18.020 ±1.04 3.374 ±0.03 3.417 ±0.05 3.307 ±0.13 11.705 ±0.06 2.975 ±0.00 4.062 ±0.02

@jumoel jumoel force-pushed the jumoel/support-web-publish branch 2 times, most recently from 12b10ba to 835033d Compare June 29, 2022 10:31
@jumoel
Copy link
Contributor Author

jumoel commented Jun 29, 2022

I get output after my prompt to open an URL:

image

Any advice on how I stop that from appearing?

@jumoel jumoel force-pushed the jumoel/support-web-publish branch from 3bc3617 to 1298d8e Compare June 29, 2022 10:46
@jumoel jumoel changed the title feat: Support web authentication for supported commands feat: Support pure web authentication for commands Jun 29, 2022
lib/utils/web-auth.js Outdated Show resolved Hide resolved
lib/utils/web-auth.js Outdated Show resolved Hide resolved
@hfaulds hfaulds marked this pull request as ready for review July 13, 2022 14:15
@hfaulds hfaulds requested a review from a team as a code owner July 13, 2022 14:15
@hfaulds hfaulds force-pushed the jumoel/support-web-publish branch from a95b9f6 to a0fd462 Compare July 13, 2022 15:05
@hfaulds hfaulds force-pushed the jumoel/support-web-publish branch from 53443a4 to 38cef3a Compare July 13, 2022 15:45
@hfaulds
Copy link
Contributor

hfaulds commented Jul 13, 2022

@jumoel the request logging comes from https://github.com/npm/npm-registry-fetch/blob/43c91f5042854705c9d74445d197a138e530a690/lib/check-response.js#L51-L55. I don't see an easy way to avoid this logging 😢

@wraithgar wraithgar self-assigned this Jul 13, 2022
@wraithgar
Copy link
Member

Logging works a little weirdly when you have progress bars up. The loglevel is ignored and everything above a certain level automatically shows after the progress bar. Normally output has to go through a "pause" of the progress bar.

Typically when you see artifacts like this it is because there was some sort of disconnect between pausing the progress bar and whatever output was done by something in the cli. I'd start by looking at what does output and make sure it only does ut w/ a paused progress (or through npm.output if it's inside the npm codebase itself and not in a dependency)

@wraithgar
Copy link
Member

Logging concern is fixed in another PR #5172

@sandeepmeduru
Copy link
Contributor

Logging issue is fixed in 1231bf7. Thanks for suggesting the solution @wraithgar.. 🙇🏻

Working demo from local 👇🏻 -

Screen.Recording.2022-07-19.at.11.10.02.PM.mov

cc @saquibkhan @MylesBorins @jumoel @hfaulds @neeldani

@fritzy fritzy force-pushed the jumoel/support-web-publish branch from 4d6b770 to e1de7fd Compare July 20, 2022 19:38
@fritzy fritzy force-pushed the jumoel/support-web-publish branch from e1de7fd to e137c4d Compare July 20, 2022 19:39
@fritzy fritzy merged commit ed80a6e into latest Jul 20, 2022
@fritzy fritzy deleted the jumoel/support-web-publish branch July 20, 2022 19:49
@fritzy fritzy mentioned this pull request Jul 20, 2022
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.

8 participants