From 8666f497ff13c100f6cd2339971e430dbf05470f Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Sat, 11 Apr 2015 17:56:45 +0300 Subject: fix ldap identities --- app/models/identity.rb | 1 + config/initializers/1_settings.rb | 7 ++++++- db/migrate/20150411000035_fix_identities.rb | 16 ++++++++++++++++ db/schema.rb | 6 +++--- 4 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20150411000035_fix_identities.rb diff --git a/app/models/identity.rb b/app/models/identity.rb index 440fcd0d052..756d19adec7 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -15,4 +15,5 @@ class Identity < ActiveRecord::Base belongs_to :user validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } + validates :user_id, uniqueness: { scope: :provider } end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index e6b00c531ac..c25c799d8f6 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -67,7 +67,7 @@ Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil? if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? server = Settings.ldap.except('sync_time') - server['provider_name'] = 'ldap' + server['provider_name'] = 'ldapmain' Settings.ldap['servers'] = { 'ldap' => server } @@ -80,8 +80,13 @@ if Settings.ldap['enabled'] || Rails.env.test? server['provider_name'] ||= "ldap#{key}".downcase server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name']) end + + unless Settings.ldap['servers'].select{ |k, server| server['provider_name'] == "ldapmain"}.any? + raise "Wrong LDAP configuration. The 'main' LDAP section is missing" + end end + Settings['omniauth'] ||= Settingslogic.new({}) Settings.omniauth['enabled'] = false if Settings.omniauth['enabled'].nil? Settings.omniauth['providers'] ||= [] diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb new file mode 100644 index 00000000000..12526b10e6e --- /dev/null +++ b/db/migrate/20150411000035_fix_identities.rb @@ -0,0 +1,16 @@ +class FixIdentities < ActiveRecord::Migration + def up + new_provider = Gitlab.config.ldap.servers.first.last['provider_name'] + # Delete duplicate identities + Identity.connection.select_one("DELETE FROM identities WHERE provider = 'ldap' AND user_id IN (SELECT user_id FROM identities WHERE provider = '#{new_provider}')") + # Update legacy identities + Identity.where(provider: 'ldap').update_all(provider: new_provider) + + if defined?(LdapGroupLink) + LdapGroupLink.where('provider IS NULL').update_all(provider: new_provider) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index 14e32a7946e..903ed161e4a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150328132231) do +ActiveRecord::Schema.define(version: 20150411000035) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -336,12 +336,12 @@ ActiveRecord::Schema.define(version: 20150328132231) do t.string "import_url" t.integer "visibility_level", default: 0, null: false t.boolean "archived", default: false, null: false - t.string "avatar" t.string "import_status" t.float "repository_size", default: 0.0 t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" + t.string "avatar" end add_index "projects", ["created_at", "id"], name: "index_projects_on_created_at_and_id", using: :btree @@ -459,6 +459,7 @@ ActiveRecord::Schema.define(version: 20150328132231) do t.integer "notification_level", default: 1, null: false t.datetime "password_expires_at" t.integer "created_by_id" + t.datetime "last_credential_check_at" t.string "avatar" t.string "confirmation_token" t.datetime "confirmed_at" @@ -466,7 +467,6 @@ ActiveRecord::Schema.define(version: 20150328132231) do t.string "unconfirmed_email" t.boolean "hide_no_ssh_key", default: false t.string "website_url", default: "", null: false - t.datetime "last_credential_check_at" t.string "github_access_token" t.string "gitlab_access_token" t.string "notification_email" -- cgit v1.2.1 From 896ea2482bd78f3683140bb8aa08f0583a58361e Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Apr 2015 11:50:21 +0300 Subject: Change migration to SQL Signed-off-by: Dmitriy Zaporozhets --- db/migrate/20150411000035_fix_identities.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb index 12526b10e6e..93beb046d78 100644 --- a/db/migrate/20150411000035_fix_identities.rb +++ b/db/migrate/20150411000035_fix_identities.rb @@ -1,13 +1,15 @@ class FixIdentities < ActiveRecord::Migration def up new_provider = Gitlab.config.ldap.servers.first.last['provider_name'] + # Delete duplicate identities - Identity.connection.select_one("DELETE FROM identities WHERE provider = 'ldap' AND user_id IN (SELECT user_id FROM identities WHERE provider = '#{new_provider}')") - # Update legacy identities - Identity.where(provider: 'ldap').update_all(provider: new_provider) + execute "DELETE FROM identities WHERE provider = 'ldap' AND user_id IN (SELECT user_id FROM identities WHERE provider = '#{new_provider}')" + + # Update legacy identities + execute "UPDATE identities SET provider = '#{new_provider}' WHERE provider = 'ldap';" if defined?(LdapGroupLink) - LdapGroupLink.where('provider IS NULL').update_all(provider: new_provider) + execute "UPDATE ldap_group_links SET provider = '#{new_provider}' WHERE provider IS NULL;" end end -- cgit v1.2.1 From 325b66365bc0bac4c17398dca397cfa30637de24 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 11:04:31 +0200 Subject: Remove special cases for the 'ldap' provider --- lib/gitlab/ldap/config.rb | 2 -- lib/gitlab/ldap/user.rb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index 0cb24d0ccc1..fa5b6c1e230 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -27,8 +27,6 @@ module Gitlab def initialize(provider) if self.class.valid_provider?(provider) @provider = provider - elsif provider == 'ldap' - @provider = self.class.providers.first else self.class.invalid_provider(provider) end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index b04f5b4ac37..75026aeaeb2 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -13,7 +13,7 @@ module Gitlab def find_by_uid_and_provider(uid, provider) # LDAP distinguished name is case-insensitive identity = ::Identity. - where(provider: [provider, :ldap]). + where(provider: provider). where('lower(extern_uid) = ?', uid.downcase).last identity && identity.user end -- cgit v1.2.1 From f64db1fab95751bc2b1cf04641bb031d6289d16b Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 11:22:31 +0200 Subject: Try to explain what we are doing --- db/migrate/20150411000035_fix_identities.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb index 93beb046d78..b65ad138b0f 100644 --- a/db/migrate/20150411000035_fix_identities.rb +++ b/db/migrate/20150411000035_fix_identities.rb @@ -1,5 +1,14 @@ class FixIdentities < ActiveRecord::Migration def up + # Up until now, legacy 'ldap' references in the database were charitably + # interpreted to point to the first LDAP server specified in the GitLab + # configuration. So if the database said 'provider: ldap' but the first + # LDAP server was called 'ldapmain', then we would try to interpret + # 'provider: ldap' as if it said 'provider: ldapmain'. This migration (and + # accompanying changes in the GitLab LDAP code) get rid of this complicated + # behavior. Any database references to 'provider: ldap' get rewritten to + # whatever the code would have interpreted it as, i.e. as a reference to + # the first LDAP server specified in gitlab.yml / gitlab.rb. new_provider = Gitlab.config.ldap.servers.first.last['provider_name'] # Delete duplicate identities -- cgit v1.2.1 From 04f05ac1fb43904229bc084813dab92e82343f02 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Apr 2015 12:26:04 +0300 Subject: Check for table instead of class --- db/migrate/20150411000035_fix_identities.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb index b65ad138b0f..e86264b166a 100644 --- a/db/migrate/20150411000035_fix_identities.rb +++ b/db/migrate/20150411000035_fix_identities.rb @@ -11,13 +11,13 @@ class FixIdentities < ActiveRecord::Migration # the first LDAP server specified in gitlab.yml / gitlab.rb. new_provider = Gitlab.config.ldap.servers.first.last['provider_name'] - # Delete duplicate identities + # Delete duplicate identities execute "DELETE FROM identities WHERE provider = 'ldap' AND user_id IN (SELECT user_id FROM identities WHERE provider = '#{new_provider}')" # Update legacy identities execute "UPDATE identities SET provider = '#{new_provider}' WHERE provider = 'ldap';" - if defined?(LdapGroupLink) + if table_exists?('ldap_group_links') execute "UPDATE ldap_group_links SET provider = '#{new_provider}' WHERE provider IS NULL;" end end -- cgit v1.2.1 From 8b4705fea6297a23f708c59cbce3c8a3115128c0 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 11:50:00 +0200 Subject: Make migration work if LDAP is disabled --- db/migrate/20150411000035_fix_identities.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb index b65ad138b0f..297e7eaa5e9 100644 --- a/db/migrate/20150411000035_fix_identities.rb +++ b/db/migrate/20150411000035_fix_identities.rb @@ -9,9 +9,14 @@ class FixIdentities < ActiveRecord::Migration # behavior. Any database references to 'provider: ldap' get rewritten to # whatever the code would have interpreted it as, i.e. as a reference to # the first LDAP server specified in gitlab.yml / gitlab.rb. - new_provider = Gitlab.config.ldap.servers.first.last['provider_name'] + new_provider = if Gitlab.config.ldap.enabled + first_ldap_server = Gitlab.config.ldap.servers.values.first + first_ldap_server['provider_name'] + else + 'ldapmain' + end - # Delete duplicate identities + # Delete duplicate identities execute "DELETE FROM identities WHERE provider = 'ldap' AND user_id IN (SELECT user_id FROM identities WHERE provider = '#{new_provider}')" # Update legacy identities -- cgit v1.2.1 From ec7a68a1a5c7df1a1710eb1df31e410f6c171289 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 11:50:44 +0200 Subject: Simplify legacy LDAP config interpretation --- config/initializers/1_settings.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index c25c799d8f6..e23ff1f985b 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -66,10 +66,11 @@ Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil? # backwards compatibility, we only have one host if Settings.ldap['enabled'] || Rails.env.test? if Settings.ldap['host'].present? + # We detected old LDAP configuration syntax. Update the config to make it + # look like it was entered with the new syntax. server = Settings.ldap.except('sync_time') - server['provider_name'] = 'ldapmain' Settings.ldap['servers'] = { - 'ldap' => server + 'main' => server } end -- cgit v1.2.1 From afa47eddccc45ed9cfcd70891d4013c9f8d04d25 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 11:56:35 +0200 Subject: Also ldap_group_links where provider='ldap' --- db/migrate/20150411000035_fix_identities.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20150411000035_fix_identities.rb b/db/migrate/20150411000035_fix_identities.rb index d4c6e545204..8f11a96ab01 100644 --- a/db/migrate/20150411000035_fix_identities.rb +++ b/db/migrate/20150411000035_fix_identities.rb @@ -23,7 +23,7 @@ class FixIdentities < ActiveRecord::Migration execute "UPDATE identities SET provider = '#{new_provider}' WHERE provider = 'ldap';" if table_exists?('ldap_group_links') - execute "UPDATE ldap_group_links SET provider = '#{new_provider}' WHERE provider IS NULL;" + execute "UPDATE ldap_group_links SET provider = '#{new_provider}' WHERE provider IS NULL OR provider = 'ldap';" end end -- cgit v1.2.1 From 65a3928b0b5900872142b632b93f1937ad373e77 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 12:58:10 +0200 Subject: Remove test for 'ldap' provider special case --- spec/lib/gitlab/ldap/config_spec.rb | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb index 2df2beca7a6..00e9076c787 100644 --- a/spec/lib/gitlab/ldap/config_spec.rb +++ b/spec/lib/gitlab/ldap/config_spec.rb @@ -16,19 +16,5 @@ describe Gitlab::LDAP::Config do it "raises an error if a unknow provider is used" do expect{ Gitlab::LDAP::Config.new 'unknown' }.to raise_error end - - context "if 'ldap' is the provider name" do - let(:provider) { 'ldap' } - - context "and 'ldap' is not in defined as a provider" do - before { Gitlab::LDAP::Config.stub(providers: %w{ldapmain}) } - - it "uses the first provider" do - # Fetch the provider_name attribute from 'options' so that we know - # that the 'options' Hash is not empty/nil. - expect(config.options['provider_name']).to eq('ldapmain') - end - end - end end end -- cgit v1.2.1 From f39b150a02836f620fe77e1731064b5e6e01b5b1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Apr 2015 15:45:19 +0200 Subject: Call your existing LDAP server 'main' By imposing this rule we avoid having to demand that 'ldapmain' exists in the settings initializer. --- config/gitlab.yml.example | 9 +++++++++ config/initializers/1_settings.rb | 4 ---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 3d91b67e748..46b9f05cc17 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -105,6 +105,15 @@ production: &base ldap: enabled: false servers: + ########################################################################## + # + # Since GitLab 7.4, LDAP servers get ID's (below the ID is 'main'). GitLab + # Enterprise Edition now supports connecting to multiple LDAP servers. + # + # If you are updating from the old (pre-7.4) syntax, you MUST give your + # old server the ID 'main'. + # + ########################################################################## main: # 'main' is the GitLab 'provider ID' of this LDAP server ## label # diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index e23ff1f985b..d5cddb8dbf0 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -81,10 +81,6 @@ if Settings.ldap['enabled'] || Rails.env.test? server['provider_name'] ||= "ldap#{key}".downcase server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name']) end - - unless Settings.ldap['servers'].select{ |k, server| server['provider_name'] == "ldapmain"}.any? - raise "Wrong LDAP configuration. The 'main' LDAP section is missing" - end end -- cgit v1.2.1