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

Sharing does not work with custom Storages, expects oc_filecache #26607

Closed
labkode opened this issue Nov 11, 2016 · 38 comments
Closed

Sharing does not work with custom Storages, expects oc_filecache #26607

labkode opened this issue Nov 11, 2016 · 38 comments

Comments

@labkode
Copy link

labkode commented Nov 11, 2016

@DeepDiver1975 @PVince81

We have our own implementation of the Sharing2.0 and we register it via config.php
'sharing.managerFactory' => 'OC\CernBox\Share\ProviderFactory',

So far, we can CRUD user, group and link shares from the sharing dialog and we haven't patched any class in ownCloud.

Now the problem resides when accessing the shared resources, as we cannot access them.
We think that the problem is that the sharing application still relies on filecache:

/var/www/html/core  git:(neweosstorage*) $ grep -r filecache . | grep -v tests | grep -v xml | grep -i share
./apps/files_sharing/lib/share/folder.php:              $query = \OCP\DB::prepare('SELECT `parent` FROM `*PREFIX*filecache` WHERE `fileid` = ?');
./apps/files_sharing/lib/share/folder.php:                      $query = \OCP\DB::prepare('SELECT `fileid`, `name`, `mimetype` FROM `*PREFIX*filecache`'
./apps/files_sharing/lib/API/Remote.php:         * @return array enriched share info with data from the filecache
./apps/files_sharing/lib/DeleteOrphanedSharesJob.php:                   'AND NOT EXISTS (SELECT `fileid` FROM `*PREFIX*filecache` WHERE `file_source` = `fileid`)';
./apps/files_sharing/lib/SharedMount.php:                       ->from('filecache')
./lib/private/Repair/RemoveRootShares.php:                      ->from('filecache')
./lib/private/Share/Share.php:                                  FROM `*PREFIX*filecache`
./lib/private/Share/Share.php:                  $fileDependentWhere = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` ';
./lib/private/Share/Share.php:                  $fileDependentWhere .= 'INNER JOIN `*PREFIX*storages` ON `numeric_id` = `*PREFIX*filecache`.`storage` ';
./lib/private/Share/Share.php:                  $where = 'INNER JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` ';
./lib/private/Share/Share.php:                  $where .= 'INNER JOIN `*PREFIX*storages` ON `numeric_id` = `*PREFIX*filecache`.`storage` ';
./lib/private/Share/Share.php:                                  . '`*PREFIX*storages`.`id` AS `storage_id`, `*PREFIX*filecache`.`parent` as `file_parent`, '
./lib/private/Share/Share.php:                                          . '`*PREFIX*storages`.`id` AS `storage_id`, `*PREFIX*filecache`.`parent` as `file_parent`';
./lib/private/Share/Share.php:                                                  . '`*PREFIX*share`.`permissions`, `expiration`, `storage`, `*PREFIX*filecache`.`parent` as `file_parent`, '
./lib/private/Share/Share.php:                                                  . '`*PREFIX*storages`.`id` AS `storage_id`, `*PREFIX*filecache`.`parent` as `file_parent`';
./lib/private/Share20/DefaultShareProvider.php:                         ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
./lib/private/Share20/DefaultShareProvider.php:                                 ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))

We have seen that:

./apps/files_sharing/lib/share/folder.php:  
./apps/files_sharing/lib/share/file.php:  

are registered in /apps/files_sharing/appinfo/app.php

\OC::$CLASSPATH['OC_Share_Backend_File'] = 'files_sharing/lib/share/file.php';
\OC::$CLASSPATH['OC_Share_Backend_Folder'] = 'files_sharing/lib/share/folder.php';

and if you comment these lines sharing stop working, therefore there is a strong dependency on these classes that rely on filecache to work.

This a is a high priority item for us to have CERNBox totally plugable in ownCloud 9 and avoid dirty hacks we did in the past.

@PVince81
Copy link
Contributor

  • Is "lib/private/Share/Share.php" still used anywhere from your implementation ? AFAIK it's the old legacy code and should be unused (at least in 9.1) since we now use the Share20 code.
  • DeleteOrphanedSharesJob.php => Use ShareManager to delete orphaned and expired shares #26265
  • regarding "share/folder.php" and "share/file.php", argh, I hoped the new sharing didn't use these any more.
  • Share20/DefaultShareProvider.php and the left join: this is used to make sure the share is still accessible through the isAccessibleResult() call that happens later, in case of trashed file. As you're using your custom implementation you probably don't need this check and could remove it from your "getSharedWith" implementation.

@PVince81
Copy link
Contributor

Indeed, seems the share backends are still used in some places (from v9.1.2):

