4253: Use new username to set up repository and VM logins.

The usernames added in 4253 have stricter limits than past usernames
generated to set up a repository and VM login.  Use the new generated
username to avoid a weird disconnect between that and these related
objects.  Do a little cleanup in the tests, including removing some
test parameters that now seem redundant under the new rules.

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index d4d722f86fe6ef2f96abdb1cd9614f953fa9b4d5..c395d23bb1c799c4c748927f27b3a7af33d4b963 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -22,7 +22,11 @@ class User < ArvadosModel
     user.username.nil? and user.email
   }
   after_create :add_system_group_permission_link
-  after_create :auto_setup_new_user
+  after_create :auto_setup_new_user, :if => Proc.new { |user|
+    Rails.configuration.auto_setup_new_users and
+    (user.uuid != system_user_uuid) and
+    (user.uuid != anonymous_user_uuid)
+  }
   after_create :send_admin_notifications
   after_update :send_profile_created_notification
 
@@ -459,44 +463,16 @@ class User < ArvadosModel
 
   # Automatically setup new user during creation
   def auto_setup_new_user
-    return true if !Rails.configuration.auto_setup_new_users
-    return true if !self.email
-    return true if self.uuid == system_user_uuid
-    return true if self.uuid == anonymous_user_uuid
-
-    if Rails.configuration.auto_setup_new_users_with_vm_uuid ||
-       Rails.configuration.auto_setup_new_users_with_repository
-      username = self.email.partition('@')[0] if self.email
-      return true if !username
-
-      blacklisted_usernames = Rails.configuration.auto_setup_name_blacklist
-      if blacklisted_usernames.include?(username)
-        return true
-      elsif !(/^[a-zA-Z][-._a-zA-Z0-9]{0,30}[a-zA-Z0-9]$/.match(username))
-        return true
-      else
-        return true if !(username = derive_unique_username username)
-      end
-    end
-
-    # setup user
-    setup_repo_vm_links(username,
-                        Rails.configuration.auto_setup_new_users_with_vm_uuid,
-                        Rails.configuration.default_openid_prefix)
-  end
-
-  # Find a username that starts with the given string and does not collide
-  # with any existing repository name or VM login name
-  def derive_unique_username username
-    while true
-      if Repository.where(name: username).empty?
-        login_collisions = Link.where(link_class: 'permission',
-                                      name: 'can_login').select do |perm|
-          perm.properties['username'] == username
-        end
-        return username if login_collisions.empty?
+    setup_repo_vm_links(nil, nil, Rails.configuration.default_openid_prefix)
+    if username
+      create_vm_login_permission_link(Rails.configuration.auto_setup_new_users_with_vm_uuid,
+                                      username)
+      if Rails.configuration.auto_setup_new_users_with_repository and
+          Repository.where(name: username).first.nil?
+        repo = Repository.create!(name: username)
+        Link.create!(tail_uuid: uuid, head_uuid: repo.uuid,
+                     link_class: "permission", name: "can_manage")
       end
-      username = username + SecureRandom.random_number(10).to_s
     end
   end
 

diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 9ba5646013fc8a17ae25955e607f19653c14b7b8..6eabdfd359a4ef5601023fe9fb217e239bf50bd3 100644 (file)
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -294,106 +294,60 @@ class UserTest < ActiveSupport::TestCase
   test "create new user with notifications" do
     set_user_from_auth :admin
 
