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(npm): add hook for getting the authentication header #2664

Merged
merged 1 commit into from
May 3, 2021

Conversation

waterfoul
Copy link
Contributor

@waterfoul waterfoul commented Mar 31, 2021

What's the problem this PR addresses?

Adds a hook for adding authentication headers, intended to be used for OAuth style authentication flows.
Closes #2067

...

How did you fix it?

I used reduceHook inside getAuthenticationHeader to collect the potential values from the hook, then return the provided value if there is one instead of checking npmAuthToken or npmAuthIdent

...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@waterfoul waterfoul force-pushed the auth-plugin branch 4 times, most recently from f6dfc1d to 8751e67 Compare March 31, 2021 19:18
@merceyz merceyz changed the title Added a hook for the authentication header feat(npm): add hook for getting the authentication header Apr 4, 2021
@waterfoul
Copy link
Contributor Author

@merceyz / @arcanis any chance one of you could take a look at this?

@waterfoul
Copy link
Contributor Author

@merceyz or @arcanis is there anything I should be doing differently to get this reviewed?

@waterfoul
Copy link
Contributor Author

@arcanis Is there something wrong with this PR? If there is I'd like to fix it

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Hey @waterfoul! Sorry for the delay, it felt off my radar 🙁

Just two nits (and a rebase); I'd have done it myself but your PR doesn't seem open to maintainers edit (probably because of your org settings).

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/cli"
Copy link
Member

Choose a reason for hiding this comment

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

You need to mark @yarnpkg/cli as minor as well.

Comment on lines +195 to +196


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

const effectiveConfiguration = npmConfigUtils.getAuthConfiguration(registry, {configuration, ident});
const mustAuthenticate = shouldAuthenticate(effectiveConfiguration, authType);

if (!mustAuthenticate)
return null;

const header = await configuration.reduceHook((hooks: Hooks) => {
return hooks.getNpmAuthenticationHeader;
}, undefined, registry, {configuration, ident});
Copy link
Member

Choose a reason for hiding this comment

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

Would this be a good time to use an object instead of multiple arguments?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep things uniform for now, until it gets fixed in a sweeping pass. Given that BC will have to be preserved somehow, new hooks will likely follow a slightly different format.

@arcanis arcanis merged commit b6068ac into yarnpkg:master May 3, 2021
@arcanis
Copy link
Member

arcanis commented May 3, 2021

I forgot to wait for your fixes 🤦‍♀️ Will fix it on master, nevermind.

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.

[Feature] Add the ability to build authentication plugins
3 participants