diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-12-09 04:56:31 +0300 |
---|---|---|
committer | Alejandro RodrÃguez <alejorro70@gmail.com> | 2016-12-15 17:40:12 +0300 |
commit | 12db4cc0e70d3e249f3bf9fde85e336839422319 (patch) | |
tree | f0313d1f56d6e21403a9a0a16010163766a70d86 | |
parent | 1e1887697d212d8b21f3be3d80247787159e49c5 (diff) |
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?
:warning: - Potentially untested
:bomb: - No test coverage
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)
:white_check_mark: - Permissions check tested
### Note lookup without access check
- [x] :white_check_mark: app/finders/notes_finder.rb:13 :download_code check
- [x] :white_check_mark: app/finders/notes_finder.rb:19 `SnippetsFinder`
- [x] :white_check_mark: app/models/note.rb:121 [`Issue#visible_to_user`]
- [x] :white_check_mark: 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
-rw-r--r-- | app/controllers/projects/notes_controller.rb | 2 | ||||
-rw-r--r-- | app/finders/notes_finder.rb | 113 | ||||
-rw-r--r-- | app/models/merge_request.rb | 4 | ||||
-rw-r--r-- | app/models/note.rb | 17 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_merge_request.html.haml | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_show.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/jej-note-search-uses-finder.yml | 4 | ||||
-rw-r--r-- | lib/gitlab/project_search_results.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/sql/union.rb | 4 | ||||
-rw-r--r-- | spec/controllers/search_controller_spec.rb | 61 | ||||
-rw-r--r-- | spec/factories/notes.rb | 2 | ||||
-rw-r--r-- | spec/finders/notes_finder_spec.rb | 200 | ||||
-rw-r--r-- | spec/lib/gitlab/project_search_results_spec.rb | 29 | ||||
-rw-r--r-- | spec/lib/gitlab/sql/union_spec.rb | 22 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 6 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 38 |
16 files changed, 387 insertions, 121 deletions
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 15ca080c696..b71509f2c9b 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -217,6 +217,6 @@ class Projects::NotesController < Projects::ApplicationController end 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 diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 2484339e3a4..4bd8c83081a 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -1,27 +1,102 @@ class NotesFinder FETCH_OVERLAP = 5.seconds - def execute(project, current_user, params) - target_type = params[:target_type] - target_id = params[:target_id] - # Default to 0 to remain compatible with old clients - last_fetched_at = Time.at(params.fetch(:last_fetched_at, 0).to_i) - - notes = - case target_type - when "commit" - project.notes.for_commit_id(target_id).non_diff_notes - when "issue" - IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author - when "merge_request" - MergeRequestsFinder.new(current_user, project_id: project.id).find(target_id).mr_and_commit_notes.inc_author - when "snippet", "project_snippet" - project.snippets.find(target_id).notes + # Used to filter Notes + # When used with target_type and target_id this returns notes specifically for the controller + # + # Arguments: + # 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 + + def execute + @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at] + @notes + end + + private + + def init_collection + if @params[:target_id] + @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 + Note.none + end + else + finder = noteables_for_type(noteable_type) + @project.notes.where(noteable_type: finder.base_class.name, noteable_id: finder.reorder(nil)) + end + end + + def on_target(target_type, target_id) + 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 - raise 'invalid target_type' + 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) - # Use overlapping intervals to avoid worrying about race conditions - notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh + @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ea3cf1cdaac..b73d7acefea 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -452,7 +452,7 @@ class MergeRequest < ActiveRecord::Base should_remove_source_branch? || force_remove_source_branch? end - def mr_and_commit_notes + def related_notes # Fetch comments only from last 100 commits commits_for_notes_limit = 100 commit_ids = commits.last(commits_for_notes_limit).map(&:id) @@ -468,7 +468,7 @@ class MergeRequest < ActiveRecord::Base end def discussions - @discussions ||= self.mr_and_commit_notes. + @discussions ||= self.related_notes. inc_relations_for_view. fresh. discussions diff --git a/app/models/note.rb b/app/models/note.rb index 08bd08743ef..0c1b05dabf2 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -107,23 +107,6 @@ class Note < ActiveRecord::Base Discussion.for_diff_notes(active_notes). map { |d| [d.line_code, d] }.to_h 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 def cross_reference? diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index fa189ae62d8..959c796ecec 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -39,7 +39,7 @@ = icon('thumbs-down') = downvotes - - note_count = merge_request.mr_and_commit_notes.user.count + - note_count = merge_request.related_notes.user.count %li = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do = icon('comments') diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 896f10104fa..5febe806bdd 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -53,7 +53,7 @@ %li.notes-tab = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do Discussion - %span.badge= @merge_request.mr_and_commit_notes.user.count + %span.badge= @merge_request.related_notes.user.count - if @merge_request.source_project %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 diff --git a/changelogs/unreleased/jej-note-search-uses-finder.yml b/changelogs/unreleased/jej-note-search-uses-finder.yml new file mode 100644 index 00000000000..1768bdfd487 --- /dev/null +++ b/changelogs/unreleased/jej-note-search-uses-finder.yml @@ -0,0 +1,4 @@ +--- +title: Fix missing Note access checks by moving Note#search to updated NoteFinder +merge_request: +author: diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 66e6b29e798..6bdf3db9cb8 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -110,7 +110,7 @@ module Gitlab end 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 def commits diff --git a/lib/gitlab/sql/union.rb b/lib/gitlab/sql/union.rb index 1cd89b3a9c4..222021e8802 100644 --- a/lib/gitlab/sql/union.rb +++ b/lib/gitlab/sql/union.rb @@ -22,9 +22,7 @@ module Gitlab # By using "unprepared_statements" we remove the usage of placeholders # (thus fixing this problem), at a slight performance cost. fragments = ActiveRecord::Base.connection.unprepared_statement do - @relations.map do |rel| - rel.reorder(nil).to_sql - end + @relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?) end fragments.join("\nUNION\n") diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb new file mode 100644 index 00000000000..b7bb9290712 --- /dev/null +++ b/spec/controllers/search_controller_spec.rb @@ -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 diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 6919002dedc..a10ba629760 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -67,7 +67,7 @@ FactoryGirl.define do end trait :on_project_snippet do - noteable { create(:snippet, project: project) } + noteable { create(:project_snippet, project: project) } end trait :system do diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 7c6860372cc..4d21254323c 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -2,59 +2,203 @@ require 'spec_helper' describe NotesFinder do let(:user) { create :user } - 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(:project) { create(:empty_project) } before do project.team << [user, :master] end 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 - note1 - note2 + it 'finds notes on merge requests' do + create(:note_on_merge_request, project: project) + + notes = described_class.new(project, user).execute + + expect(notes.count).to eq(1) end - it 'finds all notes' do - notes = NotesFinder.new.execute(project, user, params) - expect(notes.size).to eq(2) + it 'finds notes on snippets' do + create(:note_on_project_snippet, project: project) + + notes = described_class.new(project, user).execute + + expect(notes.count).to eq(1) end - it 'raises an exception for an invalid target_type' do - params.merge!(target_type: 'invalid') - expect { NotesFinder.new.execute(project, user, params) }.to raise_error('invalid target_type') + it "excludes notes on commits the author can't download" do + project = create(:project, :private) + 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 - it 'filters out old notes' do - note2.update_attribute(:updated_at, 2.hours.ago) - notes = NotesFinder.new.execute(project, user, params) - expect(notes).to eq([note1]) + it 'succeeds when no notes found' do + notes = described_class.new(project, create(:user)).execute + + expect(notes.count).to eq(0) 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) } + 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 } - let(:params) { { target_id: confidential_issue.id, target_type: 'issue', last_fetched_at: 1.hour.ago.to_i } } + notes = described_class.new(project, user, params).execute - it 'returns notes if user can see the issue' do - expect(NotesFinder.new.execute(project, user, params)).to eq([confidential_note]) + expect(notes.count).to eq(1) end - it 'raises an error if user can not see the issue' do + 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_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.new(confidential_note.project, user, search: confidential_note.note).execute).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 { 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 - 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) project.team << [user, :guest] + expect(described_class.new(confidential_note.project, user, search: confidential_note.note).execute).to be_empty + end + + 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 - expect { NotesFinder.new.execute(project, user, params) }.to raise_error(ActiveRecord::RecordNotFound) + specify 'search filter' do + expect(sql.scan(/LIKE/).count).to be >= number_of_noteable_types end end end diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb index 3cd9863ec6a..14ee386dba6 100644 --- a/spec/lib/gitlab/project_search_results_spec.rb +++ b/spec/lib/gitlab/project_search_results_spec.rb @@ -149,4 +149,33 @@ describe Gitlab::ProjectSearchResults, lib: true do expect(results.issues_count).to eq 3 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 diff --git a/spec/lib/gitlab/sql/union_spec.rb b/spec/lib/gitlab/sql/union_spec.rb index 0cdbab87544..849edb09476 100644 --- a/spec/lib/gitlab/sql/union_spec.rb +++ b/spec/lib/gitlab/sql/union_spec.rb @@ -1,16 +1,26 @@ require 'spec_helper' 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 it 'returns a String joining relations together using a UNION' do - rel1 = User.where(email: 'alice@example.com') - rel2 = User.where(email: 'bob@example.com') - union = described_class.new([rel1, rel2]) + union = described_class.new([relation_1, relation_2]) + + expect(union.to_sql).to eq("#{to_sql(relation_1)}\nUNION\n#{to_sql(relation_2)}") + end - sql1 = rel1.reorder(nil).to_sql - sql2 = rel2.reorder(nil).to_sql + it 'skips Model.none segements' do + empty_relation = User.none + union = described_class.new([empty_relation, relation_1, relation_2]) - expect(union.to_sql).to eq("#{sql1}\nUNION\n#{sql2}") + 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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 8b730be91fd..1b71d00eb8f 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -205,7 +205,7 @@ describe MergeRequest, models: true do end end - describe "#mr_and_commit_notes" do + describe "#related_notes" do let!(:merge_request) { create(:merge_request) } before do @@ -217,7 +217,7 @@ describe MergeRequest, models: true do it "includes notes for commits" do 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 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) 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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 17a15b12dcb..310fecd8a5c 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -162,44 +162,6 @@ describe Note, models: true do 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 it "returns true" do note = build(:note) |