Fix bugs and security concerns in docker group API

Add additional permission checks to container finder
Refactor ContainerRepositoriesFinder
Fix finder to include all group descendents as well
Only include projects with container registry enabled

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) }