Merge branch 'master' of https://gitlab.com/gitlab-org/gitlab-ee into...

Merge branch 'master' of https://gitlab.com/gitlab-org/gitlab-ee into ee-projects-tree-tags-dispatcher-refactor

app/models/repository.rb

@@ -838,13 +838,12 @@ class Repository
   end
 
   def can_be_merged?(source_sha, target_branch)
-    our_commit = rugged.branches[target_branch].target
-    their_commit = rugged.lookup(source_sha)
-
-    if our_commit && their_commit
-      !rugged.merge_commits(our_commit, their_commit).conflicts?
-    else
-      false
+    raw_repository.gitaly_migrate(:can_be_merged) do |is_enabled|
+      if is_enabled
+        gitaly_can_be_merged?(source_sha, find_branch(target_branch).target)
+      else
+        rugged_can_be_merged?(source_sha, target_branch)
+      end
     end
   end
 
@@ -1187,6 +1186,14 @@ class Repository
     Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, is_wiki))
   end
 
+  def gitaly_can_be_merged?(their_commit, our_commit)
+    !raw_repository.gitaly_conflicts_client(our_commit, their_commit).conflicts?
+  end
+
+  def rugged_can_be_merged?(their_commit, our_commit)
+    !rugged.merge_commits(our_commit, their_commit).conflicts?
+  end
+
   def find_commits_by_message_by_shelling_out(query, ref, path, limit, offset)
     ref ||= root_ref
 

app/models/user.rb

@@ -55,7 +55,10 @@ class User < ActiveRecord::Base
   serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize
 
   devise :lockable, :recoverable, :rememberable, :trackable,
-    :validatable, :omniauthable, :confirmable, :registerable
+         :validatable, :omniauthable, :confirmable, :registerable
+
+  BLOCKED_MESSAGE = "Your account has been blocked. Please contact your GitLab " \
+                    "administrator if you think this is an error.".freeze
 
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
@@ -221,8 +224,7 @@ class User < ActiveRecord::Base
       end
 
       def inactive_message
-        "Your account has been blocked. Please contact your GitLab " \
-          "administrator if you think this is an error."
+        BLOCKED_MESSAGE
       end
     end
   end

app/services/system_hooks_service.rb

@@ -43,8 +43,11 @@ class SystemHooksService
     when User
       data.merge!(user_data(model))
 
-      if event == :rename
+      case event
+      when :rename
         data[:old_username] = model.username_was
+      when :failed_login
+        data[:state] = model.state
       end
     when ProjectMember
       data.merge!(project_member_data(model))

changelogs/unreleased-ee/42025-fix-issue-api.yml

+---
+title: "[API] Fix creating issue when assignee_id is empty"
+merge_request:
+author:
+type: fixed

changelogs/unreleased/fj-41598-fixing-request-mime-type.yml

+---
+title: Fixing rack request mime type when using rack attack
+merge_request: 16427
+author:
+type: fixed

changelogs/unreleased/sh-log-when-user-blocked.yml

+---
+title: Log and send a system hook if a blocked user attempts to login
+merge_request:
+author:
+type: added

config/initializers/warden.rb

@@ -2,4 +2,8 @@ Rails.application.configure do |config|
   Warden::Manager.after_set_user do |user, auth, opts|
     Gitlab::Auth::UniqueIpsLimiter.limit_user!(user)
   end
+
+  Warden::Manager.before_failure do |env, opts|
+    Gitlab::Auth::BlockedUserTracker.log_if_user_blocked(env)
+  end
 end

doc/ci/yaml/README.md

@@ -93,7 +93,7 @@ be an array or a multi-line string.
 > Introduced in GitLab 8.7 and requires Gitlab Runner v1.2
 
 `after_script` is used to define the command that will be run after for all
-jobs. This has to be an array or a multi-line string.
+jobs, including failed ones. This has to be an array or a multi-line string.
 
 > **Note:**
 The `before_script` and the main `script` are concatenated and run in a single context/container.

doc/system_hooks/system_hooks.md

@@ -11,6 +11,7 @@ Your GitLab instance can perform HTTP POST requests on the following events:
 - `user_remove_from_team`
 - `user_create`
 - `user_destroy`
