diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index 793cce191fa..292e8a58028 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -123,15 +123,19 @@ class NotificationRecipient return @read_ability if instance_variable_defined?(:@read_ability) @read_ability = - case @target - when Issuable - :"read_#{@target.to_ability_name}" - when Ci::Pipeline + if @target.is_a?(Ci::Pipeline) :read_build # We have build trace in pipeline emails - when ActiveRecord::Base - :"read_#{@target.class.model_name.name.underscore}" - else - nil + elsif default_ability_for_target + :"read_#{default_ability_for_target}" + end + end + + def default_ability_for_target + @default_ability_for_target ||= + if @target.respond_to?(:to_ability_name) + @target.to_ability_name + elsif @target.class.respond_to?(:model_name) + @target.class.model_name.name.underscore end end diff --git a/changelogs/unreleased/security-pb-email-watchers-no-access.yml b/changelogs/unreleased/security-pb-email-watchers-no-access.yml new file mode 100644 index 00000000000..cc64ef1352f --- /dev/null +++ b/changelogs/unreleased/security-pb-email-watchers-no-access.yml @@ -0,0 +1,5 @@ +--- +title: Stop sending emails to users who can't read commit +merge_request: +author: +type: security diff --git a/spec/models/notification_recipient_spec.rb b/spec/models/notification_recipient_spec.rb index 3710f2be287..1b1ede6b14c 100644 --- a/spec/models/notification_recipient_spec.rb +++ b/spec/models/notification_recipient_spec.rb @@ -9,11 +9,43 @@ describe NotificationRecipient do subject(:recipient) { described_class.new(user, :watch, target: target, project: project) } - it 'denies access to a target when cross project access is denied' do - allow(Ability).to receive(:allowed?).and_call_original - expect(Ability).to receive(:allowed?).with(user, :read_cross_project, :global).and_return(false) + describe '#has_access?' do + before do + allow(user).to receive(:can?).and_call_original + end - expect(recipient.has_access?).to be_falsy + context 'user cannot read cross project' do + it 'returns false' do + expect(user).to receive(:can?).with(:read_cross_project).and_return(false) + expect(recipient.has_access?).to eq false + end + end + + context 'user cannot read build' do + let(:target) { build(:ci_pipeline) } + + it 'returns false' do + expect(user).to receive(:can?).with(:read_build, target).and_return(false) + expect(recipient.has_access?).to eq false + end + end + + context 'user cannot read commit' do + let(:target) { build(:commit) } + + it 'returns false' do + expect(user).to receive(:can?).with(:read_commit, target).and_return(false) + expect(recipient.has_access?).to eq false + end + end + + context 'target has no policy' do + let(:target) { double.as_null_object } + + it 'returns true' do + expect(recipient.has_access?).to eq true + end + end end context '#notification_setting' do