From 352d60a931cb793c5be92dc52891f0a589d639d6 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 25 Mar 2024 11:21:23 +0100 Subject: [PATCH] feat(sharing-ng): Support for updating space permissions PATCH request on a space's root drive item should work now --- services/graph/pkg/service/v0/driveitems.go | 101 ++++++++++---- .../graph/pkg/service/v0/driveitems_test.go | 130 ++++++++++++++---- services/graph/pkg/service/v0/sharedbyme.go | 20 ++- 3 files changed, 190 insertions(+), 61 deletions(-) diff --git a/services/graph/pkg/service/v0/driveitems.go b/services/graph/pkg/service/v0/driveitems.go index e124266ba..2cc0690b6 100644 --- a/services/graph/pkg/service/v0/driveitems.go +++ b/services/graph/pkg/service/v0/driveitems.go @@ -619,20 +619,20 @@ func (g Graph) UpdatePermission(w http.ResponseWriter, r *http.Request) { } permission := &libregraph.Permission{} - if err := StrictJSONUnmarshal(r.Body, permission); err != nil { + if err = StrictJSONUnmarshal(r.Body, permission); err != nil { g.logger.Debug().Err(err).Interface("Body", r.Body).Msg("failed unmarshalling request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "invalid request body") return } ctx := r.Context() - if err := validate.StructCtx(ctx, permission); err != nil { + if err = validate.StructCtx(ctx, permission); err != nil { g.logger.Debug().Err(err).Interface("Body", r.Body).Msg("invalid request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) return } - oldPermission, sharedResourceID, err := g.getPermissionByID(ctx, permissionID) + oldPermission, sharedResourceID, err := g.getPermissionByID(ctx, permissionID, &itemID) if err != nil { errorcode.RenderError(w, r, err) return @@ -659,7 +659,7 @@ func (g Graph) UpdatePermission(w http.ResponseWriter, r *http.Request) { } // This is a user share - updatedPermission, err := g.updateUserShare(ctx, permissionID, permission) + updatedPermission, err := g.updateUserShare(ctx, permissionID, sharedResourceID, permission) if err != nil { errorcode.RenderError(w, r, err) return @@ -738,23 +738,9 @@ func (g Graph) DeletePermission(w http.ResponseWriter, r *http.Request) { } -func (g Graph) getPermissionByID(ctx context.Context, permissionID string) (*libregraph.Permission, *storageprovider.ResourceId, error) { - share, err := g.getCS3UserShareByID(ctx, permissionID) +func (g Graph) getPermissionByID(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId) (*libregraph.Permission, *storageprovider.ResourceId, error) { + publicShare, err := g.getCS3PublicShareByID(ctx, permissionID) if err == nil { - permission, err := g.cs3UserShareToPermission(ctx, share) - if err != nil { - return nil, nil, err - } - return permission, share.GetResourceId(), nil - } - - var errcode errorcode.Error - if errors.As(err, &errcode) && errcode.GetCode() == errorcode.ItemNotFound { - // there is no user share with that id, check if this is a public link - publicShare, err := g.getCS3PublicShareByID(ctx, permissionID) - if err != nil { - return nil, nil, err - } permission, err := g.libreGraphPermissionFromCS3PublicShare(publicShare) if err != nil { return nil, nil, err @@ -762,6 +748,45 @@ func (g Graph) getPermissionByID(ctx context.Context, permissionID string) (*lib return permission, publicShare.GetResourceId(), nil } + // The id is not referencing a public share, if the itemID is referencing + // a spaceroot this is a space permission. Handle that next + if IsSpaceRoot(itemID) { + gatewayClient, err := g.gatewaySelector.Next() + if err != nil { + g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + return nil, nil, err + } + // get space id + resourceInfo, err := utils.GetResourceByID(ctx, itemID, gatewayClient) + if err != nil { + return nil, nil, err + } + + perms, err := g.getSpaceRootPermissions(ctx, resourceInfo.GetSpace().GetId()) + if err != nil { + return nil, nil, err + } + for _, p := range perms { + if p.GetId() == permissionID { + return &p, itemID, nil + } + } + } + + var errcode errorcode.Error + if errors.As(err, &errcode) && errcode.GetCode() == errorcode.ItemNotFound { + // there is no public link with that id, check if this is a user share + share, err := g.getCS3UserShareByID(ctx, permissionID) + if err != nil { + return nil, nil, err + } + permission, err := g.cs3UserShareToPermission(ctx, share, false) + if err != nil { + return nil, nil, err + } + return permission, share.GetResourceId(), nil + } + return nil, nil, err } @@ -811,19 +836,37 @@ func (g Graph) getCS3UserShareByID(ctx context.Context, permissionID string) (*c return getShareResp.GetShare(), nil } -func (g Graph) updateUserShare(ctx context.Context, permissionID string, newPermission *libregraph.Permission) (*libregraph.Permission, error) { +func (g Graph) updateUserShare(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId, newPermission *libregraph.Permission) (*libregraph.Permission, error) { gatewayClient, err := g.gatewaySelector.Next() if err != nil { g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") return nil, err } - cs3UpdateShareReq := &collaboration.UpdateShareRequest{ - Share: &collaboration.Share{ + var cs3UpdateShareReq collaboration.UpdateShareRequest + // When updating a space root we need to reference the share by resourceId and grantee + if IsSpaceRoot(itemID) { + grantee, err := spacePermissionIdToCS3Grantee(permissionID) + if err != nil { + g.logger.Debug().Err(err).Str("permissionid", permissionID).Msg("failed to parse space permission id") + return nil, err + } + cs3UpdateShareReq.Share = &collaboration.Share{ + ResourceId: itemID, + Grantee: &grantee, + } + cs3UpdateShareReq.Opaque = &types.Opaque{ + Map: map[string]*types.OpaqueEntry{ + "spacegrant": {}, + }, + } + cs3UpdateShareReq.Opaque = utils.AppendPlainToOpaque(cs3UpdateShareReq.Opaque, "spacetype", _spaceTypeProject) + } else { + cs3UpdateShareReq.Share = &collaboration.Share{ Id: &collaboration.ShareId{ OpaqueId: permissionID, }, - }, + } } fieldmask := []string{} if expiration, ok := newPermission.GetExpirationDateTimeOk(); ok { @@ -842,8 +885,12 @@ func (g Graph) updateUserShare(ctx context.Context, permissionID string, newPerm return nil, err } + condition := unifiedrole.UnifiedRoleConditionGrantee + if IsSpaceRoot(itemID) { + condition = unifiedrole.UnifiedRoleConditionOwner + } // FIXME: When setting permissions on a space, we need to use UnifiedRoleConditionOwner here - allowedResourceActions = unifiedrole.GetAllowedResourceActions(role, unifiedrole.UnifiedRoleConditionGrantee) + allowedResourceActions = unifiedrole.GetAllowedResourceActions(role, condition) if len(allowedResourceActions) == 0 { return nil, errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") } @@ -872,12 +919,12 @@ func (g Graph) updateUserShare(ctx context.Context, permissionID string, newPerm Paths: fieldmask, } - updateUserShareResp, err := gatewayClient.UpdateShare(ctx, cs3UpdateShareReq) + updateUserShareResp, err := gatewayClient.UpdateShare(ctx, &cs3UpdateShareReq) if errCode := errorcode.FromCS3Status(updateUserShareResp.GetStatus(), err); errCode != nil { return nil, *errCode } - permission, err := g.cs3UserShareToPermission(ctx, updateUserShareResp.GetShare()) + permission, err := g.cs3UserShareToPermission(ctx, updateUserShareResp.GetShare(), IsSpaceRoot(itemID)) if err != nil { return nil, err } diff --git a/services/graph/pkg/service/v0/driveitems_test.go b/services/graph/pkg/service/v0/driveitems_test.go index 0f2fe3e37..006f50318 100644 --- a/services/graph/pkg/service/v0/driveitems_test.go +++ b/services/graph/pkg/service/v0/driveitems_test.go @@ -12,7 +12,6 @@ import ( gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" grouppb "github.com/cs3org/go-cs3apis/cs3/identity/group/v1beta1" - user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" @@ -64,6 +63,7 @@ var _ = Describe("Driveitems", func() { identityBackend *identitymocks.Backend getPublicShareResponse *link.GetPublicShareResponse getShareResponse *collaboration.GetShareResponse + listSpacesResponse *provider.ListStorageSpacesResponse rr *httptest.ResponseRecorder @@ -95,6 +95,31 @@ var _ = Describe("Driveitems", func() { Status: status.NewNotFound(ctx, "not found"), } + grantMapJSON, _ := json.Marshal( + map[string]*provider.ResourcePermissions{ + "userid": roleconversions.NewSpaceViewerRole().CS3ResourcePermissions(), + }, + ) + spaceOpaque := &types.Opaque{ + Map: map[string]*types.OpaqueEntry{ + "grants": { + Decoder: "json", + Value: grantMapJSON, + }, + }, + } + listSpacesResponse = &provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{ + { + Id: &provider.StorageSpaceId{ + OpaqueId: "2", + }, + Opaque: spaceOpaque, + }, + }, + } + identityBackend = &identitymocks.Backend{} newGroup = libregraph.NewGroup() newGroup.SetMembersodataBind([]string{"/users/user1"}) @@ -277,7 +302,7 @@ var _ = Describe("Driveitems", func() { driveItemPermission *libregraph.Permission getShareMockResponse *collaboration.GetShareResponse getPublicShareMockResponse *link.GetPublicShareResponse - getUserMockResponse *user.GetUserResponse + getUserMockResponse *userpb.GetUserResponse updateShareMockResponse *collaboration.UpdateShareResponse updatePublicShareMockResponse *link.UpdatePublicShareResponse ) @@ -323,7 +348,7 @@ var _ = Describe("Driveitems", func() { Grantee: &provider.Grantee{ Type: provider.GranteeType_GRANTEE_TYPE_USER, Id: &provider.Grantee_UserId{ - UserId: &user.UserId{ + UserId: &userpb.UserId{ OpaqueId: "userid", }, }, @@ -397,8 +422,33 @@ var _ = Describe("Driveitems", func() { Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, }, } - statMock.Return(statResponse, nil) + spaceRootStatMock := gatewayClient.On("Stat", + mock.Anything, + mock.MatchedBy(func(req *provider.StatRequest) bool { + return utils.ResourceIDEqual( + req.GetRef().GetResourceId(), + &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "2", + }, + ) + })) + + spaceRootStatMock.Return( + &provider.StatResponse{ + Status: status.NewOK(ctx), + Info: &provider.ResourceInfo{ + Id: &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "2", + }, + Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, + }, + }, nil) + }) It("fails when no share is found", func() { getShareMockResponse.Share = nil @@ -615,6 +665,8 @@ var _ = Describe("Driveitems", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) It("updates the expiration date", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") expiration := time.Now().Add(time.Hour) updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, @@ -647,6 +699,8 @@ var _ = Describe("Driveitems", func() { Expect(res.GetExpirationDateTime().Equal(expiration)).To(BeTrue()) }) It("deletes the expiration date", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { @@ -678,6 +732,8 @@ var _ = Describe("Driveitems", func() { Expect(ok).To(BeFalse()) }) It("updates the share permissions with changing the role", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { @@ -706,6 +762,8 @@ var _ = Describe("Driveitems", func() { Expect(ok).To(BeTrue()) }) It("fails to update the share permissions for a file share when setting a space specific role", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { @@ -724,7 +782,44 @@ var _ = Describe("Driveitems", func() { ) Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) + It("fails to update the space permissions for a space share when setting a file specific role", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + gatewayClient.On("GetPublicShare", + mock.Anything, + mock.Anything, + ).Return(getPublicShareMockResponse, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) + + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") + updateShareMock := gatewayClient.On("UpdateShare", + mock.Anything, + mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { + return req.GetShare().GetId().GetOpaqueId() == "permissionid" + }), + ) + updateShareMock.Return(updateShareMockResponse, nil) + + driveItemPermission.SetRoles([]string{unifiedrole.NewFileEditorUnifiedRole(false).GetId()}) + body, err := driveItemPermission.MarshalJSON() + Expect(err).To(BeNil()) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("driveID", "1$2") + // This is a space root + rctx.URLParams.Add("itemID", "1$2!2") + rctx.URLParams.Add("permissionID", "u:userid") + ctx = context.WithValue(context.Background(), chi.RouteCtxKey, rctx) + svc.UpdatePermission( + rr, + httptest.NewRequest(http.MethodPatch, "/", strings.NewReader(string(body))). + WithContext(ctx), + ) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + }) It("updates the share permissions when changing the resource permission actions", func() { + getPublicShareMockResponse.Share = nil + getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { @@ -1324,32 +1419,7 @@ var _ = Describe("Driveitems", func() { rctx.URLParams.Add("driveID", "1$2") rctx.URLParams.Add("itemID", "1$2!2") statResponse.Info.Id.OpaqueId = "2" - grantMap := map[string]*provider.ResourcePermissions{ - "userid": roleconversions.NewSpaceViewerRole().CS3ResourcePermissions(), - } - grantMapJSON, _ := json.Marshal(grantMap) - spaceOpaque := &types.Opaque{ - Map: map[string]*types.OpaqueEntry{ - "grants": { - Decoder: "json", - Value: grantMapJSON, - }, - }, - } - - listSpacesMock := gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything) - listSpacesResponse := &provider.ListStorageSpacesResponse{ - Status: status.NewOK(ctx), - StorageSpaces: []*provider.StorageSpace{ - { - Id: &provider.StorageSpaceId{ - OpaqueId: "2", - }, - Opaque: spaceOpaque, - }, - }, - } - listSpacesMock.Return(listSpacesResponse, nil) + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) getUserMock := gatewayClient.On("GetUser", mock.Anything, mock.Anything) getUserMockResponse := &userpb.GetUserResponse{ diff --git a/services/graph/pkg/service/v0/sharedbyme.go b/services/graph/pkg/service/v0/sharedbyme.go index 340a0b0cf..c21dff898 100644 --- a/services/graph/pkg/service/v0/sharedbyme.go +++ b/services/graph/pkg/service/v0/sharedbyme.go @@ -124,7 +124,7 @@ func (g Graph) cs3UserSharesToDriveItems(ctx context.Context, shares []*collabor } item = *itemptr } - perm, err := g.cs3UserShareToPermission(ctx, s) + perm, err := g.cs3UserShareToPermission(ctx, s, false) var errcode errorcode.Error switch { @@ -140,10 +140,12 @@ func (g Graph) cs3UserSharesToDriveItems(ctx context.Context, shares []*collabor return driveItems, nil } -func (g Graph) cs3UserShareToPermission(ctx context.Context, share *collaboration.Share) (*libregraph.Permission, error) { +func (g Graph) cs3UserShareToPermission(ctx context.Context, share *collaboration.Share, isSpacePermission bool) (*libregraph.Permission, error) { perm := libregraph.Permission{} perm.SetRoles([]string{}) - perm.SetId(share.Id.OpaqueId) + if !isSpacePermission { + perm.SetId(share.GetId().GetOpaqueId()) + } grantedTo := libregraph.SharePointIdentitySet{} switch share.GetGrantee().GetType() { case storageprovider.GranteeType_GRANTEE_TYPE_USER: @@ -157,6 +159,9 @@ func (g Graph) cs3UserShareToPermission(ctx context.Context, share *collaboratio return nil, errorcode.New(errorcode.GeneralException, err.Error()) default: grantedTo.SetUser(user) + if isSpacePermission { + perm.SetId("u:" + user.GetId()) + } } case storageprovider.GranteeType_GRANTEE_TYPE_GROUP: group, err := groupIdToIdentity(ctx, g.identityCache, share.Grantee.GetGroupId().GetOpaqueId()) @@ -169,6 +174,9 @@ func (g Graph) cs3UserShareToPermission(ctx context.Context, share *collaboratio return nil, errorcode.New(errorcode.GeneralException, err.Error()) default: grantedTo.SetGroup(group) + if isSpacePermission { + perm.SetId("g:" + group.GetId()) + } } } @@ -176,9 +184,13 @@ func (g Graph) cs3UserShareToPermission(ctx context.Context, share *collaboratio if share.GetExpiration() != nil { perm.SetExpirationDateTime(cs3TimestampToTime(share.GetExpiration())) } + condition := unifiedrole.UnifiedRoleConditionGrantee + if isSpacePermission { + condition = unifiedrole.UnifiedRoleConditionOwner + } role := unifiedrole.CS3ResourcePermissionsToUnifiedRole( *share.GetPermissions().GetPermissions(), - unifiedrole.UnifiedRoleConditionGrantee, + condition, g.config.FilesSharing.EnableResharing, ) if role != nil {