Merge branch 'bwill/security-policy-edit-page' into 'master'

Add security policies edit page

See merge request gitlab-org/gitlab!66543

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

@@ -6,6 +6,7 @@ module Projects
       include SecurityAndCompliancePermissions
 
       before_action :authorize_security_orchestration_policies!
+      before_action :validate_policy_configuration, only: :edit
 
       before_action do
         push_frontend_feature_flag(:security_orchestration_policies_configuration, project)
@@ -18,8 +19,53 @@ module Projects
         render :show, locals: { project: project }
       end
 
+      def edit
+        @policy_name = URI.decode_www_form_component(params[:id])
+        @policy_type = params[:type]
+
+        result = ::Security::SecurityOrchestrationPolicies::FetchPolicyService
+                  .new(policy_configuration: policy_configuration, name: @policy_name, type: @policy_type.to_sym)
+                  .execute
+        @policy = result[:policy]
+
+        return render_404 if @policy.blank?
+
+        render :edit
+      end
+
       private
 
+      def validate_policy_configuration
+        type = params[:type]
+        result = ::Security::SecurityOrchestrationPolicies::PolicyConfigurationValidationService
+          .new(policy_configuration: policy_configuration, type: (type.to_sym if type)).execute
+
+        if result[:status] == :error
+          case result[:invalid_component]
+          when :policy_configuration
+            redirect_to project_security_policy_path(project), alert: result[:message]
+          when :policy_project
+            redirect_to project_path(policy_configuration.security_policy_management_project)
+          when :policy_yaml
+            policy_management_project = policy_configuration.security_policy_management_project
+            policy_path = File.join(policy_management_project.default_branch, ::Security::OrchestrationPolicyConfiguration::POLICY_PATH)
+
+            redirect_to project_blob_path(policy_management_project, policy_path), alert: result[:message]
+          else
+            # We should redirect to security policies list view once it is implemented.
+            # For now, we will render_404
+
+            # This case also covers `when :parameter`
+            # redirect_to project_security_policies_path(project), alert: result[:message]
+            render_404
+          end
+        end
+      end
+
+      def policy_configuration
+        @policy_configuration ||= project.security_orchestration_policy_configuration
+      end
+
       def check_feature_flag!
         render_404 if Feature.disabled?(:security_orchestration_policies_configuration, project)
       end

ee/app/models/security/orchestration_policy_configuration.rb

@@ -99,10 +99,14 @@ module Security
       rule_schedules.delete_all(:delete_all)
     end
 
-    def scan_execution_policy
+    def policy_by_type(type)
       return [] if policy_hash.blank?
 
-      policy_hash.fetch(:scan_execution_policy, [])
+      policy_hash.fetch(type, [])
+    end
+
+    def scan_execution_policy
+      policy_by_type(:scan_execution_policy)
     end
 
     def default_branch_or_main

ee/app/services/security/security_orchestration_policies/fetch_policy_service.rb

+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class FetchPolicyService
+      include BaseServiceUtility
+
+      def initialize(policy_configuration:, name:, type:)
+        @policy_configuration = policy_configuration
+        @name = name
+        @type = type
+      end
+
+      def execute
+        success({ policy: policy })
+      end
+
+      private
+
+      attr_reader :policy_configuration, :type, :name
+
+      def policy
+        policy_configuration
+          .policy_by_type(type)
+          .find { |policy| policy[:name] == name }
+      end
+    end
+  end
+end

ee/app/services/security/security_orchestration_policies/policy_configuration_validation_service.rb

+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class PolicyConfigurationValidationService
+      include BaseServiceUtility
+
+      def initialize(policy_configuration:, type:)
+        @policy_configuration = policy_configuration
+        @type = type
+      end
+
+      def execute
+        return error_response(_('type parameter is missing and is required'), :parameter) unless @type
+        return error_response(_('Invalid policy type'), :parameter) unless valid_type?
+        return error_response(_('Project does not have a policy configuration'), :policy_configuration) if policy_configuration.nil?
+
+        unless policy_configuration.policy_configuration_exists?
+          return error_response(
+            _("Policy management project does have any policies in %{policy_path}" % {
+              policy_path: ::Security::OrchestrationPolicyConfiguration::POLICY_PATH
+            }),
+            :policy_project
+          )
+        end
+
+        unless policy_configuration.policy_configuration_valid?
+          return error_response(_('Could not fetch policy because existing policy YAML is invalid'), :policy_yaml)
+        end
+
+        success
+      end
+
+      private
+
+      attr_reader :policy_configuration, :type
+
+      def error_response(message, invalid_component)
+        error(message, pass_back: { invalid_component: invalid_component })
+      end
+
+      def valid_type?
+        Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(type)
+      end
+    end
+  end
+end

