Merge pull request #3894 from 2403905/issue-6197

fix unexpected behavior when the user try to share the locked resource

changelog/unreleased/fix-share-locked-file.md

@@ -0,0 +1,7 @@
+Enhancement: Fix err when the user share the locked file
+
+Fix unexpected behavior when the user try to share the locked file
+
+
+https://github.com/cs3org/reva/pull/3894
+https://github.com/owncloud/ocis/issues/6197

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-20221012090518-ef2996678965
+	github.com/cs3org/go-cs3apis v0.0.0-20230516150832-730ac860c71d
 	github.com/cubewise-code/go-mime v0.0.0-20200519001935-8c5762b177d8
 	github.com/dgraph-io/ristretto v0.1.1
 	github.com/emvi/iso-639-1 v1.0.1
@@ -213,4 +213,4 @@ require (
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
 
-replace github.com/cs3org/go-cs3apis => github.com/c0rby/go-cs3apis v0.0.0-20230110100311-5b424f1baa35
+replace github.com/cs3org/go-cs3apis => github.com/2403905/go-cs3apis v0.0.0-20230517122726-727045414fd1

go.sum

@@ -119,6 +119,8 @@ cloud.google.com/go/workflows v1.6.0/go.mod h1:6t9F5h/unJz41YqfBmqSASJSXccBLtD1V
 contrib.go.opencensus.io/exporter/prometheus v0.4.2 h1:sqfsYl5GIY/L570iT+l93ehxaWJs2/OwXtiWwew3oAg=
 contrib.go.opencensus.io/exporter/prometheus v0.4.2/go.mod h1:dvEHbiKmgvbr5pjaF9fpw1KeYcjrnC1J8B+JKjsZyRQ=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
+github.com/2403905/go-cs3apis v0.0.0-20230517122726-727045414fd1 h1:dOIG9lXUo5CAZbjlegvZpeTqfAlH+zn+0uXbtlZjCPY=
+github.com/2403905/go-cs3apis v0.0.0-20230517122726-727045414fd1/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY=
 github.com/Azure/azure-pipeline-go v0.2.3/go.mod h1:x841ezTBIMG6O3lAcl8ATHnsOPVl2bqk7S3ta6S6u4k=
 github.com/Azure/azure-storage-blob-go v0.14.0/go.mod h1:SMqIBi+SuiQH32bvyjngEewEeXoPfKMgWlBDaYf6fck=
 github.com/Azure/go-autorest v14.2.0+incompatible/go.mod h1:r+4oMnoxhatjLLJ6zxSWATqVooLgysK6ZNox3g/xq24=
@@ -203,8 +205,6 @@ github.com/bwesterb/go-ristretto v1.2.0/go.mod h1:fUIoIZaG73pV5biE2Blr2xEzDoMj7N
 github.com/bwesterb/go-ristretto v1.2.1/go.mod h1:fUIoIZaG73pV5biE2Blr2xEzDoMj7NFEuV9ekS419A0=
 github.com/c-bata/go-prompt v0.2.6 h1:POP+nrHE+DfLYx370bedwNhsqmpCUynWPxuHi0C5vZI=
 github.com/c-bata/go-prompt v0.2.6/go.mod h1:/LMAke8wD2FsNu9EXNdHxNLbd9MedkPnCdfpU9wwHfY=
-github.com/c0rby/go-cs3apis v0.0.0-20230110100311-5b424f1baa35 h1:bbpRY/l4z5MTH+TRGZdkIqDM9JXQQewJdO1o+80zcok=
-github.com/c0rby/go-cs3apis v0.0.0-20230110100311-5b424f1baa35/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY=
 github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4=
 github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
 github.com/cenkalti/backoff/v4 v4.1.2/go.mod h1:scbssz8iZGpm3xbr14ovlUdkxfGXNInqkPWOWmG2CLw=

internal/grpc/services/datatx/datatx.go

@@ -156,7 +156,7 @@ func (s *service) UnprotectedEndpoints() []string {
 	return []string{}
 }
 
-func (s *service) PullTransfer(ctx context.Context, req *datatx.PullTransferRequest) (*datatx.PullTransferResponse, error) {
+func (s *service) CreateTransfer(ctx context.Context, req *datatx.CreateTransferRequest) (*datatx.CreateTransferResponse, error) {
 	srcEp, err := s.extractEndpointInfo(ctx, req.SrcTargetUri)
 	if err != nil {
 		return nil, err
@@ -189,21 +189,21 @@ func (s *service) PullTransfer(ctx context.Context, req *datatx.PullTransferRequ
 	s.txShareDriver.model.TxShares[txInfo.GetId().OpaqueId] = txShare
 	if err := s.txShareDriver.model.saveTxShare(); err != nil {
 		err = errors.Wrap(err, "datatx service: error saving transfer share: "+datatx.Status_STATUS_INVALID.String())
-		return &datatx.PullTransferResponse{
+		return &datatx.CreateTransferResponse{
 			Status: status.NewInvalid(ctx, "error pulling transfer"),
 		}, err
 	}
 
 	// now check start transfer outcome
 	if startTransferErr != nil {
 		startTransferErr = errors.Wrap(startTransferErr, "datatx service: error starting transfer job")
-		return &datatx.PullTransferResponse{
+		return &datatx.CreateTransferResponse{
 			Status: status.NewInvalid(ctx, "datatx service: error pulling transfer"),
 			TxInfo: txInfo,
 		}, startTransferErr
 	}
 
-	return &datatx.PullTransferResponse{
+	return &datatx.CreateTransferResponse{
 		Status: status.NewOK(ctx),
 		TxInfo: txInfo,
 	}, err

internal/grpc/services/gateway/datatx.go

@@ -27,15 +27,15 @@ import (
 	"github.com/pkg/errors"
 )
 
-func (s *svc) PullTransfer(ctx context.Context, req *datatx.PullTransferRequest) (*datatx.PullTransferResponse, error) {
+func (s *svc) CreateTransfer(ctx context.Context, req *datatx.CreateTransferRequest) (*datatx.CreateTransferResponse, error) {
 	c, err := pool.GetDataTxClient(s.c.DataTxEndpoint)
 	if err != nil {
-		return &datatx.PullTransferResponse{
+		return &datatx.CreateTransferResponse{
 			Status: status.NewInternal(ctx, "error getting data transfer client"),
 		}, nil
 	}
 
-	res, err := c.PullTransfer(ctx, req)
+	res, err := c.CreateTransfer(ctx, req)
 	if err != nil {
 		return nil, errors.Wrap(err, "gateway: error calling PullTransfer")
 	}

internal/grpc/services/gateway/ocmshareprovider.go

@@ -345,12 +345,12 @@ func (s *svc) UpdateReceivedOCMShare(ctx context.Context, req *ocm.UpdateReceive
 							},
 						},
 					}
-					req := &datatx.PullTransferRequest{
+					req := &datatx.CreateTransferRequest{
 						SrcTargetUri:  srcTargetURI,
 						DestTargetUri: destTargetURI,
 						Opaque:        opaqueObj,
 					}
-					res, err := s.PullTransfer(ctx, req)
+					res, err := s.CreateTransfer(ctx, req)
 					if err != nil {
 						log.Err(err).Msg("gateway: error calling PullTransfer")
 						return &ocm.UpdateReceivedOCMShareResponse{

internal/grpc/services/gateway/usershareprovider.go

@@ -468,6 +468,23 @@ func (s *svc) addShare(ctx context.Context, req *collaboration.CreateShareReques
 		return res, nil
 	}
 
+	rollBackFn := func(status *rpc.Status) {
+		rmvReq := &collaboration.RemoveShareRequest{
+			Ref: &collaboration.ShareReference{
+				Spec: &collaboration.ShareReference_Key{
+					Key: &collaboration.ShareKey{
+						ResourceId: req.ResourceInfo.Id,
+						Grantee:    req.Grant.Grantee,
+					},
+				},
+			},
+		}
+		appctx.GetLogger(ctx).Debug().Interface("status", status).Interface("req", req).Msg("rollback the CreateShare attempt")
+		if resp, err := s.removeShare(ctx, rmvReq); err != nil {
+			appctx.GetLogger(ctx).Debug().Interface("status", resp.GetStatus()).Interface("req", req).Msg(err.Error())
+		}
+	}
+
 	if s.c.CommitShareToStorageGrant {
 		// If the share is a denial we call  denyGrant instead.
 		var status *rpc.Status
@@ -479,6 +496,8 @@ func (s *svc) addShare(ctx context.Context, req *collaboration.CreateShareReques
 		} else {
 			status, err = s.addGrant(ctx, req.ResourceInfo.Id, req.Grant.Grantee, req.Grant.Permissions.Permissions, req.Grant.Expiration, nil)
 			if err != nil {
+				appctx.GetLogger(ctx).Debug().Interface("status", status).Interface("req", req).Msg(err.Error())
+				rollBackFn(status)
 				return nil, errors.Wrap(err, "gateway: error adding grant to storage")
 			}
 		}
@@ -488,7 +507,10 @@ func (s *svc) addShare(ctx context.Context, req *collaboration.CreateShareReques
 			s.statCache.RemoveStat(ctxpkg.ContextMustGetUser(ctx).GetId(), req.ResourceInfo.Id)
 		case rpc.Code_CODE_UNIMPLEMENTED:
 			appctx.GetLogger(ctx).Debug().Interface("status", status).Interface("req", req).Msg("storing grants not supported, ignoring")
+			rollBackFn(status)
 		default:
+			appctx.GetLogger(ctx).Debug().Interface("status", status).Interface("req", req).Msg("storing grants is not successful")
+			rollBackFn(status)
 			return &collaboration.CreateShareResponse{
 				Status: status,
 			}, err

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

@@ -1453,6 +1453,12 @@ func (h *Handler) createCs3Share(ctx context.Context, w http.ResponseWriter, r *
 				Message: createShareResponse.Status.Message,
 				Error:   nil,
 			}
+		case rpc.Code_CODE_LOCKED:
+			return nil, &ocsError{
+				Code:    response.MetaLocked.StatusCode,
+				Message: response.MessageLockedForSharing,
+				Error:   nil,
+			}
 		default:
 			return nil, &ocsError{
 				Code:    response.MetaServerError.StatusCode,

internal/http/services/owncloud/ocs/response/response.go

@@ -127,6 +127,9 @@ var MetaBadRequest = Meta{Status: "error", StatusCode: 400, Message: "Bad Reques
 // MetaPathNotFound is returned when trying to share not existing resources
 var MetaPathNotFound = Meta{Status: "failure", StatusCode: 404, Message: MessagePathNotFound}
 
+// MetaLocked is returned when trying to share not existing resources
+var MetaLocked = Meta{Status: "failure", StatusCode: 423, Message: "The file is locked"}
+
 // MetaServerError is returned on server errors
 var MetaServerError = Meta{Status: "error", StatusCode: 996, Message: "Server Error"}
 
@@ -151,6 +154,9 @@ var MessagePathNotFound = "Wrong path, file/folder doesn't exist"
 // MessageShareExists is used when a user tries to create a new share for the same user
 var MessageShareExists = "A share for the recipient already exists"
 
+// MessageLockedForSharing is used when a user tries to create a new share until the file is in use by at least one user
+var MessageLockedForSharing = "The file is locked until the file is in use by at least one user"
+
 // WriteOCSSuccess handles writing successful ocs response data
 func WriteOCSSuccess(w http.ResponseWriter, r *http.Request, d interface{}) {
 	WriteOCSData(w, r, MetaOK, d, nil)

pkg/errtypes/errtypes.go

@@ -286,8 +286,8 @@ func NewErrtypeFromStatus(status *rpc.Status) error {
 			return Locked(strings.TrimPrefix(status.Message, "set lock: error: locked by "))
 		}
 		return PermissionDenied(status.Message)
-	// case rpc.Code_CODE_LOCKED:
-	//	return Locked(status.Message)
+	case rpc.Code_CODE_LOCKED:
+		return Locked(status.Message)
 	// case rpc.Code_CODE_DATA_LOSS: ?
 	//	IsPartialContent
 	case rpc.Code_CODE_ABORTED:

pkg/rgrpc/status/status.go

@@ -144,6 +144,15 @@ func NewConflict(ctx context.Context, err error, msg string) *rpc.Status {
 	}
 }
 
+// NewLocked returns a status Code_CODE_LOCKED
+func NewLocked(ctx context.Context, msg string) *rpc.Status {
+	return &rpc.Status{
+		Code:    rpc.Code_CODE_LOCKED,
+		Message: msg,
+		Trace:   getTrace(ctx),
+	}
+}
+
 // NewStatusFromErrType returns a status that corresponds to the given errtype
 func NewStatusFromErrType(ctx context.Context, msg string, err error) *rpc.Status {
 	switch e := err.(type) {
@@ -161,7 +170,7 @@ func NewStatusFromErrType(ctx context.Context, msg string, err error) *rpc.Statu
 	case errtypes.Locked:
 		// FIXME a locked error returns the current lockid
 		// FIXME use NewAborted as per the rpc code docs
-		return NewPermissionDenied(ctx, e, msg+": "+err.Error())
+		return NewLocked(ctx, msg+": "+err.Error())
 	case errtypes.Aborted:
 		return NewAborted(ctx, e, msg+": "+err.Error())
 	case errtypes.PreconditionFailed:
@@ -231,6 +240,7 @@ var httpStatusCode = map[rpc.Code]int{
 	rpc.Code_CODE_UNAVAILABLE:          http.StatusServiceUnavailable,
 	rpc.Code_CODE_UNIMPLEMENTED:        http.StatusNotImplemented,
 	rpc.Code_CODE_UNKNOWN:              http.StatusInternalServerError,
+	rpc.Code_CODE_LOCKED:               http.StatusLocked,
 }
 
 // HTTPStatusFromCode returns an HTTP status code for the rpc code. It returns

tests/integration/grpc/storageprovider_test.go

@@ -608,7 +608,7 @@ var _ = Describe("storage providers", func() {
 					Ref: subdirRef,
 				})
 				Expect(err).ToNot(HaveOccurred())
-				Expect(delRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_PERMISSION_DENIED))
+				Expect(delRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_LOCKED))
 
 				unlockRes, err := serviceClient.Unlock(ctx, &storagep.UnlockRequest{
 					Ref:  subdirRef,
@@ -652,7 +652,7 @@ var _ = Describe("storage providers", func() {
 						Ref: subdirRef,
 					})
 					Expect(err).ToNot(HaveOccurred())
-					Expect(delRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_PERMISSION_DENIED))
+					Expect(delRes.Status.Code).To(Equal(rpcv1beta1.Code_CODE_LOCKED))
 				})
 			})