Merge branch 'master' into 15803-unsetup
authorPeter Amstutz <pamstutz@veritasgenetics.com>
Mon, 25 Nov 2019 22:37:25 +0000 (17:37 -0500)
committerPeter Amstutz <pamstutz@veritasgenetics.com>
Mon, 25 Nov 2019 22:37:25 +0000 (17:37 -0500)
1  2 
services/api/app/models/user.rb
services/api/test/functional/arvados/v1/users_controller_test.rb

index 8a7c71f00a53f9874c26039f6a660027880b13f0,a49aa6f56a22ead2398198401c15be2a8860ec97..d9bb18c3e505338daee7152746fe58af267fff4f
@@@ -21,7 -21,6 +21,7 @@@ class User < ArvadosMode
              },
              uniqueness: true,
              allow_nil: true)
 +  validate :must_unsetup_to_deactivate
    before_update :prevent_privilege_escalation
    before_update :prevent_inactive_admin
    before_update :verify_repositories_empty, :if => Proc.new { |user|
  
    # create links
    def setup(openid_prefix:, repo_name: nil, vm_uuid: nil)
 -    oid_login_perm = create_oid_login_perm openid_prefix
      repo_perm = create_user_repo_link repo_name
      vm_login_perm = create_vm_login_permission_link(vm_uuid, username) if vm_uuid
      group_perm = create_user_group_link
  
 -    return [oid_login_perm, repo_perm, vm_login_perm, group_perm, self].compact
 +    return [repo_perm, vm_login_perm, group_perm, self].compact
    end
  
    # delete user signatures, login, repo, and vm perms, and mark as inactive
    def unsetup
      # delete oid_login_perms for this user
 +    #
 +    # note: these permission links are obsolete, they have no effect
 +    # on anything and they are not created for new users.
      Link.where(tail_uuid: self.email,
                       link_class: 'permission',
                       name: 'can_login').destroy_all
      self.save!
    end
  
 +  def must_unsetup_to_deactivate
 +    if self.is_active_changed? &&
 +       self.is_active_was == true &&
 +       !self.is_active
 +
 +      group = Group.where(name: 'All users').select do |g|
 +        g[:uuid].match(/-f+$/)
 +      end.first
 +
 +      # When a user is set up, they are added to the "All users"
 +      # group.  A user that is part of the "All users" group is
 +      # allowed to self-activate.
 +      #
 +      # It doesn't make sense to deactivate a user (set is_active =
 +      # false) without first removing them from the "All users" group,
 +      # because they would be able to immediately reactivate
 +      # themselves.
 +      #
 +      # The 'unsetup' method removes the user from the "All users"
 +      # group (and also sets is_active = false) so send a message
 +      # explaining the correct way to deactivate a user.
 +      #
 +      if Link.where(tail_uuid: self.uuid,
 +                    head_uuid: group[:uuid],
 +                    link_class: 'permission',
 +                    name: 'can_read').any?
 +        errors.add :is_active, "cannot be set to false directly, use the 'Deactivate' button on Workbench, or the 'unsetup' API call"
 +      end
 +    end
 +  end
 +
    def set_initial_username(requested: false)
      if !requested.is_a?(String) || requested.empty?
        email_parts = email.partition("@")
                                :is_admin => false,
                                :is_active => Rails.configuration.Users.NewUsersAreActive)
  
-       primary_user.set_initial_username(requested: info['username']) if info['username']
+       primary_user.set_initial_username(requested: info['username']) if info['username'] && !info['username'].blank?
        primary_user.identity_url = info['identity_url'] if identity_url
      end
  
      merged
    end
  
 -  def create_oid_login_perm(openid_prefix)
 -    # Check oid_login_perm
 -    oid_login_perms = Link.where(tail_uuid: self.email,
 -                                 head_uuid: self.uuid,
 -                                 link_class: 'permission',
 -                                 name: 'can_login')
 -
 -    if !oid_login_perms.any?
 -      # create openid login permission
 -      oid_login_perm = Link.create!(link_class: 'permission',
 -                                   name: 'can_login',
 -                                   tail_uuid: self.email,
 -                                   head_uuid: self.uuid,
 -                                   properties: {
 -                                     "identity_url_prefix" => openid_prefix,
 -                                   })
 -      logger.info { "openid login permission: " + oid_login_perm[:uuid] }
 -    else
 -      oid_login_perm = oid_login_perms.first
 -    end
 -
 -    return oid_login_perm
 -  end
 -
    def create_user_repo_link(repo_name)
      # repo_name is optional
      if not repo_name
