From: Peter Amstutz <peter.amstutz@curii.com>
Date: Wed, 13 May 2020 18:52:37 +0000 (-0400)
Subject: 16007: All the API tests pass!!!
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/bc35acd3025ff29f1b2246d470505eea4e457cb0?ds=sidebyside

16007: All the API tests pass!!!

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>
---

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index d9ef342b37..1d69300b60 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -18,14 +18,14 @@ class Group < ArvadosModel
 
   validate :ensure_filesystem_compatible_name
   before_create :assign_name
-  after_create :update_permissions
+  after_create :after_ownership_change
   after_create :update_trash
 
-  after_update :update_permissions, :if => :owner_uuid_changed?
-  after_update :update_trash
-
-  after_destroy :clear_permissions_and_trash
+  before_update :before_ownership_change
+  after_update :after_ownership_change
 
+  after_update :update_trash
+  before_destroy :clear_permissions_and_trash
 
   api_accessible :user, extend: :common do |t|
     t.add :name
@@ -75,12 +75,21 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
     end
   end
 
-  def update_permissions
-    User.update_permissions self.owner_uuid, self.uuid, 3
+  def before_ownership_change
+    if owner_uuid_changed? and !self.owner_uuid_was.nil?
+      MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
+      User.update_permissions self.owner_uuid, self.uuid, 0
+    end
+  end
+
+  def after_ownership_change
+    if owner_uuid_changed?
+      User.update_permissions self.owner_uuid, self.uuid, 3
+    end
   end
 
   def clear_permissions_and_trash
-    User.update_permissions self.owner_uuid, self.uuid, 0
+    MaterializedPermission.where(target_uuid: uuid).delete_all
     ActiveRecord::Base.connection.exec_query %{
 delete from trashed_groups where group_uuid=$1
 }, "Group.clear_trash", [[nil, self.uuid]]
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 9e52c05e64..2aadb374d2 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -16,6 +16,7 @@ class Link < ArvadosModel
   before_update :permission_to_attach_to_objects
   after_update :update_permissions
   after_create :update_permissions
+  before_destroy :clear_permissions
   after_destroy :clear_permissions
 
   api_accessible :user, extend: :common do |t|
@@ -79,7 +80,13 @@ class Link < ArvadosModel
 
   def clear_permissions
     if self.link_class == 'permission'
-      User.update_permissions tail_uuid, head_uuid, 0
+      User.update_permissions tail_uuid, head_uuid, 0, false
+    end
+  end
+
+  def check_permissions
+    if self.link_class == 'permission'
+      User.update_permissions tail_uuid, head_uuid, 0, true
     end
   end
 
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 6dce3a42e0..4df2039d83 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -27,11 +27,12 @@ class User < ArvadosModel
     user.username.nil? and user.username_changed?
   }
   before_update :setup_on_activate
+
   before_create :check_auto_admin
   before_create :set_initial_username, :if => Proc.new { |user|
     user.username.nil? and user.email
   }
-  after_create :update_permissions
+  after_create :after_ownership_change
   after_create :setup_on_activate
   after_create :add_system_group_permission_link
   after_create :auto_setup_new_user, :if => Proc.new { |user|
@@ -40,14 +41,16 @@ class User < ArvadosModel
     (user.uuid != anonymous_user_uuid)
   }
   after_create :send_admin_notifications
-  after_update :update_permissions, :if => :owner_uuid_changed?
+
+  before_update :before_ownership_change
+  after_update :after_ownership_change
   after_update :send_profile_created_notification
   after_update :sync_repository_names, :if => Proc.new { |user|
     (user.uuid != system_user_uuid) and
     user.username_changed? and
     (not user.username_was.nil?)
   }
-
+  after_destroy :clear_permissions
 
   has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
   has_many :repositories, foreign_key: :owner_uuid, primary_key: :uuid
