Merge branch...

Merge branch 'philipcunningham-check-branch-permission-when-creating-dast-profile-323860' into 'master'

Add improved branch validation for Dast::Profiles

See merge request gitlab-org/gitlab!56053

ee/app/models/dast/profile.rb

@@ -19,6 +19,7 @@ module Dast
     validates :project_id, :dast_site_profile_id, :dast_scanner_profile_id, presence: true
 
     validate :project_ids_match
+    validate :branch_name_exists_in_repository
     validate :description_not_nil
 
     scope :by_project_id, -> (project_id) do
@@ -42,6 +43,19 @@ module Dast
       association_project_id_matches(dast_scanner_profile)
     end
 
+    def branch_name_exists_in_repository
+      return unless branch_name
+
+      unless project.repository.exists?
+        errors.add(:project, 'must have a repository')
+        return
+      end
+
+      unless project.repository.branch_exists?(branch_name)
+        errors.add(:branch_name, 'can\'t reference a branch that does not exist')
+      end
+    end
+
     def description_not_nil
       errors.add(:description, 'can\'t be nil') if description.nil?
     end

ee/changelogs/unreleased/philipcunningham-check-branch-permission-when-creating-dast-profile-32.yml

+---
+title: Add improved branch validation when creating and updating Dast::Profiles
+  in GraphQL
+merge_request: 56053
+author:
+type: changed

ee/spec/graphql/mutations/dast/profiles/create_spec.rb

@@ -8,8 +8,9 @@ RSpec.describe Mutations::Dast::Profiles::Create do
   let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
   let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
 
-  let(:name) { SecureRandom.hex }
+  let(:branch_name) { project.default_branch }
   let(:description) { SecureRandom.hex }
+  let(:name) { SecureRandom.hex }
   let(:run_after_create) { false }
 
   let(:dast_profile) { Dast::Profile.find_by(project: project, name: name) }
@@ -28,7 +29,7 @@ RSpec.describe Mutations::Dast::Profiles::Create do
         full_path: project.full_path,
         name: name,
         description: description,
-        branch_name: project.default_branch,
+        branch_name: branch_name,
         dast_site_profile_id: dast_site_profile.to_global_id.to_s,
         dast_scanner_profile_id: dast_scanner_profile.to_global_id.to_s,
         run_after_create: run_after_create
@@ -44,18 +45,13 @@ RSpec.describe Mutations::Dast::Profiles::Create do
         context 'when run_after_create=true' do
           let(:run_after_create) { true }
 
+          it_behaves_like 'it checks branch permissions before creating a DAST on-demand scan pipeline'
           it_behaves_like 'it creates a DAST on-demand scan pipeline'
 
           it_behaves_like 'it delegates scan creation to another service' do
             let(:delegated_params) { hash_including(dast_profile: instance_of(Dast::Profile)) }
           end
         end
-
-        context "when branch_name='orphaned_branch'" do
-          it 'sets the branch_name' do
-            expect(subject[:dast_profile].branch_name).to eq(project.default_branch)
-          end
-        end
       end
     end
   end

ee/spec/graphql/mutations/dast/profiles/run_spec.rb

@@ -63,6 +63,10 @@ RSpec.describe Mutations::Dast::Profiles::Run do
           end
         end
 
+        it_behaves_like 'it checks branch permissions before creating a DAST on-demand scan pipeline' do
+          let(:branch_name) { dast_profile.branch_name }
+        end
+
         it_behaves_like 'it delegates scan creation to another service' do
           let(:delegated_params) { hash_including(dast_profile: dast_profile) }
         end

ee/spec/graphql/mutations/dast/profiles/update_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Mutations::Dast::Profiles::Update do
 
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project, branch_name: 'audio') }
+  let_it_be(:dast_profile, reload: true) { create(:dast_profile, project: project) }
 
   let(:dast_profile_gid) { dast_profile.to_global_id }
   let(:run_after_update) { false }
@@ -84,6 +84,10 @@ RSpec.describe Mutations::Dast::Profiles::Update do
 
           it_behaves_like 'it creates a DAST on-demand scan pipeline'
 
+          it_behaves_like 'it checks branch permissions before creating a DAST on-demand scan pipeline' do
+            let(:branch_name) { params[:branch_name] }
+          end
+
           it_behaves_like 'it delegates scan creation to another service' do
             let(:delegated_params) { hash_including(dast_profile: dast_profile) }
           end

ee/spec/models/dast/branch_spec.rb

@@ -3,18 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Dast::Branch do
-  let_it_be(:dast_profile) { create(:dast_profile) }
-
   subject { described_class.new(dast_profile) }
 
