diff --git a/changelog/unreleased/fix-mount-points-naming.md b/changelog/unreleased/fix-mount-points-naming.md new file mode 100644 index 00000000000..a3b2699b757 --- /dev/null +++ b/changelog/unreleased/fix-mount-points-naming.md @@ -0,0 +1,6 @@ +Bugfix: Fix the mount points naming + +We fixed a bug that caused inconsistent naming when multiple users share the resource with same name to another user. + +https://github.com/cs3org/reva/pull/4546 +https://github.com/owncloud/ocis/issues/8471 diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go index ca6c28df5d9..5ab1c1782c8 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go @@ -24,7 +24,7 @@ import ( "net/http" "path" "path/filepath" - "sort" + "slices" "strconv" "strings" @@ -128,55 +128,62 @@ func GetMountpointAndUnmountedShares(ctx context.Context, gwc gateway.GatewayAPI } // we need to sort the received shares by mount point in order to make things easier to evaluate. - mount := filepath.Clean(info.Name) + base := filepath.Clean(info.Name) + mount := base existingMountpoint := "" - mountedShares := make([]*collaboration.ReceivedShare, 0, len(receivedShares)) + mountedShares := make([]string, 0, len(receivedShares)) + var pathExists bool + for _, s := range receivedShares { - if utils.ResourceIDEqual(s.Share.ResourceId, info.GetId()) { - if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED { - // a share to the resource already exists and is mounted, remember the mount point - _, err := utils.GetResourceByID(ctx, s.Share.ResourceId, gwc) - if err == nil { - existingMountpoint = s.MountPoint.Path - } - } else { - // a share to the resource already exists but is not mounted, collect the unmounted share - unmountedShares = append(unmountedShares, s) + resourceIDEqual := utils.ResourceIDEqual(s.GetShare().GetResourceId(), info.GetId()) + + if resourceIDEqual && s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED { + // a share to the resource already exists and is mounted, remember the mount point + _, err := utils.GetResourceByID(ctx, s.GetShare().GetResourceId(), gwc) + if err == nil { + existingMountpoint = s.GetMountPoint().GetPath() } } + 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 { - mountedShares = append(mountedShares, s) + // collect all accepted mount points + mountedShares = append(mountedShares, s.GetMountPoint().GetPath()) + if s.GetMountPoint().GetPath() == mount { + // does the shared resource still exist? + _, err := utils.GetResourceByID(ctx, s.GetShare().GetResourceId(), gwc) + if err == nil { + pathExists = true + } + // TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better + } } } - sort.Slice(mountedShares, func(i, j int) bool { - return mountedShares[i].MountPoint.Path > mountedShares[j].MountPoint.Path - }) - if existingMountpoint != "" { // we want to reuse the same mountpoint for all unmounted shares to the same resource return existingMountpoint, unmountedShares, nil } - // we have a list of shares, we want to iterate over all of them and check for name collisions - for i, ms := range mountedShares { - if ms.MountPoint.Path == mount { - // does the shared resource still exist? - _, err := utils.GetResourceByID(ctx, ms.Share.ResourceId, gwc) - if err == nil { - // The mount point really already exists, we need to insert a number into the filename - ext := filepath.Ext(mount) - name := strings.TrimSuffix(mount, ext) - // be smart about .tar.(gz|bz) files - if strings.HasSuffix(name, ".tar") { - name = strings.TrimSuffix(name, ".tar") - ext = ".tar" + ext - } - - mount = fmt.Sprintf("%s (%s)%s", name, strconv.Itoa(i+1), ext) + // If the mount point really already exists, we need to insert a number into the filename + if pathExists { + // now we have a list of shares, we want to iterate over all of them and check for name collisions agents a mount points list + for i := 1; i <= len(mountedShares)+1; i++ { + ext := filepath.Ext(base) + name := strings.TrimSuffix(base, ext) + // be smart about .tar.(gz|bz) files + if strings.HasSuffix(name, ".tar") { + name = strings.TrimSuffix(name, ".tar") + ext = ".tar" + ext + } + mount = name + " (" + strconv.Itoa(i) + ")" + ext + if !slices.Contains(mountedShares, mount) { + return mount, unmountedShares, nil } - // TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better } } return mount, unmountedShares, nil diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go index 404f094b210..5977e3574d8 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go @@ -24,6 +24,7 @@ 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/cs3org/reva/v2/internal/http/services/owncloud/ocs/config" @@ -341,5 +342,117 @@ var _ = Describe("The ocs API", func() { client.AssertNumberOfCalls(GinkgoT(), "UpdateReceivedShare", 1) }) }) + + Context("GetMountpointAndUnmountedShares ", func() { + storage := "storage-users-1" + userOneSpaceId := "first-user-id-0000-000000000000" + userTwoSpaceId := "second-user-id-0000-000000000000" + BeforeEach(func() { + client.On("ListReceivedShares", mock.Anything, mock.Anything, mock.Anything).Return(&collaboration.ListReceivedSharesResponse{ + Status: status.NewOK(context.Background()), + Shares: []*collaboration.ReceivedShare{ + { + Share: &collaboration.Share{ + ResourceId: &provider.ResourceId{ + StorageId: storage, + OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d", + SpaceId: userOneSpaceId, + }, + }, + State: 1, + }, + { + Share: &collaboration.Share{ + ResourceId: &provider.ResourceId{ + StorageId: storage, + OpaqueId: "9284d5fc-da4c-448f-a999-797a2aa1376e", + SpaceId: userOneSpaceId, + }, + }, + State: 2, + MountPoint: &provider.Reference{ + Path: "b.txt", + }, + }, + { + Share: &collaboration.Share{ + ResourceId: &provider.ResourceId{ + StorageId: storage, + OpaqueId: "3a83e157-f03b-4cd5-b64a-d5240c6e06b5", + SpaceId: userOneSpaceId, + }, + }, + State: 2, + MountPoint: &provider.Reference{ + Path: "b (1).txt", + }, + }, + { + Share: &collaboration.Share{ + ResourceId: &provider.ResourceId{ + StorageId: storage, + OpaqueId: "9bed6929-6691-4f5d-ba5e-b2069fe508c7", + SpaceId: userTwoSpaceId, + }, + }, + State: 2, + MountPoint: &provider.Reference{ + Path: "demo.tar.gz", + }, + }, + { + Share: &collaboration.Share{ + ResourceId: &provider.ResourceId{ + StorageId: storage, + OpaqueId: "f1a62fe5-6acb-469c-bbe8-d5206a13b3a1", + SpaceId: userOneSpaceId, + }, + }, + State: 2, + MountPoint: &provider.Reference{ + Path: "a (1).txt", + }, + }, + }, + }, nil) + }) + + DescribeTable("Resolve mountpoint name", + func(info *provider.ResourceInfo, expected string, 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) + fileName, unmounted, err := shares.GetMountpointAndUnmountedShares(ctx, client, info) + Expect(fileName).To(Equal(expected)) + 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}, + }, "b (2).txt", 0), + Entry("new mountpoint, name changed", &provider.ResourceInfo{ + Name: "a (1).txt", + Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId}, + }, "a (1) (1).txt", 0), + Entry("new mountpoint, name is not collide", &provider.ResourceInfo{ + Name: "file.txt", + Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId}, + }, "file.txt", 0), + Entry("existing mountpoint", &provider.ResourceInfo{ + Name: "b.txt", + Id: &provider.ResourceId{StorageId: storage, OpaqueId: "9284d5fc-da4c-448f-a999-797a2aa1376e", SpaceId: userOneSpaceId}, + }, "b.txt", 0), + Entry("existing mountpoint tar.gz", &provider.ResourceInfo{ + Name: "demo.tar.gz", + Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId}, + }, "demo (1).tar.gz", 0), + Entry("unmountpoint", &provider.ResourceInfo{ + Name: "b.txt", + Id: &provider.ResourceId{StorageId: storage, OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d", SpaceId: userOneSpaceId}, + }, "b (2).txt", 1), + ) + }) }) })