diff options
author | Kerri Miller <kerrizor@kerrizor.com> | 2019-10-25 07:46:40 -0500 |
---|---|---|
committer | Kerri Miller <kerrizor@kerrizor.com> | 2019-11-20 07:39:50 -0800 |
commit | debb36496b4805beae28262fbb24a692018178e2 (patch) | |
tree | edfda5416822ccba914b2b6a1db627a6832f6655 | |
parent | 9d3adee84c62861b87b7891d15005d4a950d9c5a (diff) | |
download | gitlab-ce-debb36496b4805beae28262fbb24a692018178e2.tar.gz |
Restrict branches visible to guests in Issue feed
Notes related to branch creation should not be shown in an issue's
activity feed when the user doesn't have access to :download_code.
-rw-r--r-- | app/models/note.rb | 15 | ||||
-rw-r--r-- | changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml | 6 | ||||
-rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 37 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 64 |
4 files changed, 121 insertions, 1 deletions
diff --git a/app/models/note.rb b/app/models/note.rb index a0c5414aede..8bb4df9a20b 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,6 +37,10 @@ class Note < ApplicationRecord redact_field :note + TYPES_RESTRICTED_BY_ABILITY = { + branch: :download_code + }.freeze + # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes. # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102 alias_attribute :last_edited_at, :updated_at @@ -330,7 +334,7 @@ class Note < ApplicationRecord end def visible_for?(user) - !cross_reference_not_visible_for?(user) + !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user) end def award_emoji? @@ -483,6 +487,15 @@ class Note < ApplicationRecord private + def system_note_viewable_by?(user) + return true unless system_note_metadata + + restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym] + return Ability.allowed?(user, restriction, project) if restriction + + true + end + def keep_around_commit project.repository.keep_around(self.commit_id) end diff --git a/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml b/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml new file mode 100644 index 00000000000..78d87ef37a5 --- /dev/null +++ b/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml @@ -0,0 +1,6 @@ +--- +title: Remove notes regarding Related Branches from Issue activity feeds for guest + users +merge_request: +author: +type: security diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index ff41f46fc3c..b258ec96fbc 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1380,6 +1380,43 @@ describe Projects::IssuesController do expect { get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } }.not_to exceed_query_limit(control_count) end end + + context 'private project' do + let!(:branch_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) } + let!(:commit_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) } + let!(:branch_note_meta) { create(:system_note_metadata, note: branch_note, action: "branch") } + let!(:commit_note_meta) { create(:system_note_metadata, note: commit_note, action: "commit") } + + context 'user is allowed access' do + before do + project.add_user(user, :maintainer) + end + + it 'displays all available notes' do + get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } + + expect(json_response.length).to eq(3) + end + end + + context 'user is a guest' do + let(:json_response_note_ids) do + json_response.collect { |discussion| discussion["notes"] }.flatten + .collect { |note| note["id"].to_i } + end + + before do + project.add_guest(user) + end + + it 'does not display notes w/type listed in TYPES_RESTRICTED_BY_ACCESS_LEVEL' do + get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } + + expect(json_response.length).to eq(2) + expect(json_response_note_ids).not_to include(branch_note.id) + end + end + end end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 7c6de1e1e06..d69048ccc76 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -252,6 +252,70 @@ describe Note do end end + describe "#visible_for?" do + using RSpec::Parameterized::TableSyntax + + let(:note) { create(:note) } + let(:user) { create(:user) } + + where(:cross_reference_visible, :system_note_viewable, :result) do + true | true | false + false | true | true + false | false | false + end + + with_them do + it "returns expected result" do + expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible) + + unless cross_reference_visible + expect(note).to receive(:system_note_viewable_by?) + .with(user).and_return(system_note_viewable) + end + + expect(note.visible_for?(user)).to eq result + end + end + end + + describe "#system_note_viewable_by?(user)" do + let(:note) { create(:note) } + let(:user) { create(:user) } + let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") } + + context "when system_note_metadata is not present" do + it "returns true" do + expect(note).to receive(:system_note_metadata).and_return(nil) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + + context "system_note_metadata isn't of type 'branch'" do + before do + metadata.action = "not_a_branch" + end + + it "returns true" do + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + + context "user doesn't have :download_code ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :download_code ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + end + describe "cross_reference_not_visible_for?" do let(:private_user) { create(:user) } let(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } } |