revert some locking status codes back to precondition failed

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

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))

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")
 	}

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() {