@@ -137,13 +140,42 @@ SELECT 1 FROM #{PERMISSION_VIEW}
     true
   end
 
-  def update_permissions
+  def before_ownership_change
+    if owner_uuid_changed? and !self.owner_uuid_was.nil?
+      MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
+      User.update_permissions self.owner_uuid_was, self.uuid, 0, false
+    end
+  end
+
+  def after_ownership_change
 
 #       puts "Update permissions for #{uuid}"
 #     User.printdump %{
 # select * from materialized_permissions where user_uuid='#{uuid}'
 # }
-#     puts "---"
+    puts "-1- #{uuid_changed?} and #{uuid} and #{uuid_was}"
+    puts "-2- #{owner_uuid_changed?} and #{owner_uuid} and #{owner_uuid_was}"
+
+    unless owner_uuid_changed?
+      return
+    end
+
+#     if !uuid_was.nil? and uuid_changed?
+#       puts "Cleaning house #{uuid_was}"
+#     ActiveRecord::Base.connection.exec_query %{
+# update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2
+# },
+#                                              'Change user uuid',
+#                                              [[nil, uuid],
+#                                               [nil, uuid_was]]
+#     ActiveRecord::Base.connection.exec_query %{
+# update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
+# },
+#                                              'Change user uuid',
+#                                              [[nil, uuid],
+#                                               [nil, uuid_was]]
+#     end
+
     User.update_permissions self.owner_uuid, self.uuid, 3
 
 #   puts "post-update"
@@ -153,6 +185,10 @@ SELECT 1 FROM #{PERMISSION_VIEW}
 #    puts "<<<"
   end
 
+  def clear_permissions
+    MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all
+  end
+
   def self.printdump qr
     q1 = ActiveRecord::Base.connection.exec_query qr
     q1.each do |r|
@@ -169,7 +205,7 @@ from search_permission_graph('#{uuid}', 3) as g
 }
   end
 
-  def self.update_permissions perm_origin_uuid, starting_uuid, perm_level
+  def self.update_permissions perm_origin_uuid, starting_uuid, perm_level, check=true
     # Update a subset of the permission graph
     # perm_level is the inherited permission
     # perm_level is a number from 0-3
@@ -228,8 +264,7 @@ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
            should_traverse_owned(ps.target_uuid, ps.val)
       from edges, lateral search_permission_graph(edges.head_uuid, edges.val) as ps
       where (not (edges.tail_uuid = '#{perm_origin_uuid}' and
-                  edges.head_uuid = '#{starting_uuid}' and
-                  edges.val = #{perm_level})) and
+                  edges.head_uuid = '#{starting_uuid}')) and
             edges.tail_uuid not in (select target_uuid from perm_from_start) and
             edges.head_uuid in (select target_uuid from perm_from_start)),
 
@@ -242,7 +277,8 @@ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
   user_identity_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
     select users.uuid as perm_origin_uuid, ps.target_uuid, ps.val, ps.traverse_owned
       from users, lateral search_permission_graph(users.uuid, 3) as ps
-      where users.owner_uuid not in (select target_uuid from partial_perms where traverse_owned) and
+      where (users.owner_uuid not in (select target_uuid from partial_perms) or
+             users.owner_uuid = users.uuid) and
       users.uuid in (select target_uuid from partial_perms)
   ),
 
