fix crash when droping a role and revoking it's memberships afterwards

lib/pg_ldap_sync/application.rb

@@ -230,18 +230,10 @@ class Application
     pg_exec_modify "DROP ROLE \"#{role.name}\""
   end
 
-  def sync_roles_to_pg(roles)
+  def sync_roles_to_pg(roles, for_state)
-    roles.sort{|a,b|
+    roles.sort{|a,b| a.name<=>b.name }.each do |role|
-      t = b.state.to_s<=>a.state.to_s
+      create_pg_role(role) if role.state==:create && for_state==:create
-      t = a.name<=>b.name if t==0
+      drop_pg_role(role) if role.state==:drop && for_state==:drop
-      t
-    }.each do |role|
-      case role.state
-      when :create
-        create_pg_role(role)
-      when :drop
-        drop_pg_role(role)
-      end
     end
   end
 
@@ -298,20 +290,17 @@ class Application
     pg_exec_modify "REVOKE \"#{role_name}\" FROM #{rm_members_escaped}"
   end
 
-  def sync_membership_to_pg(memberships)
+  def sync_membership_to_pg(memberships, for_state)
     grants = {}
-    memberships.select{|ms| ms.state==:grant }.each do |ms|
+    memberships.select{|ms| ms.state==for_state }.each do |ms|
       grants[ms.role_name] ||= []
       grants[ms.role_name] << ms.has_member
     end
-    revokes = {}
-    memberships.select{|ms| ms.state==:revoke }.each do |ms|
-      revokes[ms.role_name] ||= []
-      revokes[ms.role_name] << ms.has_member
-    end
 
-    grants.each{|role_name, members| grant_membership(role_name, members) }
+    grants.each do |role_name, members|
-    revokes.each{|role_name, members| revoke_membership(role_name, members) }
+      grant_membership(role_name, members) if for_state==:grant
+      revoke_membership(role_name, members) if for_state==:revoke
+    end
   end
 
   def start!
@@ -334,10 +323,12 @@ class Application
     # compare LDAP to PG memberships
     mmemberships = match_memberships(ldap_users+ldap_groups, pg_users+pg_groups)
 
-    # apply changes on roles
+    # drop/revoke roles/memberships first
-    sync_roles_to_pg(mroles)
+    sync_membership_to_pg(mmemberships, :revoke)
-    # apply changes on memberships
+    sync_roles_to_pg(mroles, :drop)
-    sync_membership_to_pg(mmemberships)
+    # create/grant roles/memberships
+    sync_roles_to_pg(mroles, :create)
+    sync_membership_to_pg(mmemberships, :grant)
 
     @pgconn.close
   end

test/test_pg_ldap_sync.rb

@@ -62,7 +62,7 @@ class TestPgLdapSync < Test::Unit::TestCase
     stop_ldap_server
     stop_pg_server
   end
-  
+
   def psqlre(*args)
     /^\s*#{args[0]}[ |]*#{args[1]}[ |\{"]*#{args[2..-1].join('[", ]+')}["\}\s]*$/
   end
@@ -73,23 +73,40 @@ class TestPgLdapSync < Test::Unit::TestCase
     ENV['LC_MESSAGES'] = 'C'
     psql_du = `psql -c \\\\du postgres`
     puts psql_du
-    
+
     assert_match(psqlre('All Users','Cannot login'), psql_du)
     assert_match(psqlre('Flintstones','Cannot login'), psql_du)
     assert_match(psqlre('Wilmas','Cannot login','All Users'), psql_du)
     assert_match(psqlre('fred','','All Users','Flintstones'), psql_du)
     assert_match(psqlre('wilma','','Flintstones','Wilmas'), psql_du)
-    
+
+    # revoke membership of 'wilma' to 'Flintstones'
     @directory['cn=Flintstones,dc=example,dc=com']['member'].pop
-    
+
     PgLdapSync::Application.run(%w[-c test/fixtures/config-ldapdb.yaml -vv])
     psql_du = `psql -c \\\\du postgres`
     puts psql_du
-    
+
     assert_match(psqlre('All Users','Cannot login'), psql_du)
     assert_match(psqlre('Flintstones','Cannot login'), psql_du)
     assert_match(psqlre('Wilmas','Cannot login','All Users'), psql_du)
     assert_match(psqlre('fred','','All Users','Flintstones'), psql_du)
     assert_match(psqlre('wilma','','Wilmas'), psql_du)
+
+    # rename role 'wilma'
+    @directory['cn=Wilma Flintstone,dc=example,dc=com']['sAMAccountName'] = ['Wilma Flintstone']
+    # re-add 'Wilma' to 'Flintstones'
+    @directory['cn=Flintstones,dc=example,dc=com']['member'] << 'cn=Wilma Flintstone,dc=example,dc=com'
+
+    PgLdapSync::Application.run(%w[-c test/fixtures/config-ldapdb.yaml -vv])
+    psql_du = `psql -c \\\\du postgres`
+    puts psql_du
+
+    assert_match(psqlre('All Users','Cannot login'), psql_du)
+    assert_match(psqlre('Flintstones','Cannot login'), psql_du)
+    assert_match(psqlre('Wilmas','Cannot login','All Users'), psql_du)
+    assert_match(psqlre('fred','','All Users','Flintstones'), psql_du)
+    assert_no_match(/wilma/, psql_du)
+    assert_match(psqlre('Wilma Flintstone','','Flintstones','Wilmas'), psql_du)
   end
 end