Fixed issue #1709 - LDAP User uid attribute is not reliable.

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

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

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

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

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',

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

lib/sequencer/unit/import/ldap/user/remote_id.rb

@@ -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

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

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

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

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

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

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