diff --git a/changelog/unreleased/fix-share-locked-file.md b/changelog/unreleased/fix-share-locked-file.md new file mode 100644 index 0000000000..24349b2f1f --- /dev/null +++ b/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 diff --git a/go.mod b/go.mod index 80a982538a..99cb980d8d 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-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 diff --git a/go.sum b/go.sum index ccb8c34567..4544e1ee1e 100644 --- a/go.sum +++ b/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= diff --git a/internal/grpc/services/datatx/datatx.go b/internal/grpc/services/datatx/datatx.go index b7c364da47..c43e75ec88 100644 --- a/internal/grpc/services/datatx/datatx.go +++ b/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,7 +189,7 @@ 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 } @@ -197,13 +197,13 @@ func (s *service) PullTransfer(ctx context.Context, req *datatx.PullTransferRequ // 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 diff --git a/internal/grpc/services/gateway/datatx.go b/internal/grpc/services/gateway/datatx.go index 400a87491d..4b894922bb 100644 --- a/internal/grpc/services/gateway/datatx.go +++ b/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") } diff --git a/internal/grpc/services/gateway/ocmshareprovider.go b/internal/grpc/services/gateway/ocmshareprovider.go index 5db6b0b4b9..cd42d68079 100644 --- a/internal/grpc/services/gateway/ocmshareprovider.go +++ b/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{ diff --git a/internal/grpc/services/gateway/usershareprovider.go b/internal/grpc/services/gateway/usershareprovider.go index 81f3ffbe84..7d09bbc83d 100644 --- a/internal/grpc/services/gateway/usershareprovider.go +++ b/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 diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go index 54babc0f21..5fa074205f 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go +++ b/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, diff --git a/internal/http/services/owncloud/ocs/response/response.go b/internal/http/services/owncloud/ocs/response/response.go index c72030ead1..6e5497dd95 100644 --- a/internal/http/services/owncloud/ocs/response/response.go +++ b/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) diff --git a/pkg/errtypes/errtypes.go b/pkg/errtypes/errtypes.go index 197a2c6127..affdd04320 100644 --- a/pkg/errtypes/errtypes.go +++ b/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: diff --git a/pkg/rgrpc/status/status.go b/pkg/rgrpc/status/status.go index c524c2753a..6ca73cb4aa 100644 --- a/pkg/rgrpc/status/status.go +++ b/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 diff --git a/tests/cs3mocks/mocks/GatewayAPIClient.go b/tests/cs3mocks/mocks/GatewayAPIClient.go index 169a60df32..0bf22edfc5 100644 --- a/tests/cs3mocks/mocks/GatewayAPIClient.go +++ b/tests/cs3mocks/mocks/GatewayAPIClient.go @@ -495,6 +495,39 @@ func (_m *GatewayAPIClient) CreateSymlink(ctx context.Context, in *providerv1bet return r0, r1 } +// CreateTransfer provides a mock function with given fields: ctx, in, opts +func (_m *GatewayAPIClient) CreateTransfer(ctx context.Context, in *txv1beta1.CreateTransferRequest, opts ...grpc.CallOption) (*txv1beta1.CreateTransferResponse, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, in) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *txv1beta1.CreateTransferResponse + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *txv1beta1.CreateTransferRequest, ...grpc.CallOption) (*txv1beta1.CreateTransferResponse, error)); ok { + return rf(ctx, in, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, *txv1beta1.CreateTransferRequest, ...grpc.CallOption) *txv1beta1.CreateTransferResponse); ok { + r0 = rf(ctx, in, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*txv1beta1.CreateTransferResponse) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *txv1beta1.CreateTransferRequest, ...grpc.CallOption) error); ok { + r1 = rf(ctx, in, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // Delete provides a mock function with given fields: ctx, in, opts func (_m *GatewayAPIClient) Delete(ctx context.Context, in *providerv1beta1.DeleteRequest, opts ...grpc.CallOption) (*providerv1beta1.DeleteResponse, error) { _va := make([]interface{}, len(opts)) @@ -2310,39 +2343,6 @@ func (_m *GatewayAPIClient) OpenInApp(ctx context.Context, in *gatewayv1beta1.Op return r0, r1 } -// PullTransfer provides a mock function with given fields: ctx, in, opts -func (_m *GatewayAPIClient) PullTransfer(ctx context.Context, in *txv1beta1.PullTransferRequest, opts ...grpc.CallOption) (*txv1beta1.PullTransferResponse, error) { - _va := make([]interface{}, len(opts)) - for _i := range opts { - _va[_i] = opts[_i] - } - var _ca []interface{} - _ca = append(_ca, ctx, in) - _ca = append(_ca, _va...) - ret := _m.Called(_ca...) - - var r0 *txv1beta1.PullTransferResponse - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, *txv1beta1.PullTransferRequest, ...grpc.CallOption) (*txv1beta1.PullTransferResponse, error)); ok { - return rf(ctx, in, opts...) - } - if rf, ok := ret.Get(0).(func(context.Context, *txv1beta1.PullTransferRequest, ...grpc.CallOption) *txv1beta1.PullTransferResponse); ok { - r0 = rf(ctx, in, opts...) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*txv1beta1.PullTransferResponse) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, *txv1beta1.PullTransferRequest, ...grpc.CallOption) error); ok { - r1 = rf(ctx, in, opts...) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // PurgeRecycle provides a mock function with given fields: ctx, in, opts func (_m *GatewayAPIClient) PurgeRecycle(ctx context.Context, in *providerv1beta1.PurgeRecycleRequest, opts ...grpc.CallOption) (*providerv1beta1.PurgeRecycleResponse, error) { _va := make([]interface{}, len(opts)) diff --git a/tests/integration/grpc/storageprovider_test.go b/tests/integration/grpc/storageprovider_test.go index 5404ba3f7f..06e3dfd9a6 100644 --- a/tests/integration/grpc/storageprovider_test.go +++ b/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)) }) })