-    create_user_and_verify_setup_and_notifications true, 'active-notify-address@example.com', 'inactive-notify-address@example.com', nil, false
-    create_user_and_verify_setup_and_notifications true, 'active-notify-address@example.com', [], nil, false
-    create_user_and_verify_setup_and_notifications true, [], [], nil, false
-    create_user_and_verify_setup_and_notifications false, 'active-notify-address@example.com', 'inactive-notify-address@example.com', nil, false
-    create_user_and_verify_setup_and_notifications false, [], 'inactive-notify-address@example.com', nil, false
-    create_user_and_verify_setup_and_notifications false, [], [], nil, false
+    create_user_and_verify_setup_and_notifications true, 'active-notify-address@example.com', 'inactive-notify-address@example.com', nil, nil
+    create_user_and_verify_setup_and_notifications true, 'active-notify-address@example.com', [], nil, nil
+    create_user_and_verify_setup_and_notifications true, [], [], nil, nil
+    create_user_and_verify_setup_and_notifications false, 'active-notify-address@example.com', 'inactive-notify-address@example.com', nil, nil
+    create_user_and_verify_setup_and_notifications false, [], 'inactive-notify-address@example.com', nil, nil
+    create_user_and_verify_setup_and_notifications false, [], [], nil, nil
   end
 
   [
-    [false, [], [], 'inactive-none@example.com', false, false, true],
-    [false, [], [], 'inactive-vm@example.com', true, false, true],
-    [false, [], [], 'inactive-repo@example.com', false, true, true],
-    [false, [], [], 'inactive-both@example.com', true, true, true],
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'active-none@example.com', false, false, true],
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'active-vm@example.com', true, false, true],
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'active-repo@example.com', false, true, true],
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'active-both@example.com', true, true, true],
-
-    [false, [], [], nil, true, true, false],
-
-    [false, [], [], 'arvados', true, true, false],
-    [false, [], [], 'arvados', true, false, false],   # blacklisted username
-    [false, [], [], 'arvados', false, false, true],   # since we are not creating repo and vm login, this blacklisted name is not a problem
-
-    [false, [], [], 'arvados@example.com', false, false, true],   # since we are not creating repo and vm login, this blacklisted name is not a problem
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'arvados@example.com', false, false, true],   # since we are not creating repo and vm login, this blacklisted name is not a problem
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'root@example.com', true, false, false], # blacklisted name
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'root@example.com', true, false, false], # blacklisted name
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'roo_t@example.com', false, true, true], # not blacklisted name
-
-    [false, [], [], '@example.com', true, false, false],  # incorrect format
-    [false, [], [], '@example.com', false, true, false],
-    [false, [], [], '@example.com', false, false, true],  # no repo and vm login, so no issue with email format
-
-    [false, [], [], '^^incorrect_format@example.com', true, true, false],
-
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_repo@example.com', true, true, true],  # existing repository name 'auto_setup_repo'
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_repo@example.com', true, false, true],  # existing repository name 'auto_setup_repo'
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_repo@example.com', false, true, true],  # existing repository name 'auto_setup_repo'
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_repo@example.com', false, false, true],  # existing repository name 'auto_setup_repo', but we are not creating repo or login link
-
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_vm_login@example.com', true, true, true], # existing vm login name
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_vm_login@example.com', true, false, true], # existing vm login name
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_vm_login@example.com', false, true, true], # existing vm login name
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', 'auto_setup_vm_login@example.com', false, false, true], # existing vm login name, but we are not creating repo or login link
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '*!*@example.com', true, false, false], # username is invalid format
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', '*!*@example.com', false, false, true], # since no repo and vm login, username is ok (not validated)
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '*!*@example.com', false, false, true], # since no repo and vm login, username is ok (not validated)
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '&4ad@example.com', true, true, false], # username is invalid format
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '&4ad@example.com', false, false, true], # no repo or vm login, so format not checked
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', '&4ad@example.com', true, true, false], # username is invalid format
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', '&4ad@example.com', false, false, true], # no repo or vm login, so format not checked
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '4ad@example.com', true, true, false], # username is invalid format
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '4ad@example.com', false, false, true], # no repo or vm login, so format not checked
-    [false, 'active-notify@example.com', 'inactive-notify@example.com', '4ad@example.com', false, false, true], # no repo or vm login, so format not checked
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '.foo@example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', '.foo@example.com', true, false, false], # invalid format
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'bar.@example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'bar.@example.com', true, false, false], # valid format
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'ice9@example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'ice9@example.com', true, false, true], # valid format
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'o_o@example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'o_o@example.com', true, false, true], # valid format
-
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'r00t@example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify@example.com', 'inactive-notify@example.com', 'r00t@example.com', true, false, true], # valid format
-
-  ].each do |active, new_user_recipients, inactive_recipients, email, auto_setup_vm, auto_setup_repo, ok_to_auto_setup|
+    # Easy inactive user tests.
+    [false, [], [], "inactive-none@example.com", false, false, "inactivenone"],
+    [false, [], [], "inactive-vm@example.com", true, false, "inactivevm"],
+    [false, [], [], "inactive-repo@example.com", false, true, "inactiverepo"],
+    [false, [], [], "inactive-both@example.com", true, true, "inactiveboth"],
+
+    # Easy active user tests.
+    [true, "active-notify@example.com", "inactive-notify@example.com", "active-none@example.com", false, false, "activenone"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "active-vm@example.com", true, false, "activevm"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "active-repo@example.com", false, true, "activerepo"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "active-both@example.com", true, true, "activeboth"],
+
+    # Test users with malformed e-mail addresses.
+    [false, [], [], nil, true, true, nil],
+    [false, [], [], "arvados", true, true, nil],
+    [false, [], [], "@example.com", true, true, nil],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "*!*@example.com", true, false, nil],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "*!*@example.com", false, false, nil],
+
+    # Test users with various username transformations.
+    [false, [], [], "arvados@example.com", false, false, "arvados2"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "arvados@example.com", false, false, "arvados2"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "root@example.com", true, false, "root2"],
+    [false, "active-notify@example.com", "inactive-notify@example.com", "root@example.com", true, false, "root2"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "roo_t@example.com", false, true, "root2"],
+    [false, [], [], "^^incorrect_format@example.com", true, true, "incorrectformat"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "&4a_d9.@example.com", true, true, "ad9"],
+    [true, "active-notify@example.com", "inactive-notify@example.com", "&4a_d9.@example.com", false, false, "ad9"],
+    [false, "active-notify@example.com", "inactive-notify@example.com", "&4a_d9.@example.com", true, true, "ad9"],
+    [false, "active-notify@example.com", "inactive-notify@example.com", "&4a_d9.@example.com", false, false, "ad9"],
+  ].each do |active, new_user_recipients, inactive_recipients, email, auto_setup_vm, auto_setup_repo, expect_username|
     test "create new user with auto setup #{active} #{email} #{auto_setup_vm} #{auto_setup_repo}" do
