Merge branch 'master' into 'bootstrap4'

CHANGELOG.md

@@ -2,6 +2,14 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 10.7.2 (2018-04-25)
+
+### Security (2 changes)
+
+- Serve archive requests with the correct file in all cases.
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.7.1 (2018-04-23)
 
 ### Fixed (11 changes)
@@ -237,6 +245,13 @@ entry.
 - Upgrade Gitaly to upgrade its charlock_holmes.
 
 
+## 10.6.5 (2018-04-24)
+
+### Security (1 change)
+
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.6.4 (2018-04-09)
 
 ### Fixed (8 changes, 1 of them is from the community)
@@ -478,6 +493,13 @@ entry.
 - Use host URL to build JIRA remote link icon.
 
 
+## 10.5.8 (2018-04-24)
+
+### Security (1 change)
+
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.5.7 (2018-04-03)
 
 ### Security (2 changes)

GITLAB_WORKHORSE_VERSION

-4.1.0
+4.2.0

app/assets/javascripts/notes/components/discussion_counter.vue

@@ -86,7 +86,7 @@ export default {
             v-html="resolveSvg"
           ></span>
         </span>
-        <span class=".line-resolve-text">
+        <span class="line-resolve-text">
           {{ resolvedDiscussionCount }}/{{ discussionCount }} {{ countText }} resolved
         </span>
       </div>

app/assets/javascripts/sidebar/lib/sidebar_move_issue.js

 import $ from 'jquery';
+import _ from 'underscore';
 
 function isValidProjectId(id) {
   return id > 0;
@@ -43,7 +44,7 @@ class SidebarMoveIssue {
       renderRow: project => `
         <li>
           <a href="#" class="js-move-issue-dropdown-item">
-            ${project.name_with_namespace}
+            ${_.escape(project.name_with_namespace)}
           </a>
         </li>
       `,

app/assets/javascripts/vue_shared/components/identicon.vue

@@ -17,7 +17,7 @@ export default {
   },
   computed: {
     /**
-     * This method is based on app/helpers/application_helper.rb#project_identicon
+     * This method is based on app/helpers/avatars_helper.rb#project_identicon
      */
     identiconStyles() {
       const allowedColors = [

app/assets/stylesheets/pages/notes.scss

@@ -776,7 +776,3 @@ ul.notes {
     height: auto;
   }
 }
-
-.line-resolve-text {
-  vertical-align: middle;
-}

app/controllers/omniauth_callbacks_controller.rb

@@ -8,8 +8,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     omniauth_flow(Gitlab::Auth::OAuth)
   end
 
-  Gitlab.config.omniauth.providers.each do |provider|
-    alias_method provider['name'], :handle_omniauth
+  AuthHelper.providers_for_base_controller.each do |provider|
+    alias_method provider, :handle_omniauth
   end
 
   # Extend the standard implementation to also increment

app/controllers/projects/notes_controller.rb

@@ -33,9 +33,7 @@ class Projects::NotesController < Projects::ApplicationController
   def resolve
     return render_404 unless note.resolvable?
 
-    note.resolve!(current_user)
-
-    MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
+    Notes::ResolveService.new(project, current_user).execute(note)
 
     discussion = note.discussion
 

app/finders/groups_finder.rb

@@ -39,7 +39,7 @@ class GroupsFinder < UnionFinder
 
   def all_groups
     return [owned_groups] if params[:owned]
-    return [Group.all] if current_user&.full_private_access?
+    return [Group.all] if current_user&.full_private_access? && all_available?
 
     groups = []
     groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user
@@ -67,6 +67,10 @@ class GroupsFinder < UnionFinder
   end
 
   def include_public_groups?
-    current_user.nil? || params.fetch(:all_available, true)
+    current_user.nil? || all_available?
+  end
+
+  def all_available?
+    params.fetch(:all_available, true)
   end
 end

app/helpers/application_helper.rb

@@ -32,80 +32,6 @@ module ApplicationHelper
     args.any? { |v| v.to_s.downcase == action_name }
   end
 
-  def project_icon(project_id, options = {})
-    project =
-      if project_id.respond_to?(:avatar_url)
-        project_id
-      else
-        Project.find_by_full_path(project_id)
-      end
-
-    if project.avatar_url
-      image_tag project.avatar_url, options
-    else # generated icon
-      project_identicon(project, options)
-    end
-  end
-
-  def project_identicon(project, options = {})
-    allowed_colors = {
-      red: 'FFEBEE',
-      purple: 'F3E5F5',
-      indigo: 'E8EAF6',
-      blue: 'E3F2FD',
-      teal: 'E0F2F1',
-      orange: 'FBE9E7',
-      gray: 'EEEEEE'
-    }
-
-    options[:class] ||= ''
-    options[:class] << ' identicon'
-    bg_key = project.id % 7
-    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
-
-    content_tag(:div, class: options[:class], style: style) do
-      project.name[0, 1].upcase
-    end
-  end
-
-  # Takes both user and email and returns the avatar_icon by
-  # user (preferred) or email.
-  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    elsif email
-      avatar_icon_for_email(email, size, scale, only_path: only_path)
-    else
-      default_avatar
-    end
-  end
-
-  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
-    user = User.find_by_any_email(email.try(:downcase))
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    else
-      gravatar_icon(email, size, scale)
-    end
-  end
-
-  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
-    if user
-      user.avatar_url(size: size, only_path: only_path) || default_avatar
-    else
-      gravatar_icon(nil, size, scale)
-    end
-  end
-
-  def gravatar_icon(user_email = '', size = nil, scale = 2)
-    GravatarService.new.execute(user_email, size, scale) ||
-      default_avatar
-  end
-
-  def default_avatar
-    asset_path('no_avatar.png')
-  end
-
   def last_commit(project)
     if project.repo_exists?
       time_ago_with_tooltip(project.repository.commit.committed_date)

app/helpers/auth_helper.rb

 module AuthHelper
   PROVIDERS_WITH_ICONS = %w(twitter github gitlab bitbucket google_oauth2 facebook azure_oauth2 authentiq).freeze
-  FORM_BASED_PROVIDERS = [/\Aldap/, 'crowd'].freeze
+  LDAP_PROVIDER = /\Aldap/
 
   def ldap_enabled?
     Gitlab::Auth::LDAP::Config.enabled?
@@ -23,7 +23,7 @@ module AuthHelper
   end
 
   def form_based_provider?(name)
-    FORM_BASED_PROVIDERS.any? { |pattern| pattern === name.to_s }
+    [LDAP_PROVIDER, 'crowd'].any? { |pattern| pattern === name.to_s }
   end
 
   def form_based_providers
@@ -38,6 +38,10 @@ module AuthHelper
     auth_providers.reject { |provider| form_based_provider?(provider) }
   end
 
+  def providers_for_base_controller
+    auth_providers.reject { |provider| LDAP_PROVIDER === provider }
+  end
+
   def enabled_button_based_providers
     disabled_providers = Gitlab::CurrentSettings.disabled_oauth_sign_in_sources || []
 

app/helpers/avatars_helper.rb

 module AvatarsHelper
+  def project_icon(project_id, options = {})
+    project =
+      if project_id.respond_to?(:avatar_url)
+        project_id
+      else
+        Project.find_by_full_path(project_id)
+      end
+
+    if project.avatar_url
+      image_tag project.avatar_url, options
+    else # generated icon
+      project_identicon(project, options)
+    end
+  end
+
+  def project_identicon(project, options = {})
+    allowed_colors = {
+      red: 'FFEBEE',
+      purple: 'F3E5F5',
+      indigo: 'E8EAF6',
+      blue: 'E3F2FD',
+      teal: 'E0F2F1',
+      orange: 'FBE9E7',
+      gray: 'EEEEEE'
+    }
+
+    options[:class] ||= ''
+    options[:class] << ' identicon'
+    bg_key = project.id % 7
+    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
+
+    content_tag(:div, class: options[:class], style: style) do
+      project.name[0, 1].upcase
+    end
+  end
+
+  # Takes both user and email and returns the avatar_icon by
+  # user (preferred) or email.
+  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    elsif email
+      avatar_icon_for_email(email, size, scale, only_path: only_path)
+    else
+      default_avatar
+    end
+  end
+
+  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
+    user = User.find_by_any_email(email.try(:downcase))
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    else
+      gravatar_icon(email, size, scale)
+    end
+  end
+
+  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
+    if user
+      user.avatar_url(size: size, only_path: only_path) || default_avatar
+    else
+      gravatar_icon(nil, size, scale)
+    end
+  end
+
+  def gravatar_icon(user_email = '', size = nil, scale = 2)
+    GravatarService.new.execute(user_email, size, scale) ||
+      default_avatar
+  end
+
+  def default_avatar
+    ActionController::Base.helpers.image_path('no_avatar.png')
+  end
+
   def author_avatar(commit_or_event, options = {})
     user_avatar(options.merge({
       user: commit_or_event.author,

app/helpers/system_note_helper.rb

 module SystemNoteHelper
   ICON_NAMES_BY_ACTION = {
     'commit' => 'commit',
-    'description' => 'pencil',
+    'description' => 'pencil-square',
     'merge' => 'git-merge',
     'merged' => 'git-merge',
     'opened' => 'issue-open',
     'closed' => 'issue-close',
     'time_tracking' => 'timer',
     'assignee' => 'user',
-    'title' => 'pencil',
+    'title' => 'pencil-square',
     'task' => 'task-done',
     'label' => 'label',
     'cross_reference' => 'comment-dots',
@@ -18,7 +18,7 @@ module SystemNoteHelper
     'milestone' => 'clock',
     'discussion' => 'comment',
     'moved' => 'arrow-right',
-    'outdated' => 'pencil',
+    'outdated' => 'pencil-square',
     'duplicate' => 'issue-duplicate',
     'locked' => 'lock',
     'unlocked' => 'lock-open'

app/mailers/notify.rb

@@ -16,6 +16,7 @@ class Notify < BaseMailer
   helper BlobHelper
   helper EmailsHelper
   helper MembersHelper
+  helper AvatarsHelper
   helper GitlabRoutingHelper
 
   def test_email(recipient_email, subject, body)

app/models/commit.rb

@@ -105,6 +105,10 @@ class Commit
         end
       end
     end
+
+    def parent_class
+      ::Project
+    end
   end
 
   attr_accessor :raw

app/models/diff_note.rb

@@ -54,7 +54,20 @@ class DiffNote < Note
   end
 
   def diff_file
-    @diff_file ||= self.original_position.diff_file(self.project.repository)
+    @diff_file ||=
+      begin
+        if created_at_diff?(noteable.diff_refs)
+          # We're able to use the already persisted diffs (Postgres) if we're
+          # presenting a "current version" of the MR discussion diff.
+          # So no need to make an extra Gitaly diff request for it.
+          # As an extra benefit, the returned `diff_file` already
+          # has `highlighted_diff_lines` data set from Redis on
+          # `Diff::FileCollection::MergeRequestDiff`.
+          noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+        else
+          original_position.diff_file(self.project.repository)
+        end
+      end
   end
 
   def diff_line

app/services/notes/resolve_service.rb

+module Notes
+  class ResolveService < ::BaseService
+    def execute(note)
+      note.resolve!(current_user)
+
+      ::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
+    end
+  end
+end

app/services/repository_archive_clean_up_service.rb

@@ -20,11 +20,12 @@ class RepositoryArchiveCleanUpService
   private
 
   def clean_up_old_archives
-    run(%W(find #{path} -not -path #{path} -type f \( -name \*.tar -o -name \*.bz2 -o -name \*.tar.gz -o -name \*.zip \) -maxdepth 2 -mmin +#{mmin} -delete))
+    run(%W(find #{path} -mindepth 1 -maxdepth 3 -type f \( -name \*.tar -o -name \*.bz2 -o -name \*.tar.gz -o -name \*.zip \) -mmin +#{mmin} -delete))
   end
 
   def clean_up_empty_directories
-    run(%W(find #{path} -not -path #{path} -type d -empty -name \*.git -maxdepth 1 -delete))
+    run(%W(find #{path} -mindepth 2 -maxdepth 2 -type d -empty -delete))
+    run(%W(find #{path} -mindepth 1 -maxdepth 1 -type d -empty -delete))
   end
 
   def run(cmd)

app/views/projects/branches/_branch.html.haml

@@ -8,18 +8,17 @@
 %li{ class: "branch-item js-branch-#{branch.name}" }
   .branch-info
     .branch-title
-      = link_to project_tree_path(@project, branch.name), class: 'item-title str-truncated-100 ref-name' do
-        = sprite_icon('fork', size: 12)
+      = sprite_icon('fork', size: 12)
+      = link_to project_tree_path(@project, branch.name), class: 'item-title str-truncated-100 ref-name prepend-left-8' do
         = branch.name
-       
       - if branch.name == @repository.root_ref
-        %span.badge.badge-primary default
+        %span.badge.badge-primary.prepend-left-5 default
       - elsif merged
-        %span.badge.badge-info.has-tooltip{ title: s_('Branches|Merged into %{default_branch}') % { default_branch: @repository.root_ref } }
+        %span.badge.badge-info.has-tooltip.prepend-left-5{ title: s_('Branches|Merged into %{default_branch}') % { default_branch: @repository.root_ref } }
           = s_('Branches|merged')
 
       - if protected_branch?(@project, branch)
-        %span.badge.badge-success
+        %span.badge.badge-success.prepend-left-5
           = s_('Branches|protected')
 
     .block-truncated

changelogs/unreleased/dm-commit-trailer-without-gravatar.yml

+---
+title: Fix commit trailer rendering when Gravatar is disabled
+merge_request:
+author:
+type: fixed

changelogs/unreleased/feature-show-only-groups-user-is-member-of-in-dashboard.yml

+---
+title: For group dashboard, we no longer show groups which the visitor is not a member of (this applies to admins and auditors)
+merge_request: 17884
+author: Roger Rüttimann
+type: changed

changelogs/unreleased/fix-inconsistent-protected-branch-pill-baseline.yml

+---
+title: Fixed inconsistent protected branch pill baseline
+merge_request:
+author:
+type: fixed

changelogs/unreleased/helm-add-alpine-mirrors.yml

+---
+title: Increase cluster applications installer availability using alpine linux mirrors
+merge_request:
+author:
+type: performance

changelogs/unreleased/jprovazn-commit-notes-api.yml

+---
+title: Add discussion API for merge requests and commits
+merge_request:
+author:
+type: added

changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml

+---
+title: Use persisted diff data instead fetching Git on discussions
+merge_request:
+author:
+type: performance

changelogs/unreleased/revert-discussion-counter-height.yml

+---
+title: Revert discussion counter height
+merge_request: 18656
+author: George Tsiolis
+type: changed

changelogs/unreleased/security-45689-fix-archive-cache-bug.yml

+---
+title: Serve archive requests with the correct file in all cases
+merge_request:
+author:
+type: security

changelogs/unreleased/security_issue_42029.yml

+---
+title: Sanitizes user name to avoid XSS attacks
+merge_request:
+author:
+type: security

changelogs/unreleased/update-timeline-icon-for-description-edit.yml

+---
+title: Update timeline icon for description edit
+merge_request: 18633
+author: George Tsiolis
+type: changed

doc/api/groups.md

@@ -10,7 +10,7 @@ Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `skip_groups` | array of integers | no | Skip the group IDs passed |
-| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
+| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
 | `search` | string | no | Return the list of authorized groups matching the search criteria |
 | `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
 | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
@@ -94,7 +94,7 @@ Parameters:
 | --------- | ---- | -------- | ----------- |
 | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) of the parent group |
 | `skip_groups` | array of integers | no | Skip the group IDs passed |
-| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
+| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
 | `search` | string | no | Return the list of authorized groups matching the search criteria |
 | `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
 | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |

doc/api/notes.md

@@ -39,7 +39,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
     "system": true,
     "noteable_id": 377,
     "noteable_type": "Issue",
-    "noteable_iid": 377
+    "noteable_iid": 377,
+    "resolvable": false
   },
   {
     "id": 305,
@@ -58,7 +59,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
     "system": true,
     "noteable_id": 121,
     "noteable_type": "Issue",
-    "noteable_iid": 121
+    "noteable_iid": 121,
+    "resolvable": false
   }
 ]
 ```
@@ -314,7 +316,8 @@ Parameters:
   "system": false,
   "noteable_id": 2,
   "noteable_type": "MergeRequest",
-  "noteable_iid": 2
+  "noteable_iid": 2,
+  "resolvable": false
 }
 ```
 

lib/api/discussions.rb

@@ -5,11 +5,12 @@ module API
 
     before { authenticate! }
 
-    NOTEABLE_TYPES = [Issue, Snippet].freeze
+    NOTEABLE_TYPES = [Issue, Snippet, MergeRequest, Commit].freeze
 
     NOTEABLE_TYPES.each do |noteable_type|
       parent_type = noteable_type.parent_class.to_s.underscore
       noteables_str = noteable_type.to_s.underscore.pluralize
+      noteables_path = noteable_type == Commit ? "repository/#{noteables_str}" : noteables_str
 
       params do
         requires :id, type: String, desc: "The ID of a #{parent_type}"
@@ -19,14 +20,12 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           use :pagination
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions" do
+        get ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
           notes = noteable.notes
             .inc_relations_for_view
             .includes(:noteable)
@@ -43,13 +42,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Discussion")
           end
 
@@ -62,19 +61,36 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
+          optional :position, type: Hash do
+            requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
+            requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
+            requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
+            requires :position_type, type: String, desc: 'Type of the position reference', values: %w(text image)
+            optional :new_path, type: String, desc: 'File path after change'
+            optional :new_line, type: Integer, desc: 'Line number after change'
+            optional :old_path, type: String, desc: 'File path before change'
+            optional :old_line, type: Integer, desc: 'Line number before change'
+            optional :width, type: Integer, desc: 'Width of the image'
+            optional :height, type: Integer, desc: 'Height of the image'
+            optional :x, type: Integer, desc: 'X coordinate in the image'
+            optional :y, type: Integer, desc: 'Y coordinate in the image'
+          end
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions" do
+        post ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+          type = params[:position] ? 'DiffNote' : 'DiscussionNote'
+          id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
 
           opts = {
             note: params[:body],
             created_at: params[:created_at],
-            type: 'DiscussionNote',
+            type: type,
             noteable_type: noteables_str.classify,
-            noteable_id: noteable.id
+            position: params[:position],
+            id_key => noteable.id
           }
 
           note = create_note(noteable, opts)
@@ -91,13 +107,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Notes")
           end
 
@@ -108,12 +124,12 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
@@ -139,11 +155,11 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           get_note(noteable, params[:note_id])
@@ -153,30 +169,52 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
-          requires :body, type: String, desc: 'The content of a note'
+          optional :body, type: String, desc: 'The content of a note'
+          optional :resolved, type: Boolean, desc: 'Mark note resolved/unresolved'
+          exactly_one_of :body, :resolved
         end
-        put ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          update_note(noteable, params[:note_id])
+          if params[:resolved].nil?
+            update_note(noteable, params[:note_id])
+          else
+            resolve_note(noteable, params[:note_id], params[:resolved])
+          end
         end
 
         desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        delete ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           delete_note(noteable, params[:note_id])
         end
+
+        if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
+          desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
+            success Entities::Discussion
+          end
+          params do
+            requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
+            requires :discussion_id, type: String, desc: 'The ID of a discussion'
+            requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved'
+          end
+          put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
+            noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+
+            resolve_discussion(noteable, params[:discussion_id], params[:resolved])
+          end
+        end
       end
     end
 

lib/api/entities.rb

@@ -286,6 +286,10 @@ module API
       end
     end
 
+    class DiffRefs < Grape::Entity
+      expose :base_sha, :head_sha, :start_sha
+    end
+
     class Commit < Grape::Entity
       expose :id, :short_id, :title, :created_at
       expose :parent_ids
@@ -601,6 +605,8 @@ module API
         merge_request.metrics&.pipeline
       end
 
+      expose :diff_refs, using: Entities::DiffRefs
+
       def build_available?(options)
         options[:project]&.feature_available?(:builds, options[:current_user])
       end
@@ -642,6 +648,11 @@ module API
       expose :id, :key, :created_at
     end
 
+    class DiffPosition < Grape::Entity
+      expose :base_sha, :start_sha, :head_sha, :old_path, :new_path,
+        :position_type
+    end
+
     class Note < Grape::Entity
       # Only Issue and MergeRequest have iid
       NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
@@ -655,6 +666,14 @@ module API
       expose :system?, as: :system
       expose :noteable_id, :noteable_type
 
+      expose :position, if: ->(note, options) { note.diff_note? } do |note|
+        note.position.to_h
+      end
+
+      expose :resolvable?, as: :resolvable
+      expose :resolved?, as: :resolved, if: ->(note, options) { note.resolvable? }
+      expose :resolved_by, using: Entities::UserBasic, if: ->(note, options) { note.resolvable? }
+
       # Avoid N+1 queries as much as possible
       expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
     end

lib/api/groups.rb

@@ -37,13 +37,11 @@ module API
         use :pagination
       end
 
-      def find_groups(params)
-        find_params = {
-          all_available: params[:all_available],
-          custom_attributes: params[:custom_attributes],
-          owned: params[:owned]
-        }
-        find_params[:parent] = find_group!(params[:id]) if params[:id]
+      def find_groups(params, parent_id = nil)
+        find_params = params.slice(:all_available, :custom_attributes, :owned)
+        find_params[:parent] = find_group!(parent_id) if parent_id
+        find_params[:all_available] =
+          find_params.fetch(:all_available, current_user&.full_private_access?)
 
         groups = GroupsFinder.new(current_user, find_params).execute
         groups = groups.search(params[:search]) if params[:search].present?
@@ -85,7 +83,7 @@ module API
         use :with_custom_attributes
       end
       get do
-        groups = find_groups(params)
+        groups = find_groups(declared_params(include_missing: false), params[:id])
         present_groups params, groups
       end
 
@@ -213,7 +211,7 @@ module API
         use :with_custom_attributes
       end
       get ":id/subgroups" do
-        groups = find_groups(params)
+        groups = find_groups(declared_params(include_missing: false), params[:id])
         present_groups params, groups
       end
 

lib/api/helpers.rb

@@ -171,6 +171,10 @@ module API
       MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
     end
 
+    def find_project_commit(id)
+      user_project.commit_by(oid: id)
+    end
+
     def find_project_snippet(id)
       finder_params = { project: user_project }
       SnippetsFinder.new(current_user, finder_params).find(id)

lib/api/helpers/custom_attributes.rb

@@ -7,6 +7,9 @@ module API
         helpers do
           params :with_custom_attributes do
             optional :with_custom_attributes, type: Boolean, default: false, desc: 'Include custom attributes in the response'
+
+            optional :custom_attributes, type: Hash,
+                                         desc: 'Filter with custom attributes'
           end
 
           def with_custom_attributes(collection_or_resource, options = {})

lib/api/helpers/notes_helpers.rb

@@ -21,6 +21,23 @@ module API
         end
       end
 
+      def resolve_note(noteable, note_id, resolved)
+        note = noteable.notes.find(note_id)
+
+        authorize! :resolve_note, note
+
+        bad_request!("Note is not resolvable") unless note.resolvable?
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Notes::ResolveService.new(parent, current_user).execute(note)
+        else
+          note.unresolve!
+        end
+
+        present note, with: Entities::Note
+      end
+
       def delete_note(noteable, note_id)
         note = noteable.notes.find(note_id)
 
@@ -35,7 +52,7 @@ module API
 
       def get_note(noteable, note_id)
         note = noteable.notes.with_metadata.find(params[:note_id])
-        can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
+        can_read_note = !note.cross_reference_not_visible_for?(current_user)
 
         if can_read_note
           present note, with: Entities::Note
@@ -49,7 +66,20 @@ module API
       end
 
       def find_noteable(parent, noteables_str, noteable_id)
-        public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+        noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+
+        readable =
+          if noteable.is_a?(Commit)
+            # for commits there is not :read_commit policy, check if user
+            # has :read_note permission on the commit's project
+            can?(current_user, :read_note, user_project)
+          else
+            can?(current_user, noteable_read_ability_name(noteable), noteable)
+          end
+
+        return not_found!(noteables_str) unless readable
+
+        noteable
       end
 
       def noteable_parent(noteable)
@@ -57,11 +87,8 @@ module API
       end
 
       def create_note(noteable, opts)
-        noteables_str = noteable.model_name.to_s.underscore.pluralize
-
-        return not_found!(noteables_str) unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
-        authorize! :create_note, noteable
+        policy_object = noteable.is_a?(Commit) ? user_project : noteable
+        authorize!(:create_note, policy_object)
 
         parent = noteable_parent(noteable)
 
@@ -73,6 +100,21 @@ module API
         project = parent if parent.is_a?(Project)
         ::Notes::CreateService.new(project, current_user, opts).execute
       end
+
+      def resolve_discussion(noteable, discussion_id, resolved)
+        discussion = noteable.find_discussion(discussion_id)
+
+        forbidden! unless discussion.can_resolve?(current_user)
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
+        else
+          discussion.unresolve!
+        end
+
+        present discussion, with: Entities::Discussion
+      end
     end
   end
 end

lib/api/notes.rb

@@ -31,23 +31,19 @@ module API
         get ":id/#{noteables_str}/:noteable_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          if can?(current_user, noteable_read_ability_name(noteable), noteable)
-            # We exclude notes that are cross-references and that cannot be viewed
-            # by the current user. By doing this exclusion at this level and not
-            # at the DB query level (which we cannot in that case), the current
-            # page can have less elements than :per_page even if
-            # there's more than one page.
-            raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
-            notes =
-              # paginate() only works with a relation. This could lead to a
-              # mismatch between the pagination headers info and the actual notes
-              # array returned, but this is really a edge-case.
-              paginate(raw_notes)
-              .reject { |n| n.cross_reference_not_visible_for?(current_user) }
-            present notes, with: Entities::Note
-          else
-            not_found!("Notes")
-          end
+          # We exclude notes that are cross-references and that cannot be viewed
+          # by the current user. By doing this exclusion at this level and not
+          # at the DB query level (which we cannot in that case), the current
+          # page can have less elements than :per_page even if
+          # there's more than one page.
+          raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
+          notes =
+            # paginate() only works with a relation. This could lead to a
+            # mismatch between the pagination headers info and the actual notes
+            # array returned, but this is really a edge-case.
+            paginate(raw_notes)
+            .reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          present notes, with: Entities::Note
         end
 
         desc "Get a single #{noteable_type.to_s.downcase} note" do

lib/banzai/filter/commit_trailers_filter.rb

@@ -13,7 +13,6 @@ module Banzai
     # * https://git.wiki.kernel.org/index.php/CommitMessageConventions
     class CommitTrailersFilter < HTML::Pipeline::Filter
       include ActionView::Helpers::TagHelper
-      include ApplicationHelper
       include AvatarsHelper
 
       TRAILER_REGEXP = /(?<label>[[:alpha:]-]+-by:)/i.freeze

lib/gitlab/diff/file_collection/base.rb

@@ -36,6 +36,8 @@ module Gitlab
         private
 
         def decorate_diff!(diff)
+          return diff if diff.is_a?(File)
+
           Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
         end
       end

lib/gitlab/diff/position.rb

@@ -12,6 +12,10 @@ module Gitlab
                :head_sha,
                :old_line,
                :new_line,
+               :width,
+               :height,
+               :x,
+               :y,
                :position_type, to: :formatter
 
       # A position can belong to a text line or to an image coordinate

lib/gitlab/git/raw_diff_change.rb

@@ -38,7 +38,9 @@ module Gitlab
       end
 
       def extract_operation
-        case @raw_operation&.first(1)
+        return :unknown unless @raw_operation
+
+        case @raw_operation[0]
         when 'A'
           :added
         when 'C'

lib/gitlab/git/repository.rb

@@ -391,18 +391,6 @@ module Gitlab
         nil
       end
 
-      def archive_prefix(ref, sha, append_sha:)
-        append_sha = (ref != sha) if append_sha.nil?
-
-        project_name = self.name.chomp('.git')
-        formatted_ref = ref.tr('/', '-')
-
-        prefix_segments = [project_name, formatted_ref]
-        prefix_segments << sha if append_sha
-
-        prefix_segments.join('-')
-      end
-
       def archive_metadata(ref, storage_path, format = "tar.gz", append_sha:)
         ref ||= root_ref
         commit = Gitlab::Git::Commit.find(self, ref)
@@ -413,12 +401,44 @@ module Gitlab
         {
           'RepoPath' => path,
           'ArchivePrefix' => prefix,
-          'ArchivePath' => archive_file_path(prefix, storage_path, format),
+          'ArchivePath' => archive_file_path(storage_path, commit.id, prefix, format),
           'CommitId' => commit.id
         }
       end
 
-      def archive_file_path(name, storage_path, format = "tar.gz")
+      # This is both the filename of the archive (missing the extension) and the
+      # name of the top-level member of the archive under which all files go
+      #
+      # FIXME: The generated prefix is incorrect for projects with hashed
+      # storage enabled
+      def archive_prefix(ref, sha, append_sha:)
+        append_sha = (ref != sha) if append_sha.nil?
+
+        project_name = self.name.chomp('.git')
+        formatted_ref = ref.tr('/', '-')
+
+        prefix_segments = [project_name, formatted_ref]
+        prefix_segments << sha if append_sha
+
+        prefix_segments.join('-')
+      end
+      private :archive_prefix
+
+      # The full path on disk where the archive should be stored. This is used
+      # to cache the archive between requests.
+      #
+      # The path is a global namespace, so needs to be globally unique. This is
+      # achieved by including `gl_repository` in the path.
+      #
+      # Archives relating to a particular ref when the SHA is not present in the
+      # filename must be invalidated when the ref is updated to point to a new
+      # SHA. This is achieved by including the SHA in the path.
+      #
+      # As this is a full path on disk, it is not "cloud native". This should
+      # be resolved by either removing the cache, or moving the implementation
+      # into Gitaly and removing the ArchivePath parameter from the git-archive
+      # senddata response.
+      def archive_file_path(storage_path, sha, name, format = "tar.gz")
         # Build file path
         return nil unless name
 
@@ -436,8 +456,9 @@ module Gitlab
           end
 
         file_name = "#{name}.#{extension}"
-        File.join(storage_path, self.name, file_name)
+        File.join(storage_path, self.gl_repository, sha, file_name)
       end
+      private :archive_file_path
 
       # Return repo size in megabytes
       def size

lib/gitlab/kubernetes/helm/base_command.rb

@@ -15,6 +15,9 @@ module Gitlab
         def generate_script
           <<~HEREDOC
             set -eo pipefail
+            ALPINE_VERSION=$(cat /etc/alpine-release | cut -d '.' -f 1,2)
+            echo http://mirror.clarkson.edu/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+            echo http://mirror1.hs-esslingen.de/pub/Mirrors/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
             apk add -U ca-certificates openssl >/dev/null
             wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v#{Gitlab::Kubernetes::Helm::HELM_VERSION}-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
             mv /tmp/linux-amd64/helm /usr/bin/

spec/features/dashboard/groups_list_spec.rb

@@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do
     expect(page).to have_content(nested_group.name)
   end
 
-  describe 'when filtering groups', :nested_groups do
+  context 'when filtering groups', :nested_groups do
     before do
       group.add_owner(user)
       nested_group.add_owner(user)
@@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do
     end
   end
 
-  describe 'group with subgroups', :nested_groups do
+  context 'with subgroups', :nested_groups do
     let!(:subgroup) { create(:group, :public, parent: group) }
 
     before do
@@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do
     end
   end
 
-  describe 'when using pagination' do
+  context 'when using pagination' do
     let(:group)  { create(:group, created_at: 5.days.ago) }
     let(:group2) { create(:group, created_at: 2.days.ago) }
 
@@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do
       expect(page).not_to have_selector("#group-#{group2.id}")
     end
   end
+
+  context 'when signed in as admin' do
+    let(:admin) { create(:admin) }
+
+    it 'shows only groups admin is member of' do
+      group.add_owner(admin)
+      expect(another_group).to be_persisted
+
+      sign_in(admin)
+      visit dashboard_groups_path
+      wait_for_requests
+
+      expect(page).to have_content(group.name)
+      expect(page).not_to have_content(another_group.name)
+    end
+  end
 end

spec/features/projects/issues/user_toggles_subscription_spec.rb

 require "spec_helper"
 
 describe "User toggles subscription", :js do
-  set(:project) { create(:project_empty_repo, :public) }
-  set(:user) { create(:user) }
-  set(:issue) { create(:issue, project: project, author: user) }
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:user) { create(:user) }
+  let(:issue) { create(:issue, project: project, author: user) }
 
   before do
     project.add_developer(user)
@@ -12,7 +12,7 @@ describe "User toggles subscription", :js do
     visit(project_issue_path(project, issue))
   end
 
-  it "unsibscribes from issue" do
+  it "unsubscribes from issue" do
     subscription_button = find(".js-issuable-subscribe-button")
 
     # Check we're subscribed.

spec/finders/groups_finder_spec.rb

@@ -2,43 +2,71 @@ require 'spec_helper'
 
 describe GroupsFinder do
   describe '#execute' do
-    let(:user)            { create(:user) }
-
-    context 'root level groups' do
-      let!(:private_group)  { create(:group, :private) }
-      let!(:internal_group) { create(:group, :internal) }
-      let!(:public_group)   { create(:group, :public) }
-
-      context 'without a user' do
-        subject { described_class.new.execute }
-
-        it { is_expected.to eq([public_group]) }
+    let(:user) { create(:user) }
+
+    describe 'root level groups' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:user_type, :params, :results) do
+        nil | { all_available: true  } | %i(public_group user_public_group)
+        nil | { all_available: false  } | %i(public_group user_public_group)
+        nil | {} | %i(public_group user_public_group)
+
+        :regular | { all_available: true  } | %i(public_group internal_group user_public_group user_internal_group
+                                                 user_private_group)
+        :regular | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :regular | {} | %i(public_group internal_group user_public_group user_internal_group user_private_group)
+
+        :external | { all_available: true  } | %i(public_group user_public_group user_internal_group user_private_group)
+        :external | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :external | {} | %i(public_group user_public_group user_internal_group user_private_group)
+
+        :admin | { all_available: true  } | %i(public_group internal_group private_group user_public_group
+                                               user_internal_group user_private_group)
+        :admin | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :admin | {} | %i(public_group internal_group private_group user_public_group user_internal_group
+                         user_private_group)
       end
 
-      context 'with a user' do
-        subject { described_class.new(user).execute }
-
-        context 'normal user' do
-          it { is_expected.to contain_exactly(public_group, internal_group) }
-        end
-
-        context 'external user' do
-          let(:user) { create(:user, external: true) }
-
-          it { is_expected.to contain_exactly(public_group) }
+      with_them do
+        before do
+          # Fixme: Because of an issue: https://github.com/tomykaira/rspec-parameterized/issues/8#issuecomment-381888428
+          # The groups need to be created here, not with let syntax, and also compared by name and not ids
+
+          @groups = {
+            private_group: create(:group, :private, name: 'private_group'),
+            internal_group: create(:group, :internal, name: 'internal_group'),
+            public_group: create(:group, :public, name: 'public_group'),
+
+            user_private_group: create(:group, :private, name: 'user_private_group'),
+            user_internal_group: create(:group, :internal, name: 'user_internal_group'),
+            user_public_group: create(:group, :public, name: 'user_public_group')
+          }
+
+          if user_type
+            user =
+              case user_type
+              when :regular
+                create(:user)
+              when :external
+                create(:user, external: true)
+              when :admin
+                create(:user, :admin)
+              end
+            @groups.values_at(:user_private_group, :user_internal_group, :user_public_group).each do |group|
+              group.add_developer(user)
+            end
+          end
         end
 
-        context 'user is member of the private group' do
-          before do
-            private_group.add_guest(user)
-          end
+        subject { described_class.new(User.last, params).execute.to_a }
 
-          it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
-        end
+        it { is_expected.to match_array(@groups.values_at(*results)) }
       end
     end
 
     context 'subgroups', :nested_groups do
+      let(:user) { create(:user) }
       let!(:parent_group) { create(:group, :public) }
       let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
       let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }

spec/fixtures/api/schemas/public_api/v4/notes.json

@@ -24,7 +24,10 @@
       "system": { "type": "boolean" },
       "noteable_id": { "type": "integer" },
       "noteable_iid": { "type": "integer" },
-      "noteable_type": { "type": "string" }
+      "noteable_type": { "type": "string" },
+      "resolved": { "type": "boolean" },
+      "resolvable": { "type": "boolean" },
+      "resolved_by": { "type": ["string", "null"] }
     },
     "required": [
       "id", "body", "attachment", "author", "created_at", "updated_at",

spec/helpers/application_helper_spec.rb

@@ -2,8 +2,6 @@
 require 'spec_helper'
 
 describe ApplicationHelper do
-  include UploadHelpers
-
   describe 'current_controller?' do
     it 'returns true when controller matches argument' do
       stub_controller_name('foo')
@@ -54,143 +52,6 @@ describe ApplicationHelper do
     end
   end
 
-  describe 'project_icon' do
-    it 'returns an url for the avatar' do
-      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
-
-      expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-    end
-  end
-
-  describe 'avatar_icon_for' do
-    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
-    let(:email) { 'foo@example.com' }
-    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
-
-    it 'prefers the user to retrieve the avatar_url' do
-      expect(helper.avatar_icon_for(user, email).to_s)
-        .to eq(user.avatar.url)
-    end
-
-    it 'falls back to email lookup if no user given' do
-      expect(helper.avatar_icon_for(nil, email).to_s)
-        .to eq(another_user.avatar.url)
-    end
-  end
-
-  describe 'avatar_icon_for_email' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'using an email' do
-      context 'when there is a matching user' do
-        it 'returns a relative URL for the avatar' do
-          expect(helper.avatar_icon_for_email(user.email).to_s)
-            .to eq(user.avatar.url)
-        end
-      end
-
-      context 'when no user exists for the email' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
-
-          helper.avatar_icon_for_email('foo@example.com', 20, 2)
-        end
-      end
-
-      context 'without an email passed' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-          helper.avatar_icon_for_email(nil, 20, 2)
-        end
-      end
-    end
-  end
-
-  describe 'avatar_icon_for_user' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'with a user object passed' do
-      it 'returns a relative URL for the avatar' do
-        expect(helper.avatar_icon_for_user(user).to_s)
-          .to eq(user.avatar.url)
-      end
-    end
-
-    context 'without a user object passed' do
-      it 'calls gravatar_icon' do
-        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-        helper.avatar_icon_for_user(nil, 20, 2)
-      end
-    end
-  end
-
-  describe 'gravatar_icon' do
-    let(:user_email) { 'user@email.com' }
-
-    context 'with Gravatar disabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: false)
-      end
-
-      it 'returns a generic avatar' do
-        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
-      end
-    end
-
-    context 'with Gravatar enabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: true)
-      end
-
-      it 'returns a generic avatar when email is blank' do
-        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
-      end
-
-      it 'returns a valid Gravatar URL' do
-        stub_config_setting(https: false)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'uses HTTPs when configured' do
-        stub_config_setting(https: true)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://secure.gravatar.com')
-      end
-
-      it 'returns custom gravatar path when gravatar_url is set' do
-        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
-
-        expect(gravatar_icon(user_email, 20))
-          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'accepts a custom size argument' do
-        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
-      end
-
-      it 'defaults size to 40@2x when given an invalid size' do
-        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
-      end
-
-      it 'accepts a scaling factor' do
-        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
-      end
-
-      it 'ignores case and surrounding whitespace' do
-        normal = helper.gravatar_icon('foo@example.com')
-        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
-
-        expect(normal).to eq upcase
-      end
-    end
-  end
-
   describe 'simple_sanitize' do
     let(:a_tag) { '<a href="#">Foo</a>' }
 

spec/helpers/auth_helper_spec.rb

@@ -18,6 +18,30 @@ describe AuthHelper do
     end
   end
 
+  describe "providers_for_base_controller" do
+    it 'returns all enabled providers from devise' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :github] }
+      expect(helper.providers_for_base_controller).to include(*[:twitter, :github])
+    end
+
+    it 'excludes ldap providers' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      expect(helper.providers_for_base_controller).not_to include(:ldapmain)
+    end
+  end
+
+  describe "form_based_providers" do
+    it 'includes LDAP providers' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      expect(helper.form_based_providers).to eq %i(ldapmain)
+    end
+
+    it 'includes crowd provider' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :crowd] }
+      expect(helper.form_based_providers).to eq %i(crowd)
+    end
+  end
+
   describe 'enabled_button_based_providers' do
     before do
       allow(helper).to receive(:auth_providers) { [:twitter, :github] }

spec/helpers/avatars_helper_spec.rb

 require 'rails_helper'
 
 describe AvatarsHelper do
-  include ApplicationHelper
+  include UploadHelpers
 
   let(:user) { create(:user) }
 
+  describe '#project_icon' do
+    it 'returns an url for the avatar' do
+      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
+
+      expect(helper.project_icon(project.full_path).to_s)
+        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
+    end
+  end
+
+  describe '#avatar_icon_for' do
+    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
+    let(:email) { 'foo@example.com' }
+    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
+
+    it 'prefers the user to retrieve the avatar_url' do
+      expect(helper.avatar_icon_for(user, email).to_s)
+        .to eq(user.avatar.url)
+    end
+
+    it 'falls back to email lookup if no user given' do
+      expect(helper.avatar_icon_for(nil, email).to_s)
+        .to eq(another_user.avatar.url)
+    end
+  end
+
+  describe '#avatar_icon_for_email' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'using an email' do
+      context 'when there is a matching user' do
+        it 'returns a relative URL for the avatar' do
+          expect(helper.avatar_icon_for_email(user.email).to_s)
+            .to eq(user.avatar.url)
+        end
+      end
+
+      context 'when no user exists for the email' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
+
+          helper.avatar_icon_for_email('foo@example.com', 20, 2)
+        end
+      end
+
+      context 'without an email passed' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+          helper.avatar_icon_for_email(nil, 20, 2)
+        end
+      end
+    end
+  end
+
+  describe '#avatar_icon_for_user' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'with a user object passed' do
+      it 'returns a relative URL for the avatar' do
+        expect(helper.avatar_icon_for_user(user).to_s)
+          .to eq(user.avatar.url)
+      end
+    end
+
+    context 'without a user object passed' do
+      it 'calls gravatar_icon' do
+        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+        helper.avatar_icon_for_user(nil, 20, 2)
+      end
+    end
+  end
+
+  describe '#gravatar_icon' do
+    let(:user_email) { 'user@email.com' }
+
+    context 'with Gravatar disabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: false)
+      end
+
+      it 'returns a generic avatar' do
+        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
+      end
+    end
+
+    context 'with Gravatar enabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: true)
+      end
+
+      it 'returns a generic avatar when email is blank' do
+        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
+      end
+
+      it 'returns a valid Gravatar URL' do
+        stub_config_setting(https: false)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'uses HTTPs when configured' do
+        stub_config_setting(https: true)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://secure.gravatar.com')
+      end
+
+      it 'returns custom gravatar path when gravatar_url is set' do
+        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
+
+        expect(gravatar_icon(user_email, 20))
+          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'accepts a custom size argument' do
+        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
+      end
+
+      it 'defaults size to 40@2x when given an invalid size' do
+        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
+      end
+
+      it 'accepts a scaling factor' do
+        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
+      end
+
+      it 'ignores case and surrounding whitespace' do
+        normal = helper.gravatar_icon('foo@example.com')
+        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
+
+        expect(normal).to eq upcase
+      end
+    end
+  end
+
   describe '#user_avatar' do
     subject { helper.user_avatar(user: user) }
 

spec/javascripts/sidebar/mock_data.js

@@ -138,7 +138,7 @@ const RESPONSE_MAP = {
       },
       {
         id: 20,
-        name_with_namespace: 'foo / bar',
+        name_with_namespace: '<img src=x onerror=alert(document.domain)> foo / bar',
       },
     ],
   },

spec/javascripts/sidebar/sidebar_move_issue_spec.js

@@ -71,6 +71,15 @@ describe('SidebarMoveIssue', function () {
 
       expect($.fn.glDropdown).toHaveBeenCalled();
     });
+
+    it('escapes html from project name', (done) => {
+      this.$toggleButton.dropdown('toggle');
+
+      setTimeout(() => {
+        expect(this.$content.find('.js-move-issue-dropdown-item')[1].innerHTML.trim()).toEqual('<img src=x onerror=alert(document.domain)> foo / bar');
+        done();
+      });
+    });
   });
 
   describe('onConfirmClicked', () => {

spec/lib/banzai/filter/commit_trailers_filter_spec.rb

@@ -47,16 +47,36 @@ describe Banzai::Filter::CommitTrailersFilter do
       )
     end
 
-    it 'non GitLab users and replaces them with mailto links' do
-      _, message_html = build_commit_message(
-        trailer: trailer,
-        name: FFaker::Name.name,
-        email: email
-      )
+    context 'non GitLab users' do
+      shared_examples 'mailto links' do
+        it 'replaces them with mailto links' do
+          _, message_html = build_commit_message(
+            trailer: trailer,
+            name: FFaker::Name.name,
+            email: email
+          )
 
-      doc = filter(message_html)
+          doc = filter(message_html)
 
-      expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
+        end
+      end
+
+      context 'when Gravatar is disabled' do
+        before do
+          stub_application_setting(gravatar_enabled: false)
+        end
+
+        it_behaves_like 'mailto links'
+      end
+
+      context 'when Gravatar is enabled' do
+        before do
+          stub_application_setting(gravatar_enabled: true)
+        end
+
+        it_behaves_like 'mailto links'
+      end
     end
 
     it 'multiple trailers in the same message' do
@@ -69,7 +89,7 @@ describe Banzai::Filter::CommitTrailersFilter do
       doc = filter(message)
 
       expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer)
-      expect_to_have_mailto_link(doc, email: email, trailer: different_trailer)
+      expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: different_trailer)
     end
 
     context 'special names' do
@@ -90,7 +110,7 @@ describe Banzai::Filter::CommitTrailersFilter do
 
           doc = filter(message_html)
 
-          expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
           expect(doc.text).to match Regexp.escape(message)
         end
       end

spec/lib/gitlab/git/repository_spec.rb

@@ -234,59 +234,72 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names
   end
 
-  shared_examples 'archive check' do |extenstion|
-    it { expect(metadata['ArchivePath']).to match(%r{tmp/gitlab-git-test.git/gitlab-git-test-master-#{SeedRepo::LastCommit::ID}}) }
-    it { expect(metadata['ArchivePath']).to end_with extenstion }
-  end
+  describe '#archive_metadata' do
+    let(:storage_path) { '/tmp' }
+    let(:cache_key) { File.join(repository.gl_repository, SeedRepo::LastCommit::ID) }
 
-  describe '#archive_prefix' do
-    let(:project_name) { 'project-name'}
+    let(:append_sha) { true }
+    let(:ref) { 'master' }
+    let(:format) { nil }
 
-    before do
-      expect(repository).to receive(:name).once.and_return(project_name)
-    end
+    let(:expected_extension) { 'tar.gz' }
+    let(:expected_filename) { "#{expected_prefix}.#{expected_extension}" }
+    let(:expected_path) { File.join(storage_path, cache_key, expected_filename) }
+    let(:expected_prefix) { "gitlab-git-test-#{ref}-#{SeedRepo::LastCommit::ID}" }
 
-    it 'returns parameterised string for a ref containing slashes' do
-      prefix = repository.archive_prefix('test/branch', 'SHA', append_sha: nil)
+    subject(:metadata) { repository.archive_metadata(ref, storage_path, format, append_sha: append_sha) }
 
-      expect(prefix).to eq("#{project_name}-test-branch-SHA")
+    it 'sets RepoPath to the repository path' do
+      expect(metadata['RepoPath']).to eq(repository.path)
     end
 
-    it 'returns correct string for a ref containing dots' do
-      prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: nil)
-
-      expect(prefix).to eq("#{project_name}-test.branch-SHA")
+    it 'sets CommitId to the commit SHA' do
+      expect(metadata['CommitId']).to eq(SeedRepo::LastCommit::ID)
     end
 
-    it 'returns string with sha when append_sha is false' do
-      prefix = repository.archive_prefix('test.branch', 'SHA', append_sha: false)
-
-      expect(prefix).to eq("#{project_name}-test.branch")
+    it 'sets ArchivePrefix to the expected prefix' do
+      expect(metadata['ArchivePrefix']).to eq(expected_prefix)
     end
-  end
 
-  describe '#archive' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', append_sha: true) }
+    it 'sets ArchivePath to the expected globally-unique path' do
+      # This is really important from a security perspective. Think carefully
+      # before changing it: https://gitlab.com/gitlab-org/gitlab-ce/issues/45689
+      expect(expected_path).to include(File.join(repository.gl_repository, SeedRepo::LastCommit::ID))
 
-    it_should_behave_like 'archive check', '.tar.gz'
-  end
-
-  describe '#archive_zip' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'zip', append_sha: true) }
+      expect(metadata['ArchivePath']).to eq(expected_path)
+    end
 
-    it_should_behave_like 'archive check', '.zip'
-  end
+    context 'append_sha varies archive path and filename' do
+      where(:append_sha, :ref, :expected_prefix) do
+        sha = SeedRepo::LastCommit::ID
 
-  describe '#archive_bz2' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'tbz2', append_sha: true) }
+        true  | 'master' | "gitlab-git-test-master-#{sha}"
+        true  | sha      | "gitlab-git-test-#{sha}-#{sha}"
+        false | 'master' | "gitlab-git-test-master"
+        false | sha      | "gitlab-git-test-#{sha}"
+        nil   | 'master' | "gitlab-git-test-master-#{sha}"
+        nil   | sha      | "gitlab-git-test-#{sha}"
+      end
 
-    it_should_behave_like 'archive check', '.tar.bz2'
-  end
+      with_them do
+        it { expect(metadata['ArchivePrefix']).to eq(expected_prefix) }
+        it { expect(metadata['ArchivePath']).to eq(expected_path) }
+      end
+    end
 
-  describe '#archive_fallback' do
-    let(:metadata) { repository.archive_metadata('master', '/tmp', 'madeup', append_sha: true) }
+    context 'format varies archive path and filename' do
+      where(:format, :expected_extension) do
+        nil      | 'tar.gz'
+        'madeup' | 'tar.gz'
+        'tbz2'   | 'tar.bz2'
+        'zip'    | 'zip'
+      end
 
-    it_should_behave_like 'archive check', '.tar.gz'
+      with_them do
+        it { expect(metadata['ArchivePrefix']).to eq(expected_prefix) }
+        it { expect(metadata['ArchivePath']).to eq(expected_path) }
+      end
+    end
   end
 
   describe '#size' do

spec/lib/gitlab/kubernetes/helm/base_command_spec.rb

@@ -4,22 +4,10 @@ describe Gitlab::Kubernetes::Helm::BaseCommand do
   let(:application) { create(:clusters_applications_helm) }
   let(:base_command) { described_class.new(application.name) }
 
-  describe '#generate_script' do
-    let(:helm_version) { Gitlab::Kubernetes::Helm::HELM_VERSION }
-    let(:command) do
-      <<~HEREDOC
-         set -eo pipefail
-         apk add -U ca-certificates openssl >/dev/null
-         wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v#{helm_version}-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-         mv /tmp/linux-amd64/helm /usr/bin/
-      HEREDOC
-    end
-
-    subject { base_command.generate_script }
+  subject { base_command }
 
-    it 'should return a command that prepares the environment for helm-cli' do
-      expect(subject).to eq(command)
-    end
+  it_behaves_like 'helm commands' do
+    let(:commands) { '' }
   end
 
   describe '#pod_resource' do

spec/lib/gitlab/kubernetes/helm/init_command_spec.rb

@@ -2,23 +2,9 @@ require 'spec_helper'
 
 describe Gitlab::Kubernetes::Helm::InitCommand do
   let(:application) { create(:clusters_applications_helm) }
-  let(:init_command) { described_class.new(application.name) }
+  let(:commands) { 'helm init >/dev/null' }
 
-  describe '#generate_script' do
-    let(:command) do
-      <<~MSG.chomp
-        set -eo pipefail
-        apk add -U ca-certificates openssl >/dev/null
-        wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-        mv /tmp/linux-amd64/helm /usr/bin/
-        helm init >/dev/null
-      MSG
-    end
+  subject { described_class.new(application.name) }
 
-    subject { init_command.generate_script }
-
-    it 'should return the appropriate command' do
-      is_expected.to eq(command)
-    end
-  end
+  it_behaves_like 'helm commands'
 end

spec/lib/gitlab/kubernetes/helm/install_command_spec.rb

@@ -12,50 +12,36 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do
     )
   end
 
-  describe '#generate_script' do
-    let(:command) do
-      <<~MSG
-      set -eo pipefail
-      apk add -U ca-certificates openssl >/dev/null
-      wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-      mv /tmp/linux-amd64/helm /usr/bin/
-      helm init --client-only >/dev/null
-      helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
-      MSG
-    end
-
-    subject { install_command.generate_script }
+  subject { install_command }
 
-    it 'should return appropriate command' do
-      is_expected.to eq(command)
+  it_behaves_like 'helm commands' do
+    let(:commands) do
+      <<~EOS
+         helm init --client-only >/dev/null
+         helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
+      EOS
     end
+  end
 
-    context 'with an application with a repository' do
-      let(:ci_runner) { create(:ci_runner) }
-      let(:application) { create(:clusters_applications_runner, runner: ci_runner) }
-      let(:install_command) do
-        described_class.new(
-          application.name,
-          chart: application.chart,
-          values: application.values,
-          repository: application.repository
-        )
-      end
-
-      let(:command) do
-        <<~MSG
-        set -eo pipefail
-        apk add -U ca-certificates openssl >/dev/null
-        wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-        mv /tmp/linux-amd64/helm /usr/bin/
-        helm init --client-only >/dev/null
-        helm repo add #{application.name} #{application.repository}
-        helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
-        MSG
-      end
+  context 'with an application with a repository' do
+    let(:ci_runner) { create(:ci_runner) }
+    let(:application) { create(:clusters_applications_runner, runner: ci_runner) }
+    let(:install_command) do
+      described_class.new(
+        application.name,
+        chart: application.chart,
+        values: application.values,
+        repository: application.repository
+      )
+    end
 
-      it 'should return appropriate command' do
-        is_expected.to eq(command)
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+           helm init --client-only >/dev/null
+           helm repo add #{application.name} #{application.repository}
+           helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
+        EOS
       end
     end
   end

spec/models/diff_note_spec.rb

@@ -85,12 +85,35 @@ describe DiffNote do
   end
 
   describe "#diff_file" do
-    it "returns the correct diff file" do
-      diff_file = subject.diff_file
+    context 'when the discussion was created in the diff' do
+      it 'returns correct diff file' do
+        diff_file = subject.diff_file
 
-      expect(diff_file.old_path).to eq(position.old_path)
-      expect(diff_file.new_path).to eq(position.new_path)
-      expect(diff_file.diff_refs).to eq(position.diff_refs)
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
+    end
+
+    context 'when discussion is outdated or not created in the diff' do
+      let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+      let(:position) do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: 14,
+          diff_refs: diff_refs
+        )
+      end
+
+      it 'returns the correct diff file' do
+        diff_file = subject.diff_file
+
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
     end
   end
 

spec/requests/api/discussions_spec.rb

@@ -2,32 +2,53 @@ require 'spec_helper'
 
 describe API::Discussions do
   let(:user) { create(:user) }
-  let!(:project) { create(:project, :public, namespace: user.namespace) }
+  let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
   let(:private_user)    { create(:user) }
 
   before do
-    project.add_reporter(user)
+    project.add_developer(user)
   end
 
-  context "when noteable is an Issue" do
+  context 'when noteable is an Issue' do
     let!(:issue) { create(:issue, project: project, author: user) }
     let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) }
 
