Commit 6061c9fa authored by Lin Jen-Shin's avatar Lin Jen-Shin

Send only to users have :read_build access, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6342#note_17193335
parent 1cdad622
...@@ -5,7 +5,7 @@ module Ci ...@@ -5,7 +5,7 @@ module Ci
# If we can't read build we should also not have that # If we can't read build we should also not have that
# ability when looking at this in context of commit_status # ability when looking at this in context of commit_status
%w(read create update admin).each do |rule| %w[read create update admin].each do |rule|
cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build" cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build"
end end
end end
......
module Ci
class PipelinePolicy < BuildPolicy
end
end
...@@ -489,9 +489,14 @@ class NotificationService ...@@ -489,9 +489,14 @@ class NotificationService
end end
def reject_users_without_access(recipients, target) def reject_users_without_access(recipients, target)
return recipients unless target.is_a?(Issuable) ability = case target
when Issuable
:"read_#{target.to_ability_name}"
when Ci::Pipeline
:read_build # We have build trace in pipeline emails
end
ability = :"read_#{target.to_ability_name}" return recipients unless ability
recipients.select do |user| recipients.select do |user|
user.can?(ability, target) user.can?(ability, target)
......
...@@ -17,6 +17,11 @@ describe PipelineNotificationWorker do ...@@ -17,6 +17,11 @@ describe PipelineNotificationWorker do
describe '#execute' do describe '#execute' do
before do before do
reset_delivered_emails! reset_delivered_emails!
pipeline.project.team << [pusher, Gitlab::Access::DEVELOPER]
end
context 'when watcher has developer access' do
before do
pipeline.project.team << [watcher, Gitlab::Access::DEVELOPER] pipeline.project.team << [watcher, Gitlab::Access::DEVELOPER]
end end
...@@ -28,7 +33,7 @@ describe PipelineNotificationWorker do ...@@ -28,7 +33,7 @@ describe PipelineNotificationWorker do
emails = ActionMailer::Base.deliveries emails = ActionMailer::Base.deliveries
actual = emails.flat_map(&:bcc).sort actual = emails.flat_map(&:bcc).sort
expected_receivers = [pusher, watcher].map(&:email).uniq.sort expected_receivers = receivers.map(&:email).uniq.sort
expect(actual).to eq(expected_receivers) expect(actual).to eq(expected_receivers)
expect(emails.size).to eq(1) expect(emails.size).to eq(1)
...@@ -39,15 +44,15 @@ describe PipelineNotificationWorker do ...@@ -39,15 +44,15 @@ describe PipelineNotificationWorker do
context 'with success pipeline' do context 'with success pipeline' do
let(:status) { 'success' } let(:status) { 'success' }
let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" }
let(:receivers) { [pusher, watcher] }
it_behaves_like 'sending emails' it_behaves_like 'sending emails'
context 'with pipeline from someone else' do context 'with pipeline from someone else' do
let(:pusher) { create(:user) } let(:pusher) { create(:user) }
context 'with success pipeline notification on' do
let(:watcher) { user } let(:watcher) { user }
context 'with success pipeline notification on' do
before do before do
watcher.global_notification_setting. watcher.global_notification_setting.
update(level: 'custom', success_pipeline: true) update(level: 'custom', success_pipeline: true)
...@@ -57,6 +62,8 @@ describe PipelineNotificationWorker do ...@@ -57,6 +62,8 @@ describe PipelineNotificationWorker do
end end
context 'with success pipeline notification off' do context 'with success pipeline notification off' do
let(:receivers) { [pusher] }
before do before do
watcher.global_notification_setting. watcher.global_notification_setting.
update(level: 'custom', success_pipeline: false) update(level: 'custom', success_pipeline: false)
...@@ -65,7 +72,6 @@ describe PipelineNotificationWorker do ...@@ -65,7 +72,6 @@ describe PipelineNotificationWorker do
it_behaves_like 'sending emails' it_behaves_like 'sending emails'
end end
end end
end
context 'with failed pipeline' do context 'with failed pipeline' do
let(:status) { 'failed' } let(:status) { 'failed' }
...@@ -75,10 +81,9 @@ describe PipelineNotificationWorker do ...@@ -75,10 +81,9 @@ describe PipelineNotificationWorker do
context 'with pipeline from someone else' do context 'with pipeline from someone else' do
let(:pusher) { create(:user) } let(:pusher) { create(:user) }
context 'with failed pipeline notification on' do
let(:watcher) { user } let(:watcher) { user }
context 'with failed pipeline notification on' do
before do before do
watcher.global_notification_setting. watcher.global_notification_setting.
update(level: 'custom', failed_pipeline: true) update(level: 'custom', failed_pipeline: true)
...@@ -88,6 +93,8 @@ describe PipelineNotificationWorker do ...@@ -88,6 +93,8 @@ describe PipelineNotificationWorker do
end end
context 'with failed pipeline notification off' do context 'with failed pipeline notification off' do
let(:receivers) { [pusher] }
before do before do
watcher.global_notification_setting. watcher.global_notification_setting.
update(level: 'custom', failed_pipeline: false) update(level: 'custom', failed_pipeline: false)
...@@ -98,4 +105,27 @@ describe PipelineNotificationWorker do ...@@ -98,4 +105,27 @@ describe PipelineNotificationWorker do
end end
end end
end end
end
context 'when watcher has no read_build access' do
let(:status) { 'failed' }
let(:email_subject) { "Pipeline ##{pipeline.id} has failed" }
let(:watcher) { create(:user) }
before do
pipeline.project.team << [watcher, Gitlab::Access::GUEST]
watcher.global_notification_setting.
update(level: 'custom', failed_pipeline: true)
perform_enqueued_jobs do
subject.perform(pipeline.id)
end
end
it 'does not send emails' do
should_only_email(pusher, kind: :bcc)
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment