Skip to content

Commit

Permalink
fix: ocs handles the mount_point on its own
Browse files Browse the repository at this point in the history
  • Loading branch information
@fschade @rhafer
fschade authored and rhafer committed Jun 18, 2024
1 parent 46dd321 commit ed0273c
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 123 deletions.
1 change: 1 addition & 0 deletions changelog/unreleased/enhance-unique-share-mountpoint-name.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Enhancement: Unique share mountpoint name

Accepting a received share with a mountpoint name that already exists will now append a unique suffix to the mountpoint name.

https://github.com/cs3org/reva/pull/4725
https://github.com/cs3org/reva/pull/4723
https://github.com/cs3org/reva/pull/4714
https://github.com/owncloud/ocis/issues/8961
136 changes: 71 additions & 65 deletions internal/grpc/services/usershareprovider/usershareprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,22 +547,81 @@ func (s *service) UpdateReceivedShare(ctx context.Context, req *collaboration.Up
}, nil
}

// GetAvailableMountpoint returns a new or existing mountpoint
func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId, name string, userId *userpb.UserId) (string, error) {
func setReceivedShareMountPoint(ctx context.Context, gwc gateway.GatewayAPIClient, req *collaboration.UpdateReceivedShareRequest) (*rpc.Status, error) {
receivedShare, err := gwc.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{
Ref: &collaboration.ShareReference{
Spec: &collaboration.ShareReference_Id{
Id: req.GetShare().GetShare().GetId(),
},
},
})
switch {
case err != nil:
fallthrough
case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK:
return receivedShare.GetStatus(), err
}

if receivedShare.GetShare().GetMountPoint().GetPath() != "" {
return status.NewOK(ctx), nil
}

resourceStat, err := gwc.Stat(ctx, &provider.StatRequest{
Ref: &provider.Reference{
ResourceId: receivedShare.GetShare().GetShare().GetResourceId(),
},
})
switch {
case err != nil:
fallthrough
case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK:
return resourceStat.GetStatus(), err
}

// handle mount point related updates
{
var userID *userpb.UserId
_ = utils.ReadJSONFromOpaque(req.Opaque, "userid", &userID)

// check if the requested mount point is available and if not, find a suitable one
availableMountpoint, _, err := GetMountpointAndUnmountedShares(ctx, gwc,
resourceStat.GetInfo().GetId(),
resourceStat.GetInfo().GetName(),
userID,
)
if err != nil {
return status.NewInternal(ctx, err.Error()), nil
}

if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) {
req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint)
}

req.GetShare().MountPoint = &provider.Reference{
Path: availableMountpoint,
}
}

return status.NewOK(ctx), nil
}

// GetMountpointAndUnmountedShares returns a new or existing mountpoint for the given info and produces a list of unmounted received shares for the same resource
func GetMountpointAndUnmountedShares(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId, name string, userId *userpb.UserId) (string, []*collaboration.ReceivedShare, error) {
listReceivedSharesReq := &collaboration.ListReceivedSharesRequest{}
if userId != nil {
listReceivedSharesReq.Opaque = utils.AppendJSONToOpaque(nil, "userid", userId)
}

listReceivedSharesRes, err := gwc.ListReceivedShares(ctx, listReceivedSharesReq)
if err != nil {
return "", errtypes.InternalError("grpc list received shares request failed")
return "", nil, errtypes.InternalError("grpc list received shares request failed")
}

if err := errtypes.NewErrtypeFromStatus(listReceivedSharesRes.GetStatus()); err != nil {
return "", err
return "", nil, err
}

unmountedShares := []*collaboration.ReceivedShare{}
base := filepath.Clean(name)
mount := base
existingMountpoint := ""
Expand All @@ -580,6 +639,11 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i
}
}

if resourceIDEqual && s.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
// a share to the resource already exists but is not mounted, collect the unmounted share
unmountedShares = append(unmountedShares, s)
}

if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
// collect all accepted mount points
mountedShares = append(mountedShares, s.GetMountPoint().GetPath())
Expand All @@ -596,7 +660,7 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i

if existingMountpoint != "" {
// we want to reuse the same mountpoint for all unmounted shares to the same resource
return existingMountpoint, nil
return existingMountpoint, unmountedShares, nil
}

// If the mount point really already exists, we need to insert a number into the filename
Expand All @@ -608,68 +672,10 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i

mount = name + " (" + strconv.Itoa(i) + ")" + ext
if !slices.Contains(mountedShares, mount) {
return mount, nil
return mount, unmountedShares, nil
}
}
}

return mount, nil
}

