Skip to content

Refactor NPM requests specs for shared endpoints

David Fernandez requested to merge 33685-refactor-npm-requests-specs into master

This MR was extracted from !53491 (merged) changes.

Related issue: #33685 (closed)

🎱 Context

The ~"NPM Registry" API can be accessed at two levels:

  • Instance
  • Project

A number of API endpoints are present in both levels. As such, they're centralized in a single file.

Likewise, the requests specs for those endpoints are centralized with shared examples.

While working on !53491 (merged) that aims to remove the naming convention at the project level, I noticed the following:

  1. The current shared examples don't really go through all the possible conditions. For example, for the metadata endpoint, we have these variables to play with:
    • Project visibility: public, private, internal
    • User role on the project: anonymous, guest, reporter, maintainer
    • Request forward enabled or not
    • Package name style: following the naming convention or not.
      • This is not a huge factor in this MR but we will play with this a lot on !53491 (merged)
  2. The specs don't authenticate the user in the same way as $ npm would do.
    • They use a request paremeter whereas $ npm uses the Bearer http Authorization header.
    • This can lead to issues when we will move to the new authentication logic we're working on for package managers API.
      • This logic will reject any authentication method that has not been explicitly allowed on the API and request parameters will not be allowed 😺

Given that the ~"NPM Registry" is one of the most used package registry among all types, I think it would be better to go through as many conditions as possible. This way, when working on deep changes such as !53491 (merged), a 💚 specs suite will bring us more confidence.

🤔 What does this MR do?

Refactor the shared examples for the npm request specs:

  • Use table based specs to better describe the different conditions.
    • Take into account the exceptional cases that can happen when we are at the instance level.
      • Here is an example: a guest user asking a non existing package on private project:
        • At the project level, the response will be forbidden because we located the project but the user can't read the packages (he needs to be at least reporter).
        • At the instance level, there is no way to locate the project = we end up replying not found.
  • Send the Authorization: Bearer X header for authenticate the request
  • Move the shared example for package tags to a dedicated file for npm.
  • No feature change in this MR
    • Thus, this MR doesn't need a changelog entry
    • or a documentation change

🖥 Screenshots (strongly suggested)

🔍 Before the MR

$ bundle exec spring rspec /Users/david/projects/gitlab-development-kit/gitlab/spec/requests/api/npm_instance_packages_spec.rb /Users/david/projects/gitlab-development-kit/gitlab/spec/requests/api/npm_project_packages_spec.rb
# [snip..snip]
Finished in 1 minute 43.68 seconds (files took 4.47 seconds to load)
262 examples, 0 failures

📈 With this MR

$ bundle exec spring rspec /Users/david/projects/gitlab-development-kit/gitlab/spec/requests/api/npm_instance_packages_spec.rb /Users/david/projects/gitlab-development-kit/gitlab/spec/requests/api/npm_project_packages_spec.rb  
# [snip..snip]
Finished in 4 minutes 7.1 seconds (files took 5.28 seconds to load)
712 examples, 0 failures

🚓 Does this MR meet the acceptance criteria?

⚙ Conformity

🔩 Availability and Testing

🔒 Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] 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
Edited by David Fernandez

Merge request reports