fix: remove unnecessary logging for webauthn auth

[hfaulds]

Description

When authenticating using Webauthn it is unnecessary to log Log in on.... This PR hides that logging in that case.

Prior to this PR the output looks like:

An alternative approach would be to use the existing auth handler polymorphism, rather than adding branching into lib/auth/legacy.js. If we introduce a new lib/auth/webauthn.js handler and move our legacy.js logic there, then legacy.js can log the notice and then call webauthn.js. This would avoid the branching introduced by this PR and the unfortunate confusion of legacy.js checking if authType == 'legacy'. The downside of that approach is the merge conflicts it will create with ongoing work to these files, and the naming confusion around legacy and webauthn.

References

• Closes https://github.com/github/npm/issues/5497

[npm-cli-bot]

no statistically significant performance changes detected

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	35.125 ±1.19	18.420 ±0.01	16.582 ±0.12	19.063 ±0.73	2.981 ±0.01	2.910 ±0.11	2.496 ±0.06	11.431 ±0.04	2.307 ±0.12	3.404 ±0.12
#5077	34.532 ±0.89	18.885 ±0.52	16.560 ±0.01	19.554 ±0.99	2.950 ±0.04	3.005 ±0.13	2.390 ±0.06	11.757 ±0.27	2.371 ±0.05	3.366 ±0.20

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	24.179 ±0.13	14.010 ±0.09	12.272 ±0.02	13.594 ±0.49	2.777 ±0.02	2.701 ±0.03	2.390 ±0.04	8.437 ±0.10	2.239 ±0.07	3.010 ±0.06
#5077	24.061 ±0.16	13.901 ±0.32	12.395 ±0.27	13.305 ±0.10	2.815 ±0.15	2.823 ±0.03	2.323 ±0.00	8.431 ±0.19	2.268 ±0.02	3.038 ±0.00

[darcyclarke]

@hfaulds @MylesBorins why is this "unnecessary"? The whole reason to log the location is for end-user clarity/security. npm users can be configured to use any number of third-party registries & displaying that information on any login seems useful (whether that's before sending any kind sensitive auth information, headers or even, in this case, redirecting the user to login in to a website in a browser).

[jumoel]

@darcyclarke It’s also showing a URL in the output that prompts for login.

[hfaulds]

@darcyclarke I updated the PR description with a screenshot of what the output looked like prior to this PR

[ljharb]

The registry url and the webauthn URL aren’t the same though. Isn’t the registry url important?

[MylesBorins]

@ljharb two things to clarify

1. it isn't a webauthn URL, this flag is essentially for "Authenticate through the website". The names are confusing. "WebAuthn" is a spec for supporting specific multi factor devices.
2. The URL you are forwarded to will always be for the registry you are authenticating with, so the two lines are superfluous. There is never a case where you would use this flag and get a url to click on that isn't for the registry you are authenticating with.

Also worth mentioning, this functionality has not yet been implemented by any public or private registry. we have something currently in staging that we are testing which is where we found this unnecessary copy that makes the login experience noisy

[ljharb]

@MylesBorins in the screenshot, the registry is registry-staging.npm.red, and the web URL is www-staging.npm.red - if the domains can be different then how is "There is never a case where you would use this flag and get a url to click on that isn't for the registry you are authenticating with." accurate?

[MylesBorins]

ok I'm seeing the distinction now between the registry URL and the auth URL... one of which is registry.npmjs.com and the other which would be www.npmjs.com

We can keep both, it just seems noisy. Maybe we could change the copy to be a bit more explicit than the current text... previously when logging in you would be prompted for username and have no insight into where you were logging in. Now you will see a URL you are clicking. While it won't have necessarily have "registry" in the URL it will be explicitly tied to the registry / service you are authenticating with.

[jumoel]

@MylesBorins: So what would you prefer we do? 🙂

[MylesBorins]

I think we can still log this information but at a different log level (verbose rather than info). I think we can consider this a post GA improvement rather than exit criteria.

What isn't clear is if we want to make this loglevel vebose for all login, or just for web based login. As such I think we can punt on this being blocking

Thoughts?

[wraithgar]

I think #5172 might fix this in a much more holistic way.

[MylesBorins]

@wraithgar would the account for the npm info logging the name of the registry?

[wraithgar]

@wraithgar would the account for the npm info logging the name of the registry?

That would still be there, but the default loglevel is notice which means it would not show up on stdout unless the user had set their loglevel to info or higher. It's very important it's still there in some form, as all log messages to go the logfile (we are running this through replaceInfo right?), and that's important for support/debugging.

This PR means we don't have to worry about this logging showing up as apparently duplicate info.

[wraithgar]

I really honestly don't see what the problem is here. It really is not that noisy and this does feel like anticipating a problem that doesn't exist. I don't think changing it to info helps (then you don't see the message on other forms of login by default), and I don't think removing it for one type of login helps (the registry itself is a config value that carries a lot of meaning to the cli and to the user, independent of the web url it then uses).

Especially given that the web login url is not the same as the registry, I think both are appropriate.

Also, I was in error about the other PR being related, that one affects publish, not login.