Arvados

This is ready for review (ticket #3075).

I haven't implemented a get_permissions method on the ApplicationController as specified in the story; the approach I took didn't require modifying the ApplicationController, and it's not clear to me that we want to expose the permission links via the public API that way. Is that definitely the goal? I did get part of the way on that and can go back to finish the job, but wanted to confirm before I put more effort into it.

Yes, we really need the permissions API: The Workbench "manage sharing" interface will need to show a list of users/groups who currently have access to the object in question, and the relevant link UUIDs so they can be deleted. (Without this permissions API, the permission links are invisible to non-admin users.)

at 975b191

You should make use of the existing permission function current_user.can?(manage: object) when checking permission to create/modify a permission link. (After using resource_class_for_uuid and get the object referenced by head_uuid.) Then, I think the new has_permission?, owns?, can_manage?, and find_user_owning methods will be unnecessary.

Is the change to ArvadosModel#ensure_owner_uuid_is_permitted necessary? It looks similar to the existing checks in Link#permission_to_attach_to_objects. Perhaps this needs to be fixed in Link#permission_to_attach_to_objects:

-  return true if head_obj.owner_uuid == current_user.uuid
+  return current_user.can?(manage: head_obj)

(and remove the can_grant stuff since that's taken care of by can?(manage:...)...)

I think it would be better to override ensure_owner_uuid_is_permitted in Link class rather than adding a new before_filter -- otherwise, the generic ownership test will go to the trouble of checking the provided owner_uuid, even though it would be ignored anyway.

class Link ...
  ...
  def ensure_owner_uuid_is_permitted
    if link_class == 'permission'
      self.owner_uuid = system_user_uuid
      # (look up object)
      if current_user.can?(manage: object)
        return true
      else
        raise PermissionDeniedError
      end
    else
      super
    end
  end
  ...

Tests to add (or point out to me if I just missed them):

• user owns> group can_manage> group owns> object. User should be able to create a permission link with head_uuid==object.uuid.
• user can delete the resulting permission link.
• non-permission links do not get their owner_uuids mangled.

PTAL at 6f70a51

Incorporated the changes you suggested and removed unnecessary code.

Added the API call /arvados/v1/permissions/:uuid routed to Link#get_permissions.

Unit tests:

• "get_permissions returns list"
  • Creates several permissions on an object and tests that the result from /arvados/v1/permissions includes all of the permissions we expect.
• "get_permissions returns 404 for nonexistent uuid"
  • Tests that if /arvados/v1/permissions is called on a uuid that does not exist, it returns 404 instead of an empty permission list.
• "get_permissions returns 403 if user lacks manage permission"
  • Tests that /arvados/v1/permissions returns 403 if the uuid exists but the user does not have can_manage permission on it.
• test/unit/link.rb
  • Added assertions to these tests to confirm that ownership is not changed on non-permission, non-name links.

At 6f70a51

In ArvadosModel.lookup_by_id

• I like this convenience. But could this be find_by_uuid, to match the usual rails finders? (That way "acts like find_*(), not find_all_*()" would be more obvious, for example.) Perhaps this would work:

def self.find_by_uuid uuid
  if self == ArvadosModel
    # If called directly as ArvadosModel.find_by_uuid rather than via subclass,
    # delegate to the appropriate subclass based on the given uuid.
    self.resource_class_for_uuid(uuid).find_by_uuid(uuid)
  else
    super
  end
end

In Link#get_permissions

• I think we don't want find_objects_for_index: it applies the usual permission model, which we don't want, and applies filters/paging, which we have just gone out of our way to skip. Perhaps better to replace @where = {...} etc. with
  

  @objects = Link.where(link_class: 'permission', head_uuid: params[:uuid])
  

• (This should also avoid a situation where it's impossible to get anything beyond 1 page of results.)

In Link#find_object_by_uuid

• I think it would be better to look up @objects here (instead of in get_permissions), or set @objects to nil. As it stands, the meaning of @objects is quite different depending on where you are in the filter chain / action code, which seems likely to confuse programmers (or code) in the future.

In integration tests

• assert_includes is nicer than assert [].include?() -- the test failure message will automatically include the details (what the haystack and needle were) instead of just "false": e.g., assert_includes perm_uuids, can_read_uuid, "can_read_uuid not found"
• Test /permissions with a non-admin user. Perhaps the :inactive user who just received can_manage permission? (I suspect the get_permissions method as written doesn't actually work for non-admins -- this test should resolve the question one way or the other.)

Tom Clegg wrote:

At 6f70a51

In ArvadosModel.lookup_by_id

• I like this convenience. But could this be find_by_uuid, to match the usual rails finders? (That way "acts like find_*(), not find_all_*()" would be more obvious, for example.) Perhaps this would work:

[...]

This would probably work. I chose not to use find_by_uuid specifically so that this wouldn't be interfering with the automagically defined Rails finders, because that seems riskier and more fragile. But I defer to your Rails expertise here, and if this seems like a reasonable approach to you I'll roll with it.

In Link#get_permissions

• I think we don't want find_objects_for_index: it applies the usual permission model, which we don't want, and applies filters/paging, which we have just gone out of our way to skip. Perhaps better to replace @where = {...} etc. with
  [...]
• (This should also avoid a situation where it's impossible to get anything beyond 1 page of results.)

It seems to me that get_permissions is conceptually an index method and it's best to mimic the existing index methods as much as possible. I guess that users are unlikely to apply any other filter conditions to permission searches so maybe that's overreaching. I do think paging is a good idea, but it can all probably wait until/unless we implement a PermissionController to treat permissions as first-class objects.

In Link#find_object_by_uuid

• I think it would be better to look up @objects here (instead of in get_permissions), or set @objects to nil. As it stands, the meaning of @objects is quite different depending on where you are in the filter chain / action code, which seems likely to confuse programmers (or code) in the future.

I don't think I follow. This is where the code populates @objects. I suppose it can set @objects to nil and just populate @object directly if that's what you'd prefer. Here again I'm trying to make get_permissions behave as much as possible like the existing "index" methods because that seemed like the right approach, but if we don't want to pretend that it's an index method then there's no advantage in making it act like one.

In integration tests

• assert_includes is nicer than assert [].include?() -- the test failure message will automatically include the details (what the haystack and needle were) instead of just "false": e.g., assert_includes perm_uuids, can_read_uuid, "can_read_uuid not found"
• Test /permissions with a non-admin user. Perhaps the :inactive user who just received can_manage permission? (I suspect the get_permissions method as written doesn't actually work for non-admins -- this test should resolve the question one way or the other.)

Thanks, I looked for assert_includes but did not find anything like it -- I was probably looking at Rails documentation for version 1.9.3.6.4.385.3.1 or something like that. And excellent point about a non-admin lookup for permissions, I should have caught that right off.

Tim Pierce wrote:

This would probably work. I chose not to use find_by_uuid specifically so that this wouldn't be interfering with the automagically defined Rails finders, because that seems riskier and more fragile. But I defer to your Rails expertise here, and if this seems like a reasonable approach to you I'll roll with it.

Yeah, I think "do X if caller is doing {something that would otherwise just crash}, otherwise do the normal Rails thing" is pretty safe here.

In Link#get_permissions

• I think we don't want find_objects_for_index: it applies the usual permission model, which we don't want, and applies filters/paging, which we have just gone out of our way to skip. Perhaps better to replace @where = {...} etc. with
  [...]
• (This should also avoid a situation where it's impossible to get anything beyond 1 page of results.)

It seems to me that get_permissions is conceptually an index method and it's best to mimic the existing index methods as much as possible. I guess that users are unlikely to apply any other filter conditions to permission searches so maybe that's overreaching. I do think paging is a good idea, but it can all probably wait until/unless we implement a PermissionController to treat permissions as first-class objects.

I think there are two ways we can go here:

1. Obey the usual limit/offset parameters.
2. Ignore/forbid limit/offset parameters. Always return all results.

I agree that paging doesn't seem essential here, so I thought the above "simple version" would be a good way to achieve way #2.

Currently I think we have an undesirable third way: "obey limit (or impose default limit 100), but ignore offset" -- but we can resolve that easily by avoiding find_objects_for_index in favor of a simple lookup.

(Looking further, even with way #2, I think we should set @offset=0 and @limit=@objects.count in get_permissions (or wherever we set @objects), to ensure render_list yields something more closely resembling reality. In the event some client code looks at the response to decide whether it should get page 2, it should come to the correct conclusion (i.e., "not necessary to get page 2").

In Link#find_object_by_uuid

• I think it would be better to look up @objects here (instead of in get_permissions), or set @objects to nil. As it stands, the meaning of @objects is quite different depending on where you are in the filter chain / action code, which seems likely to confuse programmers (or code) in the future.

I don't think I follow. This is where the code populates @objects. I suppose it can set @objects to nil and just populate @object directly if that's what you'd prefer. Here again I'm trying to make get_permissions behave as much as possible like the existing "index" methods because that seemed like the right approach, but if we don't want to pretend that it's an index method then there's no advantage in making it act like one.

Ah, sorry. That's my goal too, but I was unclear with the phrase "look up". I meant "...populate @objects with the objects that will eventually be returned to the client" -- as opposed to "populate @objects with something".

If I'm reading correctly, currently (for sake of argument, let's say we're looking up all permissions on a Specimen object) find_object_by_uuid sets @objects to an array containing [zero or] one Specimen object, then later on get_permissions sets @objects to nil, then calls find_objects_for_index, which sets @objects to an array containing Link objects.

I think it's clearer to make @objects stay nil -- rather than an array of other stuff -- until it gets the array of Link objects.

And yes, this is just a matter of saying

@object = ArvadosModel::....first

instead of

@objects = ArvadosModel::...
@object = @objects.first

Thanks, I looked for assert_includes but did not find anything like it -- I was probably looking at Rails documentation for version 1.9.3.6.4.385.3.1 or something like that.

fyi https://whatdoitest.com/rails-assertions-cheat-sheet

And excellent point about a non-admin lookup for permissions, I should have caught that right off.

(This is the more important reason for my suggestion to use a simple lookup instead of deferring to the usual find_objects_for_index...)

Changes at bbc3324:

• Link#get_permissions uses Link.where instead of find_objects_for_index; find_object_for_uuid just populates @object with the head_uuid object and leaves @objects alone.
• ArvadosModel.lookup_by_uuid renamed ArvadosModel.find_by_uuid and punts to the superclass if called as a subclass method.
• Test get_permissions_returns_list checks that the :active user can get permissions. Also uses the group fixtures for testing permissions instead of collections (because can_manage permissions only work so far on users and groups.

Tim Pierce wrote:

Changes at bbc3324:

• Link#get_permissions uses Link.where instead of find_objects_for_index; find_object_for_uuid just populates @object with the head_uuid object and leaves @objects alone.

Great

• ArvadosModel.lookup_by_uuid renamed ArvadosModel.find_by_uuid and punts to the superclass if called as a subclass method.

LGTM - except I still think we should set @offset=0 and @limit=@objects.count in get_permissions...?

• Test get_permissions_returns_list checks that the :active user can get permissions. Also uses the group fixtures for testing permissions instead of collections (because can_manage permissions only work so far on users and groups.

• Should "get_permissions returns 403 if user lacks manage permission" also get updated to use a group rather than a collection?
• Add a "get_permissions returns 404 if uuid exists but user lacks read permission" test?

At a9980e0. Thanks for your patience on this.

Tom Clegg wrote:

Tim Pierce wrote:

Changes at bbc3324:

• Link#get_permissions uses Link.where instead of find_objects_for_index; find_object_for_uuid just populates @object with the head_uuid object and leaves @objects alone.

Great

• ArvadosModel.lookup_by_uuid renamed ArvadosModel.find_by_uuid and punts to the superclass if called as a subclass method.

LGTM - except I still think we should set @offset=0 and @limit=@objects.count in get_permissions...?

Yeah, I missed that. I lost track that render_list would be affected by these.

• Test get_permissions_returns_list checks that the :active user can get permissions. Also uses the group fixtures for testing permissions instead of collections (because can_manage permissions only work so far on users and groups.

• Should "get_permissions returns 403 if user lacks manage permission" also get updated to use a group rather than a collection?

Yes, for consistency they should all use groups. Done.

• Add a "get_permissions returns 404 if uuid exists but user lacks read permission" test?

Yes, added this. We now test explicitly for nonexistent UUID, unreadable UUID, and readable but unmanageable UUID.

Tim Pierce wrote:

At a9980e0. Thanks for your patience on this.

NP! Looks great, please merge.