From 60dd206a9285f8161b13a89b72fe68d093cf1e38 Mon Sep 17 00:00:00 2001 From: Rahmat Hidayat Date: Wed, 29 Nov 2023 17:25:30 +0700 Subject: [PATCH] fix: fix error to status code mapping for UpdateApproval --- api/handler/v1beta1/approval.go | 32 +++++------ api/handler/v1beta1/approval_test.go | 39 +++----------- core/appeal/errors.go | 30 ++++++----- core/appeal/service.go | 80 +++++++++++++--------------- core/appeal/service_test.go | 25 +++++---- core/approval/errors.go | 18 +++---- domain/appeal.go | 17 ++++++ 7 files changed, 115 insertions(+), 126 deletions(-) diff --git a/api/handler/v1beta1/approval.go b/api/handler/v1beta1/approval.go index 223ef544b..586533c58 100644 --- a/api/handler/v1beta1/approval.go +++ b/api/handler/v1beta1/approval.go @@ -78,22 +78,24 @@ func (s *GRPCServer) UpdateApproval(ctx context.Context, req *guardianv1beta1.Up Reason: req.GetAction().GetReason(), }) if err != nil { - switch err { - case appeal.ErrAppealStatusCanceled, - appeal.ErrAppealStatusApproved, - appeal.ErrAppealStatusRejected, - appeal.ErrAppealStatusUnrecognized, - appeal.ErrApprovalDependencyIsPending, - appeal.ErrApprovalStatusUnrecognized, - appeal.ErrApprovalStatusApproved, - appeal.ErrApprovalStatusRejected, - appeal.ErrApprovalStatusSkipped, - appeal.ErrActionInvalidValue: - return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err) - case appeal.ErrActionForbidden: + switch { + case + errors.Is(err, appeal.ErrInvalidUpdateApprovalParameter), + errors.Is(err, appeal.ErrAppealIDEmptyParam), + errors.Is(err, appeal.ErrActionInvalidValue): + return nil, status.Errorf(codes.InvalidArgument, err.Error()) + case + errors.Is(err, appeal.ErrAppealNotEligibleForApproval), + errors.Is(err, appeal.ErrAppealStatusUnrecognized), + errors.Is(err, appeal.ErrApprovalNotEligibleForAction), + errors.Is(err, appeal.ErrApprovalStatusUnrecognized): + return nil, status.Errorf(codes.FailedPrecondition, err.Error()) + case errors.Is(err, appeal.ErrActionForbidden): return nil, status.Error(codes.PermissionDenied, "permission denied") - case appeal.ErrApprovalNotFound: - return nil, status.Errorf(codes.NotFound, "approval not found: %v", id) + case + errors.Is(err, appeal.ErrAppealNotFound), + errors.Is(err, appeal.ErrApprovalNotFound): + return nil, status.Errorf(codes.NotFound, err.Error()) default: return nil, s.internalError(ctx, "failed to update approval: %v", err) } diff --git a/api/handler/v1beta1/approval_test.go b/api/handler/v1beta1/approval_test.go index 2c42050d1..b9285ebe6 100644 --- a/api/handler/v1beta1/approval_test.go +++ b/api/handler/v1beta1/approval_test.go @@ -493,49 +493,24 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() { }{ { - "should return invalid error if appeal status already cancelled", - appeal.ErrAppealStatusCanceled, - codes.InvalidArgument, - }, - { - "should return invalid error if appeal status already approved", - appeal.ErrAppealStatusApproved, - codes.InvalidArgument, - }, - { - "should return invalid error if appeal status already rejected", - appeal.ErrAppealStatusRejected, - codes.InvalidArgument, + "should return invalid error if appeal status is not eligible for approval", + appeal.ErrAppealNotEligibleForApproval, + codes.FailedPrecondition, }, { "should return invalid error if appeal status unrecognized", appeal.ErrAppealStatusUnrecognized, - codes.InvalidArgument, + codes.FailedPrecondition, }, { "should return invalid error if approval dependency is still pending", - appeal.ErrApprovalDependencyIsPending, - codes.InvalidArgument, + appeal.ErrApprovalNotEligibleForAction, + codes.FailedPrecondition, }, { "should return invalid error if approval status unrecognized", appeal.ErrApprovalStatusUnrecognized, - codes.InvalidArgument, - }, - { - "should return invalid error if approval status already approved", - appeal.ErrApprovalStatusApproved, - codes.InvalidArgument, - }, - { - "should return invalid error if approval status already rejected", - appeal.ErrApprovalStatusRejected, - codes.InvalidArgument, - }, - { - "should return invalid error if approval status already skipped", - appeal.ErrApprovalStatusSkipped, - codes.InvalidArgument, + codes.FailedPrecondition, }, { "should return invalid error if action name is invalid", diff --git a/core/appeal/errors.go b/core/appeal/errors.go index 116504334..444e8e47f 100644 --- a/core/appeal/errors.go +++ b/core/appeal/errors.go @@ -20,15 +20,13 @@ var ( ErrGrantNotEligibleForExtension = errors.New("grant not eligible for extension") ErrCannotCreateAppealForOtherUser = errors.New("creating appeal for other individual user (account_type=\"user\") is not allowed") - ErrApprovalDependencyIsBlocked = errors.New("found previous approval step that is still in blocked") - ErrApprovalDependencyIsPending = errors.New("found previous approval step that is still in pending") - ErrApprovalStatusApproved = errors.New("approval already approved") - ErrApprovalStatusRejected = errors.New("approval already rejected") - ErrApprovalStatusSkipped = errors.New("approval already skipped") - ErrApprovalStatusUnrecognized = errors.New("unrecognized approval status") - ErrApprovalNotFound = errors.New("approval not found") - ErrUnableToAddApprover = errors.New("unable to add a new approver") - ErrUnableToDeleteApprover = errors.New("unable to remove approver") + ErrApprovalStatusApproved = errors.New("approval already approved") + ErrApprovalStatusRejected = errors.New("approval already rejected") + ErrApprovalStatusSkipped = errors.New("approval already skipped") + ErrApprovalStatusUnrecognized = errors.New("unrecognized approval status") + ErrApprovalNotFound = errors.New("approval not found") + ErrUnableToAddApprover = errors.New("unable to add a new approver") + ErrUnableToDeleteApprover = errors.New("unable to remove approver") ErrActionForbidden = errors.New("user is not allowed to make action on this approval step") ErrActionInvalidValue = errors.New("invalid action value") @@ -45,11 +43,15 @@ var ( ErrDurationNotAllowed = errors.New("duration value not allowed") ErrDurationIsRequired = errors.New("having permanent access to this resource is not allowed, access duration is required") - ErrApproverKeyNotRecognized = errors.New("unrecognized approvers key") - ErrApproverInvalidType = errors.New("invalid approver type, expected an email string or array of email string") - ErrApproverEmail = errors.New("approver is not a valid email") - ErrApproverNotFound = errors.New("approver not found") - ErrGrantNotFound = errors.New("grant not found") + ErrApproverKeyNotRecognized = errors.New("unrecognized approvers key") + ErrApproverInvalidType = errors.New("invalid approver type, expected an email string or array of email string") + ErrApproverEmail = errors.New("approver is not a valid email") + ErrApproverNotFound = errors.New("approver not found") + ErrGrantNotFound = errors.New("grant not found") + ErrInvalidUpdateApprovalParameter = errors.New("invalid parameter") + + ErrAppealNotEligibleForApproval = errors.New("appeal status not eligible for approval") + ErrApprovalNotEligibleForAction = errors.New("approval not eligible for action") ) type InvalidError struct { diff --git a/core/appeal/service.go b/core/appeal/service.go index 2f75d7cdf..567bf6ae7 100644 --- a/core/appeal/service.go +++ b/core/appeal/service.go @@ -439,12 +439,15 @@ func validateAppealOnBehalf(a *domain.Appeal, policy *domain.Policy) error { // UpdateApproval Approve an approval step func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.ApprovalAction) (*domain.Appeal, error) { - if err := utils.ValidateStruct(approvalAction); err != nil { - return nil, err + if err := approvalAction.Validate(); err != nil { + return nil, fmt.Errorf("%w: %v", ErrInvalidUpdateApprovalParameter, err) } appeal, err := s.GetByID(ctx, approvalAction.AppealID) if err != nil { + if errors.Is(err, ErrAppealNotFound) { + return nil, fmt.Errorf("%w: %q", ErrAppealNotFound, approvalAction.AppealID) + } return nil, err } @@ -454,16 +457,14 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr for i, approval := range appeal.Approvals { if approval.Name != approvalAction.ApprovalName { - if err := checkPreviousApprovalStatus(approval.Status); err != nil { + if err := checkPreviousApprovalStatus(approval.Status, approval.Name); err != nil { return nil, err } continue } - if approval.Status != domain.ApprovalStatusPending { - if err := checkApprovalStatus(approval.Status); err != nil { - return nil, err - } + if err := checkApprovalStatus(approval.Status); err != nil { + return nil, err } if !utils.ContainsString(approval.Approvers, approvalAction.Actor) { @@ -592,7 +593,7 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr return appeal, nil } - return nil, ErrApprovalNotFound + return nil, fmt.Errorf("%w: %q", ErrApprovalNotFound, approvalAction.ApprovalName) } func (s *Service) Update(ctx context.Context, appeal *domain.Appeal) error { @@ -904,57 +905,48 @@ func (s *Service) getApprovalNotifications(ctx context.Context, appeal *domain.A } func checkIfAppealStatusStillPending(status string) error { - if status == domain.AppealStatusPending { - return nil - } - - var err error switch status { - case domain.AppealStatusCanceled: - err = ErrAppealStatusCanceled - case domain.AppealStatusApproved: - err = ErrAppealStatusApproved - case domain.AppealStatusRejected: - err = ErrAppealStatusRejected + case domain.AppealStatusPending: + return nil + case + domain.AppealStatusCanceled, + domain.AppealStatusApproved, + domain.AppealStatusRejected: + return fmt.Errorf("%w: %q", ErrAppealNotEligibleForApproval, status) default: - err = ErrAppealStatusUnrecognized + return fmt.Errorf("%w: %q", ErrAppealStatusUnrecognized, status) } - return err } -func checkPreviousApprovalStatus(status string) error { - var err error +func checkPreviousApprovalStatus(status, name string) error { switch status { - case domain.ApprovalStatusApproved, + case + domain.ApprovalStatusApproved, domain.ApprovalStatusSkipped: - err = nil - case domain.ApprovalStatusBlocked: - err = ErrApprovalDependencyIsBlocked - case domain.ApprovalStatusPending: - err = ErrApprovalDependencyIsPending - case domain.ApprovalStatusRejected: - err = ErrAppealStatusRejected + return nil + case + domain.ApprovalStatusBlocked, + domain.ApprovalStatusPending, + domain.ApprovalStatusRejected: + return fmt.Errorf("%w: found previous approval %q with status %q", ErrApprovalNotEligibleForAction, name, status) default: - err = ErrApprovalStatusUnrecognized + return fmt.Errorf("%w: found previous approval %q with unrecognized status %q", ErrApprovalStatusUnrecognized, name, status) } - return err } func checkApprovalStatus(status string) error { - var err error switch status { - case domain.ApprovalStatusBlocked: - err = ErrAppealStatusBlocked - case domain.ApprovalStatusApproved: - err = ErrApprovalStatusApproved - case domain.ApprovalStatusRejected: - err = ErrApprovalStatusRejected - case domain.ApprovalStatusSkipped: - err = ErrApprovalStatusSkipped + case domain.ApprovalStatusPending: + return nil + case + domain.ApprovalStatusBlocked, + domain.ApprovalStatusApproved, + domain.ApprovalStatusRejected, + domain.ApprovalStatusSkipped: + return fmt.Errorf("%w: approval status %q is not actionable", ErrApprovalNotEligibleForAction, status) default: - err = ErrApprovalStatusUnrecognized + return fmt.Errorf("%w: %q", ErrApprovalStatusUnrecognized, status) } - return err } func (s *Service) handleAppealRequirements(ctx context.Context, a *domain.Appeal, p *domain.Policy) error { diff --git a/core/appeal/service_test.go b/core/appeal/service_test.go index eab643a49..2e5998aa1 100644 --- a/core/appeal/service_test.go +++ b/core/appeal/service_test.go @@ -1626,7 +1626,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { s.mockRepository.AssertExpectations(s.T()) s.Nil(actualResult) - s.EqualError(actualError, expectedError.Error()) + s.ErrorIs(actualError, expectedError) }) s.Run("should return error if appeal not found", func() { @@ -1637,7 +1637,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { s.mockRepository.AssertExpectations(s.T()) s.Nil(actualResult) - s.EqualError(actualError, appeal.ErrAppealNotFound.Error()) + s.ErrorIs(actualError, appeal.ErrAppealNotFound) }) s.Run("should return error based on statuses conditions", func() { @@ -1647,15 +1647,20 @@ func (s *ServiceTestSuite) TestUpdateApproval() { approvals []*domain.Approval expectedError error }{ + { + name: "appeal not eligible, status: canceled", + appealStatus: domain.AppealStatusCanceled, + expectedError: appeal.ErrAppealNotEligibleForApproval, + }, { name: "appeal not eligible, status: approved", appealStatus: domain.AppealStatusApproved, - expectedError: appeal.ErrAppealStatusApproved, + expectedError: appeal.ErrAppealNotEligibleForApproval, }, { name: "appeal not eligible, status: rejected", appealStatus: domain.AppealStatusRejected, - expectedError: appeal.ErrAppealStatusRejected, + expectedError: appeal.ErrAppealNotEligibleForApproval, }, { name: "invalid appeal status", @@ -1675,7 +1680,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusPending, }, }, - expectedError: appeal.ErrApprovalDependencyIsPending, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "found one previous approval is reject", @@ -1690,7 +1695,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusPending, }, }, - expectedError: appeal.ErrAppealStatusRejected, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "invalid approval status", @@ -1720,7 +1725,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusApproved, }, }, - expectedError: appeal.ErrApprovalStatusApproved, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "approval step already rejected", @@ -1735,7 +1740,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusRejected, }, }, - expectedError: appeal.ErrApprovalStatusRejected, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "approval step already skipped", @@ -1750,7 +1755,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusSkipped, }, }, - expectedError: appeal.ErrApprovalStatusSkipped, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "invalid approval status", @@ -1813,7 +1818,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { actualResult, actualError := s.service.UpdateApproval(context.Background(), validApprovalActionParam) s.Nil(actualResult) - s.EqualError(actualError, tc.expectedError.Error()) + s.ErrorIs(actualError, tc.expectedError) } }) diff --git a/core/approval/errors.go b/core/approval/errors.go index 70157d395..a7fe3e0ee 100644 --- a/core/approval/errors.go +++ b/core/approval/errors.go @@ -9,9 +9,7 @@ var ( ErrNilResourceInAppeal = errors.New("unable to resolve resource from the appeal") ErrInvalidConditionField = errors.New("invalid condition field") - ErrAppealStatusCanceled = errors.New("appeal already canceled") ErrAppealStatusApproved = errors.New("appeal already approved") - ErrAppealStatusRejected = errors.New("appeal already rejected") ErrAppealStatusBlocked = errors.New("approval is blocked") ErrAppealStatusUnrecognized = errors.New("unrecognized appeal status") ErrAppealDuplicate = errors.New("appeal with the same resource and role already exists") @@ -20,15 +18,13 @@ var ( ErrGrantNotEligibleForExtension = errors.New("existing grant is not eligible for extension") ErrCannotCreateAppealForOtherUser = errors.New("creating appeal for other individual user (account_type=\"user\") is not allowed") - ErrApprovalDependencyIsBlocked = errors.New("found previous approval step that is still in blocked") - ErrApprovalDependencyIsPending = errors.New("found previous approval step that is still in pending") - ErrApprovalStatusApproved = errors.New("approval already approved") - ErrApprovalStatusRejected = errors.New("approval already rejected") - ErrApprovalStatusSkipped = errors.New("approval already skipped") - ErrApprovalStatusUnrecognized = errors.New("unrecognized approval status") - ErrApprovalNotFound = errors.New("approval not found") - ErrUnableToAddApprover = errors.New("unable to add a new approver") - ErrUnableToDeleteApprover = errors.New("unable to remove approver") + ErrApprovalStatusApproved = errors.New("approval already approved") + ErrApprovalStatusRejected = errors.New("approval already rejected") + ErrApprovalStatusSkipped = errors.New("approval already skipped") + ErrApprovalStatusUnrecognized = errors.New("unrecognized approval status") + ErrApprovalNotFound = errors.New("approval not found") + ErrUnableToAddApprover = errors.New("unable to add a new approver") + ErrUnableToDeleteApprover = errors.New("unable to remove approver") ErrActionForbidden = errors.New("user is not allowed to make action on this approval step") ErrActionInvalidValue = errors.New("invalid action value") diff --git a/domain/appeal.go b/domain/appeal.go index dc3893be9..86911bd49 100644 --- a/domain/appeal.go +++ b/domain/appeal.go @@ -6,6 +6,7 @@ import ( "reflect" "time" + "github.com/go-playground/validator/v10" "github.com/goto/guardian/pkg/evaluator" "github.com/goto/guardian/utils" ) @@ -279,6 +280,22 @@ type ApprovalAction struct { Reason string `json:"reason"` } +func (a ApprovalAction) Validate() error { + if a.AppealID == "" { + return fmt.Errorf("appeal id is required") + } + if a.ApprovalName == "" { + return fmt.Errorf("approval name is required") + } + if validator.New().Var(a.Actor, "email") != nil { + return fmt.Errorf("actor is not a valid email: %q", a.Actor) + } + if a.Action != string(ApprovalActionApprove) && a.Action != string(ApprovalActionReject) { + return fmt.Errorf("invalid action: %q", a.Action) + } + return nil +} + type ListAppealsFilter struct { Q string `mapstructure:"q" validate:"omitempty"` AccountTypes []string `mapstructure:"account_types" validate:"omitempty,min=1"`