-    it_behaves_like "discussions API", 'projects', 'issues', 'iid' do
+    it_behaves_like 'discussions API', 'projects', 'issues', 'iid' do
       let(:parent) { project }
       let(:noteable) { issue }
       let(:note) { issue_note }
     end
   end
 
-  context "when noteable is a Snippet" do
+  context 'when noteable is a Snippet' do
     let!(:snippet) { create(:project_snippet, project: project, author: user) }
     let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) }
 
-    it_behaves_like "discussions API", 'projects', 'snippets', 'id' do
+    it_behaves_like 'discussions API', 'projects', 'snippets', 'id' do
       let(:parent) { project }
       let(:noteable) { snippet }
       let(:note) { snippet_note }
     end
   end
+
+  context 'when noteable is a Merge Request' do
+    let!(:noteable) { create(:merge_request_with_diffs, source_project: project, target_project: project, author: user) }
+    let!(:note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, author: user) }
+    let!(:diff_note) { create(:diff_note_on_merge_request, noteable: noteable, project: project, author: user) }
+    let(:parent) { project }
+
+    it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid'
+    it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid'
+    it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid'
+  end
+
+  context 'when noteable is a Commit' do
+    let!(:noteable) { create(:commit, project: project, author: user) }
+    let!(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user) }
+    let!(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user) }
+    let(:parent) { project }
+
+    it_behaves_like 'discussions API', 'projects', 'repository/commits', 'id'
+    it_behaves_like 'diff discussions API', 'projects', 'repository/commits', 'id'
+  end
 end

