Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Upgrade to bearer with otp #18711

Merged
merged 1 commit into from
Oct 19, 2017
Merged

Upgrade to bearer with otp #18711

merged 1 commit into from
Oct 19, 2017

Conversation

iarna
Copy link
Contributor

@iarna iarna commented Oct 4, 2017

This adds legacy auth support to the new commands. (They thought they had it previously but weren't passing it along correctly.)

This also makes it so that enabling two-factor authentication upgrades your auth method to a bearer token. 2fa does not support basic authentication. If your registry does not support bearer authentication then we will now refuse to enable two-factor authentication.

@iarna iarna requested a review from a team as a code owner October 4, 2017 17:24
lib/profile.js Outdated
conf.auth = {basic: {username: creds.username, password: creds.password}}
} else if (creds.auth) {
const auth = Buffer.from(creds.auth, 'base64').toString().split(':', 2)
conf.auth = {basic: {username: auth[0], passowrd: auth[1]}}
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: passowrd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ty!

lib/profile.js Outdated
return profile.login(conf.auth.basic.username, conf.auth.basic.password, conf).then((result) => {
if (!result.token) throw new Error("Your registry " + conf.registry + "does not seem to support bearer tokens. Bearer tokens are required for two-factor authentication")
npm.config.setCredentialsByURI(conf.registry, {token: result.token})
return Bluebird.fromNode((cb) => npm.config.save('user', cb))
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about fromNode!

lib/profile.js Outdated
if (conf.auth.basic) {
log.info('profile', 'Updating authentication to bearer token')
return profile.login(conf.auth.basic.username, conf.auth.basic.password, conf).then((result) => {
if (!result.token) throw new Error("Your registry " + conf.registry + "does not seem to support bearer tokens. Bearer tokens are required for two-factor authentication")
Copy link
Contributor

Choose a reason for hiding this comment

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

I note that it's possible that they enabled TFA in another session, & the reason that they cannot authenticate here is because of their already-active TFA status. Does profile.login handle the OTP prompt in that case?

Copy link
Contributor Author

@iarna iarna Oct 4, 2017

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

@iarna iarna Oct 4, 2017

Choose a reason for hiding this comment

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

ah, I see what you're saying…

So this flow only happens if you run npm profile enable-2fa. So if you've enabled 2fa elsewhere but have a legacy auth on this account, I wouldn't expect you to be enabling 2fa here too? If you try you'll get an uncaught EOTP error. We could catch it here, but it's really a more general problem because sudenly every operation you do will ask for an OTP and I think it's far more likely that you'll get failures due to, for example, npm install.

Tweaking the error handler to be more specific, that is, to tell folks to npm login if they have bearer auth and just errored out due to EOTP (or E401 w/ no www-authenticate headers) would probably be good though.

@iarna iarna force-pushed the upgrade-to-bearer-with-otp branch 3 times, most recently from 8ece5e1 to 70fc403 Compare October 4, 2017 23:39
@iarna iarna force-pushed the upgrade-to-bearer-with-otp branch from 70fc403 to 8a0e800 Compare October 14, 2017 20:33
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

🐑 LGTM

@zkat zkat changed the base branch from latest to release-next October 19, 2017 01:42
@zkat zkat merged commit be67de7 into release-next Oct 19, 2017
@zkat zkat deleted the upgrade-to-bearer-with-otp branch October 19, 2017 01:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants