Merge branch 'refactor-project-level-protected-envs' into 'master'

Refactor project-level protected environments for group-level [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58633

.rubocop_manual_todo.yml

@@ -52,7 +52,6 @@ Rails/SaveBang:
     - 'ee/spec/models/approval_project_rule_spec.rb'
     - 'ee/spec/models/burndown_spec.rb'
     - 'ee/spec/models/elasticsearch_indexed_namespace_spec.rb'
-    - 'ee/spec/models/environment_spec.rb'
     - 'ee/spec/models/epic_spec.rb'
     - 'ee/spec/models/gitlab_subscription_spec.rb'
     - 'ee/spec/models/issue_spec.rb'

app/models/ci/bridge.rb

@@ -163,6 +163,9 @@ module Ci
     def expanded_environment_name
     end
 
+    def instantized_environment
+    end
+
     def execute_hooks
       raise NotImplementedError
     end

app/models/ci/build.rb

@@ -88,6 +88,16 @@ module Ci
       end
     end
 
+    # Initializing an object instead of fetching `persisted_environment` for avoiding unnecessary queries.
+    # We're planning to introduce a direct relationship between build and environment
+    # in https://gitlab.com/gitlab-org/gitlab/-/issues/326445 to let us to preload
+    # in batch.
+    def instantized_environment
+      return unless has_environment?
+
+      ::Environment.new(project: self.project, name: self.expanded_environment_name)
+    end
+
     serialize :options # rubocop:disable Cop/ActiveRecordSerialize
     serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiveRecordSerialize
 

app/models/ci/processable.rb

@@ -120,6 +120,10 @@ module Ci
       raise NotImplementedError
     end
 
+    def instantized_environment
+      raise NotImplementedError
+    end
+
     override :all_met_to_become_pending?
     def all_met_to_become_pending?
       super && !with_resource_group?

app/policies/ci/build_policy.rb

@@ -21,7 +21,7 @@ module Ci
     end
 
     # overridden in EE
-    condition(:protected_environment_access) do
+    condition(:protected_environment) do
       false
     end
 
@@ -68,7 +68,10 @@ module Ci
     rule { project_read_build }.enable :read_build_trace
     rule { debug_mode & ~project_update_build }.prevent :read_build_trace
 
-    rule { ~protected_environment_access & (protected_ref | archived) }.policy do
+    # Authorizing the user to access to protected entities.
+    # There is a "jailbreak" mode to exceptionally bypass the authorization,
+    # however, you should NEVER allow it, rather suspect it's a wrong feature/product design.
+    rule { ~can?(:jailbreak) & (archived | protected_ref | protected_environment) }.policy do
       prevent :update_build
       prevent :update_commit_status
       prevent :erase_build

ee/app/helpers/protected_environments_helper.rb

-# frozen_string_literal: true
-
-module ProtectedEnvironmentsHelper
-  def protected_environments_enabled?(project)
-    project.protected_environments_feature_available?
-  end
-end

ee/app/models/ee/environment.rb

@@ -61,11 +61,40 @@ module EE
     end
 
     def protected?
-      project.protected_environment_by_name(name).present?
+      return false unless project.licensed_feature_available?(:protected_environments)
+
+      associated_protected_environments.present?
+    end
+
+    def protected_from?(user)
+      return true unless user.is_a?(User)
+      return false unless protected?
+
+      protected_environment_accesses(user).any? { |access, _| access == false }
+    end
+
+    def protected_by?(user)
+      return false unless user.is_a?(User) && protected?
+
+      protected_environment_accesses(user).all? { |access, _| access == true }
     end
 
-    def protected_deployable_by_user?(user)
-      project.protected_environment_accessible_to?(name, user)
+    private
+
+    def protected_environment_accesses(user)
+      key = "environment:#{self.project_id}:#{self.name}:for:#{user.id}"
+
+      ::Gitlab::SafeRequestStore.fetch(key) do
+        associated_protected_environments.group_by do |pe|
+          pe.accessible_to?(user)
+        end
+      end
+    end
+
+    def associated_protected_environments
+      strong_memoize(:associated_protected_environments) do
+        ::ProtectedEnvironment.for_environment(self)
+      end
     end
   end
 end

ee/app/models/ee/project.rb

@@ -661,22 +661,6 @@ module EE
     end
     request_cache(:any_path_locks?) { self.id }
 
-    def protected_environment_accessible_to?(environment_name, user)
-      protected_environment = protected_environment_by_name(environment_name)
-
-      !protected_environment || protected_environment.accessible_to?(user)
-    end
-
-    def protected_environment_by_name(environment_name)
-      return unless protected_environments_feature_available?
-
-      key = "protected_environment_by_name:#{id}:#{environment_name}"
-
-      ::Gitlab::SafeRequestStore.fetch(key) do
-        protected_environments.find { |pe| pe.name == environment_name }
-      end
-    end
-
     override :after_import
     def after_import
       super
@@ -702,10 +686,6 @@ module EE
         ::Gitlab::CurrentSettings.custom_project_templates_enabled?
     end
 
-    def protected_environments_feature_available?
-      feature_available?(:protected_environments)
-    end
-
     # Update the default branch querying the remote to determine its HEAD
     def update_root_ref(remote, remote_url, authorization)
       root_ref = repository.find_remote_root_ref(remote, remote_url, authorization)

ee/app/models/protected_environment.rb

@@ -24,6 +24,18 @@ class ProtectedEnvironment < ApplicationRecord
       .joins(:protected_environment).where(group: group)
   end
 
+  class << self
+    def for_environment(environment)
+      raise ArgumentError unless environment.is_a?(::Environment)
+
+      key = "protected_environment:for_environment:#{environment.project_id}:#{environment.name}"
+
+      ::Gitlab::SafeRequestStore.fetch(key) do
+        where(project: environment.project_id, name: environment.name)
+      end
+    end
+  end
+
   def accessible_to?(user)
     deploy_access_levels
       .any? { |deploy_access_level| deploy_access_level.check_access(user) }

ee/app/policies/ee/ci/build_policy.rb

@@ -5,44 +5,24 @@ module EE
       extend ActiveSupport::Concern
 
       prepended do
-        condition(:deployable_by_user) { deployable_by_user? }
-
-        condition(:protected_environment_access) do
-          project = @subject.project
-          environment = @subject.environment
-
-          if environment && project.protected_environments_feature_available?
-            protected_environment = project.protected_environment_by_name(environment)
-
-            !!protected_environment&.accessible_to?(user)
-          else
-            false
-          end
+        # overriding
+        condition(:protected_environment) do
+          @subject.instantized_environment&.protected_from?(user)
         end
 
-        rule { ~deployable_by_user & ~protected_environment_access}.policy do
-          prevent :update_build
+        condition(:reporter_has_access_to_protected_environment) do
+          @subject.instantized_environment&.protected_by?(user) &&
+            can?(:reporter_access, @subject.project)
         end
 
-        rule { protected_environment_access }.policy do
+        # If a reporter has an access to the protected environment,
+        # the user can jailbreak from the fundamental CI permissions and execute the deployment job.
+        # See https://gitlab.com/gitlab-org/gitlab/-/issues/225482
+        rule { reporter_has_access_to_protected_environment }.policy do
+          enable :jailbreak
           enable :update_commit_status
           enable :update_build
         end
-
-        private
-
-        alias_method :current_user, :user
-        alias_method :build, :subject
-
-        def deployable_by_user?
-          # We need to check if Protected Environments feature is available,
-          # and whether there is an environment defined for the current build,
-          # as evaluating `build.expanded_environment_name` is expensive.
-          return true unless build.has_environment?
-          return true unless build.project.protected_environments_feature_available?
-
-          build.project.protected_environment_accessible_to?(build.expanded_environment_name, user)
-        end
       end
     end
   end

ee/app/policies/ee/environment_policy.rb

@@ -4,9 +4,11 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      condition(:deployable_by_user) { deployable_by_user? }
+      condition(:protected_environment) do
+        @subject.protected_from?(user)
+      end
 
-      rule { ~deployable_by_user }.policy do
+      rule { protected_environment }.policy do
         prevent :stop_environment
         prevent :create_environment_terminal
         prevent :create_deployment
@@ -14,15 +16,6 @@ module EE
         prevent :update_environment
         prevent :destroy_environment
       end
-
-      private
-
-      alias_method :current_user, :user
-      alias_method :environment, :subject
-
-      def deployable_by_user?
-        environment.protected_deployable_by_user?(current_user)
-      end
     end
   end
 end

ee/app/services/ee/ci/process_build_service.rb

@@ -6,22 +6,12 @@ module EE
 
       override :enqueue
       def enqueue(build)
-        unless allowed_to_deploy?(build)
+        if build.instantized_environment&.protected_from?(build.user)
           return build.drop!(:protected_environment_failure)
         end
 
         super
       end
-
-      private
-
-      def allowed_to_deploy?(build)
-        # We need to check if Protected Environments feature is available,
-        # as evaluating `build.expanded_environment_name` is expensive.
-        return true unless project.protected_environments_feature_available?
-
-        project.protected_environment_accessible_to?(build.expanded_environment_name, build.user)
-      end
     end
   end
 end

ee/app/views/projects/settings/ci_cd/_protected_environments.html.haml

 - expanded = expanded_by_default?
 - can_admin_project = can?(current_user, :admin_project, @project)
 
-- if protected_environments_enabled?(@project)
+- if @project.licensed_feature_available?(:protected_environments)
   %section.protected-environments-settings.settings.no-animate#js-protected-environments-settings{ class: ('expanded' if expanded) }
     .settings-header
       %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only

ee/changelogs/unreleased/refactor-project-level-protected-envs.yml

+---
+title: Refactor project-level protected environments for performance optimization
+merge_request: 58633
+author:
+type: performance

ee/spec/models/environment_spec.rb

@@ -89,29 +89,88 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
     end
   end
 
-  describe '#protected_deployable_by_user?' do
+  describe '#protected_from?' do
     let(:user) { create(:user) }
     let(:protected_environment) { create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project) }
 
-    subject { environment.protected_deployable_by_user?(user) }
+    subject { environment.protected_from?(user) }
 
     before do
-      stub_licensed_features(protected_environments: true)
+      stub_licensed_features(protected_environments: feature_available)
     end
 
     context 'when Protected Environments feature is not available on the project' do
       let(:feature_available) { false }
 
-      it { is_expected.to be_truthy }
+      it { is_expected.to be_falsy }
     end
 
     context 'when Protected Environments feature is available on the project' do
       let(:feature_available) { true }
 
       context 'when the environment is not protected' do
+        it { is_expected.to be_falsy }
+      end
+
+      context 'when the user is nil' do
+        let(:user) { }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when environment is protected and user dont have access to it' do
+        before do
+          protected_environment
+        end
+
         it { is_expected.to be_truthy }
       end
 
+      context 'when environment is protected and user have access to it' do
+        before do
+          protected_environment.deploy_access_levels.create!(user: user)
+        end
+
+        it { is_expected.to be_falsy }
+
+        it 'caches result', :request_store do
+          environment.protected_from?(user)
+
+          expect { environment.protected_from?(user) }.not_to exceed_query_limit(0)
+        end
+      end
+    end
+  end
+
+  describe '#protected_by?' do
+    let(:user) { create(:user) }
+    let(:protected_environment) { create(:protected_environment, :maintainers_can_deploy, name: environment.name, project: project) }
+
+    subject { environment.protected_by?(user) }
+
+    before do
+      stub_licensed_features(protected_environments: feature_available)
+    end
+
+    context 'when Protected Environments feature is not available on the project' do
+      let(:feature_available) { false }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when Protected Environments feature is available on the project' do
+      let(:feature_available) { true }
+
+      context 'when the environment is not protected' do
+        it { is_expected.to be_falsy }
+      end
+
+      context 'when the user is nil' do
+        let(:user) { }
+
+        it { is_expected.to be_falsy }
+      end
+
       context 'when environment is protected and user dont have access to it' do
         before do
           protected_environment
@@ -122,7 +181,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
 
       context 'when environment is protected and user have access to it' do
         before do
-          protected_environment.deploy_access_levels.create(user: user)
+          protected_environment.deploy_access_levels.create!(user: user)
         end
 
         it { is_expected.to be_truthy }

ee/spec/models/project_spec.rb

@@ -1839,95 +1839,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#protected_environment_by_name' do
-    let_it_be(:project, reload: true) { create(:project) }
-
-    subject { project.protected_environment_by_name('production') }
-
-    before do
-      allow(project).to receive(:feature_available?)
-        .with(:protected_environments).and_return(feature_available)
-    end
-
-    context 'when Protected Environments feature is not available on the project' do
-      let(:feature_available) { false }
-
-      it { is_expected.to be_nil }
-    end
-
-    context 'when Protected Environments feature is available on the project' do
-      let(:feature_available) { true }
-
-      context 'when the project environment does not exists' do
-        it { is_expected.to be_nil }
-      end
-
-      context 'when the project environment exists' do
-        let_it_be(:environment) { create(:environment, name: 'production') }
-        let_it_be(:protected_environment) { create(:protected_environment, name: environment.name, project: project) }
-
-        it { is_expected.to eq(protected_environment) }
-
-        it 'caches environment name', :request_store do
-          control_count = ActiveRecord::QueryRecorder.new { project.protected_environment_by_name(protected_environment.name) }
-
-          expect do
-            2.times { project.protected_environment_by_name(protected_environment.name) }
-          end.not_to exceed_query_limit(control_count)
-
-          expect(project.protected_environment_by_name('non-existent-env')).to be_nil
-          expect(project.protected_environment_by_name(protected_environment.name)).to eq(protected_environment)
-        end
-      end
-    end
-  end
-
-  describe '#protected_environment_accessible_to?' do
-    let(:project) { create(:project) }
-    let(:user) { create(:user) }
-    let(:environment) { create(:environment, project: project) }
-    let(:protected_environment) { create(:protected_environment, project: project, name: environment.name) }
-
-    subject { project.protected_environment_accessible_to?(environment.name, user) }
-
-    before do
-      allow(project).to receive(:feature_available?)
-        .with(:protected_environments).and_return(feature_available)
-    end
-
-    context 'when Protected Environments feature is not available on the project' do
-      let(:feature_available) { false }
-
-      it { is_expected.to be_truthy }
-    end
-
-    context 'when Protected Environments feature is available on the project' do
-      let(:feature_available) { true }
-
-      context 'when project does not have protected environments' do
-        it { is_expected.to be_truthy }
-      end
-
-      context 'when project has protected environments' do
-        context 'when user has the right access' do
-          before do
-            protected_environment.deploy_access_levels.create(user_id: user.id)
-          end
-
-          it { is_expected.to be_truthy }
-        end
-
-        context 'when user does not have the right access' do
-          before do
-            protected_environment.deploy_access_levels.create
-          end
-
-          it { is_expected.to be_falsy }
-        end
-      end
-    end
-  end
-
   describe '#after_import' do
     let_it_be(:project) { create(:project) }
 

