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 /app | |
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
Diffstat (limited to 'app')
-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 |
6 files changed, 99 insertions, 41 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 |