up and download of file shares

[butonic]

The shared folder logic in the gateway storageprovider was not allowing file up and downloads for single file shares. We now check if the reference is actually a file to determine if up / download should be allowed.

fixes: owncloud/product#205 and as a result likely owncloud/product#229, owncloud/product#208 and probably more.

cc @phil-davis

[phil-davis]

This scenario is still failing: https://cloud.drone.io/cs3org/reva/2544/5/7

  Background:                                                                     # /drone/src/tmp/testrunner/tests/acceptance/features/apiShareManagement/acceptSharesToSharesFolder.feature:7
    Given using OCS API version "1"                                               # FeatureContext::usingOcsApiVersion()
    And using new DAV path                                                        # FeatureContext::usingOldOrNewDavPath()
    And these users have been created with default attributes and skeleton files: # FeatureContext::theseUsersHaveBeenCreated()
      | username |
      | Alice    |
      | Brian    |

  Scenario: When accepting a share of a file, the received file is accessible                                                # /drone/src/tmp/testrunner/tests/acceptance/features/apiShareManagement/acceptSharesToSharesFolder.feature:15
    Given the administrator has set the default folder for received shares to "Shares"                                       # OccContext::theAdministratorHasSetTheDefaultFolderForReceivedSharesTo()
    And parameter "shareapi_auto_accept_share" of app "core" has been set to "no"                                            # AppConfigurationContext::serverParameterHasBeenSetTo()
    And user "Alice" has shared file "/textfile0.txt" with user "Brian"                                                      # FeatureContext::userHasSharedFileWithUserUsingTheSharingApi()
    When user "Brian" accepts share "/textfile0.txt" offered by user "Alice" using the sharing API                           # FeatureContext::userReactsToShareOfferedBy()
    Then the content of file "/Shares/textfile0.txt" for user "Brian" should be "ownCloud test text file 0" plus end-of-line # FeatureContext::contentOfFileForUserShouldBePlusEndOfLine()
      The downloaded content was expected to be 'ownCloud test text file 0
      ', but actually is ''.
      Failed asserting that two strings are equal.
      --- Expected
      +++ Actual
      @@ @@
      -'ownCloud test text file 0\n
      -'
      +''

I was hoping that at least that one would be a newly-passing test.

I will make those sort of Then steps emit more information when they fail - it would be nice to report the HTTP status (and anything else "interesting" in the response.

[phil-davis]

#1171 (comment)

The download gets a 404 - not found ???

[butonic]

hm the reva logs show 404s but not for a textfile0.txt. In fact textfile0.txt is nowhere to be found ...

oh wait that is an upload ... and if it is using tus the context is disconnected and we have no logs aaaaarrggg. @labkode @ishank011 we really need to reinitialize the logger for the upload context. See #923

or could we pass a logger / context to all service New() functions so we can attach a logger to the context that is used?

[butonic]

@phil-davis ok, so the problem with the ocis driver is that the skeleton files are not available and they cannot be easily put sumewhere on disk, because the layout is deconstructed. We'll have to use the cs3 api to upload the skeleton.

That is why the test gets a 404 because it cannot share the file.

[PVince81]

is this PR for fixing ocis storage only or does it fixes file sharing for other storages as well ?

[butonic]

it fixes acessing single share files for all storage drivers. but I don't know why the tests in reva fail and I am still figuring out how to run the testsuite locally...

[phil-davis]

The CI currently runs with "owncloud" storage and fails. IMO the skeleton files are correctly "simulated" in that case.

https://github.com/owncloud/core/blob/master/tests/acceptance/features/bootstrap/Provisioning.php manuallyAddSkeletonFiles copies them "underneath" into the storage of the user.

[butonic]

yep, found the readme to run the testsuite locally ... investigating

[butonic]

I used the debugger to halt the requests ... the shadow_folder really is not populated when accepting the share ... so something is going wrong earlier

[butonic]

@phil-davis 1af8f4b 🤞

[butonic]

@phil-davis this one seems to have gotten better: https://cloud.drone.io/cs3org/reva/2555/4/7

@issue-ocis-thumbnails-191 @skipOnOcis-EOS-Storage @issue-ocis-reva-308 | 271s
-- | --
1316 | Scenario: download previews of other users files                                                                           # /drone/src/tests/acceptance/features/apiOcisSpecific/apiWebdavPreviews-previews.feature:74 | 271s
1317 | Given user "Brian" has been created with default attributes and without skeleton files                                   # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles() | 271s
1318 | And user "Alice" has uploaded file "filesForUpload/lorem.txt" to "/parent.txt"                                           # FeatureContext::userHasUploadedAFileTo() | 271s
1319 | When user "Brian" downloads the preview of "/parent.txt" of "Alice" with width "32" and height "32" using the WebDAV API # FeatureContext::downloadPreviewOfOtherUser() | 271s
1320 | Then the HTTP status code should be "500"                                                                                # FeatureContext::thenTheHTTPStatusCodeShouldBe() | 271s
1321 | HTTP status code 200 is not the expected value 500 | 271s
1322 | Failed asserting that 200 matches expected '500'.

[phil-davis]

this one seems to have gotten better

Yes, the local API tests are bug-demo scenarios. We expect that when a bug is fixed, then the bug-demo scenario will fail. The process is:

• adjust the bug-demo scenario to make it pass
• if it is now demonstrating correct behavior, look for the matching scenario in core (there should be one in the matching core feature).
• confirm that the core scenario is now passing (it should have been reported in the CI as an unexpected success)
• remove the now-passing core scenario from expected-failures
• delete the local bug-demo scenario (it no longer demonstrates a bug - it is just a duplicate of a core scenario)

If we had software that already worked, we would not have to try and track all this :)

