Merge branch...

Merge branch 'philipcunningham-add-exlicit-authorisation-schema-for-dast-secrets-330297' into 'master'

Add explicit authorization schema for DAST secrets

See merge request gitlab-org/gitlab!62654

ee/app/models/dast/profile.rb

@@ -26,16 +26,14 @@ module Dast
       where(project_id: project_id)
     end
 
+    delegate :secret_ci_variables, to: :dast_site_profile
+
     def branch
       return unless project.repository.exists?
 
       Dast::Branch.new(self)
     end
 
-    def secret_ci_variables
-      ::Gitlab::Ci::Variables::Collection.new(secret_variables)
-    end
-
     private
 
     def project_ids_match

ee/app/models/dast_site_profile.rb

@@ -52,8 +52,12 @@ class DastSiteProfile < ApplicationRecord
     collection.compact
   end
 
-  def secret_ci_variables
-    ::Gitlab::Ci::Variables::Collection.new(secret_variables)
+  def secret_ci_variables(user)
+    collection = ::Gitlab::Ci::Variables::Collection.new
+
+    return collection unless Ability.allowed?(user, :read_on_demand_scans, self)
+
+    collection.concat(secret_variables)
   end
 
   def status

ee/app/models/ee/ci/build.rb

@@ -52,7 +52,7 @@ module EE
               # Subject to change. Please see gitlab-org/gitlab#330950 for more info.
               profile = pipeline.dast_profile || pipeline.dast_site_profile
 
-              collection.concat(profile.secret_ci_variables)
+              collection.concat(profile.secret_ci_variables(pipeline.user))
             end
           end
         end

ee/spec/models/ci/build_spec.rb

@@ -151,14 +151,19 @@ RSpec.describe Ci::Build do
 
       context 'when there is a dast_profile associated with the pipeline' do
         let_it_be(:project) { create(:project, :repository) }
+        let_it_be(:user) { create(:user, developer_projects: [project]) }
         let_it_be(:dast_profile) { create(:dast_profile, project: project) }
         let_it_be(:dast_site_profile_secret_variable) { create(:dast_site_profile_secret_variable, key: 'DAST_PASSWORD_BASE64', dast_site_profile: dast_profile.dast_site_profile) }
 
-        let(:pipeline) { create(:ci_pipeline, pipeline_params.merge!(project: project, dast_profile: dast_profile) ) }
+        let(:pipeline) { create(:ci_pipeline, pipeline_params.merge!(project: project, dast_profile: dast_profile, user: user) ) }
 
         let(:key) { dast_site_profile_secret_variable.key }
         let(:value) { dast_site_profile_secret_variable.value }
 
+        before do
+          stub_licensed_features(security_on_demand_scans: true)
+        end
+
         shared_examples 'a pipeline with no dast on-demand variables' do
           it 'does not include variables associated with the profile' do
             keys = subject.to_runner_variables.map { |var| var[:key] }
@@ -181,6 +186,24 @@ RSpec.describe Ci::Build do
           it 'includes variables associated with the profile' do
             expect(subject.to_runner_variables).to include(key: key, value: value, public: false, masked: true)
           end
+
+          context 'when user cannot read secrets' do
+            before do
+              stub_licensed_features(security_on_demand_scans: false)
+            end
+
+            it 'does not include variables associated with the profile' do
+              expect(subject.to_runner_variables).not_to include(key: key, value: value, public: false, masked: true)
+            end
+          end
+
+          context 'when there is no user associated with the pipeline' do
+            let_it_be(:user) { nil }
+
+            it 'does not include variables associated with the profile' do
+              expect(subject.to_runner_variables).not_to include(key: key, value: value, public: false, masked: true)
+            end
+          end
         end
       end
     end

ee/spec/models/dast/profile_spec.rb

@@ -123,19 +123,7 @@ RSpec.describe Dast::Profile, type: :model do
     end
 
     describe '#secret_ci_variables' do