ee/app/views/projects/security/policies/edit.html.haml

+- add_to_breadcrumbs s_("SecurityPolicies|Policies"), project_security_policy_path(@project)
+- breadcrumb_title s_("SecurityPolicies|Edit policy")
+- page_title s_("SecurityPolicies|Edit policy")
+
+#js-policy-builder-app{ data: { policy: @policy.to_json,
+    policy_type: @policy_type,
+    project_path: @project.full_path,
+    project_id: @project.id } }

ee/config/routes/project.rb

@@ -62,6 +62,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
           resource :policy, only: [:show]
 
+          resources :policies, only: [:edit], controller: :policies, constraints: { id: %r{[^/]+} }
+
           resource :configuration, only: [], controller: :configuration do
             post :auto_fix, on: :collection
             resource :corpus_management, only: [:show], controller: :corpus_management

ee/spec/models/security/orchestration_policy_configuration_spec.rb

@@ -157,6 +157,46 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
     end
   end
 
+  describe '#policy_by_type' do
+    subject { security_orchestration_policy_configuration.policy_by_type(:scan_execution_policy) }
+
+    before do
+      allow(security_policy_management_project).to receive(:repository).and_return(repository)
+      allow(repository).to receive(:blob_data_at).with(default_branch, Security::OrchestrationPolicyConfiguration::POLICY_PATH).and_return(policy_yaml)
+    end
+
+    context 'when policy is present' do
+      let(:policy_yaml) do
+        <<-EOS
+        scan_execution_policy:
+        - name: Run DAST in every pipeline
+          description: This policy enforces to run DAST for every pipeline within the project
+          enabled: true
+          rules:
+          - type: pipeline
+            branches:
+            - "production"
+          actions:
+          - scan: dast
+            site_profile: Site Profile
+            scanner_profile: Scanner Profile
+        EOS
+      end
+
+      it 'retrieves policy by type' do
+        expect(subject.first[:name]).to eq('Run DAST in every pipeline')
+      end
+    end
+
+    context 'when policy is nil' do
+      let(:policy_yaml) { nil }
+
+      it 'returns an empty array' do
+        expect(subject).to eq([])
+      end
+    end
+  end
+
   describe '#policy_configuration_valid?' do
     subject { security_orchestration_policy_configuration.policy_configuration_valid? }
 

ee/spec/requests/projects/security/policies_controller_spec.rb

@@ -5,14 +5,157 @@ require 'spec_helper'
 RSpec.describe Projects::Security::PoliciesController, type: :request do
   let_it_be(:owner) { create(:user) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:project, reload: true) { create(:project, namespace: owner.namespace) }
+  let_it_be(:project) { create(:project, namespace: owner.namespace) }
+  let_it_be(:policy_management_project) { create(:project, :repository, namespace: owner.namespace) }
+  let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
+  let_it_be(:policy) do
+    {
+      name: 'Run DAST in every pipeline',
+      description: 'This policy enforces to run DAST for every pipeline within the project',
+      enabled: true,
+      rules: [{ type: 'pipeline', branches: %w[production] }],
+      actions: [
+        { scan: 'dast', site_profile: 'Site Profile', scanner_profile: 'Scanner Profile' }
+      ]
+    }
+  end
+
+  let_it_be(:type) { 'scan_execution_policy' }
+  let_it_be(:show) { project_security_policy_url(project) }
+  let_it_be(:edit) { edit_project_security_policy_url(project, id: policy[:name], type: type) }
+
+  let_it_be(:feature_enabled) { true }
 
   before do
     project.add_developer(user)
