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/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)) 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") } diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 78f127eda5..250d8ff036 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() { @@ -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() {