Merge pull request #8511 from GabrielNagy/fix-gid-idempotency-main

(PUP-10896) Fix gid idempotency on user resource

Author: ciprianbadescu

acceptance/tests/resource/user/should_modify_gid.rb

@@ -2,7 +2,7 @@
 confine :except, :platform => 'windows'
 confine :except, :platform => /aix/ # PUP-5358
 
-tag 'audit:medium',
+tag 'audit:high',
     'audit:refactor',  # Use block style `test_run`
     'audit:acceptance' # Could be done as integration tests, but would
                        # require changing the system running the test

acceptance/tests/resource/user/should_modify_gid_forcelocal.rb

@@ -0,0 +1,56 @@
+test_name "verify that we can modify the gid with forcelocal" do
+  confine :to, :platform => /el|fedora/ # PUP-5358
+
+  require 'puppet/acceptance/common_utils'
+  extend Puppet::Acceptance::PackageUtils
+  extend Puppet::Acceptance::ManifestUtils
+
+  tag 'audit:high'
+
+  user = "u#{rand(99999).to_i}"
+  group1 = "#{user}o"
+  group2 = "#{user}n"
+
+  agents.each do |host|
+    teardown do
+      on(host, puppet_resource('user',  user,   'ensure=absent'))
+      on(host, puppet_resource('group', group1, 'ensure=absent'))
+      on(host, puppet_resource('group', group2, 'ensure=absent'))
+    end
+
+    step "ensure that the groups both exist" do
+      on(host, puppet_resource('group', group1, 'ensure=present'))
+      on(host, puppet_resource('group', group2, 'ensure=present'))
+    end
+
+    step "ensure the user exists and has the old group" do
+      apply_manifest_on(agent, resource_manifest('user', user, ensure: 'present', gid: group1, forcelocal: true))
+    end
+
+    step "verify that the user has the correct gid" do
+      group_gid1 = host.group_gid(group1)
+      host.user_get(user) do |result|
+        user_gid1 = result.stdout.split(':')[3]
+
+        fail_test "didn't have the expected old GID #{group_gid1}, but got: #{user_gid1}" unless group_gid1 == user_gid1
+      end
+    end
+
+    step "modify the GID of the user" do
+      apply_manifest_on(agent, resource_manifest('user', user, ensure: 'present', gid: group2, forcelocal: true), expect_changes: true)
+    end
+
+    step "verify that the user has the updated gid" do
+      group_gid2 = host.group_gid(group2)
+      host.user_get(user) do |result|
+        user_gid2 = result.stdout.split(':')[3]
+
+        fail_test "didn't have the expected old GID #{group_gid}, but got: #{user_gid2}" unless group_gid2 == user_gid2
+      end
+    end
+
+    step "run again for idempotency" do
+      apply_manifest_on(agent, resource_manifest('user', user, ensure: 'present', gid: group2, forcelocal: true), catch_changes: true)
+    end
+  end
+end

lib/puppet/provider/user/useradd.rb

@@ -104,7 +104,14 @@ def localuid
 
   def localgid
     user = finduser(:account, resource[:name])
-    return user[:gid] if user
+    if user
+      begin
+        return Integer(user[:gid])
+      rescue ArgumentError
+        Puppet.debug("Non-numeric GID found in /etc/passwd for user #{resource[:name]}")
+        return user[:gid]
+      end
+    end
     false
   end
 

spec/unit/provider/user/useradd_spec.rb

@@ -360,14 +360,14 @@
       resource[:forcelocal] = true
       allow(Puppet::FileSystem).to receive(:exist?).with('/etc/passwd').and_return(true)
       allow(Puppet::FileSystem).to receive(:each_line).with('/etc/passwd').and_yield(content)
-      expect(provider.gid).to eq('999')
+      expect(provider.gid).to eq(999)
     end
 
     it "should fall back to nameservice GID when forcelocal is false" do
       resource[:forcelocal] = false
-      allow(provider).to receive(:get).with(:gid).and_return('1234')
+      allow(provider).to receive(:get).with(:gid).and_return(1234)
       expect(provider).not_to receive(:localgid)
-      expect(provider.gid).to eq('1234')
+      expect(provider.gid).to eq(1234)
     end
   end