use a relative reference for the stat request in the cs3 api

wkloucek commented 2 years ago

this is about this code path: https://github.com/cs3org/reva/blob/208ecee6d113e2b323c80e366a92eedb776200f9/pkg/storage/utils/decomposedfs/node/node.go#L512-L520

If we omit the path='.' we'll get a stat response containing the absolute path of the file (not the filename only)

glpatcern commented 2 years ago

That's interesting, is that a feature or a bug? ;-)

Jokes apart, if it is intended that the full path be exposed (including in public links and single-file shares), one might not need the special <parent_opaque_id>/<base_filename> format. Or am I missing something?

wkloucek commented 2 years ago

That's interesting, is that a feature or a bug? ;-)

A feature, but I didn't knew about it before, because I intuitively always used references with all three (storage-id, opaque-id and path=".")

Jokes apart, if it is intended that the full path be exposed (including in public links and single-file shares), one might not need the special <parent_opaque_id>/<base_filename> format. Or am I missing something?

You'll never see paths outside of your permissions. Therefore the could be different for you and me.

In a single file public share you should see something like /single-file.example

glpatcern commented 2 years ago

In a single file public share you should see something like /single-file.example

OK, good to know about this feature indeed. But with that case, you really need the other patch such that

filepath = statInfo.info.parent_id.opaque_id + '/' + os.path.basename(statInfo.info.path)

in all cases. Correct?

glpatcern commented 2 years ago

I'm just thinking that we could use that form everywhere and that's it, i.e. dropping the first case from _getcs3reference(). Also in the EOS world where we are exposing the full paths, WOPI never needs to go above the parent folder.

wkloucek commented 2 years ago

In a single file public share you should see something like /single-file.example

OK, good to know about this feature indeed. But with that case, you really need the other patch such that
filepath = statInfo.info.parent_id.opaque_id + '/' + os.path.basename(statInfo.info.path)
in all cases. Correct?

You'll only see /single-file.example if you omit the path=".". Since we include it in this PR, we will see single-file.example. Therefore we will never run into the first _getcs3reference() case with REVA edge. But REVA master could still use that.

I'm just thinking that we could use that form everywhere and that's it, i.e. dropping the first case from _getcs3reference(). Also in the EOS world where we are exposing the full paths, WOPI never needs to go above the parent folder.

As far as I know, EOS doesn't yet support relative references!?

glpatcern commented 2 years ago

You'll only see /single-file.example if you omit the path=".". Since we include it in this PR, we will see single-file.example. Therefore we will never run into the first _getcs3reference() case with REVA edge. But REVA master could still use that.

Yes, except that once the PR is merged, also Reva master will (eventually) be exposed to those relative paths (assuming they are implemented in EOS - which is expected to happen, otherwise how are we going to support spaces?). What I mean is: at the moment the code path in this PR is used by Reva master and edge on the very first stat() call in order to generate the access_token, and depending what this first call returns, we may use afterwards this hybrid parent_id/filename references everywhere else.

wkloucek commented 2 years ago

You'll only see /single-file.example if you omit the path=".". Since we include it in this PR, we will see single-file.example. Therefore we will never run into the first _getcs3reference() case with REVA edge. But REVA master could still use that.

Yes, except that once the PR is merged, also Reva master will (eventually) be exposed to those relative paths (assuming they are implemented in EOS - which is expected to happen, otherwise how are we going to support spaces?). What I mean is: at the moment the code path in this PR is used by Reva master and edge on the very first stat() call in order to generate the access_token, and depending what this first call returns, we may use afterwards this hybrid parent_id/filename references everywhere else.

True... We could add a flag for the reference types:

[cs3]
cs3api_reference_type = absolute # "absolute" for REVA master, "relative" for REVA edge

glpatcern commented 2 years ago

I think we can get along in a better way than by a configuration flag...

These days I'm off but for next week I propose the following, given that we have added the parent_id response on Reva as part of cs3org/reva#2444:

I test the behavior of WOPI against Reva master (+ the above PR, with EOS) to work with relative paths, in the sense of supporting Stat() calls where the Reference includes storage_id = "space_id" = <eosinstance>, opaque_id = parent_id and path = ./filename. In there, <eosinstance> is the unique identifier of the EOS instance, e.g. eoshome-a, that we currently get in the /openinapp call.
If that works, I think we don't need this patch and we can just drop the case of absolute paths altogether
If that does not work, we see what options we have when including path='.' on the first stat

glpatcern commented 2 years ago

Just to keep this up to date: we have not merged this yet because (as I suspected) we do have an issue in the eosfs storage provider when calling Stat() with a given resourceid and path='.': I understand the expected response should contain a path='./filename.ext' whereas at the moment it returns some semi-absolute path, which breaks the rest.

micbar commented 2 years ago

Just to keep this up to date: we have not merged this yet because (as I suspected) we do have an issue in the eosfs storage provider when calling Stat() with a given resourceid and path='.': I understand the expected response should contain a path='./filename.ext' whereas at the moment it returns some semi-absolute path, which breaks the rest.

Please check https://github.com/owncloud/ocis/issues/3693

glpatcern commented 2 years ago

Interestingly, I'm not able to contribute to the branch, therefore I post here a patch I would include as well - which relates to https://github.com/owncloud/ocis/issues/3693 indeed. Short summary is that yes it "looks good", but with the explicit caveats in the following comments:

diff --git a/src/core/cs3iface.py b/src/core/cs3iface.py
index 4f1b305..f817876 100644
--- a/src/core/cs3iface.py
+++ b/src/core/cs3iface.py
@@ -53,10 +53,7 @@ def getuseridfromcreds(token, _wopiuser):
 def _getcs3reference(endpoint, fileref):
     '''Generates a CS3 reference for a given fileref, covering the following cases:
     absolute path, relative hybrid path, fully opaque fileid'''
-    if fileref[0] == '/':
-        # assume this is a filepath
-        ref = cs3spr.Reference(path=fileref)
-    elif fileref.find('/') > 0:
+    if fileref.find('/') > 0:
         # assume we have a relative path in the form `<parent_opaque_id>/<base_filename>`,
         # also works if we get `<parent_opaque_id>/<path>/<filename>`
         ref = cs3spr.Reference(resource_id=cs3spr.ResourceId(storage_id=endpoint,
@@ -64,7 +61,7 @@ def _getcs3reference(endpoint, fileref):
                                path='.' + fileref[fileref.find('/'):])
     else:
         # assume we have an opaque fileid
-        ref = cs3spr.Reference(resource_id=cs3spr.ResourceId(storage_id=endpoint, opaque_id=fileref),path='.')
+        ref = cs3spr.Reference(resource_id=cs3spr.ResourceId(storage_id=endpoint, opaque_id=fileref), path='.')
     return ref

@@ -80,8 +77,8 @@ def authenticate_for_test(userid, userpwd):

 def stat(endpoint, fileref, userid, versioninv=1):
     '''Stat a file and returns (size, mtime) as well as other extended info using the given userid as access token.
-    Note that endpoint here means the storage id, and fileref can be either a path (which MUST begin with /),
-    or an id (which MUST NOT start with a /). The versioninv flag is natively supported by Reva.'''
+    Note that endpoint here means the storage id, and fileref can be either a path in the form (parent_id/base_filename)
+    # or a pure id (cf. _getcs3reference). The versioninv flag is natively supported by Reva.'''
     tstart = time.time()
     ref = _getcs3reference(endpoint, fileref)
     statInfo = ctx['cs3gw'].Stat(request=cs3sp.StatRequest(ref=ref), metadata=[('x-access-token', userid)])
@@ -100,12 +97,9 @@ def stat(endpoint, fileref, userid, versioninv=1):
         raise IOError('Unexpected type %d' % statInfo.info.type)

     inode = common.encodeinode(statInfo.info.id.storage_id, statInfo.info.id.opaque_id)
-    # in case we got a relative path, build an hybrid path that can be used to reference the file:
-    # note that as per specs the parent_id MUST be available in this case
-    if statInfo.info.path[0] == '/':
-        filepath = statInfo.info.path
-    else:
-        filepath = statInfo.info.parent_id.opaque_id + '/' + os.path.basename(statInfo.info.path)
+    # here we build an hybrid path that can be used to reference the file, as the path is actually just the basename
+    # (and eventually the CS3 APIs should be updated to reflect that): note that as per specs the parent_id MUST be available
+    filepath = statInfo.info.parent_id.opaque_id + '/' + os.path.basename(statInfo.info.path)
     log.info('msg="Invoked stat" fileref="%s" trace="%s" inode="%s" filepath="%s" elapsedTimems="%.1f"' %
              (fileref, statInfo.status.trace, inode, filepath, (tend-tstart)*1000))

micbar commented 2 years ago

@wkloucek can you integrate the patch?

wkloucek commented 2 years ago

@glpatcern thanks for the patch! I applied it and did some quick testing. Looks good from my side

cs3org / wopiserver

use a relative reference for the stat request in the cs3 api #74