Story #2044

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

Added by Tom Clegg over 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
-
Target version:
Start date:
05/06/2014
Due date:
% Done:

100%

Estimated time:
(Total: 7.00 h)
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.
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

Task #2665: API server lets all known users list basic information about other usersResolvedBrett Smith

Task #3217: Review 2044-api-users-index-wipResolvedRadhika Chippada

Task #2749: Workbench has interface to share folders between users of instanceResolvedBrett Smith

Task #3238: Review 2044-workbench-project-sharing-wipResolvedBrett Smith

Associated revisions

Revision 651638a2
Added by Peter Amstutz about 6 years ago

Merge branch 'master' into 2044-share-button refs #2044

Revision ac97e708
Added by Brett Smith almost 6 years ago

Merge branch '2044-api-users-index-wip'

Refs #2044. Closes #3217.

Revision 2fccbc1d
Added by Brett Smith almost 6 years ago

Merge branch '2044-workbench-project-sharing'

Closes #2044, #2749, #3238.

Revision 5fdfe4ca (diff)
Added by Tom Clegg almost 6 years ago

3170: Use find().all() instead of page.all() to accommodate ajax content. refs #3170, #2044

Revision eea48f38 (diff)
Added by Brett Smith almost 6 years ago

2044: Workbench resource lists support Enumerable-style select.

Refs #2044. Enumerable-style select is used in places like the groups
index.

Revision 4d0866e5 (diff)
Added by Brett Smith almost 6 years ago

2044: Make project sharing tests more forgiving of Ajax timing.

Apparently waiting our default 5 seconds is not always sufficient to
get an Ajax response that requires a roundtrip to the API server.
Wait longer in these cases. Not the most elegant solution, but I
don't see a better option. Refs #2044.

Revision fa3f6860 (diff)
Added by Brett Smith almost 6 years ago

2044: Improve reporting when project sharing fails.

Our select modal knows how to render an errors key in the JSON
response. This commit adjusts the project controller to conform to
this convention. Refs #2044, #3200.

History

#1 Updated by Tom Clegg over 6 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg over 6 years ago

  • Story points set to 2.0

#3 Updated by Tom Clegg over 6 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.

#4 Updated by Peter Amstutz over 6 years ago

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

#5 Updated by Tom Clegg over 6 years ago

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

#6 Updated by Tom Clegg about 6 years ago

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

#7 Updated by Tom Clegg about 6 years ago

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

#8 Updated by Brett Smith about 6 years ago

  • Assigned To set to Brett Smith

#9 Updated by Tom Clegg about 6 years ago

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

#10 Updated by Tom Clegg about 6 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)

#11 Updated by Brett Smith about 6 years ago

  • Assigned To set to Brett Smith

#12 Updated by Brett Smith about 6 years ago

  • % Done changed from 100 to 0

#13 Updated by Brett Smith about 6 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

#14 Updated by Tom Clegg about 6 years ago

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

#15 Updated by Tom Clegg about 6 years ago

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

#16 Updated by Tom Clegg almost 6 years ago

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

#17 Updated by Tom Clegg almost 6 years ago

  • Description updated (diff)

#18 Updated by Brett Smith almost 6 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.

#19 Updated by Brett Smith almost 6 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.

#20 Updated by Radhika Chippada almost 6 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?

#21 Updated by Brett Smith almost 6 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.

#23 Updated by Tim Pierce almost 6 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?

#24 Updated by Tom Clegg almost 6 years ago

  • Status changed from New to In Progress

#25 Updated by Ward Vandewege almost 6 years ago

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

#26 Updated by Ward Vandewege almost 6 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

#27 Updated by Brett Smith almost 6 years ago

  • Story points changed from 3.0 to 1.0

Adjusting story points for the sprint carryover.

#28 Updated by Brett Smith almost 6 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.

#29 Updated by Brett Smith almost 6 years ago

  • Target version set to 2014-08-06 Sprint

#30 Updated by Brett Smith almost 6 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.

#31 Updated by Tom Clegg almost 6 years ago

  • Status changed from New to In Progress

#32 Updated by Tim Pierce almost 6 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.

#33 Updated by Brett Smith almost 6 years ago

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

Applied in changeset arvados|commit:2fccbc1d172fe4bd680651261adfdca8f1ba2a63.

Also available in: Atom PDF