Merge pull request #4057 from aduffeck/proper-error-handling-publicsh…

…areprovider

Proper error handling publicshareprovider

changelog/unreleased/fix-error-handling-in-publicshareprovider.md

@@ -0,0 +1,5 @@
+Bugfix: Properly handle not-found errors when getting a public share
+
+We fixed a problem where not-found errors caused a hard error instead of a proper RPC error state.
+
+https://github.com/cs3org/reva/pull/4057

internal/grpc/services/publicshareprovider/publicshareprovider.go

@@ -22,6 +22,7 @@ import (
 	"context"
 	"regexp"
 
+	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1"
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
@@ -218,15 +219,29 @@ func (s *service) GetPublicShare(ctx context.Context, req *link.GetPublicShareRe
 		log.Error().Msg("error getting user from context")
 	}
 
-	found, err := s.sm.GetPublicShare(ctx, u, req.Ref, req.GetSign())
-	if err != nil {
-		return nil, err
+	ps, err := s.sm.GetPublicShare(ctx, u, req.Ref, req.GetSign())
+	switch {
+	case err != nil:
+		var st *rpc.Status
+		switch err.(type) {
+		case errtypes.IsNotFound:
+			st = status.NewNotFound(ctx, err.Error())
+		default:
+			st = status.NewInternal(ctx, err.Error())
+		}
+		return &link.GetPublicShareResponse{
+			Status: st,
+		}, nil
+	case ps == nil:
+		return &link.GetPublicShareResponse{
+			Status: status.NewNotFound(ctx, "not found"),
+		}, nil
+	default:
+		return &link.GetPublicShareResponse{
+			Status: status.NewOK(ctx),
+			Share:  ps,
+		}, nil
 	}
-
-	return &link.GetPublicShareResponse{
-		Status: status.NewOK(ctx),
-		Share:  found,
-	}, nil
 }
 
 func (s *service) ListPublicShares(ctx context.Context, req *link.ListPublicSharesRequest) (*link.ListPublicSharesResponse, error) {

pkg/publicshare/manager/json/json.go

@@ -430,7 +430,7 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu
 	if ref.GetToken() != "" {
 		ps, pw, err := m.getByToken(ctx, ref.GetToken())
 		if err != nil {
-			return nil, errors.New("no shares found by token")
+			return nil, errtypes.NotFound("no shares found by token")
 		}
 		if ps.PasswordProtected && sign {
 			err := publicshare.AddSignature(ps, pw)
@@ -463,7 +463,7 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu
 				if err := m.revokeExpiredPublicShare(ctx, &ps, u); err != nil {
 					return nil, err
 				}
-				return nil, errors.New("no shares found by id:" + ref.GetId().String())
+				return nil, errtypes.NotFound("no shares found by id:" + ref.GetId().String())
 			}
 			if ps.PasswordProtected && sign {
 				err := publicshare.AddSignature(&ps, passDB)
@@ -475,7 +475,7 @@ func (m *manager) GetPublicShare(ctx context.Context, u *user.User, ref *link.Pu
 		}
 
 	}
-	return nil, errors.New("no shares found by id:" + ref.GetId().String())
+	return nil, errtypes.NotFound("no shares found by id:" + ref.GetId().String())
 }
 
 // ListPublicShares retrieves all the shares on the manager that are valid.