diff --git a/changelog/unreleased/refresh-lock-improvements.md b/changelog/unreleased/refresh-lock-improvements.md new file mode 100644 index 00000000000..69a0b543ece --- /dev/null +++ b/changelog/unreleased/refresh-lock-improvements.md @@ -0,0 +1,6 @@ +Enhancement: Make Refresh Lock operation WOPI cmpliant + +We now support the WOPI compliant `UnlockAndRelock` operation. This has been implemented in the DecomposedFS. To make use of it, we need a compatible WOPI server. + +https://github.com/cs3org/reva/pull/3286 +https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/unlockandrelock diff --git a/go.mod b/go.mod index 4006347f43d..54126f8bac8 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/cheggaaa/pb v1.0.29 github.com/coreos/go-oidc v2.2.1+incompatible github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e - github.com/cs3org/go-cs3apis v0.0.0-20220818202316-e92afdddac6d + github.com/cs3org/go-cs3apis v0.0.0-20220929083235-bb0b1a236d6c github.com/cubewise-code/go-mime v0.0.0-20200519001935-8c5762b177d8 github.com/dgraph-io/ristretto v0.1.0 github.com/emvi/iso-639-1 v1.0.1 @@ -54,7 +54,6 @@ require ( github.com/nats-io/nats.go v1.17.0 github.com/onsi/ginkgo/v2 v2.1.6 github.com/onsi/gomega v1.20.2 - github.com/owncloud/ocis/v2 v2.0.0-beta2.0.20220919072836-08a8ed20c18a github.com/pkg/errors v0.9.1 github.com/pkg/xattr v0.4.7 github.com/prometheus/alertmanager v0.24.0 diff --git a/go.sum b/go.sum index 28048665487..aec977a30f3 100644 --- a/go.sum +++ b/go.sum @@ -193,8 +193,12 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e h1:tqSPWQeueWTKnJVMJffz4pz0o1WuQxJ28+5x5JgaHD8= github.com/cs3org/cato v0.0.0-20200828125504-e418fc54dd5e/go.mod h1:XJEZ3/EQuI3BXTp/6DUzFr850vlxq11I6satRtz0YQ4= +github.com/cs3org/go-cs3apis v0.0.0-20210325133324-32b03d75a535 h1:555D8A3ddKqb4OyK9v5mdphw2zDLWKGXOkcnf1RQwTA= +github.com/cs3org/go-cs3apis v0.0.0-20210325133324-32b03d75a535/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/cs3org/go-cs3apis v0.0.0-20220818202316-e92afdddac6d h1:toyZ7IsXlUdEPZ/IG8fg7hbM8HcLPY0bkX4FKBmgLVI= github.com/cs3org/go-cs3apis v0.0.0-20220818202316-e92afdddac6d/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= +github.com/cs3org/go-cs3apis v0.0.0-20220929083235-bb0b1a236d6c h1:b+YTmOGlf43mnF8MzO0fsy8/Ho8JLu44Iq5Y0fKLJMM= +github.com/cs3org/go-cs3apis v0.0.0-20220929083235-bb0b1a236d6c/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/cubewise-code/go-mime v0.0.0-20200519001935-8c5762b177d8 h1:Z9lwXumT5ACSmJ7WGnFl+OMLLjpz5uR2fyz7dC255FI= github.com/cubewise-code/go-mime v0.0.0-20200519001935-8c5762b177d8/go.mod h1:4abs/jPXcmJzYoYGF91JF9Uq9s/KL5n1jvFDix8KcqY= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -490,6 +494,7 @@ github.com/hashicorp/go-immutable-radix v1.3.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjh github.com/hashicorp/go-immutable-radix v1.3.1 h1:DKHmCUm2hRBK510BaiZlwvpD40f8bJFeZnpfm2KLowc= github.com/hashicorp/go-immutable-radix v1.3.1/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= +github.com/hashicorp/go-msgpack v0.5.5 h1:i9R9JSrqIz0QVLz3sz+i3YJdT7TTSLcfLLzJi9aZTuI= github.com/hashicorp/go-msgpack v0.5.5/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= github.com/hashicorp/go-msgpack v1.1.5 h1:9byZdVjKTe5mce63pRVNP1L7UAmdHOTEMGehn6KvJWs= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= @@ -695,6 +700,7 @@ github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.12.1/go.mod h1:zj2OWP4+oCPe1qIXoGWkgMRwljMUYCdkwsT2108oapk= +github.com/onsi/ginkgo v1.15.0 h1:1V1NfVQR87RtWAgp1lv9JZJ5Jap+XFGKPi00andXGi4= github.com/onsi/ginkgo v1.15.0/go.mod h1:hF8qUzuuC8DJGygJH3726JnCZX4MYbRB8yFfISqnKUg= github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE= github.com/onsi/ginkgo/v2 v2.1.6 h1:Fx2POJZfKRQcM1pH49qSZiYeu319wji004qX+GDovrU= diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go index 931ca9c5fa0..7d95740c7c8 100644 --- a/internal/grpc/services/storageprovider/storageprovider.go +++ b/internal/grpc/services/storageprovider/storageprovider.go @@ -238,7 +238,7 @@ func (s *service) GetLock(ctx context.Context, req *provider.GetLockRequest) (*p // RefreshLock refreshes an existing lock on the given reference func (s *service) RefreshLock(ctx context.Context, req *provider.RefreshLockRequest) (*provider.RefreshLockResponse, error) { - err := s.storage.RefreshLock(ctx, req.Ref, req.Lock) + err := s.storage.RefreshLock(ctx, req.Ref, req.Lock, req.ExistingLockId) return &provider.RefreshLockResponse{ Status: status.NewStatusFromErrType(ctx, "refresh lock", err), diff --git a/pkg/storage/fs/nextcloud/nextcloud.go b/pkg/storage/fs/nextcloud/nextcloud.go index 618eefd11a6..41e173f7892 100644 --- a/pkg/storage/fs/nextcloud/nextcloud.go +++ b/pkg/storage/fs/nextcloud/nextcloud.go @@ -797,7 +797,7 @@ func (nc *StorageDriver) SetLock(ctx context.Context, ref *provider.Reference, l } // RefreshLock refreshes an existing lock on the given reference -func (nc *StorageDriver) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (nc *StorageDriver) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { return errtypes.NotSupported("unimplemented") } diff --git a/pkg/storage/fs/owncloudsql/owncloudsql.go b/pkg/storage/fs/owncloudsql/owncloudsql.go index 37987ef1ee5..08bb1610ce8 100644 --- a/pkg/storage/fs/owncloudsql/owncloudsql.go +++ b/pkg/storage/fs/owncloudsql/owncloudsql.go @@ -1116,7 +1116,7 @@ func (fs *owncloudsqlfs) SetLock(ctx context.Context, ref *provider.Reference, l } // RefreshLock refreshes an existing lock on the given reference -func (fs *owncloudsqlfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (fs *owncloudsqlfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { return errtypes.NotSupported("unimplemented") } diff --git a/pkg/storage/fs/s3/s3.go b/pkg/storage/fs/s3/s3.go index 5921a146be5..1d6604cc3cf 100644 --- a/pkg/storage/fs/s3/s3.go +++ b/pkg/storage/fs/s3/s3.go @@ -291,7 +291,7 @@ func (fs *s3FS) SetLock(ctx context.Context, ref *provider.Reference, lock *prov } // RefreshLock refreshes an existing lock on the given reference -func (fs *s3FS) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (fs *s3FS) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { return errtypes.NotSupported("unimplemented") } diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 7ec5de31dcd..d1b08150017 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -66,7 +66,7 @@ type FS interface { UnsetArbitraryMetadata(ctx context.Context, ref *provider.Reference, keys []string) error SetLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error GetLock(ctx context.Context, ref *provider.Reference) (*provider.Lock, error) - RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error + RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error Unlock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error // ListStorageSpaces lists the spaces in the storage. // The unrestricted parameter can be used to list other user's spaces when diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 3ebdc71fa18..6b7b913f76e 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -705,7 +705,7 @@ func (fs *Decomposedfs) SetLock(ctx context.Context, ref *provider.Reference, lo } // RefreshLock refreshes an existing lock on the given reference -func (fs *Decomposedfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (fs *Decomposedfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { if lock.LockId == "" { return errtypes.BadRequest("missing lockid") } @@ -729,7 +729,7 @@ func (fs *Decomposedfs) RefreshLock(ctx context.Context, ref *provider.Reference return errtypes.PermissionDenied(filepath.Join(node.ParentID, node.Name)) } - return node.RefreshLock(ctx, lock) + return node.RefreshLock(ctx, lock, existingLockID) } // Unlock removes an existing lock from the given reference diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 01f85ef4827..4176021880c 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -141,7 +141,7 @@ func (n Node) ReadLock(ctx context.Context, skipFileLock bool) (*provider.Lock, } // RefreshLock refreshes the node's lock -func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { +func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock, existingLockID string) error { // ensure parent path exists if err := os.MkdirAll(filepath.Dir(n.InternalPath()), 0700); err != nil { @@ -171,17 +171,22 @@ func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { } defer f.Close() - oldLock := &provider.Lock{} - if err := json.NewDecoder(f).Decode(oldLock); err != nil { + readLock := &provider.Lock{} + if err := json.NewDecoder(f).Decode(readLock); err != nil { return errors.Wrap(err, "Decomposedfs: could not read lock") } - // check lock - if oldLock.LockId != lock.LockId { - return errtypes.Aborted("mismatching lock") + // check refresh lockID match + if existingLockID == "" && readLock.LockId != lock.LockId { + return errtypes.Aborted("mismatching lock ID") } - if ok, err := isLockModificationAllowed(ctx, oldLock, lock); !ok { + // check if UnlockAndRelock sends the correct lockID + if existingLockID != "" && readLock.LockId != existingLockID { + return errtypes.Aborted("mismatching existing lock ID") + } + + if ok, err := isLockModificationAllowed(ctx, readLock, lock); !ok { return err } diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 250d8ff036c..8d30a6b011d 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -172,33 +172,46 @@ var _ = Describe("Node locks", func() { }) It("fails when the node is unlocked", func() { - err := n2.RefreshLock(env.Ctx, lockByUser) + err := n2.RefreshLock(env.Ctx, lockByUser, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("precondition failed")) }) It("refuses to refresh the lock without holding the lock", func() { newLock.LockId = "somethingsomething" - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("mismatching")) }) It("refuses to refresh the lock for other users than the lock holder", func() { - err := n.RefreshLock(otherCtx, newLock) + err := n.RefreshLock(otherCtx, newLock, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("permission denied")) }) It("refuses to change the lock holder", func() { newLock.User = otherUser.Id - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("permission denied")) }) + It("refuses to change the lock when we do not send the correct lock id for the old lock", func() { + newLock.LockId = "somethingsomething" + err := n.RefreshLock(env.Ctx, newLock, "somethingdifferent") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mismatching")) + }) + + It("refreshes the lock when we send a new lock and the correct lock id for the old lock", func() { + newLock.LockId = "somethingsomething" + err := n.RefreshLock(env.Ctx, newLock, lockByUser.LockId) + Expect(err).ToNot(HaveOccurred()) + }) + It("refreshes the lock", func() { - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).ToNot(HaveOccurred()) }) }) @@ -276,32 +289,45 @@ var _ = Describe("Node locks", func() { }) It("fails when the node is unlocked", func() { - err := n2.RefreshLock(env.Ctx, lockByApp) + err := n2.RefreshLock(env.Ctx, lockByApp, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("precondition failed")) }) It("refuses to refresh the lock without holding the lock", func() { newLock.LockId = "somethingsomething" - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("mismatching")) }) It("refreshes the lock for other users", func() { - err := n.RefreshLock(otherCtx, lockByApp) + err := n.RefreshLock(otherCtx, lockByApp, "") Expect(err).ToNot(HaveOccurred()) }) It("refuses to change the lock holder", func() { newLock.AppName = wrongLockByApp.AppName - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("permission denied")) }) + It("refuses to change the lock when we do not send the correct lock id for the old lock", func() { + newLock.LockId = "somethingsomething" + err := n.RefreshLock(env.Ctx, newLock, "somethingdifferent") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mismatching")) + }) + + It("refreshes the lock when we send a new lock and the correct lock id for the old lock", func() { + newLock.LockId = "somethingsomething" + err := n.RefreshLock(env.Ctx, newLock, lockByApp.LockId) + Expect(err).ToNot(HaveOccurred()) + }) + It("refreshes the lock", func() { - err := n.RefreshLock(env.Ctx, newLock) + err := n.RefreshLock(env.Ctx, newLock, "") Expect(err).ToNot(HaveOccurred()) }) }) diff --git a/pkg/storage/utils/eosfs/eosfs.go b/pkg/storage/utils/eosfs/eosfs.go index c0f7c45e721..20e4c115e4a 100644 --- a/pkg/storage/utils/eosfs/eosfs.go +++ b/pkg/storage/utils/eosfs/eosfs.go @@ -588,7 +588,7 @@ func (fs *eosfs) SetLock(ctx context.Context, ref *provider.Reference, lock *pro } // RefreshLock refreshes an existing lock on the given reference -func (fs *eosfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (fs *eosfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { return errtypes.NotSupported("unimplemented") } diff --git a/pkg/storage/utils/localfs/localfs.go b/pkg/storage/utils/localfs/localfs.go index a013d7d0ba3..0e6230e60ba 100644 --- a/pkg/storage/utils/localfs/localfs.go +++ b/pkg/storage/utils/localfs/localfs.go @@ -726,7 +726,7 @@ func (fs *localfs) SetLock(ctx context.Context, ref *provider.Reference, lock *p } // RefreshLock refreshes an existing lock on the given reference -func (fs *localfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock) error { +func (fs *localfs) RefreshLock(ctx context.Context, ref *provider.Reference, lock *provider.Lock, existingLockID string) error { return errtypes.NotSupported("unimplemented") }