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

Support CRUD share permissions #30862

Merged
merged 1 commit into from Feb 16, 2022
Merged

Support CRUD share permissions #30862

merged 1 commit into from Feb 16, 2022

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Jan 26, 2022

Support fine-grained permissions for external shares of folders.

Design change

Comment -------------Before------------- -------------After------------- -------------After-------------
Before/After view.

Note the new "Custom permissions" button at the bottom.

When clicking on it, a new menu replaces the "Bundle permissions" view.
Screenshot from 2022-01-27 12-25-10 Screenshot from 2022-01-27 15-06-16 Screenshot from 2022-01-27 15-06-09
Comment ------------------------------------- -------------------------------------
When clicking on "Custom permissions", a new view is displayed, showing the checked permissions reflecting the selected "Bundled permissions".

Here, "Create" is checked because "File drop" was selected.
Screenshot from 2022-01-27 15-06-16 Screenshot from 2022-01-27 15-06-09
As expected, the user can check any set of permissions without limitation, except selecting none.

When navigating back to the initial menu, no "Bundled permissions" is selected because none reflect the set of checked permissions.

Underneath "Custom permissions", there is a summary of the selected permissions. This summary is currently also shown when a "Bundled permissions" is selected.

When a set of custom permissions is selected, the "Custom permissions" view is opened by default.
Screenshot from 2022-01-27 15-05-49 Screenshot from 2022-01-27 15-05-59
The UI will prevent checking or unchecking permissions if the resulting permissions set is invalid.

- UPDATE and DELETE need READ to be checked.
- One of READ or CREATE must be checked at all times.

If the permissions set somehow end up in an invalid state, some red outline are displayed.
Screenshot from 2022-02-02 11-32-40 Screenshot from 2022-02-02 12-35-13

Technical changes

  • The SharePermissionsEditorVuejs element has been created to remove the logic from the gigantic SharingEntryLink.
  • I changed the limitation on the allowed permissions sets in the ShareAPIController. See above tab for the allowed permissions set logic.

Interogation/Additional work

Meta

Fix #30227
Fix https://github.com/nextcloud-gmbh/server/issues/118

@artonge artonge self-assigned this Jan 26, 2022
@artonge artonge changed the title Support CRUD share permissions WIP: Support CRUD share permissions Jan 26, 2022
@artonge artonge added this to the Nextcloud 24 milestone Jan 26, 2022
@artonge artonge force-pushed the feat/crud_share_permission branch 4 times, most recently from 3c53ca5 to 0e98ed6 Compare January 27, 2022 11:56
@artonge artonge force-pushed the feat/crud_share_permission branch 2 times, most recently from 4ff7089 to 44cc6ea Compare January 27, 2022 15:11
@artonge artonge changed the title WIP: Support CRUD share permissions Support CRUD share permissions Jan 27, 2022
@artonge artonge requested review from a team, PVince81, skjnldsv and szaimen and removed request for a team January 27, 2022 16:18
@artonge artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 27, 2022
@artonge artonge force-pushed the feat/crud_share_permission branch 4 times, most recently from 792e3ac to d834616 Compare February 2, 2022 16:10
@artonge artonge force-pushed the feat/crud_share_permission branch 2 times, most recently from 457330d to d486c82 Compare February 10, 2022 14:02
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice flow! Love it!
My only tiny bit of feedback would be that the wording "Bundle permissions" was a bit confusing to me, at first glance I thought it was an action that was to be applied.
I think it would be okay if there is no wording there, just the back icon, what do you think?

@artonge
Copy link
Contributor Author

artonge commented Feb 10, 2022

I think it would be okay if there is no wording there, just the back icon, what do you think?

It was the best that I could think of, and I wanted some wording because the user can have the "Custom permissions" view opened by default. Having no wording then might be non-intuitive, no? Maybe "Bundled permissions"?

But I have more trust in your opinion than mine on that matter ;)

@jancborchardt
Copy link
Member

jancborchardt commented Feb 10, 2022

@artonge as per the comment at #30227 (comment), it would be nice if the "Custom permissions" is actually a submenu like in Deck which moves all the rest away, not just replaces some parts (because that way it’s a bit confusing).

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 code looks fine

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good (not tested)

@artonge
Copy link
Contributor Author

artonge commented Feb 10, 2022

@artonge as per the comment at #30227 (comment), it would be nice if the "Custom permissions" is actually a submenu like in Deck which moves all the rest away, not just replaces some parts (because that way it’s a bit confusing).

As the user can have the "Custom permissions" view opened by default, wouldn't it be weird to hide all other fields?
Or I can remove the automatic opening of the "Custom permissions" view.

@nimishavijay
Copy link
Member

because the user can have the "Custom permissions" view opened by default. Having no wording then might be non-intuitive, no?

Hmm, good point. In that case having some wording there makes sense. I think "Bundled permissions" sounds nicer :)
Also, noticed another thing here: we could maybe change the wording "Create", "Read", "Update", "Delete" so that it's more similar to the wording in the bundled permissions:

  • Upload
  • Read
  • Edit
  • Delete
    Will likely be easier for someone who is selecting these options :)

@artonge
Copy link
Contributor Author

artonge commented Feb 15, 2022

Labels updated to match Nimisha's suggestions.

@jancborchardt an opinion on my answer to your feedback?

Signed-off-by: Louis Chemineau <louis@chmn.me>
@jancborchardt
Copy link
Member

@artonge in which case would the custom permissions be opened by default? Was that specifically requested? (If not, I'd say yep let's remove it.)

Cause since they are custom they should stay behind a submenu like that, and behave as the submenu does in Deck.

@artonge
Copy link
Contributor Author

artonge commented Feb 15, 2022

@artonge in which case would the custom permissions be opened by default? Was that specifically requested? (If not, I'd say yep let's remove it.)

@jancborchardt In the case where custom permissions are selected. The user would be prompted with the custom permissions panel instead of the bundled permissions one, which would be empty. Not requested, but it felt better to me.

Also, keeping the other options visible prevent the resizing of the whole menu which feels clunky.

@jancborchardt
Copy link
Member

@artonge ok, then it’s fine, especially as ideally it’s a stop-gap solution anyway before the new sharing flow comes in :) 👍

@artonge
Copy link
Contributor Author

artonge commented Feb 16, 2022

CI failure unrelated

@artonge artonge merged commit 1bfd001 into master Feb 16, 2022
@artonge artonge deleted the feat/crud_share_permission branch February 16, 2022 14:31
@artonge artonge added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 16, 2022
@Alwaysin
Copy link

Alwaysin commented Feb 16, 2022

Thanks for this PR!

Does anyone know if it is going to be backported to 23?

@AndyScherzinger
Copy link
Member

Does anyone know if it is going to be backported to 23?

We don't plan to backport it since we usually don't backport features but just bug fixes and security fixes but version 24 isn't that far away anymore :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRUD permissions for internal & external shares
8 participants