ocdav: skip space lookup on spaces propfind #2977

butonic · 2022-06-16T11:33:50Z

we don't need to look up the space for spaces based requests. we already have it

fixes owncloud/ocis#1277
fixes owncloud/ocis#2144
fixes owncloud/ocis#3073

update-docs · 2022-06-16T11:33:53Z

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.

lgtm-com · 2022-06-16T11:58:35Z

This pull request introduces 3 alerts when merging b6f7a4f into f7b1b5f - view on LGTM.com

new alerts:

3 for Useless assignment to local variable

lgtm-com · 2022-06-16T12:22:58Z

This pull request introduces 3 alerts when merging f23e075 into f7b1b5f - view on LGTM.com

new alerts:

3 for Useless assignment to local variable

lgtm-com · 2022-06-16T13:51:45Z

This pull request introduces 3 alerts when merging 5be2321 into dffcaf7 - view on LGTM.com

new alerts:

3 for Useless assignment to local variable

internal/grpc/services/gateway/storageprovidercache.go

C0rby · 2022-07-01T11:05:05Z

internal/http/services/owncloud/ocdav/propfind/propfind.go

+		m := res.Status.Message
+		if res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED {
+			// check if user has access to resource
+			sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ref.GetResourceId()}})


What is the difference between this stat and the original one above?

The original stat might return an error, to check if we need to hide that error this stat checks if we are allowed to stat the space (well only the resource id, ignoring any path). If this stat returns OK we have read access and can return other errors like PrecondtitionFailed when an etag does not match.

The more I think about it, the more I think we should move 'hiding the existence of spaces' int the storage drivers. an id based lookup has to find the space first. if a user has no access to it we can always return 404 and stop any further request processing.

But that is a task for a separate PR.

But we already received a "Permission Denied" at this point so this reads like the user doesn't have access.
Otherwise the "Permission Denied" error is used incorrectly?

I have to admit this is really confusing. The testsuite does not even cover all requests and we actually need to do this error code handling for every request type...

I'll move this to the driver. That will also allow eos to return 403 forbidden and decomposedfs to return 404 not found. And we can even configure this kind of hiding per space.

internal/http/services/owncloud/ocdav/locks.go

C0rby · 2022-07-01T13:53:35Z

internal/http/services/owncloud/ocdav/propfind/propfind.go

+		m := res.Status.Message
+		if res.Status.Code == rpc.Code_CODE_PERMISSION_DENIED {
+			// check if user has access to resource
+			sRes, err := client.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ref.GetResourceId()}})


But we already received a "Permission Denied" at this point so this reads like the user doesn't have access.
Otherwise the "Permission Denied" error is used incorrectly?

internal/http/services/owncloud/ocdav/propfind/propfind.go

internal/http/services/owncloud/ocdav/report.go

internal/http/services/owncloud/ocdav/spacelookup/spacelookup.go

internal/http/services/owncloud/ocdav/versions.go

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

* ocdav: skip space lookup on spaces propfind Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * add changelog Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * update unit test Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * lint Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * reduce stat requests Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * update propfind unit tests Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * fix moq propfind comparison in propfind tests Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * lint Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * error handling and response codes Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * remove commented code Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * typos, mimic oc10 not found error message Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * add fieldmask to ListContainer and Stat Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * mimic oc10 not found error message Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * mimic oc10 not found error message in more places Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * fix oc:permissions Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * fix test compile Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * fix nc tests Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * lint Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * small cleanup Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * decomposedfs: add space if fieldmask is empty, * or has 'space' Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * fix propfind root resource path Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * update tests & changelog Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * storageprovidercache: use stringbuffer to build statKey Signed-off-by: Jörn Friedrich Dreyer <[email protected]> * comment cleanup Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

This reverts commit 2a6ff11.

butonic mentioned this pull request Jun 16, 2022

IO patterns in OCIS decomposed FS #2976

Open

butonic force-pushed the skip-space-lookup-on-space-propfind branch 8 times, most recently from c32a25d to ea1a285 Compare June 22, 2022 18:15

