Properly handle frozen strings

lib/puppet/type/posix_acl.rb

@@ -208,46 +208,45 @@ def insync?(is)
 
     # Munge into normalised form
     munge do |acl|
-      r = ''
+      result = ''
       a = acl.split ':', -1 # -1 keeps trailing empty fields.
       raise ArgumentError, "Too few fields.  At least 3 required, got #{a.length}." if a.length < 3
       raise ArgumentError, "Too many fields.  At most 4 allowed, got #{a.length}."  if a.length > 4
 
       if a.length == 4
-        d = a.shift
-        raise ArgumentError, %(First field of 4 must be "d" or "default", got "#{d}".) unless %w[d default].include?(d)
+        default, type, who, perms = a
+        raise ArgumentError, %(First field of 4 must be "d" or "default", got "#{default}".) unless %w[d default].include?(default)
 
-        r << 'default:'
+        result += 'default:'
+      else
+        type, who, perms = a
       end
-      t = a.shift # Copy the type.
-      r += case t
-           when 'u', 'user'
-             'user:'
-           when 'g', 'group'
-             'group:'
-           when 'o', 'other'
-             'other:'
-           when 'm', 'mask'
-             'mask:'
-           else
-             raise ArgumentError, %(Unknown type "#{t}", expected "user", "group", "other" or "mask".)
-           end
-      r << "#{a.shift}:" # Copy the "who".
-      p = a.shift
-      if p =~ %r{[0-7]}
-        p = p.oct
-        r << (p | 4 ? 'r' : '-')
-        r << (p | 2 ? 'w' : '-')
-        r << (p | 1 ? 'x' : '-')
+
+      result += case type
+                when 'u', 'user'
+                  'user:'
+                when 'g', 'group'
+                  'group:'
+                when 'o', 'other'
+                  'other:'
+                when 'm', 'mask'
+                  'mask:'
+                else
+                  raise ArgumentError, %(Unknown type "#{t}", expected "user", "group", "other" or "mask".)
+                end
+      result += "#{who}:"
+      if perms.match?(%r{^[0-7]$})
+        octal = perms.oct
+        result += "#{octal & 4 == 4 ? 'r' : '-'}#{octal & 2 == 2 ? 'w' : '-'}#{octal & 1 == 1 ? 'x' : '-'}"
       else
-        # Not the most efficient but checks for multiple and invalid chars.
-        s = p.tr '-', ''
-        r << (s.sub!('r', '') ? 'r' : '-')
-        r << (s.sub!('w', '') ? 'w' : '-')
-        r << (s.sub!(%r{x}i, '') ? $LAST_MATCH_INFO.to_s : '-')
-        raise ArgumentError, %(Invalid permission set "#{p}".) unless s.empty?
+        match = %r{^(?<read>r)?(?<write>w)?(?<execute>[xX])?$}.match(perms.tr('-', ''))
+        raise ArgumentError, %(Invalid permission set "#{p}".) unless match
+
+        %w[read write execute].each do |perm|
+          result += match[perm] || '-'
+        end
       end
-      r
+      result
     end
   end
 

spec/acceptance/basic_posix_acl_spec.rb

@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper_acceptance'
+
+describe 'posix_acl' do
+  context 'on a simple acl with an additional user' do
+    it_behaves_like 'an idempotent resource' do
+      let(:manifest) do
+        <<-PUPPET
+        include posix_acl::requirements
+        user { 'blub':
+          ensure => 'present',
+        }
+        file { '/opt/test':
+          ensure => directory,
+          owner  => root,
+          group  => root,
+          mode   => '2770',
+        }
+        -> posix_acl { '/opt/test':
+          action     => exact,
+          permission => [
+            'user::rwx',
+            'group::rwx',
+            'mask::rwx',
+            'other::---',
+            'user:blub:r-x',
+          ],
+          provider   => posixacl,
+          recursive  => false,
+          require => User['blub'],
+        }
+        PUPPET
+      end
+    end
+  end
+
+  context 'on a simple acl with an additional user and defaults' do
+    it_behaves_like 'an idempotent resource' do
+      let(:manifest) do
+        <<-PUPPET2
+        include posix_acl::requirements
+        user { 'blub2':
+          ensure => 'present',
+        }
+        file { '/opt/test2':
+          ensure => directory,
+          owner  => root,
+          group  => root,
+          mode   => '2770',
+        }
+        -> posix_acl { '/opt/test2':
+          action     => exact,
+          permission => [
+            'user::rwx',
+            'group::rwx',
+            'mask::rwx',
+            'other::---',
+            'user:blub2:r-x',
+            'default:user::rwx',
+            'default:group::rwx',
+            'default:mask::rwx',
+            'default:other::---',
+            'default:user:blub2:r-x',
+          ],
+          provider   => posixacl,
+          recursive  => false,
+          require    => User['blub2'],
+        }
+        PUPPET2
+      end
+    end
+  end
+end

