Merge branch '18792-modify-container-image-policies' into 'master'

Modify container_image policies to check container_registry_access_level [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55071

app/models/container_repository.rb

@@ -24,8 +24,15 @@ class ContainerRepository < ApplicationRecord
   scope :for_group_and_its_subgroups, ->(group) do
     project_scope = Project
       .for_group_and_its_subgroups(group)
-      .with_container_registry
-      .select(:id)
+
+    project_scope =
+      if Feature.enabled?(:read_container_registry_access_level, group, default_enabled: :yaml)
+        project_scope.with_feature_enabled(:container_registry)
+      else
+        project_scope.with_container_registry
+      end
+
+    project_scope = project_scope.select(:id)
 
     joins("INNER JOIN (#{project_scope.to_sql}) projects on projects.id=container_repositories.project_id")
   end

app/models/project.rb

@@ -2612,6 +2612,15 @@ class Project < ApplicationRecord
     !!read_attribute(:merge_requests_author_approval)
   end
 
+  def container_registry_enabled
+    if Feature.enabled?(:read_container_registry_access_level, self.namespace, default_enabled: :yaml)
+      project_feature.container_registry_enabled?
+    else
+      read_attribute(:container_registry_enabled)
+    end
+  end
+  alias_method :container_registry_enabled?, :container_registry_enabled
+
   private
 
   def set_container_registry_access_level

app/models/project_feature.rb

@@ -24,7 +24,11 @@ class ProjectFeature < ApplicationRecord
 
   set_available_features(FEATURES)
 
-  PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER, metrics_dashboard: Gitlab::Access::REPORTER }.freeze
+  PRIVATE_FEATURES_MIN_ACCESS_LEVEL = {
+    merge_requests: Gitlab::Access::REPORTER,
+    metrics_dashboard: Gitlab::Access::REPORTER,
+    container_registry: Gitlab::Access::REPORTER
+  }.freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
 
   class << self
@@ -92,7 +96,7 @@ class ProjectFeature < ApplicationRecord
 
   def set_container_registry_access_level
     self.container_registry_access_level =
-      if project&.container_registry_enabled
+      if project&.read_attribute(:container_registry_enabled)
         ENABLED
       else
         DISABLED

app/policies/project_policy.rb

@@ -51,7 +51,11 @@ class ProjectPolicy < BasePolicy
 
   desc "Container registry is disabled"
   condition(:container_registry_disabled, scope: :subject) do
-    !project.container_registry_enabled
+    if ::Feature.enabled?(:read_container_registry_access_level, @subject&.namespace, default_enabled: :yaml)
+      !access_allowed_to?(:container_registry)
+    else
+      !project.container_registry_enabled
+    end
   end
 
   desc "Project has an external wiki"

config/feature_flags/development/read_container_registry_access_level.yml

+---
+name: read_container_registry_access_level
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55071
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332751
+milestone: '14.0'
+type: development
+group: group::package
+default_enabled: false

db/post_migrate/20210606143426_add_index_for_container_registry_access_level.rb

+# frozen_string_literal: true
+
+class AddIndexForContainerRegistryAccessLevel < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::SchemaHelpers
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX = 'index_project_features_on_project_id_include_container_registry'
+
+  def up
+    if index_exists_by_name?('project_features', INDEX)
+      Gitlab::AppLogger.warn "Index not created because it already exists (this may be due to an aborted migration or similar): table_name: project_features, index_name: #{INDEX}"
+      return
+    end
+
+    begin
+      disable_statement_timeout do
+        execute "CREATE UNIQUE INDEX CONCURRENTLY #{INDEX} ON project_features " \
+          'USING btree (project_id) INCLUDE (container_registry_access_level)'
+      end
+    rescue ActiveRecord::StatementInvalid => ex
+      raise "The index #{INDEX} couldn't be added: #{ex.message}"
+    end
+
+    create_comment(
+      'INDEX',
+      INDEX,
+      'Included column (container_registry_access_level) improves performance of the ContainerRepository.for_group_and_its_subgroups scope query'
+    )
+  end
+
+  def down
+    remove_concurrent_index_by_name('project_features', INDEX)
+  end
+end

db/schema_migrations/20210606143426