func setReceivedShareMountPoint(ctx context.Context, gwc gateway.GatewayAPIClient, req *collaboration.UpdateReceivedShareRequest) (*rpc.Status, error) {
receivedShare, err := gwc.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{
Ref: &collaboration.ShareReference{
Spec: &collaboration.ShareReference_Id{
Id: req.GetShare().GetShare().GetId(),
},
},
})
switch {
case err != nil:
fallthrough
case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK:
return receivedShare.GetStatus(), err
}

if receivedShare.GetShare().GetMountPoint().GetPath() != "" {
return status.NewOK(ctx), nil
}

resourceStat, err := gwc.Stat(ctx, &provider.StatRequest{
Ref: &provider.Reference{
ResourceId: receivedShare.GetShare().GetShare().GetResourceId(),
},
})
switch {
case err != nil:
fallthrough
case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK:
return resourceStat.GetStatus(), err
}

// handle mount point related updates
{
var userID *userpb.UserId
_ = utils.ReadJSONFromOpaque(req.Opaque, "userid", &userID)

// check if the requested mount point is available and if not, find a suitable one
availableMountpoint, err := GetAvailableMountpoint(ctx, gwc,
resourceStat.GetInfo().GetId(),
resourceStat.GetInfo().GetName(),
userID,
)
if err != nil {
return status.NewInternal(ctx, err.Error()), nil
}

if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) {
req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint)
}

req.GetShare().MountPoint = &provider.Reference{
Path: availableMountpoint,
}
}

return status.NewOK(ctx), nil
return mount, unmountedShares, nil
}
16 changes: 8 additions & 8 deletions internal/grpc/services/usershareprovider/usershareprovider_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,15 +582,15 @@ var _ = Describe("user share provider service", func() {
})

