From 4d76ed44a2c7a8e5a0a85f0937417f6213b3f37c Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 12 Sep 2022 15:49:55 +0200 Subject: [PATCH] Try to improve error handling on share creating Try to generate a more helpful error when trying to reshare something with the original owner of the shared target. See: https://github.com/owncloud/ocis/issues/4336 --- .../unreleased/improve-create-share-err.md | 7 ++++++ .../usershareprovider/usershareprovider.go | 2 +- .../handlers/apps/sharing/shares/shares.go | 22 ++++++++++++++----- pkg/share/manager/cs3/cs3.go | 5 +++++ pkg/share/manager/json/json.go | 2 +- pkg/share/manager/jsoncs3/jsoncs3.go | 2 +- pkg/share/manager/memory/memory.go | 3 +-- pkg/share/manager/owncloudsql/owncloudsql.go | 2 +- 8 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 changelog/unreleased/improve-create-share-err.md diff --git a/changelog/unreleased/improve-create-share-err.md b/changelog/unreleased/improve-create-share-err.md new file mode 100644 index 0000000000..56e046dfb7 --- /dev/null +++ b/changelog/unreleased/improve-create-share-err.md @@ -0,0 +1,7 @@ +Enhancement: Improve CreateShare grpc error reporting + +The errorcode returned by the share provider when creating a share where the sharee +is already the owner of the shared target is a bit more explicit now. Also debug logging +was added for this. + +https://github.com/cs3org/reva/pull/3223 diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index dbd0d34f54..29e3c4a704 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -150,7 +150,7 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar share, err := s.sm.Share(ctx, req.ResourceInfo, req.Grant) if err != nil { return &collaboration.CreateShareResponse{ - Status: status.NewInternal(ctx, "error creating share"), + Status: status.NewStatusFromErrType(ctx, "error creating share", err), }, nil } 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 70d4ca70a4..98d95f0841 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 @@ -1410,6 +1410,7 @@ func (h *Handler) getResourceInfo(ctx context.Context, client gateway.GatewayAPI } func (h *Handler) createCs3Share(ctx context.Context, w http.ResponseWriter, r *http.Request, client gateway.GatewayAPIClient, req *collaboration.CreateShareRequest) (*collaboration.Share, *ocsError) { + logger := appctx.GetLogger(ctx) exists, err := h.granteeExists(ctx, req.Grant.Grantee, req.ResourceInfo.Id) if err != nil { return nil, &ocsError{ @@ -1438,17 +1439,26 @@ func (h *Handler) createCs3Share(ctx context.Context, w http.ResponseWriter, r * } } if createShareResponse.Status.Code != rpc.Code_CODE_OK { - if createShareResponse.Status.Code == rpc.Code_CODE_NOT_FOUND { + logger.Debug().Interface("Code", createShareResponse.Status.Code).Str("message", createShareResponse.Status.Message).Msg("grpc create share request failed") + switch createShareResponse.Status.Code { + case rpc.Code_CODE_NOT_FOUND: return nil, &ocsError{ Code: response.MetaNotFound.StatusCode, Message: "not found", Error: nil, } - } - return nil, &ocsError{ - Code: response.MetaServerError.StatusCode, - Message: "grpc create share request failed", - Error: nil, + case rpc.Code_CODE_INVALID_ARGUMENT: + return nil, &ocsError{ + Code: response.MetaBadRequest.StatusCode, + Message: createShareResponse.Status.Message, + Error: nil, + } + default: + return nil, &ocsError{ + Code: response.MetaServerError.StatusCode, + Message: "grpc create share request failed", + Error: nil, + } } } return createShareResponse.Share, nil diff --git a/pkg/share/manager/cs3/cs3.go b/pkg/share/manager/cs3/cs3.go index 9775c9f8d9..efd5e16081 100644 --- a/pkg/share/manager/cs3/cs3.go +++ b/pkg/share/manager/cs3/cs3.go @@ -272,6 +272,11 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla return nil, err } user := ctxpkg.ContextMustGetUser(ctx) + // do not allow share to myself or the owner if share is for a user + if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && + (utils.UserEqual(g.Grantee.GetUserId(), user.Id) || utils.UserEqual(g.Grantee.GetUserId(), md.Owner)) { + return nil, errtypes.BadRequest("cs3: owner/creator and grantee are the same") + } ts := utils.TSNow() share := &collaboration.Share{ diff --git a/pkg/share/manager/json/json.go b/pkg/share/manager/json/json.go index a0d4e6195e..60a31a7f9c 100644 --- a/pkg/share/manager/json/json.go +++ b/pkg/share/manager/json/json.go @@ -257,7 +257,7 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora // TODO(labkode): should not this be caught already at the gw level? if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && (utils.UserEqual(g.Grantee.GetUserId(), user.Id) || utils.UserEqual(g.Grantee.GetUserId(), md.Owner)) { - return nil, errors.New("json: owner/creator and grantee are the same") + return nil, errtypes.BadRequest("json: owner/creator and grantee are the same") } // check if share already exists. diff --git a/pkg/share/manager/jsoncs3/jsoncs3.go b/pkg/share/manager/jsoncs3/jsoncs3.go index e5ee379886..80d73ff338 100644 --- a/pkg/share/manager/jsoncs3/jsoncs3.go +++ b/pkg/share/manager/jsoncs3/jsoncs3.go @@ -209,7 +209,7 @@ func (m *Manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla // TODO: should this not already be caught at the gw level? if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && (utils.UserEqual(g.Grantee.GetUserId(), user.Id) || utils.UserEqual(g.Grantee.GetUserId(), md.Owner)) { - return nil, errors.New("json: owner/creator and grantee are the same") + return nil, errtypes.BadRequest("jsoncs3: owner/creator and grantee are the same") } // check if share already exists. diff --git a/pkg/share/manager/memory/memory.go b/pkg/share/manager/memory/memory.go index 6ee09e4592..5925ed3a06 100644 --- a/pkg/share/manager/memory/memory.go +++ b/pkg/share/manager/memory/memory.go @@ -20,7 +20,6 @@ package memory import ( "context" - "errors" "fmt" "sync" "sync/atomic" @@ -83,7 +82,7 @@ func (m *manager) Share(ctx context.Context, md *provider.ResourceInfo, g *colla if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && (utils.UserEqual(g.Grantee.GetUserId(), user.Id) || utils.UserEqual(g.Grantee.GetUserId(), md.Owner)) { - return nil, errors.New("memory: owner/creator and grantee are the same") + return nil, errtypes.BadRequest("memory: owner/creator and grantee are the same") } // check if share already exists. diff --git a/pkg/share/manager/owncloudsql/owncloudsql.go b/pkg/share/manager/owncloudsql/owncloudsql.go index bd8d425eae..2809b9a3fe 100644 --- a/pkg/share/manager/owncloudsql/owncloudsql.go +++ b/pkg/share/manager/owncloudsql/owncloudsql.go @@ -112,7 +112,7 @@ func (m *mgr) Share(ctx context.Context, md *provider.ResourceInfo, g *collabora // TODO(labkode): should not this be caught already at the gw level? if g.Grantee.Type == provider.GranteeType_GRANTEE_TYPE_USER && (utils.UserEqual(g.Grantee.GetUserId(), user.Id) || utils.UserEqual(g.Grantee.GetUserId(), md.Owner)) { - return nil, errors.New("owncloudsql: owner/creator and grantee are the same") + return nil, errtypes.BadRequest("owncloudsql: owner/creator and grantee are the same") } // check if share already exists.