[ARVADOS] updated: 3ef0016f85290758f286b6026fbd1a2d0c11ad4e
git at public.curoverse.com
git at public.curoverse.com
Thu Aug 21 23:16:48 EDT 2014
Summary of changes:
.../api/app/controllers/application_controller.rb | 37 +++----
.../arvados/v1/collections_controller.rb | 10 +-
.../app/controllers/arvados/v1/users_controller.rb | 19 ++--
services/api/app/models/arvados_model.rb | 108 +++++++++----------
services/api/app/models/link.rb | 15 +--
services/api/app/models/user.rb | 9 +-
services/api/test/factories/api_client.rb | 10 ++
.../api/test/factories/api_client_authorization.rb | 19 ++++
services/api/test/factories/user.rb | 25 ++++-
.../functional/arvados/v1/links_controller_test.rb | 20 ++--
.../functional/arvados/v1/users_controller_test.rb | 85 ++++++++-------
services/api/test/integration/select_test.rb | 3 +-
services/api/test/test_helper.rb | 11 +-
services/api/test/unit/link_test.rb | 4 +-
services/api/test/unit/permission_test.rb | 114 +++++++++++++++++++--
15 files changed, 323 insertions(+), 166 deletions(-)
create mode 100644 services/api/test/factories/api_client.rb
create mode 100644 services/api/test/factories/api_client_authorization.rb
via 3ef0016f85290758f286b6026fbd1a2d0c11ad4e (commit)
via 42a27c7cabdacb00bd5a9ba06443c8a72322738b (commit)
via 779547997857b04c27e5df52f70d527fcaa7a551 (commit)
from 754d85439d5e9a835562689dee597b782932914f (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 3ef0016f85290758f286b6026fbd1a2d0c11ad4e
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Aug 21 23:14:25 2014 -0400
3171: Update tests to conform to new permission behavior.
Obey "select" parameter during #show too, not just #index.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index d3b5c6b..ecf2514 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -65,7 +65,7 @@ class ApplicationController < ActionController::Base
end
def show
- render json: @object.as_api_response
+ render json: @object.as_api_response(nil, select: @select)
end
def create
@@ -204,15 +204,24 @@ class ApplicationController < ActionController::Base
end
if @select
- # Map attribute names in @select to real column names, resolve
- # those to fully-qualified SQL column names, and pass the
- # resulting string to the select method.
- api_column_map = model_class.attributes_required_columns
- columns_list = @select.
- flat_map { |attr| api_column_map[attr] }.
- uniq.
- map { |s| "#{table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }
- @objects = @objects.select(columns_list.join(", "))
+ if action_name != 'update'
+ # Map attribute names in @select to real column names, resolve
+ # those to fully-qualified SQL column names, and pass the
+ # resulting string to the select method.
+ api_column_map = model_class.attributes_required_columns
+ columns_list = @select.
+ flat_map { |attr| api_column_map[attr] }.
+ uniq.
+ map { |s| "#{table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }
+ @objects = @objects.select(columns_list.join(", "))
+ end
+
+ # This information helps clients understand what they're seeing
+ # (Workbench always expects it), but they can't select it explicitly
+ # because it's not an SQL column. Always add it.
+ # (This is harmless, given that clients can deduce what they're
+ # looking at by the returned UUID anyway.)
+ @select |= ["kind"]
end
@objects = @objects.order(@orders.join ", ") if @orders.any?
@objects = @objects.limit(@limit)
@@ -362,14 +371,6 @@ class ApplicationController < ActionController::Base
accept_param_as_json :reader_tokens, Array
def render_list
- if @select
- # This information helps clients understand what they're seeing
- # (Workbench always expects it), but they can't select it explicitly
- # because it's not an SQL column. Always add it.
- # I believe this is safe because clients can always deduce what they're
- # looking at by the returned UUID anyway.
- @select |= ["kind"]
- end
@object_list = {
:kind => "arvados##{(@response_resource_name || resource_name).camelize(:lower)}List",
:etag => "",
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index b65fa5b..2add844 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -254,10 +254,12 @@ class Arvados::V1::CollectionsController < ApplicationController
protected
- def find_objects_for_index
- # Omit manifest_text from index results unless expressly selected.
- @select ||= model_class.api_accessible_attributes(:user).
- map { |attr_spec| attr_spec.first.to_s } - ["manifest_text"]
+ def apply_filters
+ if action_name == 'index'
+ # Omit manifest_text from index results unless expressly selected.
+ @select ||= model_class.api_accessible_attributes(:user).
+ map { |attr_spec| attr_spec.first.to_s } - ["manifest_text"]
+ end
super
end
diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 0afb4e5..50ee3b0 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -137,8 +137,9 @@ class Arvados::V1::UsersController < ApplicationController
end
def apply_filters
- if (action_name == "index") and (not @read_users.any? { |u| u.is_admin })
- # Non-admin index returns very basic information about readable users.
+ return super if @read_users.any? &:is_admin
+ if params[:uuid] != current_user.andand.uuid
+ # Non-admin index/show returns very basic information about readable users.
safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name"]
if @select
@select = @select & safe_attrs
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 5798b4e..16bafca 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -131,8 +131,6 @@ class ArvadosModel < ActiveRecord::Base
# Collect the uuids for each user and any groups readable by each user.
user_uuids = users_list.map { |u| u.uuid }
uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
- sanitized_uuid_list = uuid_list.
- collect { |uuid| sanitize(uuid) }.join(', ')
sql_conds = []
sql_params = []
sql_table = kwargs.fetch(:table_name, table_name)
@@ -146,12 +144,18 @@ class ArvadosModel < ActiveRecord::Base
# A permission link exists ('write' and 'manage' implicitly include
# 'read') from a member of users_list, or a group readable by users_list,
# to this row, or to the owner of this row (see join() below).
- permitted_uuids = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (#{sanitized_uuid_list}))"
+ sql_conds += ["#{sql_table}.uuid in (?)"]
+ sql_params += [user_uuids]
- sql_conds += ["#{sql_table}.owner_uuid in (?)",
- "#{sql_table}.uuid in (?)",
- "#{sql_table}.uuid IN #{permitted_uuids}"]
- sql_params += [uuid_list, user_uuids]
+ if uuid_list.any?
+ sql_conds += ["#{sql_table}.owner_uuid in (?)"]
+ sql_params += [uuid_list]
+
+ sanitized_uuid_list = uuid_list.
+ collect { |uuid| sanitize(uuid) }.join(', ')
+ permitted_uuids = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (#{sanitized_uuid_list}))"
+ sql_conds += ["#{sql_table}.uuid IN #{permitted_uuids}"]
+ end
if sql_table == "links" and users_list.any?
# This row is a 'permission' or 'resources' link class
diff --git a/services/api/test/factories/api_client.rb b/services/api/test/factories/api_client.rb
new file mode 100644
index 0000000..7921c35
--- /dev/null
+++ b/services/api/test/factories/api_client.rb
@@ -0,0 +1,10 @@
+FactoryGirl.define do
+ factory :api_client do
+ is_trusted false
+ to_create do |instance|
+ act_as_system_user do
+ instance.save!
+ end
+ end
+ end
+end
diff --git a/services/api/test/factories/api_client_authorization.rb b/services/api/test/factories/api_client_authorization.rb
new file mode 100644
index 0000000..8bd569e
--- /dev/null
+++ b/services/api/test/factories/api_client_authorization.rb
@@ -0,0 +1,19 @@
+FactoryGirl.define do
+ factory :api_client_authorization do
+ api_client
+ scopes ['all']
+
+ trait :trusted do
+ association :api_client, factory: :api_client, is_trusted: true
+ end
+ factory :token do
+ # Just provides shorthand for "create :api_client_authorization"
+ end
+
+ to_create do |instance|
+ act_as_user instance.user do
+ instance.save!
+ end
+ end
+ end
+end
diff --git a/services/api/test/factories/user.rb b/services/api/test/factories/user.rb
index 7c48fc0..56e9125 100644
--- a/services/api/test/factories/user.rb
+++ b/services/api/test/factories/user.rb
@@ -2,12 +2,22 @@ include CurrentApiClient
FactoryGirl.define do
factory :user do
- before :create do
- Thread.current[:user_was] = Thread.current[:user]
- Thread.current[:user] = system_user
+ ignore do
+ join_groups []
end
- after :create do
- Thread.current[:user] = Thread.current[:user_was]
+ after :create do |user, evaluator|
+ act_as_system_user do
+ evaluator.join_groups.each do |g|
+ Link.create!(tail_uuid: user.uuid,
+ head_uuid: g.uuid,
+ link_class: 'permission',
+ name: 'can_read')
+ Link.create!(tail_uuid: g.uuid,
+ head_uuid: user.uuid,
+ link_class: 'permission',
+ name: 'can_read')
+ end
+ end
end
first_name "Factory"
last_name "Factory"
@@ -25,5 +35,10 @@ FactoryGirl.define do
end
end
end
+ to_create do |instance|
+ act_as_system_user do
+ instance.save!
+ end
+ end
end
end
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 b131947..4762ea1 100644
--- a/services/api/test/functional/arvados/v1/links_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/links_controller_test.rb
@@ -312,16 +312,16 @@ class Arvados::V1::LinksControllerTest < ActionController::TestCase
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.
+ test "retrieve all permissions using generic links index api" do
+ skip "(not implemented)"
+ # Links.readable_by() does not return the full set of permission
+ # links that are visible to a user (i.e., all permission links
+ # whose head_uuid references an object for which the user has
+ # ownership or can_manage permission). Therefore, neither does
+ # /arvados/v1/links.
+ #
+ # It is possible to retrieve the full set of permissions for a
+ # single object via /arvados/v1/permissions.
authorize_with :active
get :index, filters: [['link_class', '=', 'permission'],
['head_uuid', '=', groups(:aproject).uuid]]
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 1a8e94f..fdfc951 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -796,34 +796,38 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
'Expected workbench url in email body'
end
- test "non-admin user can get basic information about active users" do
+ test "non-admin user can get basic information about readable users" do
authorize_with :spectator
get(:index)
check_non_admin_index
- check_active_users_index
+ check_readable_users_index [:spectator], [:inactive, :active]
end
- test "non-admin user can limit index" do
- authorize_with :spectator
- get(:index, limit: 2)
- check_non_admin_index
- assert_equal(2, json_response["items"].size,
- "non-admin index limit was ineffective")
- end
-
- test "filters are ignored for non-admin index" do
- check_index_condition_fails(:spectator,
- filters: [["last_name", "=", "__nonexistent__"]])
- end
-
- test "where is ignored for non-admin index" do
- check_index_condition_fails(:spectator,
- where: {last_name: "__nonexistent__"})
+ test "non-admin user gets only safe attributes from users#show" do
+ g = act_as_system_user do
+ create :group
+ end
+ users = create_list :active_user, 2, join_groups: [g]
+ token = create :token, user: users[0]
+ authorize_with_token token
+ get :show, id: users[1].uuid
+ check_non_admin_show
end
- test "group admin is treated like non-admin for index" do
- check_index_condition_fails(:rominiadmin,
- filters: [["last_name", "=", "__nonexistent__"]])
+ test "non-admin user can limit index" do
+ g = act_as_system_user do
+ create :group
+ end
+ users = create_list :active_user, 4, join_groups: [g]
+ token = create :token, user: users[0]
+
+ [2, 4].each do |limit|
+ authorize_with_token token
+ get(:index, limit: limit)
+ check_non_admin_index
+ assert_equal(limit, json_response["items"].size,
+ "non-admin index limit was ineffective")
+ end
end
test "admin has full index powers" do
@@ -840,14 +844,14 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
authorize_with :admin
get(:index, filters: [["is_active", "=", "true"]])
assert_response :success
- check_active_users_index
+ check_readable_users_index [:active, :spectator], [:inactive]
end
test "admin can search where user.is_active" do
authorize_with :admin
get(:index, where: {is_active: true})
assert_response :success
- check_active_users_index
+ check_readable_users_index [:active, :spectator], [:inactive]
end
test "update active_no_prefs user profile and expect notification email" do
@@ -923,30 +927,33 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
response_items = json_response["items"]
assert_not_nil response_items
response_items.each do |user_data|
- assert_equal(NON_ADMIN_USER_DATA, user_data.keys.sort,
- "data in all users response did not match expectations")
- assert_equal("arvados#user", user_data["kind"])
+ check_non_admin_item user_data
assert(user_data["is_active"], "non-admin index returned inactive user")
end
end
- def check_active_users_index
+ def check_non_admin_show
+ assert_response :success
+ check_non_admin_item json_response
+ end
+
+ def check_non_admin_item user_data
+ assert_equal(NON_ADMIN_USER_DATA, user_data.keys.sort,
+ "data in response had missing or extra attributes")
+ assert_equal("arvados#user", user_data["kind"])
+ end
+
+
+ def check_readable_users_index expect_present, expect_missing
response_uuids = json_response["items"].map { |u| u["uuid"] }
- [:admin, :miniadmin, :active, :spectator].each do |user_key|
+ expect_present.each do |user_key|
assert_includes(response_uuids, users(user_key).uuid,
"#{user_key} missing from index")
end
- refute_includes(response_uuids, users(:inactive).uuid,
- "inactive user included in index")
- end
-
- def check_index_condition_fails(user_sym, params)
- authorize_with user_sym
- get(:index, params)
- check_non_admin_index
- assert(json_response["items"]
- .any? { |u| u["last_name"] != "__nonexistent__" },
- "#{params.inspect} successfully applied to non-admin index")
+ expect_missing.each do |user_key|
+ refute_includes(response_uuids, users(user_key).uuid,
+ "#{user_key} included in index")
+ end
end
def check_inactive_user_findable(params={})
diff --git a/services/api/test/integration/select_test.rb b/services/api/test/integration/select_test.rb
index 69ac914..a7bd545 100644
--- a/services/api/test/integration/select_test.rb
+++ b/services/api/test/integration/select_test.rb
@@ -18,7 +18,8 @@ class SelectTest < ActionDispatch::IntegrationTest
assert_response :success
distinct = json_response['items']
- assert distinct.count < links.count, "distinct count should be less than link count"
+ assert_operator(distinct.count, :<, links.count,
+ "distinct count should be less than link count")
assert_equal links.uniq.count, distinct.count
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index cd535d2..dfebd92 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -62,8 +62,15 @@ class ActiveSupport::TestCase
self.request.headers["Accept"] = "text/json"
end
- def authorize_with(api_client_auth_name)
- ArvadosApiToken.new.call ({"rack.input" => "", "HTTP_AUTHORIZATION" => "OAuth2 #{api_client_authorizations(api_client_auth_name).api_token}"})
+ def authorize_with api_client_auth_name
+ authorize_with_token api_client_authorizations(api_client_auth_name).api_token
+ end
+
+ def authorize_with_token token
+ t = token
+ t = t.api_token if t.respond_to? :api_token
+ ArvadosApiToken.new.call("rack.input" => "",
+ "HTTP_AUTHORIZATION" => "OAuth2 #{t}")
end
end
diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index 640b26c..3763706 100644
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -100,7 +100,7 @@ class LinkTest < ActiveSupport::TestCase
tail_uuid: users(:admin).uuid)
end
- test "link granting project permissions to unreadable user is valid" do
- assert new_active_link_valid?(tail_uuid: users(:admin).uuid)
+ test "link granting project permissions to unreadable user is invalid" do
+ refute new_active_link_valid?(tail_uuid: users(:admin).uuid)
end
end
commit 42a27c7cabdacb00bd5a9ba06443c8a72322738b
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Aug 21 20:16:47 2014 -0400
3171: Do not follow permission graph through a User, unless permission on the User is can_manage. Restore usual permission model to user lookups. Add tests.
diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 271299b..0afb4e5 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -136,14 +136,16 @@ class Arvados::V1::UsersController < ApplicationController
}
end
- def find_objects_for_index
+ def apply_filters
if (action_name == "index") and (not @read_users.any? { |u| u.is_admin })
- # Non-admin index returns very basic information about all active users.
- # We ignore where and filters params to avoid leaking information.
- @where = {}
- @filters = []
- @select = ["uuid", "is_active", "email", "first_name", "last_name"]
- @objects = model_class.where(is_active: true)
+ # Non-admin index returns very basic information about readable users.
+ safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name"]
+ if @select
+ @select = @select & safe_attrs
+ else
+ @select = safe_attrs
+ end
+ @filters += [['is_active', '=', true]]
end
super
end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 3058081..f4a7de2 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -89,8 +89,9 @@ class Link < ArvadosModel
end
# A user is permitted to create, update or modify a permission link
- # if and only if they have "manage" permission on the destination
- # object.
+ # if and only if they have "manage" permission on the object
+ # indicated by the permission link's head_uuid.
+ #
# All other links are treated as regular ArvadosModel objects.
#
def ensure_owner_uuid_is_permitted
@@ -105,14 +106,4 @@ class Link < ArvadosModel
end
end
- # A user can give all other users permissions on projects.
- def skip_uuid_read_permission_check
- skipped_attrs = super
- if link_class == "permission" and
- (ArvadosModel.resource_class_for_uuid(head_uuid) == Group) and
- (ArvadosModel.resource_class_for_uuid(tail_uuid) == User)
- skipped_attrs << "tail_uuid"
- end
- skipped_attrs
- end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 19e84dc..4df38ce 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -103,12 +103,13 @@ class User < ArvadosModel
Group.where('owner_uuid in (?)', lookup_uuids).each do |group|
newgroups << [group.owner_uuid, group.uuid, 'can_manage']
end
- # add any permission links from the current lookup_uuids to a
- # User or Group.
- Link.where('tail_uuid in (?) and link_class = ? and (head_uuid like ? or head_uuid like ?)',
- lookup_uuids,
+ # add any permission links from the current lookup_uuids to a Group.
+ Link.where('link_class = ? and tail_uuid in (?) and ' \
+ '(head_uuid like ? or (name = ? and head_uuid like ?))',
'permission',
+ lookup_uuids,
Group.uuid_like_pattern,
+ 'can_manage',
User.uuid_like_pattern).each do |link|
newgroups << [link.tail_uuid, link.head_uuid, link.name]
end
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 24399f5..315f2bd 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -132,10 +132,105 @@ class PermissionTest < ActiveSupport::TestCase
end
end
+ test "manager user gets permission to minions' articles via can_manage link" do
+ manager = create :active_user, first_name: "Manage", last_name: "Er"
+ minion = create :active_user, first_name: "Min", last_name: "Ion"
+ minions_specimen = act_as_user minion do
+ Specimen.create!
+ end
+ # Manager creates a group. (Make sure it doesn't magically give
+ # anyone any additional permissions.)
+ g = nil
+ act_as_user manager do
+ g = create :group, name: "NoBigSecret Lab"
+ assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+ "saw a user I shouldn't see")
+ assert_raises(ArvadosModel::PermissionDeniedError,
+ ActiveRecord::RecordInvalid,
+ "gave can_read permission to a user I shouldn't see") do
+ create(:permission_link,
+ name: 'can_read', tail_uuid: minion.uuid, head_uuid: g.uuid)
+ end
+ %w(can_manage can_write can_read).each do |perm_type|
+ assert_raises(ArvadosModel::PermissionDeniedError,
+ ActiveRecord::RecordInvalid,
+ "escalated privileges") do
+ create(:permission_link,
+ name: perm_type, tail_uuid: g.uuid, head_uuid: minion.uuid)
+ end
+ end
+ assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+ "manager saw minion too soon")
+ assert_empty(User.readable_by(minion).where(uuid: manager.uuid),
+ "minion saw manager too soon")
+ assert_empty(Group.readable_by(minion).where(uuid: g.uuid),
+ "minion saw manager's new NoBigSecret Lab group too soon")
+
+ # Manager declares everybody on the system should be able to see
+ # the NoBigSecret Lab group.
+ create(:permission_link,
+ name: 'can_read',
+ tail_uuid: 'zzzzz-j7d0g-fffffffffffffff',
+ head_uuid: g.uuid)
+ # ...but nobody has joined the group yet. Manager still can't see
+ # minion.
+ assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+ "manager saw minion too soon")
+ end
+
+ act_as_user minion do
+ # Minion can see the group.
+ assert_not_empty(Group.readable_by(minion).where(uuid: g.uuid),
+ "minion could not see the NoBigSecret Lab group")
+ # Minion joins the group.
+ create(:permission_link,
+ name: 'can_read',
+ tail_uuid: g.uuid,
+ head_uuid: minion.uuid)
+ end
+
+ act_as_user manager do
+ # Now, manager can see minion.
+ assert_not_empty(User.readable_by(manager).where(uuid: minion.uuid),
+ "manager could not see minion")
+ # But cannot obtain further privileges this way.
+ assert_raises(ArvadosModel::PermissionDeniedError,
+ "escalated privileges") do
+ create(:permission_link,
+ name: 'can_manage', tail_uuid: manager.uuid, head_uuid: minion.uuid)
+ end
+ assert_empty(Specimen
+ .readable_by(manager)
+ .where(uuid: minions_specimen.uuid),
+ "manager saw the minion's private stuff")
+ assert_raises(ArvadosModel::PermissionDeniedError,
+ "manager could update minion's private stuff") do
+ minions_specimen.update_attributes(properties: {'x' => 'y'})
+ end
+ end
+
+ act_as_system_user do
+ # Root can give Manager more privileges over Minion.
+ create(:permission_link,
+ name: 'can_manage', tail_uuid: g.uuid, head_uuid: minion.uuid)
+ end
+
+ act_as_user manager do
+ # Now, manager can read and write Minion's stuff.
+ assert_not_empty(Specimen
+ .readable_by(manager)
+ .where(uuid: minions_specimen.uuid),
+ "manager could not find minion's specimen by uuid")
+ assert_equal(true,
+ minions_specimen.update_attributes(properties: {'x' => 'y'}),
+ "manager could not update minion's specimen object")
+ end
+ end
+
test "users with bidirectional read permission in group can see each other, but cannot see each other's private articles" do
- a = create :active_user first_name: "A"
- b = create :active_user first_name: "B"
- other = create :active_user first_name: "OTHER"
+ a = create :active_user, first_name: "A"
+ b = create :active_user, first_name: "B"
+ other = create :active_user, first_name: "OTHER"
act_as_system_user do
g = create :group
[a,b].each do |u|
@@ -161,15 +256,16 @@ class PermissionTest < ActiveSupport::TestCase
assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do
other.update_attributes!(prefs: {'pwned' => true})
end
- assert_equal true, u.update_attributes!(prefs: {'thisisme' => true})
+ assert_equal(true, u.update_attributes!(prefs: {'thisisme' => true}),
+ "#{u.first_name} can't update its own prefs")
end
act_as_user other do
- ([other, a, b] - [u]).each do |x|
- assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do
- x.update_attributes!(prefs: {'pwned' => true})
- end
+ assert_raises(ArvadosModel::PermissionDeniedError,
+ "OTHER wrote #{u.first_name} without perm") do
+ u.update_attributes!(prefs: {'pwned' => true})
end
- assert_equal true, other.update_attributes!(prefs: {'thisisme' => true})
+ assert_equal(true, other.update_attributes!(prefs: {'thisisme' => true}),
+ "OTHER can't update its own prefs")
end
end
end
commit 779547997857b04c27e5df52f70d527fcaa7a551
Author: Tom Clegg <tom at curoverse.com>
Date: Thu Aug 21 20:12:48 2014 -0400
3171: Outdent giant "if ... else return self" construct.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 539f69d..5798b4e 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -124,67 +124,67 @@ class ArvadosModel < ActiveRecord::Base
end
# Check if any of the users are admin. If so, we're done.
- if users_list.select { |u| u.is_admin }.empty?
-
- # Collect the uuids for each user and any groups readable by each user.
- user_uuids = users_list.map { |u| u.uuid }
- uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
- sanitized_uuid_list = uuid_list.
- collect { |uuid| sanitize(uuid) }.join(', ')
- sql_conds = []
- sql_params = []
- sql_table = kwargs.fetch(:table_name, table_name)
- or_object_uuid = ''
-
- # This row is owned by a member of users_list, or owned by a group
- # readable by a member of users_list
- # or
- # This row uuid is the uuid of a member of users_list
- # or
- # A permission link exists ('write' and 'manage' implicitly include
- # 'read') from a member of users_list, or a group readable by users_list,
- # to this row, or to the owner of this row (see join() below).
- permitted_uuids = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (#{sanitized_uuid_list}))"
-
- sql_conds += ["#{sql_table}.owner_uuid in (?)",
- "#{sql_table}.uuid in (?)",
- "#{sql_table}.uuid IN #{permitted_uuids}"]
- sql_params += [uuid_list, user_uuids]
-
- if sql_table == "links" and users_list.any?
- # 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 += [user_uuids, user_uuids]
- end
-
- if sql_table == "logs" and users_list.any?
- # Link head points to the object described by this row
- sql_conds += ["#{sql_table}.object_uuid IN #{permitted_uuids}"]
-
- # This object described by this row is owned by this user, or owned by a group readable by this user
- sql_conds += ["#{sql_table}.object_owner_uuid in (?)"]
- sql_params += [uuid_list]
- end
-
- if sql_table == "collections" and users_list.any?
- # There is a 'name' link going from a readable group to the collection.
- name_links = "(SELECT head_uuid FROM links WHERE link_class='name' AND tail_uuid IN (#{sanitized_uuid_list}))"
- sql_conds += ["#{sql_table}.uuid IN #{name_links}"]
- end
-
- # Link head points to this row, or to the owner of this row (the thing to be read)
- #
- # Link tail originates from this user, or a group that is readable by this
- # user (the identity with authorization to read)
- #
- # Link class is 'permission' ('write' and 'manage' implicitly include 'read')
- where(sql_conds.join(' OR '), *sql_params)
- else
- # At least one user is admin, so don't bother to apply any restrictions.
- self
- end
+ if users_list.select { |u| u.is_admin }.any?
+ return self
+ end
+
+ # Collect the uuids for each user and any groups readable by each user.
+ user_uuids = users_list.map { |u| u.uuid }
+ uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
+ sanitized_uuid_list = uuid_list.
+ collect { |uuid| sanitize(uuid) }.join(', ')
+ sql_conds = []
+ sql_params = []
+ sql_table = kwargs.fetch(:table_name, table_name)
+ or_object_uuid = ''
+
+ # This row is owned by a member of users_list, or owned by a group
+ # readable by a member of users_list
+ # or
+ # This row uuid is the uuid of a member of users_list
+ # or
+ # A permission link exists ('write' and 'manage' implicitly include
+ # 'read') from a member of users_list, or a group readable by users_list,
+ # to this row, or to the owner of this row (see join() below).
+ permitted_uuids = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (#{sanitized_uuid_list}))"
+
+ sql_conds += ["#{sql_table}.owner_uuid in (?)",
+ "#{sql_table}.uuid in (?)",
+ "#{sql_table}.uuid IN #{permitted_uuids}"]
+ sql_params += [uuid_list, user_uuids]
+
+ if sql_table == "links" and users_list.any?
+ # 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 += [user_uuids, user_uuids]
+ end
+
+ if sql_table == "logs" and users_list.any?
+ # Link head points to the object described by this row
+ sql_conds += ["#{sql_table}.object_uuid IN #{permitted_uuids}"]
+
+ # This object described by this row is owned by this user, or owned by a group readable by this user
+ sql_conds += ["#{sql_table}.object_owner_uuid in (?)"]
+ sql_params += [uuid_list]
+ end
+
+ if sql_table == "collections" and users_list.any?
+ # There is a 'name' link going from a readable group to the collection.
+ name_links = "(SELECT head_uuid FROM links WHERE link_class='name' AND tail_uuid IN (#{sanitized_uuid_list}))"
+ sql_conds += ["#{sql_table}.uuid IN #{name_links}"]
+ end
+
+ # Link head points to this row, or to the owner of this row (the
+ # thing to be read)
+ #
+ # Link tail originates from this user, or a group that is readable
+ # by this user (the identity with authorization to read)
+ #
+ # Link class is 'permission' ('write' and 'manage' implicitly
+ # include 'read')
+ where(sql_conds.join(' OR '), *sql_params)
end
def logged_attributes
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list