-      context 'when there are no secret_variables' do
-        it 'returns an empty collection' do
-          expect(subject.secret_ci_variables.size).to be_zero
-        end
-      end
-
-      context 'when there are secret_variables' do
-        it 'returns a collection containing that variable' do
-          variable = create(:dast_site_profile_secret_variable, dast_site_profile: subject.dast_site_profile)
-
-          expect(subject.secret_ci_variables.to_runner_variables).to include(key: variable.key, value: variable.value, public: false, masked: true)
-        end
-      end
+      it { is_expected.to delegate_method(:secret_ci_variables).to(:dast_site_profile) }
     end
   end
 end

ee/spec/models/dast_site_profile_spec.rb

@@ -249,17 +249,47 @@ RSpec.describe DastSiteProfile, type: :model do
     end
 
     describe '#secret_ci_variables' do
-      context 'when there are no secret_variables' do
-        it 'returns an empty collection' do
-          expect(subject.secret_ci_variables.size).to be_zero
+      let_it_be(:user) { create(:user, developer_projects: [project]) }
+
+      context 'when user can read secrets' do
+        before do
+          stub_licensed_features(security_on_demand_scans: true)
+        end
+
+        it 'works with policy' do
+          expect(Ability.allowed?(user, :read_on_demand_scans, subject)).to be_truthy
+        end
+
+        it 'checks the policy' do
+          expect(Ability).to receive(:allowed?).with(user, :read_on_demand_scans, subject).and_call_original
+
+          subject.secret_ci_variables(user)
+        end
+
+        context 'when there are no secret_variables' do
+          it 'returns an empty collection' do
+            expect(subject.secret_ci_variables(user).size).to be_zero
+          end
+        end
+
+        context 'when there are secret_variables' do
+          it 'returns a collection containing that variable' do
+            variable = create(:dast_site_profile_secret_variable, dast_site_profile: subject)
+
+            expect(subject.secret_ci_variables(user).to_runner_variables).to include(key: variable.key, value: variable.value, public: false, masked: true)
+          end
         end
       end
 
-      context 'when there are secret_variables' do
-        it 'returns a collection containing that variable' do
-          variable = create(:dast_site_profile_secret_variable, dast_site_profile: subject)
+      context 'when user cannot read secrets' do
+        before do
+          stub_licensed_features(security_on_demand_scans: false)
+        end
+
+        it 'returns an empty collection' do
+          create(:dast_site_profile_secret_variable, dast_site_profile: subject)
 
-          expect(subject.secret_ci_variables.to_runner_variables).to include(key: variable.key, value: variable.value, public: false, masked: true)
+          expect(subject.secret_ci_variables(user).size).to be_zero
         end
       end
     end

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

@@ -11,9 +11,11 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
     stub_licensed_features(security_on_demand_scans: true)
   end
 
-  describe 'create_on_demand_dast_scan' do
+  describe 'dast on-demand policies' do
+    let(:policies) { [:create_on_demand_dast_scan, :read_on_demand_scans] }
+
     context 'when a user does not have access to the project' do
-      it { is_expected.to be_disallowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_disallowed(*policies) }
     end
 
     context 'when the user is a guest' do
@@ -21,7 +23,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
         project.add_guest(user)
       end
 
-      it { is_expected.to be_disallowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_disallowed(*policies) }
     end
 
     context 'when the user is a reporter' do
@@ -29,7 +31,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
         project.add_reporter(user)
       end
 
-      it { is_expected.to be_disallowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_disallowed(*policies) }
     end
 
     context 'when the user is a developer' do
@@ -37,7 +39,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
         project.add_developer(user)
       end
 
-      it { is_expected.to be_allowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_allowed(*policies) }
     end
 
     context 'when the user is a maintainer' do
@@ -45,7 +47,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
         project.add_maintainer(user)
       end
 
-      it { is_expected.to be_allowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_allowed(*policies) }
     end
 
     context 'when the user is an owner' do
@@ -53,7 +55,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
         group.add_owner(user)
       end
 
-      it { is_expected.to be_allowed(:create_on_demand_dast_scan) }
+      it { is_expected.to be_allowed(*policies) }
     end
 
     context 'when the user is allowed' do
@@ -68,7 +70,7 @@ RSpec.shared_examples 'a dast on-demand scan policy' do
           stub_licensed_features(security_on_demand_scans: false)
         end
 
-        it { is_expected.to be_disallowed(:create_on_demand_dast_scan) }
+        it { is_expected.to be_disallowed(*policies) }
       end
     end
   end