[ARVADOS] created: c422cbe72db43dc133a45cc8d7da6ea0f87967ca
git at public.curoverse.com
git at public.curoverse.com
Wed Oct 15 14:05:13 EDT 2014
at c422cbe72db43dc133a45cc8d7da6ea0f87967ca (commit)
commit c422cbe72db43dc133a45cc8d7da6ea0f87967ca
Author: Brett Smith <brett at curoverse.com>
Date: Wed Oct 15 14:04:12 2014 -0400
4189: Workbench lets users with write permission rename objects in projects.
The interesting part of this commit is the fact that
object_attribute_editable? looks at Group.writable_by, rather than the
old code's my_projects. I refactored this method out of the original
code to make it unit testable.
There are also some tests in here that I wrote while I was tracking
down the problem. They seem like a useful addition.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 4f5d8fd..a454822 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -337,6 +337,15 @@ class ApplicationController < ActionController::Base
Thread.current[:user]
end
+ helper_method :object_attribute_editable?
+ def object_attribute_editable?(object, attr)
+ object and object.attribute_editable?(attr, :ever) and
+ (object.editable? or
+ (object.owner_uuid == current_user.uuid) or
+ (Group.find(object.owner_uuid).writable_by
+ .include?(current_user.uuid) rescue false))
+ end
+
def model_class
controller_name.classify.constantize
end
diff --git a/apps/workbench/app/helpers/application_helper.rb b/apps/workbench/app/helpers/application_helper.rb
index d343249..d7578ca 100644
--- a/apps/workbench/app/helpers/application_helper.rb
+++ b/apps/workbench/app/helpers/application_helper.rb
@@ -150,9 +150,7 @@ module ApplicationHelper
def render_editable_attribute(object, attr, attrvalue=nil, htmloptions={})
attrvalue = object.send(attr) if attrvalue.nil?
- if !object.attribute_editable?(attr, :ever) or
- (!object.editable? and
- !object.owner_uuid.in?(my_projects.collect(&:uuid)))
+ if not object_attribute_editable?(object, attr)
if attrvalue && attrvalue.length > 0
return render_attribute_as_textile( object, attr, attrvalue, false )
else
@@ -241,10 +239,7 @@ module ApplicationHelper
preconfigured_search_str = value_info[:search_for]
end
- if !object or
- !object.attribute_editable?(attr, :ever) or
- (!object.editable? and
- !object.owner_uuid.in?(my_projects.collect(&:uuid)))
+ if not object_attribute_editable?(object, attr)
return link_to_if_arvados_object attrvalue
end
diff --git a/apps/workbench/test/functional/application_controller_test.rb b/apps/workbench/test/functional/application_controller_test.rb
index c282802..ec15518 100644
--- a/apps/workbench/test/functional/application_controller_test.rb
+++ b/apps/workbench/test/functional/application_controller_test.rb
@@ -321,4 +321,33 @@ class ApplicationControllerTest < ActionController::TestCase
Rails.configuration.arvados_v1_base = orig_api_server
end
end
+
+ def user_can_edit_collection_name?(token_sym, collection_key)
+ use_token token_sym
+ coll = Collection.find(api_fixture("collections")[collection_key]["uuid"])
+ @controller.object_attribute_editable?(coll, "name")
+ end
+
+ test "admin can edit object attributes" do
+ assert(user_can_edit_collection_name?(:admin, "foo_file"),
+ "admin not allowed to edit collection name")
+ end
+
+ test "project owner can edit object attributes" do
+ assert(user_can_edit_collection_name?(:active,
+ "foo_collection_in_aproject"),
+ "project owner not allowed to edit collection name")
+ end
+
+ test "project admin can edit object attributes" do
+ assert(user_can_edit_collection_name?(:subproject_admin,
+ "baz_file_in_asubproject"),
+ "project admin not allowed to edit collection name")
+ end
+
+ test "project viewer cannot edit object attributes" do
+ refute(user_can_edit_collection_name?(:project_viewer,
+ "foo_collection_in_aproject"),
+ "project viewer allowed to edit collection name")
+ end
end
diff --git a/apps/workbench/test/functional/projects_controller_test.rb b/apps/workbench/test/functional/projects_controller_test.rb
index d76430c..8eb0cdc 100644
--- a/apps/workbench/test/functional/projects_controller_test.rb
+++ b/apps/workbench/test/functional/projects_controller_test.rb
@@ -93,6 +93,23 @@ class ProjectsControllerTest < ActionController::TestCase
refute user_can_manage(:project_viewer, "asubproject")
end
+ test "subproject_admin can_manage asubproject" do
+ assert user_can_manage(:subproject_admin, "asubproject")
+ end
+
+ test "project admin can remove items from the project" do
+ coll_key = "collection_to_remove_from_subproject"
+ coll_uuid = api_fixture("collections")[coll_key]["uuid"]
+ delete(:remove_item,
+ { id: api_fixture("groups")["asubproject"]["uuid"],
+ item_uuid: coll_uuid,
+ format: "js" },
+ session_for(:subproject_admin))
+ assert_response :success
+ assert_match(/\b#{coll_uuid}\b/, @response.body,
+ "removed object not named in response")
+ end
+
test 'projects#show tab infinite scroll partial obeys limit' do
get_contents_rows(limit: 1, filters: [['uuid','is_a',['arvados#job']]])
assert_response :success
diff --git a/apps/workbench/test/unit/group_test.rb b/apps/workbench/test/unit/group_test.rb
index 3f5cebc..4a4530c 100644
--- a/apps/workbench/test/unit/group_test.rb
+++ b/apps/workbench/test/unit/group_test.rb
@@ -25,4 +25,16 @@ class GroupTest < ActiveSupport::TestCase
assert_nil user.owner_uuid
end
end
+
+ test "project editable by its admin" do
+ use_token :subproject_admin
+ project = Group.find(api_fixture("groups")["asubproject"]["uuid"])
+ assert(project.editable?, "project not editable by admin")
+ end
+
+ test "project not editable by reader" do
+ use_token :project_viewer
+ project = Group.find(api_fixture("groups")["aproject"]["uuid"])
+ refute(project.editable?, "project editable by reader")
+ end
end
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 32560c3..8f7b125 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -49,6 +49,12 @@ project_viewer:
api_token: projectviewertoken1234567890abcdefghijklmnopqrstuv
expires_at: 2038-01-01 00:00:00
+subproject_admin:
+ api_client: untrusted
+ user: subproject_admin
+ api_token: subprojectadmintoken1234567890abcdefghijklmnopqrst
+ expires_at: 2038-01-01 00:00:00
+
admin_vm:
api_client: untrusted
user: admin
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 2fb235c..e1e568a 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -257,3 +257,12 @@ collection_owned_by_foo:
owner_uuid: zzzzz-tpzed-81hsbo6mk8nl05c
created_at: 2014-02-03T17:22:54Z
name: collection_owned_by_foo
+
+collection_to_remove_from_subproject:
+ # The Workbench tests remove this from subproject.
+ uuid: zzzzz-4zz18-subprojgonecoll
+ portable_data_hash: 2386ca6e3fffd4be5e197a72c6c80fb2+51
+ manifest_text: ". 8258b505536a9ab47baa2f4281cb932a+9 0:9:missingno\n"
+ owner_uuid: zzzzz-j7d0g-axqo7eu9pwvna1x
+ created_at: 2014-10-15T10:45:00
+ name: Collection to remove from subproject
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 9e13971..1771fcc 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -377,6 +377,20 @@ project_viewer_can_read_project:
head_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
properties: {}
+subproject_admin_can_manage_subproject:
+ uuid: zzzzz-o0j2j-subprojadminlnk
+ owner_uuid: zzzzz-tpzed-000000000000000
+ created_at: 2014-10-15 10:00:00 -0000
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ modified_at: 2014-10-15 10:00:00 -0000
+ updated_at: 2014-10-15 10:00:00 -0000
+ tail_uuid: zzzzz-tpzed-subprojectadmin
+ link_class: permission
+ name: can_manage
+ head_uuid: zzzzz-j7d0g-axqo7eu9pwvna1x
+ properties: {}
+
foo_collection_tag:
uuid: zzzzz-o0j2j-eedahfaho8aphiv
owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index 2dd72ab..9cd2db8 100644
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -85,6 +85,20 @@ future_project_user:
organization: example.com
role: Computational biologist
+subproject_admin:
+ owner_uuid: zzzzz-tpzed-000000000000000
+ uuid: zzzzz-tpzed-subprojectadmin
+ email: subproject-admin at arvados.local
+ first_name: Subproject
+ last_name: Admin
+ identity_url: https://subproject-admin.openid.local
+ is_active: true
+ is_admin: false
+ prefs:
+ profile:
+ organization: example.com
+ role: Computational biologist
+
spectator:
owner_uuid: zzzzz-tpzed-000000000000000
uuid: zzzzz-tpzed-l1s2piq4t4mps8r
commit 330e94979e169436bbc82517c651084d0687bb2c
Author: Brett Smith <brett at curoverse.com>
Date: Wed Oct 15 13:29:48 2014 -0400
4189: Workbench tests clear more state during teardown.
I'm working on some new tests that pick up on stale state without
these changes.
diff --git a/apps/workbench/test/test_helper.rb b/apps/workbench/test/test_helper.rb
index 5253676..f99144a 100644
--- a/apps/workbench/test/test_helper.rb
+++ b/apps/workbench/test/test_helper.rb
@@ -38,7 +38,9 @@ class ActiveSupport::TestCase
teardown do
Thread.current[:arvados_api_token] = nil
+ Thread.current[:user] = nil
Thread.current[:reader_tokens] = nil
+ Rails.cache.clear
# Restore configuration settings changed during tests
$application_config.each do |k,v|
if k.match /^[^.]*$/
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list