Skip to content

Conversation

jumoel
Copy link
Contributor

@jumoel jumoel commented May 23, 2022

What

Add --auth-type=webauthn flag to npm.

Why

We want user to be able to opt-in to web logins.

References

@jumoel jumoel requested a review from a team as a code owner May 23, 2022 14:22
@jumoel
Copy link
Contributor Author

jumoel commented May 23, 2022

For this PR to actually do anything, it requires changes to npm-profile.

Should I raise and merge a PR to npm-profile before this PR is merged, or how should I proceed?

Edit: Clarified with @darcyclarke regarding process. npm-profile changes should land before this is merged. 🙂

@jumoel jumoel requested review from darcyclarke and nlf May 23, 2022 14:43
@nlf
Copy link
Contributor

nlf commented May 23, 2022

we might need to do a little bike shedding of the option name, i almost wonder if adding web to --auth-type is better. i'll bounce that around in the team while you work on the npm-profile changes and circle back here.

other than that, this looks great!

@jumoel
Copy link
Contributor Author

jumoel commented May 23, 2022

Added the related npm-profile PR: npm/npm-profile#48.

@nlf
Copy link
Contributor

nlf commented May 24, 2022

ok, so we talked it out and i think what we'd rather do here is modify the auth-type config definition such that webauthn is a new allowed value for it. if you can make that adjustment i think we're ready to land this right after the npm-profile update lands (i'll post a review over there detailing the necessary changes on that end)

thanks!

@jumoel
Copy link
Contributor Author

jumoel commented May 24, 2022

@nlf

Thanks for the feedback!

Given that the behavior is exactly the same as legacy, do you prefer that I make an alias for legacy in https://github.com/npm/cli/blob/latest/lib/commands/adduser.js#L4-L9 or that I add lib/auth/webauthn.js that uses legacy.js#login directly?

@nlf
Copy link
Contributor

nlf commented May 24, 2022

Given that the behavior is exactly the same as legacy, do you prefer that I make an alias for legacy in https://github.com/npm/cli/blob/latest/lib/commands/adduser.js#L4-L9 or that I add lib/auth/webauthn.js that uses legacy.js#login directly?

i think making it an alias for legacy makes sense, seeing as they call identical code. we can always break it up later if we have a need to.

@jumoel
Copy link
Contributor Author

jumoel commented May 24, 2022

i think making it an alias for legacy makes sense, seeing as they call identical code. we can always break it up later if we have a need to.

Alright, got it, thanks! 🙂

@jumoel jumoel changed the title feat: Add --use-webauth flag May 24, 2022
@jumoel jumoel changed the title feat: Add webauthn value to --auth-type flag May 24, 2022
@jumoel
Copy link
Contributor Author

jumoel commented May 25, 2022

I changed the flag to be a variation of auth-type. Please re-review @nlf 🙂

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this looks good!

@ruyadorno ruyadorno merged commit a8ae177 into latest Jun 1, 2022
@ruyadorno ruyadorno deleted the jumoel/add-use-webauth-flag branch June 1, 2022 21:12
@ruyadorno ruyadorno mentioned this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants