From 02b43ee3886edf8373737941eb785ee590ef71e6 Mon Sep 17 00:00:00 2001
From: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date: Thu, 7 Oct 2021 16:21:30 +0200
Subject: [PATCH] Handle "not found" errors properly in user- and groupprovider

Return a proper NOT_FOUND error instead of INTERNAL when a user or group is not found.
---
 .../unreleased/handle-user-group-not-found.md |  3 +++
 .../services/groupprovider/groupprovider.go   | 24 ++++++++++++-------
 .../services/userprovider/userprovider.go     | 20 +++++++++-------
 tests/integration/grpc/userprovider_test.go   |  6 ++---
 4 files changed, 34 insertions(+), 19 deletions(-)
 create mode 100644 changelog/unreleased/handle-user-group-not-found.md

diff --git a/changelog/unreleased/handle-user-group-not-found.md b/changelog/unreleased/handle-user-group-not-found.md
new file mode 100644
index 00000000000..5cf9f05dd0d
--- /dev/null
+++ b/changelog/unreleased/handle-user-group-not-found.md
@@ -0,0 +1,3 @@
+Enhancement: Return a proper NOT_FOUND error when a user or group is not found
+
+https://github.com/cs3org/reva/pull/2282
diff --git a/internal/grpc/services/groupprovider/groupprovider.go b/internal/grpc/services/groupprovider/groupprovider.go
index 1fda6e7d050..a601c4fd528 100644
--- a/internal/grpc/services/groupprovider/groupprovider.go
+++ b/internal/grpc/services/groupprovider/groupprovider.go
@@ -103,10 +103,14 @@ func (s *service) Register(ss *grpc.Server) {
 func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*grouppb.GetGroupResponse, error) {
 	group, err := s.groupmgr.GetGroup(ctx, req.GroupId)
 	if err != nil {
-		err = errors.Wrap(err, "groupprovidersvc: error getting group")
-		return &grouppb.GetGroupResponse{
-			Status: status.NewInternal(ctx, err, "error getting group"),
-		}, nil
+		res := &grouppb.GetGroupResponse{}
+		if _, ok := err.(errtypes.NotFound); ok {
+			res.Status = status.NewNotFound(ctx, fmt.Sprintf("group not found"))
+		} else {
+			err = errors.Wrap(err, "groupprovidersvc: error getting group")
+			res.Status = status.NewInternal(ctx, err, "error getting group")
+		}
+		return res, nil
 	}
 
 	return &grouppb.GetGroupResponse{
@@ -118,10 +122,14 @@ func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*
 func (s *service) GetGroupByClaim(ctx context.Context, req *grouppb.GetGroupByClaimRequest) (*grouppb.GetGroupByClaimResponse, error) {
 	group, err := s.groupmgr.GetGroupByClaim(ctx, req.Claim, req.Value)
 	if err != nil {
-		err = errors.Wrap(err, "groupprovidersvc: error getting group by claim")
-		return &grouppb.GetGroupByClaimResponse{
-			Status: status.NewInternal(ctx, err, "error getting group by claim"),
-		}, nil
+		res := &grouppb.GetGroupByClaimResponse{}
+		if _, ok := err.(errtypes.NotFound); ok {
+			res.Status = status.NewNotFound(ctx, fmt.Sprintf("group not found %s %s", req.Claim, req.Value))
+		} else {
+			err = errors.Wrap(err, "groupprovidersvc: error getting group by claim")
+			res.Status = status.NewInternal(ctx, err, "error getting group by claim")
+		}
+		return res, nil
 	}
 
 	return &grouppb.GetGroupByClaimResponse{
diff --git a/internal/grpc/services/userprovider/userprovider.go b/internal/grpc/services/userprovider/userprovider.go
index 66f91a1e1ce..cd82bf33048 100644
--- a/internal/grpc/services/userprovider/userprovider.go
+++ b/internal/grpc/services/userprovider/userprovider.go
@@ -127,10 +127,12 @@ func (s *service) Register(ss *grpc.Server) {
 func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*userpb.GetUserResponse, error) {
 	user, err := s.usermgr.GetUser(ctx, req.UserId)
 	if err != nil {
-		// TODO(labkode): check for not found.
-		err = errors.Wrap(err, "userprovidersvc: error getting user")
-		res := &userpb.GetUserResponse{
-			Status: status.NewInternal(ctx, err, "error getting user"),
+		res := &userpb.GetUserResponse{}
+		if _, ok := err.(errtypes.NotFound); ok {
+			res.Status = status.NewNotFound(ctx, "user not found")
+		} else {
+			err = errors.Wrap(err, "userprovidersvc: error getting user")
+			res.Status = status.NewInternal(ctx, err, "error getting user")
 		}
 		return res, nil
 	}
@@ -145,10 +147,12 @@ func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*use
 func (s *service) GetUserByClaim(ctx context.Context, req *userpb.GetUserByClaimRequest) (*userpb.GetUserByClaimResponse, error) {
 	user, err := s.usermgr.GetUserByClaim(ctx, req.Claim, req.Value)
 	if err != nil {
-		// TODO(labkode): check for not found.
-		err = errors.Wrap(err, "userprovidersvc: error getting user by claim")
-		res := &userpb.GetUserByClaimResponse{
-			Status: status.NewInternal(ctx, err, "error getting user by claim"),
+		res := &userpb.GetUserByClaimResponse{}
+		if _, ok := err.(errtypes.NotFound); ok {
+			res.Status = status.NewNotFound(ctx, fmt.Sprintf("user not found %s %s", req.Claim, req.Value))
+		} else {
+			err = errors.Wrap(err, "userprovidersvc: error getting user by claim")
+			res.Status = status.NewInternal(ctx, err, "error getting user by claim")
 		}
 		return res, nil
 	}
diff --git a/tests/integration/grpc/userprovider_test.go b/tests/integration/grpc/userprovider_test.go
index c0252e457b0..1de51e683f3 100644
--- a/tests/integration/grpc/userprovider_test.go
+++ b/tests/integration/grpc/userprovider_test.go
@@ -134,7 +134,7 @@ var _ = Describe("user providers", func() {
 					},
 					want: &userpb.GetUserResponse{
 						Status: &rpc.Status{
-							Code: 15,
+							Code: 6,
 						},
 					},
 				},
@@ -147,7 +147,7 @@ var _ = Describe("user providers", func() {
 					},
 					want: &userpb.GetUserResponse{
 						Status: &rpc.Status{
-							Code: 15,
+							Code: 6,
 						},
 					},
 				},
@@ -160,7 +160,7 @@ var _ = Describe("user providers", func() {
 					},
 					want: &userpb.GetUserResponse{
 						Status: &rpc.Status{
-							Code: 15,
+							Code: 6,
 						},
 					},
 				},