[butonic]

oohohhhhhh and see what we got:

runsh: Total unexpected passed scenarios throughout the test run:
--
13825 | apiShareManagement/acceptSharesToSharesFolder.feature:15 
apiShareManagement/acceptSharesToSharesFolder.feature:22 
apiShareToSharesManagementBasic/excludeGroupFromReceivingSharesToSharesFolder.feature:115 
apiShareToSharesManagementBasic/excludeGroupFromReceivingSharesToSharesFolder.feature:116 
apiShareToSharesManagementBasic/excludeGroupFromReceivingSharesToSharesFolder.feature:153 
apiShareToSharesManagementBasic/excludeGroupFromReceivingSharesToSharesFolder.feature:154

[butonic]

fixes owncloud/product#208

[phil-davis]

apiShareManagement/acceptSharesToSharesFolder.feature:15 
apiShareManagement/acceptSharesToSharesFolder.feature:22

Those are a real win - they are the genuine happy-path workflow for ordinary share-accept-download.

[butonic]

that Scenario: download previews of other users files now gives a 200 is worrying ... because it allows access to another users files ... I updated https://github.com/owncloud/ocis-reva/issues/308 but it might be limited to the owncloud driver ...

[butonic]

@phil-davis did I adjust the tests correctly in b0dfd59 ?

[phil-davis]

@phil-davis did I adjust the tests correctly in b0dfd59 ?

Yes, looks good. More tests are passing. The preview problem with '500' status is "improved" - anything is better than 500 ! Now the bug-demo scenario shows the "real" issue - a user can download a preview of another user's file, but they should get some 4nn unauthorised status response.

[phil-davis]

Test changes look good - there is improvement in the test results. So the code must be good ;)

Needs someone to review the actual code. @labkode @ishank011 ?

[PVince81]

👍 code looks fine

[PVince81]

optional: for the sake of readability, would be nice to have all if statement lead to error case branching, and the non-branching code path being the success path

[ishank011]

@butonic the code looks good, but this would allow all operations for sharing single files, which we currently don't allow because EOS doesn't support it. Should we allow it @labkode?

[labkode]

@butonic @ishank011 the code can be merged given that this is an optional configuration at the storage provider and is set to false as default. Specially when not all filesystem support it.

[ishank011]

@labkode that sounds good. @butonic can you add a config option to allow this change? With a default value of false.

[butonic]

@labkode @ishank011 hm,

at the storage provider

or at the gateway storage provider?

