Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ee into ce_upstrem_import-url-fix

CHANGELOG-EE

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.7.0 (unreleased)
-  - Don't attempt to include too large diffs in e-mail-on-push messages (Stan Hu)
   - Update GitLab Pages to 0.2.1: support user-defined 404 pages
   - Refactor group sync to pull access level logic to its own class. !306
   - [Elastic] Stabilize database indexer if database is inconsistent
   - Add ability to sync to remote mirrors. !249
 
 v 8.6.6
-  - Fix LDAP group sync regression for groups with member value `uid=<username>` !335
-  - Concat AD group recursive member results with regular member results
+  - Concat AD group recursive member results with regular member results. !333
+  - Fix LDAP group sync regression for groups with member value `uid=<username>`. !335
+  - Don't attempt to include too large diffs in e-mail-on-push messages (Stan Hu). !338
 
 v 8.6.5
   - No EE-specific changes

app/controllers/projects/merge_requests_controller.rb

@@ -197,6 +197,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       return
     end
 
+    merge_request_service = MergeRequests::MergeService.new(@project, current_user, merge_params)
+    unless merge_request_service.hooks_validation_pass?(@merge_request)
+      @status = :hook_validation_error
+      return
+    end
+
     TodoService.new.merge_merge_request(merge_request, current_user)
 
     @merge_request.update(merge_error: nil)

app/services/merge_requests/merge_service.rb

@@ -28,6 +28,23 @@ module MergeRequests
       end
     end
 
+    def hooks_validation_pass?(merge_request)
+      git_hook = merge_request.project.git_hook
+      return true unless git_hook
+
+      unless git_hook.commit_message_allowed?(params[:commit_message])
+        merge_request.update(merge_error: "Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+        return false
+      end
+
+      unless git_hook.author_email_allowed?(current_user.email)
+        merge_request.update(merge_error: "Commit author's email '#{current_user.email}' does not follow the pattern '#{git_hook.author_email_regex}'")
+        return false
+      end
+
+      true
+    end
+
     private
 
     def commit

app/views/projects/merge_requests/merge.js.haml

@@ -5,6 +5,9 @@
 - when :merge_when_build_succeeds
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/merge_when_build_succeeds'))}");
+- when :hook_validation_error
+  :plain
+    $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/error'))}");
 - else
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");

app/views/projects/merge_requests/widget/open/_error.html.haml

+%h4
+  = icon('exclamation-triangle')
+  This merge request failed to be merged automatically
+
+%p
+  = @merge_request.merge_error

lib/gitlab/ldap/group_sync.rb

+require 'net/ldap/dn'
+
 module Gitlab
   module LDAP
     class GroupSync
@@ -181,13 +183,17 @@ module Gitlab
       # account for that. See gitlab-ee#442
       def ensure_full_dns!(dns)
         dns.map! do |dn|
-          # If there is more than one equal sign we must have a full DN
-          # Or at least the probability is higher.
-          return dn if dn.count('=') > 1
-
-          # If there is only one equal sign, we may only have a `uid`.
-          # In this case, strip the first part and look up full DN by UID
-          dn_for_uid(dn.split('=')[1])
+          parsed_dn = Net::LDAP::DN.new(dn).to_a
+          # If there is more than one key/value set we must have a full DN,
+          # or at least the probability is higher.
+          if parsed_dn.count > 2
+            dn
+          elsif parsed_dn[0] == 'uid'
+            dn_for_uid(parsed_dn[1])
+          else
+            logger.warn { "Found potentially malformed/incomplete DN: '#{dn}'" }
+            dn
+          end
         end
       end
 

spec/factories/git_hooks.rb

@@ -2,11 +2,18 @@
 
 FactoryGirl.define do
   factory :git_hook do
-    force_push_regex "MyString"
+    force_push_regex 'feature\/.*'
     deny_delete_tag false
-    delete_branch_regex "MyString"
+    delete_branch_regex 'bug\/.*'
     project
-    commit_message_regex "MyString"
+
+    trait :commit_message do
+      commit_message_regex "(f|F)ixes #\d+.*"
+    end
+
+    trait :author_email do
+      author_email_regex '.*@veryspecificedomain.com'
+    end
 
     factory :git_hook_sample do
       is_sample true

spec/features/merge_requests/merge_with_hooks_validation_spec.rb

+require 'spec_helper'
+
+feature 'Merge With Git Hooks Validation', feature: true, js: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public, git_hook: git_hook) }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04') }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  context 'commit message is invalid' do
+    let(:git_hook) { create(:git_hook, :commit_message) }
+
+    before do
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'displays error message after merge request is clicked' do
+      click_button 'Accept Merge Request'
+
+      expect(page).to have_content('Merge in progress')
+      expect(page).to have_content('This merge request failed to be merged automatically')
+      expect(page).to have_content("Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+    end
+  end
+
+  context 'author email is invalid' do
+    let(:git_hook) { create(:git_hook, :author_email) }
+
+    before do
+      login_as user
+      visit_merge_request(merge_request)
+    end
+
+    it 'displays error message after merge request is clicked' do
+      click_button 'Accept Merge Request'
+
+      expect(page).to have_content('Merge in progress')
+      expect(page).to have_content('This merge request failed to be merged automatically')
+      expect(page).to have_content("Commit author's email '#{user.email}' does not follow the pattern '#{git_hook.author_email_regex}'")
+    end
+  end
+
+  def visit_merge_request(merge_request)
+    visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
+  end
+end

spec/services/merge_requests/merge_service_spec.rb

@@ -52,4 +52,34 @@ describe MergeRequests::MergeService, services: true do
       end
     end
   end
+
+  describe '#hooks_validation_pass?' do
+    let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
+
+    it 'returns true when valid' do
+      expect(service.hooks_validation_pass?(merge_request)).to be_truthy
+    end
+
+    context 'commit message validation' do
+      before do
+        allow(project).to receive(:git_hook) { build(:git_hook, commit_message_regex: 'unmatched pattern .*') }
+      end
+
+      it 'returns false and saves error when invalid' do
+        expect(service.hooks_validation_pass?(merge_request)).to be_falsey
+        expect(merge_request.merge_error).not_to be_empty
+      end
+    end
+
+    context 'authors email validation' do
+      before do
+        allow(project).to receive(:git_hook) { build(:git_hook, author_email_regex: '.*@unmatchedemaildomain.com') }
+      end
+
+      it 'returns false and saves error when invalid' do
+        expect(service.hooks_validation_pass?(merge_request)).to be_falsey
+        expect(merge_request.merge_error).not_to be_empty
+      end
+    end
+  end
 end