From 857125577f2775d98a3806b88454f4c588a3cb6b Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 30 Jan 2024 16:41:11 +0100 Subject: [PATCH] graph/sharedWithMe: return shares for the same resource as a single driveItem - multiple shares for the same resource are now returned as a single driveItem - the id for that driveItem is for now based on the resourceId of the shared item {sharesstorageproviderid}${sharejailid}!{resourceid of shared item} - each share is exposed as a separate permission on the remoteId - the permission now has an invitation property which provides the id of the creator of the share - the client.synchronize flag is now exposed on the top-level driveitem. If at least on share of a resource is in accepted state the client.synchronize flag will be set to true. - the UI.Hidden flag is now exposed on the top-level driveitem. If at least on share of a resource is marked as hidden the UI.Hidden flag will be set to true. - the 'shared' property is no longer available (the relevant information from that moved to the 'invitation' property of the individual permissions. --- services/graph/pkg/service/v0/sharedwithme.go | 195 ++++++++++-------- .../graph/pkg/service/v0/sharedwithme_test.go | 137 ++++++------ 2 files changed, 179 insertions(+), 153 deletions(-) diff --git a/services/graph/pkg/service/v0/sharedwithme.go b/services/graph/pkg/service/v0/sharedwithme.go index 9a8ea148e..a0f907d4f 100644 --- a/services/graph/pkg/service/v0/sharedwithme.go +++ b/services/graph/pkg/service/v0/sharedwithme.go @@ -4,17 +4,16 @@ import ( "context" "net/http" "reflect" - "slices" cs3User "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" storageprovider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" - "github.com/cs3org/reva/v2/pkg/utils" "github.com/go-chi/render" libregraph "github.com/owncloud/libre-graph-api-go" "golang.org/x/sync/errgroup" "github.com/cs3org/reva/v2/pkg/storagespace" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/owncloud/ocis/v2/services/graph/pkg/errorcode" "github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole" @@ -48,6 +47,16 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er return nil, *errCode } + return g.cs3ReceivedSharesToDriveItems(ctx, listReceivedSharesResponse.GetShares()) +} + +func (g Graph) cs3ReceivedSharesToDriveItems(ctx context.Context, receivedShares []*collaboration.ReceivedShare) ([]libregraph.DriveItem, error) { + gatewayClient, err := g.gatewaySelector.Next() + if err != nil { + g.logger.Error().Err(err).Msg("could not select next gateway client") + return nil, err + } + // doStat is a helper function that stat a resource. doStat := func(resourceId *storageprovider.ResourceId) (*storageprovider.StatResponse, error) { shareStat, err := gatewayClient.Stat(ctx, &storageprovider.StatRequest{ @@ -67,28 +76,87 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er return shareStat, nil } + ch := make(chan libregraph.DriveItem) group := new(errgroup.Group) - receivedShares := listReceivedSharesResponse.GetShares() - driveItems := make([]libregraph.DriveItem, len(receivedShares)) + // Set max concurrency + group.SetLimit(10) - for i, receivedShare := range receivedShares { - i, receivedShare := i, receivedShare + receivedSharesByResourceID := make(map[string][]*collaboration.ReceivedShare, len(receivedShares)) + for _, receivedShare := range receivedShares { + rIDStr := storagespace.FormatResourceID(*receivedShare.GetShare().GetResourceId()) + receivedSharesByResourceID[rIDStr] = append(receivedSharesByResourceID[rIDStr], receivedShare) + } + + for _, receivedSharesForResource := range receivedSharesByResourceID { + receivedShares := receivedSharesForResource group.Go(func() error { - shareStat, err := doStat(receivedShare.GetShare().GetResourceId()) + var err error // redeclare + resourceID := receivedShares[0].GetShare().GetResourceId() + shareStat, err := doStat(receivedShares[0].GetShare().GetResourceId()) if shareStat == nil || err != nil { return err } - permission, err := g.cs3ReceivedShareToLibreGraphPermissions(ctx, receivedShare, shareStat.GetInfo()) - if err != nil { - return err + driveItem := libregraph.NewDriveItem() + + // The id of the driveItem will be the composed of the StorageID and the SpaceID of the sharestorage + // appended with the ResourceID of the shared resource + // '$!::' + driveItem.SetId(storagespace.FormatResourceID(storageprovider.ResourceId{ + StorageId: utils.ShareStorageProviderID, + OpaqueId: resourceID.GetStorageId() + ":" + resourceID.GetSpaceId() + ":" + resourceID.GetOpaqueId(), + SpaceId: utils.ShareStorageSpaceID, + })) + permissions := make([]libregraph.Permission, 0, len(receivedShares)) + + for _, receivedShare := range receivedShares { + permission, err := g.cs3ReceivedShareToLibreGraphPermissions(ctx, receivedShare) + if err != nil { + return err + } + + // If at least one of the shares was accepted, we consider the driveItem's synchronized + // flag enabled. + // Also we use the Mountpoint name of the first accepted mountpoint as the name of + // of the driveItem + if receivedShare.GetState() == collaboration.ShareState_SHARE_STATE_ACCEPTED { + driveItem.SetClientSynchronize(true) + if name := receivedShare.GetMountPoint().GetPath(); name != "" && driveItem.GetName() == "" { + driveItem.SetName(receivedShare.GetMountPoint().GetPath()) + } + } + + // if at least one share is marked as hidden, consider the whole driveItem to be hidden + if receivedShare.GetHidden() { + driveItem.SetUIHidden(true) + } + + if userID := receivedShare.GetShare().GetCreator(); userID != nil { + identity, err := g.cs3UserIdToIdentity(ctx, userID) + if err != nil { + g.logger.Warn().Err(err).Str("userid", userID.String()).Msg("could not get creator of the share") + } + + permission.SetInvitation( + libregraph.SharingInvitation{ + InvitedBy: &libregraph.IdentitySet{ + User: &identity, + }, + }, + ) + } + permissions = append(permissions, *permission) + } - shared := libregraph.NewShared() - { - if cTime := receivedShare.GetShare().GetCtime(); cTime != nil { - shared.SetSharedDateTime(cs3TimestampToTime(cTime)) + if !driveItem.HasUIHidden() { + driveItem.SetUIHidden(false) + } + if !driveItem.HasClientSynchronize() { + driveItem.SetClientSynchronize(false) + if name := shareStat.GetInfo().GetName(); name != "" { + driveItem.SetName(name) } } @@ -128,43 +196,18 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er } - driveItem := libregraph.NewDriveItem() - - // handle share state related stuff - switch receivedShare.GetState() { - case collaboration.ShareState_SHARE_STATE_ACCEPTED: - - driveItem.SetId(storagespace.FormatResourceID(storageprovider.ResourceId{ - StorageId: utils.ShareStorageProviderID, - OpaqueId: storagespace.FormatResourceID(*receivedShare.GetShare().GetResourceId()), - SpaceId: utils.ShareStorageSpaceID, - })) - - if name := receivedShare.GetMountPoint().GetPath(); name != "" { - driveItem.SetName(receivedShare.GetMountPoint().GetPath()) - } - - if etag := shareStat.GetInfo().GetEtag(); etag != "" { - driveItem.SetETag(etag) - } - - // parentReference of the out driveItem should be the drive containing the mountpoint - // i.e. the share jail - driveItem.ParentReference = libregraph.NewItemReference() - driveItem.ParentReference.SetDriveType("virtual") - driveItem.ParentReference.SetDriveId(storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID)) - driveItem.ParentReference.SetId(storagespace.FormatResourceID(storageprovider.ResourceId{ - StorageId: utils.ShareStorageProviderID, - OpaqueId: utils.ShareStorageSpaceID, - SpaceId: utils.ShareStorageSpaceID, - })) - - case collaboration.ShareState_SHARE_STATE_PENDING: - fallthrough - case collaboration.ShareState_SHARE_STATE_REJECTED: - if name := shareStat.GetInfo().GetName(); name != "" { - driveItem.SetName(name) - } + // the parentReference of the outer driveItem should be the drive + // containing the mountpoint i.e. the share jail + driveItem.ParentReference = libregraph.NewItemReference() + driveItem.ParentReference.SetDriveType("virtual") + driveItem.ParentReference.SetDriveId(storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID)) + driveItem.ParentReference.SetId(storagespace.FormatResourceID(storageprovider.ResourceId{ + StorageId: utils.ShareStorageProviderID, + OpaqueId: utils.ShareStorageSpaceID, + SpaceId: utils.ShareStorageSpaceID, + })) + if etag := shareStat.GetInfo().GetEtag(); etag != "" { + driveItem.SetETag(etag) } // connect the dots @@ -198,25 +241,6 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er remoteItem.SetCreatedBy(libregraph.IdentitySet{User: &identity}) driveItem.SetCreatedBy(libregraph.IdentitySet{User: &identity}) } - - if userID := receivedShare.GetShare().GetOwner(); userID != nil { - identity, err := g.cs3UserIdToIdentity(ctx, userID) - if err != nil { - g.logger.Warn().Err(err).Str("userid", userID.String()).Msg("could not get owner of the share") - } - shared.SetOwner(libregraph.IdentitySet{User: &identity}) - } - - if userID := receivedShare.GetShare().GetCreator(); userID != nil { - identity, err := g.cs3UserIdToIdentity(ctx, userID) - if err != nil { - g.logger.Warn().Err(err).Str("userid", userID.String()).Msg("could not get creator of the share") - } - - shared.SetSharedBy(libregraph.IdentitySet{User: &identity}) - - } - switch info := shareStat.GetInfo(); { case info.GetType() == storageprovider.ResourceType_RESOURCE_TYPE_CONTAINER: folder := libregraph.NewFolder() @@ -234,13 +258,7 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er driveItem.File = file } - if !reflect.ValueOf(*shared).IsZero() { - remoteItem.Shared = shared - } - - if !reflect.ValueOf(*permission).IsZero() { - permissions := []libregraph.Permission{*permission} - + if len(permissions) > 0 { remoteItem.Permissions = permissions } @@ -249,22 +267,27 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er } } - driveItems[i] = *driveItem + ch <- *driveItem return nil }) } // wait for concurrent requests to finish - err = group.Wait() + go func() { + err = group.Wait() + close(ch) + }() - // filter out empty drive items - return slices.Clip(slices.DeleteFunc(driveItems, func(item libregraph.DriveItem) bool { - return reflect.ValueOf(item).IsZero() - })), err + driveItems := make([]libregraph.DriveItem, 0, len(receivedSharesByResourceID)) + for di := range ch { + driveItems = append(driveItems, di) + } + + return driveItems, err } -func (g Graph) cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, receivedShare *collaboration.ReceivedShare, shareStatInfo *storageprovider.ResourceInfo) (*libregraph.Permission, error) { +func (g Graph) cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, receivedShare *collaboration.ReceivedShare) (*libregraph.Permission, error) { permission := libregraph.NewPermission() if id := receivedShare.GetShare().GetId().GetOpaqueId(); id != "" { permission.SetId(id) @@ -274,7 +297,7 @@ func (g Graph) cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, rece permission.SetExpirationDateTime(cs3TimestampToTime(expiration)) } - if permissionSet := shareStatInfo.GetPermissionSet(); permissionSet != nil { + if permissionSet := receivedShare.GetShare().GetPermissions().GetPermissions(); permissionSet != nil { role := unifiedrole.CS3ResourcePermissionsToUnifiedRole( *permissionSet, unifiedrole.UnifiedRoleConditionGrantee, @@ -321,8 +344,6 @@ func (g Graph) cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, rece }, }) } - permission.SetUiHidden(receivedShare.GetHidden()) - permission.SetClientSynchronize(receivedShare.GetState() == collaboration.ShareState_SHARE_STATE_ACCEPTED) return permission, nil } diff --git a/services/graph/pkg/service/v0/sharedwithme_test.go b/services/graph/pkg/service/v0/sharedwithme_test.go index 3426fe93f..a8f235b04 100644 --- a/services/graph/pkg/service/v0/sharedwithme_test.go +++ b/services/graph/pkg/service/v0/sharedwithme_test.go @@ -2,11 +2,13 @@ package svc_test import ( "context" + "encoding/json" "net/http" "net/http/httptest" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + groupv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" collaborationv1beta1 "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -23,6 +25,7 @@ import ( "github.com/tidwall/gjson" "google.golang.org/grpc" + libregraph "github.com/owncloud/libre-graph-api-go" "github.com/owncloud/ocis/v2/ocis-pkg/shared" "github.com/owncloud/ocis/v2/services/graph/pkg/config" "github.com/owncloud/ocis/v2/services/graph/pkg/config/defaults" @@ -130,11 +133,32 @@ var _ = Describe("SharedWithMe", func() { Return(getUserResponseShareCreator, nil) gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponseDefault, nil) + gatewayClient.On("GetGroup", mock.Anything, mock.Anything). + Return( + &groupv1beta1.GetGroupResponse{ + Status: status.NewOK(ctx), + Group: &groupv1beta1.Group{ + Id: &groupv1beta1.GroupId{ + OpaqueId: "group-id", + }, + DisplayName: "Group", + }, + }, nil) + listReceivedSharesResponse = &collaborationv1beta1.ListReceivedSharesResponse{ Status: status.NewOK(ctx), Shares: []*collaborationv1beta1.ReceivedShare{ { - Share: &collaborationv1beta1.Share{ResourceId: toResourceID("1$2!3")}, + Share: &collaborationv1beta1.Share{ + ResourceId: toResourceID("1$2!3"), + Id: &collaborationv1beta1.ShareId{ + OpaqueId: "sh:are:id", + }, + Permissions: &collaborationv1beta1.SharePermissions{ + Permissions: roleconversions.NewViewerRole(true).CS3ResourcePermissions(), + }, + Creator: getUserResponseShareCreator.User.Id, + }, MountPoint: &providerv1beta1.Reference{ ResourceId: &providerv1beta1.ResourceId{ StorageId: utils.ShareStorageProviderID, @@ -209,9 +233,6 @@ var _ = Describe("SharedWithMe", func() { share.Id = &collaborationv1beta1.ShareId{OpaqueId: "1:2:3"} share.Ctime = &typesv1beta1.Timestamp{Seconds: 4001} share.Mtime = &typesv1beta1.Timestamp{Seconds: 4002} - share.Creator = &userv1beta1.UserId{ - OpaqueId: "share-creator-id", - } resourceInfo := statResponse.Info resourceInfo.Name = "some folder" @@ -337,72 +358,24 @@ var _ = Describe("SharedWithMe", func() { httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil), ) - jsonData := gjson.Get(tape.Body.String(), "value.0.remoteItem.permissions.0") + driveitemJSON := gjson.Get(tape.Body.String(), "value.0") + Expect(driveitemJSON.Get("@UI\\.Hidden").Exists()).To(BeTrue()) + Expect(driveitemJSON.Get("@UI\\.Hidden").Bool()).To(BeFalse()) + Expect(driveitemJSON.Get("@client\\.synchronize").Exists()).To(BeTrue()) + Expect(driveitemJSON.Get("@client\\.synchronize").Bool()).To(BeTrue()) - Expect(jsonData.Get("roles.0").String()).To(Equal(unifiedrole.UnifiedRoleViewerID)) - Expect(jsonData.Get("@ui\\.hidden").Exists()).To(BeTrue()) - Expect(jsonData.Get("@ui\\.hidden").Bool()).To(BeFalse()) - Expect(jsonData.Get("@client\\.synchronize").Exists()).To(BeTrue()) - Expect(jsonData.Get("@client\\.synchronize").Bool()).To(BeTrue()) - }) - - It("populates the driveItem.remoteItem.shared properties", func() { - share := listReceivedSharesResponse.Shares[0].Share - share.Ctime = &typesv1beta1.Timestamp{Seconds: 4000} - - svc.ListSharedWithMe( - tape, - httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil), - ) - - jsonData := gjson.Get(tape.Body.String(), "value.0.remoteItem.shared") - - Expect(jsonData.Get("sharedDateTime").String()).To(Equal(utils.TSToTime(share.Ctime).Format(time.RFC3339Nano))) - }) - - It("populates the driveItem.remoteItem.shared.owner properties", func() { - shareOwner := getUserResponseDefault.User - - share := listReceivedSharesResponse.Shares[0].Share - share.Owner = shareOwner.Id - - svc.ListSharedWithMe( - tape, - httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil), - ) - - jsonData := gjson.Get(tape.Body.String(), "value.0.remoteItem.shared.owner") - - Expect(jsonData.Get("user.displayName").String()).To(Equal(shareOwner.DisplayName)) - Expect(jsonData.Get("user.id").String()).To(Equal(shareOwner.Id.OpaqueId)) - }) - - It("populates the driveItem.remoteItem.shared.sharedBy properties", func() { - shareCreator := getUserResponseDefault.User - - share := listReceivedSharesResponse.Shares[0].Share - share.Creator = shareCreator.Id - - svc.ListSharedWithMe( - tape, - httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil), - ) - - jsonData := gjson.Get(tape.Body.String(), "value.0.remoteItem.shared.sharedBy") - - Expect(jsonData.Get("user.displayName").String()).To(Equal(shareCreator.DisplayName)) - Expect(jsonData.Get("user.id").String()).To(Equal(shareCreator.Id.OpaqueId)) + permissionsJSON := driveitemJSON.Get("remoteItem.permissions.0") + Expect(permissionsJSON.Get("id").String()).To(Equal(listReceivedSharesResponse.Shares[0].Share.Id.OpaqueId)) + Expect(permissionsJSON.Get("roles.0").String()).To(Equal(unifiedrole.UnifiedRoleViewerID)) + Expect(permissionsJSON.Get("invitation.invitedBy.user.id").String()).To(Equal(getUserResponseShareCreator.User.Id.OpaqueId)) }) It("returns shares created on project space", func() { - shareCreator := getUserResponseDefault.User - ownerID := &userv1beta1.UserId{ OpaqueId: "project-space-id", Type: userv1beta1.UserType_USER_TYPE_SPACE_OWNER, } share := listReceivedSharesResponse.Shares[0].Share - share.Creator = shareCreator.Id share.Owner = ownerID resourceInfo := statResponse.Info resourceInfo.Owner = ownerID @@ -417,11 +390,43 @@ var _ = Describe("SharedWithMe", func() { Expect(jsonData.Get("user.displayName").String()).To(Equal("")) Expect(jsonData.Get("user.id").String()).To(Equal(ownerID.OpaqueId)) - jsonData = gjson.Get(tape.Body.String(), "value.0.remoteItem.shared") - Expect(jsonData.Get("sharedBy.user.displayName").String()).To(Equal(shareCreator.DisplayName)) - Expect(jsonData.Get("sharedBy.user.id").String()).To(Equal(shareCreator.Id.OpaqueId)) - Expect(jsonData.Get("owner.user.displayName").String()).To(Equal("")) - Expect(jsonData.Get("owner.user.id").String()).To(Equal(ownerID.OpaqueId)) + jsonData = gjson.Get(tape.Body.String(), "value.0.remoteItem.permissions.0.invitation.invitedBy.user") + Expect(jsonData.Get("displayName").String()).To(Equal(getUserResponseShareCreator.User.DisplayName)) + Expect(jsonData.Get("id").String()).To(Equal(getUserResponseShareCreator.User.Id.OpaqueId)) + }) + + It("returns a single drive item when multiple shares exist for the same resource", func() { + anotherShare := &collaborationv1beta1.ReceivedShare{ + Share: &collaborationv1beta1.Share{ + ResourceId: toResourceID("1$2!3"), + Id: &collaborationv1beta1.ShareId{ + OpaqueId: "sh:are:id2", + }, + Permissions: &collaborationv1beta1.SharePermissions{ + Permissions: roleconversions.NewViewerRole(true).CS3ResourcePermissions(), + }, + Creator: getUserResponseShareCreator.User.Id, + Grantee: &providerv1beta1.Grantee{ + Type: providerv1beta1.GranteeType_GRANTEE_TYPE_GROUP, + Id: &providerv1beta1.Grantee_GroupId{ + GroupId: &groupv1beta1.GroupId{ + OpaqueId: "group-id", + }, + }, + }, + }, + } + listReceivedSharesResponse.Shares = append(listReceivedSharesResponse.Shares, anotherShare) + svc.ListSharedWithMe( + tape, + httptest.NewRequest(http.MethodGet, "/graph/v1beta1/me/drive/sharedWithMe", nil), + ) + driveItems := libregraph.CollectionOfDriveItems{} + err := json.Unmarshal(tape.Body.Bytes(), &driveItems) + Expect(err).To(BeNil()) + Expect(len(driveItems.Value)).To(Equal(1)) + ri := driveItems.GetValue()[0].GetRemoteItem() + Expect(len(ri.GetPermissions())).To(Equal(2)) }) }) })