Merge branch 'jej-note-search-uses-finder' into 'security'

Fix missing Note access checks in by moving Note#search to updated NoteFinder

Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867

## Which fixes are in this MR?

⚠️ - Potentially untested  
💣 - No test coverage  
🚥 - Test coverage of some sort exists (a test failed when error raised)  
🚦 - Test coverage of return value (a test failed when nil used)  
 - Permissions check tested

### Note lookup without access check

- [x]  app/finders/notes_finder.rb:13 :download_code check
- [x]  app/finders/notes_finder.rb:19 `SnippetsFinder`
- [x]  app/models/note.rb:121 [`Issue#visible_to_user`]
- [x]  lib/gitlab/project_search_results.rb:113
  - This is the only use of `app/models/note.rb:121` above, but importantly has no access checks at all. This means it leaks MR comments and snippets when those features are `team-only` in addition to the issue comments which would be fixed by `app/models/note.rb:121`.
  - It is only called from SearchController where `can?(current_user, :download_code, @project)` is checked, so commit comments are not leaked.

### Previous discussions
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_13_13 `: download_code` check on commit
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_19_19 `SnippetsFinder` should be used
  - `SnippetsFinder` should check if the snippets feature is enabled -> https://gitlab.com/gitlab-org/gitlab-ce/issues/25223

###  Acceptance criteria met?
- [x] Tests added for new code
- [x] TODO comments removed
- [x] Squashed and removed skipped tests
- [x] Changelog entry
- [ ] State Gitlab versions affected and issue severity in description
- [ ] Create technical debt issue for NotesFinder.
  - Either split into `NotesFinder::ForTarget` and `NotesFinder::Search` or consider object per notable type such as `NotesFinder::OnIssue`. For the first option could create `NotesFinder::Base` which is either inherited from or which can be included in the other two.
  - Avoid case statement anti-pattern in this finder with use of `NotesFinder::OnCommit` etc. Consider something on the finder for this? `Model.finder(user, project)`
  - Move `inc_author` to the controller, and implement `related_notes` to replace `non_diff_notes`/`mr_and_commit_notes`

See merge request !2035
This commit is contained in:
Douwe Maan 2016-12-09 01:56:31 +00:00 committed by Alejandro Rodríguez
parent 1e1887697d
commit 12db4cc0e7
16 changed files with 386 additions and 120 deletions

View File

@ -217,6 +217,6 @@ class Projects::NotesController < Projects::ApplicationController
end end
def find_current_user_notes def find_current_user_notes
@notes = NotesFinder.new.execute(project, current_user, params) @notes = NotesFinder.new(project, current_user, params).execute.inc_author
end end
end end

View File

@ -1,27 +1,102 @@
class NotesFinder class NotesFinder
FETCH_OVERLAP = 5.seconds FETCH_OVERLAP = 5.seconds
def execute(project, current_user, params) # Used to filter Notes
target_type = params[:target_type] # When used with target_type and target_id this returns notes specifically for the controller
target_id = params[:target_id] #
# Default to 0 to remain compatible with old clients # Arguments:
last_fetched_at = Time.at(params.fetch(:last_fetched_at, 0).to_i) # current_user - which user check authorizations with
# project - which project to look for notes on
# params:
# target_type: string
# target_id: integer
# last_fetched_at: time
# search: string
#
def initialize(project, current_user, params = {})
@project = project
@current_user = current_user
@params = params
init_collection
end
notes = def execute
case target_type @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at]
when "commit" @notes
project.notes.for_commit_id(target_id).non_diff_notes end
when "issue"
IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author private
when "merge_request"
MergeRequestsFinder.new(current_user, project_id: project.id).find(target_id).mr_and_commit_notes.inc_author def init_collection
when "snippet", "project_snippet" if @params[:target_id]
project.snippets.find(target_id).notes @notes = on_target(@params[:target_type], @params[:target_id])
else
@notes = notes_of_any_type
end
end
def notes_of_any_type
types = %w(commit issue merge_request snippet)
note_relations = types.map { |t| notes_for_type(t) }
note_relations.map!{ |notes| search(@params[:search], notes) } if @params[:search]
UnionFinder.new.find_union(note_relations, Note)
end
def noteables_for_type(noteable_type)
case noteable_type
when "issue"
IssuesFinder.new(@current_user, project_id: @project.id).execute
when "merge_request"
MergeRequestsFinder.new(@current_user, project_id: @project.id).execute
when "snippet", "project_snippet"
SnippetsFinder.new.execute(@current_user, filter: :by_project, project: @project)
else
raise 'invalid target_type'
end
end
def notes_for_type(noteable_type)
if noteable_type == "commit"
if Ability.allowed?(@current_user, :download_code, @project)
@project.notes.where(noteable_type: 'Commit')
else else
raise 'invalid target_type' Note.none
end end
else
finder = noteables_for_type(noteable_type)
@project.notes.where(noteable_type: finder.base_class.name, noteable_id: finder.reorder(nil))
end
end
# Use overlapping intervals to avoid worrying about race conditions def on_target(target_type, target_id)
notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh if target_type == "commit"
notes_for_type('commit').for_commit_id(target_id)
else
target = noteables_for_type(target_type).find(target_id)
if target.respond_to?(:related_notes)
target.related_notes
else
target.notes
end
end
end
# Searches for notes matching the given query.
#
# This method uses ILIKE on PostgreSQL and LIKE on MySQL.
#
def search(query, notes_relation = @notes)
pattern = "%#{query}%"
notes_relation.where(Note.arel_table[:note].matches(pattern))
end
# Notes changed since last fetch
# Uses overlapping intervals to avoid worrying about race conditions
def since_fetch_at(fetch_time)
# Default to 0 to remain compatible with old clients
last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
@notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
end end
end end

View File

@ -452,7 +452,7 @@ class MergeRequest < ActiveRecord::Base
should_remove_source_branch? || force_remove_source_branch? should_remove_source_branch? || force_remove_source_branch?
end end
def mr_and_commit_notes def related_notes
# Fetch comments only from last 100 commits # Fetch comments only from last 100 commits
commits_for_notes_limit = 100 commits_for_notes_limit = 100
commit_ids = commits.last(commits_for_notes_limit).map(&:id) commit_ids = commits.last(commits_for_notes_limit).map(&:id)
@ -468,7 +468,7 @@ class MergeRequest < ActiveRecord::Base
end end
def discussions def discussions
@discussions ||= self.mr_and_commit_notes. @discussions ||= self.related_notes.
inc_relations_for_view. inc_relations_for_view.
fresh. fresh.
discussions discussions

View File

@ -107,23 +107,6 @@ class Note < ActiveRecord::Base
Discussion.for_diff_notes(active_notes). Discussion.for_diff_notes(active_notes).
map { |d| [d.line_code, d] }.to_h map { |d| [d.line_code, d] }.to_h
end end
# Searches for notes matching the given query.
#
# This method uses ILIKE on PostgreSQL and LIKE on MySQL.
#
# query - The search query as a String.
# as_user - Limit results to those viewable by a specific user
#
# Returns an ActiveRecord::Relation.
def search(query, as_user: nil)
table = arel_table
pattern = "%#{query}%"
Note.joins('LEFT JOIN issues ON issues.id = noteable_id').
where(table[:note].matches(pattern)).
merge(Issue.visible_to_user(as_user))
end
end end
def cross_reference? def cross_reference?

View File

@ -39,7 +39,7 @@
= icon('thumbs-down') = icon('thumbs-down')
= downvotes = downvotes
- note_count = merge_request.mr_and_commit_notes.user.count - note_count = merge_request.related_notes.user.count
%li %li
= link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
= icon('comments') = icon('comments')

View File

@ -53,7 +53,7 @@
%li.notes-tab %li.notes-tab
= link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do
Discussion Discussion
%span.badge= @merge_request.mr_and_commit_notes.user.count %span.badge= @merge_request.related_notes.user.count
- if @merge_request.source_project - if @merge_request.source_project
%li.commits-tab %li.commits-tab
= link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do

View File

@ -0,0 +1,4 @@
---
title: Fix missing Note access checks by moving Note#search to updated NoteFinder
merge_request:
author:

View File

@ -110,7 +110,7 @@ module Gitlab
end end
def notes def notes
@notes ||= project.notes.user.search(query, as_user: @current_user).order('updated_at DESC') @notes ||= NotesFinder.new(project, @current_user, search: query).execute.user.order('updated_at DESC')
end end
def commits def commits

View File

@ -22,9 +22,7 @@ module Gitlab
# By using "unprepared_statements" we remove the usage of placeholders # By using "unprepared_statements" we remove the usage of placeholders
# (thus fixing this problem), at a slight performance cost. # (thus fixing this problem), at a slight performance cost.
fragments = ActiveRecord::Base.connection.unprepared_statement do fragments = ActiveRecord::Base.connection.unprepared_statement do
@relations.map do |rel| @relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?)
rel.reorder(nil).to_sql
end
end end
fragments.join("\nUNION\n") fragments.join("\nUNION\n")

View File

@ -0,0 +1,61 @@
require 'spec_helper'
describe SearchController do
let(:user) { create(:user) }
let(:project) { create(:empty_project, :public) }
before do
sign_in(user)
end
it 'finds issue comments' do
project = create(:empty_project, :public)
note = create(:note_on_issue, project: project)
get :show, project_id: project.id, scope: 'notes', search: note.note
expect(assigns[:search_objects].first).to eq note
end
context 'on restricted projects' do
context 'when signed out' do
before { sign_out(user) }
it "doesn't expose comments on issues" do
project = create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_issue, project: project)
get :show, project_id: project.id, scope: 'notes', search: note.note
expect(assigns[:search_objects].count).to eq(0)
end
end
it "doesn't expose comments on issues" do
project = create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_issue, project: project)
get :show, project_id: project.id, scope: 'notes', search: note.note
expect(assigns[:search_objects].count).to eq(0)
end
it "doesn't expose comments on merge_requests" do
project = create(:empty_project, :public, merge_requests_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_merge_request, project: project)
get :show, project_id: project.id, scope: 'notes', search: note.note
expect(assigns[:search_objects].count).to eq(0)
end
it "doesn't expose comments on snippets" do
project = create(:empty_project, :public, snippets_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_project_snippet, project: project)
get :show, project_id: project.id, scope: 'notes', search: note.note
expect(assigns[:search_objects].count).to eq(0)
end
end
end

View File

@ -67,7 +67,7 @@ FactoryGirl.define do
end end
trait :on_project_snippet do trait :on_project_snippet do
noteable { create(:snippet, project: project) } noteable { create(:project_snippet, project: project) }
end end
trait :system do trait :system do

View File