spec/services/notes/resolve_service_spec.rb

+require 'spec_helper'
+
+describe Notes::ResolveService do
+  let(:merge_request) { create(:merge_request) }
+  let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) }
+  let(:user) { merge_request.author }
+
+  describe '#execute' do
+    it "resolves the note" do
+      described_class.new(merge_request.project, user).execute(note)
+      note.reload
+
+      expect(note.resolved?).to be true
+      expect(note.resolved_by).to eq(user)
+    end
+
+    it "sends notifications if all discussions are resolved" do
+      expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
+
+      described_class.new(merge_request.project, user).execute(note)
+    end
+  end
+end

spec/services/repository_archive_clean_up_service_spec.rb

 require 'spec_helper'
 
 describe RepositoryArchiveCleanUpService do
-  describe '#execute' do
-    subject(:service) { described_class.new }
+  subject(:service) { described_class.new }
 
+  describe '#execute (new archive locations)' do
+    let(:sha) { "0" * 40 }
+
+    it 'removes outdated archives and directories in a new-style path' do
+      in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 3.hours) do |dirname, files|
+        service.execute
+
+        files.each { |filename| expect(File.exist?(filename)).to be_falsy }
+        expect(File.directory?(dirname)).to be_falsy
+        expect(File.directory?(File.dirname(dirname))).to be_falsy
+      end
+    end
+
+    it 'does not remove directories when they contain outdated non-archives' do
+      in_directory_with_files("project-999/#{sha}", %w[tar conf rb], 2.hours) do |dirname, files|
+        service.execute
+
+        expect(File.directory?(dirname)).to be_truthy
+      end
+    end
+
+    it 'does not remove in-date archives in a new-style path' do
+      in_directory_with_files("project-999/#{sha}", %w[tar tar.bz2 tar.gz zip], 1.hour) do |dirname, files|
+        service.execute
+
+        files.each { |filename| expect(File.exist?(filename)).to be_truthy }
+      end
+    end
+  end
+
+  describe '#execute (legacy archive locations)' do
     context 'when the downloads directory does not exist' do
       it 'does not remove any archives' do
         path = '/invalid/path/'
         stub_repository_downloads_path(path)
 
