Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to share a Tale for active collaboration #16

Merged
merged 20 commits into from Dec 17, 2020
Merged

Conversation

bodom0015
Copy link
Member

@bodom0015 bodom0015 commented Mar 23, 2020

Problem

A Tale can be either private or public, but this does not allow multiple users to actively collaborate on the same Tale at the same time.

Approach

Girder offers a set of ACLs for each resource, and we can leverage their API to offer a familiar interface for our users.

Add the Run > Share tab, allowing for more fine-grained management of sharing and permissions.

Demonstration: https://recordit.co/E7JNi9hKwl

Fixes #13

How to Test

  1. Checkout and run this branch locally, rebuild the dashboard
  2. Login to the WholeTale Dashboard
  3. Create a Tale, if you haven't already - you don't need to launch/run the Tale
  4. Navigate to the Run view for this Tale
    • You should see a new Share tab, indicating the number of collaborators the Tale currently has
  5. Navigate to Run > Share
    • You should see radio buttons allowing you to toggle whether the Tale is currently PRIVATE or PUBLIC
    • You should see the Tale Owner listed here, along with their gravatar
    • You should see a form at the bottom allowing you to invite new collaborators
  6. Toggle Tale to PUBLIC and refresh the page
    • PUBLIC should still be selected after reload
  7. Toggle Tale back to PRIVATE and refresh the page
    • PRIVATE should still be selected after reload
  8. Under "Collaborators", click on the user search
    • You should see results appear listing all users
    • NOTE: You will NOT see the Tale owner listed in the collaborators list - removing them from the ACLs gets rid of their access, so we hide them from view to make sure the user can't remove them or modify their access
  9. Enter a search query to filter users by name or login, and pick a user
    • You should see the results filter appropriately
    • Upon selecting a user, the "Add" button should be enabled
    • NOTE: You will NOT see the Tale owner listed in the search results - removing them from the ACLs gets rid of their access, so we hide them from view to make sure the user can't remove them or modify their access
  10. Choose a user, select "Can Edit" from the dropdown, and click "Add"
    • You should see the user appear in the Collaborators list below
    • You should see that the dropdown beside the user matches the permission you assigned
  11. Refresh the page
    • The view should reload exactly the same
  12. Toggle the user you added above from "Can Edit" to "Can View" and refresh the page again
    • The user should still have the "Can View" permission, as expected

@craig-willis
Copy link
Contributor

@bodom0015 is this intended to just cover the sharing page functionality or also that sharing is working as expected via the UI? When I shared a tale with either read or edit permissions to a second user, it doesn't appear on either Public or My Tales.

My Tales is filtering based on creator:

return value.filter(tale => user && user._id && tale.creatorId === user._id);

Public Tales is filtering based on the public flag:

return value.filter(tale => tale.public);

The tale is technically neither public nor mine. Do we need a "Shared with me" option that filters based on access level? Or should we re-interpret Public as "things I can read" (per @Xarthisius tale => tale._accessLevel < 1 || tale.public) and Mine as "things I can edit" (tale => tale._accessLevel >=1).

Note, there are a couple of ways to setup a second user for testing. First, via girder-shell (thanks to @Xarthisius):

from girder.models.user import User
from girder.models.token import Token
user = {'email': 'joe@dev.null', 'login': 'joeregular', 'firstName': 'Joe', 'lastName': 'Regular', 'password': 'secret'}
user = User().createUser(**user)
Token().createToken(user)

Then go to https://dashboard.local.wholetale.org/login?token=<_id>

Another option is to setup a secondary Google or ORCID account that isn't attached to your main email. I setup ORCID with my gmail and it works fine.

@bodom0015
Copy link
Member Author

bodom0015 commented Nov 12, 2020

@craig-willis that is an excellent point, and a great catch on the filtering stuff! I must not have tested this nearly as thoroughly as I had thought :X

In the short-term (for testing), I've made the edits you suggested above in the two pipes.

  • My Tales => Anything with _accessLevel >= 1 (aka editable Tales)
  • Public Tales => Anything with _accessLevel < 1 (aka read-only Tales), or Tales explicitly marked as public