@ -2,59 +2,203 @@ require 'spec_helper'
describe NotesFinder do describe NotesFinder do
let(:user) { create :user } let(:user) { create :user }
let(:project) { create :project } let(:project) { create(:empty_project) }
let(:note1) { create :note_on_commit, project: project }
let(:note2) { create :note_on_commit, project: project }
let(:commit) { note1.noteable }
before do before do
project.team << [user, :master] project.team << [user, :master]
end end
describe '#execute' do describe '#execute' do
let(:params) { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } } it 'finds notes on snippets when project is public and user isnt a member'
before do it 'finds notes on merge requests' do
note1 create(:note_on_merge_request, project: project)
note2
notes = described_class.new(project, user).execute
expect(notes.count).to eq(1)
end end
it 'finds all notes' do it 'finds notes on snippets' do
notes = NotesFinder.new.execute(project, user, params) create(:note_on_project_snippet, project: project)
expect(notes.size).to eq(2)
notes = described_class.new(project, user).execute
expect(notes.count).to eq(1)
end end
it 'raises an exception for an invalid target_type' do it "excludes notes on commits the author can't download" do
params.merge!(target_type: 'invalid') project = create(:project, :private)
expect { NotesFinder.new.execute(project, user, params) }.to raise_error('invalid target_type') note = create(:note_on_commit, project: project)
params = { target_type: 'commit', target_id: note.noteable.id }
notes = described_class.new(project, create(:user), params).execute
expect(notes.count).to eq(0)
end end
it 'filters out old notes' do it 'succeeds when no notes found' do
note2.update_attribute(:updated_at, 2.hours.ago) notes = described_class.new(project, create(:user)).execute
notes = NotesFinder.new.execute(project, user, params)
expect(notes).to eq([note1]) expect(notes.count).to eq(0)
end end
context 'confidential issue notes' do context 'on restricted projects' do
let(:project) do
create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE,
snippets_access_level: ProjectFeature::PRIVATE,
merge_requests_access_level: ProjectFeature::PRIVATE)
end
it 'publicly excludes notes on merge requests' do
create(:note_on_merge_request, project: project)
notes = described_class.new(project, create(:user)).execute
expect(notes.count).to eq(0)
end
it 'publicly excludes notes on issues' do
create(:note_on_issue, project: project)
notes = described_class.new(project, create(:user)).execute
expect(notes.count).to eq(0)
end
it 'publicly excludes notes on snippets' do
create(:note_on_project_snippet, project: project)
notes = described_class.new(project, create(:user)).execute
expect(notes.count).to eq(0)
end
end
context 'for target' do
let(:project) { create(:project) }
let(:note1) { create :note_on_commit, project: project }
let(:note2) { create :note_on_commit, project: project }
let(:commit) { note1.noteable }
let(:params) { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } }
before do
note1
note2
end
it 'finds all notes' do
notes = described_class.new(project, user, params).execute
expect(notes.size).to eq(2)
end
it 'finds notes on merge requests' do
note = create(:note_on_merge_request, project: project)
params = { target_type: 'merge_request', target_id: note.noteable.id }
notes = described_class.new(project, user, params).execute
expect(notes).to include(note)
end
it 'finds notes on snippets' do
note = create(:note_on_project_snippet, project: project)
params = { target_type: 'snippet', target_id: note.noteable.id }
notes = described_class.new(project, user, params).execute
expect(notes.count).to eq(1)
end
it 'raises an exception for an invalid target_type' do
params.merge!(target_type: 'invalid')
expect { described_class.new(project, user, params).execute }.to raise_error('invalid target_type')
end
it 'filters out old notes' do
note2.update_attribute(:updated_at, 2.hours.ago)
notes = described_class.new(project, user, params).execute
expect(notes).to eq([note1])
end
context 'confidential issue notes' do
let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) }
let!(:confidential_note) { create(:note, noteable: confidential_issue, project: confidential_issue.project) }
let(:params) { { target_id: confidential_issue.id, target_type: 'issue', last_fetched_at: 1.hour.ago.to_i } }
it 'returns notes if user can see the issue' do
expect(described_class.new(project, user, params).execute).to eq([confidential_note])
end
it 'raises an error if user can not see the issue' do
user = create(:user)
expect { described_class.new(project, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'raises an error for project members with guest role' do
user = create(:user)
project.team << [user, :guest]
expect { described_class.new(project, user, params).execute }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end
describe '.search' do
let(:project) { create(:empty_project, :public) }
let(:note) { create(:note_on_issue, note: 'WoW', project: project) }
it 'returns notes with matching content' do
expect(described_class.new(note.project, nil, search: note.note).execute).to eq([note])
end
it 'returns notes with matching content regardless of the casing' do
expect(described_class.new(note.project, nil, search: 'WOW').execute).to eq([note])
end
it 'returns commit notes user can access' do
note = create(:note_on_commit, project: project)
expect(described_class.new(note.project, create(:user), search: note.note).execute).to eq([note])
end
context "confidential issues" do
let(:user) { create(:user) }
let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) } let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) }
let!(:confidential_note) { create(:note, noteable: confidential_issue, project: confidential_issue.project) } let(:confidential_note) { create(:note, note: "Random", noteable: confidential_issue, project: confidential_issue.project) }
let(:params) { { target_id: confidential_issue.id, target_type: 'issue', last_fetched_at: 1.hour.ago.to_i } } it "returns notes with matching content if user can see the issue" do
expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to eq([confidential_note])
it 'returns notes if user can see the issue' do
expect(NotesFinder.new.execute(project, user, params)).to eq([confidential_note])
end end
it 'raises an error if user can not see the issue' do it "does not return notes with matching content if user can not see the issue" do
user = create(:user) user = create(:user)
expect { NotesFinder.new.execute(project, user, params) }.to raise_error(ActiveRecord::RecordNotFound) expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to be_empty
end end
it 'raises an error for project members with guest role' do it "does not return notes with matching content for project members with guest role" do
user = create(:user) user = create(:user)
project.team << [user, :guest] project.team << [user, :guest]
expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to be_empty
end
expect { NotesFinder.new.execute(project, user, params) }.to raise_error(ActiveRecord::RecordNotFound) it "does not return notes with matching content for unauthenticated users" do
expect(described_class.new(confidential_note.project, nil, search: confidential_note.note).execute).to be_empty
end
end
context 'inlines SQL filters on subqueries for performance' do
let(:sql) { described_class.new(note.project, nil, search: note.note).execute.to_sql }
let(:number_of_noteable_types) { 4 }
specify 'project_id check' do
expect(sql.scan(/project_id/).count).to be >= (number_of_noteable_types + 2)
end
specify 'search filter' do
expect(sql.scan(/LIKE/).count).to be >= number_of_noteable_types
end end
end end
end end

View File

