summaryrefslogtreecommitdiff
path: root/app/controllers/projects/notes_controller.rb
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-12-09 01:56:31 +0000
committerAlejandro Rodríguez <alejorro70@gmail.com>2016-12-15 11:40:12 -0300
commit12db4cc0e70d3e249f3bf9fde85e336839422319 (patch)
treef0313d1f56d6e21403a9a0a16010163766a70d86 /app/controllers/projects/notes_controller.rb
parent1e1887697d212d8b21f3be3d80247787159e49c5 (diff)
downloadgitlab-ce-12db4cc0e70d3e249f3bf9fde85e336839422319.tar.gz
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/controllers/projects/notes_controller.rb')
-rw-r--r--app/controllers/projects/notes_controller.rb2
1 files changed, 1 insertions, 1 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