Merge branch 'security-11-5-test-permissions' into 'security-11-5'

[11.5] Pipelines section is available to unauthorized users

See merge request gitlab/gitlabhq!2806

(cherry picked from commit 3a060db7ea48eee0f08d06f312b01936abf9cc70)

bd1ae349 Backport security fix
b2469eeb Add CHANGELOG entry
957f6694 Rename Project#all_pipelines to Project#pipelines
8a9894d6 Remove destroy_pipeline specs

app/controllers/projects/merge_requests/application_controller.rb

@@ -39,8 +39,11 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
   end
   def set_pipeline_variables
-    @pipelines = @merge_request.all_pipelines
-    @pipeline = @merge_request.head_pipeline
-    @statuses_count = @pipeline.present? ? @pipeline.statuses.relevant.count : 0
+    @pipelines =
+      if can?(current_user, :read_pipeline, @project)
+        @merge_request.all_pipelines
+      else
+        Ci::Pipeline.none
+      end
   end
 end

app/controllers/projects/pipelines_controller.rb

@@ -4,6 +4,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   before_action :whitelist_query_limiting, only: [:create, :retry]
   before_action :pipeline, except: [:index, :new, :create, :charts]
   before_action :authorize_read_pipeline!
+  before_action :authorize_read_build!, only: [:index]
   before_action :authorize_create_pipeline!, only: [:new, :create]
   before_action :authorize_update_pipeline!, only: [:retry, :cancel]

app/helpers/projects_helper.rb

@@ -278,7 +278,8 @@ module ProjectsHelper
       nav_tabs << :container_registry
     end
-    if project.builds_enabled? && can?(current_user, :read_pipeline, project)
+    # Pipelines feature is tied to presence of builds
+    if can?(current_user, :read_build, project)
       nav_tabs << :pipelines
     end

app/models/commit.rb

@@ -11,6 +11,7 @@ class Commit
   include Mentionable
   include Referable
   include StaticModel
+  include Presentable
   include ::Gitlab::Utils::StrongMemoize
   attr_mentionable :safe_message, pipeline: :single_line
@@ -313,7 +314,9 @@ class Commit
   end
   def last_pipeline
-    @last_pipeline ||= pipelines.last
+    strong_memoize(:last_pipeline) do
+      pipelines.last
+    end
   end
   def status(ref = nil)

app/models/project.rb

@@ -531,6 +531,14 @@ class Project < ActiveRecord::Base
     end
   end
+  def pipelines
+    if builds_enabled?
+      super
+    else
+      super.external
+    end
+  end
+
   # returns all ancestor-groups upto but excluding the given namespace
   # when no namespace is given, all ancestors upto the top are returned
   def ancestors_upto(top = nil)

app/policies/ci/pipeline_policy.rb

@@ -10,6 +10,15 @@ module Ci
       @subject.project.branch_allows_collaboration?(@user, @subject.ref)
     end
+    condition(:external_pipeline, scope: :subject, score: 0) do
+      @subject.external?
+    end
+
+    # Disallow users without permissions from accessing internal pipelines
+    rule { ~can?(:read_build) & ~external_pipeline }.policy do
+      prevent :read_pipeline
+    end
+
     rule { protected_ref }.prevent :update_pipeline
     rule { can?(:public_access) & branch_allows_collaboration }.policy do

app/policies/project_policy.rb

@@ -103,6 +103,10 @@ class ProjectPolicy < BasePolicy
     @subject.feature_available?(:merge_requests, @user)
   end
