Story #2044
closed[Workbench] interface lets users share a project folder with other users on this instance
Added by Tom Clegg almost 11 years ago. Updated over 10 years ago.
100%
Description
- "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.
- This requires changes to
- 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.
- Don't auto-refresh while the dialog is open, or worry about race conditions with other users/windows.
- 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.)
- 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 |
Updated by Tom Clegg almost 11 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.
Updated by Peter Amstutz almost 11 years ago
- Target version deleted (
2014-03-05 Data management)
Updated by Tom Clegg almost 11 years ago
- Target version set to 2014-05-07 Storing and Organizing Data
Updated by Tom Clegg almost 11 years ago
- Description updated (diff)
- Story points changed from 2.0 to 3.0
Updated by Tom Clegg over 10 years ago
- Target version changed from 2014-05-07 Storing and Organizing Data to 2014-04-16 Dev tools and data/resource management
Updated by Tom Clegg over 10 years ago
- Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data
Updated by Tom Clegg over 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)
Updated by Brett Smith over 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
Updated by Tom Clegg over 10 years ago
- Target version deleted (
2014-05-07 Storing and Organizing Data)
Updated by Tom Clegg over 10 years ago
- Target version set to 2014-06-17 Curating and Crunch
Updated by Tom Clegg over 10 years ago
- Target version changed from 2014-06-17 Curating and Crunch to 2014-07-16 Sprint
Updated by Brett Smith over 10 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.
Updated by Brett Smith over 10 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.
Updated by Radhika Chippada over 10 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?
Updated by Brett Smith over 10 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.
Updated by Brett Smith over 10 years ago
- File Workbench Sharing 1.png Workbench Sharing 1.png added
- File Workbench Sharing 2.png Workbench Sharing 2.png added
- File Workbench Sharing 3.png Workbench Sharing 3.png added
- File Workbench Sharing 4.png Workbench Sharing 4.png added
Updated by Tim Pierce over 10 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 exposeUser.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?
- It looks like
- 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?
Updated by Ward Vandewege over 10 years ago
- Target version changed from 2014-07-16 Sprint to 2014-08-06 Sprint
Updated by Ward Vandewege over 10 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
Updated by Brett Smith over 10 years ago
- Story points changed from 3.0 to 1.0
Adjusting story points for the sprint carryover.
Updated by Brett Smith over 10 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.
Updated by Brett Smith over 10 years ago
- Target version set to 2014-08-06 Sprint
Updated by Brett Smith over 10 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.
Updated by Tim Pierce over 10 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.
Updated by Brett Smith over 10 years ago
- % Done changed from 54 to 100
- Status changed from In Progress to Resolved
Applied in changeset arvados|commit:2fccbc1d172fe4bd680651261adfdca8f1ba2a63.