+- `user_failed_login`
 - `user_rename`
 - `key_create`
 - `key_destroy`
@@ -22,6 +23,8 @@ Your GitLab instance can perform HTTP POST requests on the following events:
 
 The triggers for most of these are self-explanatory, but `project_update` and `project_rename` deserve some clarification: `project_update` is fired any time an attribute of a project is changed (name, description, tags, etc.) *unless* the `path` attribute is also changed. In that case, a `project_rename` is triggered instead (so that, for instance, if all you care about is the repo URL, you can just listen for `project_rename`).
 
+`user_failed_login` is sent whenever a **blocked** user attempts to login and denied access.
+
 System hooks can be used, e.g. for logging or changing information in a LDAP server.
 
 > **Note:**
@@ -196,6 +199,23 @@ Please refer to `group_rename` and `user_rename` for that case.
 }
 ```
 
+**User failed login:**
+
+```json
+{
+  "event_name": "user_failed_login",
+  "created_at": "2017-10-03T06:08:48Z",
+  "updated_at": "2018-01-15T04:52:06Z",
+        "name": "John Smith",
+       "email": "user4@example.com",
+     "user_id": 26,
+    "username": "user4",
+       "state": "blocked"
+}
+```
+
+If the user is blocked via LDAP, `state` will be `ldap_blocked`.
+
 **User renamed:**
 
 ```json

lib/api/helpers/common_helpers.rb

@@ -3,8 +3,10 @@ module API
     module CommonHelpers
       def convert_parameters_from_legacy_format(params)
         params.tap do |params|
-          if params[:assignee_id].present?
-            params[:assignee_ids] = [params.delete(:assignee_id)]
+          assignee_id = params.delete(:assignee_id)
+
+          if assignee_id.present?
+            params[:assignee_ids] = [assignee_id]
           end
         end
       end

lib/gitlab/auth/blocked_user_tracker.rb

+# frozen_string_literal: true
+module Gitlab
+  module Auth
+    class BlockedUserTracker
+      ACTIVE_RECORD_REQUEST_PARAMS = 'action_dispatch.request.request_parameters'
+
+      def self.log_if_user_blocked(env)
+        message = env.dig('warden.options', :message)
+
+        # Devise calls User#active_for_authentication? on the User model and then
+        # throws an exception to Warden with User#inactive_message:
+        # https://github.com/plataformatec/devise/blob/v4.2.1/lib/devise/hooks/activatable.rb#L8
+        #
+        # Since Warden doesn't pass the user record to the failure handler, we
+        # need to do a database lookup with the username. We can limit the
+        # lookups to happen when the user was blocked by checking the inactive
+        # message passed along by Warden.
+        return unless message == User::BLOCKED_MESSAGE
+
+        login = env.dig(ACTIVE_RECORD_REQUEST_PARAMS, 'user', 'login')
+
+        return unless login.present?
+
+        user = User.by_login(login)
+
+        return unless user&.blocked?
+
+        Gitlab::AppLogger.info("Failed login for blocked user: user=#{user.username} ip=#{env['REMOTE_ADDR']}")
+        SystemHooksService.new.execute_hooks_for(user, :failed_login)
+
+        true
+      rescue TypeError
+      end
+    end
+  end
+end

lib/gitlab/auth/user_auth_finders.rb

@@ -118,9 +118,7 @@ module Gitlab
       end
 
       def ensure_action_dispatch_request(request)
-        return request if request.is_a?(ActionDispatch::Request)
-
-        ActionDispatch::Request.new(request.env)
+        ActionDispatch::Request.new(request.env.dup)
       end
 
       def current_request

lib/gitlab/git/conflict/resolver.rb

@@ -15,7 +15,7 @@ module Gitlab
           @conflicts ||= begin
             @target_repository.gitaly_migrate(:conflicts_list_conflict_files) do |is_enabled|
               if is_enabled
-                gitaly_conflicts_client(@target_repository).list_conflict_files
+                gitaly_conflicts_client(@target_repository).list_conflict_files.to_a
               else
                 rugged_list_conflict_files
               end