apps/files_sharing/lib/API/Sharees.php:458:                     $backend = Share::getBackend($itemType);
lib/private/Share/Helper.php:49:                $backend = \OC\Share\Share::getBackend($itemType);
lib/private/Share/Share.php:635:                $backend = self::getBackend($itemType);
lib/private/Share/Share.php:936:                self::getBackend($itemType);
lib/private/Share/Share.php:1597:               if (isset(self::$backendTypes[$itemType]) && (!self::getBackend($itemType) instanceof \OCP\Share_Backend_Collection || $itemType != 'folder')) {
lib/private/Share/Share.php:1683:               $backend = self::getBackend($itemType);
lib/private/Share/Share.php:1974:                                       if (($collectionBackend = self::getBackend($row['item_type']))
lib/private/Share/Share.php:2037:                                       $collectionBackend = self::getBackend($row['item_type']);
lib/private/Share/Share.php:2061:                       $collectionBackend = self::getBackend('folder');
lib/private/Share/Share.php:2338:               $backend = self::getBackend($itemType);
lib/private/Tags.php:130:                       $this->backend = \OC\Share\Share::getBackend($this->type);
lib/public/Share.php:371:               return \OC\Share\Share::getBackend($itemType);
core/ajax/share.php:327:                                $backend = \OCP\Share::getBackend((string)$_GET['itemType']);

and from what I see those backend classes are using the old OCP\Share API, so that answers my question above. If we could rewrite those to use the new share API instead it might work.
Getting rid of file/folder sharing backend would be even better.

@PVince81
Copy link
Contributor

added to the main share 2.0 ticket: #22209

@PVince81
Copy link
Contributor

Let's see why the backends are used in these places:

Let's do some experimentation...

@PVince81
Copy link
Contributor

  • regarding SharedMount: I wonder if it would make sense to expect the share provider to provide its own mount instance/mount provider. Currently the mount provider is registered in "files_sharing".

@PVince81
Copy link
Contributor

Regarding the file/folder sharing backends, my findings so far:

  • if we do remove those file/folder sharing backends
    • can make sharing in the web UI still work (need some tweaks to JS loading, will push an experimental PR soon)
    • most integration tests seem to pass, will check again on CI (they use the OCS API)
    • we have to make sure that the old "\OC\Share" APIs are not used at all any more for file/folder
    • "API/Sharee" might fail if still used by file/folder. Might be possible to patch it up

So for now I'd say the goal is to find out how much of the sharing logic still goes through \OC\Share. I even thought maybe to log warnings if we find that they are called with item type "file" and "folder" to find all code paths.

@PVince81
Copy link
Contributor

Here's an experimental PR that gets rid of the file/folder share backends: #26608

@labkode Since you're using OC 9, you might want to only comment them out and also adjust the "loadScripts" part, see the first commit and how it moves JS loading for shares to the template: 42ba7db#diff-f7a9bb6da381ad3d1651acb5f47162a1R149

@labkode and for the sharee (autocomplete), this might be needed to avoid errors: 8b2a6c4

So far on that PR manual sharing in the web UI still works but I haven't tested complex scenarios yet.
I'd expect this also work on your environment.

@PVince81
Copy link
Contributor

After working on #26608 and checking the code paths a bit, I found that there are still places in the code that rely on OC\Share::getItemShared() which itself can request parent collections which can currently only be done through the 'FolderandFile` backends.
So we need to find a way to move away from that: #22209 (comment)

@PVince81 PVince81 added this to the 9.2 milestone Nov 11, 2016
@PVince81
Copy link
Contributor

... or the short-term alternative would be to make these share backends use the node API and share 2.0 API when possible. But long-term I'd like to kill these.

@PVince81
Copy link
Contributor

So, in this PR #26608 the occurrences of filecache are already reduced:

./apps/files_sharing/lib/DeleteOrphanedSharesJob.php:                   'AND NOT EXISTS (SELECT `fileid` FROM `*PREFIX*filecache` WHERE `file_source` = `fileid`)';
./apps/files_sharing/lib/SharedMount.php:                       ->from('filecache')
./lib/private/Repair/RemoveRootShares.php:                      ->from('filecache')
./lib/private/Share/Share.php:                                  FROM `*PREFIX*filecache`
./lib/private/Share20/DefaultShareProvider.php:                         ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
./lib/private/Share20/DefaultShareProvider.php:                                 ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))

(DefaultShareProvider can be ignored)

Still a lot of work to do to make most of the OC code rely on the new Share API.
I'll continue adding these to the same PR (and optionally split to separate smaller PRs).

@PVince81
Copy link
Contributor

Now let's make a nice list of todos:

Once all these are done there should be no trace left of filecache in the sharing code. (separate apps might have their own though, like activity, etc)

