Fix the npm package already taken validator
🦐 Context
When pulling packages from the npm package registry, it can be accessed on two different levels:
- The project level
- Basically, users can pull packages from a given project
- The instance level
- Users can target an instance url
- Only packages that follow the naming convention are available at this level. Basically the package scope must be the root group (or user namespace) path.
Uploading an npm package can only be done at the project level.
In addition of this, we have an additional restriction on uploads:
You cannot publish a package if a package of the same name and version already exists.
At the project level, the above is implemented as a unique model validation.
At the instance level, we have a custom validation function that will look for duplicate package names in all the projects of the root namespace (except the current project).
🔥 The ~bug
That custom validation function looking for duplicates has a few issues:
- When looking for other packages within the root group, all package types are considered. The validation is for npm packages only
- The validation is executed even for packages that don't follow the naming convention.
- Those packages will never be available at the instance level. There is no point in checking packages outside of the given project.
Those issues leads to npm package uploads getting rejected with 400 Bad Request
. That's issue #37364 (closed).
🚒 What does this MR do?
- Change how the
package_already_taken?
validator works:- Don't run when the package name doesn't follow the naming convention.
- When looking for duplicates from the root namespace, only consider npm packages.
- Update the related specs
📷 Screenshots or Screencasts (strongly suggested)
n / a
⚗ How to setup and validate locally (strongly suggested)
- Create a group (public or private, it doesn't matter). Note the path. (path example:
fruits
) - Create a project with whatever visibility and whatever name. Note the id. (example:
Project1
) - Create a second project with whatever visibility and whatever name. Note the id. (example:
Project2
) - Have a personal access token ready with the
api
scope. - We're going to install a small util project that will manage npm commands for us: checkout https://gitlab.com/10io/gl_pru.
- cd into
gl_pru
Case: Package not following the naming convention
- Let's create a random package on the first project:
$ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<first project id>/packages/npm/ --name=test --version=1.2.3 --npm-package-scope=@test
-
⚠ The package scope MUST NOT be the group path. (example:fruits
)
-
- Make sure that the upload succeeds. You should see
+ @test/test@1.2.3
- Let's create a package with the same name and version on the second project
$ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<second project id>/packages/npm/ --name=test --version=1.2.3 --npm-package-scope=@test
Using master
: this second upload will fail with
npm ERR! code E400
npm ERR! 400 Bad Request - PUT <url>
Using this MR branch: the second upload succeeds
Case: Package of different type with same name
- Let's create a random nuget package on the first project. Not everyone has
$ nuget
installed so let's do it with a rails console:project = Project.find(<first project id>) project.packages.nuget.create!(name: "bananas", version: "1.2.3")
- Let's try to upload an npm package on the second project.
$ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<second project id>/packages/npm/ --name=bananas --version=1.2.3
Using master
, this second upload will fail with
npm ERR! code E400
npm ERR! 400 Bad Request - PUT <url>
Using this MR branch: the second upload succeeds
📐 Does this MR meet the acceptance criteria?
Conformity
-
I have included changelog trailers. (Does this MR need a changelog?) - [-]
I have added/updated documentation, orit's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) - [-]
I have tested this MR in all supported browsers, orit's not needed. - [-]
I have informed the Infrastructure department of a default or new setting change per definition of done, orit's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
💾 Database review
Query updated: !67107 (comment 638647444)