10557: Tidy up users#setup controller.

Simplify long conditional, and fix bug where admin asks for repo
"username/reponame" but "username/username/reponame" gets created.

This also fixes an unpredictable API: Previously, if params included
{user:{uuid:X,email:Y}}, the setup API would either create a new user
with uuid X and email Y, or set up an existing user (ignoring Y),
depending on whether X was the UUID of an existing user. Now, passing
a "user" hash like this always tries to create a new user with
uuid=X (if given) and email=Y, and returns an error if the given UUID
is already in use.

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 0e95042f68a428401868a67838ed409cd221da50..96dfaaddd300e5466050ed91eb87e233fdd02420 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -64,36 +64,19 @@ class Arvados::V1::UsersController < ApplicationController
 
   # create user object and all the needed links
   def setup
-    @object = nil
     if params[:uuid]
-      @object = User.find_by_uuid params[:uuid]
+      @object = User.find_by_uuid(params[:uuid])
       if !@object
         return render_404_if_no_object
       end
-      object_found = true
+    elsif !params[:user]
+      raise ArgumentError.new "Required uuid or user"
+    elsif !params[:user]['email']
+      raise ArgumentError.new "Require user email"
+    elsif !params[:openid_prefix]
+      raise ArgumentError.new "Required openid_prefix parameter is missing."
     else
-      if !params[:user]
-        raise ArgumentError.new "Required uuid or user"
-      else
-        if params[:user]['uuid']
-          @object = User.find_by_uuid params[:user]['uuid']
-          if @object
-            object_found = true
-          end
-        end
-
-        if !@object
-          if !params[:user]['email']
-            raise ArgumentError.new "Require user email"
-          end
-
-          if !params[:openid_prefix]
-            raise ArgumentError.new "Required openid_prefix parameter is missing."
-          end
-
-          @object = model_class.create! resource_attrs
-        end
-      end
+      @object = model_class.create! resource_attrs
     end
 
     # It's not always possible for the client to know the user's
@@ -106,8 +89,7 @@ class Arvados::V1::UsersController < ApplicationController
     elsif @object.username.nil?
       raise ArgumentError.
         new("cannot setup a repository because user has no username")
-    elsif object_found and
-        params[:repo_name].start_with?("#{@object.username}/")
+    elsif params[:repo_name].index("/")
       full_repo_name = params[:repo_name]
     else
       full_repo_name = "#{@object.username}/#{params[:repo_name]}"

diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 789242f6e02e9080790f2e37f52d8040df1f8ac1..da9c595e8d186189f9a68ef1cda91519e7a88a09 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -214,26 +214,6 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
         @vm_uuid, resp_obj['uuid'], 'arvados#virtualMachine', false, 'VirtualMachine'
   end
 
-  test "invoke setup with existing uuid in user, verify response" do
-    authorize_with :admin
-    inactive_user = users(:inactive)
-
-    post :setup, {
-      user: {uuid: inactive_user['uuid']},
-      openid_prefix: 'https://www.google.com/accounts/o8/id'
-    }
-
-    assert_response :success
-
-    response_items = JSON.parse(@response.body)['items']
-    resp_obj = find_obj_in_resp response_items, 'User', nil
-
-    assert_not_nil resp_obj['uuid'], 'expected uuid for the new user'
-    assert_equal inactive_user['uuid'], resp_obj['uuid']
-    assert_equal inactive_user['email'], resp_obj['email'],
-        'expecting inactive user email'
-  end
-
   test "invoke setup with existing uuid but different email, expect original email" do
     authorize_with :admin
     inactive_user = users(:inactive)

diff --git a/services/api/test/integration/users_test.rb b/services/api/test/integration/users_test.rb
index 38ac12267aaf8ec6b894835ae0f5876aff9e04d2..09f29b81c08dfba105a5dfa613cb6fb421caa9bb 100644 (file)
--- a/services/api/test/integration/users_test.rb
+++ b/services/api/test/integration/users_test.rb
@@ -57,8 +57,15 @@ class UsersTest < ActionDispatch::IntegrationTest
         email: "foo@example.com"
       }
     }, auth(:admin)
+    assert_response 422         # cannot create another user with same UUID
 
-    assert_response :success
+    # invoke setup on the same user
+    post "/arvados/v1/users/setup", {
+      repo_name: repo_name,
+      vm_uuid: virtual_machines(:testvm).uuid,
+      openid_prefix: 'https://www.google.com/accounts/o8/id',
+      uuid: 'zzzzz-tpzed-abcdefghijklmno',
+    }, auth(:admin)
 
     response_items = json_response['items']