-  describe '#project' do
-    it 'delegates to profile.project' do
-      expect(subject.project).to eq(dast_profile.project)
-    end
-  end
+  context 'when repository does not exist' do
+    let_it_be(:dast_profile) { create(:dast_profile) }
 
-  context 'when profile.branch_name is nil' do
-    context 'when the associated project does not have a repository' do
     describe '#name' do
       it 'returns nil' do
         expect(subject.name).to be_nil
@@ -28,48 +21,29 @@ RSpec.describe Dast::Branch do
     end
   end
 
-    context 'when the associated project has a repository' do
-      let_it_be(:dast_profile) { create(:dast_profile, project: create(:project, :repository)) }
+  context 'when repository exists' do
+    let_it_be(:dast_profile) { create(:dast_profile, branch_name: 'orphaned-branch', project: create(:project, :repository)) }
 
     describe '#name' do
-        it 'returns project.default_branch' do
-          expect(subject.name).to eq(subject.project.default_branch)
+      it 'returns profile.branch_name' do
+        expect(subject.name).to eq(dast_profile.branch_name)
       end
     end
 
-      describe '#exists' do
+    context 'when branch exists' do
       it 'returns true' do
         expect(subject.exists).to eq(true)
       end
     end
-    end
-  end
 
-  context 'when profile.branch_name is not nil' do
-    let_it_be(:dast_profile) { create(:dast_profile, branch_name: 'orphaned-branch') }
-
-    describe '#name' do
-      it 'returns profile.branch_name' do
-        expect(subject.name).to eq(dast_profile.branch_name)
-      end
+    context 'when branch does not exist' do
+      before do
+        dast_profile.branch_name = SecureRandom.hex
       end
 
-    context 'when the associated project does not have a repository' do
-      describe '#exists' do
       it 'returns false' do
         expect(subject.exists).to eq(false)
       end
     end
   end
-
-    context 'when the associated branch has a repository and the branch exists' do
-      let_it_be(:dast_profile) { create(:dast_profile, project: create(:project, :repository), branch_name: 'orphaned-branch') }
-
-      describe '#exists' do
-        it 'returns true' do
-          expect(subject.exists).to eq(true)
-        end
-      end
-    end
-  end
 end

ee/spec/models/dast/profile_spec.rb

@@ -64,6 +64,28 @@ RSpec.describe Dast::Profile, type: :model do
         end
       end
     end
+
+    context 'when a branch_name is specified but the project does not have a respository' do
+      subject { build(:dast_profile, branch_name: SecureRandom.hex) }
+
+      it 'is not valid', :aggregate_failures do
+        expect(subject).not_to be_valid
+        expect(subject.errors.full_messages).to include('Project must have a repository')
+        expect(subject.errors.full_messages).not_to include('Branch name can\'t reference a branch that does not exist')
+      end
+    end
+
+    context 'when a branch_name is specified but the project does not have a respository' do
+      let_it_be(:project) { create(:project, :repository) }
+
+      subject { build(:dast_profile, project: project, branch_name: SecureRandom.hex) }
+
+      it 'is not valid', :aggregate_failures do
+        expect(subject).not_to be_valid
+        expect(subject.errors.full_messages).not_to include('Project must have a repository')
+        expect(subject.errors.full_messages).to include('Branch name can\'t reference a branch that does not exist')
+      end
+    end
   end
 
   describe 'scopes' do

ee/spec/services/dast_on_demand_scans/params_create_service_spec.rb

@@ -128,7 +128,7 @@ RSpec.describe DastOnDemandScans::ParamsCreateService do
     end
 
     context 'when the dast_profile is provided' do
-      let_it_be(:dast_profile) { create(:dast_profile, project: project, dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile, branch_name: 'hello-world') }
+      let_it_be(:dast_profile) { create(:dast_profile, project: project, dast_site_profile: dast_site_profile, dast_scanner_profile: dast_scanner_profile, branch_name: project.default_branch) }
 
       let(:params) { { dast_profile: dast_profile } }
 

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

@@ -9,3 +9,15 @@ RSpec.shared_examples 'it delegates scan creation to another service' do
     subject
   end
 end
+
+RSpec.shared_examples 'it checks branch permissions before creating a DAST on-demand scan pipeline' do
+  context 'when the user does not have access to the branch' do
+    before do
+      create(:protected_branch, project: project, name: branch_name)
+    end
+
+    it 'communicates failure' do
+      expect(subject[:errors]).to include(a_string_including("You do not have sufficient permission to run a pipeline on '#{branch_name}'"))
+    end
+  end
+end