From 49b338d025e66b597591e7c2a0b7095caf235457 Mon Sep 17 00:00:00 2001 From: sushmith Date: Fri, 19 May 2023 12:18:54 +0530 Subject: [PATCH 1/2] fix: send proper error code in case of bad request - If the id is empty or invalid in request, server should return 400 bad request instead of 500 internal server error - fix and add test cases --- api/handler/v1beta1/appeal.go | 21 +++++--- api/handler/v1beta1/appeal_test.go | 26 +++++++--- core/appeal/errors.go | 13 ++++- core/appeal/service.go | 12 +++++ core/appeal/service_test.go | 47 ++++++++++++----- utils/utils.go | 8 +++ utils/utils_test.go | 83 ++++++++++++++++++++++++++++++ 7 files changed, 182 insertions(+), 28 deletions(-) diff --git a/api/handler/v1beta1/appeal.go b/api/handler/v1beta1/appeal.go index 8a953f637..f124846a1 100644 --- a/api/handler/v1beta1/appeal.go +++ b/api/handler/v1beta1/appeal.go @@ -3,7 +3,6 @@ package v1beta1 import ( "context" "errors" - guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1" "github.com/goto/guardian/core/appeal" "github.com/goto/guardian/domain" @@ -106,15 +105,22 @@ func (s *GRPCServer) CreateAppeal(ctx context.Context, req *guardianv1beta1.Crea func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppealRequest) (*guardianv1beta1.GetAppealResponse, error) { id := req.GetId() - appeal, err := s.appealService.GetByID(ctx, id) + + a, err := s.appealService.GetByID(ctx, id) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err) + switch err { + case appeal.ErrAppealIDEmptyParam, + new(appeal.InvalidError): + return nil, status.Errorf(codes.InvalidArgument, err.Error()) + default: + return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err) + } } - if appeal == nil { + if a == nil { return nil, status.Errorf(codes.NotFound, "appeal not found: %v", id) } - appealProto, err := s.adapter.ToAppealProto(appeal) + appealProto, err := s.adapter.ToAppealProto(a) if err != nil { return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err) } @@ -126,6 +132,7 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.CancelAppealRequest) (*guardianv1beta1.CancelAppealResponse, error) { id := req.GetId() + a, err := s.appealService.Cancel(ctx, id) if err != nil { switch err { @@ -134,7 +141,9 @@ func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.Canc case appeal.ErrAppealStatusCanceled, appeal.ErrAppealStatusApproved, appeal.ErrAppealStatusRejected, - appeal.ErrAppealStatusUnrecognized: + appeal.ErrAppealStatusUnrecognized, + appeal.ErrAppealIDEmptyParam, + new(appeal.InvalidError): return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err) default: return nil, status.Errorf(codes.Internal, "failed to cancel appeal: %v", err) diff --git a/api/handler/v1beta1/appeal_test.go b/api/handler/v1beta1/appeal_test.go index ccbb96289..f3fccb0af 100644 --- a/api/handler/v1beta1/appeal_test.go +++ b/api/handler/v1beta1/appeal_test.go @@ -3,6 +3,7 @@ package v1beta1_test import ( "context" "errors" + "github.com/google/uuid" "time" guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1" @@ -557,7 +558,7 @@ func (s *GrpcHandlersSuite) TestGetAppeal() { s.setup() timeNow := time.Now() - expectedID := "test-appeal-id" + expectedID := uuid.New().String() expectedAppeal := &domain.Appeal{ ID: expectedID, ResourceID: "test-resource-id", @@ -655,7 +656,9 @@ func (s *GrpcHandlersSuite) TestGetAppeal() { s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything). Return(nil, expectedError).Once() - req := &guardianv1beta1.GetAppealRequest{} + req := &guardianv1beta1.GetAppealRequest{ + Id: uuid.New().String(), + } res, err := s.grpcServer.GetAppeal(context.Background(), req) s.Equal(codes.Internal, status.Code(err)) @@ -669,7 +672,9 @@ func (s *GrpcHandlersSuite) TestGetAppeal() { s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything). Return(nil, nil).Once() - req := &guardianv1beta1.GetAppealRequest{} + req := &guardianv1beta1.GetAppealRequest{ + Id: uuid.New().String(), + } res, err := s.grpcServer.GetAppeal(context.Background(), req) s.Equal(codes.NotFound, status.Code(err)) @@ -688,13 +693,16 @@ func (s *GrpcHandlersSuite) TestGetAppeal() { s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything). Return(invalidAppeal, nil).Once() - req := &guardianv1beta1.GetAppealRequest{} + req := &guardianv1beta1.GetAppealRequest{ + Id: uuid.New().String(), + } res, err := s.grpcServer.GetAppeal(context.Background(), req) s.Equal(codes.Internal, status.Code(err)) s.Nil(res) s.appealService.AssertExpectations(s.T()) }) + } func (s *GrpcHandlersSuite) TestCancelAppeal() { @@ -702,7 +710,7 @@ func (s *GrpcHandlersSuite) TestCancelAppeal() { s.setup() timeNow := time.Now() - expectedID := "test-appeal-id" + expectedID := uuid.New().String() expectedAppeal := &domain.Appeal{ ID: expectedID, ResourceID: "test-resource-id", @@ -838,7 +846,9 @@ func (s *GrpcHandlersSuite) TestCancelAppeal() { s.appealService.EXPECT().Cancel(mock.AnythingOfType("*context.emptyCtx"), mock.Anything). Return(nil, tc.expectedError).Once() - req := &guardianv1beta1.CancelAppealRequest{} + req := &guardianv1beta1.CancelAppealRequest{ + Id: uuid.New().String(), + } res, err := s.grpcServer.CancelAppeal(context.Background(), req) s.Equal(tc.expectedStatusCode, status.Code(err)) @@ -859,7 +869,9 @@ func (s *GrpcHandlersSuite) TestCancelAppeal() { s.appealService.EXPECT().Cancel(mock.AnythingOfType("*context.emptyCtx"), mock.Anything). Return(invalidAppeal, nil).Once() - req := &guardianv1beta1.CancelAppealRequest{} + req := &guardianv1beta1.CancelAppealRequest{ + Id: uuid.New().String(), + } res, err := s.grpcServer.CancelAppeal(context.Background(), req) s.Equal(codes.Internal, status.Code(err)) diff --git a/core/appeal/errors.go b/core/appeal/errors.go index f67982011..e8d76b5fe 100644 --- a/core/appeal/errors.go +++ b/core/appeal/errors.go @@ -1,6 +1,9 @@ package appeal -import "errors" +import ( + "errors" + "fmt" +) var ( ErrAppealIDEmptyParam = errors.New("appeal id is required") @@ -50,3 +53,11 @@ var ( ErrApproverNotFound = errors.New("approver not found") ErrGrantNotFound = errors.New("grant not found") ) + +type InvalidError struct { + AppealID string +} + +func (ie InvalidError) Error() string { + return fmt.Sprintf("invalid appeal id: %s", ie.AppealID) +} diff --git a/core/appeal/service.go b/core/appeal/service.go index 454b01946..ac93a88af 100644 --- a/core/appeal/service.go +++ b/core/appeal/service.go @@ -159,6 +159,10 @@ func (s *Service) GetByID(ctx context.Context, id string) (*domain.Appeal, error return nil, ErrAppealIDEmptyParam } + if !utils.IsValidUUID(id) { + return nil, InvalidError{AppealID: id} + } + return s.repo.GetByID(ctx, id) } @@ -590,6 +594,14 @@ func (s *Service) Update(ctx context.Context, appeal *domain.Appeal) error { } func (s *Service) Cancel(ctx context.Context, id string) (*domain.Appeal, error) { + if id == "" { + return nil, ErrAppealIDEmptyParam + } + + if !utils.IsValidUUID(id) { + return nil, InvalidError{AppealID: id} + } + appeal, err := s.GetByID(ctx, id) if err != nil { return nil, err diff --git a/core/appeal/service_test.go b/core/appeal/service_test.go index 053d49ce8..ea3ffccf5 100644 --- a/core/appeal/service_test.go +++ b/core/appeal/service_test.go @@ -87,14 +87,15 @@ func (s *ServiceTestSuite) TestGetByID() { expectedError := errors.New("repository error") s.mockRepository.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).Return(nil, expectedError).Once() - actualResult, actualError := s.service.GetByID(context.Background(), "1") + id := uuid.New().String() + actualResult, actualError := s.service.GetByID(context.Background(), id) s.Nil(actualResult) s.EqualError(actualError, expectedError.Error()) }) s.Run("should return record on success", func() { - expectedID := "1" + expectedID := uuid.New().String() expectedResult := &domain.Appeal{ ID: expectedID, } @@ -1430,6 +1431,7 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithAdditionalAppeals() { func (s *ServiceTestSuite) TestUpdateApproval() { timeNow := time.Now() + appealID := uuid.New().String() appeal.TimeNow = func() time.Time { return timeNow } @@ -1441,23 +1443,23 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Action: "name", }, { - AppealID: "1", + AppealID: appealID, Actor: "user@email.com", Action: "name", }, { - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Actor: "invalidemail", Action: "name", }, { - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Action: "name", }, { - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Actor: "user@email.com", }, @@ -1472,7 +1474,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { }) validApprovalActionParam := domain.ApprovalAction{ - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Actor: "user@email.com", Action: "approve", @@ -1718,13 +1720,13 @@ func (s *ServiceTestSuite) TestUpdateApproval() { s.Run("should terminate existing active grant if present", func() { action := domain.ApprovalAction{ - AppealID: "1", + AppealID: appealID, ApprovalName: "test-approval-step", Action: "approve", Actor: "approver@example.com", } appealDetails := &domain.Appeal{ - ID: "1", + ID: appealID, AccountID: "user@example.com", ResourceID: "1", Role: "test-role", @@ -1887,7 +1889,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { { name: "reject", expectedApprovalAction: domain.ApprovalAction{ - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Actor: "user@email.com", Action: domain.AppealActionNameReject, @@ -1962,7 +1964,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { { name: "reject in the middle step", expectedApprovalAction: domain.ApprovalAction{ - AppealID: "1", + AppealID: appealID, ApprovalName: "approval_1", Actor: user, Action: domain.AppealActionNameReject, @@ -2289,9 +2291,26 @@ func (s *ServiceTestSuite) TestGrantAccessToProvider() { }) } -// func (s *ServiceTestSuite) TestCancel() { -// s.Run("should return error from") -// } +func (s *ServiceTestSuite) TestCancel() { + s.Run("should return error if appeal id is empty", func() { + id := "" + expectedErr := appeal.ErrAppealIDEmptyParam + + actualResult, actualErr := s.service.Cancel(context.Background(), id) + s.Nil(actualResult) + s.EqualError(actualErr, expectedErr.Error()) + }) + + s.Run("should return error if appeal id is invalid", func() { + id := "abc" + expectedErr := appeal.InvalidError{AppealID: id} + + actualResult, actualErr := s.service.Cancel(context.Background(), id) + s.Nil(actualResult) + s.EqualError(actualErr, expectedErr.Error()) + }) + +} func (s *ServiceTestSuite) TestAddApprover() { s.Run("should return appeal on success", func() { diff --git a/utils/utils.go b/utils/utils.go index 74fa4b186..77d5ed3b7 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -1,5 +1,7 @@ package utils +import "github.com/google/uuid" + func IsInteger(val float64) bool { return val == float64(int(val)) } @@ -19,3 +21,9 @@ func MapToSlice(m map[string]string) []string { } return s } + +// IsValidUUID returns true if uuid is valid +func IsValidUUID(u string) bool { + _, err := uuid.Parse(u) + return err == nil +} diff --git a/utils/utils_test.go b/utils/utils_test.go index 74c2620a7..1a122a13f 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -48,3 +48,86 @@ func TestMapToSlice(t *testing.T) { }) } } + +func TestIsValidUUID(t *testing.T) { + type args struct { + u string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "valid uuid", + args: args{ + u: "123e4567-e89b-12d3-a456-426614174000", + }, + want: true, + }, + { + name: "invalid uuid", + args: args{ + u: "123e4567-a456-42661417400", + }, + want: false, + }, + { + name: "empty uuid", + args: args{ + u: "", + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, IsValidUUID(tt.args.u), "IsValidUUID(%v)", tt.args.u) + }) + } +} + +func TestIsInteger(t *testing.T) { + type args struct { + val float64 + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "is integer", + args: args{ + val: 1, + }, + want: true, + }, + { + name: "is not integer", + args: args{ + val: 1.1, + }, + want: false, + }, + { + name: "is not integer", + args: args{ + val: 0.1, + }, + want: false, + }, + { + name: "is integer", + args: args{ + val: 0.0, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equalf(t, tt.want, IsInteger(tt.args.val), "IsInteger(%v)", tt.args.val) + }) + } +} From 87e2d0acf436487b0ebaaa20a14cdc0177e6f521 Mon Sep 17 00:00:00 2001 From: sushmith Date: Fri, 19 May 2023 14:01:17 +0530 Subject: [PATCH 2/2] fix: use if-else instead of switch case for catching typed errors --- api/handler/v1beta1/appeal.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/api/handler/v1beta1/appeal.go b/api/handler/v1beta1/appeal.go index f124846a1..ea35ec4a2 100644 --- a/api/handler/v1beta1/appeal.go +++ b/api/handler/v1beta1/appeal.go @@ -108,14 +108,12 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe a, err := s.appealService.GetByID(ctx, id) if err != nil { - switch err { - case appeal.ErrAppealIDEmptyParam, - new(appeal.InvalidError): + if errors.As(err, new(appeal.InvalidError)) || errors.Is(err, appeal.ErrAppealIDEmptyParam) { return nil, status.Errorf(codes.InvalidArgument, err.Error()) - default: - return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err) } + return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err) } + if a == nil { return nil, status.Errorf(codes.NotFound, "appeal not found: %v", id) } @@ -135,15 +133,17 @@ func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.Canc a, err := s.appealService.Cancel(ctx, id) if err != nil { + if errors.As(err, new(appeal.InvalidError)) || errors.Is(err, appeal.ErrAppealIDEmptyParam) { + return nil, status.Errorf(codes.InvalidArgument, err.Error()) + } + switch err { case appeal.ErrAppealNotFound: return nil, status.Errorf(codes.NotFound, "appeal not found: %v", id) case appeal.ErrAppealStatusCanceled, appeal.ErrAppealStatusApproved, appeal.ErrAppealStatusRejected, - appeal.ErrAppealStatusUnrecognized, - appeal.ErrAppealIDEmptyParam, - new(appeal.InvalidError): + appeal.ErrAppealStatusUnrecognized: return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err) default: return nil, status.Errorf(codes.Internal, "failed to cancel appeal: %v", err)