[ARVADOS] created: fa92aa4a1db149f2938351ae1220f5d1a08a3b8e

git at public.curoverse.com git at public.curoverse.com
Thu Mar 19 12:00:06 EDT 2015


        at  fa92aa4a1db149f2938351ae1220f5d1a08a3b8e (commit)


commit fa92aa4a1db149f2938351ae1220f5d1a08a3b8e
Author: Brett Smith <brett at curoverse.com>
Date:   Wed Mar 18 16:00:01 2015 -0400

    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 d4d722f..c395d23 100644
--- 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 9ba5646..6eabdfd 100644
--- 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 at example.com', 'inactive-notify-address at example.com', nil, false
-    create_user_and_verify_setup_and_notifications true, 'active-notify-address at 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 at example.com', 'inactive-notify-address at example.com', nil, false
-    create_user_and_verify_setup_and_notifications false, [], 'inactive-notify-address at 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 at example.com', 'inactive-notify-address at example.com', nil, nil
+    create_user_and_verify_setup_and_notifications true, 'active-notify-address at 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 at example.com', 'inactive-notify-address at example.com', nil, nil
+    create_user_and_verify_setup_and_notifications false, [], 'inactive-notify-address at example.com', nil, nil
+    create_user_and_verify_setup_and_notifications false, [], [], nil, nil
   end
 
   [
-    [false, [], [], 'inactive-none at example.com', false, false, true],
-    [false, [], [], 'inactive-vm at example.com', true, false, true],
-    [false, [], [], 'inactive-repo at example.com', false, true, true],
-    [false, [], [], 'inactive-both at example.com', true, true, true],
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'active-none at example.com', false, false, true],
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'active-vm at example.com', true, false, true],
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'active-repo at example.com', false, true, true],
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'active-both at 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 at example.com', false, false, true],   # since we are not creating repo and vm login, this blacklisted name is not a problem
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'arvados at example.com', false, false, true],   # since we are not creating repo and vm login, this blacklisted name is not a problem
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'root at example.com', true, false, false], # blacklisted name
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'root at example.com', true, false, false], # blacklisted name
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'roo_t at 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 at example.com', true, true, false],
-
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_repo at example.com', true, true, true],  # existing repository name 'auto_setup_repo'
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_repo at example.com', true, false, true],  # existing repository name 'auto_setup_repo'
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_repo at example.com', false, true, true],  # existing repository name 'auto_setup_repo'
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_repo at example.com', false, false, true],  # existing repository name 'auto_setup_repo', but we are not creating repo or login link
-
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_vm_login at example.com', true, true, true], # existing vm login name
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_vm_login at example.com', true, false, true], # existing vm login name
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_vm_login at example.com', false, true, true], # existing vm login name
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', 'auto_setup_vm_login at example.com', false, false, true], # existing vm login name, but we are not creating repo or login link
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '*!*@example.com', true, false, false], # username is invalid format
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', '*!*@example.com', false, false, true], # since no repo and vm login, username is ok (not validated)
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '*!*@example.com', false, false, true], # since no repo and vm login, username is ok (not validated)
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '&4ad at example.com', true, true, false], # username is invalid format
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '&4ad at example.com', false, false, true], # no repo or vm login, so format not checked
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', '&4ad at example.com', true, true, false], # username is invalid format
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', '&4ad at example.com', false, false, true], # no repo or vm login, so format not checked
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '4ad at example.com', true, true, false], # username is invalid format
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '4ad at example.com', false, false, true], # no repo or vm login, so format not checked
-    [false, 'active-notify at example.com', 'inactive-notify at example.com', '4ad at example.com', false, false, true], # no repo or vm login, so format not checked
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '.foo at example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', '.foo at example.com', true, false, false], # invalid format
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'bar. at example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'bar. at example.com', true, false, false], # valid format
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'ice9 at example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'ice9 at example.com', true, false, true], # valid format
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'o_o at example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'o_o at example.com', true, false, true], # valid format
-
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'r00t at example.com', false, false, true], # no repo or vm login, so format not checked
-    [true, 'active-notify at example.com', 'inactive-notify at example.com', 'r00t at 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 at example.com", false, false, "inactivenone"],
+    [false, [], [], "inactive-vm at example.com", true, false, "inactivevm"],
+    [false, [], [], "inactive-repo at example.com", false, true, "inactiverepo"],
+    [false, [], [], "inactive-both at example.com", true, true, "inactiveboth"],
+
+    # Easy active user tests.
+    [true, "active-notify at example.com", "inactive-notify at example.com", "active-none at example.com", false, false, "activenone"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "active-vm at example.com", true, false, "activevm"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "active-repo at example.com", false, true, "activerepo"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "active-both at 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 at example.com", "inactive-notify at example.com", "*!*@example.com", true, false, nil],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "*!*@example.com", false, false, nil],
+
+    # Test users with various username transformations.
+    [false, [], [], "arvados at example.com", false, false, "arvados2"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "arvados at example.com", false, false, "arvados2"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "root at example.com", true, false, "root2"],
+    [false, "active-notify at example.com", "inactive-notify at example.com", "root at example.com", true, false, "root2"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "roo_t at example.com", false, true, "root2"],
+    [false, [], [], "^^incorrect_format at example.com", true, true, "incorrectformat"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "&4a_d9. at example.com", true, true, "ad9"],
+    [true, "active-notify at example.com", "inactive-notify at example.com", "&4a_d9. at example.com", false, false, "ad9"],
+    [false, "active-notify at example.com", "inactive-notify at example.com", "&4a_d9. at example.com", true, true, "ad9"],
+    [false, "active-notify at example.com", "inactive-notify at example.com", "&4a_d9. at 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,

commit 1a3c91dc982c91416d4f0ed1135ab5bc84f32a7c
Author: Brett Smith <brett at curoverse.com>
Date:   Wed Mar 18 15:59:51 2015 -0400

    4253: Clean up some user setup methods.
    
    As much as possible, use more methods provided by Rails, rather than
    reimplementing them ourselves.

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 3978df7..d4d722f 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -384,80 +384,45 @@ class User < ArvadosModel
       return
     end
 
-    # Check for an existing repository with the same name we're about to use.
-    repo = Repository.where(name: repo_name).first
-
-    if repo
-      logger.warn "Repository exists for #{repo_name}: #{repo[:uuid]}."
-
-      # Look for existing repository access for this repo
-      repo_perms = Link.where(tail_uuid: self.uuid,
-                              head_uuid: repo[:uuid],
-                              link_class: 'permission',
-                              name: 'can_manage')
-      if repo_perms.any?
-        logger.warn "User already has repository access " +
-            repo_perms.collect { |p| p[:uuid] }.inspect
-        return repo_perms.first
-      end
-    end
-
-    # create repo, if does not already exist
-    repo ||= Repository.create(name: repo_name)
+    repo = Repository.where(name: repo_name).first_or_create!
     logger.info { "repo uuid: " + repo[:uuid] }
-
-    repo_perm = Link.create(tail_uuid: self.uuid,
-                            head_uuid: repo[:uuid],
-                            link_class: 'permission',
-                            name: 'can_manage')
+    repo_perm = Link.where(tail_uuid: uuid, head_uuid: repo.uuid,
+                           link_class: "permission",
+                           name: "can_manage").first_or_create!
     logger.info { "repo permission: " + repo_perm[:uuid] }
     return repo_perm
   end
 
   # create login permission for the given vm_uuid, if it does not already exist
   def create_vm_login_permission_link(vm_uuid, repo_name)
-    begin
-
-      # vm uuid is optional
-      if vm_uuid
-        vm = VirtualMachine.where(uuid: vm_uuid).first
+    # vm uuid is optional
+    if vm_uuid
+      vm = VirtualMachine.where(uuid: vm_uuid).first
 
-        if not vm
-          logger.warn "Could not find virtual machine for #{vm_uuid.inspect}"
-          raise "No vm found for #{vm_uuid}"
-        end
-      else
-        return
+      if not vm
+        logger.warn "Could not find virtual machine for #{vm_uuid.inspect}"
+        raise "No vm found for #{vm_uuid}"
       end
+    else
+      return
+    end
 
-      logger.info { "vm uuid: " + vm[:uuid] }
-
-      login_perms = Link.where(tail_uuid: self.uuid,
-                              head_uuid: vm[:uuid],
-                              link_class: 'permission',
-                              name: 'can_login')
+    logger.info { "vm uuid: " + vm[:uuid] }
+    login_attrs = {
+      tail_uuid: uuid, head_uuid: vm.uuid,
+      link_class: "permission", name: "can_login",
+    }
 
-      perm_exists = false
-      login_perms.each do |perm|
-        if perm.properties['username'] == repo_name
-          perm_exists = perm
-          break
-        end
-      end
+    login_perm = Link.
+      where(login_attrs).
+      select { |link| link.properties["username"] == repo_name }.
+      first
 
-      if perm_exists
-        login_perm = perm_exists
-      else
-        login_perm = Link.create(tail_uuid: self.uuid,
-                                 head_uuid: vm[:uuid],
-                                 link_class: 'permission',
-                                 name: 'can_login',
-                                 properties: {'username' => repo_name})
-        logger.info { "login permission: " + login_perm[:uuid] }
-      end
+    login_perm ||= Link.
+      create(login_attrs.merge(properties: {"username" => repo_name}))
 
-      return login_perm
-    end
+    logger.info { "login permission: " + login_perm[:uuid] }
+    login_perm
   end
 
   # add the user to the 'All users' group

commit ee46c18dc4aa90e84cd022543b754ca25e0f242a
Author: Brett Smith <brett at curoverse.com>
Date:   Tue Mar 17 18:10:14 2015 -0400

    4253: Add a username attribute to users.
    
    * Add the column, and propagate it based on available VM logins or
      e-mail address, if possible.
    * Add format validation and tests.
    * Set new usernames based on e-mail address, with tests.

diff --git a/doc/api/schema/User.html.textile.liquid b/doc/api/schema/User.html.textile.liquid
index 9a1b056..335b6c1 100644
--- a/doc/api/schema/User.html.textile.liquid
+++ b/doc/api/schema/User.html.textile.liquid
@@ -19,6 +19,7 @@ Each User has, in addition to the usual "attributes of Arvados resources":{{site
 table(table table-bordered table-condensed).
 |_. Attribute|_. Type|_. Description|_. Example|
 |email|string|||
+|username|string|The username used for the user's git repositories and virtual machine logins.  Usernames must start with a letter, and contain only alphanumerics.  When a new user is created, a default username is set from their e-mail address.  Only administrators may change the username.||
 |first_name|string|||
 |last_name|string|||
 |identity_url|string|||
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index a47a458..3978df7 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -8,19 +8,29 @@ class User < ArvadosModel
 
   serialize :prefs, Hash
   has_many :api_client_authorizations
+  validates(:username,
+            format: {
+              with: /^[A-Za-z][A-Za-z0-9]*$/,
+              message: "must begin with a letter and contain only alphanumerics",
+            },
+            uniqueness: true,
+            allow_nil: true)
   before_update :prevent_privilege_escalation
   before_update :prevent_inactive_admin
   before_create :check_auto_admin
+  before_create :set_initial_username, :if => Proc.new { |user|
+    user.username.nil? and user.email
+  }
   after_create :add_system_group_permission_link
   after_create :auto_setup_new_user
   after_create :send_admin_notifications
   after_update :send_profile_created_notification
 
-
   has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
 
   api_accessible :user, extend: :common do |t|
     t.add :email
+    t.add :username
     t.add :full_name
     t.add :first_name
     t.add :last_name
@@ -222,9 +232,13 @@ class User < ArvadosModel
   end
 
   def permission_to_update
-    # users must be able to update themselves (even if they are
-    # inactive) in order to create sessions
-    self == current_user or super
+    if username_changed?
+      current_user.andand.is_admin
+    else
+      # users must be able to update themselves (even if they are
+      # inactive) in order to create sessions
+      self == current_user or super
+    end
   end
 
   def permission_to_create
@@ -237,13 +251,62 @@ class User < ArvadosModel
     return if self.uuid.end_with?('anonymouspublic')
     if (User.where("email = ?",self.email).where(:is_admin => true).count == 0 and
         Rails.configuration.auto_admin_user and self.email == Rails.configuration.auto_admin_user) or
-       (User.where("uuid not like '%-000000000000000'").where(:is_admin => true).count == 0 and 
+       (User.where("uuid not like '%-000000000000000'").where(:is_admin => true).count == 0 and
         Rails.configuration.auto_admin_first_user)
       self.is_admin = true
       self.is_active = true
     end
   end
 
+  def find_usable_username_from(basename)
+    # If "basename" is a usable username, return that.
+    # Otherwise, find a unique username "basenameN", where N is the
+    # smallest integer greater than 1, and return that.
+    # Return nil if a unique username can't be found after reasonable
+    # searching.
+    quoted_name = self.class.connection.quote_string(basename)
+    next_username = basename
+    next_suffix = 1
+    while Rails.configuration.auto_setup_name_blacklist.include?(next_username)
+      next_suffix += 1
+      next_username = "%s%i" % [basename, next_suffix]
+    end
+    0.upto(6).each do |suffix_len|
+      pattern = "%s%s" % [quoted_name, "_" * suffix_len]
+      self.class.
+          where("username like '#{pattern}'").
+          select(:username).
+          order(username: :asc).
+          find_each do |other_user|
+        if other_user.username > next_username
+          break
+        elsif other_user.username == next_username
+          next_suffix += 1
+          next_username = "%s%i" % [basename, next_suffix]
+        end
+      end
+      return next_username if (next_username.size <= pattern.size)
+    end
+    nil
+  end
+
+  def set_initial_username
+    email_parts = email.partition("@")
+    local_parts = email_parts.first.partition("+")
+    if email_parts.any?(&:empty?)
+      return
+    elsif not local_parts.first.empty?
+      base_username = local_parts.first
+    else
+      base_username = email_parts.first
+    end
+    base_username.sub!(/^[^A-Za-z]+/, "")
+    base_username.gsub!(/[^A-Za-z0-9]/, "")
+    unless base_username.empty?
+      self.username = find_usable_username_from(base_username)
+    end
+  end
+
   def prevent_privilege_escalation
     if current_user.andand.is_admin
       return true
diff --git a/services/api/db/migrate/20150317132720_add_username_to_users.rb b/services/api/db/migrate/20150317132720_add_username_to_users.rb
new file mode 100644
index 0000000..6772414
--- /dev/null
+++ b/services/api/db/migrate/20150317132720_add_username_to_users.rb
@@ -0,0 +1,130 @@
+require 'has_uuid'
+require 'kind_and_etag'
+
+class AddUsernameToUsers < ActiveRecord::Migration
+  include CurrentApiClient
+
+  SEARCH_INDEX_COLUMNS =
+    ["uuid", "owner_uuid", "modified_by_client_uuid",
+     "modified_by_user_uuid", "email", "first_name", "last_name",
+     "identity_url", "default_owner_uuid"]
+
+  class ArvadosModel < ActiveRecord::Base
+    self.abstract_class = true
+    extend HasUuid::ClassMethods
+    include CurrentApiClient
+    include KindAndEtag
+    before_create do |record|
+      record.uuid ||= record.class.generate_uuid
+      record.owner_uuid ||= system_user_uuid
+    end
+    serialize :properties, Hash
+
+    def self.to_s
+      # Clean up the name of the stub model class so we generate correct UUIDs.
+      super.rpartition("::").last
+    end
+  end
+
+  class Log < ArvadosModel
+    def self.log_for(thing, age="old")
+      { "#{age}_etag" => thing.etag,
+        "#{age}_attributes" => thing.attributes,
+      }
+    end
+
+    def self.log_create(thing)
+      new_log("create", thing, log_for(thing, "new"))
+    end
+
+    def self.log_update(thing, start_state)
+      new_log("update", thing, start_state.merge(log_for(thing, "new")))
+    end
+
+    def self.log_destroy(thing)
+      new_log("destroy", thing, log_for(thing, "old"))
+    end
+
+    private
+
+    def self.new_log(event_type, thing, properties)
+      create!(event_type: event_type,
+              event_at: Time.now,
+              object_uuid: thing.uuid,
+              object_owner_uuid: thing.owner_uuid,
+              properties: properties)
+    end
+  end
+
+  class Link < ArvadosModel
+  end
+
+  class User < ArvadosModel
+  end
+
+  def sanitize_username(username)
+    username.
+      sub(/^[^A-Za-z]+/, "").
+      gsub(/[^A-Za-z0-9]/, "")
+  end
+
+  def usernames_wishlist(user)
+    usernames = Hash.new(0)
+    usernames[user.email.split("@", 2).first] += 1
+    Link.
+       where(tail_uuid: user.uuid, link_class: "permission", name: "can_login").
+       find_each do |login_perm|
+      username = login_perm.properties["username"]
+      usernames[username] += 2 if (username and not username.empty?)
+    end
+    usernames.keys.
+      sort_by { |n| -usernames[n] }.
+      map { |n| sanitize_username(n) }.
+      reject(&:empty?)
+  end
+
+  def increment_username(username)
+    @username_suffixes[username] += 1
+    "%s%i" % [username, @username_suffixes[username]]
+  end
+
+  def each_wanted_username(user)
+    usernames = usernames_wishlist(user)
+    usernames.each { |n| yield n }
+    base_username = usernames.first || "arvadosuser"
+    loop { yield increment_username(base_username) }
+  end
+
+  def recreate_search_index(columns)
+    remove_index :users, name: "users_search_index"
+    add_index :users, columns, name: "users_search_index"
+  end
+
+  def up
+    @username_suffixes = Hash.new(1)
+    add_column :users, :username, :string, null: true
+    add_index :users, :username, unique: true
+    recreate_search_index(SEARCH_INDEX_COLUMNS + ["username"])
+
+    [Link, Log, User].each { |m| m.reset_column_information }
+    User.where(is_active: true).order(created_at: :asc).find_each do |user|
+      start_log = Log.log_for(user)
+      each_wanted_username(user) do |username|
+        user.username = username
+        begin
+          user.save!
+          break
+        rescue ActiveRecord::RecordNotUnique
+          # We'll try the next username.
+        end
+      end
+      Log.log_update(user, start_log)
+    end
+  end
+
+  def down
+    remove_index :users, :username
+    recreate_search_index(SEARCH_INDEX_COLUMNS)
+    remove_column :users, :username
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 0711f90..40861f4 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -889,7 +889,8 @@ CREATE TABLE users (
     prefs text,
     updated_at timestamp without time zone NOT NULL,
     default_owner_uuid character varying(255),
-    is_active boolean DEFAULT false
+    is_active boolean DEFAULT false,
+    username character varying(255)
 );
 
 
@@ -1965,6 +1966,13 @@ CREATE INDEX index_users_on_owner_uuid ON users USING btree (owner_uuid);
 
 
 --
+-- Name: index_users_on_username; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE UNIQUE INDEX index_users_on_username ON users USING btree (username);
+
+
+--
 -- Name: index_users_on_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
 
@@ -2122,7 +2130,7 @@ CREATE UNIQUE INDEX unique_schema_migrations ON schema_migrations USING btree (v
 -- Name: users_search_index; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
 
-CREATE INDEX users_search_index ON users USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, email, first_name, last_name, identity_url, default_owner_uuid);
+CREATE INDEX users_search_index ON users USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, email, first_name, last_name, identity_url, default_owner_uuid, username);
 
 
 --
@@ -2364,4 +2372,6 @@ INSERT INTO schema_migrations (version) VALUES ('20150216193428');
 
 INSERT INTO schema_migrations (version) VALUES ('20150303210106');
 
-INSERT INTO schema_migrations (version) VALUES ('20150312151136');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20150312151136');
+
+INSERT INTO schema_migrations (version) VALUES ('20150317132720');
\ No newline at end of file
diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index c04aa47..46fbd0f 100644
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -25,6 +25,7 @@ admin:
   identity_url: https://admin.openid.local
   is_active: true
   is_admin: true
+  username: admin
   prefs:
     profile:
       organization: example.com
@@ -39,6 +40,7 @@ miniadmin:
   identity_url: https://miniadmin.openid.local
   is_active: true
   is_admin: false
+  username: miniadmin
   prefs:
     profile:
       organization: example.com
@@ -53,6 +55,7 @@ rominiadmin:
   identity_url: https://rominiadmin.openid.local
   is_active: true
   is_admin: false
+  username: rominiadmin
   prefs:
     profile:
       organization: example.com
@@ -67,6 +70,7 @@ active:
   identity_url: https://active-user.openid.local
   is_active: true
   is_admin: false
+  username: active
   prefs:
     profile:
       organization: example.com
@@ -81,6 +85,7 @@ project_viewer:
   identity_url: https://project-viewer.openid.local
   is_active: true
   is_admin: false
+  username: projectviewer
   prefs:
     profile:
       organization: example.com
@@ -96,6 +101,7 @@ future_project_user:
   identity_url: https://future-project-user.openid.local
   is_active: true
   is_admin: false
+  username: futureprojectviewer
   prefs:
     profile:
       organization: example.com
@@ -110,6 +116,7 @@ subproject_admin:
   identity_url: https://subproject-admin.openid.local
   is_active: true
   is_admin: false
+  username: subprojectadmin
   prefs:
     profile:
       organization: example.com
@@ -124,6 +131,7 @@ spectator:
   identity_url: https://spectator.openid.local
   is_active: true
   is_admin: false
+  username: spectator
   prefs:
     profile:
       organization: example.com
@@ -184,6 +192,7 @@ job_reader:
   identity_url: https://spectator.openid.local
   is_active: true
   is_admin: false
+  username: jobber
   prefs:
     profile:
       organization: example.com
@@ -223,6 +232,7 @@ user_foo_in_sharing_group:
   identity_url: https://user_foo_in_sharing_group.openid.local
   is_active: true
   is_admin: false
+  username: fooinsharing
 
 user_bar_in_sharing_group:
   owner_uuid: zzzzz-tpzed-000000000000000
@@ -233,6 +243,7 @@ user_bar_in_sharing_group:
   identity_url: https://user_bar_in_sharing_group.openid.local
   is_active: true
   is_admin: false
+  username: barinsharing
 
 user1_with_load:
   owner_uuid: zzzzz-tpzed-000000000000000
@@ -257,6 +268,7 @@ fuse:
   identity_url: https://fuse.openid.local
   is_active: true
   is_admin: false
+  username: FUSE
   prefs:
     profile:
       organization: example.com
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 9bcb011..9ba5646 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -9,6 +9,105 @@ class UserTest < ActiveSupport::TestCase
     system_user
   end
 
+  %w(a aa a0 aA Aa AA A0).each do |username|
+    test "#{username.inspect} is a valid username" do
+      user = User.new(username: username)
+      assert(user.valid?)
+    end
+  end
+
+  test "username is not required" do
+    user = User.new(username: nil)
+    assert(user.valid?)
+  end
+
+  test "username beginning with numeral is invalid" do
+    user = User.new(username: "0a")
+    refute(user.valid?)
+  end
+
+  "\\.-_/!@#$%^&*()[]{}".each_char do |bad_char|
+    test "username containing #{bad_char.inspect} is invalid" do
+      user = User.new(username: "bad#{bad_char}username")
+      refute(user.valid?)
+    end
+  end
+
+  test "username must be unique" do
+    user = User.new(username: users(:active).username)
+    refute(user.valid?)
+  end
+
+  test "non-admin can't update username" do
+    set_user_from_auth :active_trustedclient
+    user = User.find_by_uuid(users(:active).uuid)
+    user.username = "selfupdate"
+    begin
+      refute(user.save)
+    rescue ArvadosModel::PermissionDeniedError
+      # That works too.
+    end
+  end
+
+  def check_admin_username_change(fixture_name)
+    set_user_from_auth :admin_trustedclient
+    user = User.find_by_uuid(users(fixture_name).uuid)
+    user.username = "newnamefromtest"
+    assert(user.save)
+  end
+
+  test "admin can set username" do
+    check_admin_username_change(:active_no_prefs)
+  end
+
+  test "admin can update username" do
+    check_admin_username_change(:active)
+  end
+
+  test "admin can update own username" do
+    check_admin_username_change(:admin)
+  end
+
+  def check_new_username_setting(email_name, expect_name)
+    set_user_from_auth :admin
+    user = User.create!(email: "#{email_name}@example.org")
+    assert_equal(expect_name, user.username)
+  end
+
+  test "new username set from e-mail" do
+    check_new_username_setting("dakota", "dakota")
+  end
+
+  test "new username set from e-mail with leading digits" do
+    check_new_username_setting("1dakota9", "dakota9")
+  end
+
+  test "new username set from e-mail with punctuation" do
+    check_new_username_setting("dakota.9", "dakota9")
+  end
+
+  test "new username set from e-mail with leading digits and punctuation" do
+    check_new_username_setting("1.dakota.z", "dakotaz")
+  end
+
+  test "new username set from e-mail with extra part" do
+    check_new_username_setting("dakota+arvados", "dakota")
+  end
+
+  test "new username set with deduplication" do
+    name = users(:active).username
+    check_new_username_setting(name, "#{name}2")
+  end
+
+  test "new username set avoiding blacklist" do
+    Rails.configuration.auto_setup_name_blacklist = ["root"]
+    check_new_username_setting("root", "root2")
+  end
+
+  test "no username set when no base available" do
+    check_new_username_setting("_", nil)
+  end
+
   [[false, 'foo at example.com', true, nil],
    [false, 'bar at example.com', nil, true],
    [true, 'foo at example.com', true, nil],

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list