Restrict deletion of share_folder by user

[sharidas]

Restrict deletion of share_folder by user

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

The idea behind this PR is to restrict user to delete the folder share_folder set in the config.php. In this change, I have tried to restrict the deletion from the View and from the dav app (i.e, access via webdav ).

Related Issue

• Fixes <issue_link>

Motivation and Context

Restrict user to delete folder share_folder set in the config.php.

How Has This Been Tested?

• Add SharedFolder to the config as share_folder => 'SharedFolder'
• Tested by deleting the folder from the UI results in the notification as shown below:
  
• The above test covers testing of webdav request to delete.
• Created a script to access the View of a user which has SharedFolder and then called the rmdir method of View. Received the return as false.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[sharidas]

Queries I have here is :

• Do we need to DI for config in the Directory.php ?

[codecov]

Codecov Report

Merging #35998 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #35998   +/-   ##
=======================================
  Coverage   53.85%   53.85%           
=======================================
  Files          63       63           
  Lines        7377     7377           
  Branches     1301     1301           
=======================================
  Hits         3973     3973           
  Misses       3019     3019           
  Partials      385      385

Flag	Coverage Δ
#javascript	53.85% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9898824...14d830c. Read the comment docs.

[jvillafanez]

I don't like this solution. Neither the directory nor the dav app doesn't need to know that the folder will be used to hold the shares. For them, it's just another folder without any special restriction.

If the files_sharing app is the one creating the folder, it should be the files_sharing app the one preventing the deletion. The app already has a storage wrapper available, so we can reuse that storage wrapper and prevent the folder deletion there.
This approach has several advantages:

• If all the sharing is disabled at some point, that folder will become empty (no shares are expected) and could be removed as long as the sharing app is disabled because the wrapper won't be registered any longer.
• If the files_sharing app decide to change the expected folder name to avoid collisions, this can be easily handled dinamically within the files_sharing app (including the storage wrapper)
• Config key handling is managed only by the one that creates / use the config key, not by any other app.

[sharidas]

Looking at the code, the share_folder storage points to home storage and not the ShareStorage. And hence deletion call made from the trashbin storage causes the code flow reach Local storage. Debugged the storages available during the rename from the home folder to the trashbin, unfortunately sharedstorage is not there.

[sharidas]

Looks like from the codeflow, the folder created using share_folder cannot be added to the sharedstorage, as it is not a share. The share is within the folder. Hence deletion of share_folder will not traverse through the sharedstorage. We can trim down the solution again to View only and remove the dav app change in the PR.
Other than that I not able to find an apt solution to make it totally depend on the files_sharing app.
Any suggestion or pointers here would be appreciated.

[jvillafanez]

Is it possible to use a mount point instead of creating a folder? it might be worthy to have a look and check if it's possible or not.

[sharidas]

Is it possible to use a mount point instead of creating a folder? it might be worthy to have a look and check if it's possible or not.

It looks like say if we use mount point for share_folder, then there is a chance that our code might fail at https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/SharedStorage.php#L103. Because share_folder is not a share.

[jvillafanez]

Looks like we've run out of ideas.... it seems we'll have to go with this solution.

Do we need the changes in the dav app? I think the changes in the View should be enough. In addition, we should include a log somewhere in order to know that the folder deletion is being prevented on purpose.

[sharidas]

Do we need the changes in the dav app?

Yes you are right, we don't need the dav change. The View change should be enough. I have adjusted the PR for view only. Let me know if this change looks ok.

[jvillafanez]

Any better alternative to this cleanup? There are a couple of things I don't like:

• This will change the configuration without restoring the previous value. It will delete whatever value there was before.
• If the test fails for whatever reason, a different value for the "share_folder" will be kept. This might cause problems in other "not-so-unit" tests.

[sharidas]

I have updated the PR by using overwriteService in the test method. And using restoreService in the tearDown. The reason for using overwriteService in the method and not in the setUp is: there are methods which have call to config and that were causing issues. So I think its better to have it only for one method where we need it.

[haribhandari07]

@sharidas I added 'share_folder' => 'SharedFolder' in config.php but I can delete SharedFolder using the webUI.

[phil-davis]

When I delete SharedFolder then all the received shares that are in SharedFolder go to the declined state. I can see them in "Shared with you". When I accept them again, then SharedFolder comes back and has the share(s) in it again.

What is the requirement here? (the behaviour above seems not so bad - you do not lose any data and can easily accept yyour shares again if you accidentallly delete them)

Before I receive any shares, there is no SharedFolder - that seems reasonable (it only gets created when the first share comes)

[phil-davis]

Now I discovered that you have to put the leading / in the config:
'share_folder' => '/SharedFolder'
then it prevents deletion.

@sharidas that is not mentioned in the original post describing the manual testing?