diff --git a/libraries/ident.rb b/libraries/ident.rb index 7391b9d03..7f790984c 100644 --- a/libraries/ident.rb +++ b/libraries/ident.rb @@ -79,21 +79,31 @@ def initialize def add(entry) raise unless entry.is_a?(PgIdentFileEntry) - return false if entry?(entry.map_name) + return false if include?(entry) @entries.push(entry) sort! end - def entry(map_name) - entry = @entries.filter { |e| e.map_name.eql?(map_name) } + def entry(map_name, system_username = nil, database_username = nil) + entries = @entries.filter { |e| e.map_name.eql?(map_name) } - return if nil_or_empty?(entry) + return if nil_or_empty?(entries) - raise PgIdentFileDuplicateEntry, "Duplicate entries found for #{map_name}" unless entry.one? + # If specific system_username and database_username are provided, find exact match + if system_username && database_username + entry = entries.filter { |e| e.system_username.eql?(system_username) && e.database_username.eql?(database_username) } + return entry.first if entry.one? + return + end + + # If only map_name is provided and there's only one entry, return it + return entries.first if entries.one? - entry.pop + # If multiple entries exist for the same map_name but no specific system/database username provided + # This is for backward compatibility - return the first one but don't raise an error + entries.first end def entry?(map_name) @@ -103,7 +113,7 @@ def entry?(map_name) def include?(entry) raise unless entry.is_a?(PgIdentFileEntry) - @entries.any? { |e| e.map_name.eql?(entry.map_name) } + @entries.any? { |e| e.eql?(entry) } end def read!(file = 'pg_ident.conf', sort: true) @@ -123,14 +133,13 @@ def read!(file = 'pg_ident.conf', sort: true) def remove(entry) raise unless entry.is_a?(PgIdentFileEntry) || entry.is_a?(String) - remove_name = case entry - when PgIdentFileEntry - entry.map_name - when String - entry - end - - @entries.reject! { |e| e.map_name.eql?(remove_name) } + case entry + when PgIdentFileEntry + @entries.reject! { |e| e.eql?(entry) } + when String + # For backward compatibility, remove all entries with this map_name + @entries.reject! { |e| e.map_name.eql?(entry) } + end end def sort! @@ -202,7 +211,7 @@ def to_s def eql?(entry) return false unless self.class.eql?(entry.class) - return true if self.class.const_get(:ENTRY_FIELDS).all? { |field| send(field).eql?(entry.send(field)) } + return true if %i(map_name system_username database_username).all? { |field| send(field).eql?(entry.send(field)) } false end diff --git a/resources/ident.rb b/resources/ident.rb index 6cf1a4325..567edfc1c 100644 --- a/resources/ident.rb +++ b/resources/ident.rb @@ -54,7 +54,9 @@ current_value_does_not_exist! unless ident_file.entry?(new_resource.map_name) - entry = ident_file.entry(new_resource.map_name) + entry = ident_file.entry(new_resource.map_name, new_resource.system_username, new_resource.database_username) + current_value_does_not_exist! unless entry + %i(map_name system_username database_username comment).each { |p| send(p, entry.send(p)) } end @@ -65,7 +67,7 @@ action :create do converge_if_changed do config_resource_init - entry = config_resource.variables[:pg_ident].entry(new_resource.map_name) + entry = config_resource.variables[:pg_ident].entry(new_resource.map_name, new_resource.system_username, new_resource.database_username) if nil_or_empty?(entry) resource_properties = %i(map_name system_username database_username comment).map { |p| [ p, new_resource.send(p) ] }.to_h.compact @@ -80,9 +82,9 @@ action :update do converge_if_changed(:system_username, :database_username, :comment) do config_resource_init - entry = config_resource.variables[:pg_ident].entry(new_resource.map_name) + entry = config_resource.variables[:pg_ident].entry(new_resource.map_name, new_resource.system_username, new_resource.database_username) - raise Chef::Exceptions::CurrentValueDoesNotExist, "Cannot update ident entry for '#{new_resource.map_name}' as it does not exist" if nil_or_empty?(entry) + raise Chef::Exceptions::CurrentValueDoesNotExist, "Cannot update ident entry for '#{new_resource.map_name}' with system_username '#{new_resource.system_username}' and database_username '#{new_resource.database_username}' as it does not exist" if nil_or_empty?(entry) entry.update(system_username: new_resource.system_username, database_username: new_resource.database_username, comment: new_resource.comment) end @@ -91,7 +93,14 @@ action :delete do config_resource_init - converge_by("Remove ident entry with map_name: #{new_resource.map_name}") do - config_resource.variables[:pg_ident].remove(new_resource.map_name) - end if config_resource.variables[:pg_ident].entry?(new_resource.map_name) + # Create an entry object to match for removal + entry_to_remove = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: new_resource.map_name, + system_username: new_resource.system_username, + database_username: new_resource.database_username + ) + + converge_by("Remove ident entry with map_name: #{new_resource.map_name}, system_username: #{new_resource.system_username}, database_username: #{new_resource.database_username}") do + config_resource.variables[:pg_ident].remove(entry_to_remove) + end if config_resource.variables[:pg_ident].include?(entry_to_remove) end diff --git a/spec/libraries/ident_spec.rb b/spec/libraries/ident_spec.rb new file mode 100644 index 000000000..639f721b4 --- /dev/null +++ b/spec/libraries/ident_spec.rb @@ -0,0 +1,186 @@ +require 'spec_helper' +require_relative '../../libraries/ident' + +RSpec.describe PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFile do + let(:ident_file) { described_class.new } + + describe '#add' do + let(:entry1) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + let(:entry2) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user2', + database_username: 'dbuser2' + ) + end + + let(:duplicate_entry) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + it 'allows multiple entries with the same map_name but different system/database usernames' do + result1 = ident_file.add(entry1) + result2 = ident_file.add(entry2) + + expect(result1).to be_truthy + expect(result2).to be_truthy + expect(ident_file.entries.size).to eq(2) + end + + it 'prevents adding duplicate entries' do + ident_file.add(entry1) + result = ident_file.add(duplicate_entry) + + expect(result).to be_falsy + expect(ident_file.entries.size).to eq(1) + end + end + + describe '#entry' do + let(:entry1) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + let(:entry2) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user2', + database_username: 'dbuser2' + ) + end + + before do + ident_file.add(entry1) + ident_file.add(entry2) + end + + it 'returns specific entry when all parameters are provided' do + result = ident_file.entry('testmap', 'user1', 'dbuser1') + expect(result).to eq(entry1) + + result = ident_file.entry('testmap', 'user2', 'dbuser2') + expect(result).to eq(entry2) + end + + it 'returns first entry when only map_name is provided and multiple entries exist' do + result = ident_file.entry('testmap') + expect(result).to be_a(PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry) + expect(result.map_name).to eq('testmap') + end + + it 'returns nil when no matching entry is found' do + result = ident_file.entry('testmap', 'nonexistent', 'user') + expect(result).to be_nil + end + end + + describe '#include?' do + let(:entry) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + let(:different_entry) do + PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'testmap', + system_username: 'user2', + database_username: 'dbuser2' + ) + end + + it 'returns true for existing entries' do + ident_file.add(entry) + expect(ident_file.include?(entry)).to be_truthy + end + + it 'returns false for non-existing entries' do + ident_file.add(entry) + expect(ident_file.include?(different_entry)).to be_falsy + end + end + + describe 'issue scenario' do + # Test the specific scenario from GitHub issue #787 + it 'allows multiple mappings per map name as described in the issue' do + entry1 = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'someuser_postgres', + system_username: 'someuser', + database_username: 'postgres' + ) + + entry2 = PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry.new( + map_name: 'someuser_postgres', + system_username: 'postgres', + database_username: 'postgres' + ) + + result1 = ident_file.add(entry1) + result2 = ident_file.add(entry2) + + expect(result1).to be_truthy + expect(result2).to be_truthy + expect(ident_file.entries.size).to eq(2) + + # Verify both entries can be retrieved + retrieved1 = ident_file.entry('someuser_postgres', 'someuser', 'postgres') + retrieved2 = ident_file.entry('someuser_postgres', 'postgres', 'postgres') + + expect(retrieved1).to eq(entry1) + expect(retrieved2).to eq(entry2) + end + end +end + +RSpec.describe PostgreSQL::Cookbook::IdentHelpers::PgIdent::PgIdentFileEntry do + describe '#eql?' do + let(:entry1) do + described_class.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + let(:entry2) do + described_class.new( + map_name: 'testmap', + system_username: 'user1', + database_username: 'dbuser1' + ) + end + + let(:entry3) do + described_class.new( + map_name: 'testmap', + system_username: 'user2', + database_username: 'dbuser1' + ) + end + + it 'returns true for entries with same map_name, system_username, and database_username' do + expect(entry1.eql?(entry2)).to be_truthy + end + + it 'returns false for entries with different system_username' do + expect(entry1.eql?(entry3)).to be_falsy + end + end +end \ No newline at end of file diff --git a/test/cookbooks/test/recipes/ident.rb b/test/cookbooks/test/recipes/ident.rb index 8626e53c7..6c861a872 100644 --- a/test/cookbooks/test/recipes/ident.rb +++ b/test/cookbooks/test/recipes/ident.rb @@ -36,6 +36,27 @@ notifies :reload, 'postgresql_service[postgresql]', :delayed end +# Test the fix for issue #787 - multiple mappings per map name +postgresql_ident 'someuser to postgres mapping' do + map_name 'someuser_postgres' + system_username 'someuser' + database_username 'postgres' + comment 'Test mapping for issue #787' + + notifies :reload, 'postgresql_service[postgresql]', :delayed +end + +# Make sure that the postgres keeps its own identity +# Without this, the cookbook would previously fail to run +postgresql_ident 'postgres to postgres mapping' do + map_name 'someuser_postgres' + system_username 'postgres' + database_username 'postgres' + comment 'Second mapping with same map_name for issue #787' + + notifies :reload, 'postgresql_service[postgresql]', :delayed +end + postgresql_ident 'shef remove mapping' do map_name 'testmap3' system_username 'shef_remove' diff --git a/test/integration/ident/controls/multiple_mappings.rb b/test/integration/ident/controls/multiple_mappings.rb new file mode 100644 index 000000000..d54658a1b --- /dev/null +++ b/test/integration/ident/controls/multiple_mappings.rb @@ -0,0 +1,31 @@ +control 'postgresql-ident-map' do + impact 1.0 + desc 'This test ensures postgres configures ident access correctly' + + describe command("sudo -u shef bash -c \"psql -U sous_chef -d postgres -c 'SELECT 1;'\"") do + its('exit_status') { should eq 0 } + end +end + +control 'postgresql-ident-multiple-mappings' do + impact 1.0 + desc 'This test ensures multiple mappings per map name work correctly (issue #787)' + + # Use a command to find and check the pg_ident.conf file + describe command("find /etc /var -name 'pg_ident.conf' 2>/dev/null | head -1 | xargs cat") do + its('stdout') { should match(/^someuser_postgres\s+someuser\s+postgres/) } + its('stdout') { should match(/^someuser_postgres\s+postgres\s+postgres/) } + end +end + +control 'shef and postgres roles should exist' do + impact 1.0 + desc 'The shef & postgres database user role should exist' + + postgres_access = postgres_session('postgres', '12345', '127.0.0.1') + + describe postgres_access.query('SELECT rolname FROM pg_roles;') do + its('output') { should include 'postgres' } + its('output') { should include 'sous_chef' } + end +end