The code checks if the share node exists and is of type file, otherwise it prevents up or download. so this really only allows up and download for referenced files. I don't see the necessity for a config option. If the storage driver does not alllow creating shares on individual shares ... that is an implementation detail of the driver. And it might differ between multiple storage providers. The gateway should be agnostic about this.

[ishank011]

@butonic The storage driver doesn't control which files can be shared, that is determined by the share provider and so we have no mechanism in place to stop creating shares for single files for any storage drivers. Because of this, we also can't prevent uploads/downloads at the driver level because we don't know where it's coming from. So it'd have to be checked at the gateway storage provider only.

[butonic]

@ishank011 @labkode I still don't get what you want me to make configurable.

https://github.com/cs3org/reva/pull/1170/files#diff-87a927da7f0ac3d45225420a90987c21R154 is for InitiateFileDownload

https://github.com/cs3org/reva/pull/1170/files#diff-87a927da7f0ac3d45225420a90987c21R396 if for InitiateFileUpload

both are in the gateway storage provider.

do you want me to add a new enable_single_file_shares config to the real storage provider?

or a now config option to the gateway storage provider? the code changes does not affect the eos driver. shared files will not show up in the Shares folder ...

AFAICT the storage driver should return a not implemented error when trying to persist a grant on a file. because only the storage driver knows what it supports. it is also responsible for not showing the sharing / grant permissions when stating or listing files.

the gateway needs to send the share request to the storage first. the share manager is only responsible for caching the actually persisted shares. if the storage provider returns a not implemented error, there is nothing for the share manager to cache / store.

the code change in this pr is unrelated to that behavior, because the eos driver does not persist file grants. the share does not show up in the shared folder ...

if the share manager is responsible for determining if a resource can be shared or not he would have to check with the responsible storage provider, and as a result the driver. sharing or resharing permissions can change at any node in the tree. the single source of truth for that is the storage driver. the manager is only responsible for caching the shares to efficiently answer queries like "shared with me" / "shared with others".

the gateway is responsible for updating both: the storage provider and the share manager. they may run out of sync, but it should be possible to repopulate the complete share manager from the storage providers. if not we have a design problem, because we would need to keep the two services in sync, something we were trying to avoid AFAIR.

please help me understand the problem.

[butonic]

@ishank011 btw eos does support acls on files: https://github.com/cern-eos/eos/blob/master/doc/configuration/permission.rst#acls

For efficiency, file-level ACLs should only be used sparingly, in favour of directory-level ACLs.

We could add a flag to the eosdriver to disable setting shares on files...

[labkode]

@butonic I've been brainstorming with @ishank011 of what could be done on EOS to support ACL on files and is doable, we'll prototype implementation after finishing etag propagation for the shared folder.

For filesystems not supporting file-based sharing, we need a capability to be queried for the storage provider to avoid showing the share icon in the WebUI.

@butonic rebase, and when is green I'll merge.

[butonic]

@labkode @ishank011 rebased.

Regarding ACLs: CS3 grants != ACLs ... very much so. I realized the following difference when implementing permissions checks for the owncloud driver, which uses extended attributes to store permissions. IIRC we discussed the following scenario at the last workshop:

Given 'einstein' shared folder '/users/einstein/projects/top secret' with 'marie'
And 'marie' accepted the share and mounted it at '/users/marie/Shares/top secret'
And 'marie' created a new folder '/users/marie/Shares/top secret/assets'
When 'einstein' moves '/users/einstein/projects/top secret/assets' to '/users/einstein/top secret assets'
Then 'marie' cannot access '/users/einstein/top secret assets'

Or in short: the assets folder is moved outside of a shared folder.

With ACLs that is not true, because ACLs are propagated down the tree when they are set. This might lead to O(n) writes, where n is the number of children in the tree.

Grants work differently. They are only applied to a single node (file or folder). To check the grant permissions all parent nodes of the path need to be checked. This creates O(n) reads / stat calls, where n is the number of path segments.

In EOS ACLs work differently yet again: https://github.com/cern-eos/eos/blob/master/doc/configuration/permission.rst

Regardless of the storage, an integration with OS level ACLs will always lead to O(n) writes when renaming a shared folder. And for grants we need to implement expiry eg as an extended attribute anyway.

