From 617f43c74b967a085f6cd7afb1408cfa28187b52 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 13 Oct 2016 09:38:03 +0200 Subject: [PATCH 1/2] Guests can read builds if those are public Fixes #18448 --- app/policies/ci/build_policy.rb | 2 + app/policies/project_policy.rb | 5 ++ .../zj-guest-reads-public-builds.yml | 4 ++ .../projects/guest_navigation_menu_spec.rb | 4 +- .../security/project/private_access_spec.rb | 52 +++++++++++++++++++ spec/policies/project_policy_spec.rb | 36 ++++++++++--- spec/requests/api/builds_spec.rb | 2 +- 7 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/zj-guest-reads-public-builds.yml diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 8b25332b73c..7b1752df0e1 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -1,6 +1,8 @@ module Ci class BuildPolicy < CommitStatusPolicy def rules + can! :read_build if @subject.project.public_builds? + super # If we can't read build we should also not have that diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 8ac4bd9df6d..b4c1fcabefd 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -46,6 +46,11 @@ class ProjectPolicy < BasePolicy can! :create_note can! :upload_file can! :read_cycle_analytics + + if project.public_builds? + can! :read_pipeline + can! :read_build + end end def reporter_access! diff --git a/changelogs/unreleased/zj-guest-reads-public-builds.yml b/changelogs/unreleased/zj-guest-reads-public-builds.yml new file mode 100644 index 00000000000..1859addd606 --- /dev/null +++ b/changelogs/unreleased/zj-guest-reads-public-builds.yml @@ -0,0 +1,4 @@ +--- +title: Guests can read builds when public +merge_request: 6842 +author: diff --git a/spec/features/projects/guest_navigation_menu_spec.rb b/spec/features/projects/guest_navigation_menu_spec.rb index c22441f8929..8120a51c515 100644 --- a/spec/features/projects/guest_navigation_menu_spec.rb +++ b/spec/features/projects/guest_navigation_menu_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe "Guest navigation menu" do - let(:project) { create :empty_project, :private } - let(:guest) { create :user } + let(:project) { create(:empty_project, :private, public_builds: false) } + let(:guest) { create(:user) } before do project.team << [guest, :guest] diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index 290ddb4c6dd..a942a1ace3b 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -260,6 +260,19 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } + + context 'when public builds is enabled' do + it { is_expected.to be_allowed_for guest } + end + + context 'when public buils are disabled' do + before do + project.public_builds = false + project.save + end + + it { is_expected.to be_denied_for guest } + end end describe "GET /:project_path/pipelines/:id" do @@ -275,6 +288,19 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } + + context 'when public builds is enabled' do + it { is_expected.to be_allowed_for guest } + end + + context 'when public buils are disabled' do + before do + project.public_builds = false + project.save + end + + it { is_expected.to be_denied_for guest } + end end describe "GET /:project_path/builds" do @@ -289,6 +315,19 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } + + context 'when public builds is enabled' do + it { is_expected.to be_allowed_for guest } + end + + context 'when public buils are disabled' do + before do + project.public_builds = false + project.save + end + + it { is_expected.to be_denied_for guest } + end end describe "GET /:project_path/builds/:id" do @@ -305,6 +344,19 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } + + context 'when public builds is enabled' do + it { is_expected.to be_allowed_for guest } + end + + context 'when public buils are disabled' do + before do + project.public_builds = false + project.save + end + + it { is_expected.to be_denied_for guest } + end end describe "GET /:project_path/environments" do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index b49e4f3a8bc..34a0937d9bc 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -111,13 +111,35 @@ describe ProjectPolicy, models: true do context 'guests' do let(:current_user) { guest } - it do - is_expected.to include(*guest_permissions) - is_expected.not_to include(*reporter_permissions) - is_expected.not_to include(*team_member_reporter_permissions) - is_expected.not_to include(*developer_permissions) - is_expected.not_to include(*master_permissions) - is_expected.not_to include(*owner_permissions) + context 'public builds enabled' do + let(:reporter_public_build_permissions) do + reporter_permissions - [:read_build, :read_pipeline] + end + + it do + is_expected.to include(*guest_permissions) + is_expected.not_to include(*reporter_public_build_permissions) + is_expected.not_to include(*team_member_reporter_permissions) + is_expected.not_to include(*developer_permissions) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + end + + context 'public builds disabled' do + before do + project.public_builds = false + project.save + end + + it do + is_expected.to include(*guest_permissions) + is_expected.not_to include(*reporter_permissions) + is_expected.not_to include(*team_member_reporter_permissions) + is_expected.not_to include(*developer_permissions) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end end end diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 0ea991b18b8..7be7acebb19 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -5,7 +5,7 @@ describe API::Builds, api: true do let(:user) { create(:user) } let(:api_user) { user } - let!(:project) { create(:project, creator_id: user.id) } + let!(:project) { create(:project, creator_id: user.id, public_builds: false) } let!(:developer) { create(:project_member, :developer, user: user, project: project) } let(:reporter) { create(:project_member, :reporter, project: project) } let(:guest) { create(:project_member, :guest, project: project) } From 10960400245ca338e32a3c55538ace976df962c6 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 29 Nov 2016 13:43:58 +0100 Subject: [PATCH 2/2] Update effected tests --- app/policies/project_policy.rb | 3 -- features/steps/shared/project.rb | 2 +- .../security/project/private_access_spec.rb | 49 ++++++++++--------- .../cycle_analytics/permissions_spec.rb | 2 +- spec/policies/project_policy_spec.rb | 32 ++++++------ .../projects/cycle_analytics_events_spec.rb | 2 +- .../pipeline_notification_worker_spec.rb | 2 +- 7 files changed, 45 insertions(+), 47 deletions(-) diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index b4c1fcabefd..d5aadfce76a 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -12,9 +12,6 @@ class ProjectPolicy < BasePolicy guest_access! public_access! - # Allow to read builds for internal projects - can! :read_build if project.public_builds? - if project.request_access_enabled && !(owner || user.admin? || project.team.member?(user) || project_group_member?(user)) can! :request_access diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index cab85a48396..b51152c79c6 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -9,7 +9,7 @@ module SharedProject step "project exists in some group namespace" do @group = create(:group, name: 'some group') - @project = create(:project, namespace: @group) + @project = create(:project, namespace: @group, public_builds: false) end # Create a specific project called "Shop" diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index a942a1ace3b..f52e23f9433 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe "Private Project Access", feature: true do include AccessMatchers - let(:project) { create(:project, :private) } + let(:project) { create(:project, :private, public_builds: false) } describe "Project should be private" do describe '#private?' do @@ -262,16 +262,15 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:visitor) } context 'when public builds is enabled' do - it { is_expected.to be_allowed_for guest } + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:guest).of(project) } end context 'when public buils are disabled' do - before do - project.public_builds = false - project.save - end - - it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for(:guest).of(project) } end end @@ -290,16 +289,15 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:visitor) } context 'when public builds is enabled' do - it { is_expected.to be_allowed_for guest } + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:guest).of(project) } end context 'when public buils are disabled' do - before do - project.public_builds = false - project.save - end - - it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for(:guest).of(project) } end end @@ -317,16 +315,15 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:visitor) } context 'when public builds is enabled' do - it { is_expected.to be_allowed_for guest } + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:guest).of(project) } end context 'when public buils are disabled' do - before do - project.public_builds = false - project.save - end - - it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for(:guest).of(project) } end end @@ -346,7 +343,11 @@ describe "Private Project Access", feature: true do it { is_expected.to be_denied_for(:visitor) } context 'when public builds is enabled' do - it { is_expected.to be_allowed_for guest } + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:guest).of(project) } end context 'when public buils are disabled' do @@ -355,7 +356,7 @@ describe "Private Project Access", feature: true do project.save end - it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for(:guest).of(project) } end end diff --git a/spec/lib/gitlab/cycle_analytics/permissions_spec.rb b/spec/lib/gitlab/cycle_analytics/permissions_spec.rb index dc4f7dc69db..2d85e712db0 100644 --- a/spec/lib/gitlab/cycle_analytics/permissions_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/permissions_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::CycleAnalytics::Permissions do - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, public_builds: false) } let(:user) { create(:user) } subject { described_class.get(user: user, project: project) } diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 34a0937d9bc..eeab9827d99 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -111,34 +111,34 @@ describe ProjectPolicy, models: true do context 'guests' do let(:current_user) { guest } - context 'public builds enabled' do - let(:reporter_public_build_permissions) do - reporter_permissions - [:read_build, :read_pipeline] - end + let(:reporter_public_build_permissions) do + reporter_permissions - [:read_build, :read_pipeline] + end + it do + is_expected.to include(*guest_permissions) + is_expected.not_to include(*reporter_public_build_permissions) + is_expected.not_to include(*team_member_reporter_permissions) + is_expected.not_to include(*developer_permissions) + is_expected.not_to include(*master_permissions) + is_expected.not_to include(*owner_permissions) + end + + context 'public builds enabled' do it do is_expected.to include(*guest_permissions) - is_expected.not_to include(*reporter_public_build_permissions) - is_expected.not_to include(*team_member_reporter_permissions) - is_expected.not_to include(*developer_permissions) - is_expected.not_to include(*master_permissions) - is_expected.not_to include(*owner_permissions) + is_expected.to include(:read_build, :read_pipeline) end end context 'public builds disabled' do before do - project.public_builds = false - project.save + project.update(public_builds: false) end it do is_expected.to include(*guest_permissions) - is_expected.not_to include(*reporter_permissions) - is_expected.not_to include(*team_member_reporter_permissions) - is_expected.not_to include(*developer_permissions) - is_expected.not_to include(*master_permissions) - is_expected.not_to include(*owner_permissions) + is_expected.not_to include(:read_build, :read_pipeline) end end end diff --git a/spec/requests/projects/cycle_analytics_events_spec.rb b/spec/requests/projects/cycle_analytics_events_spec.rb index f5e0fdcda2d..e0368e6001f 100644 --- a/spec/requests/projects/cycle_analytics_events_spec.rb +++ b/spec/requests/projects/cycle_analytics_events_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe 'cycle analytics events' do let(:user) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project, public_builds: false) } let(:issue) { create(:issue, project: project, created_at: 2.days.ago) } describe 'GET /:namespace/:project/cycle_analytics/events/issues' do diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb index 739f9b63967..603ae52ed1e 100644 --- a/spec/workers/pipeline_notification_worker_spec.rb +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -11,7 +11,7 @@ describe PipelineNotificationWorker do status: status) end - let(:project) { create(:project) } + let(:project) { create(:project, public_builds: false) } let(:user) { create(:user) } let(:pusher) { user } let(:watcher) { pusher }