@PVince81 PVince81 changed the title Sharing does not work with custom Storages Sharing does not work with custom Storages, expects oc_filecache Nov 11, 2016
@labkode
Copy link
Author

labkode commented Nov 14, 2016

@PVince81

Just a few remarks to give more light to the problem:

Sharing via link works, I can access shared folders with no problems with our custom storage.
The problem is just for user and group shares.

Example of the problem:

  • user Y shares folder A with user X
  • user Y can access the share information and modify it without any problem
  • user Y can also see the shares in the Shared with others tab
  • user X is not able to see the shared resources in the Shared with you tab not in the root user tree, he gets an empty response, see below:
curl 'http://localhost/core/ocs/v1.php/apps/files_sharing/api/v1/shares?format=json&shared_with_me=true'

{"ocs":{"meta":{"status":"ok","statuscode":100,"message":null},"data":[]}}

Our code base is based on stable9.1 not on master so merging your code gives lot of conflicts. What I have done is just cherry picked the commit 42ba7db and the UI and share via link continue to work, user and groups shares still not.

@PVince81
Copy link
Contributor

@labkode please debug into 'ShareManager::getSharedWith()` from https://github.com/owncloud/core/blob/v9.1.2/apps/files_sharing/lib/MountProvider.php#L70 and see why it doesn't return any results for you.

@labkode
Copy link
Author

labkode commented Nov 14, 2016

@PVince81
This is the error message I get from the log:

