Whole bunch of changes

Run sync within a SQL transaction, so that no partial sync happens

Don't abort on SQL errors, but print ERROR notice

Exit with code 1 when any ERRORs were logged

Silence the test suite, so that test runs are more clearly

Use PG queries instead of psql in test. This is more flexible
than parsing text outputs per Regexp.

exe/pg_ldap_sync

@@ -2,4 +2,8 @@
 
 require 'pg_ldap_sync'
 
-PgLdapSync::Application.run(ARGV)
+begin
+  PgLdapSync::Application.run(ARGV)
+rescue PgLdapSync::ApplicationExit => ex
+  exit ex.exitcode
+end

lib/pg_ldap_sync.rb

@@ -1,2 +1,21 @@
 require "pg_ldap_sync/application"
 require "pg_ldap_sync/version"
+
+module PgLdapSync
+  class LdapError < RuntimeError
+  end
+
+  class ApplicationExit < RuntimeError
+    attr_reader :exitcode
+
+    def initialize(exitcode)
+      @exitcode = exitcode
+    end
+  end
+
+  class InvalidConfig < ApplicationExit
+  end
+
+  class ErrorExit < ApplicationExit
+  end
+end

lib/pg_ldap_sync/application.rb

@@ -1,16 +1,14 @@
 #!/usr/bin/env ruby
 
-require 'rubygems'
 require 'net/ldap'
 require 'optparse'
 require 'yaml'
-require 'logger'
 require 'kwalify'
 require 'pg'
+require "pg_ldap_sync/logger"
 
 module PgLdapSync
 class Application
-  class LdapError < RuntimeError; end
   attr_accessor :config_fname
   attr_accessor :log
   attr_accessor :test
@@ -36,7 +34,7 @@ class Application
       errors.each do |err|
         log.fatal "error in #{fname}: [#{err.path}] #{err.message}"
       end
-      exit(-1)
+      raise InvalidConfig, 78 # EX_CONFIG
     end
   end
 
@@ -194,10 +192,21 @@ class Application
     return roles
   end
 
+  def try_sql(text)
+    begin
+      @pgconn.exec "SAVEPOINT try_sql;"
+      @pgconn.exec text
+    rescue PG::Error => err
+      @pgconn.exec "ROLLBACK TO try_sql;"
+
+      log.error{ "#{err} (#{err.class})" }
+    end
+  end
+
   def pg_exec_modify(sql)
     log.info{ "SQL: #{sql}" }
     unless self.test
-      res = @pgconn.exec sql
+      try_sql sql
     end
   end
 
@@ -298,36 +307,45 @@ class Application
 
     # gather PGs users and groups
     @pgconn = PG.connect @config[:pg_connection]
-    pg_users = uniq_names search_pg_users
-    pg_groups = uniq_names search_pg_groups
+    begin
+      @pgconn.transaction do
+        pg_users = uniq_names search_pg_users
+        pg_groups = uniq_names search_pg_groups
 
-    # compare LDAP to PG users and groups
-    mroles = match_roles(ldap_users, pg_users, :user)
-    mroles += match_roles(ldap_groups, pg_groups, :group)
+        # compare LDAP to PG users and groups
+        mroles = match_roles(ldap_users, pg_users, :user)
+        mroles += match_roles(ldap_groups, pg_groups, :group)
 
-    # compare LDAP to PG memberships
-    mmemberships = match_memberships(ldap_users+ldap_groups, pg_users+pg_groups)
+        # compare LDAP to PG memberships
+        mmemberships = match_memberships(ldap_users+ldap_groups, pg_users+pg_groups)
 
-    # drop/revoke roles/memberships first
-    sync_membership_to_pg(mmemberships, :revoke)
-    sync_roles_to_pg(mroles, :drop)
-    # create/grant roles/memberships
-    sync_roles_to_pg(mroles, :create)
-    sync_membership_to_pg(mmemberships, :grant)
+        # drop/revoke roles/memberships first
+        sync_membership_to_pg(mmemberships, :revoke)
+        sync_roles_to_pg(mroles, :drop)
+        # create/grant roles/memberships
+        sync_roles_to_pg(mroles, :create)
+        sync_membership_to_pg(mmemberships, :grant)
+      end
+    ensure
+      @pgconn.close
+    end
 
-    @pgconn.close
+    # Determine exitcode
+    if log.had_errors?
+      raise ErrorExit, 1
+    end
   end
 
   def self.run(argv)
     s = self.new
     s.config_fname = '/etc/pg_ldap_sync.yaml'
