[ARVADOS] created: 1a443cf1eae37912dc90c1d74f41d7a1c68f5587
git at public.curoverse.com
git at public.curoverse.com
Wed Jul 9 17:32:14 EDT 2014
at 1a443cf1eae37912dc90c1d74f41d7a1c68f5587 (commit)
commit 1a443cf1eae37912dc90c1d74f41d7a1c68f5587
Author: Brett Smith <brett at curoverse.com>
Date: Tue Jul 1 13:45:49 2014 -0400
2044: Improve API test fixtures around projects.
This change adjusts the API server test fixtures to look more like
real data would: items in projects have the project's UUID as their
owner_uuid.
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 48b4b87..4d465a6 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -43,6 +43,12 @@ active_noscope:
expires_at: 2038-01-01 00:00:00
scopes: []
+project_viewer:
+ api_client: untrusted
+ user: project_viewer
+ api_token: projectviewertoken1234567890abcdefghijklmnopqrstuv
+ expires_at: 2038-01-01 00:00:00
+
admin_vm:
api_client: untrusted
user: admin
diff --git a/services/api/test/fixtures/jobs.yml b/services/api/test/fixtures/jobs.yml
index adfc90f..e142fae 100644
--- a/services/api/test/fixtures/jobs.yml
+++ b/services/api/test/fixtures/jobs.yml
@@ -1,6 +1,6 @@
running:
uuid: zzzzz-8i9sb-pshmckwoma9plh7
- owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ owner_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
cancelled_at: ~
cancelled_by_user_uuid: ~
cancelled_by_client_uuid: ~
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index e9cb9ef..313db7f 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -166,6 +166,20 @@ foo_file_readable_by_active_redundant_permission_via_private_group:
head_uuid: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45
properties: {}
+foo_file_readable_by_aproject:
+ uuid: zzzzz-o0j2j-fp1d8395ldqw22p
+ 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-000000000000000
+ modified_at: 2014-01-24 20:42:26 -0800
+ updated_at: 2014-01-24 20:42:26 -0800
+ tail_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
+ link_class: permission
+ name: can_read
+ head_uuid: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45
+ properties: {}
+
bar_file_readable_by_active:
uuid: zzzzz-o0j2j-8hppiuduf8eqdng
owner_uuid: zzzzz-tpzed-000000000000000
@@ -304,6 +318,20 @@ test_timestamps:
name: test
properties: {}
+project_viewer_can_read_project:
+ uuid: zzzzz-o0j2j-projviewerreadp
+ 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-projectviewer1a
+ link_class: permission
+ name: can_read
+ head_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
+ properties: {}
+
specimen_is_in_two_projects:
uuid: zzzzz-o0j2j-ryhm1bn83ni03sn
owner_uuid: zzzzz-j7d0g-axqo7eu9pwvna1x
diff --git a/services/api/test/fixtures/pipeline_templates.yml b/services/api/test/fixtures/pipeline_templates.yml
index 74cb1e8..8e3a070 100644
--- a/services/api/test/fixtures/pipeline_templates.yml
+++ b/services/api/test/fixtures/pipeline_templates.yml
@@ -1,6 +1,6 @@
two_part:
uuid: zzzzz-p5p6p-aox0k0ofxrystgw
- owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ owner_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
created_at: 2014-04-14 12:35:04 -0400
updated_at: 2014-04-14 12:35:04 -0400
modified_at: 2014-04-14 12:35:04 -0400
diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml
index ac76d5f..f6d5b21 100644
--- a/services/api/test/fixtures/users.yml
+++ b/services/api/test/fixtures/users.yml
@@ -40,6 +40,16 @@ active:
is_admin: false
prefs: {}
+project_viewer:
+ uuid: zzzzz-tpzed-projectviewer1a
+ email: project-viewer at arvados.local
+ first_name: Project
+ last_name: Viewer
+ identity_url: https://project-viewer.openid.local
+ is_active: true
+ is_admin: false
+ prefs: {}
+
spectator:
uuid: zzzzz-tpzed-l1s2piq4t4mps8r
email: spectator at arvados.local
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 49e9b7d..1e7a881 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -54,6 +54,15 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_equal 0, json_response['items_available']
end
+ def check_project_contents_response
+ assert_response :success
+ assert_operator 2, :<=, json_response['items_available']
+ assert_operator 2, :<=, json_response['items'].count
+ kinds = json_response['items'].collect { |i| i['kind'] }.uniq
+ expect_kinds = %w'arvados#group arvados#specimen arvados#pipelineTemplate arvados#job'
+ assert_equal expect_kinds, (expect_kinds & kinds)
+ end
+
test 'get group-owned objects' do
authorize_with :active
get :contents, {
@@ -61,12 +70,45 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
format: :json,
include_linked: true,
}
- assert_response :success
- assert_operator 2, :<=, json_response['items_available']
- assert_operator 2, :<=, json_response['items'].count
- kinds = json_response['items'].collect { |i| i['kind'] }.uniq
- expect_kinds = %w'arvados#group arvados#specimen arvados#pipelineTemplate arvados#job'
- assert_equal expect_kinds, (expect_kinds & kinds)
+ check_project_contents_response
+ end
+
+ test "user with project read permission can see project objects" do
+ authorize_with :project_viewer
+ get :contents, {
+ id: groups(:aproject).uuid,
+ format: :json,
+ include_linked: true,
+ }
+ check_project_contents_response
+ end
+
+ # Even though the next two tests go through other controllers, I'm
+ # putting them here so they're easy to find alongside the other
+ # project tests.
+ test "user with project read permission can't add users to it" do
+ @controller = Arvados::V1::LinksController.new
+ authorize_with :project_viewer
+ post :create, link: {
+ tail_uuid: users(:spectator).uuid,
+ link_class: "permission",
+ name: "can_read",
+ head_uuid: groups(:aproject).uuid,
+ }
+ # 404 seems like the best error, but that's not nailed down yet.
+ assert_includes(403..422, response.status)
+ end
+
+ test "user with project read permission can't remove items from it" do
+ @controller = Arvados::V1::PipelineTemplatesController.new
+ authorize_with :project_viewer
+ post :update, {
+ id: links(:template_name_in_aproject).head_uuid,
+ pipeline_template: {
+ owner_uuid: users(:project_viewer).uuid,
+ }
+ }
+ assert_response 403
end
test 'get group-owned objects with limit' do
commit 21195757b4c46d8af6f88b99bf667fd1f21dbe16
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jul 7 11:01:51 2014 -0400
2044: API filters can search on boolean columns.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 41286fe..1ea9332 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -52,13 +52,16 @@ class ArvadosModel < ActiveRecord::Base
def self.searchable_columns operator
textonly_operator = !operator.match(/[<=>]/)
- self.columns.collect do |col|
- if [:string, :text].index(col.type)
- col.name
- elsif !textonly_operator and [:datetime, :integer].index(col.type)
- col.name
+ self.columns.select do |col|
+ case col.type
+ when :string, :text
+ true
+ when :datetime, :integer, :boolean
+ !textonly_operator
+ else
+ false
end
- end.compact
+ end.map(&:name)
end
def self.attribute_column attr
diff --git a/services/api/lib/record_filters.rb b/services/api/lib/record_filters.rb
index 01d0ae4..f31875b 100644
--- a/services/api/lib/record_filters.rb
+++ b/services/api/lib/record_filters.rb
@@ -30,22 +30,39 @@ module RecordFilters
end
case operator.downcase
when '=', '<', '<=', '>', '>=', '!=', 'like'
+ attr_type = model_class.attribute_column(attr).type
+ operator = '<>' if operator == '!='
if operand.is_a? String
- if operator == '!='
- operator = '<>'
+ if attr_type == :boolean
+ if not ['=', '<>'].include?(operator)
+ raise ArgumentError.new("Invalid operator '#{operator}' for " \
+ "boolean attribute '#{attr}'")
+ end
+ case operand.downcase
+ when '1', 't', 'true', 'y', 'yes'
+ operand = true
+ when '0', 'f', 'false', 'n', 'no'
+ operand = false
+ else
+ raise ArgumentError("Invalid operand '#{operand}' for " \
+ "boolean attribute '#{attr}'")
+ end
end
cond_out << "#{ar_table_name}.#{attr} #{operator} ?"
if (# any operator that operates on value rather than
# representation:
- operator.match(/[<=>]/) and
- model_class.attribute_column(attr).type == :datetime)
+ operator.match(/[<=>]/) and (attr_type == :datetime))
operand = Time.parse operand
end
param_out << operand
elsif operand.nil? and operator == '='
cond_out << "#{ar_table_name}.#{attr} is null"
- elsif operand.nil? and operator == '!='
+ elsif operand.nil? and operator == '<>'
cond_out << "#{ar_table_name}.#{attr} is not null"
+ elsif (attr_type == :boolean) and ['=', '<>'].include?(operator) and
+ [true, false].include?(operand)
+ cond_out << "#{ar_table_name}.#{attr} #{operator} ?"
+ param_out << operand
else
raise ArgumentError.new("Invalid operand type '#{operand.class}' "\
"for '#{operator}' operator in filters")
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 aed6a83..cdbc79b 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -828,6 +828,20 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
check_inactive_user_findable(reader_tokens: [api_token(:admin)])
end
+ test "admin can filter on user.is_active" do
+ authorize_with :admin
+ get(:index, filters: [["is_active", "=", "true"]])
+ assert_response :success
+ check_active_users_index
+ 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
+ end
+
NON_ADMIN_USER_DATA = ["uuid", "kind", "is_active", "email", "first_name",
"last_name"].sort
commit a33fb608ea2731701604e7b26b7b0e4229c5ab2c
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jul 7 11:01:30 2014 -0400
2044: Non-admins get all users' basic info from index API.
This will support a Workbench feature to let users at the same site
share projects with each other. Adding kind to index requests with
select parameters is necessary to help Workbench understand the
result.
There was extensive discussion on IRC about whether or not e-mail
addresses should be included in this information. We toyed with ideas
like providing an e-mail address checksum, so that you could find
exact but not partial matches. Ultimately I decided that none of
those measures were worth the hassle, because the domain of addresses
at any given site would be small enough that they would easily be
discoverable through brute force.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 7e5b316..8464a4a 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -351,6 +351,14 @@ 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/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index a671809..a31fddc 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -122,4 +122,15 @@ class Arvados::V1::UsersController < ApplicationController
}
end
+ def find_objects_for_index
+ 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)
+ end
+ super
+ end
end
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 04e74aa..aed6a83 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -788,6 +788,92 @@ 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
+ authorize_with :spectator
+ get(:index)
+ check_non_admin_index
+ check_active_users_index
+ 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__"})
+ end
+
+ test "group admin is treated like non-admin for index" do
+ check_index_condition_fails(:rominiadmin,
+ filters: [["last_name", "=", "__nonexistent__"]])
+ end
+
+ test "admin has full index powers" do
+ authorize_with :admin
+ check_inactive_user_findable
+ end
+
+ test "reader token can grant admin index powers" do
+ authorize_with :spectator
+ check_inactive_user_findable(reader_tokens: [api_token(:admin)])
+ end
+
+ NON_ADMIN_USER_DATA = ["uuid", "kind", "is_active", "email", "first_name",
+ "last_name"].sort
+
+ def check_non_admin_index
+ assert_response :success
+ 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"])
+ assert(user_data["is_active"], "non-admin index returned inactive user")
+ end
+ end
+
+ def check_active_users_index
+ response_uuids = json_response["items"].map { |u| u["uuid"] }
+ [:admin, :miniadmin, :active, :spectator].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")
+ end
+
+ def check_inactive_user_findable(params={})
+ inactive_user = users(:inactive)
+ get(:index, params.merge(filters: [["email", "=", inactive_user.email]]))
+ assert_response :success
+ user_list = json_response["items"]
+ assert_equal(1, user_list.andand.count)
+ # This test needs to check a column non-admins have no access to,
+ # to ensure that admins see all user information.
+ assert_equal(inactive_user.identity_url, user_list.first["identity_url"],
+ "admin's filtered index did not return inactive user")
+ end
+
def verify_num_links (original_links, expected_additional_links)
links_now = Link.all
assert_equal expected_additional_links, Link.all.size-original_links.size,
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index 095c2dc..274e8f1 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -100,7 +100,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
# try to read collection as spectator
get "/arvados/v1/collections/#{collections(:foo_file).uuid}", {:format => :json}, auth(:spectator)
assert_response 404
-
+
end
@@ -151,7 +151,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
# try to read collection as spectator
get "/arvados/v1/collections/#{collections(:foo_file).uuid}", {:format => :json}, auth(:spectator)
assert_response 404
-
+
end
test "adding can_read links from user to group, group to group, group to collection" do
@@ -212,18 +212,6 @@ class PermissionsTest < ActionDispatch::IntegrationTest
assert_response 404
end
- test "read-only group-admin sees correct subset of user list" do
- get "/arvados/v1/users", {:format => :json}, auth(:rominiadmin)
- assert_response :success
- resp_uuids = json_response['items'].collect { |i| i['uuid'] }
- [[true, users(:rominiadmin).uuid],
- [true, users(:active).uuid],
- [false, users(:miniadmin).uuid],
- [false, users(:spectator).uuid]].each do |should_find, uuid|
- assert_equal should_find, !resp_uuids.index(uuid).nil?, "rominiadmin should #{'not ' if !should_find}see #{uuid} in user list"
- end
- end
-
test "read-only group-admin cannot modify administered user" do
put "/arvados/v1/users/#{users(:active).uuid}", {
:user => {
diff --git a/services/api/test/integration/select_test.rb b/services/api/test/integration/select_test.rb
index 2057565..69ac914 100644
--- a/services/api/test/integration/select_test.rb
+++ b/services/api/test/integration/select_test.rb
@@ -5,7 +5,7 @@ class SelectTest < ActionDispatch::IntegrationTest
get "/arvados/v1/links", {:format => :json, :select => ['uuid', 'link_class']}, auth(:active)
assert_response :success
assert_equal json_response['items'].count, json_response['items'].select { |i|
- i.count == 2 and i['uuid'] != nil and i['link_class'] != nil
+ i.count == 3 and i['uuid'] != nil and i['link_class'] != nil
}.count
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list