Sharestorageprovider oc10 sm #2023

butonic · 2021-08-31T10:05:42Z

continuation of #1846 and #1584

rebased and fixed mocks
prototype share manager changes as described in make share manager single source of truth #2001
check if the deleted webdav / ocm code is breaking things ... and what it was supposed to handle

update-docs · 2021-08-31T10:05:45Z

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.

aduffeck · 2021-09-22T09:06:19Z

Following our discussion these are the remaining tasks to implement:

Change the ocs API to check for and handle collisions with already existing shares or other nodes on the storage when accepting shares (@butonic, @refs)
Extend the sharesstorageprovider to consider all shares pointing to the mountpoint in question, e.g. when renaming or deleting (declining) a share (@butonic, @refs)
Make the sql share manager completely hide the fact that accepted group shares can be represented as child shares in the database (@aduffeck)

We also concluded that spaces support will not be implemented as part of this PR.

refs · 2021-09-23T12:39:05Z

2b9adac (#2023) should address points 1 and 2 from the last comment.

There is however a tradeoff that we made. When there are 2 shares to the same resource, one is a group share and the other a user share, the group share is excluded from the results, so there are not 2 folders to the same location, this is as we discussed.

However, when listing all shares (in the sahred with me tab) we should list both received shares, as the user might want to decline them (i.e update the share state). This is currently not possible since listing the shares (by navigating to /Shares/) uses the same endpoint as the shared with me endpoint, and there is no filtering options to hide shares which point to the same resource.

@aduffeck could you udate the todo list with a checkmark for both tasks? I for some reason am not able to do so :(

//cc @butonic

aduffeck · 2021-09-23T13:15:48Z

@refs I'm not really following the problem with the same endpoint being used in /Shares and the shared-with-me endpoint. /Shares uses the sharesstorageprovider which talks to the share manager directly and can - and in fact does - merge the two shares to the same resource together, no?

refs · 2021-09-23T13:48:25Z

@refs I'm not really following the problem with the same endpoint being used in /Shares and the shared-with-me endpoint. /Shares uses the sharesstorageprovider which talks to the share manager directly and can - and in fact does - merge the two shares to the same resource together, no?

@aduffeck What I meant is when we list shares on "Shared with me":

I think it would be better if we list all of the shares, group and user shares alongside, even if the mount point is the same. And I'm comparing it to this view:

navigating to /Shares from the web ui actually renders the mount point, but here we should hide the group share (based in: user shares are more explicit, and just hide the less explicit shares, totally my choice, we can decide on this).

The issue comes that these 2 different pages use the same endpoint, and there are currently no filters in the CS3APIS to capture this behavior. What I want is to use the same endpoint and fetch different results depending on where the request was made. Maybe I'm missing an essential part elsewhere 🤷

butonic · 2021-09-28T10:43:48Z

internal/grpc/services/gateway/storageprovider.go

+						Status: status.NewStatusFromErrType(ctx, "stat ref: "+req.Ref.String(), err),
+					}, nil
+				}
+				_, _ = io.WriteString(etagHash, childStatRes.Info.Etag)


need to check childStatRes.Status.Code != OK or something

butonic · 2021-09-28T10:44:56Z

storage-sharing_1        | {"level":"error","service":"storage","pkg":"rgrpc","error":"Error 1064: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'results AS\n\t\t(\n\t\t\tSELECT s.*, storages.numeric_id FROM oc_share s\n\t\t\tLEFT JOIN o' at line 1","time":"2021-09-28T10:41:09.102292283Z","caller":"/home/jfd/Repositories/reva/internal/grpc/services/usershareprovider/usershareprovider.go:193","message":"error listing received shares"}

mysql hates us ;-)

lgtm-com · 2021-10-07T07:06:59Z

This pull request introduces 1 alert when merging 18cdc37 into 937db22 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-07T08:22:36Z

This pull request introduces 1 alert when merging 52cabec into 937db22 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-08T09:07:03Z

This pull request introduces 1 alert when merging 1329eb9 into 9be4c4f - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-08T09:29:32Z

This pull request introduces 1 alert when merging 3b51cd5 into 9be4c4f - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-26T14:57:39Z

This pull request introduces 1 alert when merging 92c6605 into fe35bd1 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-27T08:12:37Z

This pull request introduces 1 alert when merging 4f0949d into ab1db2a - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-27T08:39:16Z

This pull request introduces 1 alert when merging a5ca0f3 into ab1db2a - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-10-27T08:56:57Z

This pull request introduces 1 alert when merging df6021e into ab1db2a - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

butonic · 2021-10-29T14:55:03Z

