From ef3a7bc786d108f597edfa3f63a1d06752002fd6 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Sat, 23 Aug 2014 21:14:23 -0400 Subject: [PATCH] 3660: Show add/run buttons if project is writable. Hide move/delete buttons if move is impossible. --- apps/workbench/app/models/group.rb | 6 --- .../app/views/projects/show.html.erb | 2 + services/api/app/models/arvados_model.rb | 22 +++++--- .../arvados/v1/groups_controller_test.rb | 5 +- services/api/test/unit/permission_test.rb | 54 +++++++++++++++++++ 5 files changed, 74 insertions(+), 15 deletions(-) diff --git a/apps/workbench/app/models/group.rb b/apps/workbench/app/models/group.rb index 558c587a1c..b7ffd638bb 100644 --- a/apps/workbench/app/models/group.rb +++ b/apps/workbench/app/models/group.rb @@ -24,10 +24,4 @@ class Group < ArvadosBase def class_for_display group_class == 'project' ? 'Project' : super end - - def editable? - respond_to?(:writable_by) and - writable_by and - writable_by.index(current_user.uuid) - end end diff --git a/apps/workbench/app/views/projects/show.html.erb b/apps/workbench/app/views/projects/show.html.erb index c221ca1d16..e3c9484b3e 100644 --- a/apps/workbench/app/views/projects/show.html.erb +++ b/apps/workbench/app/views/projects/show.html.erb @@ -39,6 +39,8 @@ Add a subproject <% end %> + <% end %> + <% if @object.owner_uuid == current_user.uuid or (Group.find(@object.owner_uuid).writable_by.include?(current_user.uuid) rescue nil) %> <% if @object.uuid != current_user.uuid # Not the "Home" project %> <%= link_to( choose_projects_path( diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 539f69d6ca..b9f22057e4 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -90,16 +90,24 @@ class ArvadosModel < ActiveRecord::Base api_column_map end - # Return nil if current user is not allowed to see the list of - # writers. Otherwise, return a list of user_ and group_uuids with - # write permission. (If not returning nil, current_user is always in - # the list because can_manage permission is needed to see the list - # of writers.) + # Return an array of uuids of users and groups that have permission + # to write this object. The first two elements are always + # [self.owner_uuid, current user's uuid]. + # + # If the the current user can write but not manage the object, + # return [self.owner_uuid, current user's uuid]. + # + # If current user cannot write this object, just return + # [self.owner_uuid]. def writable_by unless (owner_uuid == current_user.uuid or current_user.is_admin or - current_user.groups_i_can(:manage).index(owner_uuid)) - return nil + (current_user.groups_i_can(:manage) & [uuid, owner_uuid]).any?) + if current_user.groups_i_can(:write).index(uuid) + return [owner_uuid, current_user.uuid] + else + return [owner_uuid] + end end [owner_uuid, current_user.uuid] + permissions.collect do |p| if ['can_write', 'can_manage'].index p.name diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb index 217809ae65..b755bb742b 100644 --- a/services/api/test/functional/arvados/v1/groups_controller_test.rb +++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb @@ -369,8 +369,9 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase format: :json } assert_response :success - assert_nil(json_response['writable_by'], - "Should not receive uuid list in 'writable_by' field") + assert_equal([json_response['owner_uuid']], + json_response['writable_by'], + "Should only see owner_uuid in 'writable_by' field") end test 'get writable_by list by admin user' do diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 1ea1419147..56fe867f5a 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -63,6 +63,60 @@ class PermissionTest < ActiveSupport::TestCase assert ob.writable_by.include?(users(:active).uuid), "user does not have write permission" end + test "writable_by reports requesting user's own uuid for a writable project" do + invited_to_write = users(:project_viewer) + group = groups(:asubproject) + + # project_view can read, but cannot see write or see writers list + set_user_from_auth :project_viewer + assert_equal([group.owner_uuid], + group.writable_by, + "writers list should just have owner_uuid") + + # allow project_viewer to write for the remainder of the test + set_user_from_auth :admin + Link.create!(tail_uuid: invited_to_write.uuid, + head_uuid: group.uuid, + link_class: 'permission', + name: 'can_write') + group.permissions.reload + + # project_viewer should see self in writers list (but not all writers) + set_user_from_auth :project_viewer + assert_not_nil(group.writable_by, + "can write but cannot see writers list") + assert_includes(group.writable_by, invited_to_write.uuid, + "self missing from writers list") + assert_includes(group.writable_by, group.owner_uuid, + "project owner missing from writers list") + refute_includes(group.writable_by, users(:active).uuid, + "saw :active user in writers list") + + # active user should see full writers list + set_user_from_auth :active + assert_includes(group.writable_by, invited_to_write.uuid, + "permission just added, but missing from writers list") + + # allow project_viewer to manage for the remainder of the test + set_user_from_auth :admin + Link.create!(tail_uuid: invited_to_write.uuid, + head_uuid: group.uuid, + link_class: 'permission', + name: 'can_manage') + # invite another writer we can test for + Link.create!(tail_uuid: users(:spectator).uuid, + head_uuid: group.uuid, + link_class: 'permission', + name: 'can_write') + group.permissions.reload + + set_user_from_auth :project_viewer + assert_not_nil(group.writable_by, + "can manage but cannot see writers list") + assert_includes(group.writable_by, users(:spectator).uuid, + ":spectator missing from writers list") + end + test "user owns group, group can_manage object's group, user can add permissions" do set_user_from_auth :admin -- 2.39.5