lib/gitlab/gitaly_client/conflict_files_stitcher.rb

+module Gitlab
+  module GitalyClient
+    class ConflictFilesStitcher
+      include Enumerable
+
+      def initialize(rpc_response)
+        @rpc_response = rpc_response
+      end
+
+      def each
+        current_file = nil
+
+        @rpc_response.each do |msg|
+          msg.files.each do |gitaly_file|
+            if gitaly_file.header
+              yield current_file if current_file
+
+              current_file = file_from_gitaly_header(gitaly_file.header)
+            else
+              current_file.content << gitaly_file.content
+            end
+          end
+        end
+
+        yield current_file if current_file
+      end
+
+      private
+
+      def file_from_gitaly_header(header)
+        Gitlab::Git::Conflict::File.new(
+          Gitlab::GitalyClient::Util.git_repository(header.repository),
+          header.commit_oid,
+          conflict_from_gitaly_file_header(header),
+          ''
+        )
+      end
+
+      def conflict_from_gitaly_file_header(header)
+        {
+          ours: { path: header.our_path, mode: header.our_mode },
+          theirs: { path: header.their_path }
+        }
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client/conflicts_service.rb

@@ -20,7 +20,11 @@ module Gitlab
         )
         response = GitalyClient.call(@repository.storage, :conflicts_service, :list_conflict_files, request)
 
-        files_from_response(response).to_a
+        GitalyClient::ConflictFilesStitcher.new(response)
+      end
+
+      def conflicts?
+        list_conflict_files.any?
       end
 
       def resolve_conflicts(target_repository, resolution, source_branch, target_branch)
@@ -58,38 +62,6 @@ module Gitlab
           user: Gitlab::Git::User.from_gitlab(resolution.user).to_gitaly
         )
       end
-
-      def files_from_response(response)
-        files = []
-
-        response.each do |msg|
-          msg.files.each do |gitaly_file|
-            if gitaly_file.header
-              files << file_from_gitaly_header(gitaly_file.header)
-            else
-              files.last.content << gitaly_file.content
-            end
-          end
-        end
-
-        files
-      end
-
-      def file_from_gitaly_header(header)
-        Gitlab::Git::Conflict::File.new(
-          Gitlab::GitalyClient::Util.git_repository(header.repository),
-          header.commit_oid,
-          conflict_from_gitaly_file_header(header),
-          ''
-        )
-      end
-
-      def conflict_from_gitaly_file_header(header)
-        {
-          ours: { path: header.our_path, mode: header.our_mode },
-          theirs: { path: header.their_path }
-        }
-      end
     end
   end
 end

spec/lib/gitlab/auth/blocked_user_tracker_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::BlockedUserTracker do
+  set(:user) { create(:user) }
+
+  describe '.log_if_user_blocked' do
+    it 'does not log if user failed to login due to undefined reason' do
+      expect_any_instance_of(SystemHooksService).not_to receive(:execute_hooks_for)
+
+      expect(described_class.log_if_user_blocked({})).to be_nil
+    end
+
+    it 'gracefully handles malformed environment variables' do
+      env = { 'warden.options' => 'test' }
+
+      expect(described_class.log_if_user_blocked(env)).to be_nil
+    end
+
+    context 'failed login due to blocked user' do
+      let(:env) do
+        {
+          'warden.options' => { message: User::BLOCKED_MESSAGE },
+          described_class::ACTIVE_RECORD_REQUEST_PARAMS => { 'user' => { 'login' => user.username } }
+        }
+      end
+
+      subject { described_class.log_if_user_blocked(env) }
+
+      before do
+        expect_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).with(user, :failed_login)
+      end
+
+      it 'logs a blocked user' do
+        user.block!
+
+        expect(subject).to be_truthy
+      end
+
+      it 'logs a blocked user by e-mail' do
+        user.block!
+        env[described_class::ACTIVE_RECORD_REQUEST_PARAMS]['user']['login'] = user.email
+
+        expect(subject).to be_truthy
+      end
+
+      it 'logs a LDAP blocked user' do
+        user.ldap_block!
+
+        expect(subject).to be_truthy
+      end
+    end
+  end
+end