+1f99d446428ddac2a0fa7d64bdce9fc300bf02e88c35cdb3d726c501641e721d
\ No newline at end of file

db/structure.sql

@@ -24144,6 +24144,10 @@ CREATE UNIQUE INDEX index_project_features_on_project_id ON project_features USI
 
 CREATE INDEX index_project_features_on_project_id_bal_20 ON project_features USING btree (project_id) WHERE (builds_access_level = 20);
 
+CREATE UNIQUE INDEX index_project_features_on_project_id_include_container_registry ON project_features USING btree (project_id) INCLUDE (container_registry_access_level);
+
+COMMENT ON INDEX index_project_features_on_project_id_include_container_registry IS 'Included column (container_registry_access_level) improves performance of the ContainerRepository.for_group_and_its_subgroups scope query';
+
 CREATE INDEX index_project_features_on_project_id_ral_20 ON project_features USING btree (project_id) WHERE (repository_access_level = 20);
 
 CREATE INDEX index_project_group_links_on_group_id ON project_group_links USING btree (group_id);

ee/spec/policies/project_policy_spec.rb

@@ -9,6 +9,8 @@ RSpec.describe ProjectPolicy do
 
   let(:project) { public_project }
 
+  let_it_be(:auditor) { create(:user, :auditor) }
+
   subject { described_class.new(current_user, project) }
 
   before do
@@ -58,7 +60,7 @@ RSpec.describe ProjectPolicy do
     it_behaves_like 'project policies as admin without admin mode'
 
     context 'auditor' do
-      let(:current_user) { create(:user, :auditor) }
+      let(:current_user) { auditor }
 
       before do
         stub_licensed_features(security_dashboard: true, license_scanning: true, threat_monitoring: true)
@@ -91,6 +93,45 @@ RSpec.describe ProjectPolicy do
       it_behaves_like 'project private features with read_all_resources ability' do
         let(:user) { current_user }
       end
+
+      context 'with project feature related policies' do
+        using RSpec::Parameterized::TableSyntax
+
+        project_features = {
+          container_registry_access_level: [:read_container_image],
+          merge_requests_access_level: [:read_merge_request]
+        }
+
+        where(:project_visibility, :access_level, :allowed) do
+          :public   | ProjectFeature::ENABLED  | true
+          :public   | ProjectFeature::PRIVATE  | true
+          :public   | ProjectFeature::DISABLED | false
+          :internal | ProjectFeature::ENABLED  | true
+          :internal | ProjectFeature::PRIVATE  | true
+          :internal | ProjectFeature::DISABLED | false
+          :private  | ProjectFeature::ENABLED  | true
+          :private  | ProjectFeature::PRIVATE  | true
+          :private  | ProjectFeature::DISABLED | false
+        end
+
+        # For each project feature, check that an auditor is always allowed read
+        # permissions unless the feature is disabled.
+        project_features.each do |feature, permissions|
+          with_them do
+            let(:project) { send("#{project_visibility}_project") }
+
+            it 'always allows permissions except when feature disabled' do
+              project.project_feature.update!("#{feature}": access_level)
+
+              if allowed
+                expect_allowed(*permissions)
+              else
+                expect_disallowed(*permissions)
+              end
+            end
+          end
+        end
+      end
     end
   end
 

lib/api/entities/project.rb

@@ -43,7 +43,6 @@ module API
       expose :visibility
       expose :owner, using: Entities::UserBasic, unless: ->(project, options) { project.group }
       expose :resolve_outdated_diff_discussions
-      expose :container_registry_enabled
       expose :container_expiration_policy, using: Entities::ContainerExpirationPolicy,
         if: -> (project, _) { project.container_expiration_policy }
 
@@ -54,6 +53,13 @@ module API
       expose(:wiki_enabled) { |project, options| project.feature_available?(:wiki, options[:current_user]) }
       expose(:jobs_enabled) { |project, options| project.feature_available?(:builds, options[:current_user]) }
       expose(:snippets_enabled) { |project, options| project.feature_available?(:snippets, options[:current_user]) }
+      expose(:container_registry_enabled) do |project, options|
+        if ::Feature.enabled?(:read_container_registry_access_level, project.namespace, default_enabled: :yaml)
+          project.feature_available?(:container_registry, options[:current_user])
+        else
+          project.read_attribute(:container_registry_enabled)
+        end
+      end
       expose :service_desk_enabled
       expose :service_desk_address
 

spec/lib/sidebars/projects/menus/packages_registries_menu_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
-  let(:project) { build(:project) }
+  let_it_be(:project) { create(:project) }
+
   let(:user) { project.owner }
   let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project) }
 

spec/lib/sidebars/projects/menus/settings_menu_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Sidebars::Projects::Menus::SettingsMenu do
-  let(:project) { build(:project) }
+  let_it_be(:project) { create(:project) }
+
   let(:user) { project.owner }
   let(:context) { Sidebars::Projects::Context.new(current_user: user, container: project) }
 

spec/models/container_repository_spec.rb

@@ -331,6 +331,40 @@ RSpec.describe ContainerRepository do
 
       it { is_expected.to eq([]) }
     end
+
+    context 'with read_container_registry_access_level disabled' do
+      before do
+        stub_feature_flags(read_container_registry_access_level: false)
+      end
+
+      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
 
   describe '.search_by_name' do

spec/models/project_spec.rb

@@ -653,6 +653,16 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to delegate_method(:root_ancestor).to(:namespace).with_arguments(allow_nil: true) }
     it { is_expected.to delegate_method(:last_pipeline).to(:commit).with_arguments(allow_nil: true) }
     it { is_expected.to delegate_method(:allow_editing_commit_messages?).to(:project_setting) }
+    it { is_expected.to delegate_method(:container_registry_enabled?).to(:project_feature) }
+    it { is_expected.to delegate_method(:container_registry_access_level).to(:project_feature) }
+
+    context 'when read_container_registry_access_level is disabled' do
+      before do
+        stub_feature_flags(read_container_registry_access_level: false)
+      end
+
+      it { is_expected.not_to delegate_method(:container_registry_enabled?).to(:project_feature) }
+    end
   end
 
   describe 'reference methods' do
@@ -2285,35 +2295,55 @@ RSpec.describe Project, factory_default: :keep do
     it 'updates project_feature', :aggregate_failures do
       # Simulate an existing project that has container_registry enabled
       project.update_column(:container_registry_enabled, true)
-      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
-
-      expect(project.container_registry_enabled).to eq(true)
-      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::ENABLED)
 
       project.update!(container_registry_enabled: false)
 
-      expect(project.container_registry_enabled).to eq(false)
+      expect(project.read_attribute(:container_registry_enabled)).to eq(false)
       expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
 
       project.update!(container_registry_enabled: true)
 
-      expect(project.container_registry_enabled).to eq(true)
+      expect(project.read_attribute(:container_registry_enabled)).to eq(true)
       expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::ENABLED)
     end
 
     it 'rollsback both projects and project_features row in case of error', :aggregate_failures do
       project.update_column(:container_registry_enabled, true)
-      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
-
-      expect(project.container_registry_enabled).to eq(true)
-      expect(project.project_feature.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::ENABLED)
 
       allow(project).to receive(:valid?).and_return(false)
 
       expect { project.update!(container_registry_enabled: false) }.to raise_error(ActiveRecord::RecordInvalid)
 
-      expect(project.reload.container_registry_enabled).to eq(true)
-      expect(project.project_feature.reload.container_registry_access_level).to eq(ProjectFeature::DISABLED)
+      expect(project.reload.read_attribute(:container_registry_enabled)).to eq(true)
+      expect(project.project_feature.reload.container_registry_access_level).to eq(ProjectFeature::ENABLED)
+    end
+  end
+
+  describe '#container_registry_enabled' do
+    let_it_be_with_reload(:project) { create(:project) }
+
+    it 'delegates to project_feature', :aggregate_failures do
+      project.update_column(:container_registry_enabled, true)
+      project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
+
+      expect(project.container_registry_enabled).to eq(false)
+      expect(project.container_registry_enabled?).to eq(false)
+    end
+
+    context 'with read_container_registry_access_level disabled' do
+      before do
+        stub_feature_flags(read_container_registry_access_level: false)
+      end
+
+      it 'reads project.container_registry_enabled' do
+        project.update_column(:container_registry_enabled, true)
+        project.project_feature.update_column(:container_registry_access_level, ProjectFeature::DISABLED)
+
+        expect(project.container_registry_enabled).to eq(true)
+        expect(project.container_registry_enabled?).to eq(true)
+      end
     end
   end
 