-    login_as(user)
+    sign_in(user)
+    stub_feature_flags(security_orchestration_policies_configuration: feature_enabled)
+    stub_licensed_features(security_orchestration_policies: feature_enabled)
+    allow_next_instance_of(Repository) do |repository|
+      allow(repository).to receive(:blob_data_at).and_return({ scan_execution_policy: [policy] }.to_yaml)
+    end
+  end
+
+  describe 'GET #edit' do
+    context 'with authorized user' do
+      context 'when feature is available' do
+        it 'renders the edit page' do
+          get edit
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response).to render_template(:edit)
+
+          app = Nokogiri::HTML.parse(response.body).at_css('div#js-policy-builder-app')
+
+          expect(app.attributes['data-policy'].value).to eq(policy.to_json)
+          expect(app.attributes['data-policy-type'].value).to eq(type)
+        end
+
+        context 'when type is missing' do
+          let_it_be(:edit) { edit_project_security_policy_url(project, id: policy[:name]) }
+
+          it 'returns 404' do
+            get edit
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+
+        context 'when id does not exist' do
+          let_it_be(:edit) { edit_project_security_policy_url(project, id: 'no-existing-policy', type: type) }
+
+          it 'returns 404' do
+            get edit
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+
+        context 'when there is no policy configuration' do
+          let_it_be(:project) { create(:project, namespace: owner.namespace) }
+          let_it_be(:policy_configuration) { nil }
+          let_it_be(:edit) { edit_project_security_policy_url(project, id: policy[:name], type: type) }
+
+          it 'redirects to policy configuration page' do
+            get edit
+
+            expect(response).to redirect_to(project_security_policy_path(project))
+          end
+        end
+
+        context 'when policy yaml file does not exist' do
+          before do
+            allow_next_instance_of(Repository) do |repository|
+              allow(repository).to receive(:blob_data_at).and_return({}.to_yaml)
+            end
+          end
+
+          it 'redirects to project page' do
+            get edit
+
+            expect(response).to redirect_to(project_path(policy_management_project))
+          end
+        end
+
+        context 'when policy yaml is invalid' do
+          let_it_be(:policy) { 'invalid' }
+
+          it 'redirects to policy file' do
+            get edit
+
+            expect(response).to redirect_to(
+              project_blob_path(
+                policy_management_project,
+                File.join(policy_management_project.default_branch, ::Security::OrchestrationPolicyConfiguration::POLICY_PATH)
+              )
+            )
+          end
+        end
+      end
+
+      context 'when feature is not available' do
+        let_it_be(:feature_enabled) { false }
+
+        it 'returns 404' do
+          get edit
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context 'with unauthorized user' do
+      before do
+        project.add_guest(user)
+        sign_in(user)
+      end
+
+      context 'when feature is available' do
+        let_it_be(:feature_enabled) { true }
+
+        it 'returns 404' do
+          get edit
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
+    end
+
+    context 'with anonymous user' do
+      before do
+        sign_out(user)
+      end
+
+      it 'returns 302' do
+        get edit
+
+        expect(response).to have_gitlab_http_status(:found)
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
   end
 
-  context 'displaying page' do
+  describe 'GET #show' do
     using RSpec::Parameterized::TableSyntax
 
     where(:feature_flag, :license, :status) do
@@ -22,7 +165,7 @@ RSpec.describe Projects::Security::PoliciesController, type: :request do
       true | false | :not_found
     end
 
-    subject { get project_security_policy_url(project) }
+    subject(:request) { get show, params: { namespace_id: project.namespace, project_id: project } }
 
     with_them do
       before do
@@ -31,7 +174,7 @@ RSpec.describe Projects::Security::PoliciesController, type: :request do
       end
 
       specify do
-        get project_security_policy_url(project)
+        subject
 
         expect(response).to have_gitlab_http_status(status)
       end

ee/spec/routing/project_routing_spec.rb

@@ -66,9 +66,19 @@ RSpec.describe 'EE-specific project routing' do
   end
 
   describe Projects::Security::PoliciesController, 'routing' do
+    where(:id) do
+      %w[test.1.2 test-policy test:policy]
+    end
+
     it 'to #show' do
       expect(get('/gitlab/gitlabhq/-/security/policy')).to route_to('projects/security/policies#show', namespace_id: 'gitlab', project_id: 'gitlabhq')
     end
+
+    with_them do
+      it 'to #edit' do
+        expect(get("/gitlab/gitlabhq/-/security/policies/#{id}/edit")).to route_to('projects/security/policies#edit', namespace_id: 'gitlab', project_id: 'gitlabhq', id: id)
+      end
+    end
   end
 
   describe Projects::ThreatMonitoringController, 'routing' do

ee/spec/services/security/security_orchestration_policies/fetch_policy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SecurityOrchestrationPolicies::FetchPolicyService do
+  describe '#execute' do
+    let(:project) { create(:project) }
+    let(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    let(:policy) do
+      {
+        name: 'Run DAST in every pipeline',
+        description: 'This policy enforces to run DAST for every pipeline within the project',
+        enabled: true,
+        rules: [{ type: 'pipeline', branches: %w[production] }],
+        actions: [
+          { scan: 'dast', site_profile: 'Site Profile', scanner_profile: 'Scanner Profile' }
+        ]
+      }
+    end
+
+    let(:policy_blob) { { scan_execution_policy: [policy] }.to_yaml }
+    let(:type) { :scan_execution_policy }
+    let(:name) { 'Run DAST in every pipeline' }
+
+    subject(:service) do
+      described_class.new(policy_configuration: policy_configuration, name: name, type: type)
+    end
+
+    before do
+      allow_next_instance_of(Repository) do |repository|
+        allow(repository).to receive(:blob_data_at).and_return(policy_blob)
+      end
+    end
+
+    context 'when retrieving an existing policy by name' do
+      it 'returns policy' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:policy]).to eq(policy)
+      end
+    end
+
+    context 'when retrieving an non-existing policy by name' do
+      let(:name) { 'Invalid name' }
+
+      it 'returns nil' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+        expect(response[:policy]).to eq(nil)
+      end
+    end
+  end
+end

