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: Add web auth type #5076

Merged
merged 1 commit into from Jul 12, 2022
Merged

feat: Add web auth type #5076

merged 1 commit into from Jul 12, 2022

Conversation

jumoel
Copy link
Contributor

@jumoel jumoel commented Jun 24, 2022

What

Add the more generic web for the registry to use, instead of the implementation-specific webauthn authentication type.

Why

It's more generic and sounds better.

References

@jumoel jumoel requested a review from a team as a code owner June 24, 2022 09:14
@jumoel jumoel changed the title refactor: Change 'webauthn' auth type to 'web' refactor: Change webauthn auth type to web Jun 24, 2022
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jun 24, 2022

found 2 benchmarks with statistically significant performance improvements

  • app-large: modules-only, no-lock
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 45.119 ±0.85 25.349 ±0.33 22.980 ±0.02 26.219 ±0.89 4.001 ±0.03 4.129 ±0.20 3.126 ±0.00 16.040 ±0.11 3.037 ±0.04 4.119 ±0.17
#5076 45.775 ±0.89 24.831 ±0.27 21.938 ±0.29 25.913 ±1.29 3.570 ±0.13 3.534 ±0.06 3.131 ±0.02 15.788 ±0.55 3.218 ±0.13 4.508 ±0.39
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 69.035 ±44.16 18.783 ±0.60 16.726 ±0.18 18.371 ±0.02 3.577 ±0.01 3.509 ±0.09 3.215 ±0.01 11.562 ±0.15 2.982 ±0.04 4.079 ±0.06
#5076 33.534 ±2.31 18.641 ±0.24 16.398 ±0.75 17.649 ±0.21 3.489 ±0.01 3.517 ±0.09 3.094 ±0.27 10.748 ±0.21 3.060 ±0.16 3.962 ±0.02

@ljharb
Copy link
Collaborator

ljharb commented Jun 24, 2022

wouldn’t this be a breaking change? Also there’s tons of kinds of web auth that isn’t webauthn.

@jumoel
Copy link
Contributor Author

jumoel commented Jun 24, 2022

@ljharb:

wouldn’t this be a breaking change?

Technically yes, but it’s never been enabled on the registry, except in staging, so it won’t be a problem.

We’ve already enabled the new header on the registry (and kept support for the old one).

Also there’s tons of kinds of web auth that isn’t webauthn.

Which is one of the reasons it’s changing. It’s not only webauthn that’s supported.

@ljharb
Copy link
Collaborator

ljharb commented Jun 24, 2022

What about private registries that may have implemented support in the meantime?

@darcyclarke
Copy link
Contributor

@jumoel @MylesBorins as @ljharb notes, this is a breaking change now so we should probably just keep this as "webauthn" & not introduce another value for auth-type in the same major (ref. https://docs.npmjs.com/cli/v8/using-npm/config#auth-type).

Copy link
Contributor

@darcyclarke darcyclarke left a comment

Choose a reason for hiding this comment

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

!Breaking change

@jumoel
Copy link
Contributor Author

jumoel commented Jun 25, 2022

@darcyclarke what if we just add web as another alias?

@MylesBorins
Copy link
Contributor

What about private registries that may have implemented support in the meantime?

There is 0 chance anyone has implemented this. If they had they likely would have reached out to clarify why the field is called webauthn.

The auth flow we are implementing is to "authenticate via website" not specific to WebAuthn. Our enhanced 2FA experience was launched explicitly as a beta. IMHO such beta services do not have to adhere to Semver (similar to the experimental status in Node.js). With that said, I recognize there is ambiguity here and we should be thoughtful about SemVer, but I also want to be pragmatic about committing the CLI to permanent legacy behavior for something that isn't currently supported by any registry.

I think we need to introduce --auth-type web in npm 8 for this feature with the proper header being sent to the registry, it does not make sense for us to have incorrect functionality or internals due to miscommunication during product design.

It seems like what we do need to decide is "what to do with the --auth-type webauthn flag. There seem to be two options

  1. Remove --auth-type webauthn in npm 8

This is the cleanest approach imho, and operates under the assumption that the new 2FA experience and related functionality are beta. It is being a bit cavalier about SemVer and has a non-zero risk of breaking things... but I would bet a steak dinner that nothing breaks and this is a much cleaner solution

  1. Deprecate --auth-type webauthn in npm 8 and remove it in npm 9.

In this approach we doc deprecate + warn for webauthn and remove it in 9.


Thoughts?

@jumoel jumoel changed the title refactor: Change webauthn auth type to web feat: Add web auth type Jun 28, 2022
@jumoel jumoel requested a review from darcyclarke June 28, 2022 12:10
@darcyclarke darcyclarke assigned wraithgar and unassigned fritzy Jul 11, 2022
@darcyclarke
Copy link
Contributor

darcyclarke commented Jul 12, 2022

@jumoel are you going to be updating this PR from the feedback above? Based on internal discussions & this thread, I believe the path forward here is to leave webauthn as a valid auth-type & just add web separately (the two should not coerce to eachother & you'll have to update this PR to not remove webauthn).

Our team can/will deprecate & remove the webauthn value in future versions.

@MylesBorins
Copy link
Contributor

I believe the path forward here is to leave webauthn as a valid auth-type & just add web separately (the two should not coerce to eachother & you'll have to update this PR to not remove webauthn).

@darcyclarke I believe that is what this PR does in it's current implementation. We can ignore my comments about updating docs

@wraithgar
Copy link
Contributor

This is going to collide with #5149 when that one lands. We'll have to rebase and update the copy, since we have switched to being able to deprecate certain config values instead of the entire config option.

@wraithgar wraithgar force-pushed the jumoel/change-webauthn-to-web branch from d00685b to 8a24fba Compare July 12, 2022 18:59
@wraithgar wraithgar force-pushed the jumoel/change-webauthn-to-web branch from 8a24fba to 0e0310b Compare July 12, 2022 19:04
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM with a tiny nit


Pass \`webauthn\` to use a web-based login.
What authentication strategy to use with \`login\`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
What authentication strategy to use with \`login\`.
Pass \`web\` to use a web-based login.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think removing the actual description is what we want here.

@wraithgar wraithgar merged commit e8102c1 into latest Jul 12, 2022
30 checks passed
@wraithgar wraithgar deleted the jumoel/change-webauthn-to-web branch July 12, 2022 19:55
@wraithgar wraithgar mentioned this pull request Jul 13, 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.

None yet

7 participants