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 --auth-type=webauthn flag #4931

Merged
merged 4 commits into from Jun 1, 2022
Merged

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 feat: Add webauthn value to --auth-type flag May 24, 2022
@jumoel jumoel changed the title feat: Add webauthn value to --auth-type flag feat: Add --auth-type=webauthn 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants