Merge branch 'security-451014-dast-profile-permissions-17-2' into '17-2-stable-ee'

[17.2] Update edit permissions for DAST profiles

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4356



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Alejandro Rodríguez <alejandro@gitlab.com>
Co-authored-by: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com>

ee/app/controllers/projects/on_demand_scans_controller.rb

@@ -25,6 +25,9 @@ def new
     def edit
       global_id = Gitlab::GlobalId.as_global_id(params[:id], model_name: 'Dast::Profile')
+      dast_profile = Dast::Profile.find(params[:id])
+      return render_404 unless dast_profile.can_edit_scan?(current_user)
+
       query = %(
           {
             project(fullPath: "#{project.full_path}") {

ee/app/controllers/projects/security/dast_scanner_profiles_controller.rb

@@ -19,6 +19,8 @@ def edit
         @scanner_profile = @project
           .dast_scanner_profiles
           .find(params[:id])
+
+        render_404 unless @scanner_profile&.can_edit_profile?(current_user)
       end
     end
   end

ee/app/controllers/projects/security/dast_site_profiles_controller.rb

@@ -19,6 +19,9 @@ def new
       def edit
         global_id = Gitlab::GlobalId.as_global_id(params[:id], model_name: 'DastSiteProfile')
+        site_profile = DastSiteProfile.find(params[:id])
+        return render_404 unless site_profile.can_edit_profile?(current_user)
+
         query = %(
           {
             project(fullPath: "#{project.full_path}") {

ee/app/finders/dast/profiles_finder.rb

@@ -13,6 +13,9 @@ def execute
       relation = by_id(relation)
       relation = by_project(relation)
       relation = has_schedule?(relation)
+      relation = by_site_profile_id(relation)
+      relation = by_scanner_profile_id(relation)
+      relation = with_project(relation)
       sort(relation)
     end
@@ -43,6 +46,24 @@ def has_schedule?(relation)
       relation.with_schedule(params[:has_dast_profile_schedule])
     end
+    def by_site_profile_id(relation)
+      return relation if params[:site_profile_id].nil?
+
+      relation.by_site_profile_id(params[:site_profile_id])
+    end
+
+    def by_scanner_profile_id(relation)
+      return relation if params[:scanner_profile_id].nil?
+
+      relation.by_scanner_profile_id(params[:scanner_profile_id])
+    end
+
+    def with_project(relation)
+      return relation unless params[:with_project]
+
+      relation.with_project
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def sort(relation)
       relation.order(DEFAULT_SORT)

ee/app/graphql/ee/types/project_type.rb

@@ -74,6 +74,7 @@ module ProjectType
           ::Types::Dast::ProfileType,
           null: true,
           resolver: ::Resolvers::AppSec::Dast::ProfileResolver.single,
+          calls_gitaly: true,
           description: 'DAST Profile associated with the project.'
         field :dast_profiles,
@@ -82,6 +83,7 @@ module ProjectType
           extras: [:lookahead],
           late_extensions: [::Gitlab::Graphql::Project::DastProfileConnectionExtension],
           resolver: ::Resolvers::AppSec::Dast::ProfileResolver,
+          calls_gitaly: true,
           description: 'DAST Profiles associated with the project.'
         field :dast_site_profile,

ee/app/models/dast/profile.rb

@@ -39,6 +39,18 @@ class Profile < ApplicationRecord
       eager_load(dast_profile_schedule: [:owner])
     end
+    scope :by_site_profile_id, ->(site_profile_id) do
+      where(dast_site_profile_id: site_profile_id)
+    end
+
+    scope :by_scanner_profile_id, ->(scanner_profile_id) do
+      where(dast_scanner_profile_id: scanner_profile_id)
+    end
+
+    scope :with_project, -> do
+      includes(project: [:project_feature, :route, :group])
+    end
+
     delegate :secret_ci_variables, to: :dast_site_profile
     sanitizes! :name, :description
@@ -49,6 +61,12 @@ def branch
       Dast::Branch.new(self)
     end
+    def can_edit_scan?(user)
+      access = Gitlab::UserAccess.new(user, container: project)
+
+      access.can_push_to_branch?(branch.name)
+    end
+
     def tag_list
       tags.map(&:name)
     end

ee/app/models/dast_scanner_profile.rb

@@ -53,4 +53,16 @@ def referenced_in_security_policies
       .inject(&:merge)
       .to_a
   end
+
+  def can_edit_profile?(user)
+    can_edit = true
+    ::Dast::ProfilesFinder.new(scanner_profile_id: id, with_project: true).execute.find_each do |profile|
+      unless profile.can_edit_scan?(user)
+        can_edit = false
+        break
+      end
+    end
+
+    can_edit
+  end
 end

ee/app/models/dast_site_profile.rb

@@ -116,6 +116,18 @@ def validation_started_at
     dast_site_validation.validation_started_at
   end
+  def can_edit_profile?(user)
+    can_edit = true
+    ::Dast::ProfilesFinder.new(site_profile_id: id, with_project: true).execute.find_each do |profile|
+      unless profile.can_edit_scan?(user)
+        can_edit = false
+        break
+      end
+    end
+
+    can_edit
+  end
+
   private
   def cleanup_dast_site

ee/app/services/app_sec/dast/profiles/update_service.rb

@@ -9,6 +9,7 @@ class UpdateService < BaseService
         def execute
           return unauthorized unless allowed?
           return error(_('Profile parameter missing')) unless dast_profile
+          return error(_('You are not authorized to update this profile')) unless can_push_to_branch?
           return error(_('Invalid tags')) unless valid_tags?
@@ -58,6 +59,12 @@ def allowed?
             can?(current_user, :create_on_demand_dast_scan, project)
         end
+        def can_push_to_branch?
+          access = Gitlab::UserAccess.new(current_user, container: project)
+
+          access.can_push_to_branch?(dast_profile['branch_name'])
+        end
+
         def update_or_create_schedule!
           if schedule
             schedule.update!(schedule_input_params)

ee/app/services/app_sec/dast/scanner_profiles/update_service.rb

@@ -12,6 +12,7 @@ def execute
           dast_scanner_profile = find_dast_scanner_profile(params[:id])
           return ServiceResponse.error(message: _('Scanner profile not found for given parameters')) unless dast_scanner_profile
           return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in security policy') % { profile_name: dast_scanner_profile.name }) if referenced_in_security_policy?(dast_scanner_profile)
+          return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in scan') % { profile_name: dast_scanner_profile.name }) unless dast_scanner_profile.can_edit_profile?(current_user)
           old_params = dast_scanner_profile.attributes.symbolize_keys
           update_params = update_params(params[:profile_name])

ee/app/services/app_sec/dast/site_profiles/update_service.rb

@@ -14,12 +14,14 @@ def initialize(errors)
         attr_reader :dast_site_profile
+        # rubocop:disable Metrics/AbcSize
         def execute(id:, **params)
           return ServiceResponse.error(message: _('Insufficient permissions')) unless allowed?
           find_dast_site_profile!(id)
           return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy?
+          return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in scan') % { profile_name: dast_site_profile.name }) unless dast_site_profile.can_edit_profile?(current_user)
           ApplicationRecord.transaction do
             auditor = AppSec::Dast::SiteProfiles::Audit::UpdateService.new(project, current_user, {
@@ -52,6 +54,7 @@ def execute(id:, **params)
         rescue ActiveRecord::RecordInvalid => e
           ServiceResponse.error(message: e.record.errors.full_messages)
         end
+        # rubocop:enable Metrics/AbcSize
         private

ee/spec/features/projects/security/user_edits_on_demand_scan_spec.rb

@@ -5,7 +5,7 @@
 RSpec.describe 'User edits On-demand Scan', feature_category: :dynamic_application_security_testing do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project, :repository, developers: user) }
-  let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+  let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: project.default_branch) }
   let(:on_demand_scans_path) { project_on_demand_scans_path(project) }
   let(:edit_on_demand_scan_path) { edit_project_on_demand_scan_path(project, dast_profile) }

ee/spec/finders/dast/profiles_finder_spec.rb

@@ -5,9 +5,9 @@
 RSpec.describe Dast::ProfilesFinder do
   let_it_be(:project1) { create(:project) }
   let_it_be(:project2) { create(:project) }
-  let_it_be(:dast_profile1) { create(:dast_profile, project: project1) }
-  let_it_be(:dast_profile2) { create(:dast_profile, project: project2) }
-  let_it_be(:dast_profile3) { create(:dast_profile, project: project1) }
+  let_it_be(:dast_profile1) { create(:dast_profile, project: project1, branch_name: project1.default_branch) }
+  let_it_be(:dast_profile2) { create(:dast_profile, project: project2, branch_name: project2.default_branch) }
+  let_it_be(:dast_profile3) { create(:dast_profile, project: project1, branch_name: project1.default_branch) }
   let_it_be(:dast_profile_schedule) { create(:dast_profile_schedule, project: project1, dast_profile: dast_profile3) }
   let(:params) { {} }

ee/spec/frontend/fixtures/dast_profiles.rb

@@ -309,7 +309,7 @@
     describe 'scheduled_dast_profiles' do
       path = 'on_demand_scans/graphql/scheduled_dast_profiles.query.graphql'
-      let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+      let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: project.default_branch) }
       let_it_be(:dast_profile_schedule) do
         create(:dast_profile_schedule, project: project, dast_profile: dast_profile)
@@ -373,7 +373,7 @@
       path = 'on_demand_scans/graphql/dast_profiles.query.graphql'
       let_it_be(:dast_profiles) do
-        create_list(:dast_profile, 3, project: project)
+        create_list(:dast_profile, 3, project: project, branch_name: project.default_branch)
       end
       before do

ee/spec/frontend/fixtures/on_demand_dast_scans.rb

@@ -10,7 +10,7 @@
     let_it_be(:current_user) { create(:user) }
     let_it_be(:project) { create(:project, :repository, :public) }
-    let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+    let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: project.default_branch) }
     before do
       stub_licensed_features(security_on_demand_scans: true)

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

@@ -7,7 +7,7 @@
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:dast_profile) { create(:dast_profile, project: project) }
+  let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: project.default_branch) }
   let(:dast_profile_gid) { dast_profile.to_global_id }
@@ -46,7 +46,7 @@
       end
       context 'when DAST profile belongs to a project the user does not have access to' do
-        let_it_be(:dast_profile) { create(:dast_profile) }
+        let_it_be(:dast_profile) { create(:dast_profile, branch_name: project.default_branch) }
         it 'raises an exception' do
           expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)

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

@@ -79,7 +79,10 @@
         context 'when scan_type=active' do
           let(:dast_scanner_profile) { create(:dast_scanner_profile, project: project, scan_type: 'active') }
-          let(:dast_profile) { create(:dast_profile, project: project, dast_scanner_profile: dast_scanner_profile) }
+          let(:dast_profile) do
+            create(:dast_profile, project: project, dast_scanner_profile: dast_scanner_profile,
+              branch_name: project.default_branch)
+          end
           context 'when target is not validated' do
             it 'communicates failure' do

ee/spec/graphql/resolvers/app_sec/dast/profile_resolver_spec.rb

@@ -9,10 +9,10 @@
     stub_licensed_features(security_on_demand_scans: true)
   end
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :repository) }
   let_it_be(:developer) { create(:user, developer_of: project) }
