diff --git a/.gitlab/ci/qa.gitlab-ci.yml b/.gitlab/ci/qa.gitlab-ci.yml index 1048dd02cd8..b2205a6a9f1 100644 --- a/.gitlab/ci/qa.gitlab-ci.yml +++ b/.gitlab/ci/qa.gitlab-ci.yml @@ -40,3 +40,5 @@ schedule:package-and-qa: - .only-code-qa-changes - .only-canonical-schedules needs: ["build-qa-image", "gitlab:assets:compile"] + # Allowed to fail until https://gitlab.com/gitlab-org/gitlab/issues/33272 is fixed. + allow_failure: true diff --git a/app/models/note.rb b/app/models/note.rb index b1829e71017..03a38346731 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -215,7 +215,7 @@ class Note < ApplicationRecord if force_cross_reference_regex_check? matches_cross_reference_regex? else - SystemNoteService.cross_reference?(note) + ::SystemNotes::IssuablesService.cross_reference?(note) end end # rubocop: enable CodeReuse/ServiceClass diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 2cc8771bfe8..0399ce2e533 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -33,78 +33,16 @@ module SystemNoteService ::SystemNotes::CommitService.new(noteable: noteable, project: project, author: author).tag_commit(tag_name) end - # Called when the assignee of a Noteable is changed or removed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # assignee - User being assigned, or nil - # - # Example Note text: - # - # "removed assignee" - # - # "assigned to @rspeicher" - # - # Returns the created Note object def change_assignee(noteable, project, author, assignee) - body = assignee.nil? ? 'removed assignee' : "assigned to #{assignee.to_reference}" - - create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_assignee(assignee) end - # Called when the assignees of an issuable is changed or removed - # - # issuable - Issuable object (responds to assignees) - # project - Project owning noteable - # author - User performing the change - # assignees - Users being assigned, or nil - # - # Example Note text: - # - # "removed all assignees" - # - # "assigned to @user1 additionally to @user2" - # - # "assigned to @user1, @user2 and @user3 and unassigned from @user4 and @user5" - # - # "assigned to @user1 and @user2" - # - # Returns the created Note object def change_issuable_assignees(issuable, project, author, old_assignees) - unassigned_users = old_assignees - issuable.assignees - added_users = issuable.assignees.to_a - old_assignees - text_parts = [] - - Gitlab::I18n.with_default_locale do - text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any? - text_parts << "unassigned #{unassigned_users.map(&:to_reference).to_sentence}" if unassigned_users.any? - end - - body = text_parts.join(' and ') - - create_note(NoteSummary.new(issuable, project, author, body, action: 'assignee')) + ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_assignees(old_assignees) end - # Called when the milestone of a Noteable is changed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # milestone - Milestone being assigned, or nil - # - # Example Note text: - # - # "removed milestone" - # - # "changed milestone to 7.11" - # - # Returns the created Note object def change_milestone(noteable, project, author, milestone) - format = milestone&.group_milestone? ? :name : :iid - body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}" - - create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_milestone(milestone) end # Called when the due_date of a Noteable is changed @@ -184,28 +122,8 @@ module SystemNoteService create_note(NoteSummary.new(noteable, project, author, body, action: 'time_tracking')) end - # Called when the status of a Noteable is changed - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # status - String status - # source - Mentionable performing the change, or nil - # - # Example Note text: - # - # "merged" - # - # "closed via bc17db76" - # - # Returns the created Note object def change_status(noteable, project, author, status, source = nil) - body = status.dup - body << " via #{source.gfm_reference(project)}" if source - - action = status == 'reopened' ? 'opened' : status - - create_note(NoteSummary.new(noteable, project, author, body, action: action)) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_status(status, source) end # Called when 'merge when pipeline succeeds' is executed @@ -288,69 +206,16 @@ module SystemNoteService note end - # Called when the title of a Noteable is changed - # - # noteable - Noteable object that responds to `title` - # project - Project owning noteable - # author - User performing the change - # old_title - Previous String title - # - # Example Note text: - # - # "changed title from **Old** to **New**" - # - # Returns the created Note object def change_title(noteable, project, author, old_title) - new_title = noteable.title.dup - - old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs - - marked_old_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(old_title).mark(old_diffs, mode: :deletion) - marked_new_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(new_title).mark(new_diffs, mode: :addition) - - body = "changed title from **#{marked_old_title}** to **#{marked_new_title}**" - - create_note(NoteSummary.new(noteable, project, author, body, action: 'title')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_title(old_title) end - # Called when the description of a Noteable is changed - # - # noteable - Noteable object that responds to `description` - # project - Project owning noteable - # author - User performing the change - # - # Example Note text: - # - # "changed the description" - # - # Returns the created Note object def change_description(noteable, project, author) - body = 'changed the description' - - create_note(NoteSummary.new(noteable, project, author, body, action: 'description')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_description end - # Called when the confidentiality changes - # - # issue - Issue object - # project - Project owning the issue - # author - User performing the change - # - # Example Note text: - # - # "made the issue confidential" - # - # Returns the created Note object def change_issue_confidentiality(issue, project, author) - if issue.confidential - body = 'made the issue confidential' - action = 'confidential' - else - body = 'made the issue visible to everyone' - action = 'visible' - end - - create_note(NoteSummary.new(issue, project, author, body, action: action)) + ::SystemNotes::IssuablesService.new(noteable: issue, project: project, author: author).change_issue_confidentiality end # Called when a branch in Noteable is changed @@ -419,159 +284,36 @@ module SystemNoteService create_note(NoteSummary.new(issue, project, author, body, action: 'merge')) end - # Called when a Mentionable references a Noteable - # - # noteable - Noteable object being referenced - # mentioner - Mentionable object - # author - User performing the reference - # - # Example Note text: - # - # "mentioned in #1" - # - # "mentioned in !2" - # - # "mentioned in 54f7727c" - # - # See cross_reference_note_content. - # - # Returns the created Note object def cross_reference(noteable, mentioner, author) - return if cross_reference_disallowed?(noteable, mentioner) - - gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group) - body = cross_reference_note_content(gfm_reference) - - if noteable.is_a?(ExternalIssue) - noteable.project.issues_tracker.create_cross_reference_note(noteable, mentioner, author) - else - create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference')) - end + ::SystemNotes::IssuablesService.new(noteable: noteable, author: author).cross_reference(mentioner) end - # Check if a cross-reference is disallowed - # - # This method prevents adding a "mentioned in !1" note on every single commit - # in a merge request. Additionally, it prevents the creation of references to - # external issues (which would fail). - # - # noteable - Noteable object being referenced - # mentioner - Mentionable object - # - # Returns Boolean - def cross_reference_disallowed?(noteable, mentioner) - return true if noteable.is_a?(ExternalIssue) && !noteable.project.jira_tracker_active? - return false unless mentioner.is_a?(MergeRequest) - return false unless noteable.is_a?(Commit) - - mentioner.commits.include?(noteable) - end - - # Check if a cross reference to a noteable from a mentioner already exists - # - # This method is used to prevent multiple notes being created for a mention - # when a issue is updated, for example. The method also calls notes_for_mentioner - # to check if the mentioner is a commit, and return matches only on commit hash - # instead of project + commit, to avoid repeated mentions from forks. - # - # noteable - Noteable object being referenced - # mentioner - Mentionable object - # - # Returns Boolean def cross_reference_exists?(noteable, mentioner) - notes = noteable.notes.system - notes_for_mentioner(mentioner, noteable, notes).exists? + ::SystemNotes::IssuablesService.new(noteable: noteable).cross_reference_exists?(mentioner) end - # Called when the status of a Task has changed - # - # noteable - Noteable object. - # project - Project owning noteable - # author - User performing the change - # new_task - TaskList::Item object. - # - # Example Note text: - # - # "marked the task Whatever as completed." - # - # Returns the created Note object def change_task_status(noteable, project, author, new_task) - status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE - body = "marked the task **#{new_task.source}** as #{status_label}" - - create_note(NoteSummary.new(noteable, project, author, body, action: 'task')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).change_task_status(new_task) end - # Called when noteable has been moved to another project - # - # direction - symbol, :to or :from - # noteable - Noteable object - # noteable_ref - Referenced noteable - # author - User performing the move - # - # Example Note text: - # - # "moved to some_namespace/project_new#11" - # - # Returns the created Note object def noteable_moved(noteable, project, noteable_ref, author, direction:) - unless [:to, :from].include?(direction) - raise ArgumentError, "Invalid direction `#{direction}`" - end - - cross_reference = noteable_ref.to_reference(project) - body = "moved #{direction} #{cross_reference}" - - create_note(NoteSummary.new(noteable, project, author, body, action: 'moved')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).noteable_moved(noteable_ref, direction) end - # Called when a Noteable has been marked as a duplicate of another Issue - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # canonical_issue - Issue that this is a duplicate of - # - # Example Note text: - # - # "marked this issue as a duplicate of #1234" - # - # "marked this issue as a duplicate of other_project#5678" - # - # Returns the created Note object def mark_duplicate_issue(noteable, project, author, canonical_issue) - body = "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" - create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).mark_duplicate_issue(canonical_issue) end - # Called when a Noteable has been marked as the canonical Issue of a duplicate - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # duplicate_issue - Issue that was a duplicate of this - # - # Example Note text: - # - # "marked #1234 as a duplicate of this issue" - # - # "marked other_project#5678 as a duplicate of this issue" - # - # Returns the created Note object def mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) - body = "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" - create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: project, author: author).mark_canonical_issue_of_duplicate(duplicate_issue) end def discussion_lock(issuable, author) - action = issuable.discussion_locked? ? 'locked' : 'unlocked' - body = "#{action} this #{issuable.class.to_s.titleize.downcase}" - - create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action)) + ::SystemNotes::IssuablesService.new(noteable: issuable, project: issuable.project, author: author).discussion_lock end - def cross_reference?(note_text) - note_text =~ /\A#{cross_reference_note_prefix}/i + def cross_reference_disallowed?(noteable, mentioner) + ::SystemNotes::IssuablesService.new(noteable: noteable).cross_reference_disallowed?(mentioner) end def zoom_link_added(issue, project, author) @@ -584,19 +326,6 @@ module SystemNoteService private - # rubocop: disable CodeReuse/ActiveRecord - def notes_for_mentioner(mentioner, noteable, notes) - if mentioner.is_a?(Commit) - text = "#{cross_reference_note_prefix}%#{mentioner.to_reference(nil)}" - notes.where('(note LIKE ? OR note LIKE ?)', text, text.capitalize) - else - gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group) - text = cross_reference_note_content(gfm_reference) - notes.where(note: [text, text.capitalize]) - end - end - # rubocop: enable CodeReuse/ActiveRecord - def create_note(note_summary) note = Note.create(note_summary.note.merge(system: true)) note.system_note_metadata = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata? @@ -604,14 +333,6 @@ module SystemNoteService note end - def cross_reference_note_prefix - 'mentioned in ' - end - - def cross_reference_note_content(gfm_reference) - "#{cross_reference_note_prefix}#{gfm_reference}" - end - def url_helpers @url_helpers ||= Gitlab::Routing.url_helpers end diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb new file mode 100644 index 00000000000..4377f437798 --- /dev/null +++ b/app/services/system_notes/issuables_service.rb @@ -0,0 +1,314 @@ +# frozen_string_literal: true + +module SystemNotes + class IssuablesService < ::SystemNotes::BaseService + # Called when the assignee of a Noteable is changed or removed + # + # assignee - User being assigned, or nil + # + # Example Note text: + # + # "removed assignee" + # + # "assigned to @rspeicher" + # + # Returns the created Note object + def change_assignee(assignee) + body = assignee.nil? ? 'removed assignee' : "assigned to #{assignee.to_reference}" + + create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee')) + end + + # Called when the assignees of an issuable is changed or removed + # + # assignees - Users being assigned, or nil + # + # Example Note text: + # + # "removed all assignees" + # + # "assigned to @user1 additionally to @user2" + # + # "assigned to @user1, @user2 and @user3 and unassigned from @user4 and @user5" + # + # "assigned to @user1 and @user2" + # + # Returns the created Note object + def change_issuable_assignees(old_assignees) + unassigned_users = old_assignees - noteable.assignees + added_users = noteable.assignees.to_a - old_assignees + text_parts = [] + + Gitlab::I18n.with_default_locale do + text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any? + text_parts << "unassigned #{unassigned_users.map(&:to_reference).to_sentence}" if unassigned_users.any? + end + + body = text_parts.join(' and ') + + create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee')) + end + + # Called when the milestone of a Noteable is changed + # + # milestone - Milestone being assigned, or nil + # + # Example Note text: + # + # "removed milestone" + # + # "changed milestone to 7.11" + # + # Returns the created Note object + def change_milestone(milestone) + format = milestone&.group_milestone? ? :name : :iid + body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}" + + create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone')) + end + + # Called when the title of a Noteable is changed + # + # old_title - Previous String title + # + # Example Note text: + # + # "changed title from **Old** to **New**" + # + # Returns the created Note object + def change_title(old_title) + new_title = noteable.title.dup + + old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs + + marked_old_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(old_title).mark(old_diffs, mode: :deletion) + marked_new_title = Gitlab::Diff::InlineDiffMarkdownMarker.new(new_title).mark(new_diffs, mode: :addition) + + body = "changed title from **#{marked_old_title}** to **#{marked_new_title}**" + + create_note(NoteSummary.new(noteable, project, author, body, action: 'title')) + end + + # Called when the description of a Noteable is changed + # + # noteable - Noteable object that responds to `description` + # project - Project owning noteable + # author - User performing the change + # + # Example Note text: + # + # "changed the description" + # + # Returns the created Note object + def change_description + body = 'changed the description' + + create_note(NoteSummary.new(noteable, project, author, body, action: 'description')) + end + + # Called when a Mentionable references a Noteable + # + # mentioner - Mentionable object + # + # Example Note text: + # + # "mentioned in #1" + # + # "mentioned in !2" + # + # "mentioned in 54f7727c" + # + # See cross_reference_note_content. + # + # Returns the created Note object + def cross_reference(mentioner) + return if cross_reference_disallowed?(mentioner) + + gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group) + body = cross_reference_note_content(gfm_reference) + + if noteable.is_a?(ExternalIssue) + noteable.project.issues_tracker.create_cross_reference_note(noteable, mentioner, author) + else + create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference')) + end + end + + # Check if a cross-reference is disallowed + # + # This method prevents adding a "mentioned in !1" note on every single commit + # in a merge request. Additionally, it prevents the creation of references to + # external issues (which would fail). + # + # mentioner - Mentionable object + # + # Returns Boolean + def cross_reference_disallowed?(mentioner) + return true if noteable.is_a?(ExternalIssue) && !noteable.project.jira_tracker_active? + return false unless mentioner.is_a?(MergeRequest) + return false unless noteable.is_a?(Commit) + + mentioner.commits.include?(noteable) + end + + # Called when the status of a Task has changed + # + # new_task - TaskList::Item object. + # + # Example Note text: + # + # "marked the task Whatever as completed." + # + # Returns the created Note object + def change_task_status(new_task) + status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE + body = "marked the task **#{new_task.source}** as #{status_label}" + + create_note(NoteSummary.new(noteable, project, author, body, action: 'task')) + end + + # Called when noteable has been moved to another project + # + # noteable_ref - Referenced noteable + # direction - symbol, :to or :from + # + # Example Note text: + # + # "moved to some_namespace/project_new#11" + # + # Returns the created Note object + def noteable_moved(noteable_ref, direction) + unless [:to, :from].include?(direction) + raise ArgumentError, "Invalid direction `#{direction}`" + end + + cross_reference = noteable_ref.to_reference(project) + body = "moved #{direction} #{cross_reference}" + + create_note(NoteSummary.new(noteable, project, author, body, action: 'moved')) + end + + # Called when the confidentiality changes + # + # Example Note text: + # + # "made the issue confidential" + # + # Returns the created Note object + def change_issue_confidentiality + if noteable.confidential + body = 'made the issue confidential' + action = 'confidential' + else + body = 'made the issue visible to everyone' + action = 'visible' + end + + create_note(NoteSummary.new(noteable, project, author, body, action: action)) + end + + # Called when the status of a Noteable is changed + # + # status - String status + # source - Mentionable performing the change, or nil + # + # Example Note text: + # + # "merged" + # + # "closed via bc17db76" + # + # Returns the created Note object + def change_status(status, source = nil) + body = status.dup + body << " via #{source.gfm_reference(project)}" if source + + action = status == 'reopened' ? 'opened' : status + + create_note(NoteSummary.new(noteable, project, author, body, action: action)) + end + + # Check if a cross reference to a noteable from a mentioner already exists + # + # This method is used to prevent multiple notes being created for a mention + # when a issue is updated, for example. The method also calls notes_for_mentioner + # to check if the mentioner is a commit, and return matches only on commit hash + # instead of project + commit, to avoid repeated mentions from forks. + # + # mentioner - Mentionable object + # + # Returns Boolean + def cross_reference_exists?(mentioner) + notes = noteable.notes.system + notes_for_mentioner(mentioner, noteable, notes).exists? + end + + # Called when a Noteable has been marked as a duplicate of another Issue + # + # canonical_issue - Issue that this is a duplicate of + # + # Example Note text: + # + # "marked this issue as a duplicate of #1234" + # + # "marked this issue as a duplicate of other_project#5678" + # + # Returns the created Note object + def mark_duplicate_issue(canonical_issue) + body = "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" + create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) + end + + # Called when a Noteable has been marked as the canonical Issue of a duplicate + # + # duplicate_issue - Issue that was a duplicate of this + # + # Example Note text: + # + # "marked #1234 as a duplicate of this issue" + # + # "marked other_project#5678 as a duplicate of this issue" + # + # Returns the created Note object + def mark_canonical_issue_of_duplicate(duplicate_issue) + body = "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" + create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) + end + + def discussion_lock + action = noteable.discussion_locked? ? 'locked' : 'unlocked' + body = "#{action} this #{noteable.class.to_s.titleize.downcase}" + + create_note(NoteSummary.new(noteable, project, author, body, action: action)) + end + + private + + def cross_reference_note_content(gfm_reference) + "#{self.class.cross_reference_note_prefix}#{gfm_reference}" + end + + # rubocop: disable CodeReuse/ActiveRecord + def notes_for_mentioner(mentioner, noteable, notes) + if mentioner.is_a?(Commit) + text = "#{self.class.cross_reference_note_prefix}%#{mentioner.to_reference(nil)}" + notes.where('(note LIKE ? OR note LIKE ?)', text, text.capitalize) + else + gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group) + text = cross_reference_note_content(gfm_reference) + notes.where(note: [text, text.capitalize]) + end + end + # rubocop: enable CodeReuse/ActiveRecord + + def self.cross_reference_note_prefix + 'mentioned in ' + end + + def self.cross_reference?(note_text) + note_text =~ /\A#{cross_reference_note_prefix}/i + end + end +end + +SystemNotes::IssuablesService.prepend_if_ee('::EE::SystemNotes::IssuablesService') diff --git a/app/validators/named_ecdsa_key_validator.rb b/app/validators/named_ecdsa_key_validator.rb index 42ee02b6ad4..9053f375100 100644 --- a/app/validators/named_ecdsa_key_validator.rb +++ b/app/validators/named_ecdsa_key_validator.rb @@ -19,15 +19,13 @@ class NamedEcdsaKeyValidator < ActiveModel::EachValidator private - UNNAMED_CURVE = "UNDEF" - def explicit_ec?(value) return false unless value pkey = OpenSSL::PKey.read(value) return false unless pkey.is_a?(OpenSSL::PKey::EC) - pkey.group.curve_name == UNNAMED_CURVE + pkey.group.asn1_flag != OpenSSL::PKey::EC::NAMED_CURVE rescue OpenSSL::PKey::PKeyError false end diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index b95d565e557..71963ee0c0a 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -104,6 +104,11 @@ For instance: - endpoint = expose_path(api_v4_projects_issues_related_merge_requests_path(id: @project.id, issue_iid: @issue.iid)) ``` +## Internal API + +The [internal API](./internal_api.md) is documented for internal use. Please keep it up to date so we know what endpoints +different components are making use of. + [Entity]: https://gitlab.com/gitlab-org/gitlab/blob/master/lib/api/entities.rb [validation, and coercion of the parameters]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion [installing GitLab under a relative URL]: https://docs.gitlab.com/ee/install/relative_url.html diff --git a/doc/development/internal_api.md b/doc/development/internal_api.md new file mode 100644 index 00000000000..a2665fac77a --- /dev/null +++ b/doc/development/internal_api.md @@ -0,0 +1,349 @@ +# Internal API + +The internal API is used by different GitLab components, it can not be +used by other consumers. This documentation is intended for people +working on the GitLab codebase. + +This documentation does not yet include the internal api used by +GitLab pages. + +## Authentication + +These methods are all authenticated using a shared secret. This secret +is stored in a file at the path configured in `config/gitlab.yml` by +default this is in the root of the rails app named +`.gitlab_shell_secret` + +To authenticate using that token, clients read the contents of that +file, and include the token Base64 encoded in a `secret_token` param +or in the `Gitlab-Shared-Secret` header. + +NOTE: **Note:** +The internal api used by GitLab pages uses a different kind of +authentication. + +## Git Authentication + +This is called by Gitaly and GitLab-shell to check access to a +repository. + +When called from GitLab-shell no changes are passed and the internal +API replies with the information needed to pass the request on to +Gitaly. + +When called from Gitaly in a `pre-receive` hook the changes are passed +and those are validated to determine if the push is allowed. + +``` +POST /internal/allowed +``` + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `key_id` | string | no | Id of the SSH-key used to connect to GitLab-shell | +| `username` | string | no | Username from the certificate used to connect to GitLab-Shell | +| `project` | string | no (if `gl_repository` is passed) | Path to the project | +| `gl_repository` | string | no (if `project` is passed) | Path to the project | +| `protocol` | string | yes | SSH when called from GitLab-shell, HTTP or SSH when called from Gitaly | +| `action` | string | yes | Git command being run (`git-upload-pack`, `git-receive-pack`, `git-upload-archive`) | +| `changes` | string | yes | ` ` when called from Gitaly, The magic string `_any` when called from GitLab Shell | + +Example request: + +```sh +curl --request POST --header "Gitlab-Shared-Secret: " --data "key_id=11&project=gnuwget/wget2&action=git-upload-pack&protocol=ssh" http://localhost:3001/api/v4/internal/allowed +``` + +Example response: + +``` +{ + "status": true, + "gl_repository": "project-3", + "gl_project_path": "gnuwget/wget2", + "gl_id": "user-1", + "gl_username": "root", + "git_config_options": [], + "gitaly": { + "repository": { + "storage_name": "default", + "relative_path": "@hashed/4e/07/4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce.git", + "git_object_directory": "", + "git_alternate_object_directories": [], + "gl_repository": "project-3", + "gl_project_path": "gnuwget/wget2" + }, + "address": "unix:/Users/bvl/repos/gitlab/gitaly.socket", + "token": null + }, + "gl_console_messages": [] +} +``` + +### Known consumers + +- Gitaly +- GitLab-shell + +## LFS Authentication + +This is the endpoint that gets called from GitLab-shell to provide +information for LFS clients when the repository is accessed over SSH. + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `key_id` | string | no | Id of the SSH-key used to connect to GitLab-shell | +| `username`| string | no | Username from the certificate used to connect to GitLab-Shell | +| `project` | string | no | Path to the project | + +Example request: + +```sh +curl --request POST --header "Gitlab-Shared-Secret: " --data "key_id=11&project=gnuwget/wget2" http://localhost:3001/api/v4/internal/lfs_authenticate +``` + +``` +{ + "username": "root", + "lfs_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjp7ImFjdG9yIjoicm9vdCJ9LCJqdGkiOiIyYWJhZDcxZC0xNDFlLTQ2NGUtOTZlMi1mODllYWRiMGVmZTYiLCJpYXQiOjE1NzAxMTc2NzYsIm5iZiI6MTU3MDExNzY3MSwiZXhwIjoxNTcwMTE5NDc2fQ.g7atlBw1QMY7QEBVPE0LZ8ZlKtaRzaMRmNn41r2YITM", + "repository_http_path": "http://localhost:3001/gnuwget/wget2.git", + "expires_in": 1800 +} +``` + +### Known consumers + +- GitLab-shell + +## Get merge requests for a ref [NOT USED] + +``` +GET /internal/merge_request_urls +``` + +**Deprecated**: This used to be called from GitLab shell to fetch the +merge requests for a change to output them after a push, but this is +now handled in the `/internal/post_receive` call. + +## Authorized Keys Check + +This endpoint is called by the GitLab-shell authorized keys +check. Which is called by OpenSSH for [fast ssh key +lookup](../administration/operations/fast_ssh_key_lookup.md). + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `key` | string | yes | SSH key as passed by OpenSSH to GitLab-shell | + +``` +GET /internal/authorized_keys +``` + +Example request: + +```sh +curl --request GET --header "Gitlab-Shared-Secret: ""http://localhost:3001/api/v4/internal/authorized_keys?key=" +``` + +Example response: + +``` +{ + "id": 11, + "title": "admin@example.com", + "key": "ssh-rsa ...", + "created_at": "2019-06-27T15:29:02.219Z" +} +``` + +### Known consumers + +- GitLab-shell + +## Get user for user id or key + +This endpoint is used when a user performs `ssh git@gitlab.com`. It +discovers the user associated with an SSH key. + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `key_id` | integer | no | The id of the SSH key used as found in the authorized-keys file or through the `/authorized_keys` check | +| `username` | string | no | Username of the user being looked up, used by GitLab-shell when authenticating using a certificate | +| `user_id` | integer | no | **Deprecated** User_id of the user being looked up | + +``` +GET /internal/discover +``` + +Example request: + +```sh +curl --request GET --header "Gitlab-Shared-Secret: " "http://localhost:3001/api/v4/internal/discover?key_id=7" +``` + +Example response: + +``` +{ + "id": 7, + "name": "Dede Eichmann", + "username": "rubi" +} +``` + +### Known consumers + +- GitLab-shell + +## Instance information + +This get's some generic information about the instance. This is used +by Geo nodes to get information about eachother + +``` +GET /internal/check +``` + +Example request: + +```sh +curl --request GET --header "Gitlab-Shared-Secret: " "http://localhost:3001/api/v4/internal/check" +``` + +Example response: + +``` +{ + "api_version": "v4", + "gitlab_version": "12.3.0-pre", + "gitlab_rev": "d69c988e6a6", + "redis": true +} +``` + +### Known consumers + +- GitLab Geo +- GitLab-shell's `bin/check` + +## Broadcast message(s) [NOT USED] + +``` +GET /internal/broadcast_message +GET /internal/broadcast_messages +``` + +**Deprecated:** This used to be used by GitLab-shell to print out broadcast +messages. But this is now included in the `post_receive` call. Other +clients can use the public BroadcastMessages API. + +## Get new 2FA recovery codes using an SSH key + +This is called from GitLab-shell and allows users to get new 2FA +recovery codes based on their SSH key + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `key_id` | integer | no | The id of the SSH key used as found in the authorized-keys file or through the `/authorized_keys` check | +| `user_id` | integer | no | **Deprecated** User_id for which to generate new recovery codes | + +``` +GET /internal/two_factor_recovery_codes +``` + +Example request: + +```sh +curl --request POST --header "Gitlab-Shared-Secret: " --data "key_id=7" http://localhost:3001/api/v4/internal/two_factor_recovery_codes +``` + +Example response: + +``` +{ + "success": true, + "recovery_codes": [ + "d93ee7037944afd5", + "19d7b84862de93dd", + "1e8c52169195bf71", + "be50444dddb7ca84", + "26048c77d161d5b7", + "482d5c03d1628c47", + "d2c695e309ce7679", + "dfb4748afc4f12a7", + "0e5f53d1399d7979", + "af04d5622153b020" + ] +} +``` + +### Known consumers + +- GitLab-shell + +## Incrementing counter on pre-receive + +This is called from the Gitaly hooks increasing the reference counter +for a push that might be accepted. + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `gl_repository` | string | yes | repository identifier for the repository receiving the push | + +``` +POST /internal/pre_receive +``` + +Example request: + +```sh +curl --request POST --header "Gitlab-Shared-Secret: " --data "gl_repository=project-7" http://localhost:3001/api/v4/internal/pre_receive +``` + +Example response: + +``` +{ + "reference_counter_increased": true +} +``` + +## Notify Post Receive [UNUSED] ? + +## PostReceive + +Called from Gitaly after a receiving a push. This triggers the +`PostReceive`-worker in sidekiq, processes the passed push options and +builds the response including messages that need to be displayed to +the user. + +| Attribute | Type | Required | Description | +|:----------|:-------|:---------|:------------| +| `identifier` | string | yes | `user-[id]` or `key-[id]` Identifying the user performing the push | +| `gl_repository` | string | yes | identifier of the repository being pushed to | +| `push_options` | [string] | no | array of push options | +| `changes` | string | no | refs to be updated in the push in the format `oldrev newrev refname\n`. | + +``` +POST /internal/post_receive +``` + +Example Request: + +```sh +curl --request POST --header "Gitlab-Shared-Secret: " --data "gl_repository=project-7" --data "identifier=user-1" --data "changes=0000000000000000000000000000000000000000 fd9e76b9136bdd9fe217061b497745792fe5a5ee gh-pages\n" http://localhost:3001/api/v4/internal/post_receive +``` + +Example response: + +``` +{ + "messages": [ + { + "message": "Hello from post-receive", + "type": "alert" + } + ], + "reference_counter_decreased": true +} +``` diff --git a/lib/gitlab/gitaly_client/storage_service.rb b/lib/gitlab/gitaly_client/storage_service.rb index 94fb0d87364..c7fcef6ddc5 100644 --- a/lib/gitlab/gitaly_client/storage_service.rb +++ b/lib/gitlab/gitaly_client/storage_service.rb @@ -7,14 +7,6 @@ module Gitlab @storage = storage end - # Returns all directories in the git storage directory, lexically ordered - def list_directories(depth: 1) - request = Gitaly::ListDirectoriesRequest.new(storage_name: @storage, depth: depth) - - GitalyClient.call(@storage, :storage_service, :list_directories, request, timeout: GitalyClient.medium_timeout) - .flat_map(&:paths) - end - # Delete all repositories in the storage. This is a slow and VERY DESTRUCTIVE operation. def delete_all_repositories request = Gitaly::DeleteAllRepositoriesRequest.new(storage_name: @storage) diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 95abe8fe060..2e7b2b88432 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -160,7 +160,7 @@ describe PagesDomain do end context 'when curve is set explicitly by parameters' do - it 'adds errors to private key', :quarantine do + it 'adds errors to private key' do domain = build(:pages_domain, :explicit_ecdsa) expect(domain).to be_invalid diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index ad87f9e2f23..efc259babdc 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe SystemNoteService do - include ProjectForksHelper include Gitlab::Routing include RepoHelpers include AssetsHelpers @@ -41,145 +40,38 @@ describe SystemNoteService do end describe '.change_assignee' do - subject { described_class.change_assignee(noteable, project, author, assignee) } + let(:assignee) { double } - let(:assignee) { create(:user) } - - it_behaves_like 'a system note' do - let(:action) { 'assignee' } - end - - context 'when assignee added' do - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq "assigned to @#{assignee.username}" + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_assignee).with(assignee) end - end - context 'when assignee removed' do - let(:assignee) { nil } - - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq 'removed assignee' - end + described_class.change_assignee(noteable, project, author, assignee) end end describe '.change_issuable_assignees' do - subject { described_class.change_issuable_assignees(noteable, project, author, [assignee]) } + let(:assignees) { [double, double] } - let(:assignee) { create(:user) } - let(:assignee1) { create(:user) } - let(:assignee2) { create(:user) } - let(:assignee3) { create(:user) } - - it_behaves_like 'a system note' do - let(:action) { 'assignee' } - end - - def build_note(old_assignees, new_assignees) - issue.assignees = new_assignees - described_class.change_issuable_assignees(issue, project, author, old_assignees).note - end - - it_behaves_like 'a note with overridable created_at' - - it 'builds a correct phrase when an assignee is added to a non-assigned issue' do - expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" - end - - it 'builds a correct phrase when assignee removed' do - expect(build_note([assignee1], [])).to eq "unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when assignees changed' do - expect(build_note([assignee1], [assignee2])).to eq \ - "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when three assignees removed and one added' do - expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ - "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" - end - - it 'builds a correct phrase when one assignee changed from a set' do - expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \ - "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" - end - - it 'builds a correct phrase when one assignee removed from a set' do - expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \ - "unassigned @#{assignee2.username}" - end - - it 'builds a correct phrase when the locale is different' do - Gitlab::I18n.with_locale('pt-BR') do - expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ - "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issuable_assignees).with(assignees) end + + described_class.change_issuable_assignees(noteable, project, author, assignees) end end describe '.change_milestone' do - context 'for a project milestone' do - subject { described_class.change_milestone(noteable, project, author, milestone) } + let(:milestone) { double } - let(:milestone) { create(:milestone, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_milestone).with(milestone) end - context 'when milestone added' do - it 'sets the note text' do - reference = milestone.to_reference(format: :iid) - - expect(subject.note).to eq "changed milestone to #{reference}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end - end - - context 'for a group milestone' do - subject { described_class.change_milestone(noteable, project, author, milestone) } - - let(:milestone) { create(:milestone, group: group) } - - it_behaves_like 'a system note' do - let(:action) { 'milestone' } - end - - context 'when milestone added' do - it 'sets the note text to use the milestone name' do - expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" - end - - it_behaves_like 'a note with overridable created_at' - end - - context 'when milestone removed' do - let(:milestone) { nil } - - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' - end - - it_behaves_like 'a note with overridable created_at' - end + described_class.change_milestone(noteable, project, author, milestone) end end @@ -210,28 +102,15 @@ describe SystemNoteService do end describe '.change_status' do - subject { described_class.change_status(noteable, project, author, status, source) } + let(:status) { double } + let(:source) { double } - context 'with status reopened' do - let(:status) { 'reopened' } - let(:source) { nil } - - it_behaves_like 'a note with overridable created_at' - - it_behaves_like 'a system note' do - let(:action) { 'opened' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_status).with(status, source) end - end - context 'with a source' do - let(:status) { 'opened' } - let(:source) { double('commit', gfm_reference: 'commit 123456') } - - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq "#{status} via commit 123456" - end + described_class.change_status(noteable, project, author, status, source) end end @@ -285,65 +164,34 @@ describe SystemNoteService do end describe '.change_title' do - let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } + let(:title) { double } - subject { described_class.change_title(noteable, project, author, 'Old title') } - - context 'when noteable responds to `title`' do - it_behaves_like 'a system note' do - let(:action) { 'title' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_title).with(title) end - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note) - .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" - end + described_class.change_title(noteable, project, author, title) end end describe '.change_description' do - subject { described_class.change_description(noteable, project, author) } - - context 'when noteable responds to `description`' do - it_behaves_like 'a system note' do - let(:action) { 'description' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_description) end - it_behaves_like 'a note with overridable created_at' - - it 'sets the note text' do - expect(subject.note).to eq('changed the description') - end + described_class.change_description(noteable, project, author) end end describe '.change_issue_confidentiality' do - subject { described_class.change_issue_confidentiality(noteable, project, author) } - - context 'issue has been made confidential' do - before do - noteable.update_attribute(:confidential, true) + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issue_confidentiality) end - it_behaves_like 'a system note' do - let(:action) { 'confidential' } - end - - it 'sets the note text' do - expect(subject.note).to eq 'made the issue confidential' - end - end - - context 'issue has been made visible' do - it_behaves_like 'a system note' do - let(:action) { 'visible' } - end - - it 'sets the note text' do - expect(subject.note).to eq 'made the issue visible to everyone' - end + described_class.change_issue_confidentiality(noteable, project, author) end end @@ -447,262 +295,51 @@ describe SystemNoteService do end describe '.cross_reference' do - subject { described_class.cross_reference(noteable, mentioner, author) } + let(:mentioner) { double } - let(:mentioner) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'cross_reference' } - end - - context 'when cross-reference disallowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(true) + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference).with(mentioner) end - it 'returns nil' do - expect(subject).to be_nil - end - - it 'does not create a system note metadata record' do - expect { subject }.not_to change { SystemNoteMetadata.count } - end - end - - context 'when cross-reference allowed' do - before do - expect(described_class).to receive(:cross_reference_disallowed?).and_return(false) - end - - it_behaves_like 'a system note' do - let(:action) { 'cross_reference' } - end - - it_behaves_like 'a note with overridable created_at' - - describe 'note_body' do - context 'cross-project' do - let(:project2) { create(:project, :repository) } - let(:mentioner) { create(:issue, project: project2) } - - context 'from Commit' do - let(:mentioner) { project2.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference(project)}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference(project)}" - end - end - end - - context 'within the same project' do - context 'from Commit' do - let(:mentioner) { project.repository.commit } - - it 'references the mentioning commit' do - expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference}" - end - end - - context 'from non-Commit' do - it 'references the mentioning object' do - expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference}" - end - end - end - end + described_class.cross_reference(double, mentioner, double) end end describe '.cross_reference_disallowed?' do - context 'when mentioner is not a MergeRequest' do - it 'is falsey' do - mentioner = noteable.dup - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_falsey - end - end + let(:mentioner) { double } - context 'when mentioner is a MergeRequest' do - let(:mentioner) { create(:merge_request, :simple, source_project: project) } - let(:noteable) { project.commit } - - it 'is truthy when noteable is in commits' do - expect(mentioner).to receive(:commits).and_return([noteable]) - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_truthy + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference_disallowed?).with(mentioner) end - it 'is falsey when noteable is not in commits' do - expect(mentioner).to receive(:commits).and_return([]) - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_falsey - end - end - - context 'when notable is an ExternalIssue' do - let(:noteable) { ExternalIssue.new('EXT-1234', project) } - it 'is truthy' do - mentioner = noteable.dup - expect(described_class.cross_reference_disallowed?(noteable, mentioner)) - .to be_truthy - end + described_class.cross_reference_disallowed?(double, mentioner) end end describe '.cross_reference_exists?' do - let(:commit0) { project.commit } - let(:commit1) { project.commit('HEAD~2') } + let(:mentioner) { double } - context 'issue from commit' do - before do - # Mention issue (noteable) from commit0 - described_class.cross_reference(noteable, commit0, author) + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:cross_reference_exists?).with(mentioner) end - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit0)) - .to be_truthy - end - - it 'is falsey when not already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit1)) - .to be_falsey - end - - context 'legacy capitalized cross reference' do - before do - # Mention issue (noteable) from commit0 - system_note = described_class.cross_reference(noteable, commit0, author) - system_note.update(note: system_note.note.capitalize) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(noteable, commit0)) - .to be_truthy - end - end - end - - context 'commit from commit' do - before do - # Mention commit1 from commit0 - described_class.cross_reference(commit0, commit1, author) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(commit0, commit1)) - .to be_truthy - end - - it 'is falsey when not already mentioned' do - expect(described_class.cross_reference_exists?(commit1, commit0)) - .to be_falsey - end - - context 'legacy capitalized cross reference' do - before do - # Mention commit1 from commit0 - system_note = described_class.cross_reference(commit0, commit1, author) - system_note.update(note: system_note.note.capitalize) - end - - it 'is truthy when already mentioned' do - expect(described_class.cross_reference_exists?(commit0, commit1)) - .to be_truthy - end - end - end - - context 'commit with cross-reference from fork' do - let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user } - let(:forked_project) { fork_project(project, author2, repository: true) } - let(:commit2) { forked_project.commit } - - before do - described_class.cross_reference(noteable, commit0, author2) - end - - it 'is true when a fork mentions an external issue' do - expect(described_class.cross_reference_exists?(noteable, commit2)) - .to be true - end - - context 'legacy capitalized cross reference' do - before do - system_note = described_class.cross_reference(noteable, commit0, author2) - system_note.update(note: system_note.note.capitalize) - end - - it 'is true when a fork mentions an external issue' do - expect(described_class.cross_reference_exists?(noteable, commit2)) - .to be true - end - end + described_class.cross_reference_exists?(double, mentioner) end end describe '.noteable_moved' do - let(:new_project) { create(:project) } - let(:new_noteable) { create(:issue, project: new_project) } + let(:noteable_ref) { double } + let(:direction) { double } - subject do - described_class.noteable_moved(noteable, project, new_noteable, author, direction: direction) - end - - shared_examples 'cross project mentionable' do - include MarkupHelper - - it 'contains cross reference to new noteable' do - expect(subject.note).to include cross_project_reference(new_project, new_noteable) + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:noteable_moved).with(noteable_ref, direction) end - it 'mentions referenced noteable' do - expect(subject.note).to include new_noteable.to_reference - end - - it 'mentions referenced project' do - expect(subject.note).to include new_project.full_path - end - end - - context 'moved to' do - let(:direction) { :to } - - it_behaves_like 'cross project mentionable' - it_behaves_like 'a system note' do - let(:action) { 'moved' } - end - - it 'notifies about noteable being moved to' do - expect(subject.note).to match('moved to') - end - end - - context 'moved from' do - let(:direction) { :from } - - it_behaves_like 'cross project mentionable' - it_behaves_like 'a system note' do - let(:action) { 'moved' } - end - - it 'notifies about noteable being moved from' do - expect(subject.note).to match('moved from') - end - end - - context 'invalid direction' do - let(:direction) { :invalid } - - it 'raises error' do - expect { subject }.to raise_error StandardError, /Invalid direction/ - end + described_class.noteable_moved(double, double, noteable_ref, double, direction: direction) end end @@ -1064,17 +701,14 @@ describe SystemNoteService do end describe '.change_task_status' do - let(:noteable) { create(:issue, project: project) } - let(:task) { double(:task, complete?: true, source: 'task') } + let(:new_task) { double } - subject { described_class.change_task_status(noteable, project, author, task) } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_task_status).with(new_task) + end - it_behaves_like 'a system note' do - let(:action) { 'task' } - end - - it "posts the 'marked the task as complete' system note" do - expect(subject.note).to eq("marked the task **task** as completed") + described_class.change_task_status(noteable, project, author, new_task) end end @@ -1152,90 +786,42 @@ describe SystemNoteService do end describe '.mark_duplicate_issue' do - subject { described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) } + let(:canonical_issue) { double } - context 'within the same project' do - let(:canonical_issue) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:mark_duplicate_issue).with(canonical_issue) end - it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference}" } - end - - context 'across different projects' do - let(:other_project) { create(:project) } - let(:canonical_issue) { create(:issue, project: other_project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } - end - - it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" } + described_class.mark_duplicate_issue(noteable, project, author, canonical_issue) end end describe '.mark_canonical_issue_of_duplicate' do - subject { described_class.mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) } + let(:duplicate_issue) { double } - context 'within the same project' do - let(:duplicate_issue) { create(:issue, project: project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:mark_canonical_issue_of_duplicate).with(duplicate_issue) end - it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference} as a duplicate of this issue" } - end - - context 'across different projects' do - let(:other_project) { create(:project) } - let(:duplicate_issue) { create(:issue, project: other_project) } - - it_behaves_like 'a system note' do - let(:action) { 'duplicate' } - end - - it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" } + described_class.mark_canonical_issue_of_duplicate(noteable, project, author, duplicate_issue) end end describe '.discussion_lock' do - subject { described_class.discussion_lock(noteable, author) } + let(:issuable) { double } - context 'discussion unlocked' do - it_behaves_like 'a system note' do - let(:action) { 'unlocked' } - end - - it 'creates the note text correctly' do - [:issue, :merge_request].each do |type| - issuable = create(type) - - expect(described_class.discussion_lock(issuable, author).note) - .to eq("unlocked this #{type.to_s.titleize.downcase}") - end - end + before do + allow(issuable).to receive(:project).and_return(double) end - context 'discussion locked' do - before do - noteable.update_attribute(:discussion_locked, true) + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:discussion_lock) end - it_behaves_like 'a system note' do - let(:action) { 'locked' } - end - - it 'creates the note text correctly' do - [:issue, :merge_request].each do |type| - issuable = create(type, discussion_locked: true) - - expect(described_class.discussion_lock(issuable, author).note) - .to eq("locked this #{type.to_s.titleize.downcase}") - end - end + described_class.discussion_lock(issuable, double) end end end diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb new file mode 100644 index 00000000000..a914b34cb23 --- /dev/null +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -0,0 +1,619 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::SystemNotes::IssuablesService do + include ProjectForksHelper + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:author) { create(:user) } + let(:noteable) { create(:issue, project: project) } + let(:issue) { noteable } + + let(:service) { described_class.new(noteable: noteable, project: project, author: author) } + + describe '#change_assignee' do + subject { service.change_assignee(assignee) } + + let(:assignee) { create(:user) } + + it_behaves_like 'a system note' do + let(:action) { 'assignee' } + end + + context 'when assignee added' do + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq "assigned to @#{assignee.username}" + end + end + + context 'when assignee removed' do + let(:assignee) { nil } + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq 'removed assignee' + end + end + end + + describe '#change_issuable_assignees' do + subject { service.change_issuable_assignees([assignee]) } + + let(:assignee) { create(:user) } + let(:assignee1) { create(:user) } + let(:assignee2) { create(:user) } + let(:assignee3) { create(:user) } + + it_behaves_like 'a system note' do + let(:action) { 'assignee' } + end + + def build_note(old_assignees, new_assignees) + issue.assignees = new_assignees + service.change_issuable_assignees(old_assignees).note + end + + it_behaves_like 'a note with overridable created_at' + + it 'builds a correct phrase when an assignee is added to a non-assigned issue' do + expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" + end + + it 'builds a correct phrase when assignee removed' do + expect(build_note([assignee1], [])).to eq "unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when assignees changed' do + expect(build_note([assignee1], [assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when three assignees removed and one added' do + expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ + "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + end + + it 'builds a correct phrase when one assignee changed from a set' do + expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \ + "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}" + end + + it 'builds a correct phrase when one assignee removed from a set' do + expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \ + "unassigned @#{assignee2.username}" + end + + it 'builds a correct phrase when the locale is different' do + Gitlab::I18n.with_locale('pt-BR') do + expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \ + "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}" + end + end + end + + describe '#change_milestone' do + subject { service.change_milestone(milestone) } + + context 'for a project milestone' do + let(:milestone) { create(:milestone, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end + + context 'when milestone added' do + it 'sets the note text' do + reference = milestone.to_reference(format: :iid) + + expect(subject.note).to eq "changed milestone to #{reference}" + end + + it_behaves_like 'a note with overridable created_at' + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end + + it_behaves_like 'a note with overridable created_at' + end + end + + context 'for a group milestone' do + let(:milestone) { create(:milestone, group: group) } + + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end + + context 'when milestone added' do + it 'sets the note text to use the milestone name' do + expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" + end + + it_behaves_like 'a note with overridable created_at' + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end + + it_behaves_like 'a note with overridable created_at' + end + end + end + + describe '#change_status' do + subject { service.change_status(status, source) } + + context 'with status reopened' do + let(:status) { 'reopened' } + let(:source) { nil } + + it_behaves_like 'a note with overridable created_at' + + it_behaves_like 'a system note' do + let(:action) { 'opened' } + end + end + + context 'with a source' do + let(:status) { 'opened' } + let(:source) { double('commit', gfm_reference: 'commit 123456') } + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq "#{status} via commit 123456" + end + end + end + + describe '#change_title' do + let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') } + + subject { service.change_title('Old title') } + + context 'when noteable responds to `title`' do + it_behaves_like 'a system note' do + let(:action) { 'title' } + end + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note) + .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" + end + end + end + + describe '#change_description' do + subject { service.change_description } + + context 'when noteable responds to `description`' do + it_behaves_like 'a system note' do + let(:action) { 'description' } + end + + it_behaves_like 'a note with overridable created_at' + + it 'sets the note text' do + expect(subject.note).to eq('changed the description') + end + end + end + + describe '#change_issue_confidentiality' do + subject { service.change_issue_confidentiality } + + context 'issue has been made confidential' do + before do + noteable.update_attribute(:confidential, true) + end + + it_behaves_like 'a system note' do + let(:action) { 'confidential' } + end + + it 'sets the note text' do + expect(subject.note).to eq 'made the issue confidential' + end + end + + context 'issue has been made visible' do + it_behaves_like 'a system note' do + let(:action) { 'visible' } + end + + it 'sets the note text' do + expect(subject.note).to eq 'made the issue visible to everyone' + end + end + end + + describe '#cross_reference' do + let(:service) { described_class.new(noteable: noteable, author: author) } + + let(:mentioner) { create(:issue, project: project) } + + subject { service.cross_reference(mentioner) } + + it_behaves_like 'a system note' do + let(:action) { 'cross_reference' } + end + + context 'when cross-reference disallowed' do + before do + expect_any_instance_of(described_class).to receive(:cross_reference_disallowed?).and_return(true) + end + + it 'returns nil' do + expect(subject).to be_nil + end + + it 'does not create a system note metadata record' do + expect { subject }.not_to change { SystemNoteMetadata.count } + end + end + + context 'when cross-reference allowed' do + before do + expect_any_instance_of(described_class).to receive(:cross_reference_disallowed?).and_return(false) + end + + it_behaves_like 'a system note' do + let(:action) { 'cross_reference' } + end + + it_behaves_like 'a note with overridable created_at' + + describe 'note_body' do + context 'cross-project' do + let(:project2) { create(:project, :repository) } + let(:mentioner) { create(:issue, project: project2) } + + context 'from Commit' do + let(:mentioner) { project2.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference(project)}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference(project)}" + end + end + end + + context 'within the same project' do + context 'from Commit' do + let(:mentioner) { project.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference}" + end + end + end + end + end + end + + describe '#cross_reference_exists?' do + let(:commit0) { project.commit } + let(:commit1) { project.commit('HEAD~2') } + + context 'issue from commit' do + before do + # Mention issue (noteable) from commit0 + service.cross_reference(commit0) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit0)) + .to be_truthy + end + + it 'is falsey when not already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_falsey + end + + context 'legacy capitalized cross reference' do + before do + # Mention issue (noteable) from commit0 + system_note = service.cross_reference(commit0) + system_note.update(note: system_note.note.capitalize) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit0)) + .to be_truthy + end + end + end + + context 'commit from commit' do + let(:service) { described_class.new(noteable: commit0, author: author) } + + before do + # Mention commit1 from commit0 + service.cross_reference(commit1) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_truthy + end + + it 'is falsey when not already mentioned' do + service = described_class.new(noteable: commit1, author: author) + + expect(service.cross_reference_exists?(commit0)) + .to be_falsey + end + + context 'legacy capitalized cross reference' do + before do + # Mention commit1 from commit0 + system_note = service.cross_reference(commit1) + system_note.update(note: system_note.note.capitalize) + end + + it 'is truthy when already mentioned' do + expect(service.cross_reference_exists?(commit1)) + .to be_truthy + end + end + end + + context 'commit with cross-reference from fork' do + let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user } + let(:forked_project) { fork_project(project, author2, repository: true) } + let(:commit2) { forked_project.commit } + + let(:service) { described_class.new(noteable: noteable, author: author2) } + + before do + service.cross_reference(commit0) + end + + it 'is true when a fork mentions an external issue' do + expect(service.cross_reference_exists?(commit2)) + .to be true + end + + context 'legacy capitalized cross reference' do + before do + system_note = service.cross_reference(commit0) + system_note.update(note: system_note.note.capitalize) + end + + it 'is true when a fork mentions an external issue' do + expect(service.cross_reference_exists?(commit2)) + .to be true + end + end + end + end + + describe '#change_task_status' do + let(:noteable) { create(:issue, project: project) } + let(:task) { double(:task, complete?: true, source: 'task') } + + subject { service.change_task_status(task) } + + it_behaves_like 'a system note' do + let(:action) { 'task' } + end + + it "posts the 'marked the task as complete' system note" do + expect(subject.note).to eq("marked the task **task** as completed") + end + end + + describe '#noteable_moved' do + let(:new_project) { create(:project) } + let(:new_noteable) { create(:issue, project: new_project) } + + subject do + # service = described_class.new(noteable: noteable, project: project, author: author) + service.noteable_moved(new_noteable, direction) + end + + shared_examples 'cross project mentionable' do + include MarkupHelper + + it 'contains cross reference to new noteable' do + expect(subject.note).to include cross_project_reference(new_project, new_noteable) + end + + it 'mentions referenced noteable' do + expect(subject.note).to include new_noteable.to_reference + end + + it 'mentions referenced project' do + expect(subject.note).to include new_project.full_path + end + end + + context 'moved to' do + let(:direction) { :to } + + it_behaves_like 'cross project mentionable' + it_behaves_like 'a system note' do + let(:action) { 'moved' } + end + + it 'notifies about noteable being moved to' do + expect(subject.note).to match('moved to') + end + end + + context 'moved from' do + let(:direction) { :from } + + it_behaves_like 'cross project mentionable' + it_behaves_like 'a system note' do + let(:action) { 'moved' } + end + + it 'notifies about noteable being moved from' do + expect(subject.note).to match('moved from') + end + end + + context 'invalid direction' do + let(:direction) { :invalid } + + it 'raises error' do + expect { subject }.to raise_error StandardError, /Invalid direction/ + end + end + end + + describe '#mark_duplicate_issue' do + subject { service.mark_duplicate_issue(canonical_issue) } + + context 'within the same project' do + let(:canonical_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference}" } + end + + context 'across different projects' do + let(:other_project) { create(:project) } + let(:canonical_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked this issue as a duplicate of #{canonical_issue.to_reference(project)}" } + end + end + + describe '#mark_canonical_issue_of_duplicate' do + subject { service.mark_canonical_issue_of_duplicate(duplicate_issue) } + + context 'within the same project' do + let(:duplicate_issue) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference} as a duplicate of this issue" } + end + + context 'across different projects' do + let(:other_project) { create(:project) } + let(:duplicate_issue) { create(:issue, project: other_project) } + + it_behaves_like 'a system note' do + let(:action) { 'duplicate' } + end + + it { expect(subject.note).to eq "marked #{duplicate_issue.to_reference(project)} as a duplicate of this issue" } + end + end + + describe '#discussion_lock' do + subject { service.discussion_lock } + + context 'discussion unlocked' do + it_behaves_like 'a system note' do + let(:action) { 'unlocked' } + end + + it 'creates the note text correctly' do + [:issue, :merge_request].each do |type| + issuable = create(type) + + service = described_class.new(noteable: issuable, author: author) + expect(service.discussion_lock.note) + .to eq("unlocked this #{type.to_s.titleize.downcase}") + end + end + end + + context 'discussion locked' do + before do + noteable.update_attribute(:discussion_locked, true) + end + + it_behaves_like 'a system note' do + let(:action) { 'locked' } + end + + it 'creates the note text correctly' do + [:issue, :merge_request].each do |type| + issuable = create(type, discussion_locked: true) + + service = described_class.new(noteable: issuable, author: author) + expect(service.discussion_lock.note) + .to eq("locked this #{type.to_s.titleize.downcase}") + end + end + end + end + + describe '#cross_reference_disallowed?' do + context 'when mentioner is not a MergeRequest' do + it 'is falsey' do + mentioner = noteable.dup + expect(service.cross_reference_disallowed?(mentioner)) + .to be_falsey + end + end + + context 'when mentioner is a MergeRequest' do + let(:mentioner) { create(:merge_request, :simple, source_project: project) } + let(:noteable) { project.commit } + + it 'is truthy when noteable is in commits' do + expect(mentioner).to receive(:commits).and_return([noteable]) + expect(service.cross_reference_disallowed?(mentioner)) + .to be_truthy + end + + it 'is falsey when noteable is not in commits' do + expect(mentioner).to receive(:commits).and_return([]) + expect(service.cross_reference_disallowed?(mentioner)) + .to be_falsey + end + end + + context 'when notable is an ExternalIssue' do + let(:noteable) { ExternalIssue.new('EXT-1234', project) } + it 'is truthy' do + mentioner = noteable.dup + expect(service.cross_reference_disallowed?(mentioner)) + .to be_truthy + end + end + end +end diff --git a/spec/support/matchers/policy_matchers.rb b/spec/support/matchers/policy_matchers.rb new file mode 100644 index 00000000000..020c5ce2baf --- /dev/null +++ b/spec/support/matchers/policy_matchers.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +RSpec::Matchers.define :allow_action do |action| + match do |policy| + expect(policy).to be_allowed(action) + end + + failure_message do |policy| + policy.debug(action, debug_output = +'') + "expected #{policy} to allow #{action}\n\n#{debug_output}" + end + + failure_message_when_negated do |policy| + policy.debug(action, debug_output = +'') + "expected #{policy} not to allow #{action}\n\n#{debug_output}" + end +end diff --git a/spec/support/shared_examples/common_system_notes_examples.rb b/spec/support/shared_examples/common_system_notes_examples.rb index 83d0e818eec..ca79603a022 100644 --- a/spec/support/shared_examples/common_system_notes_examples.rb +++ b/spec/support/shared_examples/common_system_notes_examples.rb @@ -36,16 +36,18 @@ shared_examples_for 'a note with overridable created_at' do end end -shared_examples_for 'a system note' do +shared_examples_for 'a system note' do |params| let(:expected_noteable) { noteable } let(:commit_count) { nil } it 'has the correct attributes', :aggregate_failures do + exclude_project = !params.nil? && params[:exclude_project] + expect(subject).to be_valid expect(subject).to be_system expect(subject.noteable).to eq expected_noteable - expect(subject.project).to eq project + expect(subject.project).to eq project unless exclude_project expect(subject.author).to eq author expect(subject.system_note_metadata.action).to eq(action) diff --git a/spec/validators/named_ecdsa_key_validator_spec.rb b/spec/validators/named_ecdsa_key_validator_spec.rb index 7e3ceb1cbad..044c5b84a56 100644 --- a/spec/validators/named_ecdsa_key_validator_spec.rb +++ b/spec/validators/named_ecdsa_key_validator_spec.rb @@ -43,7 +43,7 @@ describe NamedEcdsaKeyValidator do context 'with ECDSA certificate with explicit curve params' do let(:value) { attributes_for(:pages_domain, :explicit_ecdsa)[:key] } - it 'adds errors', :quarantine do + it 'adds errors' do expect(value).to be_present subject