From debb36496b4805beae28262fbb24a692018178e2 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 25 Oct 2019 07:46:40 -0500 Subject: [PATCH] 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. --- app/models/note.rb | 15 ++++- ...er-related-branches-from-activity-feed.yml | 6 ++ .../projects/issues_controller_spec.rb | 37 +++++++++++ spec/models/note_spec.rb | 64 +++++++++++++++++++ 4 files changed, 121 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml --- a/app/models/note.rb +++ b/app/models/note.rb @@ -40,6 +40,10 @@ 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-ce/merge_requests/10392/diffs#note_28719102 alias_attribute :last_edited_at, :updated_at @@ -333,7 +337,7 @@ 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? @@ -485,6 +489,15 @@ 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 --- /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 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1343,6 +1343,43 @@ 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 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -246,6 +246,70 @@ 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) } }