-  let_it_be(:dast_profile1) { create(:dast_profile, project: project) }
-  let_it_be(:dast_profile2) { create(:dast_profile, project: project) }
+  let_it_be(:dast_profile1) { create(:dast_profile, project: project, branch_name: 'master') }
+  let_it_be(:dast_profile2) { create(:dast_profile, project: project, branch_name: 'master') }
   let(:current_user) { developer }
@@ -59,10 +59,18 @@
   private
   def dast_profiles
-    resolve(described_class, obj: project, ctx: { current_user: current_user })
+    resolve(described_class,
+      obj: project,
+      args: { calls_gitaly: true },
+      ctx: { current_user: current_user },
+      field_opts: { calls_gitaly: true })
   end
   def dast_profile(id:)
-    resolve(described_class.single, obj: project, args: { id: id }, ctx: { current_user: current_user })
+    resolve(described_class.single,
+      obj: project,
+      args: { id: id, calls_gitaly: true },
+      ctx: { current_user: current_user },
+      field_opts: { calls_gitaly: true })
   end
 end

ee/spec/models/dast/profile_spec.rb

@@ -157,5 +157,33 @@
     describe '#secret_ci_variables' do
       it { is_expected.to delegate_method(:secret_ci_variables).to(:dast_site_profile) }
     end
+
+    describe '#can_edit_scan?' do
+      using RSpec::Parameterized::TableSyntax
+
+      let_it_be(:user) { create(:user) }
+      let_it_be(:project) { create(:project, :repository) }
+
+      subject { create(:dast_profile, project: project, branch_name: 'master') }
+
+      before_all do
+        project.add_developer(user)
+      end
+
+      where(:can_push_to_branch, :result) do
+        false | false
+        true  | true
+      end
+
+      with_them do
+        it do
+          expect_next_instance_of(Gitlab::UserAccess) do |instance|
+            expect(instance).to receive(:can_push_to_branch?).with('master').and_return(can_push_to_branch)
+          end
+
+          expect(subject.can_edit_scan?(user)).to eq(result)
+        end
+      end
+    end
   end
 end

ee/spec/models/dast_scanner_profile_spec.rb

@@ -137,4 +137,44 @@
       end
     end
   end
+
+  describe '#can_edit_profile?' do
+    using RSpec::Parameterized::TableSyntax
+
+    let_it_be(:user) { create(:user, maintainer_of: project) }
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:dast_profile) { create(:dast_profile, project: project, branch_name: 'master') }
+
+    subject { DastScannerProfilesFinder.new(id: dast_profile.dast_scanner_profile_id).execute.to_a.first }
+
+    where(:can_push_to_branch, :result) do
+      false | false
+      true  | true
+    end
+
+    with_them do
+      it do
+        expect_next_instance_of(Gitlab::UserAccess) do |instance|
+          expect(instance).to receive(:can_push_to_branch?).with('master').and_return(can_push_to_branch)
+        end
+
+        expect(subject.can_edit_profile?(user)).to eq(result)
+      end
+    end
+  end
+
+  describe '#can_edit_profile? avoids N+1 queries' do
+    it do
+      project = create(:project, :repository)
+      user = create(:user, maintainer_of: project)
+      dast_profile1 = create(:dast_profile, project: project, branch_name: 'master')
+      dast_scanner_profile1 = dast_profile1.dast_scanner_profile
+
+      control = ActiveRecord::QueryRecorder.new { dast_scanner_profile1.can_edit_profile?(user) }
+
+      create(:dast_profile, project: project, branch_name: 'master', dast_scanner_profile: dast_scanner_profile1)
+
+      expect { dast_scanner_profile1.can_edit_profile?(user) }.not_to exceed_query_limit(control)
+    end
+  end
 end