-      auto_setup_new_users = Rails.configuration.auto_setup_new_users
-      auto_setup_new_users_with_vm_uuid = Rails.configuration.auto_setup_new_users_with_vm_uuid
-      auto_setup_new_users_with_repository = Rails.configuration.auto_setup_new_users_with_repository
+      set_user_from_auth :admin
 
-      begin
-        set_user_from_auth :admin
+      Rails.configuration.auto_setup_new_users = true
 
-        Rails.configuration.auto_setup_new_users = true
-
-        if auto_setup_vm
-          Rails.configuration.auto_setup_new_users_with_vm_uuid = virtual_machines(:testvm)['uuid']
-        else
-          Rails.configuration.auto_setup_new_users_with_vm_uuid = false
-        end
+      if auto_setup_vm
+        Rails.configuration.auto_setup_new_users_with_vm_uuid = virtual_machines(:testvm)['uuid']
+      else
+        Rails.configuration.auto_setup_new_users_with_vm_uuid = false
+      end
 
-        Rails.configuration.auto_setup_new_users_with_repository = auto_setup_repo
+      Rails.configuration.auto_setup_new_users_with_repository = auto_setup_repo
 
-        create_user_and_verify_setup_and_notifications active, new_user_recipients, inactive_recipients, email, ok_to_auto_setup
-      ensure
-        Rails.configuration.auto_setup_new_users = auto_setup_new_users
-        Rails.configuration.auto_setup_new_users_with_vm_uuid = auto_setup_new_users_with_vm_uuid
-        Rails.configuration.auto_setup_new_users_with_repository = auto_setup_new_users_with_repository
-      end
+      create_user_and_verify_setup_and_notifications active, new_user_recipients, inactive_recipients, email, expect_username
     end
   end
 
@@ -620,78 +574,41 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
-  def create_user_and_verify_setup_and_notifications (active, new_user_recipients, inactive_recipients, email, ok_to_auto_setup)
+  def create_user_and_verify_setup_and_notifications (active, new_user_recipients, inactive_recipients, email, expect_username)
     Rails.configuration.new_user_notification_recipients = new_user_recipients
     Rails.configuration.new_inactive_user_notification_recipients = inactive_recipients
 
-    assert_equal new_user_recipients, Rails.configuration.new_user_notification_recipients
-    assert_equal inactive_recipients, Rails.configuration.new_inactive_user_notification_recipients
-
     ActionMailer::Base.deliveries = []
 
+    can_setup = (Rails.configuration.auto_setup_new_users and
+                 (not expect_username.nil?))
+    prior_repo = Repository.where(name: expect_username).first
+
     user = User.new
     user.first_name = "first_name_for_newly_created_user"
     user.email = email
     user.is_active = active
     user.save!
+    assert_equal(expect_username, user.username)
 
     # check user setup
