Merge branch 'security-hide_merge_request_ids_on_emails' into 'master'

Prevent disclosure of merge request id via email

See merge request gitlab/gitlabhq!3313

2 files changed, 78 insertions, 18 deletions

diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb
index d25f0c6de4a..a14ae2cde4b 100644
--- a/spec/helpers/emails_helper_spec.rb
+++ b/spec/helpers/emails_helper_spec.rb
@@ -6,30 +6,62 @@ describe EmailsHelper do
       let(:merge_request) { create(:merge_request) }
       let(:merge_request_presenter) { merge_request.present }
 
-      context "and format is text" do
-        it "returns plain text" do
-          expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+      context 'when user can read merge request' do
+        let(:user) { create(:user) }
+
+        before do
+          merge_request.project.add_developer(user)
+          self.instance_variable_set(:@recipient, user)
+          self.instance_variable_set(:@project, merge_request.project)
+        end
+
+        context "and format is text" do
+          it "returns plain text" do
+            expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+          end
         end
-      end
 
-      context "and format is HTML" do
-        it "returns HTML" do
-          expect(closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}")
+        context "and format is HTML" do
+          it "returns HTML" do
+            expect(helper.closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}")
+          end
+        end
+
+        context "and format is unknown" do
+          it "returns plain text" do
+            expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+          end
         end
       end
 
-      context "and format is unknown" do
-        it "returns plain text" do
-          expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+      context 'when user cannot read merge request' do
+        it "does not have link to merge request" do
+          expect(helper.closure_reason_text(merge_request)).to be_empty
         end
       end
     end
 
     context 'when given a String' do
+      let(:user) { create(:user) }
+      let(:project) { create(:project) }
       let(:closed_via) { "5a0eb6fd7e0f133044378c662fcbbc0d0c16dbfa" }
 
-      it "returns plain text" do
-        expect(closure_reason_text(closed_via)).to eq("via #{closed_via}")
+      context 'when user can read commits' do
+        before do
+          project.add_developer(user)
+          self.instance_variable_set(:@recipient, user)
+          self.instance_variable_set(:@project, project)
+        end
+
+        it "returns plain text" do
+          expect(closure_reason_text(closed_via)).to eq("via #{closed_via}")
+        end
+      end
+
+      context 'when user cannot read commits' do
+        it "returns plain text" do
+          expect(closure_reason_text(closed_via)).to be_empty
+        end
       end
     end
 
diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
index 6874a8a0929..642a49d57d5 100644
--- a/spec/services/issues/close_service_spec.rb
+++ b/spec/services/issues/close_service_spec.rb
@@ -60,35 +60,63 @@ describe Issues::CloseService do
 
   describe '#close_issue' do
     context "closed by a merge request" do
-      before do
+      it 'mentions closure via a merge request' do
         perform_enqueued_jobs do
           described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request)
         end
-      end
 
-      it 'mentions closure via a merge request' do
         email = ActionMailer::Base.deliveries.last
 
         expect(email.to.first).to eq(user2.email)
         expect(email.subject).to include(issue.title)
         expect(email.body.parts.map(&:body)).to all(include(closing_merge_request.to_reference))
       end
+
+      context 'when user cannot read merge request' do
+        it 'does not mention merge request' do
+          project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
+          perform_enqueued_jobs do
+            described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request)
+          end
+
+          email = ActionMailer::Base.deliveries.last
+          body_text = email.body.parts.map(&:body).join(" ")
+
+          expect(email.to.first).to eq(user2.email)
+          expect(email.subject).to include(issue.title)
+          expect(body_text).not_to include(closing_merge_request.to_reference)
+        end
+      end
     end
 
     context "closed by a commit" do
-      before do
+      it 'mentions closure via a commit' do
         perform_enqueued_jobs do
           described_class.new(project, user).close_issue(issue, closed_via: closing_commit)
         end
-      end
 
-      it 'mentions closure via a commit' do
         email = ActionMailer::Base.deliveries.last
 
         expect(email.to.first).to eq(user2.email)
         expect(email.subject).to include(issue.title)
         expect(email.body.parts.map(&:body)).to all(include(closing_commit.id))
       end
+
+      context 'when user cannot read the commit' do
+        it 'does not mention the commit id' do
+          project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
+          perform_enqueued_jobs do
+            described_class.new(project, user).close_issue(issue, closed_via: closing_commit)
+          end
+
+          email = ActionMailer::Base.deliveries.last
+          body_text = email.body.parts.map(&:body).join(" ")
+
+          expect(email.to.first).to eq(user2.email)
+          expect(email.subject).to include(issue.title)
+          expect(body_text).not_to include(closing_commit.id)
+        end
+      end
     end
 
     context "valid params" do