spec/policies/project_policy_spec.rb

@@ -1434,4 +1434,165 @@ RSpec.describe ProjectPolicy do
       end
     end
   end
+
+  describe 'container_image policies' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:guest_operations_permissions) { [:read_container_image] }
+
+    let(:developer_operations_permissions) do
+      guest_operations_permissions + [
+        :create_container_image, :update_container_image, :destroy_container_image
+      ]
+    end
+
+    let(:maintainer_operations_permissions) do
+      developer_operations_permissions + [
+        :admin_container_image
+      ]
+    end
+
+    where(:project_visibility, :access_level, :role, :allowed) do
+      :public   | ProjectFeature::ENABLED   | :maintainer | true
+      :public   | ProjectFeature::ENABLED   | :developer  | true
+      :public   | ProjectFeature::ENABLED   | :reporter   | true
+      :public   | ProjectFeature::ENABLED   | :guest      | true
+      :public   | ProjectFeature::ENABLED   | :anonymous  | true
+      :public   | ProjectFeature::PRIVATE   | :maintainer | true
+      :public   | ProjectFeature::PRIVATE   | :developer  | true
+      :public   | ProjectFeature::PRIVATE   | :reporter   | true
+      :public   | ProjectFeature::PRIVATE   | :guest      | false
+      :public   | ProjectFeature::PRIVATE   | :anonymous  | false
+      :public   | ProjectFeature::DISABLED  | :maintainer | false
+      :public   | ProjectFeature::DISABLED  | :developer  | false
+      :public   | ProjectFeature::DISABLED  | :reporter   | false
+      :public   | ProjectFeature::DISABLED  | :guest      | false
+      :public   | ProjectFeature::DISABLED  | :anonymous  | false
+      :internal | ProjectFeature::ENABLED   | :maintainer | true
+      :internal | ProjectFeature::ENABLED   | :developer  | true
+      :internal | ProjectFeature::ENABLED   | :reporter   | true
+      :internal | ProjectFeature::ENABLED   | :guest      | true
+      :internal | ProjectFeature::ENABLED   | :anonymous  | false
+      :internal | ProjectFeature::PRIVATE   | :maintainer | true
+      :internal | ProjectFeature::PRIVATE   | :developer  | true
+      :internal | ProjectFeature::PRIVATE   | :reporter   | true
+      :internal | ProjectFeature::PRIVATE   | :guest      | false
+      :internal | ProjectFeature::PRIVATE   | :anonymous  | false
+      :internal | ProjectFeature::DISABLED  | :maintainer | false
+      :internal | ProjectFeature::DISABLED  | :developer  | false
+      :internal | ProjectFeature::DISABLED  | :reporter   | false
+      :internal | ProjectFeature::DISABLED  | :guest      | false
+      :internal | ProjectFeature::DISABLED  | :anonymous  | false
+      :private  | ProjectFeature::ENABLED   | :maintainer | true
+      :private  | ProjectFeature::ENABLED   | :developer  | true
+      :private  | ProjectFeature::ENABLED   | :reporter   | true
+      :private  | ProjectFeature::ENABLED   | :guest      | false
+      :private  | ProjectFeature::ENABLED   | :anonymous  | false
+      :private  | ProjectFeature::PRIVATE   | :maintainer | true
+      :private  | ProjectFeature::PRIVATE   | :developer  | true
+      :private  | ProjectFeature::PRIVATE   | :reporter   | true
+      :private  | ProjectFeature::PRIVATE   | :guest      | false
+      :private  | ProjectFeature::PRIVATE   | :anonymous  | false
+      :private  | ProjectFeature::DISABLED  | :maintainer | false
+      :private  | ProjectFeature::DISABLED  | :developer  | false
+      :private  | ProjectFeature::DISABLED  | :reporter   | false
+      :private  | ProjectFeature::DISABLED  | :guest      | false
+      :private  | ProjectFeature::DISABLED  | :anonymous  | false
+    end
+
+    with_them do
+      let(:current_user) { send(role) }
+      let(:project) { send("#{project_visibility}_project") }
+
+      it 'allows/disallows the abilities based on the container_registry feature access level' do
+        project.project_feature.update!(container_registry_access_level: access_level)
+
+        if allowed
+          expect_allowed(*permissions_abilities(role))
+        else
+          expect_disallowed(*permissions_abilities(role))
+        end
+      end
+
+      def permissions_abilities(role)
+        case role
+        when :maintainer
+          maintainer_operations_permissions
+        when :developer
+          developer_operations_permissions
+        when :reporter, :guest, :anonymous
+          guest_operations_permissions
+        else
+          raise "Unknown role #{role}"
+        end
+      end
+    end
+
+    context 'with read_container_registry_access_level disabled' do
+      before do
+        stub_feature_flags(read_container_registry_access_level: false)
+      end
+
+      where(:project_visibility, :container_registry_enabled, :role, :allowed) do
+        :public   | true   | :maintainer | true
+        :public   | true   | :developer  | true
+        :public   | true   | :reporter   | true
+        :public   | true   | :guest      | true
+        :public   | true   | :anonymous  | true
+        :public   | false  | :maintainer | false
+        :public   | false  | :developer  | false
+        :public   | false  | :reporter   | false
+        :public   | false  | :guest      | false
+        :public   | false  | :anonymous  | false
+        :internal | true   | :maintainer | true
+        :internal | true   | :developer  | true
+        :internal | true   | :reporter   | true
+        :internal | true   | :guest      | true
+        :internal | true   | :anonymous  | false
+        :internal | false  | :maintainer | false
+        :internal | false  | :developer  | false
+        :internal | false  | :reporter   | false
+        :internal | false  | :guest      | false
+        :internal | false  | :anonymous  | false
+        :private  | true   | :maintainer | true
+        :private  | true   | :developer  | true
+        :private  | true   | :reporter   | true
+        :private  | true   | :guest      | false
+        :private  | true   | :anonymous  | false
+        :private  | false  | :maintainer | false
+        :private  | false  | :developer  | false
+        :private  | false  | :reporter   | false
+        :private  | false  | :guest      | false
+        :private  | false  | :anonymous  | false
+      end
+
+      with_them do
+        let(:current_user) { send(role) }
+        let(:project) { send("#{project_visibility}_project") }
+
+        it 'allows/disallows the abilities based on container_registry_enabled' do
+          project.update_column(:container_registry_enabled, container_registry_enabled)
+
+          if allowed
+            expect_allowed(*permissions_abilities(role))
+          else
+            expect_disallowed(*permissions_abilities(role))
+          end
+        end
+
+        def permissions_abilities(role)
+          case role
+          when :maintainer
+            maintainer_operations_permissions
+          when :developer
+            developer_operations_permissions
+          when :reporter, :guest, :anonymous
+            guest_operations_permissions
+          else
+            raise "Unknown role #{role}"
+          end
+        end
+      end
+    end
+  end
 end

spec/requests/api/projects_spec.rb

@@ -184,6 +184,32 @@ RSpec.describe API::Projects do
         end
       end
 
+      it 'includes correct value of container_registry_enabled', :aggregate_failures do
+        project.update_column(:container_registry_enabled, true)
+        project.project_feature.update!(container_registry_access_level: ProjectFeature::DISABLED)
+
+        get api('/projects', user)
+        project_response = json_response.find { |p| p['id'] == project.id }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_an Array
+        expect(project_response['container_registry_enabled']).to eq(false)
+      end
+
+      it 'reads projects.container_registry_enabled when read_container_registry_access_level is disabled' do
+        stub_feature_flags(read_container_registry_access_level: false)
+
+        project.project_feature.update!(container_registry_access_level: ProjectFeature::DISABLED)
+        project.update_column(:container_registry_enabled, true)
+
+        get api('/projects', user)
+        project_response = json_response.find { |p| p['id'] == project.id }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_an Array
+        expect(project_response['container_registry_enabled']).to eq(true)
+      end
+
       it 'includes project topics' do
         get api('/projects', user)