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

External Storage Admin Checkbox to turn sharing, share links On and Off #19157

Closed
MTRichards opened this issue Sep 18, 2015 · 18 comments
Closed
Labels
app:files_external overview p1-urgent Critical issue, need to consider hotfix with just that issue
Milestone

Comments

@MTRichards
Copy link
Contributor

External Storage Sharing Switch – As an admin, I want to be able to disable sharing on a per mount point basis so that files can remain secured by the backend system and backend ACLs are maintained when used by the end user.
Acceptance Criteria:

  • Check box (disabled by default) labeled “allow sharing” is checked when the mount point for external storage or any secondary storage app is added
  • If not checked, the sharing options are disabled for that folder mount point
  • Sharing is also disabled via the various APIs for the desktop and mobile clients
  • Secondary storage API for configuring mass numbers of external storage mounts also takes this option into account
  • Same option is available for share links as well
@MTRichards MTRichards added this to the 9.0-next milestone Sep 18, 2015
@MTRichards MTRichards changed the title External Storage Admin Checkbox to turn sharing On and Off External Storage Admin Checkbox to turn sharing, share links On and Off Oct 15, 2015
@PVince81
Copy link
Contributor

The mount option will be on the same level as "disable encryption" so it will be settable through all APIs.

@PVince81
Copy link
Contributor

PVince81 commented Dec 1, 2015

Discussed with @icewind1991 and with the current architecture it is not possible to separate "user share" from "link share" permission. If we agree to combine them both into a single option "Enable sharing", then it can be easily implemented by reusing the "share" permission that we already have on the filesystem and every API (Webdav, etc). The implementation would need to make the storage return false for isSharable() if the option is not set. This would not require any changes on the client or permissions. (quick solution)

However if we do want to keep them separate, we'll need to introduce a new separate permission and modify all the API and API consumers to be able to understand the distinction, which is a lot more work.

@icewind1991
Copy link
Contributor

#20903

@PVince81
Copy link
Contributor

PVince81 commented Dec 2, 2015

Thanks. Note that this PR is the simple solution that disables sharing completely (no distinction between user share and link share)

@PVince81
Copy link
Contributor

PVince81 commented Dec 3, 2015

@MTRichards can you clarify the following:

  1. How important is it to separate user shares from link shares in this permission ? The separation needs a lot more architectural work, but combining them is already implemented in this PR Add mount option to disable sharing #20903

  2. What happens to existing shares on the storage if the admin decides to remove the sharing ability ? Should they still work or should the be blocked ? I'd vote for blocking because there is no way for the user to unshare any more because the option disappears completely in the UI.

@MTRichards
Copy link
Contributor Author

@PVince81 A few answers.

  1. How important is it to separate user shares from link shares in this permission ? The separation needs a lot more architectural work, but combining them is already implemented in this PR Add mount option to disable sharing #20903

A lot more important than you might think. A SharePoint, for example, makes sense to share internally only. But network drives are more likely to be shared as both. The more enterprise backends we add, the more that sharing granularity is necessary.
HOWEVER, if we need to do this in stages, I can live with that. First sharing overall, then separate internal sharing and share links.

  1. What happens to existing shares on the storage if the admin decides to remove the sharing ability ? Should they still work or should the be blocked ? I'd vote for blocking because there is no way for the user to unshare any more because the option disappears completely in the UI.

If the admin is changing the behavior of the system, then the shares should be blocked and not work. In theory, after a meaningful pause ("oops, I clicked the wrong button"), the shares would also be removed so we are not carrying around all this meta data for functionality that doesn't work any more. That could take forever of course, and we should discuss the implications of such.

@PVince81
Copy link
Contributor

PVince81 commented Dec 3, 2015

Thanks for the info.

  1. Regarding splitting the shares we should have a look at the scope of the changes as it might need to introduce new permissions in the storages, which will affect all APIs.
  1. Makes sense, I'll add these two checkboxes then:
  • block shares when the underlying storage does not allow sharing
  • make sure the "background job that cleans up invalid shares" is able to remove such shares (after a delay)

@PVince81
Copy link
Contributor

PVince81 commented Dec 4, 2015

@MTRichards we discovered a tricky situation:

Mount points can be set to subfolders like "/mymounts/SMB". If sharing is disabled on "SMB", then should we also disallow sharing of "mymounts" ?

It almost looks like the permissions need to propagate somehow.
(related to @rullzer's question here #20903 (comment))

@icewind1991 will have a look whether it's possible somehow

@PVince81
Copy link
Contributor

PVince81 commented Dec 7, 2015

For 9.0 we'll then aim to have the combined share part. It already comes with a few tricky edge cases, see #19157 (comment)

I raised the part about splitting shares in a separate ticket here #21000

@PVince81 PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Dec 11, 2015
@PVince81
Copy link
Contributor

PVince81 commented Feb 3, 2016

The mount option to disable sharing completely (both user share + public shares as a single option) for external storages has been implemented and merged already.

There is one open issue:

@PVince81
Copy link
Contributor

PVince81 commented Feb 3, 2016

@PVince81
Copy link
Contributor

This is implemented, closing.

This follow up ticket is about providing separate permissions for "link shares" and "user shares": #21000

@rperezb
Copy link

rperezb commented Feb 17, 2016

Doubt : previously, when this option was disable, before doint the action we warn the admin with this:
`Are you sure you want to disable personal mountpoints? By doing this, all the personal mountpoints will be removed for all the users```

This is not included in this version...do we want to add it? @MTRichards ?

@PVince81
Copy link
Contributor

"disable personal mountpoints" @rperezb this ticket is about disabling sharing, not personal mount points.

files_external works a bit differently. If you disable personal mounts, the users don't see them any more but they still exist internally. If the admin reenables it, the mount points will appear again.

The same is true for shares: the shares continue to exist but are inaccessible.
(maybe at some point we'll want to have a cleanup job to scrap them)

@rperezb
Copy link

rperezb commented Feb 17, 2016

Sorry! wrong copy-paste
Are you sure you want to disable shared mountpoints? By doing this, all the shared mountpoints will be removed for all the users

@PVince81
Copy link
Contributor

Yeah, as said above, the shares are preserved in the database but we added code to prevent these to be accessible when sharing is disabled.

@rperezb
Copy link

rperezb commented Feb 17, 2016

yes, previously, the original data was not removed either, however, the user with whom it was shared they stopped having access.
I just want to comment the difference.
FIne for me.
thx

@MTRichards MTRichards modified the milestones: 9.2-next, 9.0 Mar 25, 2016
@MorrisJobke MorrisJobke modified the milestones: 9.0, 9.2-next Apr 1, 2016
@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:files_external overview p1-urgent Critical issue, need to consider hotfix with just that issue
Projects
None yet
Development

No branches or pull requests

5 participants