var _ = Describe("helpers", func() {
type GetAvailableMountpointArgs struct {
type GetMountpointAndUnmountedSharesArgs struct {
withName string
withResourceId *providerpb.ResourceId
listReceivedSharesResponse *collaborationpb.ListReceivedSharesResponse
listReceivedSharesError error
expectedName string
}
DescribeTable("GetAvailableMountpoint",
func(args GetAvailableMountpointArgs) {
DescribeTable("GetMountpointAndUnmountedShares",
func(args GetMountpointAndUnmountedSharesArgs) {
gatewayClient := cs3mocks.NewGatewayAPIClient(GinkgoT())

gatewayClient.EXPECT().
Expand Down Expand Up @@ -627,7 +627,7 @@ var _ = Describe("helpers", func() {
}).Times(statCallCount)
}

availableMountpoint, err := usershareprovider.GetAvailableMountpoint(context.Background(), gatewayClient, args.withResourceId, args.withName, nil)
availableMountpoint, _, err := usershareprovider.GetMountpointAndUnmountedShares(context.Background(), gatewayClient, args.withResourceId, args.withName, nil)

if args.listReceivedSharesError != nil {
Expect(err).To(HaveOccurred(), "expected error, got none")
Expand All @@ -640,13 +640,13 @@ var _ = Describe("helpers", func() {
},
Entry(
"listing received shares errors",
GetAvailableMountpointArgs{
GetMountpointAndUnmountedSharesArgs{
listReceivedSharesError: errors.New("some error"),
},
),
Entry(
"returns the given name if no shares are found",
GetAvailableMountpointArgs{
GetMountpointAndUnmountedSharesArgs{
withName: "name1",
listReceivedSharesResponse: &collaborationpb.ListReceivedSharesResponse{
Status: status.NewOK(context.Background()),
Expand All @@ -656,7 +656,7 @@ var _ = Describe("helpers", func() {
),
Entry(
"returns the path as name if a share with the same resourceId is found",
GetAvailableMountpointArgs{
GetMountpointAndUnmountedSharesArgs{
withName: "name",
withResourceId: &providerpb.ResourceId{
StorageId: "1",
Expand Down Expand Up @@ -686,7 +686,7 @@ var _ = Describe("helpers", func() {
),
Entry(
"enumerates the name if a share with the same path already exists",
GetAvailableMountpointArgs{
GetMountpointAndUnmountedSharesArgs{
withName: "some name",
listReceivedSharesResponse: &collaborationpb.ListReceivedSharesResponse{
Status: status.NewOK(context.Background()),
Expand Down
3 changes: 0 additions & 3 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/export_test.go
View file

This file was deleted.

40 changes: 17 additions & 23 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ import (
"github.com/pkg/errors"
"google.golang.org/protobuf/types/known/fieldmaskpb"

"github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocs/response"
"github.com/cs3org/reva/v2/pkg/appctx"
"github.com/cs3org/reva/v2/pkg/conversions"
"github.com/cs3org/reva/v2/pkg/errtypes"
"github.com/cs3org/reva/v2/pkg/utils"
)

const (
Expand Down Expand Up @@ -73,17 +73,26 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
response.WriteOCSResponse(w, r, *ocsResponse, nil)
return
}

unmountedShares, err := getUnmountedShares(ctx, client, sharedResource.GetInfo().GetId())
mount, unmountedShares, err := usershareprovider.GetMountpointAndUnmountedShares(
ctx,
client,
sharedResource.GetInfo().GetId(),
sharedResource.GetInfo().GetName(),
nil,
)
if err != nil {
response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "could not determine mountpoint", err)
return
}

// first update the requested share
receivedShare.State = collaboration.ShareState_SHARE_STATE_ACCEPTED
// we need to add a path to the share
receivedShare.MountPoint = &provider.Reference{
Path: mount,
}

updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state"}}
updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state", "mount_point"}}
data, meta, err := h.updateReceivedShare(r.Context(), receivedShare, updateMask)
if err != nil {
// we log an error for affected shares, for the actual share we return an error
Expand All @@ -100,6 +109,10 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
}

rs.State = collaboration.ShareState_SHARE_STATE_ACCEPTED
// set the same mountpoint as for the requested received share
rs.MountPoint = &provider.Reference{
Path: mount,
}

_, _, err := h.updateReceivedShare(r.Context(), rs, updateMask)
if err != nil {
Expand Down Expand Up @@ -314,25 +327,6 @@ func getReceivedShareFromID(ctx context.Context, client gateway.GatewayAPIClient
return s.Share, nil
}

func getUnmountedShares(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId) ([]*collaboration.ReceivedShare, error) {
var unmountedShares []*collaboration.ReceivedShare
receivedShares, err := listReceivedShares(ctx, gwc)
if err != nil {
return unmountedShares, err
}

for _, s := range receivedShares {
resourceIDEqual := utils.ResourceIDEqual(s.GetShare().GetResourceId(), id)

if resourceIDEqual && s.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
// a share to the resource already exists but is not mounted, collect the unmounted share
unmountedShares = append(unmountedShares, s)
}
}

return unmountedShares, err
}

// getSharedResource attempts to get a shared resource from the storage from the resource reference.
func getSharedResource(ctx context.Context, client gateway.GatewayAPIClient, resID *provider.ResourceId) (*provider.StatResponse, *response.Response) {
res, err := client.Stat(ctx, &provider.StatRequest{
Expand Down
27 changes: 3 additions & 24 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (

gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
"github.com/go-chi/chi/v5"
Expand Down Expand Up @@ -259,7 +258,7 @@ var _ = Describe("The ocs API", func() {

It("accepts both pending shares", func() {
client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool {
return req.Share.Share.Id.OpaqueId == "1"
return req.Share.Share.Id.OpaqueId == "1" && req.Share.MountPoint.Path == "share1"
})).Return(&collaboration.UpdateReceivedShareResponse{
Status: status.NewOK(context.Background()),
Share: &collaboration.ReceivedShare{
Expand All @@ -270,7 +269,7 @@ var _ = Describe("The ocs API", func() {
}, nil)

client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool {
return req.Share.Share.Id.OpaqueId == "2"
return req.Share.Share.Id.OpaqueId == "2" && req.Share.MountPoint.Path == "share1"
})).Return(&collaboration.UpdateReceivedShareResponse{
Status: status.NewOK(context.Background()),
Share: &collaboration.ReceivedShare{
Expand Down Expand Up @@ -320,7 +319,7 @@ var _ = Describe("The ocs API", func() {

It("accepts the remaining pending share", func() {
client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool {
return req.Share.Share.Id.OpaqueId == "2"
return req.Share.Share.Id.OpaqueId == "2" && req.Share.MountPoint.Path == "existing/mountpoint"
})).Return(&collaboration.UpdateReceivedShareResponse{
Status: status.NewOK(context.Background()),
Share: &collaboration.ReceivedShare{
Expand Down Expand Up @@ -417,26 +416,6 @@ var _ = Describe("The ocs API", func() {
},
}, nil)
})

DescribeTable("Resolve unmounted shares",
func(info *provider.ResourceInfo, unmountedLen int) {
// GetMountpointAndUnmountedShares check the error Stat response only
client.On("Stat", mock.Anything, mock.Anything).
Return(&provider.StatResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK},
Info: &provider.ResourceInfo{}}, nil)
unmounted, err := shares.GetUnmountedShares(ctx, client, info.GetId())
Expect(len(unmounted)).To(Equal(unmountedLen))
Expect(err).To(BeNil())
},
Entry("new mountpoint, name changed", &provider.ResourceInfo{
Name: "b.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId},
}, 0),
Entry("unmountpoint", &provider.ResourceInfo{
Name: "b.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d", SpaceId: userOneSpaceId},
}, 1),
)
})
})
})

0 comments on commit ed0273c

Please sign in to comment.