@@ -284,59 +320,62 @@ as select * from compute_permission_subgraph($1, $2, $3)
     ActiveRecord::Base.connection.exec_query %{
 delete from materialized_permissions where
   target_uuid in (select target_uuid from #{temptable_perms}) and
-  (user_uuid not in (select user_uuid from #{temptable_perms} where target_uuid=materialized_permissions.target_uuid)
-   or user_uuid in (select user_uuid from #{temptable_perms} where target_uuid=materialized_permissions.target_uuid and perm_level=0))
+  not exists (select 1 from #{temptable_perms}
+              where target_uuid=materialized_permissions.target_uuid and
+                    user_uuid=materialized_permissions.user_uuid and
+                    val>0)
 }
 
     ActiveRecord::Base.connection.exec_query %{
 insert into materialized_permissions (user_uuid, target_uuid, perm_level, traverse_owned)
-  select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms}
+  select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
 on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
 }
 
     # for testing only - make a copy of the table and compare it to the one generated
     # using a full permission recompute
 
-#      puts "dump--"
-#      User.printdump %{
-#  select * from users
-#      }
-#      User.printdump %{
-#  select * from groups
-#      }
-#      User.printdump %{
-# select * from search_permission_graph('zzzzz-tpzed-000000000000000', 3)
-#      }
-#      puts "--"
-
+#     puts "dump--"
+#     User.printdump %{
+# select owner_uuid, uuid from users order by uuid
+#     }
+#     User.printdump %{
+# select * from groups
+#     }
+#     User.printdump %{
+#select * from search_permission_graph('zzzzz-tpzed-000000000000000', 3)
+#     }
+#     puts "--"
 
-    q1 = ActiveRecord::Base.connection.exec_query %{
+    if check
+      q1 = ActiveRecord::Base.connection.exec_query %{
 select user_uuid, target_uuid, perm_level, traverse_owned from materialized_permissions
 order by user_uuid, target_uuid
 }
 
-    q2 = ActiveRecord::Base.connection.exec_query %{
+      q2 = ActiveRecord::Base.connection.exec_query %{
 select users.uuid as user_uuid, g.target_uuid, g.val as perm_level, g.traverse_owned
 from users, lateral search_permission_graph(users.uuid, 3) as g where g.val > 0
 order by users.uuid, target_uuid
 }
 
-    if q1.count != q2.count
-      puts "Didn't match incremental+: #{q1.count} != full refresh-: #{q2.count}"
-    end
+      if q1.count != q2.count
+        puts "Didn't match incremental+: #{q1.count} != full refresh-: #{q2.count}"
+      end
 
-    if q1.count > q2.count
-      q1.each_with_index do |r, i|
-        if r != q2[i]
-          puts "+#{r}\n-#{q2[i]}"
-          raise "Didn't match"
+      if q1.count > q2.count
+        q1.each_with_index do |r, i|
+          if r != q2[i]
+            puts "+#{r}\n-#{q2[i]}"
+            raise "Didn't match"
+          end
         end
-      end
-    else
-      q2.each_with_index do |r, i|
-        if r != q1[i]
-          puts "+#{q1[i]}\n-#{r}"
-          raise "Didn't match"
+      else
+        q2.each_with_index do |r, i|
+          if r != q1[i]
+            puts "+#{q1[i]}\n-#{r}"
+            raise "Didn't match"
+          end
         end
       end
     end
@@ -496,6 +535,18 @@ SELECT target_uuid, perm_level
       self.uuid = new_uuid
       save!(validate: false)
       change_all_uuid_refs(old_uuid: old_uuid, new_uuid: new_uuid)
+    ActiveRecord::Base.connection.exec_query %{
+update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2
+},
+                                             'Change user uuid',
+                                             [[nil, new_uuid],
+                                              [nil, old_uuid]]
+      ActiveRecord::Base.connection.exec_query %{
+update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
+},
+                                             'Change target uuid',
+                                             [[nil, new_uuid],
+                                              [nil, old_uuid]]
     end
   end
 
diff --git a/services/api/db/migrate/20200501150153_permission_table.rb b/services/api/db/migrate/20200501150153_permission_table.rb
index d3afd4a749..36d618ea2f 100644
--- a/services/api/db/migrate/20200501150153_permission_table.rb
+++ b/services/api/db/migrate/20200501150153_permission_table.rb
@@ -172,8 +172,7 @@ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
            should_traverse_owned(ps.target_uuid, ps.val)
       from edges, lateral search_permission_graph(edges.head_uuid, edges.val) as ps
       where (not (edges.tail_uuid = perm_origin_uuid and
-                 edges.head_uuid = starting_uuid and
-                 edges.val = starting_perm)) and
+                 edges.head_uuid = starting_uuid)) and
             edges.tail_uuid not in (select target_uuid from perm_from_start) and
             edges.head_uuid in (select target_uuid from perm_from_start)),
 
@@ -186,7 +185,8 @@ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
   user_identity_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
     select users.uuid as perm_origin_uuid, ps.target_uuid, ps.val, ps.traverse_owned
       from users, lateral search_permission_graph(users.uuid, 3) as ps
-      where users.owner_uuid not in (select target_uuid from partial_perms where traverse_owned) and
+      where (users.owner_uuid not in (select target_uuid from partial_perms) or
+             users.owner_uuid = users.uuid) and
       users.uuid in (select target_uuid from partial_perms)
   ),
 
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 92a1ced528..22c999ecd7 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -60,6 +60,7 @@ testusergroup_admins:
   uuid: zzzzz-j7d0g-48foin4vonvc2at
   owner_uuid: zzzzz-tpzed-000000000000000
   name: Administrators of a subset of users
+  group_class: role
 
 aproject:
   uuid: zzzzz-j7d0g-v955i6s2oi1cbso
@@ -143,6 +144,7 @@ active_user_has_can_manage:
   uuid: zzzzz-j7d0g-ptt1ou6a9lxrv07
   owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
   name: Active user has can_manage
+  group_class: project
 
 # Group for testing granting permission between users who share a group.
 group_for_sharing_tests:
@@ -343,4 +345,4 @@ trashed_on_next_sweep:
   trash_at: 2001-01-01T00:00:00Z
   delete_at: 2038-03-01T00:00:00Z
   is_trashed: false
-  modified_at: 2001-01-01T00:00:00Z
\ No newline at end of file
+  modified_at: 2001-01-01T00:00:00Z
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index 528c6d253f..3280b1fc36 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -70,8 +70,13 @@ class OwnerTest < ActiveSupport::TestCase
              "new #{o_class} should really be in DB")
       old_uuid = o.uuid
       new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9])
-      assert(o.update_attributes(uuid: new_uuid),
-             "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}")
+      puts "//Changing//"
+      if o.respond_to? :update_uuid
+        o.update_uuid(new_uuid: new_uuid)
+      else
+        assert(o.update_attributes(uuid: new_uuid),
+               "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}")
+      end
       assert_equal(false, o_class.where(uuid: old_uuid).any?,
                    "#{old_uuid} should disappear when renamed to #{new_uuid}")
     end
@@ -116,8 +121,8 @@ class OwnerTest < ActiveSupport::TestCase
                  "setting owner to self should work")
     old_uuid = o.uuid
     new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9])
-    assert(o.update_attributes(uuid: new_uuid),
-           "should change uuid of User that owns self")
+    o.update_uuid(new_uuid: new_uuid)
+    o = User.find_by_uuid(new_uuid)
     assert_equal(false, User.where(uuid: old_uuid).any?,
                  "#{old_uuid} should not be in DB after deleting")
     assert_equal(true, User.where(uuid: new_uuid).any?,
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 260795c12f..5d25361eda 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -165,7 +165,12 @@ class UserTest < ActiveSupport::TestCase
 
       if auto_admin_first_user_config
         # This test requires no admin users exist (except for the system user)
-        users(:admin).delete
+        act_as_system_user do
+          users(:admin).update_attributes!(is_admin: false)
+        end
+        # need to manually call clear_permissions because we used 'delete' instead of 'destory'
+        # we use delete here because 'destroy' will throw an error
+        #users(:admin).clear_permissions
         @all_users = User.where("uuid not like '%-000000000000000'").where(:is_admin => true)
         assert_equal 0, @all_users.count, "No admin users should exist (except for the system user)"
       end