Merge branch 'dz-ci-view-sast-ultimate' into 'master'

Add missing EE check for CI SAST view

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

app/views/projects/pipelines/_with_tabs.html.haml

 - failed_builds = @pipeline.statuses.latest.failed
-- sast_artifact = @pipeline.sast_artifact
-- sast_artifact_url = raw_project_build_artifacts_url(@project, sast_artifact, path: Ci::Build::SAST_FILE) if sast_artifact
+- expose_sast_data = @pipeline.expose_sast_data?
 - blob_path = project_blob_path(@project, @pipeline.sha)
 
 .tabs-holder
@@ -17,7 +16,7 @@
         = link_to failures_project_pipeline_path(@project, @pipeline), data: { target: '#js-tab-failures', action: 'failures', toggle: 'tab' }, class: 'failures-tab' do
           = _("Failed Jobs")
           %span.badge.js-failures-counter= failed_builds.count
-    - if sast_artifact
+    - if expose_sast_data
       %li.js-security-tab-link
         = link_to security_project_pipeline_path(@project, @pipeline), data: { target: '#js-tab-security', action: 'security', toggle: 'tab' }, class: 'security-tab' do
           = _("Security report")
@@ -61,6 +60,6 @@
           %span.build-name
             = link_to build.name, pipeline_job_url(pipeline, build)
         %pre.build-log= build_summary(build, skip: index >= 10)
-  - if sast_artifact
+  - if expose_sast_data
     #js-tab-security.build-security.tab-pane
-      #js-security-report-app{ data: { endpoint: sast_artifact_url, blob_path: blob_path } }
+      #js-security-report-app{ data: { endpoint: sast_artifact_url(@pipeline), blob_path: blob_path } }

ee/app/controllers/ee/projects/pipelines_controller.rb

@@ -6,7 +6,7 @@ module EE
       def security
         commit
 
-        if pipeline.sast_artifact
+        if pipeline.expose_sast_data?
           render_show
         else
           redirect_to pipeline_path(pipeline)

ee/app/helpers/ee/gitlab_routing_helper.rb

@@ -27,5 +27,11 @@ module EE
     def epic_path(entity, *args)
       group_epic_path(entity.group, entity, *args)
     end
+
+    def sast_artifact_url(pipeline)
+      raw_project_build_artifacts_url(pipeline.project,
+                                      pipeline.sast_artifact,
+                                      path: Ci::Build::SAST_FILE)
+    end
   end
 end

ee/app/models/ee/ci/pipeline.rb

@@ -35,6 +35,50 @@ module EE
       def initialize_yaml_processor
         ::Gitlab::Ci::YamlProcessor.new(ci_yaml_file, { project: project, sha: sha })
       end
+
+      def has_sast_data?
+        sast_artifact&.success?
+      end
+
+      def has_sast_container_data?
+        sast_container_artifact&.success?
+      end
+
+      def has_dast_data?
+        dast_artifact&.success?
+      end
+
+      def has_performance_data?
+        performance_artifact&.success?
+      end
+
+      def has_codeclimate_data?
+        codeclimate_artifact&.success?
+      end
+
+      def expose_sast_data?
+        project.feature_available?(:sast) &&
+          has_sast_data?
+      end
+
+      def expose_sast_container_data?
+        project.feature_available?(:sast_container) &&
+          has_sast_container_data?
+      end
+
+      def expose_dast_data?
+        project.feature_available?(:dast) &&
+          has_dast_data?
+      end
+
+      def expose_performance_data?
+        project.feature_available?(:merge_request_performance_metrics) &&
+          has_performance_data?
+      end
+
+      def expose_codeclimate_data?
+        has_codeclimate_data?
+      end
     end
   end
 end

ee/app/models/ee/merge_request.rb

@@ -20,6 +20,10 @@ module EE
       delegate :dast_artifact, to: :head_pipeline, allow_nil: true
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
+      delegate :has_sast_data?, to: :base_pipeline, prefix: :base, allow_nil: true
+      delegate :expose_sast_data?, to: :head_pipeline, allow_nil: true
+      delegate :expose_sast_container_data?, to: :head_pipeline, allow_nil: true
+      delegate :expose_dast_data?, to: :head_pipeline, allow_nil: true
     end
 
     def squash_in_progress?
@@ -38,50 +42,14 @@ module EE
       false
     end
 
-    def has_codeclimate_data?
-      !!(head_codeclimate_artifact&.success? &&
-         base_codeclimate_artifact&.success?)
-    end
-
-    def has_performance_data?
-      !!(head_performance_artifact&.success? &&
-         base_performance_artifact&.success?)
-    end
-
-    def has_sast_data?
-      head_sast_artifact&.success?
-    end
-
-    def has_base_sast_data?
-      base_sast_artifact&.success?
-    end
-
-    def has_sast_container_data?
-      sast_container_artifact&.success?
-    end
-
-    def has_dast_data?
-      dast_artifact&.success?
+    def expose_codeclimate_data?
+      !!(head_pipeline&.expose_codeclimate_data? &&
+         base_pipeline&.expose_codeclimate_data?)
     end
 
     def expose_performance_data?
