search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.37k stars 2.06k forks source link

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

Closed labkode closed 7 years ago

labkode commented 7 years ago

@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 commented 7 years ago
PVince81 commented 7 years ago

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 commented 7 years ago

added to the main share 2.0 ticket: https://github.com/owncloud/core/issues/22209

PVince81 commented 7 years ago

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

Let's do some experimentation...

PVince81 commented 7 years ago
PVince81 commented 7 years ago

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

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 commented 7 years ago

Here's an experimental PR that gets rid of the file/folder share backends: https://github.com/owncloud/core/pull/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: https://github.com/owncloud/core/pull/26608/commits/42ba7db1f0142ee12a12a0df7cc5f4e2ef8f05da#diff-f7a9bb6da381ad3d1651acb5f47162a1R149

@labkode and for the sharee (autocomplete), this might be needed to avoid errors: https://github.com/owncloud/core/pull/26608/commits/8b2a6c43cb4a6949f5a1fc8425ebc109b1a6a94e

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 commented 7 years ago

After working on https://github.com/owncloud/core/pull/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: https://github.com/owncloud/core/issues/22209#issuecomment-259991991

PVince81 commented 7 years ago

... 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 commented 7 years ago

So, in this PR https://github.com/owncloud/core/pull/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 commented 7 years ago

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)

labkode commented 7 years ago

@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:

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 42ba7db1f0142ee12a12a0df7cc5f4e2ef8f05da and the UI and share via link continue to work, user and groups shares still not.

PVince81 commented 7 years ago

@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 commented 7 years ago

@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: 

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 commented 7 years ago

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 https://github.com/owncloud/core/issues/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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 https://github.com/owncloud/core/issues/26607#issuecomment-260285405 ? 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

To be clear with the scenario:

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?

PVince81 commented 7 years ago

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.

labkode commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

but great that you found the problem with a solution :smile:

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 commented 7 years ago

@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 commented 7 years ago

@PVince81 Sure

lock[bot] commented 5 years ago

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.