10557: Tidy up some user setup code.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 7a1f69991117118f188f1516a313a38432662bd4..0e95042f68a428401868a67838ed409cd221da50 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -113,16 +113,12 @@ class Arvados::V1::UsersController < ApplicationController
       full_repo_name = "#{@object.username}/#{params[:repo_name]}"
     end
 
-    if object_found
-      @response = @object.setup_repo_vm_links full_repo_name,
-                    params[:vm_uuid], params[:openid_prefix]
-    else
-      @response = User.setup @object, params[:openid_prefix],
-                    full_repo_name, params[:vm_uuid]
-    end
+    @response = @object.setup(repo_name: full_repo_name,
+                              vm_uuid: params[:vm_uuid],
+                              openid_prefix: params[:openid_prefix])
 
     # setup succeeded. send email to user
-    if params[:send_notification_email] == true || params[:send_notification_email] == 'true'
+    if params[:send_notification_email]
       UserNotifier.account_is_setup(@object).deliver_now
     end
 

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index d944474712708b3207f28807e10349b8f34007b0..b654b5a023071f1abe95400a3309964365c7e0af 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -182,15 +182,11 @@ class User < ArvadosModel
     r
   end
 
-  def self.setup(user, openid_prefix, repo_name=nil, vm_uuid=nil)
-    return user.setup_repo_vm_links(repo_name, vm_uuid, openid_prefix)
-  end
-
   # create links
-  def setup_repo_vm_links(repo_name, vm_uuid, openid_prefix)
+  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
+    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
@@ -364,13 +360,12 @@ class User < ArvadosModel
     merged
   end
 
-  def create_oid_login_perm (openid_prefix)
-    login_perm_props = { "identity_url_prefix" => openid_prefix}
-
+  def create_oid_login_perm(openid_prefix)
     # Check oid_login_perm
     oid_login_perms = Link.where(tail_uuid: self.email,
-                                   link_class: 'permission',
-                                   name: 'can_login').where("head_uuid = ?", self.uuid)
+                                 head_uuid: self.uuid,
+                                 link_class: 'permission',
+                                 name: 'can_login')
 
     if !oid_login_perms.any?
       # create openid login permission
@@ -378,8 +373,9 @@ class User < ArvadosModel
                                    name: 'can_login',
                                    tail_uuid: self.email,
                                    head_uuid: self.uuid,
-                                   properties: login_perm_props
-                                  )
+                                   properties: {
+                                     "identity_url_prefix" => openid_prefix,
+                                   })
       logger.info { "openid login permission: " + oid_login_perm[:uuid] }
     else
       oid_login_perm = oid_login_perms.first
@@ -407,15 +403,12 @@ class User < ArvadosModel
   # create login permission for the given vm_uuid, if it does not already exist
   def create_vm_login_permission_link(vm_uuid, repo_name)
     # vm uuid is optional
-    if vm_uuid
-      vm = VirtualMachine.where(uuid: vm_uuid).first
+    return if !vm_uuid
 
-      if not vm
-        logger.warn "Could not find virtual machine for #{vm_uuid.inspect}"
-        raise "No vm found for #{vm_uuid}"
-      end
-    else
-      return
+    vm = VirtualMachine.where(uuid: vm_uuid).first
+    if !vm
+      logger.warn "Could not find virtual machine for #{vm_uuid.inspect}"
+      raise "No vm found for #{vm_uuid}"
     end
 
     logger.info { "vm uuid: " + vm[:uuid] }
@@ -470,7 +463,7 @@ class User < ArvadosModel
 
   # Automatically setup new user during creation
   def auto_setup_new_user
-    setup_repo_vm_links(nil, nil, Rails.configuration.default_openid_prefix)
+    setup(openid_prefix: Rails.configuration.default_openid_prefix)
     if username
       create_vm_login_permission_link(Rails.configuration.auto_setup_new_users_with_vm_uuid,
                                       username)

diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 742deda0c663a0946b8eac12f224b5c072069f25..d61787320f2d4c996a772a19950543d930276702 100644 (file)
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -447,7 +447,9 @@ class UserTest < ActiveSupport::TestCase
 
     vm = VirtualMachine.create
 
-    response = User.setup user, openid_prefix, 'foo/testrepo', vm.uuid
+    response = user.setup(openid_prefix: openid_prefix,
+                          repo_name: 'foo/testrepo',
+                          vm_uuid: vm.uuid)
 
     resp_user = find_obj_in_resp response, 'User'
     verify_user resp_user, email
@@ -490,7 +492,9 @@ class UserTest < ActiveSupport::TestCase
 
     verify_link resp_link, 'permission', 'can_login', email, bad_uuid
 
-    response = User.setup user, openid_prefix, 'foo/testrepo', vm.uuid
+    response = user.setup(openid_prefix: openid_prefix,
+                          repo_name: 'foo/testrepo',
+                          vm_uuid: vm.uuid)
 
     resp_user = find_obj_in_resp response, 'User'
     verify_user resp_user, email
@@ -522,7 +526,7 @@ class UserTest < ActiveSupport::TestCase
 
     user = User.create ({uuid: 'zzzzz-tpzed-abcdefghijklmno', email: email})
 
-    response = User.setup user, openid_prefix
+    response = user.setup(openid_prefix: openid_prefix)
 
     resp_user = find_obj_in_resp response, 'User'
     verify_user resp_user, email
@@ -537,7 +541,8 @@ class UserTest < ActiveSupport::TestCase
     verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
 
     # invoke setup again with repo_name
-    response = User.setup user, openid_prefix, 'foo/testrepo'
+    response = user.setup(openid_prefix: openid_prefix,
+                          repo_name: 'foo/testrepo')
     resp_user = find_obj_in_resp response, 'User', nil
     verify_user resp_user, email
     assert_equal user.uuid, resp_user[:uuid], 'expected uuid not found'
@@ -551,7 +556,9 @@ class UserTest < ActiveSupport::TestCase
     # invoke setup again with a vm_uuid
     vm = VirtualMachine.create
 
-    response = User.setup user, openid_prefix, 'foo/testrepo', vm.uuid
+    response = user.setup(openid_prefix: openid_prefix,
+                          repo_name: 'foo/testrepo',
+                          vm_uuid: vm.uuid)
 
     resp_user = find_obj_in_resp response, 'User', nil
     verify_user resp_user, email