18693: De-duplicate permission links on create/update/delete.

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

diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index 7716a3d5cffd9f6ac44d3bb691d06c98ddf7acf2..19b6d439a473b2da5897000c443ceadbb941b69e 100644 (file)
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -20,6 +20,25 @@ class Arvados::V1::LinksController < ApplicationController
 
     resource_attrs.delete :head_kind
     resource_attrs.delete :tail_kind
+
+    if resource_attrs[:link_class] == 'permission' && Link::PermLevel[resource_attrs[:name]]
+      existing = Link.where(link_class: 'permission',
+                            tail_uuid: resource_attrs[:tail_uuid],
+                            head_uuid: resource_attrs[:head_uuid],
+                            name: Link::PermLevel.keys).first
+      if existing
+        @object = existing
+        if Link::PermLevel[resource_attrs[:name]] > Link::PermLevel[existing.name]
+          # upgrade existing permission link to the requested level.
+          return update
+        else
+          # no-op: existing permission is already greater or equal to
+          # the newly requested permission.
+          return show
+        end
+      end
+    end
+
     super
   end
 

diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 83043a56d19026d32a8c3fa65dc839908f74ee86..57dde364e4834d368137cd1212fa9c6057e79a31 100644 (file)
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -14,9 +14,12 @@ class Link < ArvadosModel
   validate :name_links_are_obsolete
   validate :permission_to_attach_to_objects
   before_update :restrict_alter_permissions
+  before_update :apply_max_overlapping_permissions
+  after_update :delete_overlapping_permissions
   after_update :call_update_permissions
   after_create :call_update_permissions
   before_destroy :clear_permissions
+  after_destroy :delete_overlapping_permissions
   after_destroy :check_permissions
 
   api_accessible :user, extend: :common do |t|
@@ -29,6 +32,35 @@ class Link < ArvadosModel
     t.add :properties
   end
 
+  PermLevel = {
+    'can_read' => 0,
+    'can_write' => 1,
+    'can_manage' => 2,
+  }
+
+  def apply_max_overlapping_permissions
+    return if self.link_class != 'permission' || !PermLevel[self.name]
+    Link.where(link_class: 'permission',
+               tail_uuid: self.tail_uuid,
+               head_uuid: self.head_uuid,
+               name: PermLevel.keys).
+      where('uuid <> ?', self.uuid).each do |other|
+      if PermLevel[other.name] > PermLevel[self.name]
+        self.name = other.name
+      end
+    end
+  end
+
+  def delete_overlapping_permissions
+    return if self.link_class != 'permission'
+    Link.where(link_class: 'permission',
+               tail_uuid: self.tail_uuid,
+               head_uuid: self.head_uuid,
+               name: PermLevel.keys).
+      where('uuid <> ?', self.uuid).
+      delete_all
+  end
+
   def head_kind
     if k = ArvadosModel::resource_class_for_uuid(head_uuid)
       k.kind

diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index 65f5adc1d150b0914324ebde3357187214c71bc2..fab51d4003232b3a02fc227daf271d026557819b 100644 (file)
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -712,4 +712,38 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_empty json_response['manifest_text'], "empty collection manifest_text is not empty"
   end
+
+  [['can_write', 'can_read', 'can_write'],
+   ['can_manage', 'can_write', 'can_manage'],
+   ['can_manage', 'can_read', 'can_manage'],
+   ['can_read', 'can_write', 'can_write'],
+   ['can_read', 'can_manage', 'can_manage'],
+   ['can_write', 'can_manage', 'can_manage'],
+  ].each do |perm1, perm2, expect|
+    test "creating #{perm2} permission returns existing #{perm1} link as #{expect}" do
+      link1 = act_as_system_user do
+        Link.create!({
+                       link_class: "permission",
+                       tail_uuid: users(:active).uuid,
+                       head_uuid: collections(:baz_file).uuid,
+                       name: perm1,
+                     })
+      end
+      post "/arvados/v1/links",
+           params: {
+             link: {
+               link_class: "permission",
+               tail_uuid: users(:active).uuid,
+               head_uuid: collections(:baz_file).uuid,
+               name: perm2,
+             },
+           },
+           headers: auth(:admin)
+      assert_response :success
+      assert_equal link1.uuid, json_response["uuid"]
+      assert_equal expect, json_response["name"]
+      link1.reload
+      assert_equal expect, link1.name
+    end
+  end
 end

diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index c7d21bdc4da721d51f40c0cb235a15a8e3c3db96..626f470c02b33641459b06f109f31e04f4d267e2 100644 (file)
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -93,4 +93,29 @@ class LinkTest < ActiveSupport::TestCase
     refute new_active_link_valid?(tail_uuid: groups(:public).uuid,
                                   head_uuid: collections(:w_a_z_file_version_1).uuid)
   end
+
+  def create_overlapping_permissions(names=[], attrs={})
+    names.map do |name|
+      link = Link.create!({
+                            link_class: "tmp",
+                            tail_uuid: users(:active).uuid,
+                            head_uuid: collections(:baz_file).uuid,
+                            name: name,
+                          }.merge(attrs).merge({name: name}))
+      ActiveRecord::Base.connection.execute "update links set link_class='permission' where uuid='#{link.uuid}'"
+      link.uuid
+    end
+  end
+
+  test "updating permission causes any conflicting links to be deleted" do
+    link1, link2 = create_overlapping_permissions(['can_read', 'can_manage'])
+    Link.find_by_uuid(link2).update_attributes!(name: 'can_write')
+    assert_empty Link.where(uuid: link1)
+  end
+
+  test "deleting permission causes any conflicting links to be deleted" do
+    rlink, wlink = create_overlapping_permissions(['can_read', 'can_write'])
+    Link.find_by_uuid(wlink).destroy
+    assert_empty Link.where(uuid: rlink)
+  end
 end