Merge branch 'feature-multi-ldap-servers' into feature-multiple-ldap-push-authentication

app/views/devise/sessions/new.html.haml

@@ -4,14 +4,14 @@
   .login-body
     - if ldap_enabled? && gitlab_config.signin_enabled
       %ul.nav.nav-tabs
-        - @ldap_servers.each do |server|
-          %li{class: (:active if server['primary'])}
+        - @ldap_servers.each_with_index do |server, i|
+          %li{class: (:active if i==0)}
             = link_to server['label'], "#tab-#{server.provider_name}", 'data-toggle' => 'tab'
         %li
           = link_to 'Standard', '#tab-signin', 'data-toggle' => 'tab'
       .tab-content
-        - @ldap_servers.each do |server|
-          %div.tab-pane{id: "tab-#{server.provider_name}", class: (:active if server['primary'])}
+        - @ldap_servers.each_with_index do |server,i|
+          %div.tab-pane{id: "tab-#{server.provider_name}", class: (:active if i==0)}
             = render 'devise/sessions/new_ldap', provider: server.provider_name
         %div#tab-signin.tab-pane
           = render 'devise/sessions/new_base'

config/gitlab.yml.example

@@ -136,6 +136,27 @@ production: &base
     enabled: false
     servers:
       -
+        ## provider_id
+        #
+        # This identifier is used by GitLab to keep track of which LDAP server each
+        # GitLab user belongs to. Each LDAP server known to GitLab should have a unique
+        # provider_id. This identifier cannot be changed once users from the LDAP server
+        # have started logging in to GitLab.
+        #
+        # Format: one word, using a-z (lower case) and 0-9
+        # Example: 'paris' or 'uswest2'
+
+        provider_id: main
+
+        ## label
+        #
+        # A human-friendly name for your LDAP server. It is OK to change the label later,
+        # for instance if you find out it is too large to fit on the web page.
+        #
+        # Example: 'Paris' or 'Acme, Ltd.'
+
+        label: 'LDAP'
+
         host: '_your_ldap_server'
         port: 636
         uid: 'sAMAccountName'
@@ -143,13 +164,6 @@ production: &base
         bind_dn: '_the_full_dn_of_the_user_you_will_bind_with'
         password: '_the_password_of_the_bind_user'
 
-        # When authenticating against an ldap server, this will provide a unique identifier
-        # Use one uniq word, no non-word charcters are allowed
-        provider_id: main
-
-        # The UI use this to make a distinction between your LDAP servers
-        label: 'LDAP'
-
         # This setting controls the amount of time between LDAP permission checks for each user.
         # After this time has expired for a given user, their next interaction with GitLab (a click in the web UI, a git pull etc.) will be slower because the LDAP permission check is being performed.
         # How much slower depends on your LDAP setup, but it is not uncommon for this check to add seconds of waiting time.

config/initializers/1_settings.rb

@@ -55,22 +55,20 @@ end
 # Default settings
 Settings['ldap'] ||= Settingslogic.new({})
 Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil?
-Settings.ldap['allow_username_or_email_login'] = false if Settings.ldap['allow_username_or_email_login'].nil?
 Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil?
-Settings.ldap['active_directory'] = true if Settings.ldap['active_directory'].nil?
 
 # backwards compatibility, we only have one host
 if Settings.ldap['enabled'] || Rails.env.test?
   if Settings.ldap['host'].present?
-    excluded_per_server_settings = %w(sync_time allow_username_or_email_login)
-    server = Settings.ldap.except(excluded_per_server_settings)
-    server['primary'] = true
+    server = Settings.ldap.except('sync_time')
     server['label'] = 'LDAP'
-    server['provider_id'] = '' #providername will be ldap
+    server['provider_id'] = 'main'
     Settings.ldap['servers'] = [server]
   end
 
   Settings.ldap['servers'].each do |server|
+    server['allow_username_or_email_login'] = false if server['allow_username_or_email_login'].nil?
+    server['active_directory'] = true if server['active_directory'].nil?
     server['provider_name'] = "ldap#{server['provider_id']}".downcase
     server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name'])
   end

config/initializers/devise.rb

@@ -205,13 +205,13 @@ Devise.setup do |config|
   # end
 
   if Gitlab.config.ldap.enabled
