[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