10232: Call getpwnam() and getgrnam() for every name instead of relying on Etc.to_enum().

diff --git a/services/login-sync/bin/arvados-login-sync b/services/login-sync/bin/arvados-login-sync
index 57487711d5235ccca9dd5f55c6872a5a629584a2..22cf0c446bbc48c03b2de61c5a91b2739c9e88ad 100755 (executable)
--- a/services/login-sync/bin/arvados-login-sync
+++ b/services/login-sync/bin/arvados-login-sync
@@ -21,17 +21,12 @@ exclusive_banner = "############################################################
 start_banner = "### BEGIN Arvados-managed keys -- changes between markers will be overwritten\n"
 end_banner = "### END Arvados-managed keys -- changes between markers will be overwritten\n"
 
-# some LDAP systems have already the user there
-# use this falg
-dont_create_user = ARGV.index("--dont-create-user")
+# Don't try to create any local accounts
+skip_missing_users = ARGV.index("--skip-missing-users")
 
 keys = ''
 
-seen = Hash.new
-
 begin
-  uids = Hash[Etc.to_enum(:passwd).map { |ent| [ent.name, ent.uid] }]
-  gids = Hash[Etc.to_enum(:group).map { |ent| [ent.name, ent.gid] }]
   arv = Arvados.new({ :suppress_ssl_warnings => false })
 
   vm_uuid = ENV['ARVADOS_VIRTUAL_MACHINE_UUID']
@@ -56,8 +51,24 @@ begin
       uid_min = new_uid_min if (new_uid_min > 0)
     end
   end
-  logins.reject! { |l| (uids[l[:username]] || 65535) < uid_min }
 
+  pwnam = Hash.new()
+  logins.reject! do |l|
+    return false if pwnam[l[:username]]
+    begin
+      pwnam[l[:username]] = Etc.getpwnam(l[:username])
+    rescue
+      if skip_missing_users
+        STDERR.puts "Account #{l[:username]} not found. Skipping"
+        true
+      end
+    else
+      if pwnam[l[:username]].uid < uid_min
+        STDERR.puts "Account #{l[:username]} uid #{pwnam[l[:username]].uid} < uid_min #{uid_min}. Skipping"
+        true
+      end
+    end
+  end
   keys = Hash.new()
 
   # Collect all keys
@@ -78,35 +89,33 @@ begin
 
   logins.each do |l|
     next if seen[l[:username]]
-    seen[l[:username]] = true if not seen.has_key?(l[:username])
+    seen[l[:username]] = true
 
-    unless uids[l[:username]] or dont_create_user
+    unless pwnam[l[:username]]
       STDERR.puts "Creating account #{l[:username]}"
       groups = l[:groups] || []
       # Adding users to the FUSE group has long been hardcoded behavior.
       groups << "fuse"
-      groups.select! { |name| gids[name] }
+      groups.select! { |g| Etc.getgrnam(g) rescue false }
       # Create new user
-      next unless system("useradd", "-m",
-                         "-c", l[:username],
-                         "-s", "/bin/bash",
-                         "-G", groups.join(","),
-                         l[:username],
-                         out: devnull)
+      unless system("useradd", "-m",
+                "-c", l[:username],
+                "-s", "/bin/bash",
+                "-G", groups.join(","),
+                l[:username],
+                out: devnull)
+        STDERR.puts "Account creation failed for #{l[:username]}: $?"
+        next
+      end
+      begin
+        pwnam[l[:username]] = Etc.getpwnam(l[:username])
+      rescue => e
+        STDERR.puts "Created account but then getpwnam() failed for #{l[:username]}: #{e}"
+        raise
+      end
     end
 
-    # If after all this effort isn't listed using Etc.getpwnam()
-    # this means that wont be available in the system
-    # some LDAP configurations will need this
-    begin
-      # Create .ssh directory if necessary
-      Etc.getpwnam(l[:username])
-    rescue ArgumentError
-      STDERR.puts "Account #{l[:username]} not found. Skipping"
-      next
-    end
-      
-    @homedir = Etc.getpwnam(l[:username]).dir
+    @homedir = pwnam[l[:username]].dir
     userdotssh = File.join(@homedir, ".ssh")
     Dir.mkdir(userdotssh) if !File.exists?(userdotssh)
 

diff --git a/services/login-sync/test/test_add_user.rb b/services/login-sync/test/test_add_user.rb
index 7a010c2a64062cb70a6ef916747c47493e3cdcf1..caaadd5fc9c352767b6d21fb78e370e2f2fc38d1 100644 (file)
--- a/services/login-sync/test/test_add_user.rb
+++ b/services/login-sync/test/test_add_user.rb
@@ -27,7 +27,7 @@ class TestAddUser < Minitest::Test
       f.puts 'useradd -m -c adminroot -s /bin/bash -G docker,fuse adminroot'
       f.puts 'useradd -m -c adminroot -s /bin/bash -G docker,admin,fuse adminroot'
     end
-    $stderr.puts "*** Expect crash in dir_s_mkdir:"
+    $stderr.puts "*** Expect crash after getpwnam() fails:"
     invoke_sync binstubs: ['new_user']
     assert !$?.success?
     spied = File.read(@tmpdir+'/spy')