Arvados

2044-api-users-index-wip is up for review. It implements the API server part of this story, making limited information about all users available through the index method.

Review comments for branch: 2044-api-users-index-wip

Brett, everything looked good. Just a couple very minor comments. You can address only if you think they add value. Thanks.

- api -> users_controller.rb
find_objects_for_index method — i think it would make it easier to understand if you add extra parentheses for the rather complex second conditional in the following:

if (action_name == "index") and not (@read_users.any? { |u| u.is_admin })

- test “user with project read permission can’t remove items from it”
should this be post :update or is the title of this test misleading?

- does it make sense to add tests to test that a user with project read permission (1) cannot add additional objects such as a subproject to it, (2) update a project title or description, (3) delete it?

Radhika Chippada wrote:

- api -> users_controller.rb
find_objects_for_index method — i think it would make it easier to understand if you add extra parentheses for the rather complex second conditional in the following:

if (action_name == "index") and not (@read_users.any? { |u| u.is_admin })

Sure. I added parentheses, including the "not" because I felt like that better captured the whole idea of the condition.

- test “user with project read permission can’t remove items from it”
should this be post :update or is the title of this test misleading?

It is post :update, and that's correct. When an item belongs to a folder, the folder UUID becomes the item's owner_uuid. Conversely, the way to remove an item from a folder is to change its owner_uuid back to some user. That's what this test tries to do, and checks for failure.

- does it make sense to add tests to test that a user with project read permission (1) cannot add additional objects such as a subproject to it, (2) update a project title or description, (3) delete it?

Sure thing, added. Please take another look at 043a68c. Thanks.

Reviewing 2044-workbench-project-sharing-wip at 5ff17b2:

• apps/workbench/app/controllers/projects_controller.rb
  • It looks like managed_by_user? will return true if @object is owned by the user, but will return false if @object is in a group that is owned by the user (or a group that the user can_manage, etc). Should this be more robust? (Should we expose User.can? through the API server to Workbench?)
  • When share_with gets an error when creating a permission link, is there useful error text it can return to the user?
• apps/workbench/app/helpers/application_helper.rb
  • I actually think that, while the fa-male icon is unfortunately named, the image is sufficiently ungendered as to be suitably inclusive for a generic "human" icon. :-)
• apps/workbench/test/integration/projects_test.rb
  • Obsolete(?) line commented out -- remove?
  • We should test not just that the workbench page doesn't offer the appropriate sharing buttons, but that if the user submits URLs to attempt unauthorized sharing anyway, the request is denied (i.e. guard against an attacker constructing requests for sharing projects by hand and submitting them via curl). OK to add this as a followup ticket.
• services/api/app/models/link.rb
  • skip_uuid_read_permission_check -- this should apply only to permission links and not to other kinds of links, right?

After discussion with Tom and Tim, we decided:

• It's okay (preferable, even) to show no sharing information to users who don't manage the project.
• The API server should not be quite so forthcoming about returning permission links.

The latest updates at 3fcd8f1 reflect these decisions. Please review this version.

One last thing: I think the new tests in services/api/test/unit/link_test.rb should explicitly set_user_from_auth(:active) to make it clear we're attempting to create these permissions as the active user.

Otherwise, LGTM. Thanks for these changes.