Try to improve error handling on share creating (#3223)

Try to generate a more helpful error when trying to reshare something with the original
owner of the shared target.

See: owncloud/ocis#4336

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

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
 	}
 

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

@@ -1360,6 +1360,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{
@@ -1388,17 +1389,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

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{

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.

pkg/share/manager/jsoncs3/jsoncs3.go

@@ -210,7 +210,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.

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.

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.