[ARVADOS] updated: 3fcd8f1eaa7a792a2ba4a76ed68663e1c73f3634
git at public.curoverse.com
git at public.curoverse.com
Fri Jul 18 16:14:38 EDT 2014
Summary of changes:
.../app/controllers/projects_controller.rb | 9 ++-
.../app/views/projects/_show_sharing.html.erb | 10 +--
.../test/functional/projects_controller_test.rb | 8 ++-
apps/workbench/test/integration/projects_test.rb | 11 +---
.../app/controllers/arvados/v1/links_controller.rb | 14 ++++-
services/api/app/models/arvados_model.rb | 8 +--
.../functional/arvados/v1/links_controller_test.rb | 72 +++++++++++++++++++---
7 files changed, 97 insertions(+), 35 deletions(-)
via 3fcd8f1eaa7a792a2ba4a76ed68663e1c73f3634 (commit)
via c160e78bcad9911d082ecb26392409d2448d6cf9 (commit)
via 2f835cb83e5b495217831bdfc44bc6e4b01ec780 (commit)
from e0e4f50570bf5337518e459decc4df8e78a172fe (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 3fcd8f1eaa7a792a2ba4a76ed68663e1c73f3634
Author: Brett Smith <brett at curoverse.com>
Date: Fri Jul 18 11:38:15 2014 -0400
2044: Non-managers don't get sharing tab on project page.
Per discussion with Tom in IRC.
diff --git a/apps/workbench/app/controllers/projects_controller.rb b/apps/workbench/app/controllers/projects_controller.rb
index 4492010..31cb5ef 100644
--- a/apps/workbench/app/controllers/projects_controller.rb
+++ b/apps/workbench/app/controllers/projects_controller.rb
@@ -8,7 +8,11 @@ class ProjectsController < ApplicationController
end
def show_pane_list
- %w(Contents Sharing Advanced)
+ if @user_is_manager
+ %w(Contents Sharing Advanced)
+ else
+ %w(Contents Advanced)
+ end
end
def remove_item
@@ -92,8 +96,7 @@ class ProjectsController < ApplicationController
@user_is_manager = true
rescue ArvadosApiClient::AccessForbiddenException,
ArvadosApiClient::NotFoundException
- @share_links = Link.filter([['head_uuid', '=', @object.uuid],
- ['link_class', '=', 'permission']])
+ @share_links = []
@user_is_manager = false
end
diff --git a/apps/workbench/app/views/projects/_show_sharing.html.erb b/apps/workbench/app/views/projects/_show_sharing.html.erb
index 47dd2d6..a5482ef 100644
--- a/apps/workbench/app/views/projects/_show_sharing.html.erb
+++ b/apps/workbench/app/views/projects/_show_sharing.html.erb
@@ -22,7 +22,6 @@
end
%>
-<% if @user_is_manager %>
<div class="pull-right">
<% ["users", "groups"].each do |share_class| %>
@@ -41,7 +40,6 @@
<% end %>
</div>
-<% end %>
<p>Permissions for this project are inherited from the <%= owner_type %>
<i class="fa fa-fw <%= owner_icon %>"></i>
@@ -51,7 +49,7 @@
<table id="project_sharing" class="topalign table" style="clear: both; margin-top: 1em;">
<tr>
<th>User/Group Name</th>
- <th<%= raw(' colspan="2"') if @user_is_manager %>>Project Access</th>
+ <th colspan="2">Project Access</th>
</tr>
<% @share_links.andand.each do |link|
@@ -68,7 +66,6 @@
<i class="fa fa-fw <%= fa_icon_class_for_uuid(link.tail_uuid) %>"></i>
<%= link_to_if_arvados_object(link.tail_uuid, link_text: link_name) %>
</td>
- <% if @user_is_manager %>
<td><%= link_to perm_name_desc_map[link.name], '#', {
"data-emptytext" => "Read",
"data-placement" => "bottom",
@@ -93,11 +90,6 @@
<i class="fa fa-fw fa-trash-o"></i>
<% end %>
</td>
- <% else %>
- <td>
- <%= perm_name_desc_map[link.name] %>
- </td>
- <% end %>
</tr>
<% end %>
</table>
diff --git a/apps/workbench/test/functional/projects_controller_test.rb b/apps/workbench/test/functional/projects_controller_test.rb
index 0637c13..dbcfdd0 100644
--- a/apps/workbench/test/functional/projects_controller_test.rb
+++ b/apps/workbench/test/functional/projects_controller_test.rb
@@ -35,7 +35,13 @@ class ProjectsControllerTest < ActionController::TestCase
def user_can_manage(user_sym, group_key)
get(:show, {id: api_fixture("groups")[group_key]["uuid"]},
session_for(user_sym))
- assigns(:user_is_manager)
+ is_manager = assigns(:user_is_manager)
+ assert_not_nil(is_manager, "user_is_manager flag not set")
+ if not is_manager
+ assert_empty(assigns(:share_links),
+ "non-manager has share links set")
+ end
+ is_manager
end
test "admin can_manage aproject" do
diff --git a/apps/workbench/test/integration/projects_test.rb b/apps/workbench/test/integration/projects_test.rb
index e3f79ff..d1bf403 100644
--- a/apps/workbench/test/integration/projects_test.rb
+++ b/apps/workbench/test/integration/projects_test.rb
@@ -121,15 +121,10 @@ class ProjectsTest < ActionDispatch::IntegrationTest
"revoking share did not remove row from sharing table")
end
- test "project viewer can see project sharing, but not change it" do
+ test "project viewer can't see project sharing tab" do
show_project_using("project_viewer")
- click_on "Sharing"
- assert(page.has_text?("Project Viewer"), "did not find self on sharing tab")
- assert(page.has_no_link?("Share with users"),
- "read-only project user given option to add permissions")
- assert_empty(all("#project_sharing a").
- reject { |a| a[:href] =~ %r{/(users|groups)/[-0-9a-z]+$} },
- "read-only project user given option to modify permissions")
+ assert(page.has_no_link?("Sharing"),
+ "read-only project user sees sharing tab")
end
test "project owner can manage sharing for another user" do
commit c160e78bcad9911d082ecb26392409d2448d6cf9
Author: Brett Smith <brett at curoverse.com>
Date: Fri Jul 18 16:13:11 2014 -0400
2044: API server lets group managers see group permission links.
This implementation hooks into find_object_by_uuid because that was
the simplest thing that could work and unblock larger development on
this story. Longer-term, we would like to solve this problem more
generally. See the comments added to the links controller test for
more explanation about that.
diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index f76af60..798217d 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -34,16 +34,26 @@ class Arvados::V1::LinksController < ApplicationController
protected
- # Override find_object_by_uuid: the get_permissions method may be
- # called on a uuid belonging to any class.
def find_object_by_uuid
if action_name == 'get_permissions'
+ # get_permissions accepts a UUID for any kind of object.
@object = ArvadosModel::resource_class_for_uuid(params[:uuid])
.readable_by(*@read_users)
.where(uuid: params[:uuid])
.first
else
super
+ if @object.nil?
+ # Normally group permission links are not readable_by users.
+ # Make an exception for users with permission to manage the group.
+ # FIXME: Solve this more generally - see the controller tests.
+ link = Link.find_by_uuid(params[:uuid])
+ if (not link.nil?) and
+ (link.link_class == "permission") and
+ (@read_users.any? { |u| u.can?(manage: link.head_uuid) })
+ @object = link
+ end
+ end
end
end
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 313db7f..1227618 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -318,6 +318,23 @@ test_timestamps:
name: test
properties: {}
+admin_can_write_aproject:
+ # Yes, this permission is effectively redundant.
+ # We use it to test that other project admins can see
+ # all the project's sharing.
+ uuid: zzzzz-o0j2j-adminmgsproject
+ owner_uuid: zzzzz-tpzed-000000000000000
+ created_at: 2014-01-24 20:42:26 -0800
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ modified_at: 2014-01-24 20:42:26 -0800
+ updated_at: 2014-01-24 20:42:26 -0800
+ tail_uuid: zzzzz-tpzed-d9tiejq69daie8f
+ link_class: permission
+ name: can_write
+ head_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
+ properties: {}
+
project_viewer_can_read_project:
uuid: zzzzz-o0j2j-projviewerreadp
owner_uuid: zzzzz-tpzed-000000000000000
diff --git a/services/api/test/functional/arvados/v1/links_controller_test.rb b/services/api/test/functional/arvados/v1/links_controller_test.rb
index d5b4266..b131947 100644
--- a/services/api/test/functional/arvados/v1/links_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/links_controller_test.rb
@@ -283,4 +283,71 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
}
assert_response 422
end
+
+ test "project owner can show a project permission" do
+ uuid = links(:project_viewer_can_read_project).uuid
+ authorize_with :active
+ get :show, id: uuid
+ assert_response :success
+ assert_equal(uuid, assigns(:object).andand.uuid)
+ end
+
+ test "admin can show a project permission" do
+ uuid = links(:project_viewer_can_read_project).uuid
+ authorize_with :admin
+ get :show, id: uuid
+ assert_response :success
+ assert_equal(uuid, assigns(:object).andand.uuid)
+ end
+
+ test "project viewer can't show others' project permissions" do
+ authorize_with :project_viewer
+ get :show, id: links(:admin_can_write_aproject).uuid
+ assert_response 404
+ end
+
+ test "requesting a nonexistent link returns 404" do
+ authorize_with :active
+ get :show, id: 'zzzzz-zzzzz-zzzzzzzzzzzzzzz'
+ assert_response 404
+ end
+
+ test "project owner can index project permissions" do
+ skip "Test tickles known bug"
+ # readable_by only lets users see permission links that relate to them
+ # directly. It does not allow users to see permission links for groups
+ # they manage.
+ # We'd like to fix this general issue, but we haven't settled on a general
+ # way to do it that doesn't involve making readable_by ridiculously hairy.
+ # This test demonstrates the desired behavior once we're ready to tackle
+ # it. In the meantime, clients should use /permissions to get this
+ # information.
+ authorize_with :active
+ get :index, filters: [['link_class', '=', 'permission'],
+ ['head_uuid', '=', groups(:aproject).uuid]]
+ assert_response :success
+ assert_not_nil assigns(:objects)
+ assert_includes(assigns(:objects).map(&:uuid),
+ links(:project_viewer_can_read_project).uuid)
+ end
+
+ test "admin can index project permissions" do
+ authorize_with :admin
+ get :index, filters: [['link_class', '=', 'permission'],
+ ['head_uuid', '=', groups(:aproject).uuid]]
+ assert_response :success
+ assert_not_nil assigns(:objects)
+ assert_includes(assigns(:objects).map(&:uuid),
+ links(:project_viewer_can_read_project).uuid)
+ end
+
+ test "project viewer can't index others' project permissions" do
+ authorize_with :project_viewer
+ get :index, filters: [['link_class', '=', 'permission'],
+ ['head_uuid', '=', groups(:aproject).uuid],
+ ['tail_uuid', '!=', users(:project_viewer).uuid]]
+ assert_response :success
+ assert_not_nil assigns(:objects)
+ assert_empty assigns(:objects)
+ end
end
commit 2f835cb83e5b495217831bdfc44bc6e4b01ec780
Author: Brett Smith <brett at curoverse.com>
Date: Fri Jul 18 11:09:41 2014 -0400
2044: Revert ff9323418d6a13da701be80aee9e3134e8aa6bab.
This commit was not the correct approach.
Non-managers should not be allowed to see sharing information for
projects they can read.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 9a806d3..1ea9332 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -130,11 +130,11 @@ class ArvadosModel < ActiveRecord::Base
sql_params += [uuid_list, user_uuids]
if sql_table == "links" and users_list.any?
- # This row is a 'permission' or 'resources' link class that
- # references a member of users_list or a group readable by
- # those users.
+ # This row is a 'permission' or 'resources' link class
+ # The uuid for a member of users_list is referenced in either the head
+ # or tail of the link
sql_conds += ["(#{sql_table}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND (#{sql_table}.head_uuid IN (?) OR #{sql_table}.tail_uuid IN (?)))"]
- sql_params += [uuid_list, uuid_list]
+ sql_params += [user_uuids, user_uuids]
end
if sql_table == "logs" and users_list.any?
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 1227618..313db7f 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -318,23 +318,6 @@ test_timestamps:
name: test
properties: {}
-admin_can_write_aproject:
- # Yes, this permission is effectively redundant.
- # We use it to test that other project admins can see
- # all the project's sharing.
- uuid: zzzzz-o0j2j-adminmgsproject
- owner_uuid: zzzzz-tpzed-000000000000000
- created_at: 2014-01-24 20:42:26 -0800
- modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
- modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
- modified_at: 2014-01-24 20:42:26 -0800
- updated_at: 2014-01-24 20:42:26 -0800
- tail_uuid: zzzzz-tpzed-d9tiejq69daie8f
- link_class: permission
- name: can_write
- head_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
- properties: {}
-
project_viewer_can_read_project:
uuid: zzzzz-o0j2j-projviewerreadp
owner_uuid: zzzzz-tpzed-000000000000000
diff --git a/services/api/test/functional/arvados/v1/links_controller_test.rb b/services/api/test/functional/arvados/v1/links_controller_test.rb
index 9dfc350..d5b4266 100644
--- a/services/api/test/functional/arvados/v1/links_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/links_controller_test.rb
@@ -283,15 +283,4 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
}
assert_response 422
end
-
- test "project owner sees project's permission links" do
- authorize_with :active
- get :index, filters: [['head_uuid', '=', groups(:aproject).uuid]]
- uuid_list = assigns(:objects).andand.map(&:uuid)
- assert_not_nil(uuid_list, "no index objects assigned")
- [:admin_can_write_aproject, :project_viewer_can_read_project].each do |lsym|
- assert_includes(uuid_list, links(lsym).uuid,
- "#{lsym} missing from project permission index")
- end
- end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list