-    s.log = Logger.new(STDOUT)
+    s.log = Logger.new($stdout, @error_counters)
     s.log.level = Logger::ERROR
 
     OptionParser.new do |opts|
       opts.version = VERSION
       opts.banner = "Usage: #{$0} [options]"
-      opts.on("-v", "--[no-]verbose", "Increase verbose level"){ s.log.level-=1 }
+      opts.on("-v", "--[no-]verbose", "Increase verbose level"){|v| s.log.level += v ? -1 : 1 }
       opts.on("-c", "--config FILE", "Config file [#{s.config_fname}]", &s.method(:config_fname=))
       opts.on("-t", "--[no-]test", "Don't do any change in the database", &s.method(:test=))
 

lib/pg_ldap_sync/logger.rb

@@ -0,0 +1,24 @@
+require 'logger'
+
+module PgLdapSync
+class Logger < ::Logger
+  def initialize(io, counters)
+    super(io)
+    @counters = {}
+  end
+
+  def add(severity, *args)
+    @counters[severity] ||= 0
+    @counters[severity] += 1
+    super
+  end
+
+  def had_logged?(severity)
+    @counters[severity] && @counters[severity] > 0
+  end
+
+  def had_errors?
+    had_logged?(Logger::FATAL) || had_logged?(Logger::ERROR)
+  end
+end
+end

test/fixtures/config-invalid.yaml

@@ -0,0 +1,2 @@
+---
+ldap_connection:

test/fixtures/config-ldapdb.yaml

@@ -5,7 +5,7 @@ ldap_connection:
 
 ldap_users:
   base: dc=example,dc=com
-  filter: (&(cn=*)(sAMAccountName=*))
+  filter: (sAMAccountName=*)
   name_attribute: sAMAccountName
 
 ldap_groups:
@@ -23,7 +23,7 @@ pg_connection:
 #   password:
 
 pg_users:
-  filter: rolcanlogin AND NOT rolsuper
+  filter: rolcanlogin AND NOT rolsuper AND rolname!='double_user'
   create_options: LOGIN
 
 pg_groups:

test/test_pg_ldap_sync.rb

@@ -8,10 +8,23 @@ require_relative 'ldap_server'
 class TestPgLdapSync < Minitest::Test
   include Minitest::Hooks
 
+  @@logid = 0
+
   def log_and_run( *cmd )
-    puts cmd.join(' ')
-    system( *cmd )
-    raise "Command failed: [%s]" % [cmd.join(' ')] unless $?.success?
+    if $DEBUG
+      puts cmd.join(' ')
+      system( *cmd )
+      raise "Command failed: [%s]" % [cmd.join(' ')] unless $?.success?
+    else
+      fname = "temp/run_#{@@logid+=1}.log"
+      pid = Process.spawn( *cmd, [:out, :err] => [fname, "w"] )
+      Process.wait(pid)
+      unless $?.success?
+        $stderr.puts "Command failed: [%s]\n%s" % [cmd.join(' '), File.read(fname)]
+      end
+      File.unlink fname
+      raise "Command failed: [%s]" % [cmd.join(' ')] unless $?.success?
+    end
   end
 
   def start_ldap_server
@@ -46,9 +59,13 @@ class TestPgLdapSync < Minitest::Test
       log_and_run 'initdb', '-D', 'temp/pg_data', '--no-locale'
     end
     log_and_run 'pg_ctl', '-w', '-o', "-k.", '-D', 'temp/pg_data', 'start'
+
+    @pgconn = PG.connect dbname: 'postgres'
+    @pgconn.exec "SET client_min_messages to warning"
   end
 
   def stop_pg_server
+    @pgconn.close if @pgconn
     log_and_run 'pg_ctl', '-w', '-o', "-k.", '-D', 'temp/pg_data', 'stop'
   end
 
@@ -66,13 +83,29 @@ class TestPgLdapSync < Minitest::Test
   end
 
   def setup
-    log_and_run 'psql', '-e', '-c', "DROP ROLE IF EXISTS fred, wilma, \"Flintstones\", \"Wilmas\", \"All Users\"", 'postgres'
+    @pgconn.exec "DROP ROLE IF EXISTS fred, wilma, \"Flintstones\", \"Wilmas\", \"All Users\", double_user"
   end
 