+        allow(File).to receive(:directory?).and_call_original
         expect(File).to receive(:directory?).with(path).and_return(false)
+
         expect(service).not_to receive(:clean_up_old_archives)
         expect(service).not_to receive(:clean_up_empty_directories)
 
@@ -19,7 +51,7 @@ describe RepositoryArchiveCleanUpService do
 
     context 'when the downloads directory exists' do
       shared_examples 'invalid archive files' do |dirname, extensions, mtime|
-        it 'does not remove files and directoy' do
+        it 'does not remove files and directory' do
           in_directory_with_files(dirname, extensions, mtime) do |dir, files|
             service.execute
 
@@ -43,7 +75,7 @@ describe RepositoryArchiveCleanUpService do
       end
 
       context 'with files older than 2 hours inside invalid directories' do
-        it_behaves_like 'invalid archive files', 'john_doe/sample.git', %w[conf rb tar tar.gz], 2.hours
+        it_behaves_like 'invalid archive files', 'john/doe/sample.git', %w[conf rb tar tar.gz], 2.hours
       end
 
       context 'with files newer than 2 hours that matches valid archive extensions' do
@@ -58,24 +90,24 @@ describe RepositoryArchiveCleanUpService do
         it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb tar tar.gz], 1.hour
       end
     end
+  end
 
