Add validation for rich rule action

lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb

@@ -117,8 +117,8 @@ def build_rich_rule
       eval_audit,
       eval_action,
     ]
-    @resource[:raw_rule] = rule.flatten.reject { |r| r.empty? }.join(" ")
-    @resource[:raw_rule]
+    @resource[:raw_rule] = raw_rule = rule.flatten.reject { |r| r.empty? }.join(" ")
+    raw_rule
   end
 
   def create

lib/puppet/type/firewalld_rich_rule.rb

@@ -1,4 +1,5 @@
 require 'puppet'
+require_relative '../../puppet_x/firewalld/property/rich_rule_action'
 
 Puppet::Type.newtype(:firewalld_rich_rule) do
 
@@ -99,7 +100,7 @@
     desc "doc"
   end
 
-  newparam(:action) do
+  newparam(:action, :parent => PuppetX::Firewalld::Property::RichRuleAction) do
     desc "doc"
   end
 

lib/puppet_x/firewalld/property/rich_rule_action.rb

@@ -0,0 +1,21 @@
+module PuppetX
+  module Firewalld
+    module Property
+      class RichRuleAction < Puppet::Property
+        def _validate_action(value)
+          raise Puppet::Error, "Authorized action values are `accept`, `reject`, `drop` or `mark`, got #{value}" unless %w[accept drop reject mark].include? value
+        end
+        validate do |value|
+          if value.is_a?(Hash)
+            if value.keys.sort != %i[action type]
+              raise Puppet::Error, "Rule action hash should contain `action` and `type` keys. Use a string if you only want to declare the action to be `accept` or `reject`. Got #{value}"
+            end
+            _validate_action(value[:action])
+          elsif value.is_a?(String)
+            _validate_action(value)
+          end
+        end
+      end
+    end
+  end
+end

spec/unit/puppet/provider/firewalld_rich_rule_spec.rb

@@ -0,0 +1,62 @@
+require 'spec_helper'
+
+provider_class = Puppet::Type.type(:firewalld_rich_rule).provider(:firewall_cmd)
+
+describe provider_class do
+  let(:resource) do
+    @resource = Puppet::Type.type(:firewalld_rich_rule).new(
+      ensure: :present,
+      name: 'Accept ssh from barny',
+      zone: 'restricted',
+      service: 'ssh',
+      source: '192.168.1.2/32',
+      action: 'accept',
+      provider: described_class.name
+    )
+  end
+  let(:provider) { resource.provider }
+
+  before :each do
+    provider.class.stubs(:execute_firewall_cmd).returns(Object.any_instance.stubs(:exitstatus => 0))
+    provider.class.stubs(:execute_firewall_cmd).with(['--list-interfaces']).returns(Object.any_instance.stubs(:exitstatus => 0, :chomp => ""))
+  end
+
+  describe 'when creating' do
+    context 'with basic parameters' do
+      it "should build the rich rule" do
+        resource.expects(:[]).with(:source).returns('192.168.1.2/32').at_least_once
+        resource.expects(:[]).with(:service).returns('ssh').at_least_once
+        resource.expects(:[]).with("family").returns('ipv4').at_least_once
+        resource.expects(:[]).with(:dest).returns(nil)
+        resource.expects(:[]).with(:port).returns(nil)
+        resource.expects(:[]).with(:protocol).returns(nil)
+        resource.expects(:[]).with(:icmp_block).returns(nil)
+        resource.expects(:[]).with(:masquerade).returns(nil)
+        resource.expects(:[]).with(:forward_port).returns(nil)
+        resource.expects(:[]).with(:log).returns(nil)
+        resource.expects(:[]).with(:audit).returns(nil)
+        resource.expects(:[]).with(:raw_rule).returns(nil)
+        resource.expects(:[]).with(:action).returns('accept')
+        expect(provider.build_rich_rule).to eq("rule family=\"ipv4\" source service name=\"ssh\" accept")
+      end
+    end
+    context 'with reject type' do
+      it "should build the rich rule" do
+        resource.expects(:[]).with(:source).returns(nil).at_least_once
+        resource.expects(:[]).with(:service).returns('ssh').at_least_once
+        resource.expects(:[]).with("family").returns('ipv4').at_least_once
+        resource.expects(:[]).with(:dest).returns({'address' => '192.168.0.1/32'})
+        resource.expects(:[]).with(:port).returns(nil)
+        resource.expects(:[]).with(:protocol).returns(nil)
+        resource.expects(:[]).with(:icmp_block).returns(nil)
+        resource.expects(:[]).with(:masquerade).returns(nil)
+        resource.expects(:[]).with(:forward_port).returns(nil)
+        resource.expects(:[]).with(:log).returns(nil)
+        resource.expects(:[]).with(:audit).returns(nil)
+        resource.expects(:[]).with(:raw_rule).returns(nil)
+        resource.expects(:[]).with(:action).returns({:action => 'reject', :type => 'icmp-admin-prohibited'})
+        expect(provider.build_rich_rule).to eq("rule family=\"ipv4\" destination address=\"192.168.0.1/32\" service name=\"ssh\" reject type=\"icmp-admin-prohibited\"")
+      end
+    end
+  end
+end

spec/unit/puppet/type/firewalld_rich_rule_spec.rb

@@ -28,6 +28,28 @@
     end
   end
 
+  describe 'action validation' do
+    it 'raises an error if wrong action string' do
+      expect do described_class.new(
+        title: 'SSH from barny',
+        action: 'accepted',
+      ) end.to raise_error(/Authorized action values are `accept`, `reject`, `drop` or `mark`/)
+    end
+    it 'raises an error if wrong action hash keys' do
+      expect do described_class.new(
+        title: 'SSH from barny',
+        action: {:type => 'accepted', :foo => 'bar'},
+      ) end.to raise_error(/Rule action hash should contain `action` and `type` keys. Use a string if you only want to declare the action to be `accept` or `reject`/)
+    end
+    it 'raises an error if wrong action hash values' do
+      expect do described_class.new(
+        title: 'SSH from barny',
+        action: {:type => 'icmp-admin-prohibited', :action => 'accepted'},
+      ) end.to raise_error(/Authorized action values are `accept`, `reject`, `drop` or `mark`/)
+    end
+
+  end
+
   describe 'namevar validation' do
     let(:attrs) {{
       :title  => 'SSH from barny',