spec/unit/puppet/type/acl_spec.rb

@@ -3,13 +3,12 @@
 require 'spec_helper'
 acl_type = Puppet::Type.type(:posix_acl)
 
-# rubocop:disable RSpec/UnspecifiedException
 describe acl_type do
   context 'when not setting parameters' do
     it 'fails without permissions' do
       expect do
         acl_type.new name: '/tmp/foo'
-      end.to raise_error
+      end.to raise_error(Puppet::ResourceError)
     end
   end
 
@@ -20,6 +19,24 @@
       expect(resource[:permission]).to eq(['user:root:rwx'])
     end
 
+    it 'works with a correct permission parameter that includes a default' do
+      resource = acl_type.new name: '/tmp/foo', permission: ['default:user:root:rwx']
+      expect(resource[:name]).to eq('/tmp/foo')
+      expect(resource[:permission]).to eq(['default:user:root:rwx'])
+    end
+
+    it 'works with octal 0 permission parameter' do
+      resource = acl_type.new name: '/tmp/foo', permission: ['user:root:0']
+      expect(resource[:name]).to eq('/tmp/foo')
+      expect(resource[:permission]).to eq(['user:root:---'])
+    end
+
+    it 'works with octal 7 permission parameter' do
+      resource = acl_type.new name: '/tmp/foo', permission: ['user:root:7']
+      expect(resource[:name]).to eq('/tmp/foo')
+      expect(resource[:permission]).to eq(['user:root:rwx'])
+    end
+
     it 'converts a permission string to an array' do
       resource = acl_type.new name: '/tmp/foo', permission: 'user:root:rwx'
       expect(resource[:name]).to eq('/tmp/foo')
@@ -146,34 +163,46 @@
       expect(resource[:ignore_missing]).to eq(:notify)
     end
 
+    it 'fails with an invalid default' do
+      expect do
+        acl_type.new name: '/tmp/foo', permission: ['o::rwx:wrong']
+      end.to raise_error(Puppet::ResourceError, %r{First field of 4 must be})
+    end
+
     it 'fails with a wrong action' do
       expect do
         acl_type.new name: '/tmp/foo', permission: ['o::rwx'], action: :xset
-      end.to raise_error
+      end.to raise_error(Puppet::ResourceError)
+    end
+
+    it 'accepts a recurselimit' do
+      expect do
+        acl_type.new name: '/tmp/foo', permission: ['o::rwx'], recurselimit: 42
+      end.to raise_error(Puppet::Error)
     end
 
     it 'fails with a wrong recurselimit' do
       expect do
         acl_type.new name: '/tmp/foo', permission: ['o::rwx'], recurselimit: :a
-      end.to raise_error
+      end.to raise_error(Puppet::Error)
     end
 
     it 'fails with a wrong first argument' do
       expect do
         acl_type.new name: '/tmp/foo', permission: ['wrong::rwx']
-      end.to raise_error
+      end.to raise_error(Puppet::ResourceError)
     end
 
     it 'fails with a wrong last argument' do
       expect do
         acl_type.new name: '/tmp/foo', permission: ['user::-_-']
-      end.to raise_error
+      end.to raise_error(Puppet::ResourceError)
     end
 
     it 'fails with a wrong ignore_missing' do
       expect do
         acl_type.new name: '/tmp/foo', permission: ['o::rwx'], ignore_missing: :true
-      end.to raise_error
+      end.to raise_error(Puppet::ResourceError)
     end
   end
 
@@ -203,4 +232,3 @@
     end
   end
 end
-# rubocop:enable RSpec/UnspecifiedException