-    def in_directory_with_files(dirname, extensions, mtime)
-      Dir.mktmpdir do |tmpdir|
-        stub_repository_downloads_path(tmpdir)
-        dir = File.join(tmpdir, dirname)
-        files = create_temporary_files(dir, extensions, mtime)
+  def in_directory_with_files(dirname, extensions, mtime)
+    Dir.mktmpdir do |tmpdir|
+      stub_repository_downloads_path(tmpdir)
+      dir = File.join(tmpdir, dirname)
+      files = create_temporary_files(dir, extensions, mtime)
 
-        yield(dir, files)
-      end
+      yield(dir, files)
     end
+  end
 
-    def stub_repository_downloads_path(path)
-      allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path)
-    end
+  def stub_repository_downloads_path(path)
+    allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path)
+  end
 
-    def create_temporary_files(dir, extensions, mtime)
-      FileUtils.mkdir_p(dir)
-      FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime)
-    end
+  def create_temporary_files(dir, extensions, mtime)
+    FileUtils.mkdir_p(dir)
+    FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime)
   end
 end

spec/support/commit_trailers_spec_helper.rb

@@ -8,7 +8,7 @@ module CommitTrailersSpecHelper
     expect(wrapper.attribute('data-user').value).to eq user.id.to_s
   end
 