The #2215 (comment) created a really ugly merge conflict: the findEmbeddedMounts() introduced in this PR solves the problem #2215 adresses in a different way. We are going to prepare tests for 'virtual views' and then make sure this PR does not break them.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic · 2021-11-04T10:23:04Z

internal/grpc/services/gateway/storageprovider.go

-		if protocol == "webdav" {
-			// TODO(ishank011): pass this through the datagateway service
-			// For now, we just expose the file server to the user
-			ep, opaque, err := s.webdavRefTransferEndpoint(ctx, statRes.Info.Target)
-			if err != nil {
-				return &gateway.InitiateFileDownloadResponse{
-					Status: status.NewInternal(ctx, err, "gateway: error downloading from webdav host: "+p),
-				}, nil
-			}
-			return &gateway.InitiateFileDownloadResponse{
-				Status: status.NewOK(ctx),
-				Protocols: []*gateway.FileDownloadProtocol{
-					{
-						Opaque:           opaque,
-						Protocol:         "simple",
-						DownloadEndpoint: ep,
-					},
-				},
-			}, nil
-		}


@ishank011 there are several places in the gateway that have special "webdav" handling ... what for? AFAICT it is related to OCM. Unfortunately, there are no tests covering that right now. Well the oc10 core testsuite does cover federated sharing, but the drone ci does not set up a second instance ...

While there are ways forward to cover OCM let's start with what these webdav handling is meant to do? Could you enlighten us?

@butonic yep, the OCM share targets are saved as webdav://$token@$idp?name=$path. When OCM shares are accepted, the references have these targets instead of cs3 for other shares.
When checkRef is called and it detects that the reference is of type 'webdav', the requests are then forwarded to the methods in the webdavstorageprovider.go file, and they actually make the webdav calls to the idp of the share creator.

phew ... do you have an example setup with two reva instances that I can use to set up a test environment?

@ishank ... um $path? so the references will break when the path changes? Why are we not using ids? Is OCM necessary for your current deployment?

IMO the share storage provider will return a cs3 reference which resolves to a yet to be implemented OCM storage provider. That can encapsulate the token and idp data necessary to access the remote instance.

I am currently looking at the /examples/ocmd setup. AFAICT it is deployed next to an existing reva deployment ... is it only starts the ocmd service on the http port ... so normal webdav is not available without an additional reva deployment.

I started a PR to clean up the toml configs and add an ocm config set in #2239

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

ishank011 · 2021-11-29T09:29:33Z

internal/grpc/services/gateway/usershareprovider.go