@ -149,4 +149,33 @@ describe Gitlab::ProjectSearchResults, lib: true do
expect(results.issues_count).to eq 3 expect(results.issues_count).to eq 3
end end
end end
describe 'notes search' do
it 'lists notes' do
project = create(:empty_project, :public)
note = create(:note, project: project)
results = described_class.new(user, project, note.note)
expect(results.objects('notes')).to include note
end
it "doesn't list issue notes when access is restricted" do
project = create(:empty_project, :public, issues_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_issue, project: project)
results = described_class.new(user, project, note.note)
expect(results.objects('notes')).not_to include note
end
it "doesn't list merge_request notes when access is restricted" do
project = create(:empty_project, :public, merge_requests_access_level: ProjectFeature::PRIVATE)
note = create(:note_on_merge_request, project: project)
results = described_class.new(user, project, note.note)
expect(results.objects('notes')).not_to include note
end
end
end end

View File

@ -1,16 +1,26 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::SQL::Union, lib: true do describe Gitlab::SQL::Union, lib: true do
let(:relation_1) { User.where(email: 'alice@example.com').select(:id) }
let(:relation_2) { User.where(email: 'bob@example.com').select(:id) }
def to_sql(relation)
relation.reorder(nil).to_sql
end
describe '#to_sql' do describe '#to_sql' do
it 'returns a String joining relations together using a UNION' do it 'returns a String joining relations together using a UNION' do
rel1 = User.where(email: 'alice@example.com') union = described_class.new([relation_1, relation_2])
rel2 = User.where(email: 'bob@example.com')
union = described_class.new([rel1, rel2])
sql1 = rel1.reorder(nil).to_sql expect(union.to_sql).to eq("#{to_sql(relation_1)}\nUNION\n#{to_sql(relation_2)}")
sql2 = rel2.reorder(nil).to_sql end
expect(union.to_sql).to eq("#{sql1}\nUNION\n#{sql2}") it 'skips Model.none segements' do
empty_relation = User.none
union = described_class.new([empty_relation, relation_1, relation_2])
expect{User.where("users.id IN (#{union.to_sql})").to_a}.not_to raise_error
expect(union.to_sql).to eq("#{to_sql(relation_1)}\nUNION\n#{to_sql(relation_2)}")
end end
end end
end end

View File

@ -205,7 +205,7 @@ describe MergeRequest, models: true do
end end
end end
describe "#mr_and_commit_notes" do describe "#related_notes" do
let!(:merge_request) { create(:merge_request) } let!(:merge_request) { create(:merge_request) }
before do before do
@ -217,7 +217,7 @@ describe MergeRequest, models: true do
it "includes notes for commits" do it "includes notes for commits" do
expect(merge_request.commits).not_to be_empty expect(merge_request.commits).not_to be_empty
expect(merge_request.mr_and_commit_notes.count).to eq(2) expect(merge_request.related_notes.count).to eq(2)
end end
it "includes notes for commits from target project as well" do it "includes notes for commits from target project as well" do
@ -225,7 +225,7 @@ describe MergeRequest, models: true do
project: merge_request.target_project) project: merge_request.target_project)
expect(merge_request.commits).not_to be_empty expect(merge_request.commits).not_to be_empty
expect(merge_request.mr_and_commit_notes.count).to eq(3) expect(merge_request.related_notes.count).to eq(3)
end end
end end

View File

@ -162,44 +162,6 @@ describe Note, models: true do
end end
end end
describe '.search' do
let(:note) { create(:note_on_issue, note: 'WoW') }
it 'returns notes with matching content' do
expect(described_class.search(note.note)).to eq([note])
end
it 'returns notes with matching content regardless of the casing' do
expect(described_class.search('WOW')).to eq([note])
end
context "confidential issues" do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) }
let(:confidential_note) { create(:note, note: "Random", noteable: confidential_issue, project: confidential_issue.project) }
it "returns notes with matching content if user can see the issue" do
expect(described_class.search(confidential_note.note, as_user: user)).to eq([confidential_note])
end
it "does not return notes with matching content if user can not see the issue" do
user = create(:user)
expect(described_class.search(confidential_note.note, as_user: user)).to be_empty
end
it "does not return notes with matching content for project members with guest role" do
user = create(:user)
project.team << [user, :guest]
expect(described_class.search(confidential_note.note, as_user: user)).to be_empty
end
it "does not return notes with matching content for unauthenticated users" do
expect(described_class.search(confidential_note.note)).to be_empty
end
end
end
describe "editable?" do describe "editable?" do
it "returns true" do it "returns true" do
note = build(:note) note = build(:note)