-  def expect_to_have_mailto_link(doc, email:, trailer:)
+  def expect_to_have_mailto_link_with_avatar(doc, email:, trailer:)
     wrapper = find_user_wrapper(doc, trailer)
 
     expect_to_have_links_with_url_and_avatar(wrapper, "mailto:#{CGI.escape_html(email)}", email)

spec/support/shared_examples/helm_generated_script.rb

+shared_examples 'helm commands' do
+  describe '#generate_script' do
+    let(:helm_setup) do
+      <<~EOS
+         set -eo pipefail
+         ALPINE_VERSION=$(cat /etc/alpine-release | cut -d '.' -f 1,2)
+         echo http://mirror.clarkson.edu/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+         echo http://mirror1.hs-esslingen.de/pub/Mirrors/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+         apk add -U ca-certificates openssl >/dev/null
+         wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
+         mv /tmp/linux-amd64/helm /usr/bin/
+      EOS
+    end
+
+    it 'should return appropriate command' do
+      expect(subject.generate_script).to eq(helm_setup + commands)
+    end
+  end
+end

spec/support/shared_examples/requests/api/diff_discussions.rb

+shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name|
+  describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
+    it "includes diff discussions" do
+      get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user)
+
+      discussion = json_response.find { |record| record['id'] == diff_note.discussion_id }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(discussion).not_to be_nil
+      expect(discussion['individual_note']).to eq(false)
+      expect(discussion['notes'].first['body']).to eq(diff_note.note)
+    end
+  end
+
+  describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
+    it "returns a discussion by id" do
+      get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['id']).to eq(diff_note.discussion_id)
+      expect(json_response['notes'].first['body']).to eq(diff_note.note)
+      expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys)
+    end
+  end
+
+  describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
+    it "creates a new diff note" do
+      position = diff_note.position.to_h
+
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['notes'].first['body']).to eq('hi!')
+      expect(json_response['notes'].first['type']).to eq('DiffNote')
+      expect(json_response['notes'].first['position']).to eq(position.stringify_keys)
+    end
+
+    it "returns a 400 bad request error when position is invalid" do
+      position = diff_note.position.to_h.merge(new_line: '100000')
+
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+  end
+
+  describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do
+    it 'adds a new note to the diff discussion' do
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+               "discussions/#{diff_note.discussion_id}/notes", user), body: 'hi!'
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['body']).to eq('hi!')
+      expect(json_response['type']).to eq('DiffNote')
+    end
+  end
+end

