Merge branch 'bvl-validate-label-for-project-before-save' into 'master'

Validate classification label on create & update

Closes #5336

See merge request gitlab-org/gitlab-ee!5976

app/services/projects/create_service.rb

@@ -48,6 +48,9 @@ module Projects
 
       yield(@project) if block_given?
 
+      # If the block added errors, don't try to save the project
+      return @project if @project.errors.any?
+
       @project.creator = current_user
 
       if forked_from_project_id

app/services/projects/update_service.rb

@@ -21,6 +21,9 @@ module Projects
 
       yield if block_given?
 
+      # If the block added errors, don't try to save the project
+      return validation_failed! if project.errors.any?
+
       if project.update_attributes(params.except(:default_branch))
         if project.previous_changes.include?('path')
           project.rename_repo
@@ -32,10 +35,7 @@ module Projects
 
         success
       else
-        model_errors = project.errors.full_messages.to_sentence
-        error_message = model_errors.presence || 'Project could not be updated!'
-
-        error(error_message)
+        validation_failed!
       end
     end
 
@@ -47,6 +47,13 @@ module Projects
 
     private
 
+    def validation_failed!
+      model_errors = project.errors.full_messages.to_sentence
+      error_message = model_errors.presence || 'Project could not be updated!'
+
+      error(error_message)
+    end
+
     def renaming_project_with_container_registry_tags?
       new_path = params[:path]
 

ee/app/services/concerns/validates_classification_label.rb

+module ValidatesClassificationLabel
+  def validate_classification_label(record, attribute_name)
+    return unless EE::Gitlab::ExternalAuthorization.enabled?
+    return unless classification_label_change?(record, attribute_name)
+
+    new_label = params[attribute_name].presence
+    new_label ||= ::Gitlab::CurrentSettings.current_application_settings
+                    .external_authorization_service_default_label
+
+    unless EE::Gitlab::ExternalAuthorization.access_allowed?(current_user, new_label)
+      reason = rejection_reason_for_label(new_label)
+      message = s_('ClassificationLabelUnavailable|is unavailable: %{reason}') % { reason: reason }
+      record.errors.add(attribute_name, message)
+    end
+  end
+
+  def rejection_reason_for_label(label)
+    reason_from_service = EE::Gitlab::ExternalAuthorization.rejection_reason(current_user, label).presence
+    reason_from_service || _("Access to '%{classification_label}' not allowed") % { classification_label: label }
+  end
+
+  def classification_label_change?(record, attribute_name)
+    params.key?(attribute_name) || record.new_record?
+  end
+end

ee/app/services/ee/application_settings/update_service.rb

 module EE
   module ApplicationSettings
     module UpdateService
+      include ValidatesClassificationLabel
       extend ::Gitlab::Utils::Override
       extend ActiveSupport::Concern
 
@@ -10,6 +11,12 @@ module EE
         limit = params.delete(:repository_size_limit)
         application_setting.repository_size_limit = ::Gitlab::Utils.try_megabytes_to_bytes(limit) if limit
 
+        validate_classification_label(application_setting, :external_authorization_service_default_label)
+
+        if application_setting.errors.any?
+          return false
+        end
+
         super
       end
     end

ee/app/services/ee/projects/create_service.rb

@@ -2,6 +2,7 @@ module EE
   module Projects
     module CreateService
       extend ::Gitlab::Utils::Override
+      include ValidatesClassificationLabel
 
       override :execute
       def execute
@@ -20,6 +21,8 @@ module EE
             project.mirror_trigger_builds = mirror_trigger_builds unless mirror_trigger_builds.nil?
             project.mirror_user_id = mirror_user_id
           end
+
+          validate_classification_label(project, :external_authorization_classification_label)
         end
 
         if project&.persisted?

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

@@ -2,7 +2,7 @@ module EE
   module Projects
     module UpdateService
       extend ::Gitlab::Utils::Override
-
+      include ValidatesClassificationLabel
       include CleanupApprovers
 
       override :execute
@@ -24,6 +24,8 @@ module EE
           if changing_storage_size?
             project.change_repository_storage(params.delete(:repository_storage))
           end
+
+          validate_classification_label(project, :external_authorization_classification_label)
         end
 
         if result[:status] == :success

ee/changelogs/unreleased/bvl-validate-label-for-project-before-save.yml

+---
+title: Validate classification label on create & update
+merge_request: 5976
+author:
+type: fixed

ee/spec/controllers/projects_controller_spec.rb

@@ -188,6 +188,9 @@ describe ProjectsController do
       it 'updates the project classification label' do
         external_service_allow_access(user, project)
 
+        expect(EE::Gitlab::ExternalAuthorization)
+          .to receive(:access_allowed?).with(user, 'new_label') { true }
+
         expect { update_classification_label }
           .to change(project, :external_authorization_classification_label).to('new_label')
       end

ee/spec/services/application_settings/update_service_spec.rb

 require 'spec_helper'
 
 describe ApplicationSettings::UpdateService do
+  include ExternalAuthorizationServiceHelpers
+
   let(:user)    { create(:user) }
   let(:setting) { ApplicationSetting.create_from_defaults }
   let(:service) { described_class.new(setting, user, opts) }
@@ -62,4 +64,37 @@ describe ApplicationSettings::UpdateService do
       end
     end
   end
+
+  context 'when external authorization is enabled' do
+    before do
+      enable_external_authorization_service_check
+    end
+
+    it 'does not save the settings with an error if the service denies access' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label') { false }
+
+      described_class.new(setting, user, { external_authorization_service_default_label: 'new-label' }).execute
+
+      expect(setting.errors[:external_authorization_service_default_label]).to be_present
+    end
+
+    it 'saves the setting when the user has access to the label' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label') { true }
+
+      described_class.new(setting, user, { external_authorization_service_default_label: 'new-label' }).execute
+
+      # Read the attribute directly to avoid the stub from
+      # `enable_external_authorization_service_check`
+      expect(setting[:external_authorization_service_default_label]).to eq('new-label')
+    end
+
+    it 'does not validate the label if it was not passed' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .not_to receive(:access_allowed?)
+
+      described_class.new(setting, user, { home_page_url: 'http://foo.bar' }).execute
+    end
+  end
 end

ee/spec/services/projects/create_service_spec.rb

 require 'spec_helper'
 
 describe Projects::CreateService, '#execute' do
+  include ExternalAuthorizationServiceHelpers
+
   let(:user) { create :user }
   let(:opts) do
     {
@@ -261,6 +263,42 @@ describe Projects::CreateService, '#execute' do
     end
   end
 
+  context 'with external authorization enabled' do
+    before do
+      enable_external_authorization_service_check
+    end
+
+    it 'does not save the project with an error if the service denies access' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label', any_args) { false }
+
+      project = create_project(user, opts.merge({ external_authorization_classification_label: 'new-label' }))
+
+      expect(project.errors[:external_authorization_classification_label]).to be_present
+      expect(project).not_to be_persisted
+    end
+
+    it 'saves the project when the user has access to the label' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label', any_args) { true }
+
+      project = create_project(user, opts.merge({ external_authorization_classification_label: 'new-label' }))
+
+      expect(project).to be_persisted
+      expect(project.external_authorization_classification_label).to eq('new-label')
+    end
+
+    it 'does not save the project when the user has no access to the default label and no label is provided' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'default_label', any_args) { false }
+
+      project = create_project(user, opts)
+
+      expect(project.errors[:external_authorization_classification_label]).to be_present
+      expect(project).not_to be_persisted
+    end
+  end
+
   def create_project(user, opts)
     described_class.new(user, opts).execute
   end

ee/spec/services/projects/update_service_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Projects::UpdateService, '#execute' do
   include EE::GeoHelpers
+  include ExternalAuthorizationServiceHelpers
 
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository, creator: user, namespace: user.namespace) }
@@ -196,6 +197,46 @@ describe Projects::UpdateService, '#execute' do
     end
   end
 
+  context 'with external authorization enabled' do
+    before do
+      enable_external_authorization_service_check
+    end
+
+    it 'does not save the project with an error if the service denies access' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label') { false }
+
+      result = update_project(project, user, { external_authorization_classification_label: 'new-label' })
+
+      expect(result[:message]).to be_present
+      expect(result[:status]).to eq(:error)
+    end
+
+    it 'saves the new label if the service allows access' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'new-label') { true }
+
+      result = update_project(project, user, { external_authorization_classification_label: 'new-label' })
+
+      expect(result[:status]).to eq(:success)
+      expect(project.reload.external_authorization_classification_label).to eq('new-label')
+    end
+
+    it 'checks the default label when the classification label was cleared' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .to receive(:access_allowed?).with(user, 'default_label') { true }
+
+      update_project(project, user, { external_authorization_classification_label: '' })
+    end
+
+    it 'does not check the label when it does not change' do
+      expect(EE::Gitlab::ExternalAuthorization)
+        .not_to receive(:access_allowed?)
+
+      update_project(project, user, { name: 'New name' })
+    end
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end

spec/requests/api/settings_spec.rb

@@ -37,7 +37,9 @@ describe API::Settings, 'Settings' do
 
     context "custom repository storage type set in the config" do
       before do
-        storages = { 'custom' => 'tmp/tests/custom_repositories' }
+        # Add a possible storage to the config
+        storages = Gitlab.config.repositories.storages
+                     .merge({ 'custom' => 'tmp/tests/custom_repositories' })
         allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
       end