I'm not sure what would be the right solution going forward. It should be easy enough to add a new tab there for "Shared with Me", or to simply adjust the existing views/labels to reflect the idea of Read-only vs Write access.

Any preference to the long-term approach here?

@ThomasThelen
Copy link
Member

Pretty awesome feature!

Scrolling

I noticed that scrolling on the Share page is slightly different than the metadata page; the metadata page has the scroll box above the footer. In the the Share page the footer will take up real estate as you scroll down

Invitations

When I shared my Tale with my test user I saw Invitation sent... I was expecting to see an email in my test user's email or a notification asking if I wanted to accept the invitation. If it's a non-consensual share I would replace it with Shared! or something similar (opinion).

Small Error

When viewing the Share page I see a Cannot read property '_accessLevel' of undefined message pop up at the bottom of the page. I don't see any errors in the dev console and I didn't see the error in your screen recording. When running on Firefox I see an 'e.tale is undefined', so this may be a deployment specific thing?

Editable vs View Only

I like the idea of the third tab, 'Shared with Me' that has both editable and read only Tales that were shared with the user. I'm not sure how feasible it is to group those two together based on ACL information. I think that it would be safe to assume that if the Tale...

  1. Is editable and created by a user other than the current one, it's shared
  2. If the Tale is read only, private, and (need some other feature here), it's shared

@Xarthisius
Copy link
Contributor

A few random thoughts:

UI

  • Dropdown with ACLs that's next to Add button is inactive and probably should be removed
  • "Invitation sent" message should be removed. We're adding ACLs for existing users and invitations were meant only for people without accounts. Unfortunately the latter case is not possible to deal with on the backend yet.
  • Minor styling issues: buttons are larger than input field, buttons with Edit/view in the bottom table should probably have equal width
    sharing

General behavior

  • After adding 2 users to a Tale (one with view and one with edit rights), both of them get 403 while navigating to the Tale, cause dashboard tries to call GET /tale/:id/access (that call should only happen if _accessLevel >= 2 for a given Tale)
  • Write access doesn't allow me to upload files into the workspace (even though button is active). Even though permissions are set correctly and operation succeeds in girder UI.

@craig-willis
Copy link
Contributor

Overall this looks good. I like the idea of a separate "Shared with me" tab instead of conflating shared and public/mine.

I ran a few additional tests of general sharing behavior using 2 accounts:

  • As user 1, create tale "Sharing test", upload file to workspace, register dataset (10.5065/D6862DM8) and add file (us2000.xlsx). Share with user 2 with view permissions
  • As user 2, confirm that you can see the tale, registered data and uploaded file appear.
  • As user 2, confirm that you cannot edit sharing, edit metadata, edit external data, upload to workspace.
  • As user 2, confirm that you Run results in copy on launch, that the tale is copied, that external data and workspace files are present, and that the user can run the tale.
  • As user 2, delete the copy
  • As user 1, change user 2 permissions to edit
  • As user 2, confirm that you can edit metadata, edit external data, upload to workspace, and run the tale without copying.

What I found:

@bodom0015
Copy link
Member Author

bodom0015 commented Nov 18, 2020

Thank you for pointing these issues out. Apparently, I have been mixing up the magic numbers and using accessLevel=2 to mean either Write or Admin permission in different places. I've abstracted this to a real model for better clarity, so hopefully this helps me keep things straight going forward.

Permissions for operations are broken down as follows (please, @Xarthisius, correct me if this does not match backend assumptions):

  • Writeable File Operations (e.g. Upload, Create Folder, etc) => Write or Admin
  • Run Tale (Directly) => Write or Admin
  • Run Tale (Copy on Launch) => Read or None
  • Delete Tale => Admin

I also went ahead and added a "Shared with Me" tab to the Tale Catalog, which did require a bit of refactoring to the running-tales panel to reuse that code again there. Really I should have done this a long time ago, and am glad to finally be given a reason to do so:

  • Public Tales => shows any Tales with AccessLevel.None or public=true
  • My Tales => shows any Tales with AccessLevel.Admin
  • Shared with Me => shows any Tales with AccessLevel.Read or AccessLevel.Write

