From 50d0d5e02227976a0cf988e94723c4b868b54018 Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Fri, 29 Sep 2023 15:21:11 +0200 Subject: [PATCH 1/3] Add User autorequire tests --- spec/unit/type/user_spec.rb | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/spec/unit/type/user_spec.rb b/spec/unit/type/user_spec.rb index 7a9e2e6903e..72b34683748 100644 --- a/spec/unit/type/user_spec.rb +++ b/spec/unit/type/user_spec.rb @@ -239,6 +239,51 @@ def self.instances; []; end resource.parameter(:gid).sync end end + + context 'automatic relations' do + let(:testgroup) { Puppet::Type.type(:group).new(:name => 'foo', :gid => 50) } + + before do + Puppet::Resource::Catalog.new :testing do |conf| + conf.add_resource(testuser) + conf.add_resource(testgroup) + end + end + + context 'when ensure => present' do + context 'when gid specified as :absent' do + let(:testuser) { described_class.new(:name => "testuser", :gid => :absent) } + + it "has no autorequire" do + expect(testuser.autorequire).to be_empty + end + end + + context 'when gid specified as number-looking string' do + let(:testuser) { described_class.new(:name => "testuser", :gid => '50') } + + it "autorequires the group" do + expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) + end + end + + context 'when gid specified as integer' do + let(:testuser) { described_class.new(:name => "testuser", :gid => 50) } + + it "autorequires the group" do + expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) + end + end + + context 'when gid specified as name' do + let(:testuser) { described_class.new(:name => "testuser", :gid => 'foo') } + + it "autorequires the group" do + expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) + end + end + end + end end describe "when managing groups" do From d77e622fb78746aa894162638914d4ee63e9611b Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Fri, 29 Sep 2023 15:02:08 +0200 Subject: [PATCH 2/3] Shorten user autorequire code using safe navigators This refactors the code using safe navigators and modern Ruby functions. --- lib/puppet/type/user.rb | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb index 3dae335cf77..78436de6571 100644 --- a/lib/puppet/type/user.rb +++ b/lib/puppet/type/user.rb @@ -459,41 +459,24 @@ def insync?(current) end end - # Autorequire the group, if it's around + # Autorequire groups, if they're around autorequire(:group) do autos = [] - # autorequire primary group, if managed - obj = @parameters[:gid] - groups = obj.shouldorig if obj - if groups - groups = groups.collect { |group| - if group.is_a?(String) && group =~/^\d+$/ - Integer(group) + if @parameters[:gid]&.shouldorig + groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) } + autos += @parameters[:gid].shouldorig.filter_map do |group| + if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer) + gid = Integer(group) + groups_in_catalog.find { |r| r.should(:gid) == gid } else group end - } - groups.each { |group| - case group - when Integer - resource = catalog.resources.find { |r| r.is_a?(Puppet::Type.type(:group)) && r.should(:gid) == group } - if resource - autos << resource - end - else - autos << group - end - } + end end - # autorequire groups, excluding primary group, if managed - obj = @parameters[:groups] - if obj - groups = obj.should - if groups - autos += groups.split(",") - end + if @parameters[:groups]&.should + autos += @parameters[:groups].should.split(",") end autos From 65f9ee601ec0e0a6c4475625a1abf5e7a6f59243 Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Fri, 29 Sep 2023 14:46:07 +0200 Subject: [PATCH 3/3] Correct user <-> group relationship on removal When a group needs to be removed, it can't be the primary group of a user. So if a user is ensured absent, it needs to happen before the group is removed. --- lib/puppet/type/user.rb | 54 +++++++++++++++++++++++------- spec/unit/type/user_spec.rb | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 11 deletions(-) diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb index 78436de6571..cb480af61c2 100644 --- a/lib/puppet/type/user.rb +++ b/lib/puppet/type/user.rb @@ -19,7 +19,15 @@ module Puppet provided in the `gid` attribute) or any group listed in the `groups` attribute then the user resource will autorequire that group. If Puppet is managing any role accounts corresponding to the user's roles, the - user resource will autorequire those role accounts." + user resource will autorequire those role accounts. + Only when the resource is ensured to be present. + + **Autobefores:** If Puppet is managing the user's primary group (as + provided in the `gid` attribute) or any group listed in the `groups` + attribute then the user resource will autobefore that group. If Puppet + is managing any role accounts corresponding to the user's roles, the + user resource will autorequire those role accounts. + Only when the resource is ensured to be absent." feature :allows_duplicates, "The provider supports duplicate users with the same UID." @@ -463,20 +471,44 @@ def insync?(current) autorequire(:group) do autos = [] - if @parameters[:gid]&.shouldorig - groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) } - autos += @parameters[:gid].shouldorig.filter_map do |group| - if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer) - gid = Integer(group) - groups_in_catalog.find { |r| r.should(:gid) == gid } - else - group + if self[:ensure] != :absent + if @parameters[:gid]&.shouldorig + groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) } + autos += @parameters[:gid].shouldorig.filter_map do |group| + if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer) + gid = Integer(group) + groups_in_catalog.find { |r| r.should(:gid) == gid } + else + group + end end end + + if @parameters[:groups]&.should + autos += @parameters[:groups].should.split(",") + end end - if @parameters[:groups]&.should - autos += @parameters[:groups].should.split(",") + autos + end + + # Autobefore the primary group, if it's around. Otherwise group removal + # fails when that group is also ensured absent. + autobefore(:group) do + autos = [] + + if self[:ensure] == :absent + if @parameters[:gid]&.shouldorig + groups_in_catalog = catalog.resources.filter { |r| r.is_a?(Puppet::Type.type(:group)) } + autos += @parameters[:gid].shouldorig.filter_map do |group| + if (group.is_a?(String) && group.match?(/^\d+$/)) || group.is_a?(Integer) + gid = Integer(group) + groups_in_catalog.find { |r| r.should(:gid) == gid } + else + group + end + end + end end autos diff --git a/spec/unit/type/user_spec.rb b/spec/unit/type/user_spec.rb index 72b34683748..36647f29e03 100644 --- a/spec/unit/type/user_spec.rb +++ b/spec/unit/type/user_spec.rb @@ -257,6 +257,10 @@ def self.instances; []; end it "has no autorequire" do expect(testuser.autorequire).to be_empty end + + it "has no autobefore" do + expect(testuser.autobefore).to be_empty + end end context 'when gid specified as number-looking string' do @@ -265,6 +269,10 @@ def self.instances; []; end it "autorequires the group" do expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) end + + it "has no autobefore" do + expect(testuser.autobefore).to be_empty + end end context 'when gid specified as integer' do @@ -273,6 +281,10 @@ def self.instances; []; end it "autorequires the group" do expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) end + + it "has no autobefore" do + expect(testuser.autobefore).to be_empty + end end context 'when gid specified as name' do @@ -281,6 +293,60 @@ def self.instances; []; end it "autorequires the group" do expect(testuser.autorequire).to match_array([an_object_having_attributes(source: testgroup, target: testuser)]) end + + it "has no autobefore" do + expect(testuser.autobefore).to be_empty + end + end + end + + context 'when ensure => absent' do + context 'when gid specified as :absent' do + let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => :absent) } + + it "has no autorequire" do + expect(testuser.autorequire).to be_empty + end + + it "has no autobefore" do + expect(testuser.autobefore).to be_empty + end + end + + context 'when gid specified as number-looking string' do + let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => '50') } + + it "has no autorequire" do + expect(testuser.autorequire).to be_empty + end + + it "autobefores the group" do + expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)]) + end + end + + context 'when gid specified as integer' do + let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => 50) } + + it "has no autorequire" do + expect(testuser.autorequire).to be_empty + end + + it "autobefores the group" do + expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)]) + end + end + + context 'when gid specified as name' do + let(:testuser) { described_class.new(:name => "testuser", :ensure => :absent, :gid => 'foo') } + + it "has no autorequire" do + expect(testuser.autorequire).to be_empty + end + + it "autobefores the group" do + expect(testuser.autobefore).to match_array([an_object_having_attributes(source: testuser, target: testgroup)]) + end end end end