butonic force-pushed the skip-space-lookup-on-space-propfind branch 4 times, most recently from 5d106bc to 6d6beee Compare June 30, 2022 13:51

butonic marked this pull request as ready for review June 30, 2022 14:25

butonic requested review from labkode, ishank011, glpatcern and a team as code owners June 30, 2022 14:25

butonic mentioned this pull request Jul 1, 2022

render Propfind responses as xml stream #3024

Open

C0rby reviewed Jul 1, 2022

View reviewed changes

butonic force-pushed the skip-space-lookup-on-space-propfind branch from b21ce72 to a40f558 Compare July 6, 2022 07:38

butonic added 4 commits July 6, 2022 07:38

ocdav: skip space lookup on spaces propfind

fea09e9

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

add changelog

3221654

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

update unit test

7c0554b

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

lint

7b1b7e7

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic added 16 commits July 6, 2022 07:39

error handling and response codes

e8801ba

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

remove commented code

576a476

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

typos, mimic oc10 not found error message

28c7531

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

add fieldmask to ListContainer and Stat

9cecf95

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

mimic oc10 not found error message

1b591d7

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

mimic oc10 not found error message in more places

2bd1f7c

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix oc:permissions

9314060

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix test compile

e85c9d8

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix nc tests

9beebbc

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

lint

726b1ec

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

small cleanup

c64eb57

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

decomposedfs: add space if fieldmask is empty, * or has 'space'

2cfd280

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

fix propfind root resource path

50135f7

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

update tests & changelog

758715a

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

storageprovidercache: use stringbuffer to build statKey

b90148e

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

comment cleanup

0d040c3

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

butonic force-pushed the skip-space-lookup-on-space-propfind branch from a40f558 to 0d040c3 Compare July 6, 2022 07:39

C0rby approved these changes Jul 6, 2022

View reviewed changes

butonic merged commit e2232b5 into cs3org:edge Jul 6, 2022

butonic deleted the skip-space-lookup-on-space-propfind branch July 6, 2022 20:01

kobergj added a commit to kobergj/reva that referenced this pull request Jul 14, 2022

Revert "ocdav: skip space lookup on spaces propfind (cs3org#2977)"

bd2aae1

This reverts commit 2a6ff11.

micbar mentioned this pull request Jul 19, 2022

Release 2.0.0-beta.5 owncloud/ocis#4181

Closed

36 tasks

kobergj mentioned this pull request Oct 14, 2022

[tests-only] FixRescanning #3355

Merged

kiranparajuli589 mentioned this pull request Oct 20, 2022

[tests-only][full-ci] bump owncloud core commit for the tests (edge) #3380

Merged

kiranparajuli589 mentioned this pull request Oct 28, 2022

[tests-only][full-ci] Bump the oC/core commit ID for the tests" #3411

Merged

kobergj mentioned this pull request Dec 19, 2022

Fix Tags pkg #3564

Merged

kobergj mentioned this pull request May 22, 2023

Use defer to invalidate ID Cache #3910

Closed

kobergj mentioned this pull request Mar 1, 2024

Extend ServiceAccount Permissions #4544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocdav: skip space lookup on spaces propfind #2977

ocdav: skip space lookup on spaces propfind #2977

butonic commented Jun 16, 2022 •

edited

Loading

update-docs bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

C0rby Jul 1, 2022

butonic Jul 1, 2022

C0rby Jul 1, 2022

butonic Jul 4, 2022

C0rby Jul 1, 2022

ocdav: skip space lookup on spaces propfind #2977

ocdav: skip space lookup on spaces propfind #2977

Conversation

butonic commented Jun 16, 2022 • edited Loading

update-docs bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

lgtm-com bot commented Jun 16, 2022

C0rby Jul 1, 2022

Choose a reason for hiding this comment

butonic Jul 1, 2022

Choose a reason for hiding this comment

C0rby Jul 1, 2022

Choose a reason for hiding this comment

butonic Jul 4, 2022

Choose a reason for hiding this comment

C0rby Jul 1, 2022

Choose a reason for hiding this comment

butonic commented Jun 16, 2022 •

edited

Loading