index a442230a7bd8c3c6167a1e79bc2a69723fb8c2ea,74f017a678f76643884ff09261f17188483328eb..e3763c389e243a44b0bd891a9809713f481788a1
@@@ -113,8 -113,11 +113,8 @@@ class Arvados::V1::UsersControllerTest 
      assert_not_nil created['email'], 'expected non-nil email'
      assert_nil created['identity_url'], 'expected no identity_url'
  
 -    # arvados#user, repo link and link add user to 'All users' group
 -    verify_links_added 4
 -
 -    verify_link response_items, 'arvados#user', true, 'permission', 'can_login',
 -        created['uuid'], created['email'], 'arvados#user', false, 'User'
 +    # repo link and link add user to 'All users' group
 +    verify_links_added 3
  
      verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
          "foo/#{repo_name}", created['uuid'], 'arvados#repository', true, 'Repository'
      assert_not_nil response_object['uuid'], 'expected uuid for the new user'
      assert_equal response_object['email'], 'foo@example.com', 'expected given email'
  
 -    # four extra links; system_group, login, group and repo perms
 -    verify_links_added 4
 +    # three extra links; system_group, group and repo perms
 +    verify_links_added 3
    end
  
    test "setup user with fake vm and expect error" do
      assert_not_nil response_object['uuid'], 'expected uuid for the new user'
      assert_equal response_object['email'], 'foo@example.com', 'expected given email'
  
 -    # five extra links; system_group, login, group, vm, repo
 -    verify_links_added 5
 +    # four extra links; system_group, group, vm, repo
 +    verify_links_added 4
    end
  
    test "setup user with valid email, no vm and no repo as input" do
      assert_not_nil response_object['uuid'], 'expected uuid for new user'
      assert_equal response_object['email'], 'foo@example.com', 'expected given email'
  
 -    # three extra links; system_group, login, and group
 -    verify_links_added 3
 -
 -    verify_link response_items, 'arvados#user', true, 'permission', 'can_login',
 -        response_object['uuid'], response_object['email'], 'arvados#user', false, 'User'
 +    # two extra links; system_group, and group
 +    verify_links_added 2
  
      verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
          'All users', response_object['uuid'], 'arvados#group', true, 'Group'
      assert_equal 'test_first_name', response_object['first_name'],
          'expecting first name'
  
 -    # five extra links; system_group, login, group, repo and vm
 -    verify_links_added 5
 +    # four extra links; system_group, group, repo and vm
 +    verify_links_added 4
    end
  
    test "setup user with an existing user email and check different object is created" do
      assert_not_equal response_object['uuid'], inactive_user['uuid'],
          'expected different uuid after create operation'
      assert_equal inactive_user['email'], response_object['email'], 'expected given email'
 -    # system_group, openid, group, and repo. No vm link.
 -    verify_links_added 4
 +    # system_group, group, and repo. No vm link.
 +    verify_links_added 3
    end
  
    test "setup user with openid prefix" do
      assert_nil created['identity_url'], 'expected no identity_url'
  
      # verify links
 -    # four new links: system_group, arvados#user, repo, and 'All users' group.
 -    verify_links_added 4
 -
 -    verify_link response_items, 'arvados#user', true, 'permission', 'can_login',
 -        created['uuid'], created['email'], 'arvados#user', false, 'User'
 +    # three new links: system_group, repo, and 'All users' group.
 +    verify_links_added 3
  
      verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
          'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
      assert_not_nil created['email'], 'expected non-nil email'
      assert_nil created['identity_url'], 'expected no identity_url'
  
 -    # five new links: system_group, arvados#user, repo, vm and 'All
 -    # users' group link
 -    verify_links_added 5
 +    # four new links: system_group, repo, vm and 'All users' group link
 +    verify_links_added 4
  
 -    verify_link response_items, 'arvados#user', true, 'permission', 'can_login',
 -        created['uuid'], created['email'], 'arvados#user', false, 'User'
 +    # system_group isn't part of the response.  See User#add_system_group_permission_link
  
      verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
          'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
      assert_nil(users(:project_viewer).redirect_to_user_uuid)
    end
  
+   test "batch update fails for non-admin" do
+     authorize_with(:active)
+     patch(:batch_update, params: {updates: {}})
+     assert_response(403)
+   end
+   test "batch update" do
+     existinguuid = 'remot-tpzed-foobarbazwazqux'
+     newuuid = 'remot-tpzed-newnarnazwazqux'
+     act_as_system_user do
+       User.create!(uuid: existinguuid, email: 'root@existing.example.com')
+     end
+     authorize_with(:admin)
+     patch(:batch_update,
+           params: {
+             updates: {
+               existinguuid => {
+                 'first_name' => 'root',
+                 'email' => 'root@remot.example.com',
+                 'is_active' => true,
+                 'is_admin' => true,
+                 'prefs' => {'foo' => 'bar'},
+               },
+               newuuid => {
+                 'first_name' => 'noot',
+                 'email' => 'root@remot.example.com',
+               },
+             }})
+     assert_response(:success)
+     assert_equal('root', User.find_by_uuid(existinguuid).first_name)
+     assert_equal('root@remot.example.com', User.find_by_uuid(existinguuid).email)
+     assert_equal(true, User.find_by_uuid(existinguuid).is_active)
+     assert_equal(true, User.find_by_uuid(existinguuid).is_admin)
+     assert_equal({'foo' => 'bar'}, User.find_by_uuid(existinguuid).prefs)
+     assert_equal('noot', User.find_by_uuid(newuuid).first_name)
+     assert_equal('root@remot.example.com', User.find_by_uuid(newuuid).email)
+   end
    NON_ADMIN_USER_DATA = ["uuid", "kind", "is_active", "email", "first_name",
                           "last_name", "username"].sort