Merge branch 'ee-fix-failed-jobs-tab' into 'master'

Respect permissions when showing Failed Jobs (EE)

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

app/controllers/projects/pipelines_controller.rb

@@ -89,7 +89,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def failures
-    if @pipeline.statuses.latest.failed.present?
+    if @pipeline.failed_builds.present?
       render_show
     else
       redirect_to pipeline_path(@pipeline)

app/presenters/ci/pipeline_presenter.rb

 module Ci
   class PipelinePresenter < Gitlab::View::Presenter::Delegated
     prepend ::EE::Ci::PipelinePresenter
+    include Gitlab::Utils::StrongMemoize
 
     FAILURE_REASONS = {
       config_error: 'CI/CD YAML configuration error!'
@@ -8,6 +9,14 @@ module Ci
 
     presents :pipeline
 
+    def failed_builds
+      return [] unless can?(current_user, :read_build, pipeline)
+
+      strong_memoize(:failed_builds) do
+        pipeline.builds.latest.failed
+      end
+    end
+
     def failure_reason
       return unless pipeline.failure_reason?
 

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

-- failed_builds = @pipeline.statuses.latest.failed
 - expose_sast_data = @pipeline.expose_sast_data?
 - expose_dependency_data = @pipeline.expose_dependency_scanning_data?
 - blob_path = project_blob_path(@project, @pipeline.sha)
@@ -12,11 +11,11 @@
       = link_to builds_project_pipeline_path(@project, @pipeline), data: { target: '#js-tab-builds', action: 'builds', toggle: 'tab' }, class: 'builds-tab' do
         = _("Jobs")
         %span.badge.js-builds-counter= pipeline.total_size
-    - if failed_builds.present?
+    - if @pipeline.failed_builds.present?
       %li.js-failures-tab-link
         = 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
+          %span.badge.js-failures-counter= @pipeline.failed_builds.count
     - if expose_sast_data || expose_dependency_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
@@ -51,9 +50,10 @@
             %th Coverage
             %th
         = render partial: "projects/stage/stage", collection: pipeline.legacy_stages, as: :stage
-  - if failed_builds.present?
+
+  - if @pipeline.failed_builds.present?
     #js-tab-failures.build-failures.tab-pane
-      - failed_builds.each_with_index do |build, index|
+      - @pipeline.failed_builds.each_with_index do |build, index|
         .build-state
           %span.ci-status-icon-failed= custom_icon('icon_status_failed')
           %span.stage

spec/features/projects/pipelines/pipeline_spec.rb

@@ -3,10 +3,11 @@ require 'spec_helper'
 describe 'Pipeline', :js do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
+  let(:role) { :developer }
 
   before do
     sign_in(user)
-    project.add_developer(user)
+    project.add_role(user, role)
   end
 
   shared_context 'pipeline builds' do
@@ -153,9 +154,10 @@ describe 'Pipeline', :js do
     end
 
     context 'page tabs' do
-      it 'shows Pipeline and Jobs tabs with link' do
+      it 'shows Pipeline, Jobs and Failed Jobs tabs with link' do
         expect(page).to have_link('Pipeline')
         expect(page).to have_link('Jobs')
+        expect(page).to have_link('Failed Jobs')
       end
 
       it 'shows counter in Jobs tab' do
@@ -165,6 +167,16 @@ describe 'Pipeline', :js do
       it 'shows Pipeline tab as active' do
         expect(page).to have_css('.js-pipeline-tab-link.active')
       end
+
+      context 'without permission to access builds' do
+        let(:project) { create(:project, :public, :repository, public_builds: false) }
+        let(:role) { :guest }
+
+        it 'does not show failed jobs tab pane' do
+          expect(page).to have_link('Pipeline')
+          expect(page).not_to have_content('Failed Jobs')
+        end
+      end
     end
 
     context 'retrying jobs' do
@@ -308,8 +320,7 @@ describe 'Pipeline', :js do
   end
 
   describe 'GET /:project/pipelines/:id/failures' do
-    let(:project) { create(:project, :repository) }
-    let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master', sha: project.commit.id) }
+    let(:pipeline) { create(:ci_pipeline, project: project, ref: 'master', sha: '1234') }
     let(:pipeline_failures_page) { failures_project_pipeline_path(project, pipeline) }
     let!(:failed_build) { create(:ci_build, :failed, pipeline: pipeline) }
 
@@ -340,11 +351,39 @@ describe 'Pipeline', :js do
         visit pipeline_failures_page
       end
 
-      it 'includes failed jobs' do
+      it 'shows jobs tab pane as active' do
+        expect(page).to have_content('Failed Jobs')
+        expect(page).to have_css('#js-tab-failures.active')
+      end
+
+      it 'lists failed builds' do
+        expect(page).to have_content(failed_build.name)
+        expect(page).to have_content(failed_build.stage)
+      end
+
+      it 'does not show trace' do
         expect(page).to have_content('No job trace')
       end
     end
 
+    context 'without permission to access builds' do
+      let(:role) { :guest }
+
+      before do
+        project.update(public_builds: false)
+      end
+
+      context 'when accessing failed jobs page' do
+        before do
+          visit pipeline_failures_page
+        end
+
+        it 'fails to access the page' do
+          expect(page).to have_content('Access Denied')
+        end
+      end
+    end
+
     context 'without failures' do
       before do
         failed_build.update!(status: :success)