fix(auth): Fixes authentication conditions and logic with registries

[KidkArolis]

Summary

Supersedes #5162 (for now). Fixes #4157, #4451, #4672, #4119.

This PR brings 2 changes.

Fix for private npm packages

Fixes unauthed requests to yarnpk.com by treating yarnpkg.com as requests to npmjs.org in isRequestToRegistry.

Feature - simpler support for multipath registries

Before, if you wanted to use something like artifactory, Nexus or VSTS where meta url's differ from tarball urls, you had to configure like so:

@scope:registry=https://artifactory.example.com/api/npm/registry/
//artifactory.example.com/api/npm/registry/:_authToken=xxxx
custom-host-suffix=artifactory.example.com

But now you can do this instead:

@scope:registry=https://artifactory.example.com/api/npm/registry/
//artifactory.example.com/api/npm/registry/:_authToken=xxxx
//artifactory.example.com/api/tarballs/:_authToken=xxxx

This way we don't introduce a new option, we're just saying, aha, when accessing this kind of URL, make sure to authenticate it. I personally find it slightly more intuitive.

Test plan

I've added a new test case to requestIsToRegistry unit test.

I've redone the request authorisation tests. It's a small hit to the ergonomics of the tests (harder to comment some of them or use skip/only feature). But compacting them all from code to data means we can be a lot more thorough with all the different cases. The intention was to preserve all of the existing test cases, but add many more. In particular, here are the different common scenarios I tested:

1. using npm as default registry and using private registry for scoped packages
2. using scoped packages in both npm and private registry
3. using authenticated npm and using private registry for scoped packages
4. using npm with always-auth and using private registry for scoped packages
5. using private registry as default registry and using scoped packages on npm registry

And then also tested

6. registry url and request url path sensitivity
7. using custom-host-suffix for registries where pathnames for meta and tarballs differ
8. using multiple authToken entries for registries where pathnames for meta and tarballs differ

The PR is split into somewhat independent commits if that helps.

[buildsize]

This change will increase the build size from 10.4 MB to 10.4 MB, an increase of 1.86 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	900.42 KB	900.54 KB	124 bytes (0%)
yarn-[version].js	3.92 MB	3.92 MB	554 bytes (0%)
yarn-legacy-[version].js	4.06 MB	4.06 MB	837 bytes (0%)
yarn-v[version].tar.gz	906.14 KB	906.28 KB	146 bytes (0%)
yarn_[version]all.deb	669.23 KB	669.47 KB	242 bytes (0%)

[arcanis]

Why is it a function?

[KidkArolis]

Yeah, this is a bit silly. I might have been trying to "optimise" by avoiding expression evaluation until needed, but it's just string comparisons and perhaps creating functions inline is more expensive.

In any case - refactored this function slightly for overall readability:
#5321

[arcanis]

I'm going to go ahead and merge it for the release, but please check that the function is required :)

[JoostK]

@KidkArolis Is there a specific reason why only _authToken (Bearer) is considered, but not _auth (Basic) or username/_password? I'd like to be able to use the latter two authentication options as well.

[KidkArolis]

@JoostK Basic auth should work fine, it's handled in getAuth here:

yarn/src/registries/npm-registry.js

Lines 338 to 344 in 7fcf4dd

	// Check for basic username/password auth.
	const username = this.getRegistryOrGlobalOption(registry, 'username');
	const password = this.getRegistryOrGlobalOption(registry, '_password');
	if (username && password) {
	const pw = new Buffer(String(password), 'base64').toString();
	return 'Basic ' + new Buffer(String(username) + ':' + pw).toString('base64');
	}

. I guess it's just not been covered in these functional tests. I say that's fine until some reports an issue where it's not working.

[JoostK]

@KidkArolis I was referring to requestNeedsAuth as introduced in this PR, it only checks for _authToken.

[KidkArolis]

Ah, right! Hm, I suppose I could add that for completeness.

You can still use username/password if you specify custom-host-suffix for your registry.

But I suppose this PR is proposing to avoid using custom-host-suffix in favor of configuring multiple paths of the same registry. I'll see if it's easy to add.

[KidkArolis]

@JoostK Well that was easy: #5322.

[JoostK]

@KidkArolis Thanks! I wasn't aware of custom-host-suffix, totally glanced over it in your example in the description.

I prefer your approach with this PR as it is far more intuitive to configure and also more generic. In fact, I don't see a need for custom-host-suffix anymore.

Perhaps renaming the isToRegistry variable to needsAuth would also clear things up, it looks like a leftover now:

const isToRegistry = this.isRequestToRegistry(requestUrl, registry) || this.requestNeedsAuth(requestUrl);

[KidkArolis]

There are a lot of conditions that are taken into account before we call getAuth:

const isToRegistry = this.isRequestToRegistry(requestUrl, registry) || this.requestNeedsAuth(requestUrl);

    // this.token must be checked to account for publish requests on non-scopped packages
    if (this.token || (isToRegistry && (alwaysAuth || this.isScopedPackage(packageIdent)))) {
      const authorization = this.getAuth(packageIdent);
      if (authorization) {
        headers.authorization = authorization;
      }
    }

All of these conditions ultimately decide if the request needsAuth..

So the isToRegistry variable is still more meaningful in that it's more specific, it says "is this request to the registry or somewhere else".. But you're right in that this.requestNeedsAuth() doesn't quite fit in there..

There must be a way to rewrite this block into something better, but oh my, not sure how..

Maybe something like:

const tokenInUse = !!this.token
const isToRegistry = this.isRequestToRegistry(requestUrl, registry)
const isScopedPackage = this.isScopedPackage(packageIdent)
const registryAuthRequired = this.requestNeedsAuth(requestUrl);

const requestNeedsAuth = (isToRegistry || registryAuthRequired) && (alwaysAuth || isScopedPackage)

if (tokenInUse || requestNeedsAuth) {

I'm just worried about making these expressions no longer lazy, could impact perf if calcs done for each package.

Maybe the only issue is the naming of this.requestNeedsAuth? It is actually a different way to check if the request is being made to some registry in the config. So maybe I need to pull it into isRequestToRegistry logic and || it there as this.requestMatchesConfiguredRegistry or smth.