Public Links Download / Upload #822

refs · 2020-06-10T11:40:26Z

Downloads from a public link. There are a few loose ends:

It has not yet been tried with the DataGateway
It has not yet been tested with a storage other than Owncloud's
- the path conversion then remains a black box I have not yet tapped into

Basic functionality for downloading using a configured data provider however is there.

refs · 2020-06-10T11:41:22Z

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

+	return &PathTranslator{
+		dir:   filepath.Dir(p),
+		base:  filepath.Base(p),
+		token: strings.Split(p, "/")[2],


this does not change for requests to the public storage

labkode · 2020-06-11T16:56:19Z

@refs two questions:

What is the purpose of the path translator?

What do you need to check for owncloud storage?

refs · 2020-06-11T21:13:25Z

Hi @labkode :)

What is the purpose of the path translator?

It's a dumb abstraction, as I'm not so familiar with the storages just yet, all the path operations got me a little lost, so instead of having 4 different variables this layer helps me with the steps. Needless to say, it can be removed in favor of concise code.

What do you need to check for owncloud storage?

The way I test this is with the Owncloud storage, where a path looks like /oc/{username}/.... I could not think of another solution when translating public link paths, i.e: /{token}/folderA/folderB into internal path.

PVince81 · 2020-06-17T13:11:20Z

Something is fishy:

2020-06-17T15:03:19+02:00 ERR error starting the grpc server error="unable to register services: rgrpc: grpc service publicstorageprovider could not be started,: driver not found: " service=reva

This happens both with and without the fix from #848 merged into this branch checked out locally.

On master and on #848 all alone the service starts fine.

PVince81 · 2020-06-17T13:12:44Z

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

