Fix pg_ident to allow multiple mappings per map name

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

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

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

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'

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