Refactor NPM requests specs for shared endpoints

Use tabled based specs to better test different conditions.
Make sure that we use the Bearer http authorization header and not the
request parameter.
Move package tags shared examples in a dedicated file for npm specs.

spec/requests/api/npm_instance_packages_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe API::NpmInstancePackages do
   include_context 'npm api setup'
 
   describe 'GET /api/v4/packages/npm/*package_name' do
-    it_behaves_like 'handling get metadata requests' do
+    it_behaves_like 'handling get metadata requests', scope: :instance do
       let(:url) { api("/packages/npm/#{package_name}") }
     end
   end

spec/requests/api/npm_project_packages_spec.rb

@@ -6,25 +6,25 @@ RSpec.describe API::NpmProjectPackages do
   include_context 'npm api setup'
 
   describe 'GET /api/v4/projects/:id/packages/npm/*package_name' do
-    it_behaves_like 'handling get metadata requests' do
+    it_behaves_like 'handling get metadata requests', scope: :project do
       let(:url) { api("/projects/#{project.id}/packages/npm/#{package_name}") }
     end
   end
 
   describe 'GET /api/v4/projects/:id/packages/npm/-/package/*package_name/dist-tags' do
-    it_behaves_like 'handling get dist tags requests' do
+    it_behaves_like 'handling get dist tags requests', scope: :project do
       let(:url) { api("/projects/#{project.id}/packages/npm/-/package/#{package_name}/dist-tags") }
     end
   end
 
   describe 'PUT /api/v4/projects/:id/packages/npm/-/package/*package_name/dist-tags/:tag' do
