From a0383ab43ec2c885aae6602ffa47ffde79c76786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 28 Jan 2019 12:12:30 +0000 Subject: [master] Pipelines section is available to unauthorized users --- .../merge_requests/application_controller.rb | 9 ++-- app/controllers/projects/pipelines_controller.rb | 1 + app/helpers/projects_helper.rb | 3 +- app/models/commit.rb | 5 +- app/models/project.rb | 8 ++++ app/policies/ci/pipeline_policy.rb | 9 ++++ app/policies/project_policy.rb | 20 ++++++-- app/presenters/commit_presenter.rb | 13 ++++++ app/presenters/merge_request_presenter.rb | 4 ++ app/serializers/merge_request_widget_entity.rb | 2 +- app/views/projects/commit/_ci_menu.html.haml | 4 +- app/views/projects/commit/_commit_box.html.haml | 4 +- app/views/projects/commit/show.html.haml | 5 +- app/views/projects/commits/_commit.html.haml | 5 +- .../projects/issues/_merge_requests.html.haml | 3 +- .../projects/issues/_related_branches.html.haml | 2 +- .../merge_requests/_merge_request.html.haml | 2 +- app/views/projects/pipelines/_info.html.haml | 33 +++++++------ changelogs/unreleased/test-permissions.yml | 5 ++ lib/api/pipelines.rb | 6 +-- .../projects/pipeline_schedules_controller_spec.rb | 11 ++++- .../projects/pipelines_controller_spec.rb | 47 +++++++++++++------ .../security/project/internal_access_spec.rb | 6 +-- .../security/project/private_access_spec.rb | 2 +- .../security/project/public_access_spec.rb | 10 ++-- spec/helpers/projects_helper_spec.rb | 16 ++++++- .../import_export/project_tree_restorer_spec.rb | 4 +- spec/models/commit_spec.rb | 1 + spec/models/project_spec.rb | 24 ++++++++++ spec/policies/ci/pipeline_policy_spec.rb | 8 ++++ spec/policies/project_policy_spec.rb | 44 +++++++++++++----- spec/presenters/commit_presenter_spec.rb | 54 ++++++++++++++++++++++ .../merge_request_widget_entity_spec.rb | 39 +++++++++++----- .../projects/commit/_commit_box.html.haml_spec.rb | 6 ++- .../issues/_related_branches.html.haml_spec.rb | 4 ++ 35 files changed, 324 insertions(+), 95 deletions(-) create mode 100644 app/presenters/commit_presenter.rb create mode 100644 changelogs/unreleased/test-permissions.yml create mode 100644 spec/presenters/commit_presenter_spec.rb diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 368ee89ff5c..54ff7ded8e5 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/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 diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 67827b1d3bb..df43d9994a1 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/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] diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index b0d3d509f5d..85248a16f50 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -305,7 +305,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 diff --git a/app/models/commit.rb b/app/models/commit.rb index 01f4c58daa1..982e13e2845 100644 --- a/app/models/commit.rb +++ b/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 @@ -304,7 +305,9 @@ class Commit end def last_pipeline - @last_pipeline ||= pipelines.last + strong_memoize(:last_pipeline) do + pipelines.last + end end def status(ref = nil) diff --git a/app/models/project.rb b/app/models/project.rb index 1023b40a608..cecfee82334 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -578,6 +578,14 @@ class Project < ActiveRecord::Base end end + def all_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, hierarchy_order: nil) diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index e42d78f47c5..2c90b8a73cd 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/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 diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 95ae85ed60c..cadbc5ae009 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -108,6 +108,10 @@ class ProjectPolicy < BasePolicy condition(:has_clusters, scope: :subject) { clusterable_has_clusters? } condition(:can_have_multiple_clusters) { multiple_clusters_available? } + condition(:internal_builds_disabled) do + !@subject.builds_enabled? + end + features = %w[ merge_requests issues @@ -196,7 +200,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 @@ -235,6 +238,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 @@ -320,7 +324,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)) @@ -328,11 +331,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 prevent(*create_read_update_admin_destroy(:release)) end @@ -359,7 +373,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 @@ -378,7 +391,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 diff --git a/app/presenters/commit_presenter.rb b/app/presenters/commit_presenter.rb new file mode 100644 index 00000000000..05adbe1d4f5 --- /dev/null +++ b/app/presenters/commit_presenter.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 44b6ca299ae..c59e73f824c 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/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 diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index 9361c9f987b..f42abf06e1e 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -57,7 +57,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 diff --git a/app/views/projects/commit/_ci_menu.html.haml b/app/views/projects/commit/_ci_menu.html.haml index f6666921a25..8b6e3e42ea1 100644 --- a/app/views/projects/commit/_ci_menu.html.haml +++ b/app/views/projects/commit/_ci_menu.html.haml @@ -1,9 +1,11 @@ +- 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 diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 2a919a767c0..3971ca473e0 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/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 diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 79e32949db9..06f0cd9675e 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -9,10 +9,7 @@ .container-fluid{ class: [limited_container_width, container_class] } = render "commit_box" - - if @commit.status - = render "ci_menu" - - else - .block-connector + = render "ci_menu" = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, is_commit: true .limited-width-notes diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 1a74b120c26..0d3c6e7027c 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/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) } } diff --git a/app/views/projects/issues/_merge_requests.html.haml b/app/views/projects/issues/_merge_requests.html.haml index eb46bf5c6a3..6f652391be3 100644 --- a/app/views/projects/issues/_merge_requests.html.haml +++ b/app/views/projects/issues/_merge_requests.html.haml @@ -12,6 +12,7 @@ %ul.content-list.related-items-list - 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.list-item.py-0.px-0 .item-body.issuable-info-container.py-lg-3.px-lg-3.pl-md-3 .item-contents @@ -25,7 +26,7 @@ = merge_request.target_project.full_path = merge_request.to_reference %span.mr-ci-status.flex-md-grow-1.justify-content-end.d-flex.ml-md-2 - - if merge_request.head_pipeline + - if merge_request.can_read_pipeline? = render_pipeline_status(merge_request.head_pipeline, tooltip_placement: 'bottom') - elsif has_any_head_pipeline = icon('blank fw') diff --git a/app/views/projects/issues/_related_branches.html.haml b/app/views/projects/issues/_related_branches.html.haml index 1df38db9fd4..ffdd96870ef 100644 --- a/app/views/projects/issues/_related_branches.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index faa070d0389..d7994909366 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/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? diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 0f0114d513c..69a47faabed 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/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 .icon-container diff --git a/changelogs/unreleased/test-permissions.yml b/changelogs/unreleased/test-permissions.yml new file mode 100644 index 00000000000..cfb69fdcb1e --- /dev/null +++ b/changelogs/unreleased/test-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Disallows unauthorized users from accessing the pipelines section. +merge_request: +author: +type: security diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 7a7b23d2bbb..0317d69edde 100644 --- a/lib/api/pipelines.rb +++ b/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 @@ -104,7 +104,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) @@ -119,7 +119,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 diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 80506249ea9..fa732437fc1 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -3,9 +3,14 @@ require 'spec_helper' describe Projects::PipelineSchedulesController do include AccessMatchersForController + set(:user) { create(:user) } set(:project) { create(:project, :public, :repository) } set(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } + before do + project.add_developer(user) + end + describe 'GET #index' do render_views @@ -14,6 +19,10 @@ describe Projects::PipelineSchedulesController do create(:ci_pipeline_schedule, :inactive, project: project) end + before do + sign_in(user) + end + it 'renders the index view' do visit_pipelines_schedules @@ -21,7 +30,7 @@ describe Projects::PipelineSchedulesController do expect(response).to render_template(:index) end - it 'avoids N + 1 queries' do + it 'avoids N + 1 queries', :request_store do control_count = ActiveRecord::QueryRecorder.new { visit_pipelines_schedules }.count create_list(:ci_pipeline_schedule, 2, project: project) diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 0bb3ef76a3b..740b28f0f46 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -5,7 +5,7 @@ describe Projects::PipelinesController do set(:user) { create(:user) } let(:project) { create(:project, :public, :repository) } - let(:feature) { ProjectFeature::DISABLED } + let(:feature) { ProjectFeature::ENABLED } before do stub_not_protect_default_branch @@ -186,6 +186,27 @@ describe Projects::PipelinesController do end end + context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + + it 'users can not see internal pipelines' do + get_pipeline_json + + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'when pipeline is external' do + let(:pipeline) { create(:ci_pipeline, source: :external, project: project) } + + it 'users can see the external pipeline' do + get_pipeline_json + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to be(pipeline.id) + end + end + end + def get_pipeline_json get :show, params: { namespace_id: project.namespace, project_id: project, id: pipeline }, format: :json end @@ -326,16 +347,14 @@ describe Projects::PipelinesController do format: :json end - context 'when builds are enabled' do - let(:feature) { ProjectFeature::ENABLED } - - it 'retries a pipeline without returning any content' do - expect(response).to have_gitlab_http_status(:no_content) - expect(build.reload).to be_retried - end + it 'retries a pipeline without returning any content' do + expect(response).to have_gitlab_http_status(:no_content) + expect(build.reload).to be_retried end context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + it 'fails to retry pipeline' do expect(response).to have_gitlab_http_status(:not_found) end @@ -355,16 +374,14 @@ describe Projects::PipelinesController do format: :json end - context 'when builds are enabled' do - let(:feature) { ProjectFeature::ENABLED } - - it 'cancels a pipeline without returning any content' do - expect(response).to have_gitlab_http_status(:no_content) - expect(pipeline.reload).to be_canceled - end + it 'cancels a pipeline without returning any content' do + expect(response).to have_gitlab_http_status(:no_content) + expect(pipeline.reload).to be_canceled end context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + it 'fails to retry pipeline' do expect(response).to have_gitlab_http_status(:not_found) end diff --git a/spec/features/security/project/internal_access_spec.rb b/spec/features/security/project/internal_access_spec.rb index 001e6c10eb2..9ee87563ecf 100644 --- a/spec/features/security/project/internal_access_spec.rb +++ b/spec/features/security/project/internal_access_spec.rb @@ -452,9 +452,9 @@ describe "Internal Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } - it { is_expected.to be_allowed_for(:guest).of(project) } - it { is_expected.to be_allowed_for(:user) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } end diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index c6618355eea..12613c39307 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -485,7 +485,7 @@ describe "Private Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:reporter).of(project) } it { is_expected.to be_denied_for(:guest).of(project) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index 3717dc13f1e..57cc0db1f38 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -272,11 +272,11 @@ describe "Public Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } - it { is_expected.to be_allowed_for(:guest).of(project) } - it { is_expected.to be_allowed_for(:user) } - it { is_expected.to be_allowed_for(:external) } - it { is_expected.to be_allowed_for(:visitor) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } end describe "GET /:project_path/environments" do diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index c2dd666f9df..10f61731206 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -354,8 +354,20 @@ describe ProjectsHelper do allow(project).to receive(:builds_enabled?).and_return(false) end - it "do not include pipelines tab" do - is_expected.not_to include(:pipelines) + context 'when user has access to builds' do + it "does include pipelines tab" do + is_expected.to include(:pipelines) + end + end + + context 'when user does not have access to builds' do + before do + allow(helper).to receive(:can?) { false } + end + + it "does not include pipelines tab" do + is_expected.not_to include(:pipelines) + end end end diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 9b0da882390..6084dc96410 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -12,7 +12,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ] RSpec::Mocks.with_temporary_scope do - @project = create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') + @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @shared = @project.import_export_shared allow(@shared).to receive(:export_path).and_return('spec/lib/gitlab/import_export/') @@ -40,7 +40,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do project = Project.find_by_path('project') expect(project.project_feature.issues_access_level).to eq(ProjectFeature::DISABLED) - expect(project.project_feature.builds_access_level).to eq(ProjectFeature::DISABLED) + expect(project.project_feature.builds_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.snippets_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.wiki_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.merge_requests_access_level).to eq(ProjectFeature::ENABLED) diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index a2d2d77746d..baad8352185 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -11,6 +11,7 @@ describe Commit do it { is_expected.to include_module(Participable) } it { is_expected.to include_module(Referable) } it { is_expected.to include_module(StaticModel) } + it { is_expected.to include_module(Presentable) } end describe '.lazy' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 585dfe46189..8a45f9856bc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -405,6 +405,30 @@ describe Project do end end + describe '#all_pipelines' do + let(:project) { create(:project) } + + before do + create(:ci_pipeline, project: project, ref: 'master', source: :web) + create(:ci_pipeline, project: project, ref: 'master', source: :external) + end + + it 'has all pipelines' do + expect(project.all_pipelines.size).to eq(2) + end + + context 'when builds are disabled' do + before do + project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED) + end + + it 'should return .external pipelines' do + expect(project.all_pipelines).to all(have_attributes(source: 'external')) + expect(project.all_pipelines.size).to eq(1) + end + end + end + describe 'project token' do it 'sets an random token if none provided' do project = FactoryBot.create(:project, runners_token: '') diff --git a/spec/policies/ci/pipeline_policy_spec.rb b/spec/policies/ci/pipeline_policy_spec.rb index 8022f61e67d..844d96017de 100644 --- a/spec/policies/ci/pipeline_policy_spec.rb +++ b/spec/policies/ci/pipeline_policy_spec.rb @@ -75,6 +75,14 @@ describe Ci::PipelinePolicy, :models do end end + context 'when user does not have access to internal CI' do + let(:project) { create(:project, :builds_disabled, :public) } + + it 'disallows the user from reading the pipeline' do + expect(policy).to be_disallowed :read_pipeline + end + end + describe 'destroy_pipeline' do let(:project) { create(:project, :public) } diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 6c854bab5a5..49226a01846 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -175,21 +175,41 @@ describe ProjectPolicy do end context 'builds feature' do - subject { described_class.new(owner, project) } + context 'when builds are disabled' do + subject { described_class.new(owner, project) } - it 'disallows all permissions when the feature is disabled' do - project.project_feature.update(builds_access_level: ProjectFeature::DISABLED) + before do + project.project_feature.update(builds_access_level: ProjectFeature::DISABLED) + end - builds_permissions = [ - :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline, - :create_build, :read_build, :update_build, :admin_build, :destroy_build, - :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, - :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, - :create_cluster, :read_cluster, :update_cluster, :admin_cluster, - :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment - ] + it 'disallows all permissions except pipeline when the feature is disabled' do + builds_permissions = [ + :create_build, :read_build, :update_build, :admin_build, :destroy_build, + :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, + :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, + :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster, + :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment + ] + + expect_disallowed(*builds_permissions) + end + end + + context 'when builds are disabled only for some users' do + subject { described_class.new(guest, project) } - expect_disallowed(*builds_permissions) + before do + project.project_feature.update(builds_access_level: ProjectFeature::PRIVATE) + end + + it 'disallows pipeline and commit_status permissions' do + builds_permissions = [ + :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline, + :create_commit_status, :update_commit_status, :admin_commit_status, :destroy_commit_status + ] + + expect_disallowed(*builds_permissions) + end end end diff --git a/spec/presenters/commit_presenter_spec.rb b/spec/presenters/commit_presenter_spec.rb new file mode 100644 index 00000000000..4a0d3a28c32 --- /dev/null +++ b/spec/presenters/commit_presenter_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CommitPresenter do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit } + let(:user) { create(:user) } + let(:presenter) { described_class.new(commit, current_user: user) } + + describe '#status_for' do + subject { presenter.status_for('ref') } + + context 'when user can read_commit_status' do + before do + allow(presenter).to receive(:can?).with(user, :read_commit_status, project).and_return(true) + end + + it 'returns commit status for ref' do + expect(commit).to receive(:status).with('ref').and_return('test') + + expect(subject).to eq('test') + end + end + + context 'when user can not read_commit_status' do + it 'is false' do + is_expected.to eq(false) + end + end + end + + describe '#any_pipelines?' do + subject { presenter.any_pipelines? } + + context 'when user can read pipeline' do + before do + allow(presenter).to receive(:can?).with(user, :read_pipeline, project).and_return(true) + end + + it 'returns if there are any pipelines for commit' do + expect(commit).to receive_message_chain(:pipelines, :any?).and_return(true) + + expect(subject).to eq(true) + end + end + + context 'when user can not read pipeline' do + it 'is false' do + is_expected.to eq(false) + end + end + end +end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 561421d5ac8..376698a16df 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -31,23 +31,40 @@ describe MergeRequestWidgetEntity do describe 'pipeline' do let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: resource.source_branch, sha: resource.source_branch_sha, head_pipeline_of: resource) } - context 'when is up to date' do - let(:req) { double('request', current_user: user, project: project) } + before do + allow_any_instance_of(MergeRequestPresenter).to receive(:can?).and_call_original + allow_any_instance_of(MergeRequestPresenter).to receive(:can?).with(user, :read_pipeline, anything).and_return(result) + end - it 'returns pipeline' do - pipeline_payload = PipelineDetailsEntity - .represent(pipeline, request: req) - .as_json + context 'when user has access to pipelines' do + let(:result) { true } + + context 'when is up to date' do + let(:req) { double('request', current_user: user, project: project) } + + it 'returns pipeline' do + pipeline_payload = PipelineDetailsEntity + .represent(pipeline, request: req) + .as_json + + expect(subject[:pipeline]).to eq(pipeline_payload) + end + end + + context 'when is not up to date' do + it 'returns nil' do + pipeline.update(sha: "not up to date") - expect(subject[:pipeline]).to eq(pipeline_payload) + expect(subject[:pipeline]).to eq(nil) + end end end - context 'when is not up to date' do - it 'returns nil' do - pipeline.update(sha: "not up to date") + context 'when user does not have access to pipelines' do + let(:result) { false } - expect(subject[:pipeline]).to be_nil + it 'does not have pipeline' do + expect(subject[:pipeline]).to eq(nil) end end end diff --git a/spec/views/projects/commit/_commit_box.html.haml_spec.rb b/spec/views/projects/commit/_commit_box.html.haml_spec.rb index 2fdd28a3be4..1086546c10d 100644 --- a/spec/views/projects/commit/_commit_box.html.haml_spec.rb +++ b/spec/views/projects/commit/_commit_box.html.haml_spec.rb @@ -9,6 +9,7 @@ describe 'projects/commit/_commit_box.html.haml' do assign(:commit, project.commit) allow(view).to receive(:current_user).and_return(user) allow(view).to receive(:can_collaborate_with_project?).and_return(false) + project.add_developer(user) end it 'shows the commit SHA' do @@ -48,7 +49,6 @@ describe 'projects/commit/_commit_box.html.haml' do context 'viewing a commit' do context 'as a developer' do before do - project.add_developer(user) allow(view).to receive(:can_collaborate_with_project?).and_return(true) end @@ -60,6 +60,10 @@ describe 'projects/commit/_commit_box.html.haml' do end context 'as a non-developer' do + before do + project.add_guest(user) + end + it 'does not have a link to create a new tag' do render diff --git a/spec/views/projects/issues/_related_branches.html.haml_spec.rb b/spec/views/projects/issues/_related_branches.html.haml_spec.rb index 8c845251765..5cff7694029 100644 --- a/spec/views/projects/issues/_related_branches.html.haml_spec.rb +++ b/spec/views/projects/issues/_related_branches.html.haml_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe 'projects/issues/_related_branches' do include Devise::Test::ControllerHelpers + let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:branch) { project.repository.find_branch('feature') } let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.dereferenced_target.id, ref: 'feature') } @@ -11,6 +12,9 @@ describe 'projects/issues/_related_branches' do assign(:project, project) assign(:related_branches, ['feature']) + project.add_developer(user) + allow(view).to receive(:current_user).and_return(user) + render end -- cgit v1.2.1