From 377b59da3029786f2265058972ad0bc6aefd8b53 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 12 Apr 2016 09:59:01 +0530
Subject: [PATCH 1/9] 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.
---
app/models/issue.rb | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/app/models/issue.rb b/app/models/issue.rb
index e064b0f8b95..68e0113380e 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -151,7 +151,11 @@ class Issue < ActiveRecord::Base
end
def to_branch_name
- "#{title.parameterize}-#{iid}"
+ if self.confidential?
+ "issue-#{iid}"
+ else
+ "#{title.parameterize}-#{iid}"
+ end
end
def can_be_worked_on?(current_user)
--
GitLab
From 5d88de092f37497d9c08878954b099c425376bda Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 12 Apr 2016 11:43:15 +0530
Subject: [PATCH 2/9] Refactor `Issue#related_branches`
- Previously, the controller held the logic to calculate
related branches, which was:
`<branches ending with `issue.iid`> - <branches with a merge request referenced in the current issue>`
- This logic belongs in the `related_branches` method, not in the
controller. This commit makes this change.
- This means that `Issue#related_branches` now needs to take a `User`.
When we find the branches that have a merge request referenced in the
current issue, this is limited to merge requests that the current user
has access to.
- This is not directly related to #14566, but is a related refactoring.
---
app/controllers/projects/issues_controller.rb | 2 +-
app/models/issue.rb | 13 ++++++++++---
spec/models/issue_spec.rb | 3 ++-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 6d649e72f84..a3842036982 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -66,7 +66,7 @@ class Projects::IssuesController < Projects::ApplicationController
@notes = @issue.notes.nonawards.with_associations.fresh
@noteable = @issue
@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.html
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 68e0113380e..252545d1147 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -104,10 +104,17 @@ 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 (that the current user can see)
+ # referencing the current issue.
+ def related_branches(current_user)
+ branches_with_iid = project.repository.branch_names.select do |branch|
branch.end_with?("-#{iid}")
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
@@ -161,7 +168,7 @@ class Issue < ActiveRecord::Base
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
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 15052aaca28..f33b705810e 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -192,10 +192,11 @@ describe Issue, models: true do
describe '#related_branches' do
it "selects the right branches" do
+ user = build(:user)
allow(subject.project.repository).to receive(:branch_names).
and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name])
- expect(subject.related_branches).to eq([subject.to_branch_name])
+ expect(subject.related_branches(user)).to eq([subject.to_branch_name])
end
end
--
GitLab
From 91034af3c998ce4a4f83281525304e8c50add384 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 12 Apr 2016 15:50:19 +0530
Subject: [PATCH 3/9] Augment the tests for `Issue#related_branches`
- Test the case where we have a referenced merge request that's being
- excluded as a "related branch"
- This took a while to figure out, especially the
`create_cross_references!` line.
---
spec/models/issue_spec.rb | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index f33b705810e..8cacce6a7bf 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -191,11 +191,27 @@ describe Issue, models: true do
end
describe '#related_branches' do
- it "selects the right branches" do
- user = build(:user)
+ let(:user) { build(:user, :admin) }
+ let(:merge_request) { create(:merge_request, description: "Closes ##{subject.iid}",
+ source_project: subject.project, source_branch: "branch-#{subject.iid}") }
+
+ before(:each) 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, "branch-#{subject.iid}"])
+
+ # 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, "branch-#{subject.iid}"])
+ end
+ it "selects the right branches when there is a referenced merge request" do
+ 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
end
--
GitLab
From 66ca80181bcb5aef97296fde567b899603186be6 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 12 Apr 2016 16:01:44 +0530
Subject: [PATCH 4/9] Test the `Issue#to_branch_name` method.
---
spec/models/issue_spec.rb | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 8cacce6a7bf..22dabaa0404 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -228,10 +228,19 @@ describe Issue, models: true do
end
describe "#to_branch_name" do
- let(:issue) { create(:issue, title: 'a' * 30) }
+ let(:issue) { create(:issue, title: 'testing-issue') }
- it "starts with the issue iid" do
+ it "ends with the issue iid" do
expect(issue.to_branch_name).to match /-#{issue.iid}\z/
end
+
+ it "contains the issue title if not confidential" do
+ expect(issue.to_branch_name).to match /\Atesting-issue/
+ 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 /\Aissue/
+ end
end
end
--
GitLab
From 8dd1a6fc917231e83fb64dbfa65e1edbe9ab3fee Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 13 Apr 2016 09:04:08 +0530
Subject: [PATCH 5/9] Fix the rubocop check.
---
spec/models/issue_spec.rb | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 22dabaa0404..202ce846dca 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -192,8 +192,6 @@ describe Issue, models: true do
describe '#related_branches' do
let(:user) { build(:user, :admin) }
- let(:merge_request) { create(:merge_request, description: "Closes ##{subject.iid}",
- source_project: subject.project, source_branch: "branch-#{subject.iid}") }
before(:each) do
allow(subject.project.repository).to receive(:branch_names).
@@ -210,6 +208,9 @@ describe Issue, models: true do
end
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: "branch-#{subject.iid}" })
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])
--
GitLab
From 769a8bf7284da492732d923cb888c0f5be16aff8 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 13 Apr 2016 09:18:29 +0530
Subject: [PATCH 6/9] Update CHANGELOG
---
CHANGELOG | 1 +
1 file changed, 1 insertion(+)
diff --git a/CHANGELOG b/CHANGELOG
index 3561c541df0..738162d983f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -34,6 +34,7 @@ v 8.7.0 (unreleased)
- API: Expose user location (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
v 8.6.5
- Fix importing from GitHub Enterprise. !3529
--
GitLab
From f5ce601c2b052b18398d2207c64ce8828b628521 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 15 Apr 2016 09:31:26 +0530
Subject: [PATCH 7/9] Make a few style changes based on MR feedback.
---
spec/models/issue_spec.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 202ce846dca..ed982d8a9d4 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -191,9 +191,9 @@ describe Issue, models: true do
end
describe '#related_branches' do
- let(:user) { build(:user, :admin) }
+ let(:user) { build(:admin) }
- before(:each) do
+ before do
allow(subject.project.repository).to receive(:branch_names).
and_return(["mpempe", "#{subject.iid}mepmep", subject.to_branch_name, "branch-#{subject.iid}"])
--
GitLab
From aa396ae5ee5715c1a2a8a82731cef6e3d7f73056 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 18 Apr 2016 13:01:23 +0530
Subject: [PATCH 8/9] Remove unused variable in `IssuesController`.
---
app/controllers/projects/issues_controller.rb | 2 --
1 file changed, 2 deletions(-)
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index f3056b21007..f2ae572cfe1 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -128,8 +128,6 @@ class Projects::IssuesController < Projects::ApplicationController
end
def related_branches
- merge_requests = @issue.referenced_merge_requests(current_user)
-
@related_branches = @issue.related_branches(current_user)
respond_to do |format|
--
GitLab
From f801e2243dcce6d3bdce01acf62fbf5a49a301da Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 19 Apr 2016 09:22:55 +0530
Subject: [PATCH 9/9] A new branch created for a confidential issue is named
`<id>-confidential-issue`.
---
app/models/issue.rb | 2 +-
spec/models/issue_spec.rb | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 530d2107596..2f773869603 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -158,7 +158,7 @@ class Issue < ActiveRecord::Base
def to_branch_name
if self.confidential?
- "issue-#{iid}"
+ "#{iid}-confidential-issue"
else
"#{iid}-#{title.parameterize}"
end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index da1c673653a..060e6599104 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -248,7 +248,7 @@ describe Issue, models: true do
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 /\Aissue/
+ expect(issue.to_branch_name).to match /confidential-issue\z/
end
end
end
--
GitLab