-  def psqlre(*args)
-    /^\s*#{args[0]}[ |]*#{args[1]}[ |\{"]*#{args[2..-1].join('[", ]+')}["\}\s]*$/
+  def assert_role(role_name, attrs, member_of=[])
+    res = @pgconn.exec("SELECT * FROM pg_roles WHERE rolname = '#{@pgconn.escape_string(role_name)}'")
+    assert_equal 1, res.ntuples, "Role #{role_name} not found"
+
+    res2 = @pgconn.exec "SELECT pr.rolname FROM pg_auth_members pam JOIN pg_roles pr ON pr.oid=pam.roleid WHERE pam.member=#{res.to_a[0]['oid']}"
+    rolnames = res2.map{|t| t['rolname'] }
+    assert_equal member_of.sort, rolnames.sort
+
+    exp_attrs = []
+    exp_attrs << 'Cannot login' if res[0]['rolcanlogin'] != 't'
+    exp_attrs << 'Superuser' if res[0]['rolsuper'] == 't'
+
+    assert_equal attrs, exp_attrs.join(", ")
   end
-  
+
+  def refute_role(role_name)
+    res = @pgconn.exec("SELECT oid FROM pg_roles WHERE rolname = '#{@pgconn.escape_string(role_name)}'")
+    assert_equal 0, res.ntuples, "Role #{role_name} not found"
+  end
+
   def exec_psql_du
     text = if RUBY_PLATFORM=~/mingw|mswin/
       `psql -c \\du postgres`
@@ -89,7 +122,7 @@ class TestPgLdapSync < Minitest::Test
   end
 
   def sync_with_config(config="config-ldapdb")
-    PgLdapSync::Application.run(["-c", "test/fixtures/#{config}.yaml", "-vv"])
+    PgLdapSync::Application.run(["-c", "test/fixtures/#{config}.yaml"] + ($DEBUG ? ["-vv"] : ["--no-verbose"]))
   end
 
   def sync_to_fixture(fixture: "ldapdb", config: "config-ldapdb")
@@ -103,41 +136,59 @@ class TestPgLdapSync < Minitest::Test
     yield(@directory)
 
     sync_with_config
-    exec_psql_du
+    exec_psql_du if $DEBUG
+  end
+
+  def test_invalid_config
+    assert_output(/key 'ldap_users:' is required/) do
+      assert_raises(PgLdapSync::InvalidConfig) do
+        sync_with_config("config-invalid")
+      end
+    end
   end
 
   def test_base_users_groups_memberships
-    psql_du = sync_change{}
+    sync_change{}
 
-    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)
+    assert_role('All Users', 'Cannot login')
+    assert_role('Flintstones', 'Cannot login')
+    assert_role('Wilmas', 'Cannot login', ['All Users'])
+    assert_role('fred', '', ['All Users', 'Flintstones'])
+    assert_role('wilma', '', ['Flintstones', 'Wilmas'])
   end
 
   def test_add_membership
-    psql_du = sync_change do |dir|
+    sync_change do |dir|
       # add 'Fred' to 'Wilmas'
       @directory[0]['cn=Wilmas,dc=example,dc=com']['member'] << 'cn=Fred Flintstone,dc=example,dc=com'
     end
-    assert_match(psqlre('fred','','All Users','Flintstones', 'Wilmas'), psql_du)
+    assert_role('fred', '', ['All Users', 'Flintstones', 'Wilmas'])
   end
 
   def test_revoke_membership
-    psql_du = sync_change do |dir|
+    sync_change do |dir|
       # revoke membership of 'wilma' to 'Flintstones'
       dir[0]['cn=Flintstones,dc=example,dc=com']['member'].pop
     end
-    assert_match(psqlre('wilma','','Wilmas'), psql_du)
+    assert_role('wilma', '', ['Wilmas'])
   end
 
   def test_rename_role
-    psql_du = sync_change do |dir|
+    sync_change do |dir|
       # rename role 'wilma'
       dir[0]['cn=Wilma Flintstone,dc=example,dc=com']['sAMAccountName'] = ['Wilma Flintstone']
     end
-    refute_match(/wilma/, psql_du)
-    assert_match(psqlre('Wilma Flintstone','','Flintstones','Wilmas'), psql_du)
+    refute_role('wilma')
+    assert_role('Wilma Flintstone', '', ['Flintstones', 'Wilmas'])
+  end
+
+  def test_dont_stop_on_error
+    log_and_run 'psql', '-e', '-c', "CREATE ROLE double_user LOGIN", 'postgres'
+
+    assert_raises(PgLdapSync::ErrorExit) do
+      sync_change do |dir|
+        dir[0]['cn=double_user,dc=example,dc=com'] = {'sAMAccountName' => 'double_user'}
+      end
+    end
   end
 end