From 5b3818686784304b1224bd1a620d9aafe5bbf30c Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:12:16 +0200 Subject: [PATCH 01/13] Add note for refactoring --- config/initializers/devise.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 530cf26af5..22d4bf68dd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -205,6 +205,7 @@ Devise.setup do |config| # end if Gitlab.config.ldap.enabled + # TODO: make server specific if Gitlab.config.ldap.allow_username_or_email_login email_stripping_proc = ->(name) {name.gsub(/@.*$/,'')} else From c832c50c8702c710671cf2e8644393d2151926fa Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:36:54 +0200 Subject: [PATCH 02/13] Do not use primary flag for LDAP servers --- app/views/devise/sessions/new.html.haml | 8 ++++---- config/initializers/1_settings.rb | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) 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/initializers/1_settings.rb b/config/initializers/1_settings.rb index 856afafb7e..4ac53c26ba 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -64,7 +64,6 @@ 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['label'] = 'LDAP' server['provider_id'] = '' #providername will be ldap Settings.ldap['servers'] = [server] From 1835fceb4bc9d548ace7ab0683a6b1ffd014285a Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 09:40:31 +0200 Subject: [PATCH 03/13] Make allow_username_or_email_login server specific --- config/initializers/1_settings.rb | 3 +-- config/initializers/devise.rb | 13 ++++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 4ac53c26ba..abab32265d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -62,8 +62,7 @@ Settings.ldap['active_directory'] = true if Settings.ldap['active_directory'].ni # 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 = Settings.ldap.except('sync_time') server['label'] = 'LDAP' server['provider_id'] = '' #providername will be ldap Settings.ldap['servers'] = [server] diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 22d4bf68dd..7770f018a1 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -205,14 +205,13 @@ Devise.setup do |config| # end if Gitlab.config.ldap.enabled - # TODO: make server specific - 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'], From 53816ef8f36d8e8920da1c198aa4e298797847fa Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:07:37 +0200 Subject: [PATCH 04/13] Make AD check server specific --- config/initializers/1_settings.rb | 4 ++-- lib/gitlab/ldap/access.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index abab32265d..76a87a5bf8 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -55,9 +55,7 @@ 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? @@ -69,6 +67,8 @@ if Settings.ldap['enabled'] || Rails.env.test? 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/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index ca1dcadfc5..191884e03f 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -36,7 +36,7 @@ module Gitlab def allowed? if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) - if Gitlab.config.ldap.active_directory + if ldap_config.active_directory !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) end else From adee309127374d84c6840b1c2fbb528b782467b8 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:10:39 +0200 Subject: [PATCH 05/13] Better description for new LDAP options --- config/gitlab.yml.example | 28 +++++++++++++++++++++------- lib/gitlab/ldap/person.rb | 4 ---- 2 files changed, 21 insertions(+), 11 deletions(-) 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/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 From 8271fac0a97ffbb6cbf5b8cf0ebf618f1ed1b6b7 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:39:46 +0200 Subject: [PATCH 06/13] Make AD check for access work properly --- lib/gitlab/ldap/access.rb | 5 ++--- lib/gitlab/ldap/config.rb | 4 ++++ spec/lib/gitlab/ldap/access_spec.rb | 18 ++++++------------ 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 191884e03f..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 ldap_config.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 bb9b03f370..d0dfbacc32 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -60,6 +60,10 @@ module Gitlab options['admin_group'] end + def active_directory + options['active_directory'] + end + protected def base_config Gitlab.config.ldap 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 From faf54d525c01aa8d2d2dcd0790edb622a91a13d5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 10:57:09 +0200 Subject: [PATCH 07/13] Name the default provider main, to avoid name collisions --- config/initializers/1_settings.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 76a87a5bf8..8961246ac7 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -62,7 +62,7 @@ if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? 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 From 824aeacdd7a85d9c5992609e524b36b311a929e5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:25:35 +0200 Subject: [PATCH 08/13] Only allow valid LDAP::Config instances. When trying to instantiate a new, but unknown provider, guidance is provided to get the correct provider RuntimeError: Unknown provider (henk). Available providers: ["ldapmain"] --- lib/gitlab/ldap/config.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index d0dfbacc32..af0d743f4d 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -8,11 +8,16 @@ module Gitlab Gitlab.config.ldap.enabled end - def servers + def self.servers Gitlab.config.ldap.servers end + def self.providers + servers.map &:provider_name + end + def initialize(provider) + raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}" @provider = provider @options = config_for(provider) end From 74e79105cde3b9470dc83f8705fa73e452aab75f Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:31:33 +0200 Subject: [PATCH 09/13] Cleanup provider validation --- lib/gitlab/ldap/config.rb | 10 +++++++++- spec/lib/gitlab/ldap/config_spec.rb | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index af0d743f4d..697b66dcda 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -17,8 +17,8 @@ module Gitlab end def initialize(provider) - raise "Unknown provider (#{provider}). Available providers: #{self.class.providers}" @provider = provider + invalid_provider unless valid_provider? @options = config_for(provider) end @@ -89,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/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 From 049ae5b331b7a09e23980404aa006cf21f8d8081 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:42:49 +0200 Subject: [PATCH 10/13] Clearly distance ourselfs from AR methods --- lib/gitlab/ldap/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index be9392a8d8..ba0f5844eb 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -60,7 +60,7 @@ module Gitlab def initialize(auth_hash) super - update_attributes + update_user_attributes end # instance methods @@ -79,7 +79,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, From 492398a6c6c86d84644df86e5b6bd2edf6c693a5 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:42:49 +0200 Subject: [PATCH 11/13] Clearly distance ourselfs from AR methods --- lib/gitlab/ldap/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index c8e5d11447..006ef17072 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -54,7 +54,7 @@ module Gitlab def initialize(auth_hash) super - update_attributes + update_user_attributes end # instance methods @@ -73,7 +73,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, From 9a1573b2dc2ed443638a5115542baa71b42e21d7 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 11:56:20 +0200 Subject: [PATCH 12/13] More usefull errors if saving fails --- lib/gitlab/oauth/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 70efa16a0c5dc4f031075ffaaec682f4f69fcb44 Mon Sep 17 00:00:00 2001 From: Jan-Willem van der Meer Date: Fri, 10 Oct 2014 12:00:37 +0200 Subject: [PATCH 13/13] More specific check for generated email --- spec/lib/gitlab/oauth/auth_hash_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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