[arvados] created: 2.1.0-3214-gb7f373541
git repository hosting
git at public.arvados.org
Mon Dec 19 16:55:41 UTC 2022
at b7f373541e5b6e7489573040df5b106b786814b5 (commit)
commit b7f373541e5b6e7489573040df5b106b786814b5
Author: Tom Clegg <tom at curii.com>
Date: Mon Dec 19 11:31:00 2022 -0500
18693: Delete redundant login permission links on update/delete.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 57dde364e..e8ac51f99 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -53,12 +53,24 @@ class Link < ArvadosModel
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
+ if 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).
+ delete_all
+ elsif self.name == 'can_login' &&
+ self.properties.respond_to?(:has_key?) &&
+ self.properties.has_key?('username')
+ Link.where(link_class: 'permission',
+ tail_uuid: self.tail_uuid,
+ head_uuid: self.head_uuid,
+ name: 'can_login').
+ where('properties @> ?', SafeJSON.dump({'username' => self.properties['username']})).
+ where('uuid <> ?', self.uuid).
+ delete_all
+ end
end
def head_kind
diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index 626f470c0..5d36653a5 100644
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -118,4 +118,17 @@ class LinkTest < ActiveSupport::TestCase
Link.find_by_uuid(wlink).destroy
assert_empty Link.where(uuid: rlink)
end
+
+ test "updating login permission causes any conflicting links to be deleted" do
+ link1, link2 = create_overlapping_permissions(['can_login', 'can_login'], {properties: {username: 'foo1'}})
+ Link.find_by_uuid(link1).update_attributes!(properties: {'username' => 'foo2'})
+ Link.find_by_uuid(link2).update_attributes!(properties: {'username' => 'foo2'})
+ assert_empty Link.where(uuid: link1)
+ end
+
+ test "deleting login permission causes any conflicting links to be deleted" do
+ link1, link2 = create_overlapping_permissions(['can_login', 'can_login'], {properties: {username: 'foo1'}})
+ Link.find_by_uuid(link1).destroy
+ assert_empty Link.where(uuid: link2)
+ end
end
commit 676c3f036eb69e9d9a6450d2401617bc279ae0bb
Author: Tom Clegg <tom at curii.com>
Date: Sun Dec 18 20:19:47 2022 -0500
18693: Return existing instead of creating redundant can_login link.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at 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 19b6d439a..c9ebe0346 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -37,6 +37,19 @@ class Arvados::V1::LinksController < ApplicationController
return show
end
end
+ elsif resource_attrs[:link_class] == 'permission' &&
+ resource_attrs[:name] == 'can_login' &&
+ resource_attrs[:properties].respond_to?(:has_key?) &&
+ resource_attrs[:properties].has_key?(:username)
+ existing = Link.where(link_class: 'permission',
+ tail_uuid: resource_attrs[:tail_uuid],
+ head_uuid: resource_attrs[:head_uuid]).
+ where('properties @> ?', SafeJSON.dump({'username' => resource_attrs[:properties][:username]})).
+ first
+ if existing
+ @object = existing
+ return show
+ end
end
super
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index fab51d400..d2dce44f0 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -746,4 +746,53 @@ class PermissionsTest < ActionDispatch::IntegrationTest
assert_equal expect, link1.name
end
end
+
+ test "creating duplicate login permission returns existing link" do
+ link1 = act_as_system_user do
+ Link.create!({
+ link_class: "permission",
+ tail_uuid: users(:active).uuid,
+ head_uuid: virtual_machines(:testvm2).uuid,
+ name: "can_login",
+ properties: {"username": "foo1"}
+ })
+ end
+ link2 = act_as_system_user do
+ Link.create!({
+ link_class: "permission",
+ tail_uuid: users(:active).uuid,
+ head_uuid: virtual_machines(:testvm2).uuid,
+ name: "can_login",
+ properties: {"username": "foo2"}
+ })
+ end
+ link3 = act_as_system_user do
+ Link.create!({
+ link_class: "permission",
+ tail_uuid: users(:active).uuid,
+ head_uuid: virtual_machines(:testvm2).uuid,
+ name: "can_read",
+ })
+ end
+ post "/arvados/v1/links",
+ params: {
+ link: {
+ link_class: "permission",
+ tail_uuid: users(:active).uuid,
+ head_uuid: virtual_machines(:testvm2).uuid,
+ name: "can_login",
+ properties: {"username": "foo2"},
+ },
+ },
+ headers: auth(:admin)
+ assert_response :success
+ assert_equal link2.uuid, json_response["uuid"]
+ assert_equal link2.created_at.to_date, json_response["created_at"].to_date
+ assert_equal "can_login", json_response["name"]
+ assert_equal "foo2", json_response["properties"]["username"]
+ link1.reload
+ assert_equal "foo1", link1.properties["username"]
+ link2.reload
+ assert_equal "foo2", link2.properties["username"]
+ end
end
commit 3794eb59917f309eb69dbfebbe09c8b594e712cc
Author: Tom Clegg <tom at curii.com>
Date: Sun Dec 18 08:55:03 2022 -0500
18693: De-duplicate permission links on create/update/delete.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at 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 7716a3d5c..19b6d439a 100644
--- 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 83043a56d..57dde364e 100644
--- 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 65f5adc1d..fab51d400 100644
--- 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 c7d21bdc4..626f470c0 100644
--- 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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list