diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 328a2d8f04..04e998f8be 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/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' diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 6bcdecf0bc..272463121f 100644 --- a/config/gitlab.yml.example +++ b/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. diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 856afafb7e..8961246ac7 100644 --- a/config/initializers/1_settings.rb +++ b/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 diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 530cf26af5..7770f018a1 100644 --- a/config/initializers/devise.rb +++ b/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'], diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index ca1dcadfc5..e3d2cc065f 100644 --- a/lib/gitlab/ldap/access.rb +++ b/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 diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index ee09b43089..697b66dcda 100644 --- a/lib/gitlab/ldap/config.rb +++ b/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: { diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index f4ee22476d..a35fd22073 100644 --- a/lib/gitlab/ldap/person.rb +++ b/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 diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 8326714888..3069027a42 100644 --- a/lib/gitlab/ldap/user.rb +++ b/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, diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 8231886144..699258baee 100644 --- a/lib/gitlab/oauth/user.rb +++ b/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 diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 564f4c04a0..4656442a6f 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb index a01166b264..76cc7f95c4 100644 --- a/spec/lib/gitlab/ldap/config_spec.rb +++ b/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 \ No newline at end of file diff --git a/spec/lib/gitlab/oauth/auth_hash_spec.rb b/spec/lib/gitlab/oauth/auth_hash_spec.rb index 68b87dbd4b..5eb77b492b 100644 --- a/spec/lib/gitlab/oauth/auth_hash_spec.rb +++ b/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