spec/lib/gitlab/auth/user_auth_finders_spec.rb

@@ -77,6 +77,16 @@ describe Gitlab::Auth::UserAuthFinders do
         expect(find_user_from_rss_token).to be_nil
       end
     end
+
+    context 'when the request format is empty' do
+      it 'the method call does not modify the original value' do
+        env['action_dispatch.request.formats'] = nil
+
+        find_user_from_rss_token
+
+        expect(env['action_dispatch.request.formats']).to be_nil
+      end
+    end
   end
 
   describe '#find_user_from_access_token' do

spec/lib/gitlab/gitaly_client/conflict_files_stitcher_spec.rb

+require 'spec_helper'
+
+describe Gitlab::GitalyClient::ConflictFilesStitcher do
+  describe 'enumeration' do
+    it 'combines segregated ConflictFile messages together' do
+      target_project = create(:project, :repository)
+      target_repository = target_project.repository.raw
+      target_gitaly_repository = target_repository.gitaly_repository
+
+      our_path_1 = 'our/path/1'
+      their_path_1 = 'their/path/1'
+      our_mode_1 = 0744
+      commit_oid_1 = 'f00'
+      content_1 = 'content of the first file'
+
+      our_path_2 = 'our/path/2'
+      their_path_2 = 'their/path/2'
+      our_mode_2 = 0600
+      commit_oid_2 = 'ba7'
+      content_2 = 'content of the second file'
+
+      header_1 = double(repository: target_gitaly_repository, commit_oid: commit_oid_1,
+                        our_path: our_path_1, their_path: their_path_1, our_mode: our_mode_1)
+      header_2 = double(repository: target_gitaly_repository, commit_oid: commit_oid_2,
+                        our_path: our_path_2, their_path: their_path_2, our_mode: our_mode_2)
+
+      messages = [
+        double(files: [double(header: header_1), double(header: nil, content: content_1[0..5])]),
+        double(files: [double(header: nil, content: content_1[6..-1])]),
+        double(files: [double(header: header_2)]),
+        double(files: [double(header: nil, content: content_2[0..5]), double(header: nil, content: content_2[6..10])]),
+        double(files: [double(header: nil, content: content_2[11..-1])])
+      ]
+
+      conflict_files = described_class.new(messages).to_a
+
+      expect(conflict_files.size).to be(2)
+
+      expect(conflict_files[0].content).to eq(content_1)
+      expect(conflict_files[0].their_path).to eq(their_path_1)
+      expect(conflict_files[0].our_path).to eq(our_path_1)
+      expect(conflict_files[0].our_mode).to be(our_mode_1)
+      expect(conflict_files[0].repository).to eq(target_repository)
+      expect(conflict_files[0].commit_oid).to eq(commit_oid_1)
+
+      expect(conflict_files[1].content).to eq(content_2)
+      expect(conflict_files[1].their_path).to eq(their_path_2)
+      expect(conflict_files[1].our_path).to eq(our_path_2)
+      expect(conflict_files[1].our_mode).to be(our_mode_2)
+      expect(conflict_files[1].repository).to eq(target_repository)
+      expect(conflict_files[1].commit_oid).to eq(commit_oid_2)
+    end
+  end
+end

spec/lib/gitlab/gitaly_client/conflicts_service_spec.rb

@@ -19,41 +19,12 @@ describe Gitlab::GitalyClient::ConflictsService do
         their_commit_oid: their_commit_oid
       )
     end
-    let(:our_path) { 'our/path' }
-    let(:their_path) { 'their/path' }
-    let(:our_mode) { 0744 }
-    let(:header) do
-      double(repository: target_gitaly_repository, commit_oid: our_commit_oid,
-             our_path: our_path, our_mode: 0744, their_path: their_path)
-    end
-    let(:response) do
-      [
-        double(files: [double(header: header), double(content: 'foo', header: nil)]),
-        double(files: [double(content: 'bar', header: nil)])
-      ]
-    end
-    let(:file) { subject[0] }
-
-    subject { client.list_conflict_files }
 
     it 'sends an RPC request' do
       expect_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
