[ARVADOS] updated: ac97e7087df8b48ee289c696b47973618a40ce73
git at public.curoverse.com
git at public.curoverse.com
Mon Jul 14 15:09:45 EDT 2014
Summary of changes:
.../api/app/controllers/application_controller.rb | 8 ++
.../app/controllers/arvados/v1/users_controller.rb | 11 +++
services/api/app/models/arvados_model.rb | 15 ++--
services/api/lib/record_filters.rb | 27 ++++--
.../test/fixtures/api_client_authorizations.yml | 6 ++
services/api/test/fixtures/jobs.yml | 2 +-
services/api/test/fixtures/links.yml | 28 ++++++
services/api/test/fixtures/pipeline_templates.yml | 2 +-
services/api/test/fixtures/users.yml | 10 +++
.../arvados/v1/groups_controller_test.rb | 77 ++++++++++++++--
.../functional/arvados/v1/users_controller_test.rb | 100 +++++++++++++++++++++
services/api/test/integration/permissions_test.rb | 16 +---
services/api/test/integration/select_test.rb | 2 +-
13 files changed, 270 insertions(+), 34 deletions(-)
via ac97e7087df8b48ee289c696b47973618a40ce73 (commit)
via 16bf6c2ad0f88183198701cc6242240f9b194a7b (commit)
via 07f7373ef75ae9c574060e2a61c3749dc7c6ffe3 (commit)
via be6e47b1274d4b4e80afe51593222f99d90bac66 (commit)
from 56303d3c734fa0ed9d5f077aac60b97596506efc (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 ac97e7087df8b48ee289c696b47973618a40ce73
Merge: 56303d3 16bf6c2
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jul 14 15:10:44 2014 -0400
Merge branch '2044-api-users-index-wip'
Refs #2044. Closes #3217.
commit 16bf6c2ad0f88183198701cc6242240f9b194a7b
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..0b76029 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,68 @@ 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 project_viewer tests go through other controllers,
+ # I'm putting them here so they're easy to find alongside the other
+ # project tests.
+ def check_new_project_link_fails(link_attrs)
+ @controller = Arvados::V1::LinksController.new
+ post :create, link: {
+ link_class: "permission",
+ name: "can_read",
+ head_uuid: groups(:aproject).uuid,
+ }.merge(link_attrs)
+ assert_includes(403..422, response.status)
+ end
+
+ test "user with project read permission can't add users to it" do
+ authorize_with :project_viewer
+ check_new_project_link_fails(tail_uuid: users(:spectator).uuid)
+ end
+
+ test "user with project read permission can't add items to it" do
+ authorize_with :project_viewer
+ check_new_project_link_fails(tail_uuid: collections(:baz_file).uuid)
+ end
+
+ test "user with project read permission can't rename items in it" do
+ authorize_with :project_viewer
+ @controller = Arvados::V1::LinksController.new
+ post :update, {
+ id: links(:job_name_in_aproject).uuid,
+ link: {name: "Denied test name"},
+ }
+ assert_includes(403..404, 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 "user with project read permission can't delete it" do
+ authorize_with :project_viewer
+ post :destroy, {id: groups(:aproject).uuid}
+ assert_response 403
end
test 'get group-owned objects with limit' do
commit 07f7373ef75ae9c574060e2a61c3749dc7c6ffe3
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 be6e47b1274d4b4e80afe51593222f99d90bac66
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..a044fb7 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