ee/spec/models/protected_environment_spec.rb

@@ -131,6 +131,43 @@ RSpec.describe ProtectedEnvironment do
     end
   end
 
+  describe '.for_environment' do
+    let_it_be(:project, reload: true) { create(:project) }
+    let_it_be(:environment) { build(:environment, name: 'production', project: project) }
+    let_it_be(:protected_environment) { create(:protected_environment, name: 'production', project: project) }
+
+    subject { described_class.for_environment(environment) }
+
+    it { is_expected.to eq([protected_environment]) }
+
+    it 'caches result', :request_store do
+      described_class.for_environment(environment).to_a
+
+      expect { described_class.for_environment(environment).to_a }
+        .not_to exceed_query_limit(0)
+    end
+
+    context 'when environment is a different name' do
+      let_it_be(:environment) { build(:environment, name: 'staging', project: project) }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'when environment exists in a different project' do
+      let_it_be(:environment) { build(:environment, name: 'production', project: create(:project)) }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'when environment does not exist' do
+      let(:environment) { }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+  end
+
   def create_deploy_access_level(**opts)
     protected_environment.deploy_access_levels.create(**opts)
   end

ee/spec/services/ci/process_build_service_spec.rb

@@ -6,15 +6,13 @@ RSpec.describe Ci::ProcessBuildService, '#execute' do
   let(:project) { create(:project, :repository) }
   let(:environment) { create(:environment, project: project, name: 'production') }
   let(:protected_environment) { create(:protected_environment, name: environment.name, project: project) }
-  let(:ci_build) { create(:ci_build, :created, environment: environment.name, user: user, when: :on_success) }
+  let(:ci_build) { create(:ci_build, :created, environment: environment.name, user: user, project: project, when: :on_success) }
   let(:current_status) { 'success' }
 
   subject { described_class.new(project, user).execute(ci_build, current_status) }
 
   before do
-    allow(License).to receive(:feature_available?).and_call_original
-    allow(License).to receive(:feature_available?)
-      .with(:protected_environments).and_return(feature_available)
+    stub_licensed_features(protected_environments: feature_available)
 
     protected_environment
   end

ee/spec/support/shared_examples/services/build_execute_shared_examples.rb

@@ -6,15 +6,12 @@ RSpec.shared_examples 'restricts access to protected environments' do |developer
     let(:project) { create(:project, :repository) }
     let(:pipeline) { create(:ci_pipeline, project: project) }
     let(:environment) { create(:environment, project: project, name: 'production') }
-    let(:build) { create(:ci_build, :created, pipeline: pipeline, environment: environment.name) }
+    let(:build) { create(:ci_build, :created, pipeline: pipeline, environment: environment.name, project: project) }
     let(:protected_environment) { create(:protected_environment, name: environment.name, project: project) }
     let(:service) { described_class.new(project, user) }
 
     before do
-      allow(project).to receive(:feature_available?).and_call_original
-      allow(project).to receive(:feature_available?)
-        .with(:protected_environments).and_return(true)
-      allow(build).to receive(:project) { project }
+      stub_licensed_features(protected_environments: true)
 
       project.add_developer(user)
       protected_environment