Merge branch '14566-confidential-issue-branches' into 'master'

Sanitize branch names for confidential issues

- When creating new branches for confidential issues, prefer a branch name like `issue-15` to `some-sensitive-issue-title-15`.
- The behaviour for non-confidential issues stays the same.

Closes #14566

See merge request !3671

CHANGELOG

@@ -77,6 +77,7 @@ v 8.7.0 (unreleased)
   - API: Do not leak group existence via return code (Robert Schilling)
   - ClosingIssueExtractor regex now also works with colons. e.g. "Fixes: #1234" !3591
   - Update number of Todos in the sidebar when it's marked as "Done". !3600
+  - Sanitize branch names created for confidential issues
   - API: Expose 'updated_at' for issue, snippet, and merge request notes (Robert Schilling)
   - API: User can leave a project through the API when not master or owner. !3613
   - Fix repository cache invalidation issue when project is recreated with an empty repo (Stan Hu)

app/controllers/projects/issues_controller.rb

@@ -128,10 +128,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def related_branches
-    merge_requests = @issue.referenced_merge_requests(current_user)
-
-    @related_branches = @issue.related_branches -
-      merge_requests.map(&:source_branch)
+    @related_branches = @issue.related_branches(current_user)
 
     respond_to do |format|
       format.json do

app/models/issue.rb

@@ -104,10 +104,16 @@ class Issue < ActiveRecord::Base
     end
   end
 
-  def related_branches
-    project.repository.branch_names.select do |branch|
+  # All branches containing the current issue's ID, except for
+  # those with a merge request open referencing the current issue.
+  def related_branches(current_user)
+    branches_with_iid = project.repository.branch_names.select do |branch|
       branch =~ /\A#{iid}-(?!\d+-stable)/i
     end
+
+    branches_with_merge_request = self.referenced_merge_requests(current_user).map(&:source_branch)
+
+    branches_with_iid - branches_with_merge_request
   end
 
   # Reset issue events cache
@@ -151,13 +157,17 @@ class Issue < ActiveRecord::Base
   end
 
   def to_branch_name
+    if self.confidential?
+      "#{iid}-confidential-issue"
+    else
       "#{iid}-#{title.parameterize}"
     end
+  end
 
   def can_be_worked_on?(current_user)
     !self.closed? &&
       !self.project.forked? &&
-      self.related_branches.empty? &&
+      self.related_branches(current_user).empty? &&
       self.closed_by_merge_requests(current_user).empty?
   end
 end

spec/models/issue_spec.rb

@@ -191,18 +191,36 @@ describe Issue, models: true do
   end
 
   describe '#related_branches' do
-    it 'selects the right branches' do
+    let(:user) { build(:admin) }
+
+    before do
       allow(subject.project.repository).to receive(:branch_names).
-        and_return(['mpempe', "#{subject.iid}mepmep", subject.to_branch_name])
+                                            and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name, "#{subject.iid}-branch"])
+
+      # Without this stub, the `create(:merge_request)` above fails because it can't find
+      # the source branch. This seems like a reasonable compromise, in comparison with
+      # setting up a full repo here.
+      allow_any_instance_of(MergeRequest).to receive(:create_merge_request_diff)
+    end
+
+    it "selects the right branches when there are no referenced merge requests" do
+      expect(subject.related_branches(user)).to eq([subject.to_branch_name, "#{subject.iid}-branch"])
+    end
 
-      expect(subject.related_branches).to eq([subject.to_branch_name])
+    it "selects the right branches when there is a referenced merge request" do
+      merge_request = create(:merge_request, { description: "Closes ##{subject.iid}",
+                                               source_project: subject.project,
+                                               source_branch: "#{subject.iid}-branch" })
+      merge_request.create_cross_references!(user)
+      expect(subject.referenced_merge_requests).to_not be_empty
+      expect(subject.related_branches(user)).to eq([subject.to_branch_name])
     end
 
     it 'excludes stable branches from the related branches' do
       allow(subject.project.repository).to receive(:branch_names).
         and_return(["#{subject.iid}-0-stable"])
 
-      expect(subject.related_branches).to eq []
+      expect(subject.related_branches(user)).to eq []
     end
   end
 
@@ -217,11 +235,20 @@ describe Issue, models: true do
     let(:subject) { create :issue }
   end
 
-  describe '#to_branch_name' do
-    let(:issue) { create(:issue, title: 'a' * 30) }
+  describe "#to_branch_name" do
+    let(:issue) { create(:issue, title: 'testing-issue') }
 
     it 'starts with the issue iid' do
-      expect(issue.to_branch_name).to match /\A#{issue.iid}-a+\z/
+      expect(issue.to_branch_name).to match /\A#{issue.iid}-[A-Za-z\-]+\z/
+    end
+
+    it "contains the issue title if not confidential" do
+      expect(issue.to_branch_name).to match /testing-issue\z/
+    end
+
+    it "does not contain the issue title if confidential" do
+      issue = create(:issue, title: 'testing-issue', confidential: true)
+      expect(issue.to_branch_name).to match /confidential-issue\z/
     end
   end
 end