+	}
+
+	if isOCStorage(tokenPath) {
+		base := strings.Join(strings.Split(tokenPath, "/")[3:], "/")


are paths normalized ? could there be leaks in case of double slashes ?
does Golang have a library for properly handling paths and deduplicating separators ?

the path package

PVince81 · 2020-06-17T13:17:45Z

ok seems there are some new config values: https://github.com/cs3org/reva/pull/822/files#diff-93a24189b46141fd854292e592c3be2aR56

@refs please provide more info about how to test this

PVince81 · 2020-06-17T13:24:27Z

probably also need to extend the default config structure to have default settings for these new config values ?

PVince81 · 2020-06-17T13:28:55Z

ok, so it seems this PR also needs owncloud/ocis-reva#258

@refs would be good to always explicitly list the requirements for testing in the top post

PVince81 · 2020-06-17T13:42:50Z

BUG: When sharing a single file, opening the public link shows an empty folder. If single file shares are not supported we should prevent such shares to be created in the first place

PVince81 · 2020-06-18T08:00:05Z

I managed to run some API tests locally:

It seems the upload has troubles renaming the file to the final location:

==> reva-storage-home-data.log <==
2020-06-18T09:56:31+02:00 INF access token is already provided pkg=rhttp service=reva traceid=5cd961cf67b45a04f6c8dff718e05cc9
2020-06-18T09:56:31+02:00 DBG http routing: head=data tail=/4fac97d6-c65a-4a0d-8b68-a46f765632de svc=data pkg=rhttp service=reva
[tusd] 2020/06/18 09:56:31 event="RequestIncoming" method="PATCH" path="/4fac97d6-c65a-4a0d-8b68-a46f765632de" requestId="" 
2020-06-18T09:56:31+02:00 INF tusd routing: path=/4fac97d6-c65a-4a0d-8b68-a46f765632de pkg=rhttp service=reva traceid=5cd961cf67b45a04f6c8dff718e05cc9
[tusd] 2020/06/18 09:56:31 event="ChunkWriteStart" id="4fac97d6-c65a-4a0d-8b68-a46f765632de" maxSize="11" offset="0" 
[tusd] 2020/06/18 09:56:31 event="ChunkWriteComplete" id="4fac97d6-c65a-4a0d-8b68-a46f765632de" bytesWritten="11" 
9:56AM ERR home/vincent/Private/Work/workspace/reva/pkg/storage/fs/owncloud/upload.go:349 > ocfs: could not rename error="rename /var/tmp/reva/data/Alice/uploads/4fac97d6-c65a-4a0d-8b68-a46f765632de /var/tmp/reva/data/Alice/files/PARENT/randomfile.txt: no such file or directory" binPath=/var/tmp/reva/data/Alice/uploads/4fac97d6-c65a-4a0d-8b68-a46f765632de info={"ID":"4fac97d6-c65a-4a0d-8b68-a46f765632de","IsFinal":false,"IsPartial":false,"MetaData":{"dir":"/PARENT","filename":"randomfile.txt"},"Offset":11,"PartialUploads":null,"Size":11,"SizeIsDeferred":false,"Storage":{"BinPath":"/var/tmp/reva/data/Alice/uploads/4fac97d6-c65a-4a0d-8b68-a46f765632de","Idp":"https://localhost:9200","InternalDestination":"/var/tmp/reva/data/Alice/files/PARENT/randomfile.txt","LogLevel":"trace","Type":"OwnCloudStore","UserId":"Alice","UserName":"Alice"}} np=/var/tmp/reva/data/Alice/files/PARENT/randomfile.txt pid=13634
[tusd] 2020/06/18 09:56:31 event="ResponseOutgoing" status="404" method="PATCH" path="/4fac97d6-c65a-4a0d-8b68-a46f765632de" error="upload not found" requestId="" 
2020-06-18T09:56:31+02:00 WRN http end="18/Jun/2020:09:56:31 +0200" host=127.0.0.1 method=PATCH pkg=rhttp proto=HTTP/1.1 service=reva size=17 start="18/Jun/2020:09:56:31 +0200" status=404 time_ns=1293949 traceid=5cd961cf67b45a04f6c8dff718e05cc9 uri=/data/4fac97d6-c65a-4a0d-8b68-a46f765632de url=/data/4fac97d6-c65a-4a0d-8b68-a46f765632de

==> reva-frontend.log <==
2020-06-18T09:56:31+02:00 ERR Could not start TUS upload error="unexpected status code: 404" pkg=rhttp service=reva traceid=0b1a21dbfa37ce40f43e5cb5dd6ab99c
2020-06-18T09:56:31+02:00 ERR http end="18/Jun/2020:09:56:31 +0200" host=127.0.0.1 method=PUT pkg=rhttp proto=HTTP/1.1 service=reva size=0 start="18/Jun/2020:09:56:31 +0200" status=500 time_ns=57027080 traceid=0b1a21dbfa37ce40f43e5cb5dd6ab99c uri=/remote.php/webdav/PARENT/randomfile.txt url=/remote.php/webdav/PARENT/randomfile.txt

To reproduce, run this command in the owncloud/core repo with the test setup from https://owncloud.github.io/extensions/ocis_reva/testing/#use-existing-tests-for-bdd

% make test-acceptance-api \
TEST_SERVER_URL=http://localhost:9140 \
TEST_EXTERNAL_USER_BACKENDS=true \
TEST_OCIS=true \
OCIS_REVA_DATA_ROOT=/var/tmp/reva/ \
SKELETON_DIR=$HOME/work/workspace/owncloud/apps-external/testing/data/webUISkeleton \
BEHAT_FEATURE=tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature:107

PVince81 · 2020-06-18T08:02:56Z

oh, it seems it's the regular upload, not public link that has troubles:

    And user "Alice" has uploaded file with content "Random data" to "/PARENT/randomfile.txt"                                                                                                                    # FeatureContext::userHasUploadedAFileWithContentTo()
      HTTP status code 500 is not one of the expected values
      Failed asserting that an array contains 500.

PVince81 · 2020-06-18T08:04:40Z

unrelated to this PR, also happens on reva master... hmmm

PVince81 · 2020-06-18T08:09:13Z

I've raised https://github.com/owncloud/ocis-reva/issues/280 for when uploading to a non-existing folder.

I suspect that the skeleton files for the test have changed so I'll need to adjust the test

PVince81 · 2020-06-18T08:13:20Z

ok, so I ran the tests with the wrong skeleton folder...
the correct one is SKELETON_DIR=$HOME/work/workspace/owncloud/apps-external/testing/data/apiSkeleton and not the web ui skeleton.

with that in place, the test runs further.
and now I notice that this test is using a range request at the end, so not usable.

let's try other tests...

PVince81 · 2020-06-18T08:35:13Z

the test tests/acceptance/features/apiSharePublicLink1/createPublicLinkShare.feature:170 had troubles creating the share because of a permission issue, PR here to fix it: #852

now with that fix applied, the downloaded content is empty:

The downloaded content was expected to be 'Random data', but actually is ''.
        Failed asserting that two strings are equal.
        --- Expected
        +++ Actual
        @@ @@
        -'Random data'
        +''

PVince81 · 2020-06-18T15:29:41Z

REGRESSION: status code messed up with PUT, see https://github.com/owncloud/ocis-reva/issues/286

@refs please have a look at the second stat call in "put.go". The bug is that it reuses sRes but should use a second response object, because we need the result of the first response to know if the first stat returned null or not.

Here a possible patch:

diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go
index 5652dfc..dd55dc8 100644
--- a/internal/http/services/owncloud/ocdav/put.go
+++ b/internal/http/services/owncloud/ocdav/put.go
@@ -292,26 +292,26 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) {
        }
 
        // stat again to check the new file's metadata
