12db4cc0e7
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
222 lines
5.8 KiB
Ruby
222 lines
5.8 KiB
Ruby
class Projects::NotesController < Projects::ApplicationController
|
|
include ToggleAwardEmoji
|
|
|
|
# Authorize
|
|
before_action :authorize_read_note!
|
|
before_action :authorize_create_note!, only: [:create]
|
|
before_action :authorize_admin_note!, only: [:update, :destroy]
|
|
before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
|
|
before_action :find_current_user_notes, only: [:index]
|
|
|
|
def index
|
|
current_fetched_at = Time.now.to_i
|
|
|
|
notes_json = { notes: [], last_fetched_at: current_fetched_at }
|
|
|
|
@notes.each do |note|
|
|
next if note.cross_reference_not_visible_for?(current_user)
|
|
|
|
notes_json[:notes] << note_json(note)
|
|
end
|
|
|
|
render json: notes_json
|
|
end
|
|
|
|
def create
|
|
@note = Notes::CreateService.new(project, current_user, note_params).execute
|
|
|
|
if @note.is_a?(Note)
|
|
Banzai::NoteRenderer.render([@note], @project, current_user)
|
|
end
|
|
|
|
respond_to do |format|
|
|
format.json { render json: note_json(@note) }
|
|
format.html { redirect_back_or_default }
|
|
end
|
|
end
|
|
|
|
def update
|
|
@note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
|
|
|
|
if @note.is_a?(Note)
|
|
Banzai::NoteRenderer.render([@note], @project, current_user)
|
|
end
|
|
|
|
respond_to do |format|
|
|
format.json { render json: note_json(@note) }
|
|
format.html { redirect_back_or_default }
|
|
end
|
|
end
|
|
|
|
def destroy
|
|
if note.editable?
|
|
Notes::DeleteService.new(project, current_user).execute(note)
|
|
end
|
|
|
|
respond_to do |format|
|
|
format.js { head :ok }
|
|
end
|
|
end
|
|
|
|
def delete_attachment
|
|
note.remove_attachment!
|
|
note.update_attribute(:attachment, nil)
|
|
|
|
respond_to do |format|
|
|
format.js { head :ok }
|
|
end
|
|
end
|
|
|
|
def resolve
|
|
return render_404 unless note.resolvable?
|
|
|
|
note.resolve!(current_user)
|
|
|
|
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
|
|
|
|
discussion = note.discussion
|
|
|
|
render json: {
|
|
resolved_by: note.resolved_by.try(:name),
|
|
discussion_headline_html: (view_to_html_string('discussions/_headline', discussion: discussion) if discussion)
|
|
}
|
|
end
|
|
|
|
def unresolve
|
|
return render_404 unless note.resolvable?
|
|
|
|
note.unresolve!
|
|
|
|
discussion = note.discussion
|
|
|
|
render json: {
|
|
discussion_headline_html: (view_to_html_string('discussions/_headline', discussion: discussion) if discussion)
|
|
}
|
|
end
|
|
|
|
private
|
|
|
|
def note
|
|
@note ||= @project.notes.find(params[:id])
|
|
end
|
|
alias_method :awardable, :note
|
|
|
|
def note_html(note)
|
|
render_to_string(
|
|
"projects/notes/_note",
|
|
layout: false,
|
|
formats: [:html],
|
|
locals: { note: note }
|
|
)
|
|
end
|
|
|
|
def diff_discussion_html(discussion)
|
|
return unless discussion.diff_discussion?
|
|
|
|
if params[:view] == 'parallel'
|
|
template = "discussions/_parallel_diff_discussion"
|
|
locals =
|
|
if params[:line_type] == 'old'
|
|
{ discussion_left: discussion, discussion_right: nil }
|
|
else
|
|
{ discussion_left: nil, discussion_right: discussion }
|
|
end
|
|
else
|
|
template = "discussions/_diff_discussion"
|
|
locals = { discussion: discussion }
|
|
end
|
|
|
|
render_to_string(
|
|
template,
|
|
layout: false,
|
|
formats: [:html],
|
|
locals: locals
|
|
)
|
|
end
|
|
|
|
def discussion_html(discussion)
|
|
return unless discussion.diff_discussion?
|
|
|
|
render_to_string(
|
|
"discussions/_discussion",
|
|
layout: false,
|
|
formats: [:html],
|
|
locals: { discussion: discussion }
|
|
)
|
|
end
|
|
|
|
def note_json(note)
|
|
attrs = {
|
|
award: false,
|
|
id: note.id
|
|
}
|
|
|
|
if note.is_a?(AwardEmoji)
|
|
attrs.merge!(
|
|
valid: note.valid?,
|
|
award: true,
|
|
name: note.name
|
|
)
|
|
elsif note.persisted?
|
|
Banzai::NoteRenderer.render([note], @project, current_user)
|
|
|
|
attrs.merge!(
|
|
valid: true,
|
|
discussion_id: note.discussion_id,
|
|
html: note_html(note),
|
|
note: note.note
|
|
)
|
|
|
|
if note.diff_note?
|
|
discussion = note.to_discussion
|
|
|
|
attrs.merge!(
|
|
diff_discussion_html: diff_discussion_html(discussion),
|
|
discussion_html: discussion_html(discussion)
|
|
)
|
|
|
|
# The discussion_id is used to add the comment to the correct discussion
|
|
# element on the merge request page. Among other things, the discussion_id
|
|
# contains the sha of head commit of the merge request.
|
|
# When new commits are pushed into the merge request after the initial
|
|
# load of the merge request page, the discussion elements will still have
|
|
# the old discussion_ids, with the old head commit sha. The new comment,
|
|
# however, will have the new discussion_id with the new commit sha.
|
|
# To ensure that these new comments will still end up in the correct
|
|
# discussion element, we also send the original discussion_id, with the
|
|
# old commit sha, along, and fall back on this value when no discussion
|
|
# element with the new discussion_id could be found.
|
|
if note.new_diff_note? && note.position != note.original_position
|
|
attrs[:original_discussion_id] = note.original_discussion_id
|
|
end
|
|
end
|
|
else
|
|
attrs.merge!(
|
|
valid: false,
|
|
errors: note.errors
|
|
)
|
|
end
|
|
|
|
attrs[:commands_changes] = note.commands_changes unless attrs[:award]
|
|
attrs
|
|
end
|
|
|
|
def authorize_admin_note!
|
|
return access_denied! unless can?(current_user, :admin_note, note)
|
|
end
|
|
|
|
def authorize_resolve_note!
|
|
return access_denied! unless can?(current_user, :resolve_note, note)
|
|
end
|
|
|
|
def note_params
|
|
params.require(:note).permit(
|
|
:note, :noteable, :noteable_id, :noteable_type, :project_id,
|
|
:attachment, :line_code, :commit_id, :type, :position
|
|
)
|
|
end
|
|
|
|
def find_current_user_notes
|
|
@notes = NotesFinder.new(project, current_user, params).execute.inc_author
|
|
end
|
|
end
|