I've also fixed the "Return to Dashboard" and "Close" buttons to handle directing the user to this new tab appropriately.


NOTE 1: I think in the long term, the tab / search bars on the Tale Catalog should go below the Running Tales panel (since this panel appears the same in all three tabs, and is not affected by search), so migrating it to a component now should make it trivial to move the component around the view later on.

NOTE 2: there was some weird git nonsense happening here where it deleted some files (e.g.tale-catalog.component.html) and added some others (e.g. api/api-configuration.ts). I did my best to fix things, but let me know if you notice anything that is missing or seems out-of-place

@Xarthisius
Copy link
Contributor

Xarthisius commented Nov 23, 2020

I tried to test issue reported by Craig, but I run into a different problem. When sharing with a user ACLs are totally overwritten, instead of appended, and I lose access to my own Tale. At this point you get 403 error while in tale view and all the bad things happen.

Redeployed and it went away...

@bodom0015
Copy link
Member Author

bodom0015 commented Nov 24, 2020

Added a commit to fix file uploads, which includes handling chunking large files properly and not using the hidden chunk parameter on POST /file. I suspect this may have been what was giving back a 403 initially? I can add a check in the frontend (and I would advise the same in the backend) to prevent the Admin/owner of a Tale from being removed from its own ACLs.

Bonus feature: UI/browser no longer crashes with OOM when you upload very large files (tested up to 9GB file uploads - see #13)

I now have 2 users with Tales that they have shared with each other. User A created a tale called shared tale, and User B can View the Tale and upload files to its Workspace even without being able to see the "Share" tab:
http://recordit.co/tGChTEmwpy

One bug I am still noticing (as you'll see in the GIF above, and I'm pretty sure it has something to do with the new upload code) is that I cannot seem to upload files with duplicate names. For example, I already have a file named 100m.dat, and the new one never shows up after the upload finishes. I will also need to add progress indicators / cancel buttons for files that are currently being uploaded, and examine feature-parity with Girder for edge cases like refreshing the page during an upload, but this can happen in further issues/PRs.

@craig-willis
Copy link
Contributor

@bodom0015 this is working better for me. "Shared with me" makes sense and works as expected. I'm able to upload files. The permission issues I reported also seem to be fixed.

In my testing today, I'm seeing one problem and wanted to follow up on the "reload" discussion from Monday.

The problem I'm seeing is when adding a user with Edit permission, they are added with View only. To repeat:

  • Search for user
  • Select "Edit" in drop down
  • Select "Add"
  • Expected: User is added with Edit permissions
  • Actual: User is added with View permissions

Screen Shot 2020-11-25 at 1 29 02 PM

On the "reload" topic: when a tale is shared between two users, in almost every action I've tried the user needs to reload the tale, not simply toggle between tabs. For example. This includes when the tale is first shared with them; updating metadata; adding registered data; and adding files to the workspace. I can't simply toggle between My Tales and Shared Tales or Workspace and External Data or Files and Metadata as @Xarthisius suggested on the last dev call. This certainly merits another issue, but wanted to raise here to see if it's worth me filing.

@ThomasThelen
Copy link
Member

Miscellaneous use cases that I've gone though and tested. Noting these to keep track of what I cover, for others in case it inspires a new case, some might be good for the test plan updates. I've been treating User A as my chrome window with my main account and User B as a test account using Firefox.

  • User B can delete a copy of a shared Tale from User A
  1. Create a Tale with User A
  2. Share Tale with User B (read only)
  3. Copy & Launch with User B
  4. Make Tale public (User B)
  5. See that the Tale is public for user A
  6. Delete the Tale (User B)
  7. Refresh User B's page
  8. See that copied Tale is removed from public for User A
  • User A can delete a Tale shared with User B
  1. Create a Tale with User A
  2. Share Tale with User B (read only)
  3. Delete the Tale with User A
  4. Refresh User B's page
  5. See that the Tale disappears from User B's view
  • User A's changes to a shared Tale are propagated to User B's Tale
  1. Create a Tale with User A
  2. Share Tale with User B (read only)
  3. Change Tale properties & files with User A
  4. Refresh User B's page (switching to view files should update without refresh)
  5. See changes in User B's Tale
  • User B cannot delete a Tale that was shared by user A (read+edit)
  1. Create a Tale with User A
  2. Share Tale with User B (read only, then repeat with edit)
  3. Attempt to delete the shared tale from User B
  4. See that the Tale cannot be deleted
  • User B's changes to a shared Tale (edit) from User A propagates to User A
  1. Create a Tale with User A
  2. Share Tale with User B (Edit)
  3. Edit the Tale's metadata (add a description)
  4. Refresh User A's page
  5. Confirm metadata reflect's User B's changes
  6. Using User B, upload a file to the workspace
  7. Refresh the page with User A
  8. See the new file in the workspace

From the tests above, the only thing that stuck out was that I didn't get a message telling me I can't delete a Tale that's been shared with me (User B cannot delete a read only Tale that was shared by user A) test case.

@Xarthisius
Copy link
Contributor

This looks really good! General issues from my previous comment are now gone \o/ Share tab still needs UI fixes I mentioned there though. I additionally found that you can add a single user multiple times:

wt

which is something we should probably not allow.

@bodom0015
Copy link
Member Author

I pushed a couple new commits to address the last few remaining issues here:

  • @Xarthisius good catch! I had thought I handled this, but apparently Semantic UI's search component is very picky. It maintains its own internal cache which needed to be cleared even when searching a static source (and no documentation about this case whatsoever)
  • @craig-willis good catch! There appeared to be an issue with binding of the newCollabAccess variable that is supposed to be populated from that dropdown. This variable was setting 0 every time, so adding a user with "Can Edit" would assign an incorrect permission level of 0
  • @ThomasThelen good catch! This was an oversight on my part.. on the "Public Tales" view, we hide the remove icon if the user has improper permissions to delete a Tale. I've made sure that all three tabs have consistent behavior there now, as the tale-card should probably be refactored into its own reusable component anyway

@Xarthisius
Copy link
Contributor

@bodom0015 could you please fix the conflict?

FWIW I created https://gist.github.com/Xarthisius/fdab84ede710ddd557498fcfd6ae8c64 for an easy way of adding users to test this PR. Script spits out dashboard link that's pasteable

@craig-willis
Copy link
Contributor

craig-willis commented Dec 8, 2020

Retesting now and noticed a few issues I didn't catch before:

  • If user1 shares a tale with user2 with edit permissions and user2 runs the tale, it no longer appears in the catalog. I expect this is because the "Shared with me" is not showing running tales? User2 can still access the tale via run/taleId route.
  • If a tale is shared by user1 to user2 with "View" permissions, user2's File's tab doesn't display Home
  • When "Shared with Me" is selected on the catalog, the window title is PUBLIC.CATALOG.SHARED.PAGE_TITLE

@Xarthisius
Copy link
Contributor

  • If a tale is shared by user1 to user2 with "View" permissions, user2's File's tab doesn't display Home

That's consistent with other readonly tale that you can fork (like public tales).

@ThomasThelen
Copy link
Member

When "Shared with Me" is selected on the catalog, the window title is PUBLIC.CATALOG.SHARED.PAGE_TITLE

I'm having trouble reproducing this one. Here are the steps that I followed

  1. Share a Tale with User B
  2. On User B's view, click "Shared with Me"
  3. Unshare the tale with User B
  4. Refresh the page with User B

In steps 2 & 4 I see the window title as 'Shared Tales Catalog | WholeTale' (under Firefox and Chrome)

@craig-willis
Copy link
Contributor

@Xarthisius -- I didn't know that, so a non issue.
@bodom0015 -- likely a local problem for me, please disregard.

@bodom0015 bodom0015 merged commit 52c2a94 into master Dec 17, 2020
@Xarthisius Xarthisius deleted the tale-sharing branch April 27, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File uploads only seem to work intermittently
4 participants