[ARVADOS] updated: 1.1.2-16-g680ffd6

[Git user]

Summary of changes:
 services/api/app/models/user.rb                    |  3 ++
 .../functional/arvados/v1/users_controller_test.rb | 39 ++++++++++-----------
 services/api/test/unit/user_test.rb                | 40 ++++++++++++++--------
 3 files changed, 47 insertions(+), 35 deletions(-)

       via  680ffd64dac92aec8ad94454334db9ae69b95b56 (commit)
      from  086b27a27c844178ee52a9c4186d970689147628 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 680ffd64dac92aec8ad94454334db9ae69b95b56
Author: Tom Clegg <tclegg at veritasgenetics.com>
Date:   Mon Jan 8 13:30:55 2018 -0500

    12702: Disallow updating sys/anon UUIDs. Test updating current user.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg at veritasgenetics.com>

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 212047f..9209411 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -258,6 +258,9 @@ class User < ArvadosModel
     if !current_user.andand.is_admin
       raise PermissionDeniedError
     end
+    if uuid == system_user_uuid || uuid == anonymous_user_uuid
+      raise "update_uuid cannot update system accounts"
+    end
     if self.class != self.class.resource_class_for_uuid(new_uuid)
       raise "invalid new_uuid #{new_uuid.inspect}"
     end
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 176104e..a506486 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -794,26 +794,25 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
                     "user's writable_by should include its owner_uuid")
   end
 
-  test 'update_uuid as admin' do
-    authorize_with :admin
-    orig_uuid = users(:active).uuid
-    post :update_uuid, {
-           id: orig_uuid,
-           new_uuid: 'zbbbb-tpzed-abcde12345abcde',
-         }
-    assert_response :success
-    assert_empty User.where(uuid: orig_uuid)
-  end
-
-  test 'update_uuid as non-admin' do
-    authorize_with :active
-    orig_uuid = users(:active).uuid
-    post :update_uuid, {
-           id: orig_uuid,
-           new_uuid: 'zbbbb-tpzed-abcde12345abcde',
-         }
-    assert_response 403
-    assert_not_empty User.where(uuid: orig_uuid)
+  [
+    [:admin, true],
+    [:active, false],
+  ].each do |auth_user, expect_success|
+    test "update_uuid as #{auth_user}" do
+      authorize_with auth_user
+      orig_uuid = users(:active).uuid
+      post :update_uuid, {
+             id: orig_uuid,
+             new_uuid: 'zbbbb-tpzed-abcde12345abcde',
+           }
+      if expect_success
+        assert_response :success
+        assert_empty User.where(uuid: orig_uuid)
+      else
+        assert_response 403
+        assert_not_empty User.where(uuid: orig_uuid)
+      end
+    end
   end
 
 
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 3c85bd1..72beca6 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -722,12 +722,14 @@ class UserTest < ActiveSupport::TestCase
   end
 
   [
-    'zzzzz-borkd-abcde12345abcde',
-    'zzzzz-j7d0g-abcde12345abcde',
-    'zzzzz-tpzed-borkd',
-  ].each do |new_uuid|
-    test "update_uuid to invalid uuid #{new_uuid}" do
-      u = users(:active)
+    [:active, 'zzzzz-borkd-abcde12345abcde'],
+    [:active, 'zzzzz-j7d0g-abcde12345abcde'],
+    [:active, 'zzzzz-tpzed-borkd'],
+    [:system_user, 'zzzzz-tpzed-abcde12345abcde'],
+    [:anonymous, 'zzzzz-tpzed-abcde12345abcde'],
+  ].each do |fixture, new_uuid|
+    test "disallow update_uuid #{fixture} -> #{new_uuid}" do
+      u = users(fixture)
       orig_uuid = u.uuid
       act_as_system_user do
         assert_raises do
@@ -736,7 +738,9 @@ class UserTest < ActiveSupport::TestCase
       end
       # "Successfully aborted orig->new" outcome looks the same as
       # "successfully updated new->orig".
-      assert_update_success(old_uuid: new_uuid, new_uuid: orig_uuid)
+      assert_update_success(old_uuid: new_uuid,
+                            new_uuid: orig_uuid,
+                            expect_owned_objects: fixture == :active)
     end
   end
 
@@ -766,20 +770,24 @@ class UserTest < ActiveSupport::TestCase
   end
 
   [
-    'zbbbb-tpzed-abcde12345abcde',
-    'zzzzz-tpzed-abcde12345abcde',
-  ].each do |new_uuid|
-    test "update_uuid to unused uuid #{new_uuid}" do
-      u = users(:active)
+    [:active, 'zbbbb-tpzed-abcde12345abcde'],
+    [:active, 'zzzzz-tpzed-abcde12345abcde'],
+    [:admin, 'zbbbb-tpzed-abcde12345abcde'],
+    [:admin, 'zzzzz-tpzed-abcde12345abcde'],
+  ].each do |fixture, new_uuid|
+    test "update_uuid #{fixture} to unused uuid #{new_uuid}" do
+      u = users(fixture)
       orig_uuid = u.uuid
       act_as_system_user do
         u.update_uuid(new_uuid: new_uuid)
       end
-      assert_update_success(old_uuid: orig_uuid, new_uuid: new_uuid)
+      assert_update_success(old_uuid: orig_uuid,
+                            new_uuid: new_uuid,
+                            expect_owned_objects: fixture == :active)
     end
   end
 
-  def assert_update_success(old_uuid:, new_uuid:)
+  def assert_update_success(old_uuid:, new_uuid:, expect_owned_objects: true)
     [[User, :uuid],
      [Link, :head_uuid],
      [Link, :tail_uuid],
@@ -787,7 +795,9 @@ class UserTest < ActiveSupport::TestCase
      [Collection, :owner_uuid],
     ].each do |klass, attr|
       assert_empty klass.where(attr => old_uuid)
-      assert_not_empty klass.where(attr => new_uuid)
+      if klass == User || expect_owned_objects
+        assert_not_empty klass.where(attr => new_uuid)
+      end
     end
   end
 end

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


hooks/post-receive
--