Project

General

Profile

Actions

Idea #2044

closed

[Workbench] interface lets users share a project folder with other users on this instance

Added by Tom Clegg about 10 years ago. Updated over 9 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
1.0

Description

Acceptance criteria:
  • "Share" tab shows a list of users and groups who can already read/write this group.
  • "Add User" and "Add Group" buttons bring up our usual chooser. You can make multiple selections from here. When confirmed, they all get read permission, and the tab is reloaded to render them.
    • This requires changes to users.list api permissions, see #2665.
    • Workbench needs a special "create bulk permissions" method/route to support this.
  • Click a "trash" or "x" button to un-share with a listed user/group.
  • Click a drop-down to change permission type (read, modify, manage permissions).
  • Screen captures for demonstration purposes.
Not necessary [yet]:
  • Don't auto-refresh while the dialog is open, or worry about race conditions with other users/windows.
Aside: This is a special case of a generic "Share" mechanism needed in Workbench.
  • Collections do not have "write" permission.
  • Collections are always owned by system_user.
  • When you create a collection, you get a free permission/can_read link which is owned by system_user.
  • Other than Collections, everything else should act pretty much the same as Groups. So -- hopefully sooner rather than later -- none of the Javascript or Workbench-server support will be specific to Groups. (But if it makes the difference between getting it done or not, "only groups are essential for this story" is the rule.)
Other notes:
  • You can't necessarily tell whether another user is a member of a group.

Files

Workbench Sharing 1.png (101 KB) Workbench Sharing 1.png Sharing tab overview Brett Smith, 07/15/2014 05:12 PM
Workbench Sharing 3.png (102 KB) Workbench Sharing 3.png Share with other users Brett Smith, 07/15/2014 05:12 PM
Workbench Sharing 2.png (104 KB) Workbench Sharing 2.png Modify existing shares Brett Smith, 07/15/2014 05:12 PM
Workbench Sharing 4.png (96.9 KB) Workbench Sharing 4.png Search for new sharers, and select multiple Brett Smith, 07/15/2014 05:12 PM

Subtasks 4 (0 open4 closed)

Task #2665: API server lets all known users list basic information about other usersResolvedBrett Smith06/30/2014Actions
Task #3217: Review 2044-api-users-index-wipResolvedRadhika Chippada07/09/2014Actions
Task #2749: Workbench has interface to share folders between users of instanceResolvedBrett Smith05/06/2014Actions
Task #3238: Review 2044-workbench-project-sharing-wipResolvedBrett Smith05/06/2014Actions
Actions #1

Updated by Tom Clegg about 10 years ago

  • Description updated (diff)
Actions #2

Updated by Tom Clegg about 10 years ago

  • Story points set to 2.0
Actions #3

Updated by Tom Clegg about 10 years ago

  • Subject changed from Share a collection with other users on this instance by clicking a Share button in Workbench and selecting users and groups. to Share a collection (or any other object) with other users on this instance by clicking a Share button in Workbench and selecting users and groups.
Actions #4

Updated by Peter Amstutz about 10 years ago

  • Target version deleted (2014-03-05 Data management)
Actions #5

Updated by Tom Clegg about 10 years ago

  • Target version set to 2014-05-07 Storing and Organizing Data
Actions #6

Updated by Tom Clegg about 10 years ago

  • Description updated (diff)
  • Story points changed from 2.0 to 3.0
Actions #7

Updated by Tom Clegg about 10 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-04-16 Dev tools and data/resource management
Actions #8

Updated by Brett Smith about 10 years ago

  • Assigned To set to Brett Smith
Actions #9

Updated by Tom Clegg almost 10 years ago

  • Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data
Actions #10

Updated by Tom Clegg almost 10 years ago

  • Subject changed from Share a collection (or any other object) with other users on this instance by clicking a Share button in Workbench and selecting users and groups. to Share a project (group) with other users on this instance by clicking a Share button in Workbench and selecting users and groups.
  • Description updated (diff)
  • Assigned To deleted (Brett Smith)
Actions #11

Updated by Brett Smith almost 10 years ago

  • Assigned To set to Brett Smith
Actions #12

Updated by Brett Smith almost 10 years ago

  • % Done changed from 100 to 0
Actions #13

Updated by Brett Smith almost 10 years ago

  • Subject changed from Share a project (group) with other users on this instance by clicking a Share button in Workbench and selecting users and groups. to Workbench interface lets users share a project folder with other users on this instance
Actions #14

Updated by Tom Clegg almost 10 years ago

  • Target version deleted (2014-05-07 Storing and Organizing Data)
Actions #15

Updated by Tom Clegg almost 10 years ago

  • Target version set to 2014-06-17 Curating and Crunch
Actions #16

Updated by Tom Clegg almost 10 years ago

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

Updated by Tom Clegg over 9 years ago

  • Description updated (diff)
Actions #18

Updated by Brett Smith over 9 years ago

  • Description updated (diff)

Updated description per discussion with Tom. We want to reuse more existing Workbench UI components. The previous proposal is very slick but doesn't work like anything else in Workbench.

Actions #19

Updated by Brett Smith over 9 years ago

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.

Actions #20

Updated by Radhika Chippada over 9 years ago

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?

Actions #21

Updated by Brett Smith over 9 years ago

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.

Actions #23

Updated by Tim Pierce over 9 years ago

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?
Actions #24

Updated by Tom Clegg over 9 years ago

  • Status changed from New to In Progress
Actions #25

Updated by Ward Vandewege over 9 years ago

  • Target version changed from 2014-07-16 Sprint to 2014-08-06 Sprint
Actions #26

Updated by Ward Vandewege over 9 years ago

  • Subject changed from Workbench interface lets users share a project folder with other users on this instance to [Workbench] interface lets users share a project folder with other users on this instance
Actions #27

Updated by Brett Smith over 9 years ago

  • Story points changed from 3.0 to 1.0

Adjusting story points for the sprint carryover.

Actions #28

Updated by Brett Smith over 9 years ago

  • Status changed from In Progress to New
  • Target version deleted (2014-08-06 Sprint)

Tim Pierce wrote:

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?

Yes it should. I taught Workbench how to query the new API /permissions method, and changed this implementation to be based on that. Since it only works for users who manage the queried object, we can figure out whether or not the user manages the folder by the query's success.

  • When share_with gets an error when creating a permission link, is there useful error text it can return to the user?

Yes there is. I changed the response to map failed UUIDs to error messages reported back by the API server.

  • apps/workbench/test/integration/projects_test.rb
    • Obsolete(?) line commented out -- remove?

Thanks for that catch. Done, along with some other small test improvements.

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

Agreed. Added as a projects controller test.

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

Good catch. As it turns out, it's hard to test only this, because trying to do this already causes other validation failures (from ensure_owner_uuid_is_permitted). Still, I made the addition and added test to ensure this behavior is preserved.

Ready for another look at e0e4f50. Thanks.

Actions #29

Updated by Brett Smith over 9 years ago

  • Target version set to 2014-08-06 Sprint
Actions #30

Updated by Brett Smith over 9 years ago

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.

Actions #31

Updated by Tom Clegg over 9 years ago

  • Status changed from New to In Progress
Actions #32

Updated by Tim Pierce over 9 years ago

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.

Actions #33

Updated by Brett Smith over 9 years ago

  • % Done changed from 54 to 100
  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:2fccbc1d172fe4bd680651261adfdca8f1ba2a63.

Actions

Also available in: Atom PDF