-    if Gitlab.config.ldap.allow_username_or_email_login
-      email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')}
-    else
-      email_stripping_proc = ->(name) {name}
-    end
-
     Gitlab.config.ldap.servers.each do |server|
+      if server['allow_username_or_email_login']
+        email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')}
+      else
+        email_stripping_proc = ->(name) {name}
+      end
+
       config.omniauth server.provider_name,
         host:     server['host'],
         base:     server['base'],

lib/gitlab/ldap/access.rb

@@ -36,9 +36,8 @@ module Gitlab
 
       def allowed?
         if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
-          if Gitlab.config.ldap.active_directory
-            !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter)
-          end
+          return true unless ldap_config.active_directory
+          !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter)
         else
           false
         end

lib/gitlab/ldap/config.rb

@@ -18,6 +18,7 @@ module Gitlab
 
       def initialize(provider)
         @provider = provider
+        invalid_provider unless valid_provider?
         @options = config_for(provider)
       end
 
@@ -64,6 +65,10 @@ module Gitlab
         options['admin_group']
       end
 
+      def active_directory
+        options['active_directory']
+      end
+
       protected
       def base_config
         Gitlab.config.ldap
@@ -84,6 +89,14 @@ module Gitlab
         end
       end
 
+      def valid_provider?
+        self.class.providers.include?(provider)
+      end
+
+      def invalid_provider
+        raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}"
+      end
+
       def auth_options
         {
           auth: {

lib/gitlab/ldap/person.rb

@@ -60,10 +60,6 @@ module Gitlab
         @entry
       end
 
-      # def adapter
-      #   @adapter ||= Gitlab::LDAP::Adapter.new
-      # end
-
       def config
         @config ||= Gitlab::LDAP::Config.new(provider)
       end

lib/gitlab/ldap/user.rb

@@ -20,7 +20,7 @@ module Gitlab
 
       def initialize(auth_hash)
         super
-        update_attributes
+        update_user_attributes
       end
 
       # instance methods
@@ -39,7 +39,7 @@ module Gitlab
         model.find_by(email: auth_hash.email)
       end
 
-      def update_attributes
+      def update_user_attributes
         gl_user.attributes = {
           extern_uid: auth_hash.uid,
           provider: auth_hash.provider,

lib/gitlab/oauth/user.rb

@@ -31,7 +31,7 @@ module Gitlab
 
         gl_user
       rescue ActiveRecord::RecordInvalid => e
-        log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}"
+        log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}"
         return self, e.record.errors
       end
 

spec/lib/gitlab/ldap/access_spec.rb

@@ -28,19 +28,13 @@ describe Gitlab::LDAP::Access do
         it { should be_true }
       end
 
-      context 'and has no disabled flag in active diretory' do
-        before {
-          Gitlab::LDAP::Person.stub(disabled_via_active_directory?: false)
-          Gitlab.config.ldap['enabled'] = true
-          Gitlab.config.ldap['active_directory'] = false
-        }
+      context 'withoud ActiveDirectory enabled' do
+        before do
+          Gitlab::LDAP::Config.stub(enabled?: true)
+          Gitlab::LDAP::Config.any_instance.stub(active_directory: false)
+        end
 
-        after {
-          Gitlab.config.ldap['enabled'] = false
-          Gitlab.config.ldap['active_directory'] = true
-        }
-
-        it { should be_false }
+        it { should be_true }
       end
     end
   end

spec/lib/gitlab/ldap/config_spec.rb

@@ -12,5 +12,9 @@ describe Gitlab::LDAP::Config do
     it "works" do
       expect(config).to be_a described_class
     end
+
+    it "raises an error if a unknow provider is used" do
+      expect{ Gitlab::LDAP::Config.new 'unknown' }.to raise_error
+    end
   end
 end

spec/lib/gitlab/oauth/auth_hash_spec.rb

@@ -33,7 +33,7 @@ describe Gitlab::OAuth::AuthHash do
   context "email not provided" do
     before { info_hash.delete(:email) }
     it "generates a temp email" do
-      expect( auth_hash.email).to_not be_empty
+      expect( auth_hash.email).to start_with('temp-email-for-oauth')
     end
   end