Merge branch 'fix-flog' into 'master'

Refactor complex methods to make CI green



See merge request !57

app/models/git_hook.rb

@@ -3,18 +3,6 @@ class GitHook < ActiveRecord::Base
 
   validates :project, presence: true, unless: "is_sample?"
 
-  def commit_message_allowed?(message)
-    if commit_message_regex.present?
-      if message =~ Regexp.new(commit_message_regex)
-        true
-      else
-        false
-      end
-    else
-      true
-    end
-  end
-
   def commit_validation?
     commit_message_regex.present? ||
       author_email_regex.present? ||
@@ -22,4 +10,22 @@ class GitHook < ActiveRecord::Base
       file_name_regex.present? ||
       max_file_size > 0
   end
+
+  def commit_message_allowed?(message)
+    data_valid?(message, commit_message_regex)
+  end
+
+  def author_email_allowed?(email)
+    data_valid?(email, author_email_regex)
+  end
+
+  private
+
+  def data_valid?(data, regex)
+    if regex.present?
+      !!(data =~ Regexp.new(regex))
+    else
+      true
+    end
+  end
 end

lib/gitlab/git_access.rb

@@ -170,9 +170,9 @@ module Gitlab
     end
 
     def git_hook_check(user, project, ref, oldrev, newrev)
-      return build_status_object(true) unless project.git_hook
-
-      return build_status_object(true) unless newrev && oldrev
+      unless project.git_hook && newrev && oldrev
+        return build_status_object(true)
+      end
 
       git_hook = project.git_hook
 
@@ -182,8 +182,9 @@ module Gitlab
           return build_status_object(false, "You can not delete tag")
         end
       else
-        return build_status_object(true) unless git_hook.commit_validation?
-        return build_status_object(true) if Gitlab::Git.blank_ref?(newrev)
+        if Gitlab::Git.blank_ref?(newrev) || !git_hook.commit_validation?
+          return build_status_object(true)
+        end
 
         oldrev = project.default_branch if Gitlab::Git.blank_ref?(oldrev)
 
@@ -195,21 +196,32 @@ module Gitlab
           end
 
         commits.each do |commit|
-          if git_hook.commit_message_regex.present?
-            unless commit.safe_message =~ Regexp.new(git_hook.commit_message_regex)
-              return build_status_object(false, "Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+          if status_object = check_commit(commit, git_hook)
+            return status_object
+          end
         end
       end
 
-          if git_hook.author_email_regex.present?
-            unless commit.committer_email =~ Regexp.new(git_hook.author_email_regex)
+      build_status_object(true)
+    end
+
+    private
+
+    # If commit does not pass git hook validation the whole push should be rejected.
+    # This method should return nil if no error found or status object if there are some errors.
+    # In case of errors - all other checks will be canceled and push will be rejected.
+    def check_commit(commit, git_hook)
+      unless git_hook.commit_message_allowed?(commit.safe_message)
+        return build_status_object(false, "Commit message does not follow the pattern '#{git_hook.commit_message_regex}'")
+      end
+
+      unless git_hook.author_email_allowed?(commit.committer_email)
         return build_status_object(false, "Committer's email '#{commit.committer_email}' does not follow the pattern '#{git_hook.author_email_regex}'")
       end
 
-            unless commit.author_email =~ Regexp.new(git_hook.author_email_regex)
+      unless git_hook.author_email_allowed?(commit.author_email)
         return build_status_object(false, "Author's email '#{commit.author_email}' does not follow the pattern '#{git_hook.author_email_regex}'")
       end
-          end
 
       # Check whether author is a GitLab member
       if git_hook.member_check
@@ -224,6 +236,14 @@ module Gitlab
         end
       end
 
+      if status_object = check_commit_diff(commit, git_hook)
+        return status_object
+      end
+
+      nil
+    end
+
+    def check_commit_diff(commit, git_hook)
       if git_hook.file_name_regex.present?
         commit.diffs.each do |diff|
           if (diff.renamed_file || diff.new_file) && diff.new_path =~ Regexp.new(git_hook.file_name_regex)
@@ -242,14 +262,10 @@ module Gitlab
           end
         end
       end
-        end
-      end
 
-      build_status_object(true)
+      nil
     end
 
-    private
-
     def protected_branch_action(oldrev, newrev, branch_name)
       # we dont allow force push to protected branch
       if forced_push?(oldrev, newrev)
@@ -290,8 +306,6 @@ module Gitlab
       end
     end
 
-    protected
-
     def build_status_object(status, message = '')
       GitAccessStatus.new(status, message)
     end

lib/gitlab/ldap/access.rb

@@ -83,24 +83,39 @@ module Gitlab
 
       # Update user ssh keys if they changed in LDAP
       def update_ssh_keys
-        user.keys.ldap.where.not(key: ldap_user.ssh_keys).each do |deleted_key|
-          Rails.logger.info "#{self.class.name}: removing LDAP SSH key #{deleted_key.key} from #{user.name} (#{user.id})"
-          unless deleted_key.destroy
-            Rails.logger.error "#{self.class.name}: failed to remove LDAP SSH key #{key.inspect} from #{user.name} (#{user.id})"
-          end
+        remove_old_ssh_keys
+        add_new_ssh_keys
       end
 
-        (ldap_user.ssh_keys - user.keys.ldap.pluck(:key)).each do |key|
-          Rails.logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})"
+      # Add ssh keys that are in LDAP but not in GitLab
+      def add_new_ssh_keys
+        keys = ldap_user.ssh_keys - user.keys.ldap.pluck(:key)
+
+        keys.each do |key|
+          logger.info "#{self.class.name}: adding LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})"
           new_key = LDAPKey.new(title: "LDAP - #{ldap_config.sync_ssh_keys}", key: key)
           new_key.user = user
+
           unless new_key.save
-            Rails.logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\
+            logger.error "#{self.class.name}: failed to add LDAP SSH key #{key.inspect} to #{user.name} (#{user.id})\n"\
               "error messages: #{new_key.errors.messages}"
           end
         end
       end
 
+      # Remove ssh keys that do not exist in LDAP any more
+      def remove_old_ssh_keys
+        keys = user.keys.ldap.where.not(key: ldap_user.ssh_keys)
+
+        keys.each do |deleted_key|
+          logger.info "#{self.class.name}: removing LDAP SSH key #{deleted_key.key} from #{user.name} (#{user.id})"
+
+          unless deleted_key.destroy
+            logger.error "#{self.class.name}: failed to remove LDAP SSH key #{key.inspect} from #{user.name} (#{user.id})"
+          end
+        end
+      end
+
       # Update user email if it changed in LDAP
       def update_email
         return false unless ldap_user.try(:email)
@@ -142,7 +157,7 @@ module Gitlab
           if active_group_links.any?
             group.add_users([user.id], fetch_group_access(group, user, active_group_links), skip_notification: true)
           elsif group.last_owner?(user)
-            Rails.logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
+            logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else
             group.users.delete(user)
           end
@@ -175,6 +190,7 @@ module Gitlab
       end
 
       private
+
       def gitlab_groups_with_ldap_link
         ::Group.includes(:ldap_group_links).references(:ldap_group_links).
           where.not(ldap_group_links: { id: nil }).
@@ -190,6 +206,10 @@ module Gitlab
         # TODO: Test if nil value of current_access_level in handled properly
         [current_access_level, max_group_access_level].compact.max
       end
+
+      def logger
+        Rails.logger
+      end
     end
   end
 end