For the owncloud and ocis driver we will go with a pure grant implementation, accepting the O(n) reads tradeoff, because reads can be scaled better than writes. We already have an ACL integration with samba and already see the write bottleneck there when renaming folder. So for us ACL integration is only an additional option.

I plan to change the extended attribute prefix for shares we use to store this from user.oc.acl. to user.oc.grant. and make the difference between acls and grants more clear.

this acl could also be used to check permissions, which would be a different eos driver that has its own permissions checking ... or you know someone to add it to eos ;-)

let me know if we should discuss this in chat.

[labkode]

@butonic CI

[ishank011]

@butonic apologies for the delay in replying; this slipped out of my radar.

AFAICT the storage driver should return a not implemented error when trying to persist a grant on a file. because only the storage driver knows what it supports. it is also responsible for not showing the sharing / grant permissions when stating or listing files.

Yes, this should ideally be the case and I was wondering why we don't do that, but the latest EOS actually supports that. I was working with an older version where setting an attribute on a file meant actually setting it on its parent folder, which would lead to huge security risks. So we weren't in the favour of this change without a flag to disable it. But now it looks good to merge. The only issue is that the ACLs are not persisted across versions, which, as @labkode mentioned, we would work on next.

For the owncloud and ocis driver we will go with a pure grant implementation, accepting the O(n) reads tradeoff, because reads can be scaled better than writes. We already have an ACL integration with samba and already see the write bottleneck there when renaming folder. So for us ACL integration is only an additional option.

Makes a lot of sense. Maybe we can also discuss about this change in EOS because the reads overhead would definitely be a lot less than that of writes, but I'm pretty sure that there would be some reason behind the fact that we still apply ACLs to all the children; would be good to know what that is.

[ishank011]

But thinking back on this, how often do the writes take place as compared to the reads? A GET or a PROPFIND at the webdav endpoint involves 3-4 stat calls per resource, whereas the writes would occur only when creating new shares or renames. It would be good to run some benchmarks but I think the frequency of read operations makes up quite well for the lower overhead.

[butonic]

the CI failure seems to come from a difference in the XML output of

curl 'https://demo.owncloud.com/ocs/v2.php/apps/files_sharing/api/v1/sharees?search=sharee&itemType=file' -u demo:demo
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>200</statuscode>
  <message>OK</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data>
  <exact>
   <users/>
   <groups/>
   <remotes/>
  </exact>
  <users/>
  <groups/>
  <remotes/>
 </data>
</ocs>

vs

curl 'http://localhost:20080/ocs/v2.php/apps/files_sharing/api/v1/sharees?search=sharee&itemType=file' -u einstein:relativity
<?xml version="1.0" encoding="UTF-8"?>
<ocs><meta><status>ok</status><statuscode>200</statuscode><message>OK</message></meta><data><exact></exact></data></ocs>

reva does not show the empty elements when rendering xml (it does when rendering as json), which is exactly what the testsuite says:

Examples:
--
3498 | \| ocs-api-version \| ocs-status \| http-status \|
3499 | \| 1               \| 100        \| 200         \|
3500 | ShareesContext::getArrayOfShareesResponded The sharees response does not have key 'users'
3501 | Failed asserting that an array has the key 'users'.
3502 | \| 2               \| 200        \| 200         \|
3503 | ShareesContext::getArrayOfShareesResponded The sharees response does not have key 'users'
3504 | Failed asserting that an array has the key 'users'.

@phil-davis when was this added to the testsuite ... I don't see where my PR would touch related code. I did remove a few expected test failures, ...

oh wait ...

it is not failing because of those ... it is failing because we actually fixed stuff:

runsh: Total unexpected passed scenarios throughout the test run: | 463s
-- | --
3733 | apiShareReshareToShares3/reShareUpdate.feature:42 apiShareReshareToShares3/reShareUpdate.feature:43 apiShareReshareToShares3/reShareUpdate.feature:76 apiShareReshareToShares3/reShareUpdate.feature:77 apiShareReshareToShares3/reShareUpdate.feature:93 apiShareReshareToShares3/reShareUpdate.feature:94

getting rid of those ...

[butonic]

hm the resharing tests fail again with #1202 ...