-        .with(request, kind_of(Hash)).and_return([])
-
-      subject
-    end
-
-    it 'forms a Gitlab::Git::ConflictFile collection from the response' do
-      allow_any_instance_of(Gitaly::ConflictsService::Stub).to receive(:list_conflict_files)
-        .with(request, kind_of(Hash)).and_return(response)
+        .with(request, kind_of(Hash)).and_return([].to_enum)
 
-      expect(subject.size).to be(1)
-      expect(file.content).to eq('foobar')
-      expect(file.their_path).to eq(their_path)
-      expect(file.our_path).to eq(our_path)
-      expect(file.our_mode).to be(our_mode)
-      expect(file.repository).to eq(target_repository)
-      expect(file.commit_oid).to eq(our_commit_oid)
+      client.list_conflict_files
     end
   end
 

spec/models/repository_spec.rb

@@ -358,28 +358,38 @@ describe Repository do
   end
 
   describe '#can_be_merged?' do
-    context 'mergeable branches' do
-      subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') }
+    shared_examples 'can be merged' do
+      context 'mergeable branches' do
+        subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') }
 
-      it { is_expected.to be_truthy }
-    end
+        it { is_expected.to be_truthy }
+      end
 
-    context 'non-mergeable branches' do
-      subject { repository.can_be_merged?('bb5206fee213d983da88c47f9cf4cc6caf9c66dc', 'feature') }
+      context 'non-mergeable branches' do
+        subject { repository.can_be_merged?('bb5206fee213d983da88c47f9cf4cc6caf9c66dc', 'feature') }
 
-      it { is_expected.to be_falsey }
-    end
+        it { is_expected.to be_falsey }
+      end
 
-    context 'non merged branch' do
-      subject { repository.merged_to_root_ref?('fix') }
+      context 'non merged branch' do
+        subject { repository.merged_to_root_ref?('fix') }
 
-      it { is_expected.to be_falsey }
+        it { is_expected.to be_falsey }
+      end
+
+      context 'non existent branch' do
+        subject { repository.merged_to_root_ref?('non_existent_branch') }
+
+        it { is_expected.to be_nil }
+      end
     end
 
-    context 'non existent branch' do
-      subject { repository.merged_to_root_ref?('non_existent_branch') }
+    context 'when Gitaly can_be_merged feature is enabled' do
+      it_behaves_like 'can be merged'
+    end
 
-      it { is_expected.to be_nil }
+    context 'when Gitaly can_be_merged feature is disabled', :disable_gitaly do
+      it_behaves_like 'can be merged'
     end
   end
 

spec/requests/api/issues_spec.rb

@@ -851,6 +851,15 @@ describe API::Issues, :mailer do
         expect(json_response['assignee']['name']).to eq(user2.name)
         expect(json_response['assignees'].first['name']).to eq(user2.name)
       end
+
+      it 'creates a new project issue when assignee_id is empty' do
+        post api("/projects/#{project.id}/issues", user),
+          title: 'new issue', assignee_id: ''
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(json_response['title']).to eq('new issue')
+        expect(json_response['assignee']).to be_nil
+      end
     end
 
     context 'single assignee restrictions' do

spec/services/system_hooks_service_spec.rb

@@ -105,12 +105,25 @@ describe SystemHooksService do
         expect(data[:old_username]).to eq(user.username_was)
       end
     end
+
+    context 'user_failed_login' do
+      it 'contains state of user' do
+        user.ldap_block!
+
+        data = event_data(user, :failed_login)
+
+        expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :state)
+        expect(data[:username]).to eq(user.username)
+        expect(data[:state]).to eq('ldap_blocked')
+      end
+    end
   end
 
   context 'event names' do
     it { expect(event_name(user, :create)).to eq "user_create" }
     it { expect(event_name(user, :destroy)).to eq "user_destroy" }
     it { expect(event_name(user, :rename)).to eq 'user_rename' }
+    it { expect(event_name(user, :failed_login)).to eq 'user_failed_login' }
     it { expect(event_name(project, :create)).to eq "project_create" }
     it { expect(event_name(project, :destroy)).to eq "project_destroy" }
     it { expect(event_name(project, :rename)).to eq "project_rename" }