Story #2873

Permission links are owned by root; ability to lookup/modify is determined by current user permission on "head" object

Added by Tom Clegg about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Target version:
Start date:
06/16/2014
Due date:
% Done:

100%

Estimated time:
(Total: 25.00 h)
Story points:
2.0

Description

Todo
  • Set owner_uuid to system_user_uuid on all permission links. This prevents users from having permission to view/alter permission links by virtue of having created them.
  • Add get_permissions action to API server's ApplicationController. It accepts a single uuid and responds with a list of all permission links whose head_uuid is equal to the specified uuid.
    • respond 404 if the object with the specified uuid does not exist or is not readable by the current user (using the same before_filter stuff as the "show" action should take care of this)
    • respond 403 if the current user does not have manage permission on the specified uuid or the referenced object's owner_uuid

Subtasks

Task #3075: Review 2873-permission-links-ownershipResolvedTom Clegg

Task #2921: Link validators use head_uuid to determine permission to lookup or modifyResolvedTim Pierce

Task #2920: Link.create and Link.update sets link ownership to root when link_class is 'permission'ResolvedTim Pierce

Associated revisions

Revision 975b1912 (diff)
Added by Tim Pierce about 5 years ago

2873: permission links are owned by root

Permission links are themselves "owned" by root. (Technically, the
owner_uuid on permission links will be ignored when determining a user's
permission to modify an object -- ensuring that it is always root
provides consistency, and helps to flush out any permission edge cases
early in testing.)

ArvadosModel.ensure_owner_uuid_is_permitted and
Link.permission_to_attach_to_objects grant permission to modify
permission links based on the user's relationship to the link's head.

New methods:
  • User.owns? determines ownership of an object.
  • User.can_manage? determines whether a user has permission to manage an object.
  • ArvadosModel.has_permission? determines whether an object has been
    granted a particular permission on a specified target.

Refs #2873.

Revision 6f70a514 (diff)
Added by Tim Pierce about 5 years ago

2873: add /permissions API method

The /permissions/:uuid method will return a list of all permissions that
the current user is allowed to see on the given uuid.

  • New method LinksController::get_permissions, with a route from
    /arvados/v1/permissions.
  • LinksController overrides find_object_by_uuid to permit looking up a
    uuid in any class, when called by get_permissions.
  • Moved link permission checking to Link.ensure_owner_uuid_is_permitted.
  • Use current_user.can? to check the user's permission on head_uuid.
    Removed unnecessary owns? and can_manage? code.
  • Unit tests:
    • test/integration/permissions_test.rb: added tests: * "get_permissions returns list" * "get_permissions returns 404 for nonexistent uuid" * "get_permissions returns 403 if user lacks manage permission"
    • test/unit/link.rb: test that only permission and name links have
      their ownership changed upon save.
    • test/unit/permission_test.rb: test the following scenario: when user
      "active" owns a group G which can_manage another group H, then
      active user is permitted to create permission links directly on
      objects in group H.

Refs #2873.

2873: perms

Revision bbc3324f (diff)
Added by Tim Pierce about 5 years ago

2873: changes for code review

Incorporating code review comments:

  • 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.

(refs #2873)

Revision a9980e09 (diff)
Added by Tim Pierce about 5 years ago

2873: more code review changes

  • get_permissions sets @offset and @limit explicitly to ensure that
    render_list does the right thing.
  • Tests updated to use permission links on Groups (permissions are not
    yet working for other objects)
  • Added tests for "uuid exists but is unreadable" and "uuid is readable
    but not manageable"

Refs #2873.

Revision 5126d94f
Added by Tim Pierce about 5 years ago

Merge branch '2873-permission-links-ownership'

Closes #2873. Huzzah!

History

#1 Updated by Tom Clegg about 5 years ago

  • Subject changed from Permission links are owned by root; ability to change them is determined by current user permission on "head" object to Permission links are owned by root; ability to lookup/modify is determined by current user permission on "head" object

#2 Updated by Tim Pierce about 5 years ago

  • Assigned To set to Tim Pierce

#3 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#4 Updated by Tom Clegg about 5 years ago

  • Target version changed from 2014-06-17 Curating and Crunch to 2014-07-16 Sprint

#5 Updated by Tim Pierce about 5 years ago

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.

#6 Updated by Tom Clegg about 5 years ago

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.

#7 Updated by Tim Pierce about 5 years ago

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.

#8 Updated by Tom Clegg about 5 years ago

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.)

#9 Updated by Tim Pierce about 5 years ago

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.

#10 Updated by Tom Clegg about 5 years ago

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...)

#11 Updated by Tim Pierce about 5 years ago

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.

#12 Updated by Tom Clegg about 5 years ago

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?

#13 Updated by Tim Pierce about 5 years ago

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.

#14 Updated by Tom Clegg about 5 years ago

Tim Pierce wrote:

At a9980e0. Thanks for your patience on this.

NP! Looks great, please merge.

#15 Updated by Tim Pierce about 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 96 to 100

Applied in changeset arvados|commit:5126d94fd644a657243e5ec80d5ef1fc250f8b76.

Also available in: Atom PDF