Merge branch '321958-fix-npm-instance-level-api-with-scoped-packages' into 'master'

Fix the npm instance level API with subgroups

See merge request gitlab-org/gitlab!54554

app/models/namespace.rb

@@ -164,6 +164,10 @@ class Namespace < ApplicationRecord
       name = host.delete_suffix(gitlab_host)
       Namespace.where(parent_id: nil).by_path(name)
     end
+
+    def top_most
+      where(parent_id: nil)
+    end
   end
 
   def package_settings

changelogs/unreleased/321958-fix-npm-instance-level-api-with-scoped-packages.yml

+---
+title: Fix the npm instance level API to exclude subgroups
+merge_request: 54554
+author:
+type: fixed

ee/app/models/ee/namespace.rb

@@ -37,8 +37,6 @@ module EE
       scope :include_gitlab_subscription_with_hosted_plan, -> { includes(gitlab_subscription: :hosted_plan) }
       scope :join_gitlab_subscription, -> { joins("LEFT OUTER JOIN gitlab_subscriptions ON gitlab_subscriptions.namespace_id=namespaces.id") }
 
-      scope :top_most, -> { where(parent_id: nil) }
-
       scope :in_active_trial, -> do
         left_joins(gitlab_subscription: :hosted_plan)
           .where(gitlab_subscriptions: { trial: true, trial_ends_on: Date.today.. })

ee/spec/models/ee/namespace_spec.rb

@@ -174,17 +174,6 @@ RSpec.describe Namespace do
       end
     end
 
-    describe '.top_most' do
-      let_it_be(:namespace) { create(:namespace) }
-      let_it_be(:sub_namespace) { create(:namespace, parent: namespace) }
-
-      subject { described_class.top_most.ids }
-
-      it 'only contains root namespace' do
-        is_expected.to eq([namespace.id])
-      end
-    end
-
     describe '.in_active_trial' do
       let_it_be(:namespaces) do
         [

lib/api/helpers/packages/npm.rb

@@ -52,7 +52,8 @@ module API
               namespace_path = namespace_path_from_package_name
               next unless namespace_path
 
-              namespace = namespace_from_path(namespace_path)
+              namespace = Namespace.top_most
+                                   .by_path(namespace_path)
               next unless namespace
 
               finder = ::Packages::Npm::PackageFinder.new(params[:package_name], namespace: namespace)
@@ -70,13 +71,6 @@ module API
 
           package_name.match(Gitlab::Regex.npm_package_name_regex)&.captures&.first
         end
-
-        def namespace_from_path(path)
-          group = Group.by_path(path)
-          return group if group
-
-          Namespace.for_user.by_path(path)
-        end
       end
     end
   end

spec/models/namespace_spec.rb

@@ -285,6 +285,17 @@ RSpec.describe Namespace do
     end
   end
 
+  describe '.top_most' do
+    let_it_be(:namespace) { create(:namespace) }
+    let_it_be(:sub_namespace) { create(:namespace, parent: namespace) }
+
+    subject { described_class.top_most.ids }
+
+    it 'only contains root namespace' do
+      is_expected.to eq([namespace.id])
+    end
+  end
+
   describe '#ancestors_upto' do
     let(:parent) { create(:group) }
     let(:child) { create(:group, parent: parent) }

spec/requests/api/npm_instance_packages_spec.rb

@@ -3,6 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe API::NpmInstancePackages do
+  # We need to create a subgroup with the same name as the hosting group.
+  # It has to be created first to exhibit this bug: https://gitlab.com/gitlab-org/gitlab/-/issues/321958
+  let_it_be(:another_namespace) { create(:group, :public) }
+  let_it_be(:similarly_named_group) { create(:group, :public, parent: another_namespace, name: 'test-group') }
+
   include_context 'npm api setup'
 
   describe 'GET /api/v4/packages/npm/*package_name' do

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

@@ -5,8 +5,9 @@ RSpec.shared_context 'npm api setup' do
   include HttpBasicAuthHelpers
 
   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(:group) { create(:group, name: 'test-group') }
+  let_it_be(:namespace) { group }
+  let_it_be(:project, reload: true) { create(:project, :public, namespace: namespace) }
   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) }

spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb

@@ -45,6 +45,7 @@ RSpec.shared_examples 'handling get metadata requests' do |scope: :project|
     end
   end
 
+  shared_examples 'handling all conditions' do
     where(:auth, :package_name_type, :request_forward, :visibility, :user_role, :expected_result, :expected_status) do
       nil                    | :scoped_naming_convention    | true  | :public   | nil       | :accept   | :ok
       nil                    | :scoped_naming_convention    | false | :public   | nil       | :accept   | :ok
@@ -260,6 +261,19 @@ RSpec.shared_examples 'handling get metadata requests' do |scope: :project|
 
       it_behaves_like example_name, status: status
     end
+  end
+
+  context 'with a group namespace' do
+    it_behaves_like 'handling all conditions'
+  end
+
+  if scope != :project
+    context 'with a user namespace' do
+      let_it_be(:namespace) { user.namespace }
+
+      it_behaves_like 'handling all conditions'
+    end
+  end
 
   context 'with a developer' do
     let(:headers) { build_token_auth_header(personal_access_token.token) }
@@ -371,6 +385,7 @@ RSpec.shared_examples 'handling get dist tags requests' do |scope: :project|
     end
   end
 
+  shared_examples 'handling all conditions' do
     context 'with oauth token' do
       let(:headers) { build_token_auth_header(token.token) }
 
@@ -382,6 +397,19 @@ RSpec.shared_examples 'handling get dist tags requests' do |scope: :project|
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
+  end
+
+  context 'with a group namespace' do
+    it_behaves_like 'handling all conditions'
+  end
+
+  if scope != :project
+    context 'with a user namespace' do
+      let_it_be(:namespace) { user.namespace }
+
+      it_behaves_like 'handling all conditions'
+    end
+  end
 end
 
 RSpec.shared_examples 'handling create dist tag requests' do |scope: :project|
@@ -467,6 +495,7 @@ RSpec.shared_examples 'handling create dist tag requests' do |scope: :project|
     end
   end
 
+  shared_examples 'handling all conditions' do
     context 'with oauth token' do
       let(:headers) { build_token_auth_header(token.token) }
 
@@ -478,6 +507,19 @@ RSpec.shared_examples 'handling create dist tag requests' do |scope: :project|
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
+  end
+
+  context 'with a group namespace' do
+    it_behaves_like 'handling all conditions'
+  end
+
+  if scope != :project
+    context 'with a user namespace' do
+      let_it_be(:namespace) { user.namespace }
+
+      it_behaves_like 'handling all conditions'
+    end
+  end
 end
 
 RSpec.shared_examples 'handling delete dist tag requests' do |scope: :project|
@@ -561,6 +603,7 @@ RSpec.shared_examples 'handling delete dist tag requests' do |scope: :project|
     end
   end
 
+  shared_examples 'handling all conditions' do
     context 'with oauth token' do
       let(:headers) { build_token_auth_header(token.token) }
 
@@ -572,4 +615,17 @@ RSpec.shared_examples 'handling delete dist tag requests' do |scope: :project|
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
+  end
+
+  context 'with a group namespace' do
+    it_behaves_like 'handling all conditions'
+  end
+
+  if scope != :project
+    context 'with a user namespace' do
+      let_it_be(:namespace) { user.namespace }
+
+      it_behaves_like 'handling all conditions'
+    end
+  end
 end