Merge branch 'sh-prevent-shared-runners-toggling-cc' into 'master'

Prevent enabling of shared runners if credit card needed

See merge request gitlab-org/gitlab!64396

app/controllers/projects/runners_controller.rb

@@ -51,14 +51,14 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def toggle_shared_runners
-    if !project.shared_runners_enabled && project.group && project.group.shared_runners_setting == 'disabled_and_unoverridable'
-      render json: { error: _('Cannot enable shared runners because parent group does not allow it') }, status: :unauthorized
-      return
-    end
-
-    project.toggle!(:shared_runners_enabled)
+    update_params = { shared_runners_enabled: !project.shared_runners_enabled }
+    result = Projects::UpdateService.new(project, current_user, update_params).execute
 
-    render json: {}, status: :ok
+    if result[:status] == :success
+      render json: {}, status: :ok
+    else
+      render json: { error: result[:message] }, status: :unauthorized
+    end
   end
 
   def toggle_group_runners

ee/app/models/ee/user.rb

@@ -406,6 +406,12 @@ module EE
       has_valid_credit_card? || !requires_credit_card_to_run_pipelines?(project)
     end
 
+    # This is like has_required_credit_card_to_run_pipelines? except that
+    # former checks whether shared runners are enabled, and this method does not.
+    def has_required_credit_card_to_enable_shared_runners?(project)
+      has_valid_credit_card? || !requires_credit_card_to_enable_shared_runners?(project)
+    end
+
     protected
 
     override :password_required?
@@ -427,9 +433,18 @@ module EE
     end
 
     def requires_credit_card_to_run_pipelines?(project)
+      return false unless project.shared_runners_enabled
+
+      requires_credit_card?(project)
+    end
+
+    def requires_credit_card_to_enable_shared_runners?(project)
+      requires_credit_card?(project)
+    end
+
+    def requires_credit_card?(project)
       return false unless ::Gitlab.com?
       return false unless created_after_credit_card_release_day?(project)
-      return false unless project.shared_runners_enabled
 
       root_namespace = project.root_namespace
       if root_namespace.free_plan?

ee/app/services/ee/projects/update_service.rb

@@ -21,8 +21,10 @@ module EE
         limit = params.delete(:repository_size_limit)
         wiki_was_enabled = project.wiki_enabled?
 
+        shared_runners_setting
         mirror_user_setting
         compliance_framework_setting
+
         return update_failed! if project.errors.any?
 
         result = super do
@@ -55,6 +57,17 @@ module EE
         ).for_project.security_event
       end
 
+      # A user who enables shared runners must meet the credit card requirement if
+      # there is one.
+      def shared_runners_setting
+        return unless params[:shared_runners_enabled]
+        return if project.shared_runners_enabled
+
+        unless current_user.has_required_credit_card_to_enable_shared_runners?(project)
+          project.errors.add(:shared_runners_enabled, _('cannot be enabled until a valid credit credit is on file'))
+        end
+      end
+
       # A user who changes any aspect of pull mirroring settings must be made
       # into the mirror user, to prevent them from acquiring capabilities
       # owned by the previous user, such as writing to a protected branch.

ee/spec/controllers/projects/runners_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::RunnersController do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:namespace) { create(:namespace) }
+
+  let(:project) { create(:project, namespace: namespace, creator: user) }
+  let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+  let(:params) do
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      id: runner
+    }
+  end
+
+  before do
+    create(:gitlab_subscription, namespace: namespace, hosted_plan: create(:free_plan))
+    allow(::Gitlab).to receive(:com?).and_return(true)
+
+    sign_in(user)
+    project.add_maintainer(user)
+  end
+
+  describe '#toggle_shared_runners' do
+    let_it_be(:group) { create(:group) }
+
+    let(:project) { create(:project, group: group) }
+
+    context 'when shared runners are off' do
+      before do
+        project.update!(shared_runners_enabled: false)
+      end
+
+      context 'when user has valid credit card' do
+        before do
+          create(:credit_card_validation, user: user)
+        end
+
+        it 'permits enabling and disabling shared runners', :aggregate_failures do
+          post :toggle_shared_runners, params: params
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(project.reload.shared_runners_enabled).to eq(true)
+
+          post :toggle_shared_runners, params: params
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(project.reload.shared_runners_enabled).to eq(false)
+        end
+      end
+
+      context 'when user does not have valid credit card' do
+        it 'does not permit enabling shared runners', :aggregate_failures do
+          post :toggle_shared_runners, params: params
+
+          project.reload
+
+          expect(response).to have_gitlab_http_status(:unauthorized)
+          expect(json_response['error']).to eq('Shared runners enabled cannot be enabled until a valid credit credit is on file')
+          expect(project.shared_runners_enabled).to eq(false)
+        end
+      end
+    end
+  end
+end

ee/spec/models/ee/user_spec.rb

@@ -1768,6 +1768,53 @@ RSpec.describe User do
     end
   end
 
+  describe '#has_required_credit_card_to_enable_shared_runners?' do
+    let_it_be(:project) { create(:project) }
+
+    subject { user.has_required_credit_card_to_enable_shared_runners?(project) }
+
+    using RSpec::Parameterized::TableSyntax
+
+    where(:saas, :cc_present, :plan, :feature_flags, :days_from_release, :result, :description) do
+      # self-hosted
+      nil   | false | :paid  | %i[free trial]           | 0  | true  | 'self-hosted paid plan'
+      nil   | false | :trial | %i[free trial]           | 0  | true  | 'self-hosted missing CC on trial plan'
+
+      # saas
+      :saas | false | :paid  | %i[free trial old_users] | 0  | true  | 'missing CC on paid plan'
+
+      :saas | false | :free  | %i[free trial]           | 0  | false | 'missing CC on free plan'
+      :saas | false | :free  | %i[free trial]           | -1 | true  | 'missing CC on free plan but old user'
+      :saas | false | :free  | %i[free trial old_users] | -1 | false | 'missing CC on free plan but old user and FF enabled'
+      :saas | true  | :free  | %i[free trial]           | 0  | true  | 'present CC on free plan'
+      :saas | false | :free  | %i[]                     | 0  | true  | 'missing CC on free plan - FF off'
+
+      :saas | false | :trial | %i[free trial]           | 0  | false | 'missing CC on trial plan'
+      :saas | false | :trial | %i[free trial]           | -1 | true  | 'missing CC on trial plan but old user'
+      :saas | false | :trial | %i[free trial old_users] | -1 | false | 'missing CC on trial plan but old user and FF enabled'
+      :saas | false | :trial | %i[]                     | 0  | true  | 'missing CC on trial plan - FF off'
+      :saas | true  | :trial | %i[free trial]           | 0  | true  | 'present CC on trial plan'
+    end
+
+    with_them do
+      before do
+        allow(::Gitlab).to receive(:com?).and_return(saas == :saas)
+        user.created_at = ::Users::CreditCardValidation::RELEASE_DAY + days_from_release.days
+        allow(user).to receive(:credit_card_validated_at).and_return(Time.current) if cc_present
+        allow(project.namespace).to receive(:free_plan?).and_return(plan == :free)
+        allow(project.namespace).to receive(:trial?).and_return(plan == :trial)
+        stub_feature_flags(
+          ci_require_credit_card_on_free_plan: feature_flags.include?(:free),
+          ci_require_credit_card_on_trial_plan: feature_flags.include?(:trial),
+          ci_require_credit_card_for_old_users: feature_flags.include?(:old_users))
+      end
+
+      it description do
+        expect(subject).to eq(result)
+      end
+    end
+  end
+
   describe "#owns_group_without_trial" do
     let_it_be(:user) { create(:user) }
     let_it_be(:group) { create(:group) }

ee/spec/services/projects/update_service_spec.rb

@@ -9,6 +9,74 @@ RSpec.describe Projects::UpdateService, '#execute' do
   let(:admin) { create(:user, :admin) }
   let(:project) { create(:project, :repository, creator: user, namespace: user.namespace) }
 
+  context 'shared runners' do
+    let(:opts) { { shared_runners_enabled: enabled } }
+    let(:enabled) { true }
+
+    before do
+      create(:gitlab_subscription, namespace: user.namespace, hosted_plan: create(:free_plan))
+      allow(::Gitlab).to receive(:com?).and_return(true)
+    end
+
+    context 'when shared runners are on' do
+      let(:enabled) { false }
+
+      before do
+        project.update!(shared_runners_enabled: true)
+      end
+
+      it 'disables shared runners', :aggregate_failures do
+        result = update_project(project, user, opts)
+
+        expect(result).to eq(status: :success)
+        expect(project).to have_attributes(opts)
+      end
+
+      context 'when user has valid credit card' do
+        before do
+          create(:credit_card_validation, user: user)
+        end
+
+        it 'disables shared runners', :aggregate_failures do
+          result = update_project(project, user, opts)
+
+          expect(result).to eq(status: :success)
+          expect(project).to have_attributes(opts)
+        end
+      end
+    end
+
+    context 'when shared runners are off' do
+      before do
+        project.update!(shared_runners_enabled: false)
+      end
+
+      context 'when user has valid credit card' do
+        before do
+          create(:credit_card_validation, user: user)
+        end
+
+        it 'enables shared runners', :aggregate_failures do
+          result = update_project(project, user, opts)
+
+          expect(result).to eq(status: :success)
+          expect(project).to have_attributes(opts)
+        end
+      end
+
+      context 'when user does not have valid credit card' do
+        it 'does not enable shared runners', :aggregate_failures do
+          result = update_project(project, user, opts)
+
+          project.reload
+
+          expect(result).to eq(status: :error, message: 'Shared runners enabled cannot be enabled until a valid credit credit is on file')
+          expect(project.shared_runners_enabled).to eq(false)
+        end
+      end
+    end
+  end
+
   context 'repository mirror' do
     let(:opts) { { mirror: true, import_url: 'http://foo.com' } }
 

locale/gitlab.pot

@@ -5962,9 +5962,6 @@ msgstr ""
 msgid "Cannot delete %{profile_name} referenced in security policy"
 msgstr ""
 
-msgid "Cannot enable shared runners because parent group does not allow it"
-msgstr ""
-
 msgid "Cannot have multiple Jira imports running at the same time"
 msgstr ""
 
@@ -38177,6 +38174,9 @@ msgstr ""
 msgid "cannot be enabled unless all domains have TLS certificates"
 msgstr ""
 
+msgid "cannot be enabled until a valid credit credit is on file"
+msgstr ""
+
 msgid "cannot be modified"
 msgstr ""
 

spec/controllers/projects/runners_controller_spec.rb

@@ -111,7 +111,7 @@ RSpec.describe Projects::RunnersController do
 
       expect(response).to have_gitlab_http_status(:unauthorized)
       expect(project.shared_runners_enabled).to eq(false)
-      expect(json_response['error']).to eq('Cannot enable shared runners because parent group does not allow it')
+      expect(json_response['error']).to eq('Shared runners enabled cannot be enabled because parent group does not allow it')
     end
   end
 end