From 132a9997eb13198c1a1b1d857802ca7c334677a0 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 12 Jan 2018 12:53:43 +0100 Subject: [PATCH 1/2] Fixed issue #1709 - LDAP User uid attribute is not reliable. --- ...180111000001_ldap_samaccountname_to_uid.rb | 10 ++ lib/ldap/guid.rb | 108 ++++++++++++++++++ lib/ldap/user.rb | 3 +- .../ldap_samaccountname_to_uid.rb | 58 ++++++++++ lib/sequencer/sequence/import/ldap/user.rb | 3 +- .../unit/import/ldap/user/mapping.rb | 4 +- .../unit/import/ldap/user/remote_id.rb | 19 --- .../import/ldap/user/remote_id/from_entry.rb | 22 ++++ .../unit/import/ldap/user/remote_id/unhex.rb | 28 +++++ .../unit/import/ldap/users/sub_sequence.rb | 2 + spec/lib/ldap/guid_spec.rb | 82 +++++++++++++ .../ldap_samaccountname_to_uid_spec.rb | 49 ++++++++ .../import/ldap/user/remote_id/unhex_spec.rb | 24 ++++ 13 files changed, 388 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20180111000001_ldap_samaccountname_to_uid.rb create mode 100644 lib/ldap/guid.rb create mode 100644 lib/migration_job/ldap_samaccountname_to_uid.rb delete mode 100644 lib/sequencer/unit/import/ldap/user/remote_id.rb create mode 100644 lib/sequencer/unit/import/ldap/user/remote_id/from_entry.rb create mode 100644 lib/sequencer/unit/import/ldap/user/remote_id/unhex.rb create mode 100644 spec/lib/ldap/guid_spec.rb create mode 100644 spec/lib/migration_job/ldap_samaccountname_to_uid_spec.rb create mode 100644 spec/lib/sequencer/unit/import/ldap/user/remote_id/unhex_spec.rb diff --git a/db/migrate/20180111000001_ldap_samaccountname_to_uid.rb b/db/migrate/20180111000001_ldap_samaccountname_to_uid.rb new file mode 100644 index 000000000..6a79d14b3 --- /dev/null +++ b/db/migrate/20180111000001_ldap_samaccountname_to_uid.rb @@ -0,0 +1,10 @@ +class LdapSamaccountnameToUid < ActiveRecord::Migration[5.1] + + def up + # return if it's a new setup to avoid running the migration + return if !Setting.find_by(name: 'system_init_done') + + Delayed::Job.enqueue MigrationJob::LdapSamaccountnameToUid.new + end + +end diff --git a/lib/ldap/guid.rb b/lib/ldap/guid.rb new file mode 100644 index 000000000..e82c54e95 --- /dev/null +++ b/lib/ldap/guid.rb @@ -0,0 +1,108 @@ +class Ldap + + # Class for handling LDAP GUIDs. + # strongly inspired by + # https://gist.github.com/astockwell/359c950fbc650c339eea + # Big thanks to @astockwell + class Guid + + # Checks if the given string is a valid GUID. + # + # @param string [String] The string that should be checked for valid GUID format. + # + # @example + # Ldap::Guid.valid?('f742b361-32c6-4a92-baaa-eaae7df657ee') + # #=> true + # + # @return [Boolean] + def self.valid?(string) + string.match?(/\w{8}\-\w{4}\-\w{4}\-\w{4}\-\w+/) + end + + # Convers a given GUID string into the HEX equivalent. + # + # @param string [String] The GUID string that should converted into HEX. + # + # @example + # Ldap::Guid.hex('f742b361-32c6-4a92-baaa-eaae7df657ee') + # #=> "a\xB3B\xF7\xC62\x92J\xBA\xAA\xEA\xAE}\xF6W\xEE".b + # + # @return [String] The HEX equivalent of the given GUID string. + def self.hex(string) + new(string).hex + end + + # Convers a given HEX string into the GUID equivalent. + # + # @param string [String] The HEX string that should converted into a GUID. + # + # @example + # Ldap::Guid.string("a\xB3B\xF7\xC62\x92J\xBA\xAA\xEA\xAE}\xF6W\xEE".b) + # #=> 'f742b361-32c6-4a92-baaa-eaae7df657ee' + # + # @return [String] The GUID equivalent of the given HEX string. + def self.string(hex) + new(hex).string + end + + # Initializes an instance for the LDAP::Guid class to convert from/to HEX and GUID strings. + # + # @param string [String] The HEX or GUID string that should converted. + # + # @example + # guid = Ldap::Guid.new('f742b361-32c6-4a92-baaa-eaae7df657ee') + # + # @return [nil] + def initialize(guid) + @guid = guid + end + + # Convers the GUID string into the HEX equivalent. + # + # @example + # guid.hex + # #=> "a\xB3B\xF7\xC62\x92J\xBA\xAA\xEA\xAE}\xF6W\xEE".b + # + # @return [String] The HEX equivalent of the GUID string. + def hex + [oracle_raw16(guid)].pack('H*') + end + + # Convers the HEX string into the GUID equivalent. + # + # @example + # guid.string + # #=> 'f742b361-32c6-4a92-baaa-eaae7df657ee' + # + # @return [String] The GUID equivalent of the HEX string. + def string + oracle_raw16(guid.unpack('H*').first, dashify: true) + end + + private + + attr_reader :guid + + def oracle_raw16(string, dashify: false) + # remove dashes + string.delete!('-') + + # split every two chars + parts = string.scan(/.{1,2}/) + + # re-order according to oracle format index and join + oracle_format_indices = [3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11, 12, 13, 14, 15] + result = oracle_format_indices.map { |index| parts[index] }.join('') + + # add dashes if requested + return result if !dashify + [ + result[0..7], + result[8..11], + result[12..15], + result[16..19], + result[20..result.size] + ].join('-') + end + end +end diff --git a/lib/ldap/user.rb b/lib/ldap/user.rb index 3a7b99a1d..d53e30ecb 100644 --- a/lib/ldap/user.rb +++ b/lib/ldap/user.rb @@ -26,7 +26,6 @@ class Ldap usercertificate objectclass objectcategory - objectguid objectsid primarygroupid pwdlastset @@ -61,7 +60,7 @@ class Ldap # @return [String] The uid attribute. def self.uid_attribute(attributes) result = nil - %i[samaccountname userprincipalname uid dn].each do |attribute| + %i[objectguid entryuuid samaccountname userprincipalname uid dn].each do |attribute| next if attributes[attribute].blank? result = attribute.to_s break diff --git a/lib/migration_job/ldap_samaccountname_to_uid.rb b/lib/migration_job/ldap_samaccountname_to_uid.rb new file mode 100644 index 000000000..810be1f10 --- /dev/null +++ b/lib/migration_job/ldap_samaccountname_to_uid.rb @@ -0,0 +1,58 @@ +require 'ldap' +require 'ldap/user' + +module MigrationJob + class LdapSamaccountnameToUid + + def perform + Rails.logger.info 'Checking for active LDAP configuration...' + + if ldap_config.blank? + Rails.logger.info 'Blank LDAP configuration. Exiting.' + return + end + + Rails.logger.info 'Checking for different LDAP uid attribute...' + if uid_attribute_obsolete == uid_attribute_new + Rails.logger.info 'Equal LDAP uid attributes. Exiting.' + return + end + + Rails.logger.info 'Starting to migrate LDAP config to new uid attribute...' + migrate_ldap_config + Rails.logger.info 'LDAP uid attribute migration completed.' + end + + private + + def ldap + @ldap ||= ::Ldap.new(ldap_config) + end + + def ldap_config + @ldap_config ||= Import::Ldap.config + end + + def uid_attribute_new + @uid_attribute_new ||= begin + config = { + filter: ldap_config['user_filter'] + } + + ::Ldap::User.new(config, ldap: ldap).uid_attribute + end + end + + def uid_attribute_obsolete + @uid_attribute_obsolete ||= ldap_config['user_uid'] + end + + def migrate_ldap_config + ldap_config_new = ldap_config.merge( + 'user_uid' => uid_attribute_new + ) + + Setting.set('ldap_config', ldap_config_new) + end + end +end diff --git a/lib/sequencer/sequence/import/ldap/user.rb b/lib/sequencer/sequence/import/ldap/user.rb index 2ee175b1b..58a0f2ca3 100644 --- a/lib/sequencer/sequence/import/ldap/user.rb +++ b/lib/sequencer/sequence/import/ldap/user.rb @@ -11,7 +11,8 @@ class Sequencer def self.sequence [ 'Import::Ldap::User::NormalizeEntry', - 'Import::Ldap::User::RemoteId', + 'Import::Ldap::User::RemoteId::FromEntry', + 'Import::Ldap::User::RemoteId::Unhex', 'Import::Ldap::User::Mapping', 'Import::Ldap::User::Skip::MissingMandatory', 'Import::Ldap::User::Skip::Blank', diff --git a/lib/sequencer/unit/import/ldap/user/mapping.rb b/lib/sequencer/unit/import/ldap/user/mapping.rb index 313d14a59..53fe7498c 100644 --- a/lib/sequencer/unit/import/ldap/user/mapping.rb +++ b/lib/sequencer/unit/import/ldap/user/mapping.rb @@ -10,10 +10,10 @@ class Sequencer def mapping ldap_config[:user_attributes].dup.tap do |config| - # fallback to uid as login + # fallback to samaccountname as login # if no login is given via mapping if !config.values.include?('login') - config[ ldap_config[:user_uid] ] = 'login' + config['samaccountname'] = 'login' end end end diff --git a/lib/sequencer/unit/import/ldap/user/remote_id.rb b/lib/sequencer/unit/import/ldap/user/remote_id.rb deleted file mode 100644 index 0b3ead808..000000000 --- a/lib/sequencer/unit/import/ldap/user/remote_id.rb +++ /dev/null @@ -1,19 +0,0 @@ -class Sequencer - class Unit - module Import - module Ldap - module User - class RemoteId < Sequencer::Unit::Import::Common::Model::Attributes::RemoteId - uses :ldap_config - - private - - def attribute - ldap_config[:user_uid].to_sym - end - end - end - end - end - end -end diff --git a/lib/sequencer/unit/import/ldap/user/remote_id/from_entry.rb b/lib/sequencer/unit/import/ldap/user/remote_id/from_entry.rb new file mode 100644 index 000000000..fbc940df1 --- /dev/null +++ b/lib/sequencer/unit/import/ldap/user/remote_id/from_entry.rb @@ -0,0 +1,22 @@ +class Sequencer + class Unit + module Import + module Ldap + module User + module RemoteId + class FromEntry < Sequencer::Unit::Import::Common::Model::Attributes::RemoteId + + uses :ldap_config + + private + + def attribute + ldap_config[:user_uid].to_sym + end + end + end + end + end + end + end +end diff --git a/lib/sequencer/unit/import/ldap/user/remote_id/unhex.rb b/lib/sequencer/unit/import/ldap/user/remote_id/unhex.rb new file mode 100644 index 000000000..9e66fbb1e --- /dev/null +++ b/lib/sequencer/unit/import/ldap/user/remote_id/unhex.rb @@ -0,0 +1,28 @@ +class Sequencer + class Unit + module Import + module Ldap + module User + module RemoteId + class Unhex < Sequencer::Unit::Base + + uses :remote_id + provides :remote_id + + def process + return if remote_id.ascii_only? + state.provide(:remote_id, unhexed) + end + + private + + def unhexed + ::Ldap::Guid.string(remote_id) + end + end + end + end + end + end + end +end diff --git a/lib/sequencer/unit/import/ldap/users/sub_sequence.rb b/lib/sequencer/unit/import/ldap/users/sub_sequence.rb index ad5684219..05066c669 100644 --- a/lib/sequencer/unit/import/ldap/users/sub_sequence.rb +++ b/lib/sequencer/unit/import/ldap/users/sub_sequence.rb @@ -49,6 +49,8 @@ class Sequencer # those which are needed to improve the performance attributes = ldap_config[:user_attributes].keys attributes.push('dn') + attributes.push(ldap_config[:user_uid]) + attributes.uniq end end end diff --git a/spec/lib/ldap/guid_spec.rb b/spec/lib/ldap/guid_spec.rb new file mode 100644 index 000000000..aed19ecf7 --- /dev/null +++ b/spec/lib/ldap/guid_spec.rb @@ -0,0 +1,82 @@ +require 'rails_helper' + +RSpec.describe Ldap::Guid do + + let(:string) { 'f742b361-32c6-4a92-baaa-eaae7df657ee' } + let(:hex) { "a\xB3B\xF7\xC62\x92J\xBA\xAA\xEA\xAE}\xF6W\xEE".b } + + context '.valid?' do + + it 'responds to .valid?' do + expect(described_class).to respond_to(:valid?) + end + + it 'detects valid uid string' do + expect(described_class.valid?(string)).to be true + end + + it 'detects invalid uid string' do + invalid = 'AC2342' + expect(described_class.valid?(invalid)).to be false + end + end + + context '.hex' do + + it 'responds to .hex' do + expect(described_class).to respond_to(:hex) + end + + it 'tunnels to instance method' do + + instance = double() + expect(instance).to receive(:hex) + expect(described_class).to receive(:new).with(string).and_return(instance) + + described_class.hex(string) + end + end + + context '.string' do + + it 'responds to .string' do + expect(described_class).to respond_to(:string) + end + + it 'tunnels to instance method' do + + instance = double() + expect(instance).to receive(:string) + expect(described_class).to receive(:new).with(hex).and_return(instance) + + described_class.string(hex) + end + end + + context '#string' do + + let(:instance) { described_class.new(hex) } + + it 'responds to #string' do + expect(instance).to respond_to(:string) + end + + it 'converts to string' do + expect(instance.string).to eq(string) + end + end + + context '#hex' do + + let(:instance) { described_class.new(string) } + + it 'responds to #hex' do + expect(instance).to respond_to(:hex) + end + + it 'converts to hex' do + expect(instance.hex).to eq(hex) + end + end + +end diff --git a/spec/lib/migration_job/ldap_samaccountname_to_uid_spec.rb b/spec/lib/migration_job/ldap_samaccountname_to_uid_spec.rb new file mode 100644 index 000000000..fe9558ae1 --- /dev/null +++ b/spec/lib/migration_job/ldap_samaccountname_to_uid_spec.rb @@ -0,0 +1,49 @@ +require 'rails_helper' + +RSpec.describe MigrationJob::LdapSamaccountnameToUid do + + it 'performs no changes if no LDAP config present' do + expect(Setting).to_not receive(:set) + expect(Import::Ldap).to receive(:config).and_return(nil) + + described_class.new.perform + end + + it 'performs no changes if uid attributes equals' do + expect(Setting).to_not receive(:set) + + ldap_config = { + 'user_uid' => 'samaccountname' + } + expect(Import::Ldap).to receive(:config).and_return(ldap_config) + + ldap_user = double() + expect(ldap_user).to receive(:uid_attribute).and_return('samaccountname') + expect(::Ldap::User).to receive(:new).and_return(ldap_user) + + allow(::Ldap).to receive(:new) + + described_class.new.perform + end + + it 'performs Setting change if uid attribute differ' do + ldap_config_new = { + 'user_uid' => 'objectguid' + } + ldap_config_obsolete = { + 'user_uid' => 'samaccountname' + } + + expect(Setting).to receive(:set).with('ldap_config', ldap_config_new) + + expect(Import::Ldap).to receive(:config).and_return(ldap_config_obsolete) + + ldap_user = double() + expect(ldap_user).to receive(:uid_attribute).and_return('objectguid') + expect(::Ldap::User).to receive(:new).and_return(ldap_user) + + allow(::Ldap).to receive(:new) + + described_class.new.perform + end +end diff --git a/spec/lib/sequencer/unit/import/ldap/user/remote_id/unhex_spec.rb b/spec/lib/sequencer/unit/import/ldap/user/remote_id/unhex_spec.rb new file mode 100644 index 000000000..a514acfb6 --- /dev/null +++ b/spec/lib/sequencer/unit/import/ldap/user/remote_id/unhex_spec.rb @@ -0,0 +1,24 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Ldap::User::RemoteId::Unhex, sequencer: :unit do + + it 'unhexes hexed UID remote_ids' do + + provided = process( + remote_id: "a\xB3B\xF7\xC62\x92J\xBA\xAA\xEA\xAE}\xF6W\xEE", + ) + + expect(provided[:remote_id]).to eq('f742b361-32c6-4a92-baaa-eaae7df657ee') + end + + it 'ignores not hexed remote_ids' do + + remote_id = 'f742b361-32c6-4a92-baaa-eaae7df657ee' + + provided = process( + remote_id: remote_id, + ) + + expect(provided[:remote_id]).to eq(remote_id) + end +end From 1ec2b514bb574937bc378f46fa922768a58d5dca Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 12 Jan 2018 13:38:43 +0100 Subject: [PATCH 2/2] Fixed issue #1746 - Multiple LDAP users with same samaccountname cause repeatedly update of local user. --- .../import/ldap/user/lookup/attributes.rb | 19 +++++- .../unit/import/ldap/users/sub_sequence.rb | 8 ++- .../ldap/user/lookup/attributes_spec.rb | 64 +++++++++++++++++++ 3 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 spec/lib/sequencer/unit/import/ldap/user/lookup/attributes_spec.rb diff --git a/lib/sequencer/unit/import/ldap/user/lookup/attributes.rb b/lib/sequencer/unit/import/ldap/user/lookup/attributes.rb index a04e49722..c97b93e9e 100644 --- a/lib/sequencer/unit/import/ldap/user/lookup/attributes.rb +++ b/lib/sequencer/unit/import/ldap/user/lookup/attributes.rb @@ -4,11 +4,24 @@ class Sequencer module Ldap module User module Lookup - class Attributes < Sequencer::Unit::Import::Common::Model::Lookup::Attributes + class Attributes < Sequencer::Unit::Import::Common::Model::FindBy::UserAttributes + + uses :found_ids, :external_sync_source + private - def attributes - %i[login email] + def lookup(attribute:, value:) + entries = model_class.where(attribute => value).to_a + return if entries.blank? + not_synced(entries) + end + + def not_synced(entries) + entries.find(&method(:not_synced?)) + end + + def not_synced?(entry) + found_ids.exclude?(entry.id) end end end diff --git a/lib/sequencer/unit/import/ldap/users/sub_sequence.rb b/lib/sequencer/unit/import/ldap/users/sub_sequence.rb index 05066c669..b890ad508 100644 --- a/lib/sequencer/unit/import/ldap/users/sub_sequence.rb +++ b/lib/sequencer/unit/import/ldap/users/sub_sequence.rb @@ -12,7 +12,6 @@ class Sequencer provides :found_ids def process - found_ids = [] ldap_connection.search(ldap_config[:user_filter], attributes: relevant_attributes) do |entry| result = sequence_resource(entry) @@ -26,13 +25,18 @@ class Sequencer private + def found_ids + @found_ids ||= [] + end + def default_params super.merge( dn_roles: dn_roles, ldap_config: ldap_config, model_class: model_class, external_sync_source: external_sync_source, - signup_role_ids: signup_role_ids + signup_role_ids: signup_role_ids, + found_ids: found_ids, ) end diff --git a/spec/lib/sequencer/unit/import/ldap/user/lookup/attributes_spec.rb b/spec/lib/sequencer/unit/import/ldap/user/lookup/attributes_spec.rb new file mode 100644 index 000000000..a3ba06c4e --- /dev/null +++ b/spec/lib/sequencer/unit/import/ldap/user/lookup/attributes_spec.rb @@ -0,0 +1,64 @@ +require 'rails_helper' + +RSpec.describe Sequencer::Unit::Import::Ldap::User::Lookup::Attributes, sequencer: :unit do + + let(:model_class) { ::User } + let(:external_sync_source) { 'test' } + + it 'finds entries via lookup attributes' do + + current_user = create(:user) + + # ExternalSync.create( + # source: external_sync_source, + # source_id: remote_id, + # o_id: user.id, + # object: user.class, + # ) + + provided = process( + found_ids: [], + model_class: model_class, + external_sync_source: external_sync_source, + mapped: { + login: current_user.login, + email: current_user.email, + } + ) + + expect(provided[:instance]).to eq(current_user) + end + + it "doesn't find already synced/found entries with same lookup attributes" do + + other_user = create(:user) + + provided = process( + found_ids: [other_user.id], + model_class: model_class, + external_sync_source: external_sync_source, + mapped: { + login: other_user.login, + email: other_user.email, + } + ) + + expect(provided[:instance]).to be_nil + end + + it "doesn't not synced users" do + + provided = process( + found_ids: [], + model_class: model_class, + external_sync_source: external_sync_source, + mapped: { + login: 'example.login', + email: 'test@example.com', + } + ) + + expect(provided[:instance]).to be_nil + end + +end