-      project.feature_available?(:merge_request_performance_metrics) &&
-        has_performance_data?
-    end
-
-    def expose_sast_data?
-      project.feature_available?(:sast) &&
-        has_sast_data?
-    end
-
-    def expose_dast_data?
-      project.feature_available?(:dast) &&
-        has_dast_data?
-    end
-
-    def expose_sast_container_data?
-      project.feature_available?(:sast_container) &&
-        has_sast_container_data?
+      !!(head_pipeline&.expose_performance_data? &&
+         base_pipeline&.expose_performance_data?)
     end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -13,7 +13,7 @@ module EE
         end
       end
 
-      expose :codeclimate, if: -> (mr, _) { mr.has_codeclimate_data? } do
+      expose :codeclimate, if: -> (mr, _) { mr.expose_codeclimate_data? } do
         expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_codeclimate_artifact) } do |merge_request|
           raw_project_build_artifacts_url(merge_request.source_project,
                                           merge_request.head_codeclimate_artifact,
@@ -48,7 +48,7 @@ module EE
                                           path: Ci::Build::SAST_FILE)
         end
 
-        expose :base_path, if: -> (mr, _) { mr.has_base_sast_data? && can?(current_user, :read_build, mr.base_sast_artifact)} do |merge_request|
+        expose :base_path, if: -> (mr, _) { mr.base_has_sast_data? && can?(current_user, :read_build, mr.base_sast_artifact)} do |merge_request|
           raw_project_build_artifacts_url(merge_request.target_project,
                                           merge_request.base_sast_artifact,
                                           path: Ci::Build::SAST_FILE)

ee/spec/controllers/projects/pipelines_controller_spec.rb

@@ -17,6 +17,7 @@ describe Projects::PipelinesController do
       before do
         create(
           :ci_build,
+          :success,
           :artifacts,
           name: 'sast',
           pipeline: pipeline,
@@ -26,24 +27,56 @@ describe Projects::PipelinesController do
             }
           }
         )
+      end
+
+      context 'with feature enabled' do
+        before do
+          allow(License).to receive(:feature_available?).and_return(true)
 
-        get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+          get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+        end
+
+        it do
+          expect(response).to have_gitlab_http_status(200)
+          expect(response).to render_template :show
+        end
       end
 
-      it do
-        expect(response).to have_gitlab_http_status(200)
-        expect(response).to render_template :show
+      context 'with feature disabled' do
+        before do
+          get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+        end
+
+        it do
+          expect(response).to have_gitlab_http_status(:redirect)
+          expect(response).to redirect_to(pipeline_path(pipeline))
+        end
       end
     end
 
     context 'without sast artifact' do
-      before do
-        get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+      context 'with feature enabled' do
+        before do
+          allow(License).to receive(:feature_available?).and_return(true)
+
+          get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+        end
+
+        it do
+          expect(response).to have_gitlab_http_status(:redirect)
+          expect(response).to redirect_to(pipeline_path(pipeline))
+        end
       end
 
-      it do
-        expect(response).to have_gitlab_http_status(:redirect)
-        expect(response).to redirect_to(pipeline_path(pipeline))
+      context 'with feature disabled' do
+        before do
+          get :security, namespace_id: project.namespace, project_id: project, id: pipeline
+        end
+
+        it do
+          expect(response).to have_gitlab_http_status(:redirect)
+          expect(response).to redirect_to(pipeline_path(pipeline))
+        end
       end
     end
   end

ee/spec/features/projects/pipelines/pipeline_spec.rb

@@ -7,6 +7,8 @@ describe 'Pipeline', :js do
   before do
     sign_in(user)
     project.add_developer(user)
+
+    allow(License).to receive(:feature_available?).and_return(true)
   end
 
   describe 'GET /:project/pipelines/:id/security' do
@@ -16,6 +18,7 @@ describe 'Pipeline', :js do
       before do
         create(
           :ci_build,
+          :success,
           :artifacts,
           name: 'sast',
           pipeline: pipeline,

ee/spec/models/ci/pipeline_spec.rb

@@ -56,4 +56,31 @@ describe Ci::Pipeline do
       end
     end
   end
+
+  %w(sast dast performance sast_container).each do |type|
+    method = "has_#{type}_data?"
+
+    describe "##{method}" do
+      let(:artifact) { double(success?: true) }
+
+      before do
+        allow(pipeline).to receive(:"#{type}_artifact").and_return(artifact)
+      end
+
+      it { expect(pipeline.send(method.to_sym)).to be_truthy }
+    end
+  end
+
+  %w(sast dast performance sast_container).each do |type|
+    method = "expose_#{type}_data?"
+
+    describe "##{method}" do
+      before do
+        allow(pipeline).to receive(:"has_#{type}_data?").and_return(true)
+        allow(pipeline.project).to receive(:feature_available?).and_return(true)
+      end
+
+      it { expect(pipeline.send(method.to_sym)).to be_truthy }
+    end
+  end
 end

ee/spec/models/merge_request_spec.rb

@@ -164,22 +164,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#has_codeclimate_data?' do
-    context 'with codeclimate artifact' do
-      before do
-        artifact = double(success?: true)
-        allow(subject.head_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
-        allow(subject.base_pipeline).to receive(:codeclimate_artifact).and_return(artifact)
-      end
-
-      it { expect(subject.has_codeclimate_data?).to be_truthy }
-    end
-
-    context 'without codeclimate artifact' do
-      it { expect(subject.has_codeclimate_data?).to be_falsey }
-    end
-  end
-
   describe '#head_sast_artifact' do
     it { is_expected.to delegate_method(:sast_artifact).to(:head_pipeline).with_prefix(:head) }
   end
@@ -188,54 +172,55 @@ describe MergeRequest do
     it { is_expected.to delegate_method(:sast_artifact).to(:base_pipeline).with_prefix(:base) }
   end
 
-  describe '#has_sast_data?' do
-    let(:artifact) { double(success?: true) }
-
-    before do
-      allow(merge_request).to receive(:head_sast_artifact).and_return(artifact)
-    end
+  describe '#sast_container_artifact' do
+    it { is_expected.to delegate_method(:sast_container_artifact).to(:head_pipeline) }
+  end
 
-    it { expect(merge_request.has_sast_data?).to be_truthy }
+  describe '#dast_artifact' do
+    it { is_expected.to delegate_method(:dast_artifact).to(:head_pipeline) }
   end
 
-  describe '#has_base_sast_data?' do
-    let(:artifact) { double(success?: true) }
+  describe '#base_has_sast_data?' do
+    it { is_expected.to delegate_method(:has_sast_data?).to(:base_pipeline).with_prefix(:base) }
+  end
 
-    before do
-      allow(merge_request).to receive(:base_sast_artifact).and_return(artifact)
-    end
+  %w(sast dast sast_container).each do |type|
+    method = "expose_#{type}_data?"
 
-    it { expect(merge_request.has_base_sast_data?).to be_truthy }
+    it { is_expected.to delegate_method(method.to_sym).to(:head_pipeline) }
   end
 
-  describe '#sast_container_artifact' do
-    it { is_expected.to delegate_method(:sast_container_artifact).to(:head_pipeline) }
-  end
+  describe '#expose_codeclimate_data?' do
+    context 'with codeclimate data' do
+      let(:pipeline) { double(expose_codeclimate_data?: true) }
 
-  describe '#has_dast_data?' do
-    let(:artifact) { double(success?: true) }
+      before do
+        allow(subject).to receive(:head_pipeline).and_return(pipeline)
+        allow(subject).to receive(:base_pipeline).and_return(pipeline)
+      end
 
-    before do
-      allow(merge_request).to receive(:dast_artifact).and_return(artifact)
+      it { expect(subject.expose_codeclimate_data?).to be_truthy }
     end
 
-    it { expect(merge_request.has_dast_data?).to be_truthy }
-  end
-
-  describe '#dast_artifact' do
-    it { is_expected.to delegate_method(:dast_artifact).to(:head_pipeline) }
+    context 'without codeclimate data' do
+      it { expect(subject.expose_codeclimate_data?).to be_falsey }
+    end
   end
 
-  %w(sast dast performance sast_container).each do |type|
-    method = "expose_#{type}_data?"
+  describe '#expose_performance_data?' do
+    context 'with performance data' do
+      let(:pipeline) { double(expose_performance_data?: true) }
 
-    describe "##{method}" do
       before do
-        allow(merge_request).to receive(:"has_#{type}_data?").and_return(true)
-        allow(merge_request.project).to receive(:feature_available?).and_return(true)
+        allow(subject).to receive(:head_pipeline).and_return(pipeline)
+        allow(subject).to receive(:base_pipeline).and_return(pipeline)
       end
 
-      it { expect(merge_request.send(method.to_sym)).to be_truthy }
+      it { expect(subject.expose_performance_data?).to be_truthy }
+    end
+
+    context 'without performance data' do
+      it { expect(subject.expose_performance_data?).to be_falsey }
     end
   end
 end

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -38,7 +38,7 @@ describe MergeRequestWidgetEntity do
     build = create(:ci_build, name: 'sast', pipeline: pipeline)
 
     allow(merge_request).to receive(:expose_sast_data?).and_return(true)
-    allow(merge_request).to receive(:has_base_sast_data?).and_return(true)
+    allow(merge_request).to receive(:base_has_sast_data?).and_return(true)
     allow(merge_request).to receive(:base_sast_artifact).and_return(build)
     allow(merge_request).to receive(:head_sast_artifact).and_return(build)