spec/support/shared_examples/requests/api/resolvable_discussions.rb

+shared_examples 'resolvable discussions API' do |parent_type, noteable_type, id_name|
+  describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
+    it "resolves discussion if resolved is true" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user), resolved: true
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['notes'].size).to eq(1)
+      expect(json_response['notes'][0]['resolved']).to eq(true)
+    end
+
+    it "unresolves discussion if resolved is false" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user), resolved: false
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['notes'].size).to eq(1)
+      expect(json_response['notes'][0]['resolved']).to eq(false)
+    end
+
+    it "returns a 400 bad request error if resolved parameter is not passed" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user)
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+
+    it "returns a 401 unauthorized error if user is not authenticated" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}"), resolved: true
+
+      expect(response).to have_gitlab_http_status(401)
+    end
+
+    it "returns a 403 error if user resolves discussion of someone else" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", private_user), resolved: true
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+
+    context 'when user does not have access to read the discussion' do
+      before do
+        parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'responds with 404' do
+        put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+                "discussions/#{note.discussion_id}", private_user), resolved: true
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
+  describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+    it 'returns resolved note when resolved parameter is true' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", user), resolved: true
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['resolved']).to eq(true)
+    end
+
+    it 'returns a 404 error when note id not found' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/12345", user),
+              body: 'Hello!'
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    it 'returns a 400 bad request error if neither body nor resolved parameter is given' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", user)
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+
+    it "returns a 403 error if user resolves note of someone else" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", private_user), resolved: true
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+  end
+end