Ensure stray shares get ignored #1064

refs · 2020-08-06T14:51:13Z

Scope

JSON Shares Manager.

When using the json shares manager, it can be the case we found a share with a resource_id that no longer exists. This PR aims to address his case by changing the contract of getPath and return the result of the STAT call instead of a generic error, so we can check the cause.

This is by no means a perfect solution, but starts addressing the inconsistencies of the error handling across the code base.

update-docs · 2020-08-06T14:51:16Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

PVince81 · 2020-08-06T15:19:38Z

add changelog
follow linter

PVince81 · 2020-08-06T15:20:19Z

internal/grpc/services/gateway/storageprovider.go

@@ -139,7 +158,8 @@ func (s *svc) InitiateFileDownload(ctx context.Context, req *provider.InitiateFi
 		err := errtypes.PermissionDenied("gateway: cannot download share folder or share name: path=" + p)
 		log.Err(err).Msg("gateway: error downloading")
 		return &gateway.InitiateFileDownloadResponse{
-			Status: status.NewInvalidArg(ctx, "path points to share folder or share name"),
+			// Status: status.NewInvalidArg(ctx, "path points to share folder or share name"),


uncomment again ?

internal/grpc/services/gateway/storageprovider.go

PVince81 · 2020-08-06T15:20:47Z

internal/grpc/services/gateway/storageprovider.go

-		log.Err(err).Msg("gateway: error moving")
+	p, st := s.getPath(ctx, req.Source)
+	if st.Code != rpc.Code_CODE_OK {
+		// log.Err(err).Msg("gateway: error moving")


stray comment

PVince81 · 2020-08-06T15:21:19Z

recheck commented out code
check inline TODOs and fix or raise ticket for later

ishank011

@refs Really nice change! One comment though.

Also, we should have this for public shares as well, as there we support sharing single files and in EOS, the ResourceID changes every time a file is edited, so there you get a lot of such errors. We'll start working on fixing that at the fs level next week.

internal/grpc/services/gateway/storageprovider.go

individual-it · 2020-08-07T12:04:39Z

internal/grpc/services/gateway/storageprovider.go

 		}, nil
 	}

 	dp, err := s.getPath(ctx, req.Destination)
 	if err != nil {
-		log.Err(err).Msg("gateway: error moving")
+		// log.Err(err).Msg("gateway: error moving")


obsolete, or need to be re-enabled?

individual-it · 2020-08-10T06:25:47Z

moving a file or folder via webDAV fails with:

2020-08-10 12:09:16.81 DBG home/artur/www/cs3org/reva/pkg/storage/fs/owncloud/owncloud.go:429 > ocfs: unwrap: internal=/var/tmp/reva/data/einstein/files/destination external=/destination pid=35749 pkg=rgrpc traceid=5e237fe2ce00f9b0eef4a36ea004e093
2020-08-10 12:09:16.81 DBG home/artur/www/cs3org/reva/pkg/storage/fs/owncloud/owncloud.go:429 > ocfs: unwrap: internal=/var/tmp/reva/data/einstein/files/destination external=/destination pid=35749 pkg=rgrpc traceid=5e237fe2ce00f9b0eef4a36ea004e093
2020-08-10 12:09:16.81 WRN home/artur/www/cs3org/reva/internal/grpc/services/storageprovider/storageprovider.go:468 > file not found pid=35749 pkg=rgrpc traceid=5e237fe2ce00f9b0eef4a36ea004e093
2020-08-10 12:09:16.81 INF home/artur/www/cs3org/reva/internal/grpc/interceptors/log/log.go:69 > unary code=OK end="10/Aug/2020:12:09:16 +0545" from=tcp://127.0.0.1:54054 pid=35749 pkg=rgrpc start="10/Aug/2020:12:09:16 +0545" time_ns=475855 traceid=5e237fe2ce00f9b0eef4a36ea004e093 uri=/cs3.storage.provider.v1beta1.ProviderAPI/Stat user-agent=grpc-go/1.26.0
2020-08-10 12:09:16.81 INF home/artur/www/cs3org/reva/internal/grpc/interceptors/log/log.go:69 > unary code=OK end="10/Aug/2020:12:09:16 +0545" from=tcp://127.0.0.1:51918 pid=35762 pkg=rgrpc start="10/Aug/2020:12:09:16 +0545" time_ns=1152306 traceid=5e237fe2ce00f9b0eef4a36ea004e093 uri=/cs3.gateway.v1beta1.GatewayAPI/Stat user-agent=grpc-go/1.26.0
2020-08-10 12:09:16.811 ERR home/artur/www/cs3org/reva/internal/http/interceptors/log/log.go:112 > http end="10/Aug/2020:12:09:16 +0545" host=10.4.1.204 method=MOVE pid=35775 pkg=rhttp proto=HTTP/1.1 size=0 start="10/Aug/2020:12:09:16 +0545" status=500 time_ns=20329472 traceid=5e237fe2ce00f9b0eef4a36ea004e093 uri=/remote.php/webdav/src url=/remote.php/webdav/src

PVince81 · 2020-08-10T08:13:34Z

indeed, Litmus failed here in #1068:


10. move_coll............. FAIL (collection MOVE `/remote.php/webdav/litmus/mvsrc/' to `/remote.php/webdav/litmus/mvdest/': 500 Internal Server Error)
--

fix MOVE operation

PVince81 · 2020-08-10T09:10:01Z

fix MKCOL: [nomerge] Ocfs merge stray shares uid fixes #1068 (comment)
go through all other file operations and see that stray shares are not appearing

butonic · 2020-08-10T11:16:16Z

the list sharing endpoint should work: https://github.com/owncloud/core/pull/37754/files#diff-129a7916711570e3ddb2eb9f655ce577

PVince81 · 2020-08-10T12:20:06Z

    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsDELETEAuth.feature:54
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsDELETEAuth.feature:64
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsDELETEAuth.feature:73
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsDELETEAuth.feature:82
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsGETAuth.feature:76
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsGETAuth.feature:137
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsGETAuth.feature:253
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsGETAuth.feature:302
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsPOSTAuth.feature:74
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsPOSTAuth.feature:89
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsPOSTAuth.feature:104
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsPUTAuth.feature:43
    /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthOcs/ocsPUTAuth.feature:60
    /drone/src/tmp/testrunner/tests/acceptance/features/apiShareManagementBasic/createShare.feature:347
    /drone/src/tmp/testrunner/tests/acceptance/features/apiShareManagementBasic/createShare.feature:348

butonic · 2020-08-10T13:05:25Z

.drone.yml

@@ -342,9 +342,8 @@ steps:
    image: owncloudci/php:7.2
    commands:
      - git clone -b master --depth=1 https://github.com/owncloud/testing.git /drone/src/tmp/testing
-      - git clone -b master --single-branch --no-tags https://github.com/owncloud/core.git /drone/src/tmp/testrunner
+      - git clone -b testFixOCISPR409 --single-branch --no-tags https://github.com/owncloud/core.git /drone/src/tmp/testrunner


use reva-tests, see owncloud/core#37798

PVince81 · 2020-08-10T14:49:20Z

proposed fix:

in owncloud.go's getPath:

check if we received an existing path, and then call GetMd() (or whatever method for getting the oc fileid from storage)
if GetMd() failed with "not found", discard the value in redis and return an error as if the key did not exist
if GetMd() succeeded, compare the returned file id with the expected opaque id. if different, discard the value in redis and return an error as if the key did not exist
otherwise, return the path

refs · 2020-08-11T11:36:37Z

While working on this we realized we're using errors.Wrap all over the place on owncloud.go. GetMD(). I believe we should wrap on the lowest layers, in the context of this PR that would be getPath() and unwrapping on the top layers to assert on the underlying error type.

Signed-off-by: A.Unger <zyxancf@gmail.com>

This reverts commit 8dc7c90.

individual-it · 2020-08-12T14:06:04Z

tests/acceptance/features/apiOcisSpecific/apiShareManagementBasic-createShare.feature

@@ -84,7 +84,6 @@ Feature: sharing
      | 2               | 200             |

  @issue-ocis-reva-372 @issue-ocis-reva-243
-  # after fixing all issues delete this Scenario and use the one from oC10 core


why deleting this line

oops, got mixed up

individual-it · 2020-08-12T14:06:57Z

tests/acceptance/expected-failures.txt

@@ -912,3 +912,7 @@ apiWebdavUpload2/uploadFileUsingOldChunking.feature:98
 apiWebdavUpload2/uploadFileUsingOldChunking.feature:99
 apiWebdavUpload2/uploadFileUsingOldChunking.feature:100
 apiWebdavUpload2/uploadFileUsingOldChunking.feature:101
+#
+# https://github.com/owncloud/ocis-reva/issues/372 Listing shares via ocs API does not show path for parent folders
+apiShareManagementBasic/createShare.feature:269


@toImplementOnOCIS tag removed here owncloud/core#37809 maybe the commit id must be advanced

but this is not the same file as core, this is the local feature for reva.

even if we change anything in core this PR is still green, because we are already expecting the failures.

butonic

there was some testing confusion, but this now works as intended.

ishank011

@refs A couple of comments.

ishank011 · 2020-08-12T15:33:59Z

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

@@ -1295,7 +1295,13 @@ func (h *Handler) listUserShares(r *http.Request, filters []*collaboration.ListS
 			}

 			if statResponse.Status.Code != rpc.Code_CODE_OK {
-				return nil, errors.New("could not stat share target")
+				if statResponse.Status.Code == rpc.Code_CODE_NOT_FOUND {


Can we also add this in listPublicShares?

totally. I'd get hands on it tomorrow morning first thing.

Actually giving this a second thought, as this PR has been messy enough with the testing I would like to create a new one for these changes and keep changes as "atomic" as possible. This PR is blocking us from finishing http://github.com/owncloud/ocis/pull/409 and the earlier we merge it the better 🙄

Oh okay. No worries then. I'll merge it

ishank011 · 2020-08-12T15:40:05Z

internal/grpc/services/gateway/storageprovider.go

-		if res.Status.Code != rpc.Code_CODE_OK {
-			err := status.NewErrorFromCode(res.Status.Code, "gateway")
-			return "", err
+		if (res != nil && res.Status.Code != rpc.Code_CODE_OK) || err != nil {


Do we need to check res != nil? There might arise cases where we might have initialized it and then run into some error but returned it anyway.

ignore stray shares

50420e1

refs requested a review from labkode as a code owner August 6, 2020 14:51

Merge branch 'master' into fix-error-propagation

95927bf

refs requested a review from PVince81 August 6, 2020 14:58

kulmann mentioned this pull request Aug 6, 2020

Update deps 2020 07 22 owncloud/ocis#409

Merged

63 tasks

PVince81 reviewed Aug 6, 2020

View reviewed changes

internal/grpc/services/gateway/storageprovider.go Show resolved Hide resolved

PVince81 reviewed Aug 6, 2020

View reviewed changes

ishank011 reviewed Aug 6, 2020

View reviewed changes

internal/grpc/services/gateway/storageprovider.go Outdated Show resolved Hide resolved

refs mentioned this pull request Aug 7, 2020

Ocfs lookup userid (update) #1052

Merged

refs added 2 commits August 7, 2020 13:39

stray block

7650a6d

log reason of STAT failure

18ff637

individual-it reviewed Aug 7, 2020

View reviewed changes

refs added 2 commits August 7, 2020 14:24

fix warnings and offenses

c433b7d

add changelog

6bd190f

labkode requested a review from ishank011 August 7, 2020 13:27

PVince81 mentioned this pull request Aug 10, 2020

[nomerge] Ocfs merge stray shares uid fixes #1068

Closed

fix move1

0ef80b3

test using testFixOCISPR409

79c6a59

refs changed the title ~~Ensure stray shares get ignored (json share manager)~~ [WIP] Ensure stray shares get ignored (json share manager) Aug 10, 2020

Merge branch 'master' into fix-error-propagation

e10e878

butonic mentioned this pull request Aug 10, 2020

[Tests-Only] [nomerge] dedicated reva test suite changes owncloud/core#37798

Closed

butonic requested changes Aug 10, 2020

View reviewed changes

use reva-tests branch

d6f059f

Merge branch 'master' into fix-error-propagation

f0aeb92

refs mentioned this pull request Aug 11, 2020

Proper error wrapping #1075

Open

refs added 8 commits August 11, 2020 13:43

make getPath deal with out of sync cached values

26576d1

add changelog

391ad65

Signed-off-by: A.Unger <zyxancf@gmail.com>

update drone commit

8dc7c90

Revert "update drone commit"

7dc3bd5

This reverts commit 8dc7c90.

Merge branch 'master' into fix-error-propagation

0146550

copy feature over from core

1b1b910

adapt assert

2489aad

add expected failures on owncloud/core to the list of expected failures

5a104df

individual-it reviewed Aug 12, 2020

View reviewed changes

butonic previously approved these changes Aug 12, 2020

View reviewed changes

re-add missing comment

ee3bf31

refs dismissed butonic’s stale review via ee3bf31 August 12, 2020 14:20

get rid of expected failures (╯°□°）╯︵ ┻━┻

b00e51f

refs changed the title ~~[WIP] Ensure stray shares get ignored (json share manager)~~ Ensure stray shares get ignored Aug 12, 2020

butonic approved these changes Aug 12, 2020

View reviewed changes

ishank011 reviewed Aug 12, 2020

View reviewed changes

ishank011 approved these changes Aug 12, 2020

View reviewed changes

ishank011 merged commit f325194 into cs3org:master Aug 12, 2020

This was referenced Aug 13, 2020

Fix error handling on the PublicShare manager #1087

Closed

Ignore Stray Public Shares #1090

Merged

butonic mentioned this pull request Aug 19, 2020

update reva to v1.1.1-0.20200819100654-dcbf0c8ea187 owncloud/ocis-reva#447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure stray shares get ignored #1064

Ensure stray shares get ignored #1064

refs commented Aug 6, 2020 •

edited

Loading

update-docs bot commented Aug 6, 2020

PVince81 commented Aug 6, 2020

PVince81 Aug 6, 2020

PVince81 Aug 6, 2020

PVince81 commented Aug 6, 2020

ishank011 left a comment

individual-it Aug 7, 2020

refs Aug 7, 2020

individual-it commented Aug 10, 2020

PVince81 commented Aug 10, 2020

PVince81 commented Aug 10, 2020

butonic commented Aug 10, 2020

PVince81 commented Aug 10, 2020

butonic Aug 10, 2020

PVince81 commented Aug 10, 2020

refs commented Aug 11, 2020

individual-it Aug 12, 2020

refs Aug 12, 2020

individual-it Aug 12, 2020

refs Aug 12, 2020

butonic Aug 12, 2020

butonic left a comment

ishank011 left a comment

ishank011 Aug 12, 2020

refs Aug 12, 2020

refs Aug 12, 2020

ishank011 Aug 12, 2020

ishank011 Aug 12, 2020

Ensure stray shares get ignored #1064

Ensure stray shares get ignored #1064

Conversation

refs commented Aug 6, 2020 • edited Loading

Scope

update-docs bot commented Aug 6, 2020

PVince81 commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Aug 6, 2020

ishank011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

individual-it commented Aug 10, 2020

PVince81 commented Aug 10, 2020

PVince81 commented Aug 10, 2020

butonic commented Aug 10, 2020

PVince81 commented Aug 10, 2020

Choose a reason for hiding this comment

PVince81 commented Aug 10, 2020

refs commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic left a comment

Choose a reason for hiding this comment

ishank011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refs commented Aug 6, 2020 •

edited

Loading