+  condition(:internal_builds_disabled) do
+    !@subject.builds_enabled?
+  end
+
   features = %w[
     merge_requests
     issues
@@ -190,7 +194,6 @@ class ProjectPolicy < BasePolicy
     enable :read_build
     enable :read_container_image
     enable :read_pipeline
-    enable :read_pipeline_schedule
     enable :read_environment
     enable :read_deployment
     enable :read_merge_request
@@ -226,6 +229,7 @@ class ProjectPolicy < BasePolicy
     enable :update_build
     enable :create_pipeline
     enable :update_pipeline
+    enable :read_pipeline_schedule
     enable :create_pipeline_schedule
     enable :create_merge_request_from
     enable :create_wiki
@@ -305,7 +309,6 @@ class ProjectPolicy < BasePolicy
   end
   rule { builds_disabled | repository_disabled }.policy do
-    prevent(*create_update_admin_destroy(:pipeline))
     prevent(*create_read_update_admin_destroy(:build))
     prevent(*create_read_update_admin_destroy(:pipeline_schedule))
     prevent(*create_read_update_admin_destroy(:environment))
@@ -313,11 +316,22 @@ class ProjectPolicy < BasePolicy
     prevent(*create_read_update_admin_destroy(:deployment))
   end
+  # There's two separate cases when builds_disabled is true:
+  # 1. When internal CI is disabled - builds_disabled && internal_builds_disabled
+  #   - We do not prevent the user from accessing Pipelines to allow him to access external CI
+  # 2. When the user is not allowed to access CI - builds_disabled && ~internal_builds_disabled
+  #   - We prevent the user from accessing Pipelines
+  rule { (builds_disabled & ~internal_builds_disabled) | repository_disabled }.policy do
+    prevent(*create_read_update_admin_destroy(:pipeline))
+    prevent(*create_read_update_admin_destroy(:commit_status))
+  end
+
   rule { repository_disabled }.policy do
     prevent :push_code
     prevent :download_code
     prevent :fork_project
     prevent :read_commit_status
+    prevent :read_pipeline
   end
   rule { container_registry_disabled }.policy do
@@ -343,7 +357,6 @@ class ProjectPolicy < BasePolicy
     enable :read_merge_request
     enable :read_note
     enable :read_pipeline
-    enable :read_pipeline_schedule
     enable :read_commit_status
     enable :read_container_image
     enable :download_code
@@ -361,7 +374,6 @@ class ProjectPolicy < BasePolicy
   rule { public_builds & can?(:guest_access) }.policy do
     enable :read_pipeline
-    enable :read_pipeline_schedule
   end
   # These rules are included to allow maintainers of projects to push to certain

app/presenters/commit_presenter.rb

+# frozen_string_literal: true
+
+class CommitPresenter < Gitlab::View::Presenter::Simple
+  presents :commit
+
+  def status_for(ref)
+    can?(current_user, :read_commit_status, commit.project) && commit.status(ref)
+  end
+
+  def any_pipelines?
+    can?(current_user, :read_pipeline, commit.project) && commit.pipelines.any?
+  end
+end

app/presenters/merge_request_presenter.rb

@@ -170,6 +170,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     source_branch_exists? && merge_request.can_remove_source_branch?(current_user)
   end
+  def can_read_pipeline?
+    pipeline && can?(current_user, :read_pipeline, pipeline)
+  end
+
   def mergeable_discussions_state
     # This avoids calling MergeRequest#mergeable_discussions_state without
     # considering the state of the MR first. If a MR isn't mergeable, we can

app/serializers/merge_request_widget_entity.rb

@@ -54,7 +54,7 @@ class MergeRequestWidgetEntity < IssuableEntity
   end
   expose :merge_commit_message
-  expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline
+  expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? }
   expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)}
   # Booleans

app/views/projects/commit/_ci_menu.html.haml

+- any_pipelines = @commit.present(current_user: current_user).any_pipelines?
+
 %ul.nav-links.no-top.no-bottom.commit-ci-menu.nav.nav-tabs
   = nav_link(path: 'commit#show') do
     = link_to project_commit_path(@project, @commit.id) do
       Changes
       %span.badge.badge-pill= @diffs.size
-  - if can?(current_user, :read_pipeline, @project)
+  - if any_pipelines
     = nav_link(path: 'commit#pipelines') do
       = link_to pipelines_project_commit_path(@project, @commit.id) do
         Pipelines

app/views/projects/commit/_commit_box.html.haml

@@ -74,8 +74,8 @@
     %span.commit-info.merge-requests{ 'data-project-commit-path' => merge_requests_project_commit_path(@project, @commit.id, format: :json) }
       = icon('spinner spin')
-  - if @commit.last_pipeline
-    - last_pipeline = @commit.last_pipeline
+  - last_pipeline = @commit.last_pipeline
+  - if can?(current_user, :read_pipeline, last_pipeline)
     .well-segment.pipeline-info
       .status-icon-container
         = link_to project_pipeline_path(@project, last_pipeline.id), class: "ci-status-icon-#{last_pipeline.status}" do

app/views/projects/commit/show.html.haml

@@ -9,11 +9,8 @@
 .container-fluid{ class: [limited_container_width, container_class] }
   = render "commit_box"
-  - if @commit.status
-    = render "ci_menu"
-  - else
-    .block-connector
-  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment
+  = render "ci_menu"
+  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, is_commit: true
   .limited-width-notes
     = render "shared/notes/notes_with_form", :autocomplete => true

app/views/projects/commits/_commit.html.haml

@@ -6,6 +6,7 @@
 - merge_request = local_assigns.fetch(:merge_request, nil)
 - project       = local_assigns.fetch(:project) { merge_request&.project }
 - ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