-		}
-	}
-	return res, nil
+	return c.UpdateReceivedShare(ctx, req)
 }

 func (s *svc) removeReference(ctx context.Context, resourceID *provider.ResourceId) *rpc.Status {


Can you also remove this method?

ishank011 · 2021-11-29T09:33:11Z

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

@@ -96,6 +98,7 @@ func (h *DavHandler) Handler(s *svc) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 		log := appctx.GetLogger(ctx)
+		log.Info().Str("request", fmt.Sprintf("%#v", r)).Msg("Got webdav request")


Remove the log?

ishank011 · 2021-11-29T14:30:44Z

pkg/share/manager/json/json.go

+		for j := range rss {
+			if rss[i].Share.ResourceId.GetOpaqueId() == rss[j].Share.ResourceId.GetOpaqueId() {
+				if rss[i].Share.GetGrantee().GetType() == provider.GranteeType_GRANTEE_TYPE_GROUP && rss[j].Share.GetGrantee().GetType() == provider.GranteeType_GRANTEE_TYPE_USER {
+					if rss[i].State == rss[j].State {


Not sure about this. If the permissions differ for the shares, shouldn't be return both?

ishank011 · 2021-11-29T14:52:24Z

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

-		requestUserID, _ := router.ShiftPath(requestPath)
-		u = &userpb.User{
-			Username: requestUserID,
+		var requestUsernameOrID string


I left a couple of comments in the ldap refactor PR #2133. Can you take a look at those?

butonic · 2021-12-07T11:31:37Z

merged in edge as part of #2234

butonic requested a review from labkode as a code owner August 31, 2021 10:05

butonic marked this pull request as draft August 31, 2021 10:49

This was referenced Aug 31, 2021

[WIP] Sharestorageprovider oc10 sm #1846

Closed

add migration deployment owncloud/ocis#2302

Merged

make share manager single source of truth #2001

Open

butonic force-pushed the sharestorageprovider-oc10-sm branch 5 times, most recently from df77a6d to c0acb58 Compare September 9, 2021 08:25

aduffeck force-pushed the sharestorageprovider-oc10-sm branch from cad9c3b to 71b35bf Compare September 14, 2021 13:11

butonic commented Sep 28, 2021

View reviewed changes

aduffeck force-pushed the sharestorageprovider-oc10-sm branch 2 times, most recently from 536219a to 8c5266d Compare September 29, 2021 14:15

butonic mentioned this pull request Sep 30, 2021

Sharemanager api changes #2121

Merged

butonic force-pushed the sharestorageprovider-oc10-sm branch 4 times, most recently from d080fdd to 2581441 Compare October 5, 2021 13:32

aduffeck force-pushed the sharestorageprovider-oc10-sm branch from 52cabec to 9e0a31c Compare October 8, 2021 08:42

aduffeck added 3 commits October 27, 2021 09:55

Fix expected failures for s3ng

958f9b2

Be more robust when handling error cases

028b279

WIP: Fix restoring recycle items

4f0949d

aduffeck force-pushed the sharestorageprovider-oc10-sm branch from 92c6605 to 4f0949d Compare October 27, 2021 07:56

aduffeck added 2 commits October 27, 2021 10:22

Fix unit tests

a5ca0f3

Fix license headers

df6021e

Fix linter issues

a00d562

aduffeck force-pushed the sharestorageprovider-oc10-sm branch 4 times, most recently from 90c5b4d to a00d562 Compare October 29, 2021 11:53

butonic mentioned this pull request Nov 4, 2021

Spaces registry #2234

Merged

butonic added 3 commits November 4, 2021 10:06

Merge branch 'master' into sharestorageprovider-oc10-sm

606bfc4

add missing go.sum entry

38d8d72

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

fix import

e5bcf1e

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic commented Nov 4, 2021

View reviewed changes

butonic added 3 commits November 4, 2021 12:39

add embedded mounts when listing /home

5b5f8ed

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

remove unexpected passes

4e257e8

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

add old logic as comment as we want to bring parts of it back for OCM

d615470

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic marked this pull request as ready for review November 5, 2021 14:06

butonic requested a review from a team as a code owner November 5, 2021 14:06

ishank011 reviewed Nov 29, 2021

View reviewed changes

butonic closed this Dec 7, 2021

micbar mentioned this pull request Mar 3, 2022

Release 1.18.0 Tech Preview owncloud/ocis#3244

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharestorageprovider oc10 sm #2023

Sharestorageprovider oc10 sm #2023

butonic commented Aug 31, 2021 •

edited

Loading

update-docs bot commented Aug 31, 2021

aduffeck commented Sep 22, 2021 •

edited

Loading

refs commented Sep 23, 2021

aduffeck commented Sep 23, 2021

refs commented Sep 23, 2021

butonic Sep 28, 2021 •

edited

Loading

butonic commented Sep 28, 2021

lgtm-com bot commented Oct 7, 2021

lgtm-com bot commented Oct 7, 2021

lgtm-com bot commented Oct 8, 2021

lgtm-com bot commented Oct 8, 2021

lgtm-com bot commented Oct 26, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

butonic commented Oct 29, 2021 •

edited

Loading

butonic Nov 4, 2021

ishank011 Nov 4, 2021

butonic Nov 4, 2021

butonic Nov 5, 2021

butonic Nov 5, 2021

butonic Nov 5, 2021

butonic Nov 5, 2021

ishank011 Nov 29, 2021

ishank011 Nov 29, 2021

ishank011 Nov 29, 2021

ishank011 Nov 29, 2021

butonic commented Dec 7, 2021

Sharestorageprovider oc10 sm #2023

Sharestorageprovider oc10 sm #2023

Conversation

butonic commented Aug 31, 2021 • edited Loading

update-docs bot commented Aug 31, 2021

aduffeck commented Sep 22, 2021 • edited Loading

refs commented Sep 23, 2021

aduffeck commented Sep 23, 2021

refs commented Sep 23, 2021

butonic Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

butonic commented Sep 28, 2021

lgtm-com bot commented Oct 7, 2021

lgtm-com bot commented Oct 7, 2021

lgtm-com bot commented Oct 8, 2021

lgtm-com bot commented Oct 8, 2021

lgtm-com bot commented Oct 26, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

butonic commented Oct 29, 2021 • edited Loading

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

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 commented Dec 7, 2021

butonic commented Aug 31, 2021 •

edited

Loading

aduffeck commented Sep 22, 2021 •

edited

Loading

butonic Sep 28, 2021 •

edited

Loading

butonic commented Oct 29, 2021 •

edited

Loading