-    it_behaves_like 'handling create dist tag requests' do
+    it_behaves_like 'handling create dist tag requests', scope: :project do
       let(:url) { api("/projects/#{project.id}/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
     end
   end
 
   describe 'DELETE /api/v4/projects/:id/packages/npm/-/package/*package_name/dist-tags/:tag' do
-    it_behaves_like 'handling delete dist tag requests' do
+    it_behaves_like 'handling delete dist tag requests', scope: :project do
       let(:url) { api("/projects/#{project.id}/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
     end
   end
@@ -32,10 +32,14 @@ RSpec.describe API::NpmProjectPackages do
   describe 'GET /api/v4/projects/:id/packages/npm/*package_name/-/*file_name' do
     let_it_be(:package_file) { package.package_files.first }
 
-    let(:params) { {} }
-    let(:url) { api("/projects/#{project.id}/packages/npm/#{package_file.package.name}/-/#{package_file.file_name}") }
+    let(:headers) { {} }
+    let(:url) { api("/projects/#{project.id}/packages/npm/#{package.name}/-/#{package_file.file_name}") }
 
-    subject { get(url, params: params) }
+    subject { get(url, headers: headers) }
+
+    before do
+      project.add_developer(user)
+    end
 
     shared_examples 'a package file that requires auth' do
       it 'denies download with no token' do
@@ -45,7 +49,7 @@ RSpec.describe API::NpmProjectPackages do
       end
 
       context 'with access token' do
-        let(:params) { { access_token: token.token } }
+        let(:headers) { build_token_auth_header(token.token) }
 
         it 'returns the file' do
           subject
@@ -56,7 +60,7 @@ RSpec.describe API::NpmProjectPackages do
       end
 
       context 'with job token' do
-        let(:params) { { job_token: job.token } }
+        let(:headers) { build_token_auth_header(job.token) }
 
         it 'returns the file' do
           subject
@@ -86,7 +90,7 @@ RSpec.describe API::NpmProjectPackages do
       it_behaves_like 'a package file that requires auth'
 
       context 'with guest' do
-        let(:params) { { access_token: token.token } }
+        let(:headers) { build_token_auth_header(token.token) }
 
         it 'denies download when not enough permissions' do
           project.add_guest(user)
@@ -108,7 +112,11 @@ RSpec.describe API::NpmProjectPackages do
   end
 
   describe 'PUT /api/v4/projects/:id/packages/npm/:package_name' do
-    RSpec.shared_examples 'handling invalid record with 400 error' do
+    before do
+      project.add_developer(user)
+    end
+
+    shared_examples 'handling invalid record with 400 error' do
       it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
         expect { upload_package_with_token(package_name, params) }
           .not_to change { project.packages.count }
@@ -261,7 +269,9 @@ RSpec.describe API::NpmProjectPackages do
     end
 
     def upload_package(package_name, params = {})
-      put api("/projects/#{project.id}/packages/npm/#{package_name.sub('/', '%2f')}"), params: params
+      token = params.delete(:access_token) || params.delete(:job_token)
+      headers = build_token_auth_header(token)
+      put api("/projects/#{project.id}/packages/npm/#{package_name.sub('/', '%2f')}"), params: params, headers: headers
     end
 
     def upload_package_with_token(package_name, params = {})

spec/support/shared_contexts/requests/api/npm_packages_shared_context.rb

@@ -4,10 +4,10 @@ RSpec.shared_context 'npm api setup' do
   include PackagesManagerApiSpecHelpers
   include HttpBasicAuthHelpers
 
-  let_it_be(:user) { create(:user) }
+  let_it_be(:user, reload: true) { create(:user) }
   let_it_be(:group) { create(:group) }
   let_it_be(:project, reload: true) { create(:project, :public, namespace: group) }
-  let_it_be(:package, reload: true) { create(:npm_package, project: project) }
+  let_it_be(:package, reload: true) { create(:npm_package, project: project, name: "@#{group.path}/scoped_package") }
   let_it_be(:token) { create(:oauth_access_token, scopes: 'api', resource_owner: user) }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
   let_it_be(:job, reload: true) { create(:ci_build, user: user, status: :running) }
@@ -15,8 +15,15 @@ RSpec.shared_context 'npm api setup' do
   let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) }
 
   let(:package_name) { package.name }
+end
 
-  before do
-    project.add_developer(user)
+RSpec.shared_context 'set package name from package name type' do
+  let(:package_name) do
+    case package_name_type
+    when :scoped_naming_convention
+      "@#{group.path}/scoped-package"
+    when :non_existing
+      'non-existing-package'
+    end
   end
 end

spec/support/shared_examples/requests/api/packages_tags_shared_examples.rb → spec/support/shared_examples/requests/api/npm_packages_tags_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'rejects package tags access' do |user_type, status|
-  context "for user type #{user_type}" do
-    before do
-      project.send("add_#{user_type}", user) unless user_type == :no_type
-    end
-
-    it_behaves_like 'returning response status', status
+RSpec.shared_examples 'rejects package tags access' do |status:|
+  before do
+    package.update!(name: package_name) unless package_name == 'non-existing-package'
   end
+
+  it_behaves_like 'returning response status', status
 end
 
-RSpec.shared_examples 'returns package tags' do |user_type|
+RSpec.shared_examples 'accept package tags request' do |status:|
   using RSpec::Parameterized::TableSyntax
 
   before do
     stub_application_setting(npm_package_requests_forwarding: false)
-    project.send("add_#{user_type}", user) unless user_type == :no_type
   end
 
-  it_behaves_like 'returning response status', :success
+  context 'with valid package name' do
+    before do
+      package.update!(name: package_name) unless package_name == 'non-existing-package'
+    end
 
-  it 'returns a valid json response' do
-    subject
+    it_behaves_like 'returning response status', status
 
-    expect(response.media_type).to eq('application/json')
-    expect(json_response).to be_a(Hash)
-  end
+    it 'returns a valid json response' do
+      subject
 
-  it 'returns two package tags' do
-    subject
+      expect(response.media_type).to eq('application/json')
+      expect(json_response).to be_a(Hash)
+    end
 
-    expect(json_response).to match_schema('public_api/v4/packages/npm_package_tags')
-    expect(json_response.length).to eq(3) # two tags + latest (auto added)
-    expect(json_response[package_tag1.name]).to eq(package.version)
-    expect(json_response[package_tag2.name]).to eq(package.version)
-    expect(json_response['latest']).to eq(package.version)
+    it 'returns two package tags' do
+      subject
+
+      expect(json_response).to match_schema('public_api/v4/packages/npm_package_tags')
+      expect(json_response.length).to eq(3) # two tags + latest (auto added)
+      expect(json_response[package_tag1.name]).to eq(package.version)
+      expect(json_response[package_tag2.name]).to eq(package.version)
+      expect(json_response['latest']).to eq(package.version)
+    end
   end
 
   context 'with invalid package name' do
@@ -49,47 +52,49 @@ RSpec.shared_examples 'returns package tags' do |user_type|
   end
 end
 
-RSpec.shared_examples 'create package tag' do |user_type|
+RSpec.shared_examples 'accept create package tag request' do |user_type|
   using RSpec::Parameterized::TableSyntax
 
-  before do
-    project.send("add_#{user_type}", user) unless user_type == :no_type
-  end
+  context 'with valid package name' do
+    before do
+      package.update!(name: package_name) unless package_name == 'non-existing-package'
+    end
 
-  it_behaves_like 'returning response status', :no_content
+    it_behaves_like 'returning response status', :no_content
 
-  it 'creates the package tag' do
-    expect { subject }.to change { Packages::Tag.count }.by(1)
+    it 'creates the package tag' do
+      expect { subject }.to change { Packages::Tag.count }.by(1)
 
-    last_tag = Packages::Tag.last
-    expect(last_tag.name).to eq(tag_name)
-    expect(last_tag.package).to eq(package)
-  end
+      last_tag = Packages::Tag.last
+      expect(last_tag.name).to eq(tag_name)
+      expect(last_tag.package).to eq(package)
+    end
 
-  it 'returns a valid response' do
-    subject
+    it 'returns a valid response' do
+      subject
 
-    expect(response.body).to be_empty
-  end
+      expect(response.body).to be_empty
+    end
 
-  context 'with already existing tag' do
-    let_it_be(:package2) { create(:npm_package, project: project, name: package.name, version: '5.5.55') }
-    let_it_be(:tag) { create(:packages_tag, package: package2, name: tag_name) }
+    context 'with already existing tag' do
+      let_it_be(:package2) { create(:npm_package, project: project, name: package.name, version: '5.5.55') }
+      let_it_be(:tag) { create(:packages_tag, package: package2, name: tag_name) }
 
-    it_behaves_like 'returning response status', :no_content
+      it_behaves_like 'returning response status', :no_content
 
-    it 'reuses existing tag' do
-      expect(package.tags).to be_empty
-      expect(package2.tags).to eq([tag])
-      expect { subject }.to not_change { Packages::Tag.count }
-      expect(package.reload.tags).to eq([tag])
-      expect(package2.reload.tags).to be_empty
-    end
+      it 'reuses existing tag' do
+        expect(package.tags).to be_empty
+        expect(package2.tags).to eq([tag])
+        expect { subject }.to not_change { Packages::Tag.count }
+        expect(package.reload.tags).to eq([tag])
+        expect(package2.reload.tags).to be_empty
+      end
 
-    it 'returns a valid response' do
-      subject
+      it 'returns a valid response' do
+        subject
 
-      expect(response.body).to be_empty
+        expect(response.body).to be_empty
+      end
     end
   end
 
@@ -129,14 +134,14 @@ RSpec.shared_examples 'create package tag' do |user_type|
   end
 end
 
-RSpec.shared_examples 'delete package tag' do |user_type|
+RSpec.shared_examples 'accept delete package tag request' do |user_type|
   using RSpec::Parameterized::TableSyntax
 
-  before do
-    project.send("add_#{user_type}", user) unless user_type == :no_type
-  end
+  context 'with valid package name' do
+    before do
+      package.update!(name: package_name) unless package_name == 'non-existing-package'
+    end
 
-  context "for #{user_type} user" do
     it_behaves_like 'returning response status', :no_content
 
     it 'returns a valid response' do
@@ -157,29 +162,29 @@ RSpec.shared_examples 'delete package tag' do |user_type|
 
       it_behaves_like 'returning response status', :not_found
     end
+  end
 
-    context 'with invalid package name' do
-      where(:package_name, :status) do
-        'unknown' | :not_found
-        ''        | :not_found
-        '%20'     | :bad_request
-      end
+  context 'with invalid package name' do
+    where(:package_name, :status) do
+      'unknown' | :not_found
+      ''        | :not_found
+      '%20'     | :bad_request
+    end
 
-      with_them do
-        it_behaves_like 'returning response status', params[:status]
-      end
+    with_them do
+      it_behaves_like 'returning response status', params[:status]
     end
+  end
 
-    context 'with invalid tag name' do
-      where(:tag_name, :status) do
-        'unknown' | :not_found
-        ''        | :not_found
-        '%20'     | :bad_request
-      end
+  context 'with invalid tag name' do
+    where(:tag_name, :status) do
+      'unknown' | :not_found
+      ''        | :not_found
+      '%20'     | :bad_request
+    end
 
-      with_them do
-        it_behaves_like 'returning response status', params[:status]
-      end
+    with_them do
+      it_behaves_like 'returning response status', params[:status]
     end
   end
 end