Could not get storage info for mount at /labradorsvc/files/ONE (#6697304)/

The scenario is the following:

  • User gonzalhu shared folder /ONE with user labradorsvc
  • /ONE (#6697304) is the file target generated by us inserted into the DB
sqlite> select * from oc_share;
10|0|labradorsvc|gonzalhu|gonzalhu||folder|6697304||6697304|/ONE (#6697304)|15|1479128240|0|||0

I went to the file responsible for this:

# grep -r 'Could not get storage info' .
./lib/private/Files/Config/UserMountCache.php:                  $this->logger->debug('Could not get storage info for mount at ' . $mount->getMountPoint());
private function addToCache(ICachedMountInfo $mount) {
        if ($mount->getStorageId() !== -1) {
            $this->connection->insertIfNotExist('*PREFIX*mounts', [
                'storage_id' => $mount->getStorageId(),
                'root_id' => $mount->getRootId(),
                'user_id' => $mount->getUser()->getUID(),
                'mount_point' => $mount->getMountPoint()
            ], ['root_id', 'user_id']);
        } else {
            // in some cases this is legitimate, like orphaned shares
            $this->logger->debug('Could not get storage info for mount at ' . $mount->getMountPoint());
        }
    }

What is supposed to be returned in the getStorageId() for the mount?
At production our oc_mounts table is empty (using ownCloud 8.2 with primary object storage patched) and sharing is working while in my current dev setup (ownCloud 9.1 with IStorage) I see entries in this table and sharing is not working:

sqlite> select * from oc_mounts;
3|96616|3874498|labradorsvc|/labradorsvc/
5|95491|3873780|gonzalhu|/gonzalhu/

How can I avoid using this table in ownCloud 9.1 based on IStorage? What I should return from the getStorageId method?

Thanks in advance

@PVince81
Copy link
Contributor

The value of $mount->getStorageId() here should be the numeric_id from oc_storages.

It might be possible to skip using oc_mounts altogether by hacking UserMountCache to not store anything there. However note that some APIs will not work properly like "find storage owner by file id" which rely on this table. As far as I remember this is only used by some enterprise features (for now), so you might be fine (for now), but we might want to rely on this table more often in the future, see #26190

How many storage implementations do you have ? If you only have a single one you might be able to simply return a dummy id like "1" every time and be done with it.

@labkode
Copy link
Author

labkode commented Nov 14, 2016

@PVince81

These are some examples from prod oc_storages

| object::user:produser1                                        |       8370 |         1 |         NULL |
| object::user:produser2                                        |       8371 |         1 |         NULL |
| shared::/disney (#19875896)                                   |       8372 |         1 |         NULL |

In my dev setup I have these:

1|home::gonzalhu|1|
2|local::/var/www/html/core/data/|1|

And these two entries were created before enabling our own storage. So, after enabling it nothing has been inserted to this table, not even shared::* entries.

Can this be the primary source of the sharing problem ? so shared stuff do not appear because the mount point is not listed in this table? And if so, that means that there is a hardcoded dependency at some place to insert shared::* entries into the oc_storage table and that requirement is not gathered into the sharing interfaces.

@labkode
Copy link
Author

labkode commented Nov 14, 2016

How many storage implementations do you have ? If you only have a single one you might be able to > simply return a dummy id like "1" every time and be done with it.

We only use one, I'll try with that, but then it will be the same for all users right? And in that case, I don't understand why it is stored in oc_storage per user and in previous examples

@labkode
Copy link
Author

labkode commented Nov 14, 2016

This folder
https://github.com/cernbox/core/tree/neweosstorage/lib/private/CernBox
contains all our code, so you can get a better image of what we are trying to do

@PVince81
Copy link
Contributor

The "oc_storages" is not supposed to have any "shared::" entries for local shares, only fed shares. There was a bug in 9.0 where bogus entries were added.

Did you try debugging into #26607 (comment) ? The received share mount points are dynamic and only get setup if your sharing code properly returns the "shared with you" entries. Since you said that even "shared with you" has no results, then shared mount points will not appear. So first would be to find out why no results are returned.

@labkode
Copy link
Author

labkode commented Nov 14, 2016

Did you try debugging into #26607 (comment) ? The received share mount points are dynamic and only get setup if your sharing code properly returns the "shared with you" entries. Since you said that even "shared with you" has no results, then shared mount points will not appear. So first would be to find out why no results are returned.

I checked that area and my share provider is returning the shares, but nothing shows in the UI, so they get discarded in the way up.
So SharedMounts are created without problem (no Expection thrown) and they are passed to the next functional element.

So, the problem must be in the code calling getMountsForUser or that ownCloud expects some behaviours/structure in the fields of the share and I am not replying them correctly

@PVince81
Copy link
Contributor

Okay, weird... I commented out this line but the mount points still work: https://github.com/owncloud/core/blob/v9.1.2/lib/private/Files/Filesystem.php#L443

Actually I think the real registration of mounts for local use is done two lines above: https://github.com/owncloud/core/blob/v9.1.2/lib/private/Files/Filesystem.php#L441

With a breakpoint there, this is what I see when evaluating "self::$mounts":

  ::mounts                       = (object|OC\Files\Mount\Manager[1]);
    ::mounts->mounts             = (array[3]);
      ::mounts->mounts['/']      = (object|OC\Files\Mount\MountPoint[8])+;
      ::mounts->mounts['/admin/'] = (object|OC\Files\Mount\MountPoint[8])+;
      ::mounts->mounts['/admin/files/test/'] = (object|OCA\Files_Sharing\SharedMount[12]);
        ::mounts->mounts['/admin/files/test/']->storage = (null);
        ::mounts->mounts['/admin/files/test/']->recipientView = (object|OC\Files\View[5])+;
        ::mounts->mounts['/admin/files/test/']->user = (string[5]) 'admin';
        ::mounts->mounts['/admin/files/test/']->superShare = (object|OC\Share20\Share[19]);
          ::mounts->mounts['/admin/files/test/']->superShare->id = (string[1]) '1';
          ::mounts->mounts['/admin/files/test/']->superShare->providerId = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->node = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->fileId = (int) '17';
          ::mounts->mounts['/admin/files/test/']->superShare->nodeType = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->shareType = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->sharedWith = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->sharedBy = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->shareOwner = (string[5]) 'user1';
          ::mounts->mounts['/admin/files/test/']->superShare->permissions = (int) '31';
          ::mounts->mounts['/admin/files/test/']->superShare->expireDate = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->password = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->token = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->parent = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->target = (string[5]) '/test';
          ::mounts->mounts['/admin/files/test/']->superShare->shareTime = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->mailSend = (null);
          ::mounts->mounts['/admin/files/test/']->superShare->rootFolder = (object|OC\Files\Node\LazyRoot[2])+;
          ::mounts->mounts['/admin/files/test/']->superShare->userManager = (object|OC\User\Manager[4])+;
        ::mounts->mounts['/admin/files/test/']->groupedShares = (array[1]);
          ::mounts->mounts['/admin/files/test/']->groupedShares[0] = (object|OC\Share20\Share[19]);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->id = (string[1]) '1';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->providerId = (string[10]) 'ocinternal';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->node = (null);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->fileId = (int) '17';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->nodeType = (string[6]) 'folder';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->shareType = (int) '0';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->sharedWith = (string[5]) 'admin';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->sharedBy = (string[5]) 'user1';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->shareOwner = (string[5]) 'user1';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->permissions = (int) '31';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->expireDate = (null);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->password = (null);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->token = (null);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->parent = (null);
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->target = (string[5]) '/test';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->shareTime = (object|DateTime[3])+;
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->mailSend = (bool) '0';
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->rootFolder = (object|OC\Files\Node\LazyRoot[2])+;
            ::mounts->mounts['/admin/files/test/']->groupedShares[0]->userManager = (object|OC\User\Manager[4])+;
        ::mounts->mounts['/admin/files/test/']->class = (string[24]) '\OC\Files\Storage\Shared';
        ::mounts->mounts['/admin/files/test/']->storageId = (null);
        ::mounts->mounts['/admin/files/test/']->arguments = (array[4])+;
        ::mounts->mounts['/admin/files/test/']->mountPoint = (string[18]) '/admin/files/test/';
        ::mounts->mounts['/admin/files/test/']->mountOptions = (array[0]);
        ::mounts->mounts['/admin/files/test/']->*OC\Files\Mount\MountPoint*loader = (object|OC\Files\Storage\StorageFactory[1])+;
        ::mounts->mounts['/admin/files/test/']->*OC\Files\Mount\MountPoint*invalidStorage = (bool) '0';

"test" is the received share. Apparently no storage id is needed there.

Does your SharedMount look similar ? Are the attributes "superShare" and "groupedShares" populated ? (these are used when merging multiple shares into a single one, for example when a user receives the same folder through user and group share simultaneously)

@labkode
Copy link
Author

labkode commented Nov 14, 2016

@PVince81 ,

Thanks a lot for the input,
I will show you tomorrow my output as currently I am on Centos7 and PHP 5.4.15 so no Xdebug available and I have to update the system to make it works!

@labkode
Copy link
Author

labkode commented Nov 15, 2016

@PVince81,

here is the output, the only difference my eye could spot is that I use my IShare implementation (\OC\CernBox\Share\Share) instead yours.
And also that your groupedShares[0] has 19 items and mine 17 (no parent, no userManager)

I also expanded the file info so you can see our ICacheEntry implementation and the data it contains:

$mounts = {array} [2]
 0 = {OCA\Files_Sharing\SharedMount} [12]
  *OC\Files\Mount\MountPoint*invalidStorage = false
  *OC\Files\Mount\MountPoint*loader = {OC\Files\Storage\StorageFactory} [1]
  arguments = {array} [4]
  class = "\OC\Files\Storage\Shared"
  groupedShares = {array} [1]
   0 = {OC\CernBox\Share\Share} [17]
    expireDate = null
    fileId = 6697304
    id = 11
    mailSend = false
    node = {OC\Files\Node\Folder} [4]
     fileInfo = {OC\Files\FileInfo} [7]
      childEtags = {array} [0]
      data = {OC\CernBox\Storage\Eos\CacheEntry} [1]
       data = {array} [42]
        encrypted = 0
        eos.container = "0"
        eos.ctime = "1478769076.167256830"
        eos.etag = "663158:1479222831.705"
        eos.fid = "6697304"
        eos.file = "/eos/scratch/user/g/gonzalhu/ONE"
        eos.files = "0"
        eos.fxid = "00663158"
        eos.gid = "2763"
        eos.ino = "6697304"
        eos.mode = "42700"
        eos.mtime = "1479222831"
        eos.pid = "3873780"
        eos.pxid = "003b1bf4"
        eos.sys.acl = "u:gonzalhu:rwx!m,u:labradorsvc:rwx+d"
        eos.sys.allow.oc.sync = "1"
        eos.sys.forced.atomic = "1"
        eos.sys.forced.blockchecksum = "crc32c"
        eos.sys.forced.blocksize = "4k"
        eos.sys.forced.checksum = "adler"
        eos.sys.forced.layout = "replica"
        eos.sys.forced.nstripes = "2"
        eos.sys.forced.space = "default"
        eos.sys.mask = "700"
        eos.sys.mtime.propagation = "1"
        eos.sys.recycle = "/eos/backup/proc/recycle/"
        eos.sys.versioning = "10"
        eos.treesize = "0"
        eos.uid = "95491"
        etag = "663158:1479222831.705"
        fileid = 6697304
        mimetype = "httpd/unix-directory"
        mtime = "1479222831"
        name = "ONE"
        parent = 3873780
        path = "files/ONE"
        path_hash = "56782d2aafaa79b7dcc99aa20471102a"
        permissions = 31
        size = "0"
        storage_mtime = "1479222831"
        type = "dir"
        unencrypted_size = 0
      internalPath = "files/ONE"
      mount = {OC\Files\Mount\MountPoint} [8]
      owner = {OC\User\User} [10]
      path = "/gonzalhu/files/ONE"
      storage = {OCA\Files_Trashbin\Storage} [10]
     path = "/gonzalhu/files/ONE"
     root = {OC\Files\Node\Root} [7]
     view = {OC\Files\View} [5]
    nodeType = "folder"
    password = null
    permissions = 15
    providerId = "ocinternal"
    rootFolder = {OC\Files\Node\Root} [7]
    sharedBy = "gonzalhu"
    sharedWith = "labradorsvc"
    shareOwner = "gonzalhu"
    shareTime = {DateTime} [3]
    shareType = 0
    target = "/ONE (#6697304)"
    token = null
  mountOptions = {array} [0]
  mountPoint = "/labradorsvc/files/ONE (#6697304)/"
  recipientView = {OC\Files\View} [5]
  storage = null
  storageId = null
  superShare = {OC\Share20\Share} [19]
   expireDate = null
   fileId = 6697304
   id = "11"
   mailSend = null
   node = null
   nodeType = null
   parent = null
   password = null
   permissions = 15
   providerId = null
   rootFolder = {OC\Files\Node\LazyRoot} [2]
   sharedBy = null
   sharedWith = null
   shareOwner = "gonzalhu"
   shareTime = null
   shareType = null
   target = "/ONE (#6697304)"
   token = null
   userManager = {OC\User\Manager} [4]
  user = "labradorsvc"
 1 = {OC\Files\Mount\MountPoint} [8]

@PVince81
Copy link
Contributor

Hmmm, that looks fine to me.

If you do a PROPFIND on this "ONE" folder directly for the receiving user even though it's not visible, do you get a result or 404 ?

Next step would be do debug into https://github.com/owncloud/core/blob/v9.1.2/lib/private/Files/Mount/Manager.php#L73 and evaluate the $this->mounts and see if "ONE" is inside.
Also evaluate OC\Files\Filesystem::$mounts (static var, you might want to make it public for debugging and easy evaluation). Both should contain "ONE". If one of them doesn't then we might find where to look.

@labkode
Copy link
Author

labkode commented Nov 17, 2016

I tried with the original file name ONE and with the file target ONE (#6697304) and both results give back 404.

In the second request, at least the file target was resolved to the resource name of the share owner.

# curl -X PROPFIND http://localhost/core/remote.php/webdav/ONE -u labradorsvc
Enter host password for user 'labradorsvc':
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>File with name ONE could not be located</s:message>
</d:error>
# curl -X PROPFIND 'http://localhost/core/remote.php/webdav/ONE (#6697304)' -u labradorsvc
Enter host password for user 'labradorsvc':
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotFound</s:exception>
  <s:message>File with name ONE could not be located</s:message>
</d:error>

@labkode
Copy link
Author

labkode commented Nov 17, 2016

To be clear with the scenario:

  • user gonzalhu shared folder /ONE with user labradorsvc
  • the share file target is /ONE (#6697304)

This is how the mounts look like for the shared folder when examinating the break point at:

if (isset($this->mounts[$path])) {
$path = "/labradorsvc/files/ONE (#6697304)/"
$this = {OC\Files\Mount\Manager} [1]
 mounts = {array} [3]
  / = {OC\Files\Mount\MountPoint} [8]
  /gonzalhu/ = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/ = {OC\Files\Mount\MountPoint} [8]
$_COOKIE = {array} [3]
$_SERVER = {array} [37]
$_SESSION = {array} [1]
$GLOBALS = {array} [11]
ocp\PERMISSION_CREATE = 4
ocp\PERMISSION_READ = 1
ocp\PERMISSION_UPDATE = 2
ocp\PERMISSION_DELETE = 8
ocp\PERMISSION_SHARE = 16
ocp\PERMISSION_ALL = 31
ocp\FILENAME_INVALID_CHARS = "\/<>:"|?*\n"
RANDOM_COMPAT_READ_BUFFER = 8
GRAPHEME_CLUSTER_RX = "\X"
GRAPHEME_EXTR_COUNT = 0
GRAPHEME_EXTR_MAXBYTES = 1
GRAPHEME_EXTR_MAXCHARS = 2
CRYPT_HASH_MODE = 3

The evaluation of \OC\Files\Filesystem::$mounts gives same results:

result = {OC\Files\Mount\Manager} [1]
 mounts = {array} [3]
  / = {OC\Files\Mount\MountPoint} [8]
  /gonzalhu/ = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/ = {OC\Files\Mount\MountPoint} [8]

Reading your previous comment, which mount should I have?

  • /labradorsvc/ONE
  • /labradorsvc/ONE (#6697304)

@PVince81
Copy link
Contributor

If the share mountpoint would be mounted correctly, the mounts array would contain an additional entry:
/labradorsvc/files/ONE (#6697304).

Ok, so the result you posted means we should debug in the code that is executed between the SharedMount creation and that find call.

@PVince81 PVince81 self-assigned this Nov 21, 2016
@labkode
Copy link
Author

labkode commented Nov 22, 2016

@PVince81
The entry /labradorsvc/files/ONE (#6697304) appears only on parent finds, see examples, but not on the /labradorsvc/files/ONE (#6697304) itself as in previous comments.

Does it gives you some hint? Else I will start debugging the creation of the Shared mount.

$path = "/labradorsvc/"
$this = {OC\Files\Mount\Manager} [1]
 mounts = {array} [4]
  / = {OC\Files\Mount\MountPoint} [8]
  /gonzalhu/ = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/ = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/files/ONE (#6697304)/ = {OCA\Files_Sharing\SharedMount} [12]
$_COOKIE = {array} [3]
$_SERVER = {array} [37]
$_SESSION = {array} [1]
$GLOBALS = {array} [11]
ocp\PERMISSION_CREATE = 4
ocp\PERMISSION_READ = 1
ocp\PERMISSION_UPDATE = 2
ocp\PERMISSION_DELETE = 8
ocp\PERMISSION_SHARE = 16
ocp\PERMISSION_ALL = 31
ocp\FILENAME_INVALID_CHARS = "\/<>:"|?*\n"
RANDOM_COMPAT_READ_BUFFER = 8
GRAPHEME_CLUSTER_RX = "\X"
GRAPHEME_EXTR_COUNT = 0
GRAPHEME_EXTR_MAXBYTES = 1
GRAPHEME_EXTR_MAXCHARS = 2
CRYPT_HASH_MODE = 3
$path = "/labradorsvc/files/"
$this = {OC\Files\Mount\Manager} [1]
$_COOKIE = {array} [3]
$_SERVER = {array} [37]
$_SESSION = {array} [1]
$GLOBALS = {array} [11]
ocp\PERMISSION_CREATE = 4
ocp\PERMISSION_READ = 1
ocp\PERMISSION_UPDATE = 2
ocp\PERMISSION_DELETE = 8
ocp\PERMISSION_SHARE = 16
ocp\PERMISSION_ALL = 31
ocp\FILENAME_INVALID_CHARS = "\/<>:"|?*\n"
RANDOM_COMPAT_READ_BUFFER = 8
GRAPHEME_CLUSTER_RX = "\X"
GRAPHEME_EXTR_COUNT = 0
GRAPHEME_EXTR_MAXBYTES = 1
GRAPHEME_EXTR_MAXCHARS = 2
CRYPT_HASH_MODE = 3
 mounts = {array} [4]
  / = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/ = {OC\Files\Mount\MountPoint} [8]
  /gonzalhu/ = {OC\Files\Mount\MountPoint} [8]
  /labradorsvc/files/ONE (#6697304)/ = {OCA\Files_Sharing\SharedMount} [12]

@PVince81
Copy link
Contributor

on parent finds

What do you mean with parent find ? Mount manager find on the parent folder ?
Maybe there's a leading/trailing slash messing up with the code ?

@labkode
Copy link
Author

labkode commented Nov 23, 2016

Hi,
sorry, let me be more clear.

When I do a PROPFIND on my home directory, inside the find method I see that for paths /labrador/ and /labrador/files/ I have this mount point:
/labradorsvc/files/ONE (#6697304)/ = {OCA\Files_Sharing\SharedMount} [12]
but for path /labradorsvc/files/ONE (#6697304)/ I don't have it.

If I do the PROPFIND or MKCOL on /ONE (#6697304)/ the server behaves as expected ( see curl samples below) and for path /labradorsvc/files/ONE (#6697304)/ I have the mount point /labradorsvc/files/ONE (#6697304)/ = {OCA\Files_Sharing\SharedMount} [12]

curl 'http://localhost/core/remote.php/webdav/ONE%20(%236697304)' -X MKCOL -H 'Host: localhost''

405 Method Not Allowed
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\MethodNotAllowed</s:exception>
  <s:message>The resource you tried to create already exists</s:message>
</d:error>
curl 'http://localhost/core/remote.php/webdav/ONE%20(%236697304)' -X PROPFIND -H 'Host: localhost' 

207 Multi-Status
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/core/remote.php/webdav/ONE%20(%236697304)/</d:href>
  <d:propstat>
   <d:prop>
    <d:getlastmodified>Wed, 23 Nov 2016 08:24:19 GMT</d:getlastmodified>
    <d:getetag>&quot;3b1bf4:1479889459.092&quot;</d:getetag>
    <d:resourcetype>
     <d:collection/>
    </d:resourcetype>
    <oc:fileid>3873780</oc:fileid>
    <oc:permissions>SD</oc:permissions>
    <oc:size>17802394</oc:size>
    <oc:tags/>
    <oc:favorite></oc:favorite>
    <oc:comments-unread>0</oc:comments-unread>
    <oc:owner-display-name>gonzalhu</oc:owner-display-name>
    <oc:share-types/>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
  <d:propstat>
   <d:prop>
    <d:getcontenttype/>
    <d:getcontentlength/>
   </d:prop>
   <d:status>HTTP/1.1 404 Not Found</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>

In my previous comment I told you that PROPFINDing /ONE (#6697304) gave me 404, but that was because I did not URL encoded correctly, stupid me :(

@labkode
Copy link
Author

labkode commented Nov 23, 2016

Hi,
after digging the reason why the shared folder were not displayed was related to our caching system, so after disabling it we can see the shared folder and browse it, but we still receive

{"reqId":"WDWcoGW5OFUtURHX0iw3GgAAAAA","remoteAddr":"::1","app":"no app in context","message":"Could not get storage info for mount at \/labradorsvc\/files\/ONE (#6697304)\/","level":0,"time":"2016-11-23T13:41:55+00:00","method":"PROPFIND","url":"\/core\/remote.php\/webdav\/","user":"labradorsvc"}

Although the shared folser appears in the All files tab when I click on the Shared with you tab there aren't shares. When I see the details of the Shared folder from the All files tab, I got 'Resharing is not allowed' .

I debugged what was going on when clicking on the Shared with you tab and I discovered that shares were not returned because they could not be resolved because of following:

This line calls the formatShare method but a NotFound exception is triggered.
https://github.com/cernbox/core/blob/neweosstorage/apps/files_sharing/lib/API/Share20OCS.php#L432

The problem resides here:
https://github.com/cernbox/core/blob/neweosstorage/apps/files_sharing/lib/API/Share20OCS.php#L119

Based on my previous example (gonzalhu shared /ONE with user labradorsvc),
the $userFolder is the home folder for user labradorsvc (this->currentUser is the logged in user).
Then the getById method uses the item source from the share, i.e. the file id belonging to user gonzalhu. This file is residing on gonzalhu space and not on labradorsvc, so we return not found

The way I understand this is that your $userFolder (\OCP\Files\Folder, "Returns a view to user's files folder") allows also to access files outside the user home folder and I think is completely wrong.

The $userFolder should be created with the shareOwner, as this user is the one that has the folder with id = item_source.

Please, check if there is something wrong in my explanation,

Cheers

@PVince81
Copy link
Contributor

No, I think the Share20OCS logic is correct. The response must be with the perspective of the currently logged in user so that the paths appear as they see them.

For example:

  • user1 shares "test" with group1 (containing user2 and user3)
  • user2 renamed received "test" to "mytest"
  • user3 renamed received "test" to "sometest"

If user2 queries the API they need to see the path as "/mytest", not the one from the original share. This is important to be able to map the shares to the results from the Webdav endpoint for example.

In the regular OC impl if you call getById() as user2 and the share is mounted as "/user2/files/mytest" it will then find that entry. Note that getById() returns an array in case there are multiple results. (as far as I understand).

So either your impl needs to change to return that item too. Or maybe something is missing and the item isn't mounted properly in this specific code path ?

@labkode
Copy link
Author

labkode commented Nov 23, 2016

Hi,

I found the problem and the solution.

The problem arises due to a non handled exception here:
https://github.com/cernbox/core/blob/neweosstorage/lib/private/Files/Node/Folder.php#L279

The get method will throw a NotFoundException when asking for a path that does not exist. This exception will abort the traversal of the others mounts that have not been yet examined. And the second mount point in my case is exactly the SharedMount.

The solution I put in place is this:

try {
	$nodes[] = $node = $this->get($path);
} catch (\Exception $exception) {
	\OC::$server->getLogger()->error($exception);
}

and sharing works both from 'Shared with you' and in the share view when accessing the details of the folder.

I will create a PR for this.

What I wonder is why you did not face this problem before? i.e. the exception is not triggered for you?

@PVince81
Copy link
Contributor

for you

I didn't test with any custom storages / shared storages, so I don't see how I could run into such exception. Or maybe it happens but is not visible because it is ignored ? Haven't digged into this piece of code.

@PVince81
Copy link
Contributor

but great that you found the problem with a solution 😄

This whole thing is a bit obscure... Does the PHPDoc need updating to make sure implementers of the interface know that they need to do that ?

@PVince81
Copy link
Contributor

PVince81 commented Apr 6, 2017

@labkode it did work in the end, didn't it ?

I'd like to close this ticket unless you have anything to add.
We already have other tickets to cleanup the Sharing implementation and make it more customizable.

@labkode
Copy link
Author

labkode commented Apr 7, 2017

@PVince81 Sure

@lock
Copy link

lock bot commented Jul 31, 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 Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants