From c03de70d6fb60dddc11bf49ee4076043bab9fb90 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 17 Aug 2022 11:47:14 +0200 Subject: [PATCH 1/4] revert some locking status codes back to precondition failed, see also cs3org/reva#3003 and owncloud/ocis#4366 --- changelog/unreleased/bugfix-locking-status-codes.md | 7 +++++++ pkg/storage/utils/decomposedfs/node/locks.go | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 changelog/unreleased/bugfix-locking-status-codes.md diff --git a/changelog/unreleased/bugfix-locking-status-codes.md b/changelog/unreleased/bugfix-locking-status-codes.md new file mode 100644 index 0000000000..7c7d78226d --- /dev/null +++ b/changelog/unreleased/bugfix-locking-status-codes.md @@ -0,0 +1,7 @@ +Bugfix: Fix locking response codes + +We've fixed the status codes for locking a file that is already locked. + +https://github.com/cs3org/reva/pull/3157 +https://github.com/owncloud/ocis/issues/4366 +https://github.com/cs3org/reva/pull/3003 diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 9acfde91c7..e51185a6ed 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -67,7 +67,7 @@ func (n *Node) SetLock(ctx context.Context, lock *provider.Lock) error { // file not locked, continue case nil: if l != nil { - return errtypes.Aborted("already locked") + return errtypes.PreconditionFailed("already locked") } default: return errors.Wrap(err, "Decomposedfs: could check if file already is locked") @@ -165,7 +165,7 @@ func (n *Node) RefreshLock(ctx context.Context, lock *provider.Lock) error { f, err := os.OpenFile(n.LockFilePath(), os.O_RDWR, os.ModeExclusive) switch { case errors.Is(err, fs.ErrNotExist): - return errtypes.Aborted("lock does not exist") + return errtypes.PreconditionFailed("lock does not exist") case err != nil: return errors.Wrap(err, "Decomposedfs: could not open lock file") } From 44288f11211032e9fcfe8d067515be063de0a217 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 17 Aug 2022 13:15:17 +0200 Subject: [PATCH 2/4] fix unit tests --- pkg/storage/utils/decomposedfs/node/locks_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 78f127eda5..04dc9447de 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -109,7 +109,7 @@ var _ = Describe("Node locks", func() { err = n.SetLock(env.Ctx, lockByUser) Expect(err).To(HaveOccurred()) - _, ok := err.(errtypes.Aborted) + _, ok := err.(errtypes.PreconditionFailed) Expect(ok).To(BeTrue()) }) }) @@ -132,7 +132,7 @@ var _ = Describe("Node locks", func() { err = n.SetLock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) - _, ok := err.(errtypes.Aborted) + _, ok := err.(errtypes.PreconditionFailed) Expect(ok).To(BeTrue()) }) @@ -174,7 +174,7 @@ var _ = Describe("Node locks", func() { It("fails when the node is unlocked", func() { err := n2.RefreshLock(env.Ctx, lockByUser) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("aborted")) + Expect(err.Error()).To(ContainSubstring("precondition failed")) }) It("refuses to refresh the lock without holding the lock", func() { From f25776d33a777ac60e40b9fe6f5358fea2ca6109 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 17 Aug 2022 13:39:09 +0200 Subject: [PATCH 3/4] fix locking response codes in ocdav --- .../http/services/owncloud/ocdav/locks.go | 48 +++++++++++-------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go index f7dc30cfcc..1dc93d66e4 100644 --- a/internal/http/services/owncloud/ocdav/locks.go +++ b/internal/http/services/owncloud/ocdav/locks.go @@ -209,10 +209,15 @@ func (cls *cs3LS) Create(ctx context.Context, now time.Time, details LockDetails if err != nil { return "", err } - if res.Status.Code != rpc.Code_CODE_OK { + switch res.Status.Code { + case rpc.Code_CODE_OK: + return lockTokenPrefix + token.String(), nil + case rpc.Code_CODE_FAILED_PRECONDITION: + return "", errtypes.Aborted("file is already locked") + default: return "", errtypes.NewErrtypeFromStatus(res.Status) } - return lockTokenPrefix + token.String(), nil + } func (cls *cs3LS) Refresh(ctx context.Context, now time.Time, token string, duration time.Duration) (LockDetails, error) { @@ -232,10 +237,14 @@ func (cls *cs3LS) Unlock(ctx context.Context, now time.Time, ref *provider.Refer if err != nil { return err } - if res.Status.Code != rpc.Code_CODE_OK { + switch res.Status.Code { + case rpc.Code_CODE_OK: + return nil + case rpc.Code_CODE_FAILED_PRECONDITION: + return errtypes.Aborted("file is not locked") + default: return errtypes.NewErrtypeFromStatus(res.Status) } - return nil } // LockDetails are a lock's metadata. @@ -335,10 +344,11 @@ const ( // infiniteDepth. Parsing any other string returns invalidDepth. // // Different WebDAV methods have further constraints on valid depths: -// - PROPFIND has no further restrictions, as per section 9.1. -// - COPY accepts only "0" or "infinity", as per section 9.8.3. -// - MOVE accepts only "infinity", as per section 9.9.2. -// - LOCK accepts only "0" or "infinity", as per section 9.10.3. +// - PROPFIND has no further restrictions, as per section 9.1. +// - COPY accepts only "0" or "infinity", as per section 9.8.3. +// - MOVE accepts only "infinity", as per section 9.9.2. +// - LOCK accepts only "0" or "infinity", as per section 9.10.3. +// // These constraints are enforced by the handleXxx methods. func parseDepth(s string) int { switch s { @@ -353,21 +363,21 @@ func parseDepth(s string) int { } /* - the oc 10 wopi app code locks like this: +the oc 10 wopi app code locks like this: - $storage->lockNodePersistent($file->getInternalPath(), [ - 'token' => $wopiLock, - 'owner' => "{$user->getDisplayName()} via Office Online" - ]); + $storage->lockNodePersistent($file->getInternalPath(), [ + 'token' => $wopiLock, + 'owner' => "{$user->getDisplayName()} via Office Online" + ]); - if owner is empty it defaults to '{displayname} ({email})', which is not a url ... but ... shrug +if owner is empty it defaults to '{displayname} ({email})', which is not a url ... but ... shrug - The LockManager also defaults to exclusive locks: +The LockManager also defaults to exclusive locks: - $scope = ILock::LOCK_SCOPE_EXCLUSIVE; - if (isset($lockInfo['scope'])) { - $scope = $lockInfo['scope']; - } + $scope = ILock::LOCK_SCOPE_EXCLUSIVE; + if (isset($lockInfo['scope'])) { + $scope = $lockInfo['scope']; + } */ func (s *svc) handleLock(w http.ResponseWriter, r *http.Request, ns string) (retStatus int, retErr error) { ctx, span := s.tracerProvider.Tracer(tracerName).Start(r.Context(), fmt.Sprintf("%s %v", r.Method, r.URL.Path)) From 80bbd1c67f0b636ef08c743e524079a384618d8c Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Wed, 17 Aug 2022 13:40:41 +0200 Subject: [PATCH 4/4] fix refresh lock unit test --- pkg/storage/utils/decomposedfs/node/locks_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 04dc9447de..250d8ff036 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -278,7 +278,7 @@ var _ = Describe("Node locks", func() { It("fails when the node is unlocked", func() { err := n2.RefreshLock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("aborted")) + Expect(err.Error()).To(ContainSubstring("precondition failed")) }) It("refuses to refresh the lock without holding the lock", func() {