BREAKING: Change how id is set for keycloak_ldap_user_provider (See R…

…EADME) (#76)

* Change how id is set for keycloak_ldap_user_provider

Change how parent_id is set for keycloak_ldap_mapper

Fix keycloak::freeipa_ldap_mappers to work with updated ID handling

Update README

* Document in README that sshd provider is no longer supported

* Fix autorequire for keycloak_ldap_mapper

README.md

@@ -7,6 +7,7 @@
 
 1. [Overview](#overview)
     * [Upgrade to 8.x](#upgrade-to-8x)
+        * [Changes to LDAP user provider IDs](#changes-to-ldap-user-provider-ids)
     * [Supported Versions of Keycloak](#supported-versions-of-keycloak)
 2. [Usage - Configuration options](#usage)
     * [Keycloak](#keycloak)
@@ -97,6 +98,67 @@ Some basic configuration options are exposed using parameters but most configura
 
 Drop Debian 9 support due to OS repos not having Java 11.
 
+#### Changes to LDAP user provider IDs
+
+If you had `keycloak_ldap_user_provider` resources defined the mechanism for defining the ID has changed and requires some migration. Also the `ldap` property for any `keycloak_ldap_mapper` resources will have to be adjusted.
+
+**WARNING** The LDAP user provider ID is used to create user IDs for LDAP users. These will change if the ID is changed. This is to prevent messages such as this: `The given key is not a valid key per specification, future migration might fail: f:OSC-LDAP-osc:tdockendorf`. If you wish to keep the old style IDs you must provide the `id` parameter as `$ldap-$realm` to maintain old IDs.
+
+It's advised to either [Migrate to new IDs](#migrate-to-new-ids) or [Keep old IDs](#keep-old-ids)
+
+##### Migrate to new IDs
+
+**Changes**
+
+* Define old `keycloak_ldap_user_provider` resource as absent with new name and setting `id` and `resource_name`.
+* Define same `keycloak_ldap_user_provider` resource to get created with new ID
+* Update `keycloak_ldap_mapper` resources to point to just name of `keycloak_ldap_user_provider`.
+
+**Before:**
+
+```puppet
+keycloak_ldap_user_provider { 'LDAP on test':
+  users_dn                  => 'ou=People,dc=test',
+  connection_url            => 'ldap://localhost:389',
+  custom_user_search_filter => '(objectClass=posixAccount)',
+}
+keycloak_ldap_mapper { "first name for LDAP-test on test":
+  ensure               => 'present',
+  type                 => 'user-attribute-ldap-mapper',
+  user_model_attribute => 'firstName',
+  ldap_attribute       => 'givenName',
+}
+```
+
+**After:**
+
+```
+keycloak_ldap_user_provider { 'LDAP-remove on test':
+  ensure        => 'absent',
+  resource_name => 'LDAP',
+  id            => 'LDAP-test',
+}
+keycloak_ldap_user_provider { 'LDAP on test':
+  users_dn                  => 'ou=People,dc=test',
+  connection_url            => 'ldap://localhost:389',
+  custom_user_search_filter => '(objectClass=posixAccount)',
+}
+keycloak_ldap_mapper { "first name for LDAP on test":
+  ensure               => 'present',
+  type                 => 'user-attribute-ldap-mapper',
+  user_model_attribute => 'firstName',
+  ldap_attribute       => 'givenName',
+}
+```
+
+##### Keep old IDs
+
+If you wish to avoid re-creating `keycloak_ldap_user_provider` and `keycloak_ldap_mapper` resources then the ID parameters must be defined.
+
+For `keycloak_ldap_user_provider` ensure the `id` property is set to match the old pattern. If name was `LDAP` and realm `test` or name was componsite `LDAP on test` then set `id` to `LDAP-test`.
+
+For `keycloak_ldap_mapper` ensure the `parent_id` property is set to point to old ID for associated `keycloak_ldap_user_provider`. If the `ldap` value is `LDAP` and `realm` is `test` or composite name is `first name for LDAP on test` then ensure `parent_id` is set to `LDAP-test`.
+
 ### Supported Versions of Keycloak
 
 Currently this module supports Keycloak version 12.x.
@@ -286,8 +348,6 @@ keycloak_ldap_user_provider { 'LDAP on test':
 }
 ```
 
-**NOTE** The `Id` for the above resource would be `LDAP-test` where the format is `${resource_name}-${realm}`.
-
 If you're using FreeIPA you can use a defined resource that wraps keycloak\_ldap\_user\_provider:
 
 ```puppet
@@ -327,6 +387,8 @@ keycloak::freeipa_ldap_mappers { 'ipa.example.org':
 
 ### keycloak\_sssd\_user\_provider
 
+**WARNING** This feature is no longer tested and likely stopped working when Keycloak began requiring Java 11+. If you rely on this feature, please open an issue or pull request. Likely need to build jna from source.
+
 Define SSSD user provider.  **NOTE** This type requires that SSSD be properly configured and Keycloak service restarted after SSSD ifp service is setup.  Also requires `keycloak` class be called with `with_sssd_support` set to `true`.
 
 ```puppet
@@ -539,3 +601,13 @@ This module has been tested on:
 * Debian 11 x86_64
 * Ubuntu 18.04 x86_64
 * Ubuntu 20.04 x86_64
+
+## UUID Generation
+
+```
+bundle exec irb
+2.5.1 :001 > require File.expand_path(File.join(File.dirname(__FILE__), 'lib/puppet/provider/keycloak_api'))
+ => true 
+2.5.1 :002 > Puppet::Provider::KeycloakAPI.name_uuid('LDAP')
+ => "bc7bc27f-39b8-5152-91c3-915d710fba35" 
+```

lib/puppet/provider/keycloak_ldap_mapper/kcadm.rb

@@ -32,6 +32,7 @@ def type_supported_properties(type)
   def self.instances
     components = []
     realms.each do |realm|
+      component_ids = {}
       output = kcadm('get', 'components', realm)
       Puppet.debug("#{realm} components: #{output}")
       begin
@@ -41,6 +42,12 @@ def self.instances
         data = []
       end
 
+      data.each do |d|
+        next unless d['providerType'] == 'org.keycloak.storage.UserStorageProvider'
+        component_ids[d['id']] = d['name']
+      end
+      Puppet.debug("Realm #{realm} component_ids: #{component_ids}")
+
       data.each do |d|
         next unless d['providerType'] == 'org.keycloak.storage.ldap.mappers.LDAPStorageMapper'
         next unless [
@@ -51,7 +58,8 @@ def self.instances
         component[:id] = d['id']
         component[:realm] = realm
         component[:resource_name] = d['name']
-        component[:ldap] = d['parentId']
+        component[:parent_id] = d['parentId']
+        component[:ldap] = component_ids[component[:parent_id]]
         component[:type] = d['providerId']
         component[:name] = "#{component[:resource_name]} for #{component[:ldap]} on #{component[:realm]}"
         type_properties.each do |property|
@@ -86,11 +94,30 @@ def self.prefetch(resources)
     end
   end
 
+  def get_parent_id(ldap, realm)
+    parent_id = nil
+    output = kcadm('get', 'components', realm, nil, ['name', 'realm', 'id', 'providerType'])
+    Puppet.debug("#{realm} components: #{output}")
+    begin
+      data = JSON.parse(output)
+    rescue JSON::ParserError
+      Puppet.debug('Unable to parse output from kcadm get components')
+      data = []
+    end
+    data.each do |d|
+      next unless d['providerType'] == 'org.keycloak.storage.UserStorageProvider'
+      if d['name'] == ldap
+        parent_id = d['id']
+      end
+    end
+    parent_id
+  end
+
   def create
     data = {}
     data[:id] = resource[:id] || name_uuid(resource[:name])
     data[:name] = resource[:resource_name]
-    data[:parentId] = resource[:ldap]
+    data[:parentId] = resource[:parent_id] || get_parent_id(resource[:ldap], resource[:realm]) || name_uuid(resource[:ldap])
     data[:providerId] = resource[:type]
     data[:providerType] = 'org.keycloak.storage.ldap.mappers.LDAPStorageMapper'
     data[:config] = {}

lib/puppet/provider/keycloak_ldap_user_provider/kcadm.rb

@@ -46,7 +46,7 @@ def self.instances
   def self.prefetch(resources)
     components = instances
     resources.keys.each do |name|
-      provider = components.find { |c| c.resource_name == resources[name][:resource_name] && c.realm == resources[name][:realm] }
+      provider = components.find { |c| c.id == resources[name][:id] }
       if provider
         resources[name].provider = provider
       end
@@ -57,7 +57,7 @@ def create
     raise(Puppet::Error, "Realm is mandatory for #{resource.type} #{resource.name}") if resource[:realm].nil?
 
     data = {}
-    data[:id] = resource[:id]
+    data[:id] = resource[:id] || name_uuid(resource[:name])
     data[:name] = resource[:resource_name]
     data[:parentId] = resource[:realm]
     data[:providerId] = 'ldap'

lib/puppet/type/keycloak_ldap_mapper.rb

@@ -45,6 +45,10 @@
   end
 
   newparam(:ldap, namevar: true) do
+    desc 'Name of parent `keycloak_ldap_user_provider` resource'
+  end
+
+  newparam(:parent_id) do
     desc 'parentId'
   end
 
@@ -300,7 +304,7 @@
     requires = []
     catalog.resources.each do |resource|
       next unless resource.class.to_s == 'Puppet::Type::Keycloak_ldap_user_provider'
-      if self[:ldap] == "#{resource[:resource_name]}-#{resource[:realm]}"
+      if self[:ldap] == resource[:resource_name] && self[:realm] == resource[:realm]
         requires << resource.name
       end
     end

lib/puppet/type/keycloak_ldap_user_provider.rb

@@ -32,9 +32,9 @@
   end
 
   newparam(:id) do
-    desc 'Id. Defaults to "`resource_name`-`realm`"'
+    desc 'Id'
     defaultto do
-      "#{@resource[:resource_name]}-#{@resource[:realm]}"
+      @resource.provider.name_uuid(@resource[:name])
     end
   end
 

manifests/freeipa_ldap_mappers.pp

@@ -15,19 +15,17 @@
 # @param roles_dn
 #   Roles DN
 # @param parent_id
-#   Identifier (parentId) for the LDAP provider to add this mapper to.
-#   Will be passed to the $ldap parameter in keycloak_ldap_mapper.
+#   Used to identify the parent LDAP user provider, name used with keycloak::freeipa_user_provider
 #
 define keycloak::freeipa_ldap_mappers
 (
   String           $realm,
   String           $groups_dn,
   String           $roles_dn,
-  Optional[String] $parent_id = undef,
+  String           $parent_id = $title,
 )
 {
-  $_parent_id = pick($parent_id, "${title}-${realm}")
-  $title_suffix = "for ${_parent_id}"
+  $title_suffix = "for ${title}"
 
   # This translates to parentId in JSON and must be correct or hard-to-debug
   # issues will ensue.
@@ -36,7 +34,7 @@
     default:
       ensure                      => 'present',
       realm                       => $realm,
-      ldap                        => $_parent_id,
+      ldap                        => $parent_id,
       always_read_value_from_ldap => true,
       read_only                   => true,
       is_mandatory_in_ldap        => true,
@@ -81,7 +79,7 @@
     ensure                         => 'present',
     realm                          => $realm,
     type                           => 'role-ldap-mapper',
-    ldap                           => $_parent_id,
+    ldap                           => $parent_id,
     is_mandatory_in_ldap           => false,
     mode                           => 'READ_ONLY',
     memberof_ldap_attribute        => 'memberOf',
@@ -99,7 +97,7 @@
     ensure                               => 'present',
     realm                                => $realm,
     type                                 => 'group-ldap-mapper',
-    ldap                                 => $_parent_id,
+    ldap                                 => $parent_id,
     is_mandatory_in_ldap                 => false,
     mode                                 => 'READ_ONLY',
     memberof_ldap_attribute              => 'memberOf',