Adjust keycloak_required_action resource to not use metaparameter ali…

…as (#321)

README.md

@@ -603,9 +603,9 @@ The path for `install_dir` will be joined with `bin/kcadm.sh` to produce the ful
 The keycloak_required_action type can be used to define actions a user must perform during the authentication process.
 A user will not be able to complete the authentication process until these actions are complete. For instance, change a one-time password, accept T&C, etc.
 
-The name for an action is `$alias on $realm`.
+The name for an action is `$provider_id on $realm`.
 
-**Important**: actions from puppet config and from a server are matched based on a combination of alias and realm, so edition of aliases is not supported.
+**Important**: The keycloak rest api documentation uses the term `alias` which will be filled with the value of `provider_id` in this module.
 
  ```puppet
 # Minimal example

lib/puppet/provider/keycloak_required_action/kcadm.rb

@@ -11,7 +11,7 @@ def self.prefetch(resources)
     action_providers = instances
     resources.each_key do |name|
       provider = action_providers.find do |c|
-        c.alias == resources[name][:alias] && c.realm == resources[name][:realm]
+        c.provider_id == resources[name][:provider_id] && c.realm == resources[name][:realm]
       end
       if provider
         resources[name].provider = provider
@@ -34,7 +34,6 @@ def self.instances
       required_actions.each do |a|
         action = {
           ensure: :present,
-          alias: a['alias'],
           display_name: a['name'],
           realm: realm,
           enabled: a['enabled'],
@@ -61,7 +60,6 @@ def self.instances
       unregistered_actions.each do |a|
         action = {
           ensure: :absent,
-          alias: a['providerId'],
           display_name: a['name'],
           realm: realm,
           enabled: false,
@@ -105,18 +103,17 @@ def create
     # Asigning property_flush to is needed to make the flush method to
     # configure properties of the required action after the registration.
     @property_flush = resource.to_hash
-    @property_hash[:alias] = resource[:provider_id] # Initially it's equal to the provider id until configuration is applied to it
     @property_hash[:ensure] = :present
   end
 
   def destroy
     Puppet.debug('Keycloak required action: destroy')
     begin
-      kcadm('delete', "authentication/required-actions/#{@property_hash[:alias]}", resource[:realm])
+      kcadm('delete', "authentication/required-actions/#{@property_hash[:provider_id]}", resource[:realm])
     rescue StandardError => e
       raise Puppet::Error, "kcadm deletion of required action failed\nError message: #{e.message}"
     end
-    Puppet.info("Keycloak: deregistered required action #{@property_hash[:alias]} for #{resource[:realm]}")
+    Puppet.info("Keycloak: deregistered required action #{@property_hash[:provider_id]} for #{resource[:realm]}")
     @property_hash.clear
   end
 
@@ -130,16 +127,16 @@ def flush
 
     begin
       t = Tempfile.new('keycloak_required_action_configure')
-      t.write(JSON.pretty_generate(alias: resource[:alias],
+      t.write(JSON.pretty_generate(alias: resource[:provider_id],
                                    name: resource[:display_name] || @property_hash[:display_name],
                                    enabled: resource[:enabled],
                                    priority: resource[:priority],
                                    config: resource[:config] || {},
                                    defaultAction: resource[:default]))
       t.close
       Puppet.debug(IO.read(t.path))
-      kcadm('update', "authentication/required-actions/#{@property_hash[:alias]}", resource[:realm], t.path)
-      Puppet.info("Keycloak: configured required action #{@property_hash[:alias]} (provider #{resource[:provider_id]}) for #{resource[:realm]}")
+      kcadm('update', "authentication/required-actions/#{@property_hash[:provider_id]}", resource[:realm], t.path)
+      Puppet.info("Keycloak: configured required action #{@property_hash[:display_name]} (provider #{resource[:provider_id]}) for #{resource[:realm]}")
     rescue StandardError => e
       raise Puppet::Error, "kcadm configuration of required action failed\nError message: #{e.message}"
     end
@@ -150,7 +147,6 @@ def flush
 
   def to_keycloak_representation(resource)
     {
-      alias: resource[:alias],
       name: resource[:display_name],
       realm: resource[:realm],
       providerId: resource[:provider_id],

lib/puppet/type/keycloak_required_action.rb

@@ -9,7 +9,6 @@
 @example Enable Webauthn Register and make it default
   keycloak_required_action { 'webauthn-register on master':
     ensure       => present,
-    alias        => 'webauthn-register',
     provider_id  => 'webauthn-register',
     display_name => 'Webauthn Register',
     default      => true,
@@ -40,16 +39,9 @@
     desc 'realm'
   end
 
-  newparam(:alias, namevar: true) do
-    desc 'Alias.'
-  end
-
-  newparam(:provider_id) do
-    desc 'providerId of the required action. Default to `alias`'
+  newparam(:provider_id, namevar: true) do
+    desc 'providerId of the required action.'
     munge { |v| v.to_s }
-    defaultto do
-      @resource[:alias]
-    end
   end
 
   newproperty(:display_name) do
@@ -107,7 +99,7 @@ def self.title_patterns
         %r{^((\S+) on (\S+))$},
         [
           [:name],
-          [:alias],
+          [:provider_id],
           [:realm]
         ]
       ],
@@ -122,7 +114,7 @@ def self.title_patterns
 
   validate do
     required_properties = [
-      :alias,
+      :provider_id,
       :realm
     ]
     required_properties.each do |property|

spec/acceptance/10_required_action_spec.rb

@@ -87,4 +87,31 @@ class { 'keycloak': }
       end
     end
   end
+
+  context 'when required action with multiple realms' do
+    it 'runs successfully' do
+      pp = <<-PUPPET_PP
+      class { 'keycloak': }
+      keycloak_realm { 'test': ensure => 'present' }
+      keycloak_realm { 'test2': ensure => 'present' }
+      keycloak_required_action { 'webauthn-register on test':
+        ensure       => 'present',
+        display_name => 'Webauthn Register',
+        default      => true,
+        enabled      => true,
+        priority     => 200,
+      }
+      keycloak_required_action { 'webauthn-register on test2':
+        ensure       => 'present',
+        display_name => 'Webauthn Register',
+        default      => true,
+        enabled      => true,
+        priority     => 200,
+      }
+      PUPPET_PP
+
+      apply_manifest(pp, catch_failures: true)
+      apply_manifest(pp, catch_changes: true)
+    end
+  end
 end

spec/unit/puppet/provider/keycloak_required_action/kcadm_spec.rb

@@ -9,7 +9,6 @@
   let(:resource) do
     type.new(name: 'foo',
              realm: 'test',
-             alias: 'somealias',
              provider_id: 'webauthn-register')
   end
 
@@ -50,11 +49,9 @@
 
   describe 'destroy' do
     it 'deregisters a required action' do
-      # It suppoed to use whatever came from api and was matched by provider id
-      # But not what developer provided
-      resource.provider.instance_variable_set(:@property_hash, alias: 'otheralias')
+      resource.provider.instance_variable_set(:@property_hash, resource.to_hash)
 
-      expect(resource.provider).to receive(:kcadm).with('delete', 'authentication/required-actions/otheralias', 'test')
+      expect(resource.provider).to receive(:kcadm).with('delete', 'authentication/required-actions/webauthn-register', 'test')
 
       resource.provider.destroy
 
@@ -77,7 +74,7 @@
       temp = Tempfile.new('keycloak_required_action_configure')
       allow(Tempfile).to receive(:new).with('keycloak_required_action_configure').and_return(temp)
 
-      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/somealias', 'test', temp.path)
+      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/webauthn-register', 'test', temp.path)
 
       resource.provider.display_name = 'something'
       resource.provider.flush
@@ -86,11 +83,11 @@
     # If developer does not specify the display name, the api would use the name
     # that is initially returned from unregistered-required-actions
     it 'uses display_name from current state if none specified explicitly' do
-      resource.provider.instance_variable_set(:@property_hash, display_name: 'display name', alias: 'somealias')
+      resource.provider.instance_variable_set(:@property_hash, display_name: 'display name', provider_id: 'webauthn-register')
       temp = Tempfile.new('keycloak_required_action_configure')
       allow(Tempfile).to receive(:new).with('keycloak_required_action_configure').and_return(temp)
 
-      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/somealias', 'test', temp.path)
+      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/webauthn-register', 'test', temp.path)
 
       resource.provider.priority = 1000
       resource.provider.flush
@@ -106,7 +103,7 @@
       temp = Tempfile.new('keycloak_required_action_configure')
       allow(Tempfile).to receive(:new).with('keycloak_required_action_configure').and_return(temp)
 
-      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/somealias', 'test', temp.path)
+      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/webauthn-register', 'test', temp.path)
 
       resource.provider.priority = 200
       resource.provider.flush
@@ -115,18 +112,5 @@
       json = JSON.parse(data)
       expect(json['name']).to eq('something')
     end
-
-    it 'always uses alias from the current state to make edits' do
-      resource[:display_name] = 'newalias'
-      resource.provider.instance_variable_set(:@property_hash, alias: 'current')
-
-      temp = Tempfile.new('keycloak_required_action_configure')
-      allow(Tempfile).to receive(:new).with('keycloak_required_action_configure').and_return(temp)
-
-      expect(resource.provider).to receive(:kcadm).with('update', 'authentication/required-actions/current', 'test', temp.path)
-
-      resource.provider.priority = 200
-      resource.provider.flush
-    end
   end
 end

spec/unit/puppet/type/keycloak_required_action_spec.rb

@@ -7,7 +7,6 @@
     {
       name: 'foo',
       realm: 'test',
-      alias: 'something',
       provider_id: 'some-provider'
     }
   end
@@ -25,15 +24,9 @@
     }.not_to raise_error
   end
 
-  it 'has alias default to provider_id' do
-    config.delete(:provider_id)
-    expect(resource[:provider_id]).to eq('something')
-  end
-
   it 'handles componsite name' do
     component = described_class.new(name: 'foo on test')
     expect(component[:name]).to eq('foo on test')
-    expect(component[:alias]).to eq('foo')
     expect(component[:provider_id]).to eq('foo')
     expect(component[:realm]).to eq('test')
   end
@@ -49,8 +42,7 @@
       :realm,
       :name,
       :display_name,
-      :provider_id,
-      :alias
+      :provider_id
     ].each do |p|
       it "accepts a #{p}" do
         config[p] = 'foo'
@@ -163,16 +155,9 @@
       expect { resource }.to raise_error(%r{must have a realm defined})
     end
 
-    it 'requires alias' do
-      config.delete(:provider_id)
-      config.delete(:alias)
-      expect { resource }.to raise_error(%r{must have a alias defined})
-    end
-
-    it 'does not require provider_id for absent' do
+    it 'requires provider_id' do
       config.delete(:provider_id)
-      config[:ensure] = 'absent'
-      expect { resource }.not_to raise_error
+      expect { resource }.to raise_error(%r{must have a provider_id defined})
     end
   end
 end