Merge branch 'security-pb-email-watchers-no-access' into 'master'

Stop sending emails to users who can't read commit

See merge request gitlab/gitlabhq!3060

app/models/notification_recipient.rb

@@ -119,15 +119,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
 

changelogs/unreleased/security-pb-email-watchers-no-access.yml

+---
+title: Stop sending emails to users who can't read commit
+merge_request:
+author:
+type: security

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
+
+    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 }
 
-    expect(recipient.has_access?).to be_falsy
+      it 'returns true' do
+        expect(recipient.has_access?).to eq true
+      end
+    end
   end
 
   context '#notification_setting' do