diff options
| -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 ce60413b8a0..493132e30cc 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 @@ -341,7 +345,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? @@ -493,6 +497,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 8770a5ee303..29746bbd863 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1435,6 +1435,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 3ab88b52568..9c06cd049f5 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -285,6 +285,70 @@ describe Note do end end + describe "#visible_for?" do + using RSpec::Parameterized::TableSyntax + + let_it_be(:note) { create(:note) } + let_it_be(: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_it_be(:note) { create(:note) } + let_it_be(: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) } } |