4253: Merge Repository permission requirements of #4253 and #5416.

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 1fe58088483fad98e34531391bd2b21a5bf91deb..02e9386bfef8a8c08046e137a40e627b189e6c25 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -308,8 +308,13 @@ class ArvadosModel < ActiveRecord::Base
     # Verify "write" permission on new owner
     # default fail unless one of:
     # current_user is this object
-    # current user can_write new owner
-    unless current_user == self or current_user.can? write: owner_uuid
+    # current user can_write new owner, or this object if owner unchanged
+    if new_record? or owner_uuid_changed? or is_a?(ApiClientAuthorization)
+      write_target = owner_uuid
+    else
+      write_target = uuid
+    end
+    unless current_user == self or current_user.can? write: write_target
       logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{uuid} but does not have permission to write new owner_uuid #{owner_uuid}"
       errors.add :owner_uuid, "cannot be changed without write permission on new owner"
       raise PermissionDeniedError

diff --git a/services/api/app/models/repository.rb b/services/api/app/models/repository.rb
index 5bc467c9204caeb445b55131941f5fccd723fb10..0cab4dcf8760641b95f7eb8f3c5dfa2b05d11018 100644 (file)
--- a/services/api/app/models/repository.rb
+++ b/services/api/app/models/repository.rb
@@ -47,14 +47,15 @@ class Repository < ArvadosModel
   protected
 
   def permission_to_update
-    return false if not current_user
-    return true if current_user.is_admin
-    # For normal objects, this is a way to check whether you have
-    # write permission. Repositories should be brought closer to the
-    # normal permission model during #4253. Meanwhile, we'll
-    # special-case this so arv-git-httpd can detect write permission:
-    return super if changed_attributes.keys - ['modified_at', 'updated_at'] == []
-    false
+    if not super
+      false
+    elsif current_user.is_admin
+      true
+    elsif name_changed?
+      current_user.uuid == owner_uuid
+    else
+      true
+    end
   end
 
   def owner

diff --git a/services/api/test/unit/repository_test.rb b/services/api/test/unit/repository_test.rb
index a6b0be5f0a4eeff024136422fff08d3d6f182533..5acef1bb1e769ca0d1cbc7f6a80d124286b33aa8 100644 (file)
--- a/services/api/test/unit/repository_test.rb
+++ b/services/api/test/unit/repository_test.rb
@@ -238,19 +238,6 @@ class RepositoryTest < ActiveSupport::TestCase
     end
   end
 
-  test 'write permission not sufficient for changing name' do
-    act_as_user users(:active) do
-      r = repositories(:foo)
-      name_was = r.name
-      r.name = 'newname'
-      assert_raises ArvadosModel::PermissionDeniedError do
-        r.save!
-      end
-      r.reload
-      assert_equal name_was, r.name
-    end
-  end
-
   test 'write permission necessary for changing modified_at' do
     act_as_user users(:spectator) do
       r = repositories(:foo)