-    group = Group.where(name: 'All users').select do |g|
-      g[:uuid].match /-f+$/
-    end.first
-
-    if !Rails.configuration.auto_setup_new_users || !ok_to_auto_setup
-      # verify that the user is not added to "All groups" by auto_setup
-      verify_link_exists false, group[:uuid], user.uuid, 'permission', 'can_read', nil, nil
-
-      # check oid login link not created by auto_setup
-      verify_link_exists false, user.uuid, user.email, 'permission', 'can_login', nil, nil
-    else
-      # verify that auto_setup took place
-      # verify that the user is added to "All groups"
-      verify_link_exists true, group[:uuid], user.uuid, 'permission', 'can_read', nil, nil
-
-      # check oid login link
-      verify_link_exists true, user.uuid, user.email, 'permission', 'can_login', nil, nil
-
-      username = user.email.partition('@')[0] if email
-
-      # check repo
-      repo_names = []
-      if Rails.configuration.auto_setup_new_users_with_repository
-        repos = Repository.where('name like ?', "%#{username}%")
-        assert_not_nil repos, 'repository not found'
-        assert_equal true, repos.any?, 'repository not found'
-        repo_uuids = []
-        repos.each do |repo|
-          repo_uuids << repo[:uuid]
-          repo_names << repo[:name]
-        end
-        if username == 'auto_setup_repo'
-          begin
-            repo_names.delete('auto_setup_repo')
-          ensure
-            assert_equal true, repo_names.any?, 'Repository name for username foo is not unique'
-          end
-        end
-        verify_link_exists true, repo_uuids, user.uuid, 'permission', 'can_manage', nil, nil
-      end
-
-      # if username is existing vm login name, make sure the username used to generate any repo is unique
-      if username == 'auto_setup_vm_login' || username == 'auto_setup_repo'
-        if repo_names.any?
-          assert repo_names.first.start_with? username
-          assert_not_nil /\d$/.match(repo_names.first)
-        end
-      end
-
-      # check vm uuid
-      vm_uuid = Rails.configuration.auto_setup_new_users_with_vm_uuid
-      if vm_uuid
-        verify_link_exists true, vm_uuid, user.uuid, 'permission', 'can_login', 'username', (username == 'auto_setup_repo' ? repo_names.first : username)
-      else
-        verify_link_exists false, vm_uuid, user.uuid, 'permission', 'can_login', 'username', (username == 'auto_setup_repo' ? repo_names.first : username)
-      end
+    verify_link_exists(Rails.configuration.auto_setup_new_users,
+                       groups(:all_users).uuid, user.uuid,
+                       "permission", "can_read")
+    # Check for OID login link.
+    verify_link_exists(Rails.configuration.auto_setup_new_users,
+                       user.uuid, user.email, "permission", "can_login")
+    # Check for repository.
+    if named_repo = (prior_repo or
+                     Repository.where(name: expect_username).first)
+      verify_link_exists((can_setup and prior_repo.nil? and
+                          Rails.configuration.auto_setup_new_users_with_repository),
+                         named_repo.uuid, user.uuid, "permission", "can_manage")
+    end
+    # Check for VM login.
+    if auto_vm_uuid = Rails.configuration.auto_setup_new_users_with_vm_uuid
+      verify_link_exists(can_setup, auto_vm_uuid, user.uuid,
+                         "permission", "can_login", "username", expect_username)
     end
 
     # check email notifications
@@ -700,7 +617,7 @@ class UserTest < ActiveSupport::TestCase
 
     new_user_email_subject = "#{Rails.configuration.email_subject_prefix}New user created notification"
     if Rails.configuration.auto_setup_new_users
-      new_user_email_subject = (ok_to_auto_setup || active) ?
+      new_user_email_subject = (expect_username or active) ?
                                  "#{Rails.configuration.email_subject_prefix}New user created and setup notification" :
                                  "#{Rails.configuration.email_subject_prefix}New user created, but not setup notification"
     end
@@ -740,7 +657,7 @@ class UserTest < ActiveSupport::TestCase
 
   end
 
-  def verify_link_exists link_exists, head_uuid, tail_uuid, link_class, link_name, property_name, property_value
+  def verify_link_exists link_exists, head_uuid, tail_uuid, link_class, link_name, property_name=nil, property_value=nil
     all_links = Link.where(head_uuid: head_uuid,
                            tail_uuid: tail_uuid,
                            link_class: link_class,