-       sRes, err = client.Stat(ctx, sReq)
+       sRes2, err := client.Stat(ctx, sReq)
        if err != nil {
                log.Error().Err(err).Msg("error sending grpc stat request")
                w.WriteHeader(http.StatusInternalServerError)
                return
        }
 
-       if sRes.Status.Code != rpc.Code_CODE_OK {
-               log.Error().Err(err).Msgf("error status %d when sending grpc stat request", sRes.Status.Code)
+       if sRes2.Status.Code != rpc.Code_CODE_OK {
+               log.Error().Err(err).Msgf("error status %d when sending grpc stat request", sRes2.Status.Code)
                w.WriteHeader(http.StatusInternalServerError)
                return
        }
 
-       w.Header().Add("Content-Type", sRes.Info.MimeType)
-       w.Header().Set("ETag", sRes.Info.Etag)
-       w.Header().Set("OC-FileId", wrapResourceID(sRes.Info.Id))
-       w.Header().Set("OC-ETag", sRes.Info.Etag)
-       w.Header().Set("Last-Modified", utils.TSToTime(sRes.Info.Mtime).Format(time.RFC1123Z))
+       w.Header().Add("Content-Type", sRes2.Info.MimeType)
+       w.Header().Set("ETag", sRes2.Info.Etag)
+       w.Header().Set("OC-FileId", wrapResourceID(sRes2.Info.Id))
+       w.Header().Set("OC-ETag", sRes2.Info.Etag)
+       w.Header().Set("Last-Modified", utils.TSToTime(sRes2.Info.Mtime).Format(time.RFC1123Z))
 
-       // file was new
+       // file was new as it didn't exist during the first stat
        if sRes.Info == nil {
                w.WriteHeader(http.StatusCreated)
                return

labkode · 2020-06-22T08:48:51Z

@PVince81 @refs the usage of storage.FS from the publicstorageprovider package is bad. Please add the required method to the CS3APIs for initiating a file upload to the GatewayAPI and StorageProviderAPI.

PVince81 · 2020-06-23T13:10:12Z

internal/grpc/services/publicstorageprovider/publicstorageprovider.go

+
+// refFromToken returns the path for the publicly shared resource.
+func (s *service) refFromToken(ctx context.Context, token string) (*provider.Reference, error) {
+	driver, err := pool.GetPublicShareProviderClient(s.conf.DriverAddr)


duplicated from pathFromToken except for the return statement that wraps it
remove and use pathFromToken instead

PVince81 · 2020-06-23T14:46:55Z

for consistency: also use NewPathTranslator in stat (and any future operations)

butonic · 2020-06-23T14:55:10Z

alternate approach in #872

PVince81 · 2020-06-23T15:11:45Z

in #872 we have the public download working by talking to the gateway instead of storage after translating the paths, this obsoletes the need for a public link data provider and removes the need to know about the underlying storage as the gateway takes care of that by resolving the matching storage provider.

butonic · 2020-06-25T08:05:32Z

obsoleted byd #877, can be closed

first working draft

9299d27

refs requested a review from labkode as a code owner June 10, 2020 11:40

refs commented Jun 10, 2020

View reviewed changes

inline return

547c29b

refs mentioned this pull request Jun 10, 2020

Public Links Data Service owncloud/ocis-reva#258

Closed

working upload

5c268da

refs changed the title ~~Public Links Downloads~~ Public Links Download / Upload Jun 16, 2020

refs added 2 commits June 16, 2020 18:21

spare a few allocations

3ebcbaa

Merge branch 'master' into public-links-download

69cd5a0

PVince81 reviewed Jun 17, 2020

View reviewed changes

PVince81 mentioned this pull request Jun 17, 2020

Public link single file share owncloud/ocis-reva#278

Closed

refs mentioned this pull request Jun 17, 2020

Public Link Data Service Command owncloud/ocis#323

Closed

This was referenced Jun 18, 2020

Public link upload-only permissions should prevent download owncloud/ocis-reva#285

Closed

[Tests-only] Adjust public link tests for download, upload and other operations owncloud/core#37552

Merged

refs added 3 commits June 18, 2020 13:13

update branch

d37c4c6

provide missing parameter on InitiateUpload

8028f3f

single file publicly shared download working

e8362d8

refs added 7 commits June 18, 2020 19:18

filename on single shared file

575b62b

apply @PVince81 patch

3b6f6d5

take into account depth

625e459

improve base logic

1f7f453

Merge branch 'master' into public-links-download

52366d0

resolve append not assigned to same destination variable

5d692d2

disable gocritic for long ifElse chains

707fbe9

PVince81 reviewed Jun 23, 2020

View reviewed changes

butonic mentioned this pull request Jun 23, 2020

use gateway to resolve public share info #872

Closed

PVince81 mentioned this pull request Jun 24, 2020

Implementation of file operations in public folder shares #877

Merged

labkode closed this Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public Links Download / Upload #822

Public Links Download / Upload #822

refs commented Jun 10, 2020 •

edited

Loading

refs Jun 10, 2020

labkode commented Jun 11, 2020

refs commented Jun 11, 2020

PVince81 commented Jun 17, 2020 •

edited

Loading

PVince81 Jun 17, 2020

labkode Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020 •

edited

Loading

labkode commented Jun 22, 2020

PVince81 Jun 23, 2020

PVince81 commented Jun 23, 2020

butonic commented Jun 23, 2020

PVince81 commented Jun 23, 2020

butonic commented Jun 25, 2020

Public Links Download / Upload #822

Public Links Download / Upload #822

Conversation

refs commented Jun 10, 2020 • edited Loading

refs Jun 10, 2020

Choose a reason for hiding this comment

labkode commented Jun 11, 2020

refs commented Jun 11, 2020

PVince81 commented Jun 17, 2020 • edited Loading

PVince81 Jun 17, 2020

Choose a reason for hiding this comment

labkode Jun 17, 2020

Choose a reason for hiding this comment

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 17, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020

PVince81 commented Jun 18, 2020 • edited Loading

labkode commented Jun 22, 2020

PVince81 Jun 23, 2020

Choose a reason for hiding this comment

PVince81 commented Jun 23, 2020

butonic commented Jun 23, 2020

PVince81 commented Jun 23, 2020

butonic commented Jun 25, 2020

refs commented Jun 10, 2020 •

edited

Loading

PVince81 commented Jun 17, 2020 •

edited

Loading

PVince81 commented Jun 18, 2020 •

edited

Loading