+- commit_status = commit.present(current_user: current_user).status_for(ref)
 - link = commit_path(project, commit, merge_request: merge_request)
 %li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" }
@@ -22,7 +23,7 @@
       %span.commit-row-message.d-block.d-sm-none
         ·
         = commit.short_id
-      - if commit.status(ref)
+      - if commit_status
         .d-block.d-sm-none
           = render_commit_status(commit, ref: ref)
       - if commit.description?
@@ -45,7 +46,7 @@
       - else
         = render partial: 'projects/commit/ajax_signature', locals: { commit: commit }
-      - if commit.status(ref)
+      - if commit_status
         = render_commit_status(commit, ref: ref)
       .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } }

app/views/projects/issues/_merge_requests.html.haml

@@ -4,9 +4,10 @@
   %ul.unstyled-list.related-merge-requests
     - has_any_head_pipeline = @merge_requests.any?(&:head_pipeline_id)
     - @merge_requests.each do |merge_request|
+      - merge_request = merge_request.present(current_user: current_user)
       %li
         %span.merge-request-ci-status
-          - if merge_request.head_pipeline
+          - if merge_request.can_read_pipeline?
             = render_pipeline_status(merge_request.head_pipeline)
           - elsif has_any_head_pipeline
             = icon('blank fw')

app/views/projects/issues/_related_branches.html.haml

@@ -6,7 +6,7 @@
       %li
         - target = @project.repository.find_branch(branch).dereferenced_target
         - pipeline = @project.pipeline_for(branch, target.sha) if target
-        - if pipeline
+        - if can?(current_user, :read_pipeline, pipeline)
           %span.related-branch-ci-status
             = render_pipeline_status(pipeline)
         %span.related-branch-info

app/views/projects/merge_requests/_merge_request.html.haml

@@ -46,7 +46,7 @@
           %li.issuable-status.d-none.d-sm-inline-block
             = icon('ban')
             CLOSED
-        - if merge_request.head_pipeline
+        - if can?(current_user, :read_pipeline, merge_request.head_pipeline)
           %li.issuable-pipeline-status.d-none.d-sm-inline-block
             = render_pipeline_status(merge_request.head_pipeline)
         - if merge_request.open? && merge_request.broken?

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

@@ -6,23 +6,22 @@
       = preserve(markdown(commit.description, pipeline: :single_line))
 .info-well
-  - if commit.status
-    .well-segment.pipeline-info
-      .icon-container
-        = icon('clock-o')
-      = pluralize @pipeline.total_size, "job"
-      - if @pipeline.ref
-        from
-        - if @pipeline.ref_exists?
-          = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
-        - else
-          %span.ref-name
-            = @pipeline.ref
-      - if @pipeline.duration
-        in
-        = time_interval_in_words(@pipeline.duration)
-      - if @pipeline.queued_duration
-        = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
+  .well-segment.pipeline-info
+    .icon-container
+      = icon('clock-o')
+    = pluralize @pipeline.total_size, "job"
+    - if @pipeline.ref
+      from
+      - if @pipeline.ref_exists?
+        = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name"
+      - else
+        %span.ref-name
+          = @pipeline.ref
+    - if @pipeline.duration
+      in
+      = time_interval_in_words(@pipeline.duration)
+    - if @pipeline.queued_duration
+      = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})"
   .well-segment.branch-info
     .icon-container.commit-icon

changelogs/unreleased/security-11-5-test-permissions.yml

+---
+title: Disallows unauthorized users from accessing the pipelines section.
+merge_request:
+author:
+type: security

lib/api/pipelines.rb

@@ -76,7 +76,7 @@ module API
         requires :pipeline_id, type: Integer, desc: 'The pipeline ID'
       end
       get ':id/pipelines/:pipeline_id' do
-        authorize! :read_pipeline, user_project
+        authorize! :read_pipeline, pipeline
         present pipeline, with: Entities::Pipeline
       end
@@ -89,7 +89,7 @@ module API
         requires :pipeline_id, type: Integer,  desc: 'The pipeline ID'
       end
       post ':id/pipelines/:pipeline_id/retry' do
-        authorize! :update_pipeline, user_project
+        authorize! :update_pipeline, pipeline
         pipeline.retry_failed(current_user)
@@ -104,7 +104,7 @@ module API
         requires :pipeline_id, type: Integer,  desc: 'The pipeline ID'
       end
       post ':id/pipelines/:pipeline_id/cancel' do
-        authorize! :update_pipeline, user_project
+        authorize! :update_pipeline, pipeline
         pipeline.cancel_running