semantic-release / npm

:ship: semantic-release plugin to publish a npm package
MIT License
248 stars 116 forks source link

Check package permission on the registry #4

Open pvdlg opened 6 years ago

pvdlg commented 6 years ago

Currently we only check if the user is authenticated on the configured registry. But if a user set a validNPM_TOKEN, that doesn't have the permission to deploy the current package the release will fail during the publish phase. Similarly if the package name is already taken the release will fail.

To avoid that we should check in the verifyConditions if the package exists, and if it does, make sure the user has read-write access to the package. Unfortunately the npm-registry-client doesn't implement the method. So it would have to be done with npm access ls-collaborators <@scope/package>.

In addition it would be great, in case of a restricted package to be able to check if the organization is private and paid. But I'm didn't find a way to do that. During publish if the organization is not private and paid, the registry returns a 402 error.

kopax commented 6 years ago

During the npm release I ended up having an error:

npm ERR! This package has been marked as private
npm ERR! Remove the 'private' field from the package.json to publish it.

This could be prevented within verify stage by controlling if the package.json is having a false value for the key private.

It should break with:

{
+ "private": "true"
}
pvdlg commented 6 years ago

Maybe private: true should have the same effect as npmPublish: false, i.e. skip publish and just create the package locally, without erroring out?

kopax commented 6 years ago

@pvdlg according to https://docs.npmjs.com/files/package.json#private:

If you set "private": true in your package.json, then npm will refuse to publish it. This is a way to prevent accidental publication of private repositories. If you would like to ensure that a given package is only ever published to a specific registry (for example, an internal registry), then use the publishConfig dictionary described below to override the registry config param at publish-time.

I believe private: true and npmPublish: false are different configuration. private: true is specifically designed to return an exit code 1.

pvdlg commented 6 years ago

But in the context of semantic-release you can set private: false to make sure the package is not published on npm but still want to update the version in package.json, commit the package.json to the repo, make a release on GitHub, create a tag etc...

If you throw an error when private is true that means you can't use this plugin at all, therefore you can't update the version in package.json and you can't generate the package with npm pack.

Having a package with private set to true for safety (like outside of the semantic-release context) and wanting to update the version seems a valid use to me.

kopax commented 6 years ago

you can't generate the package with npm pack.

This is not true, this is how we install our private package which are not released on any registries.

We use private: true by default, and we have a specific command to remove it from our package.json.

I agree that a warning would be sufficient as we may want to tag a GitLab/GitHub version.

pvdlg commented 6 years ago

This is not true, this is how we install our private package which are not released on any registry.

What I'm saying is that if @semantic-release/npm throws an error when private: true that means you can't use it to generate the package with npm pack. Currently if you set npmPublish: false the plugin will run npm pack instead of npm publish.

My point is that there is no reason to throw an error is private: false because it would prevent users in such situation to use other feature of the plugin.

felixfbecker commented 6 years ago

This just bit me:

I just had a case where a user mistakenly didn't have public rights to the npm package (after changing tokens and using a different user for the token a couple weeks ago).

However, the verifyConditions didn't catch this. It was only caught when the npm publish step failed with a 403 error message stating the user doesn't have publish rights.

That meant the the git tag was still pushed to the repo, leaving the tag out of sync. Retrying the build would then not publish, because the tag was already pushed.

jedwards1211 commented 3 months ago

okay npm access list packages $(npm whoami) <pkg name> seems to work well for getting just the permissions for the package in question, it will output

<pkg name>: read-write
jedwards1211 commented 3 months ago

@travi @gr2m @babblebey I just realized, I can't run this check on the first publish because the package won't exist in npm yet.

Therefore, I should do something like

if (context.lastRelease.version) {
  // check npm access list packages ...
}

But context.lastRelease.version won't be available until after the Get last release step, so I can't do this check in the Verify conditions step. Is Verify release an appropriate step to perform this check in? It's vague to me what that step is intended for.

babblebey commented 3 months ago

I just realized, I can't run this check on the first publish because the package won't exist in npm yet.

One way we could go about this is.... Conditionally run this permission check based on existence of the package in registry.. I haven't figured exactly how but I believe we could do a search before-hand to figure whether the package exists in registry.

I found the npm-search command https://docs.npmjs.com/cli/v7/commands/npm-search

jedwards1211 commented 3 months ago

@babblebey searching for private packages won't work if the token doesn't have access, it will just look like the package doesn't exist. That's why we need to use context.lastRelease.version to know if the package should exist yet

jedwards1211 commented 2 months ago

Argh, turns out npm's API is insufficient for this right now. npm access list packages $(npm whoami) <pkg name> works for a classic token, but for a granular token it 403s, even if the granular token has read-write permission on the package. I've opened a discussion asking them to provide a command for this.

jedwards1211 commented 2 months ago

Hmmm, I could make a check that correctly errors out on a classic token with insufficient permission, but allows any granular token for now; then if npm provides a way to properly check the token's permissions someday, we could update the verification logic.

Here's what the verify logic would be:

Thoughts? @travi @gr2m @babblebey