Merge branch...

Merge branch '31047-follow-up-from-resolve-api-endpoint-to-list-the-docker-images-tags-of-a-group' into 'master'

Fix bugs and security concerns in docker group API

See merge request gitlab-org/gitlab!19246

app/finders/container_repositories_finder.rb

 # frozen_string_literal: true
 
 class ContainerRepositoriesFinder
-  # id: group or project id
-  # container_type: :group or :project
-  def initialize(id:, container_type:)
-    @id = id
-    @type = container_type.to_sym
+  VALID_SUBJECTS = [Group, Project].freeze
+
+  def initialize(user:, subject:)
+    @user = user
+    @subject = subject
   end
 
   def execute
-    if project_type?
-      project.container_repositories
-    else
-      group.container_repositories
-    end
+    raise ArgumentError, "invalid subject_type" unless valid_subject_type?
+    return unless authorized?
+
+    return project_repositories if @subject.is_a?(Project)
+    return group_repositories if @subject.is_a?(Group)
   end
 
   private
 
-  attr_reader :id, :type
+  def valid_subject_type?
+    VALID_SUBJECTS.include?(@subject.class)
+  end
+
+  def project_repositories
+    return unless @subject.container_registry_enabled
 
-  def project_type?
-    type == :project
+    @subject.container_repositories
   end
 
-  def project
-    Project.find(id)
+  def group_repositories
+    ContainerRepository.for_group_and_its_subgroups(@subject)
   end
 
-  def group
-    Group.find(id)
+  def authorized?
+    Ability.allowed?(@user, :read_container_image, @subject)
   end
 end

app/models/container_repository.rb

@@ -12,6 +12,9 @@ class ContainerRepository < ApplicationRecord
 
   scope :ordered, -> { order(:name) }
   scope :with_api_entity_associations, -> { preload(project: [:route, { namespace: :route }]) }
+  scope :for_group_and_its_subgroups, ->(group) do
+    where(project_id: Project.for_group_and_its_subgroups(group).with_container_registry.select(:id))
+  end
 
   # rubocop: disable CodeReuse/ServiceClass
   def registry

app/models/project.rb

@@ -395,6 +395,7 @@ class Project < ApplicationRecord
   scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
   scope :with_statistics, -> { includes(:statistics) }
   scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
+  scope :with_container_registry, -> { where(container_registry_enabled: true) }
   scope :inside_path, ->(path) do
     # We need routes alias rs for JOIN so it does not conflict with
     # includes(:route) which we use in ProjectsFinder.

lib/api/group_container_repositories.rb

@@ -23,7 +23,7 @@ module API
       end
       get ':id/registry/repositories' do
         repositories = ContainerRepositoriesFinder.new(
-          id: user_group.id, container_type: :group
+          user: current_user, subject: user_group
         ).execute
 
         present paginate(repositories), with: Entities::ContainerRegistry::Repository, tags: params[:tags]

lib/api/project_container_repositories.rb

@@ -24,7 +24,7 @@ module API
       end
       get ':id/registry/repositories' do
         repositories = ContainerRepositoriesFinder.new(
-          id: user_project.id, container_type: :project
+          user: current_user, subject: user_project
         ).execute
 
         present paginate(repositories), with: Entities::ContainerRegistry::Repository, tags: params[:tags]

spec/finders/container_repositories_finder_spec.rb

@@ -3,42 +3,50 @@
 require 'spec_helper'
 
 describe ContainerRepositoriesFinder do
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+
   let(:group) { create(:group) }
   let(:project) { create(:project, group: group) }
   let(:project_repository) { create(:container_repository, project: project) }
 
+  before do
+    group.add_reporter(reporter)
+    project.add_reporter(reporter)
+  end
+
   describe '#execute' do
-    let(:id) { nil }
+    context 'with authorized user' do
+      subject { described_class.new(user: reporter, subject: subject_object).execute }
 
-    subject { described_class.new(id: id, container_type: container_type).execute }
+      context 'when subject_type is group' do
+        let(:subject_object) { group }
+        let(:other_project) { create(:project, group: group) }
 
-    context 'when container_type is group' do
-      let(:other_project) { create(:project, group: group) }
+        let(:other_repository) do
+          create(:container_repository, name: 'test_repository2', project: other_project)
+        end
 
-      let(:other_repository) do
-        create(:container_repository, name: 'test_repository2', project: other_project)
+        it { is_expected.to match_array([project_repository, other_repository]) }
       end
 
-      let(:container_type) { :group }
-      let(:id) { group.id }
+      context 'when subject_type is project' do
+        let(:subject_object) { project }
 
-      it { is_expected.to match_array([project_repository, other_repository]) }
-    end
+        it { is_expected.to match_array([project_repository]) }
+      end
 
-    context 'when container_type is project' do
-      let(:container_type) { :project }
-      let(:id) { project.id }
+      context 'with invalid subject_type' do
+        let(:subject_object) { "invalid type" }
 
-      it { is_expected.to match_array([project_repository]) }
+        it { expect { subject }.to raise_exception('invalid subject_type') }
+      end
     end
 
-    context 'with invalid id' do
-      let(:container_type) { :project }
-      let(:id) { 123456789 }
+    context 'with unauthorized user' do
+      subject { described_class.new(user: guest, subject: group).execute }
 
-      it 'raises an error' do
-        expect { subject.execute }.to raise_error(ActiveRecord::RecordNotFound)
-      end
+      it { is_expected.to be nil }
     end
   end
 end

spec/models/container_repository_spec.rb

@@ -235,4 +235,36 @@ describe ContainerRepository do
       expect(repository).not_to be_persisted
     end
   end
+
+  describe '.for_group_and_its_subgroups' do
+    subject { described_class.for_group_and_its_subgroups(test_group) }
+
+    context 'in a group' do
+      let(:test_group) { group }
+
+      it { is_expected.to contain_exactly(repository) }
+    end
+
+    context 'with a subgroup' do
+      let(:test_group) { create(:group) }
+      let(:another_project) { create(:project, path: 'test', group: test_group) }
+
+      let(:another_repository) do
+        create(:container_repository, name: 'my_image', project: another_project)
+      end
+
+      before do
+        group.parent = test_group
+        group.save
+      end
+
+      it { is_expected.to contain_exactly(repository, another_repository) }
+    end
+
+    context 'group without container_repositories' do
+      let(:test_group) { create(:group) }
+
+      it { is_expected.to eq([]) }
+    end
+  end
 end

spec/requests/api/group_container_repositories_spec.rb

@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 describe API::GroupContainerRepositories do
-  set(:group) { create(:group, :private) }
-  set(:project) { create(:project, :private, group: group) }
-  let(:reporter) { create(:user) }
-  let(:guest) { create(:user) }
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, :private, group: group) }
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:guest) { create(:user) }
 
   let(:root_repository) { create(:container_repository, :root, project: project) }
   let(:test_repository) { create(:container_repository, project: project) }