ee/spec/services/security/security_orchestration_policies/policy_configuration_validation_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SecurityOrchestrationPolicies::PolicyConfigurationValidationService do
+  describe '#execute' do
+    let(:project) { create(:project) }
+    let(:policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    let(:policy) do
+      {
+        name: 'Run DAST in every pipeline',
+        description: 'This policy enforces to run DAST for every pipeline within the project',
+        enabled: true,
+        rules: [{ type: 'pipeline', branches: %w[production] }],
+        actions: [
+          { scan: 'dast', site_profile: 'Site Profile', scanner_profile: 'Scanner Profile' }
+        ]
+      }
+    end
+
+    let(:policy_blob) { { scan_execution_policy: [policy] }.to_yaml }
+    let(:type) { :scan_execution_policy }
+
+    subject(:service) do
+      described_class.new(policy_configuration: policy_configuration, type: type)
+    end
+
+    before do
+      allow_next_instance_of(Repository) do |repository|
+        allow(repository).to receive(:blob_data_at).and_return(policy_blob)
+      end
+    end
+
+    context 'when all components are valid' do
+      it 'returns success' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:success)
+      end
+    end
+
+    context 'when security_orchestration_policies_configuration is missing' do
+      let(:policy_configuration) { nil }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('Project does not have a policy configuration')
+        expect(response[:invalid_component]).to eq(:policy_configuration)
+      end
+    end
+
+    context 'when security_orchestration_policies_configuration is invalid' do
+      let(:policy_blob) { { scan_execution_policy: 'invalid' }.to_yaml }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('Could not fetch policy because existing policy YAML is invalid')
+        expect(response[:invalid_component]).to eq(:policy_yaml)
+      end
+    end
+
+    context 'when type parameter is missing' do
+      let(:type) { nil }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('type parameter is missing and is required')
+        expect(response[:invalid_component]).to eq(:parameter)
+      end
+    end
+
+    context 'when retrieving an invalid policy type' do
+      let(:type) { :invalid }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('Invalid policy type')
+        expect(response[:invalid_component]).to eq(:parameter)
+      end
+    end
+
+    context 'when policy.yml is empty' do
+      let(:policy_blob) { {}.to_yaml }
+
+      it 'returns an error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq("Policy management project does have any policies in #{::Security::OrchestrationPolicyConfiguration::POLICY_PATH}")
+        expect(response[:invalid_component]).to eq(:policy_project)
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -9229,6 +9229,9 @@ msgstr ""
 msgid "Could not draw the lines for job relationships"
 msgstr ""
 
+msgid "Could not fetch policy because existing policy YAML is invalid"
+msgstr ""
+
 msgid "Could not find design."
 msgstr ""
 
@@ -17987,6 +17990,9 @@ msgstr ""
 msgid "Invalid pod_name"
 msgstr ""
 
+msgid "Invalid policy type"
+msgstr ""
+
 msgid "Invalid query"
 msgstr ""
 
@@ -24906,6 +24912,9 @@ msgstr ""
 msgid "Policies"
 msgstr ""
 
+msgid "Policy management project does have any policies in %{policy_path}"
+msgstr ""
+
 msgid "Policy project doesn't exist"
 msgstr ""
 
@@ -25671,6 +25680,9 @@ msgstr ""
 msgid "Project does not exist or you don't have permission to perform this action"
 msgstr ""
 
+msgid "Project does not have a policy configuration"
+msgstr ""
+
 msgid "Project export could not be deleted."
 msgstr ""
 
@@ -29430,6 +29442,9 @@ msgstr ""
 msgid "SecurityPolicies|Description"
 msgstr ""
 
+msgid "SecurityPolicies|Edit policy"
+msgstr ""
+
 msgid "SecurityPolicies|Enforcement status"
 msgstr ""
 
@@ -29442,6 +29457,9 @@ msgstr ""
 msgid "SecurityPolicies|Network"
 msgstr ""
 
+msgid "SecurityPolicies|Policies"
+msgstr ""
+
 msgid "SecurityPolicies|Policy type"
 msgstr ""
 
@@ -40067,6 +40085,9 @@ msgstr ""
 msgid "type must be Debian"
 msgstr ""
 
+msgid "type parameter is missing and is required"
+msgstr ""
+
 msgid "unicode domains should use IDNA encoding"
 msgstr ""