From 16b3000d04ab58876964a5f4321dd95856994f66 Mon Sep 17 00:00:00 2001 From: Rahmat Hidayat Date: Wed, 6 Dec 2023 10:02:28 +0700 Subject: [PATCH] fix: fix error to status code mapping (#110) * fix: fix error to status code mapping for CreateAppeals * chore: logs internal error in handler * fix: fix error to status code mapping for UpdateApproval * chore: remove unused errors * test: add unit tests for ApprovalAction validation * chore: fix typo in annotation * fix: log internal error in ListUserRoles * feat: log error message for invalid argument and failed preconditions errors --- api/handler/v1beta1/activity.go | 12 +-- api/handler/v1beta1/appeal.go | 56 +++++++--- api/handler/v1beta1/approval.go | 54 +++++----- api/handler/v1beta1/approval_test.go | 39 ++----- api/handler/v1beta1/grant.go | 28 ++--- api/handler/v1beta1/grpc.go | 16 +++ api/handler/v1beta1/policy.go | 18 ++-- api/handler/v1beta1/provider.go | 24 ++--- api/handler/v1beta1/resource.go | 16 +-- core/appeal/errors.go | 46 ++++---- core/appeal/service.go | 150 ++++++++++++--------------- core/appeal/service_test.go | 81 ++++++++++----- core/approval/errors.go | 35 ------- core/provider/errors.go | 24 +++-- core/provider/service.go | 21 ++-- core/provider/service_test.go | 65 +++--------- domain/appeal.go | 22 +++- domain/appeal_test.go | 66 ++++++++++++ domain/policy.go | 23 ++-- internal/server/server.go | 5 +- 20 files changed, 428 insertions(+), 373 deletions(-) delete mode 100644 core/approval/errors.go diff --git a/api/handler/v1beta1/activity.go b/api/handler/v1beta1/activity.go index cd384c081..77364f85a 100644 --- a/api/handler/v1beta1/activity.go +++ b/api/handler/v1beta1/activity.go @@ -17,12 +17,12 @@ func (s *GRPCServer) GetActivity(ctx context.Context, req *guardianv1beta1.GetAc if errors.Is(err, activity.ErrNotFound) { return nil, status.Errorf(codes.NotFound, "activity not found") } - return nil, status.Errorf(codes.Internal, "failed to get activity: %v", err) + return nil, s.internalError(ctx, "failed to get activity: %v", err) } activityProto, err := s.adapter.ToActivityProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err) + return nil, s.internalError(ctx, "failed to parse proto: %v", err) } return &guardianv1beta1.GetActivityResponse{ @@ -48,14 +48,14 @@ func (s *GRPCServer) ListActivities(ctx context.Context, req *guardianv1beta1.Li activities, err := s.activityService.Find(ctx, filter) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to list activities: %v", err) + return nil, s.internalError(ctx, "failed to list activities: %v", err) } activityProtos := []*guardianv1beta1.ProviderActivity{} for _, a := range activities { activityProto, err := s.adapter.ToActivityProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err) + return nil, s.internalError(ctx, "failed to parse proto: %v", err) } activityProtos = append(activityProtos, activityProto) } @@ -82,14 +82,14 @@ func (s *GRPCServer) ImportActivities(ctx context.Context, req *guardianv1beta1. activities, err := s.activityService.Import(ctx, filter) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to import activities: %v", err) + return nil, s.internalError(ctx, "failed to import activities: %v", err) } activityProtos := []*guardianv1beta1.ProviderActivity{} for _, a := range activities { activity, err := s.adapter.ToActivityProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err) + return nil, s.internalError(ctx, "failed to parse proto: %v", err) } activityProtos = append(activityProtos, activity) diff --git a/api/handler/v1beta1/appeal.go b/api/handler/v1beta1/appeal.go index b96f5513f..cb54d9c89 100644 --- a/api/handler/v1beta1/appeal.go +++ b/api/handler/v1beta1/appeal.go @@ -6,6 +6,7 @@ import ( guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1" "github.com/goto/guardian/core/appeal" + "github.com/goto/guardian/core/provider" "github.com/goto/guardian/domain" "golang.org/x/sync/errgroup" "google.golang.org/grpc/codes" @@ -96,21 +97,46 @@ func (s *GRPCServer) CreateAppeal(ctx context.Context, req *guardianv1beta1.Crea appeals, err := s.adapter.FromCreateAppealProto(req, authenticatedUser) if err != nil { - return nil, status.Errorf(codes.Internal, "cannot deserialize payload: %v", err) + return nil, s.internalError(ctx, "cannot deserialize payload: %v", err) } if err := s.appealService.Create(ctx, appeals); err != nil { - if errors.Is(err, appeal.ErrAppealDuplicate) { - return nil, status.Errorf(codes.AlreadyExists, "appeal already exists: %v", err) + switch { + case errors.Is(err, provider.ErrAppealValidationInvalidAccountType), + errors.Is(err, provider.ErrAppealValidationInvalidRole), + errors.Is(err, provider.ErrAppealValidationDurationNotSpecified), + errors.Is(err, provider.ErrAppealValidationEmptyDuration), + errors.Is(err, provider.ErrAppealValidationInvalidDurationValue), + errors.Is(err, provider.ErrAppealValidationMissingRequiredParameter), + errors.Is(err, provider.ErrAppealValidationMissingRequiredQuestion), + errors.Is(err, appeal.ErrDurationNotAllowed), + errors.Is(err, appeal.ErrCannotCreateAppealForOtherUser): + return nil, s.invalidArgument(ctx, err.Error()) + case errors.Is(err, appeal.ErrAppealDuplicate): + s.logger.Error(ctx, err.Error()) + return nil, status.Errorf(codes.AlreadyExists, err.Error()) + case errors.Is(err, appeal.ErrResourceNotFound), + errors.Is(err, appeal.ErrResourceDeleted), + errors.Is(err, appeal.ErrProviderNotFound), + errors.Is(err, appeal.ErrPolicyNotFound), + errors.Is(err, appeal.ErrInvalidResourceType), + errors.Is(err, appeal.ErrAppealInvalidExtensionDuration), + errors.Is(err, appeal.ErrGrantNotEligibleForExtension), + errors.Is(err, domain.ErrFailedToGetApprovers), + errors.Is(err, domain.ErrApproversNotFound), + errors.Is(err, domain.ErrUnexpectedApproverType), + errors.Is(err, domain.ErrInvalidApproverValue): + return nil, s.failedPrecondition(ctx, err.Error()) + default: + return nil, s.internalError(ctx, "failed to create appeal(s): %v", err) } - return nil, status.Errorf(codes.Internal, "failed to create appeal: %v", err) } appealProtos := []*guardianv1beta1.Appeal{} for _, appeal := range appeals { appealProto, err := s.adapter.ToAppealProto(appeal) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err) + return nil, s.internalError(ctx, "failed to parse appeal: %v", err) } appealProtos = append(appealProtos, appealProto) } @@ -126,9 +152,9 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe a, err := s.appealService.GetByID(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()) + return nil, s.invalidArgument(ctx, err.Error()) } - return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err) + return nil, s.internalError(ctx, "failed to retrieve appeal: %v", err) } if a == nil { @@ -137,7 +163,7 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err) + return nil, s.internalError(ctx, "failed to parse appeal: %v", err) } return &guardianv1beta1.GetAppealResponse{ @@ -151,7 +177,7 @@ 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()) + return nil, s.invalidArgument(ctx, err.Error()) } switch err { @@ -161,15 +187,15 @@ func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.Canc appeal.ErrAppealStatusApproved, appeal.ErrAppealStatusRejected, appeal.ErrAppealStatusUnrecognized: - return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err) + return nil, s.invalidArgument(ctx, "unable to process the request: %v", err) default: - return nil, status.Errorf(codes.Internal, "failed to cancel appeal: %v", err) + return nil, s.internalError(ctx, "failed to cancel appeal: %v", err) } } appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err) + return nil, s.internalError(ctx, "failed to parse appeal: %v", err) } return &guardianv1beta1.CancelAppealResponse{ @@ -185,7 +211,7 @@ func (s *GRPCServer) listAppeals(ctx context.Context, filters *domain.ListAppeal eg.Go(func() error { appealRecords, err := s.appealService.Find(ctx, filters) if err != nil { - return status.Errorf(codes.Internal, "failed to get appeal list: %s", err) + return s.internalError(ctx, "failed to get appeal list: %s", err) } appeals = appealRecords return nil @@ -193,7 +219,7 @@ func (s *GRPCServer) listAppeals(ctx context.Context, filters *domain.ListAppeal eg.Go(func() error { totalRecord, err := s.appealService.GetAppealsTotalCount(ctx, filters) if err != nil { - return status.Errorf(codes.Internal, "failed to get appeal total count: %s", err) + return s.internalError(ctx, "failed to get appeal total count: %s", err) } total = totalRecord return nil @@ -207,7 +233,7 @@ func (s *GRPCServer) listAppeals(ctx context.Context, filters *domain.ListAppeal for _, a := range appeals { appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, 0, status.Errorf(codes.Internal, "failed to parse appeal: %s", err) + return nil, 0, s.internalError(ctx, "failed to parse appeal: %s", err) } appealProtos = append(appealProtos, appealProto) } diff --git a/api/handler/v1beta1/approval.go b/api/handler/v1beta1/approval.go index f1c39ecb7..00fab6f1a 100644 --- a/api/handler/v1beta1/approval.go +++ b/api/handler/v1beta1/approval.go @@ -78,30 +78,32 @@ 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, s.invalidArgument(ctx, 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, s.failedPrecondition(ctx, 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, status.Errorf(codes.Internal, "failed to update approval: %v", err) + return nil, s.internalError(ctx, "failed to update approval: %v", err) } } appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err) + return nil, s.internalError(ctx, "failed to parse appeal: %v", err) } return &guardianv1beta1.UpdateApprovalResponse{ @@ -116,17 +118,17 @@ func (s *GRPCServer) AddApprover(ctx context.Context, req *guardianv1beta1.AddAp errors.Is(err, appeal.ErrApprovalIDEmptyParam), errors.Is(err, appeal.ErrApproverEmail), errors.Is(err, appeal.ErrUnableToAddApprover): - return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %s", err) + return nil, s.invalidArgument(ctx, "unable to process the request: %s", err) case errors.Is(err, appeal.ErrAppealNotFound), errors.Is(err, appeal.ErrApprovalNotFound): return nil, status.Errorf(codes.NotFound, "resource not found: %s", err) case err != nil: - return nil, status.Errorf(codes.Internal, "failed to add approver: %s", err) + return nil, s.internalError(ctx, "failed to add approver: %s", err) } appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %s", err) + return nil, s.internalError(ctx, "failed to parse appeal: %s", err) } return &guardianv1beta1.AddApproverResponse{ @@ -141,17 +143,17 @@ func (s *GRPCServer) DeleteApprover(ctx context.Context, req *guardianv1beta1.De errors.Is(err, appeal.ErrApprovalIDEmptyParam), errors.Is(err, appeal.ErrApproverEmail), errors.Is(err, appeal.ErrUnableToDeleteApprover): - return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %s", err) + return nil, s.invalidArgument(ctx, "unable to process the request: %s", err) case errors.Is(err, appeal.ErrAppealNotFound), errors.Is(err, appeal.ErrApprovalNotFound): return nil, status.Errorf(codes.NotFound, "resource not found: %s", err) case err != nil: - return nil, status.Errorf(codes.Internal, "failed to delete approver: %s", err) + return nil, s.internalError(ctx, "failed to delete approver: %s", err) } appealProto, err := s.adapter.ToAppealProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal: %s", err) + return nil, s.internalError(ctx, "failed to parse appeal: %s", err) } return &guardianv1beta1.DeleteApproverResponse{ @@ -167,7 +169,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr eg.Go(func() error { approvalRecords, err := s.approvalService.ListApprovals(ctx, filters) if err != nil { - return status.Errorf(codes.Internal, "failed to get approval list: %s", err) + return s.internalError(ctx, "failed to get approval list: %s", err) } approvals = approvalRecords return nil @@ -176,7 +178,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr eg.Go(func() error { totalRecord, err := s.approvalService.GetApprovalsTotalCount(ctx, filters) if err != nil { - return status.Errorf(codes.Internal, "failed to get approval list: %v", err) + return s.internalError(ctx, "failed to get approval list: %v", err) } total = totalRecord return nil @@ -190,7 +192,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr for _, a := range approvals { approvalProto, err := s.adapter.ToApprovalProto(a) if err != nil { - return nil, 0, status.Errorf(codes.Internal, "failed to parse approval: %v: %s", a.ID, err) + return nil, 0, s.internalError(ctx, "failed to parse approval: %v: %s", a.ID, err) } approvalProtos = append(approvalProtos, approvalProto) } 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/api/handler/v1beta1/grant.go b/api/handler/v1beta1/grant.go index 778c4a0fc..46c650f10 100644 --- a/api/handler/v1beta1/grant.go +++ b/api/handler/v1beta1/grant.go @@ -82,12 +82,12 @@ func (s *GRPCServer) GetGrant(ctx context.Context, req *guardianv1beta1.GetGrant if errors.Is(err, grant.ErrGrantNotFound) { return nil, status.Errorf(codes.NotFound, "grant %q not found: %v", req.GetId(), err) } - return nil, status.Errorf(codes.Internal, "failed to get grant details: %v", err) + return nil, s.internalError(ctx, "failed to get grant details: %v", err) } grantProto, err := s.adapter.ToGrantProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse grant: %v", err) + return nil, s.internalError(ctx, "failed to parse grant: %v", err) } return &guardianv1beta1.GetGrantResponse{ @@ -106,12 +106,12 @@ func (s *GRPCServer) RevokeGrant(ctx context.Context, req *guardianv1beta1.Revok if errors.Is(err, grant.ErrGrantNotFound) { return nil, status.Error(codes.NotFound, "grant not found") } - return nil, status.Errorf(codes.Internal, "failed to revoke grant: %v", err) + return nil, s.internalError(ctx, "failed to revoke grant: %v", err) } grantProto, err := s.adapter.ToGrantProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse grant: %v", err) + return nil, s.internalError(ctx, "failed to parse grant: %v", err) } return &guardianv1beta1.RevokeGrantResponse{ @@ -131,13 +131,13 @@ func (s *GRPCServer) UpdateGrant(ctx context.Context, req *guardianv1beta1.Updat case errors.Is(err, grant.ErrEmptyOwner): return nil, status.Error(codes.InvalidArgument, err.Error()) default: - return nil, status.Errorf(codes.Internal, "failed to update grant: %v", err) + return nil, s.internalError(ctx, "failed to update grant: %v", err) } } grantProto, err := s.adapter.ToGrantProto(g) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse grant: %v", err) + return nil, s.internalError(ctx, "failed to parse grant: %v", err) } return &guardianv1beta1.UpdateGrantResponse{ @@ -160,14 +160,14 @@ func (s *GRPCServer) RevokeGrants(ctx context.Context, req *guardianv1beta1.Revo } grants, err := s.grantService.BulkRevoke(ctx, filter, actor, req.GetReason()) if err != nil { - return nil, status.Error(codes.Internal, "failed to revoke grants in bulk") + return nil, s.internalError(ctx, "failed to revoke grants in bulk") } var grantsProto []*guardianv1beta1.Grant for _, a := range grants { grantProto, err := s.adapter.ToGrantProto(a) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse grant: %v", err) + return nil, s.internalError(ctx, "failed to parse grant: %v", err) } grantsProto = append(grantsProto, grantProto) } @@ -185,7 +185,7 @@ func (s *GRPCServer) listGrants(ctx context.Context, filter domain.ListGrantsFil eg.Go(func() error { grantRecords, err := s.grantService.List(ctx, filter) if err != nil { - return status.Errorf(codes.Internal, "failed to get grant list: %s", err) + return s.internalError(ctx, "failed to get grant list: %s", err) } grants = grantRecords return nil @@ -193,7 +193,7 @@ func (s *GRPCServer) listGrants(ctx context.Context, filter domain.ListGrantsFil eg.Go(func() error { totalRecord, err := s.grantService.GetGrantsTotalCount(ctx, filter) if err != nil { - return status.Errorf(codes.Internal, "failed to get grant total count: %s", err) + return s.internalError(ctx, "failed to get grant total count: %s", err) } total = totalRecord return nil @@ -207,7 +207,7 @@ func (s *GRPCServer) listGrants(ctx context.Context, filter domain.ListGrantsFil for i, a := range grants { grantProto, err := s.adapter.ToGrantProto(&grants[i]) if err != nil { - return nil, 0, status.Errorf(codes.Internal, "failed to parse grant %q: %v", a.ID, err) + return nil, 0, s.internalError(ctx, "failed to parse grant %q: %v", a.ID, err) } grantProtos = append(grantProtos, grantProto) } @@ -230,14 +230,14 @@ func (s *GRPCServer) ImportGrantsFromProvider(ctx context.Context, req *guardian return nil, status.Error(codes.InvalidArgument, err.Error()) } - return nil, status.Errorf(codes.Internal, "failed to import access: %v", err) + return nil, s.internalError(ctx, "failed to import access: %v", err) } grantsProto := []*guardianv1beta1.Grant{} for _, g := range grants { grantProto, err := s.adapter.ToGrantProto(g) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse appeal proto %q: %v", g.ID, err) + return nil, s.internalError(ctx, "failed to parse appeal proto %q: %v", g.ID, err) } grantsProto = append(grantsProto, grantProto) } @@ -255,7 +255,7 @@ func (s *GRPCServer) ListUserRoles(ctx context.Context, req *guardianv1beta1.Lis roles, err := s.grantService.ListUserRoles(ctx, user) if err != nil { - return nil, status.Error(codes.Internal, "Internal Error") + return nil, s.internalError(ctx, "Internal Error: %s", err) } return &guardianv1beta1.ListUserRolesResponse{ Roles: roles, diff --git a/api/handler/v1beta1/grpc.go b/api/handler/v1beta1/grpc.go index fe5063488..a8d29e392 100644 --- a/api/handler/v1beta1/grpc.go +++ b/api/handler/v1beta1/grpc.go @@ -2,6 +2,7 @@ package v1beta1 import ( "context" + "fmt" "strings" "github.com/goto/guardian/pkg/log" @@ -169,3 +170,18 @@ func (s *GRPCServer) getUser(ctx context.Context) (string, error) { return authenticatedEmail, nil } + +func (s *GRPCServer) invalidArgument(ctx context.Context, format string, a ...interface{}) error { + s.logger.Error(ctx, fmt.Sprintf(format, a...)) + return status.Errorf(codes.InvalidArgument, format, a...) +} + +func (s *GRPCServer) failedPrecondition(ctx context.Context, format string, a ...interface{}) error { + s.logger.Error(ctx, fmt.Sprintf(format, a...)) + return status.Errorf(codes.FailedPrecondition, format, a...) +} + +func (s *GRPCServer) internalError(ctx context.Context, format string, a ...interface{}) error { + s.logger.Error(ctx, fmt.Sprintf(format, a...)) + return status.Errorf(codes.Internal, format, a...) +} diff --git a/api/handler/v1beta1/policy.go b/api/handler/v1beta1/policy.go index 28d0173c6..b7b77c30b 100644 --- a/api/handler/v1beta1/policy.go +++ b/api/handler/v1beta1/policy.go @@ -13,14 +13,14 @@ import ( func (s *GRPCServer) ListPolicies(ctx context.Context, req *guardianv1beta1.ListPoliciesRequest) (*guardianv1beta1.ListPoliciesResponse, error) { policies, err := s.policyService.Find(ctx) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to get policy list: %v", err) + return nil, s.internalError(ctx, "failed to get policy list: %v", err) } policyProtos := []*guardianv1beta1.Policy{} for _, p := range policies { policyProto, err := s.adapter.ToPolicyProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse policy %v: %v", p.ID, err) + return nil, s.internalError(ctx, "failed to parse policy %v: %v", p.ID, err) } policyProtos = append(policyProtos, policyProto) } @@ -37,13 +37,13 @@ func (s *GRPCServer) GetPolicy(ctx context.Context, req *guardianv1beta1.GetPoli case policy.ErrPolicyNotFound: return nil, status.Error(codes.NotFound, "policy not found") default: - return nil, status.Errorf(codes.Internal, "failed to retrieve policy: %v", err) + return nil, s.internalError(ctx, "failed to retrieve policy: %v", err) } } policyProto, err := s.adapter.ToPolicyProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse policy: %v", err) + return nil, s.internalError(ctx, "failed to parse policy: %v", err) } return &guardianv1beta1.GetPolicyResponse{ @@ -59,12 +59,12 @@ func (s *GRPCServer) CreatePolicy(ctx context.Context, req *guardianv1beta1.Crea p := s.adapter.FromPolicyProto(req.GetPolicy()) if err := s.policyService.Create(ctx, p); err != nil { - return nil, status.Errorf(codes.Internal, "failed to create policy: %v", err) + return nil, s.internalError(ctx, "failed to create policy: %v", err) } policyProto, err := s.adapter.ToPolicyProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse policy: %v", err) + return nil, s.internalError(ctx, "failed to parse policy: %v", err) } return &guardianv1beta1.CreatePolicyResponse{ @@ -86,12 +86,12 @@ func (s *GRPCServer) UpdatePolicy(ctx context.Context, req *guardianv1beta1.Upda return nil, status.Error(codes.InvalidArgument, err.Error()) } - return nil, status.Errorf(codes.Internal, "failed to update policy: %v", err) + return nil, s.internalError(ctx, "failed to update policy: %v", err) } policyProto, err := s.adapter.ToPolicyProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse policy: %v", err) + return nil, s.internalError(ctx, "failed to parse policy: %v", err) } return &guardianv1beta1.UpdatePolicyResponse{ @@ -106,7 +106,7 @@ func (s *GRPCServer) GetPolicyPreferences(ctx context.Context, req *guardianv1be case policy.ErrPolicyNotFound: return nil, status.Error(codes.NotFound, "policy not found") default: - return nil, status.Errorf(codes.Internal, "failed to retrieve policy: %v", err) + return nil, s.internalError(ctx, "failed to retrieve policy: %v", err) } } diff --git a/api/handler/v1beta1/provider.go b/api/handler/v1beta1/provider.go index 982f5bfe8..267d301d9 100644 --- a/api/handler/v1beta1/provider.go +++ b/api/handler/v1beta1/provider.go @@ -14,7 +14,7 @@ import ( func (s *GRPCServer) ListProviders(ctx context.Context, req *guardianv1beta1.ListProvidersRequest) (*guardianv1beta1.ListProvidersResponse, error) { providers, err := s.providerService.Find(ctx) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to list providers: %v", err) + return nil, s.internalError(ctx, "failed to list providers: %v", err) } providerProtos := []*guardianv1beta1.Provider{} @@ -22,7 +22,7 @@ func (s *GRPCServer) ListProviders(ctx context.Context, req *guardianv1beta1.Lis p.Config.Credentials = nil providerProto, err := s.adapter.ToProviderProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse provider %s: %v", p.URN, err) + return nil, s.internalError(ctx, "failed to parse provider %s: %v", p.URN, err) } providerProtos = append(providerProtos, providerProto) } @@ -39,13 +39,13 @@ func (s *GRPCServer) GetProvider(ctx context.Context, req *guardianv1beta1.GetPr case provider.ErrRecordNotFound: return nil, status.Error(codes.NotFound, "provider not found") default: - return nil, status.Errorf(codes.Internal, "failed to retrieve provider: %v", err) + return nil, s.internalError(ctx, "failed to retrieve provider: %v", err) } } providerProto, err := s.adapter.ToProviderProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse provider %s: %v", p.URN, err) + return nil, s.internalError(ctx, "failed to parse provider %s: %v", p.URN, err) } return &guardianv1beta1.GetProviderResponse{ @@ -56,7 +56,7 @@ func (s *GRPCServer) GetProvider(ctx context.Context, req *guardianv1beta1.GetPr func (s *GRPCServer) GetProviderTypes(ctx context.Context, req *guardianv1beta1.GetProviderTypesRequest) (*guardianv1beta1.GetProviderTypesResponse, error) { providerTypes, err := s.providerService.GetTypes(ctx) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to retrieve provider types: %v", err) + return nil, s.internalError(ctx, "failed to retrieve provider types: %v", err) } var providerTypeProtos []*guardianv1beta1.ProviderType @@ -83,12 +83,12 @@ func (s *GRPCServer) CreateProvider(ctx context.Context, req *guardianv1beta1.Cr if err := s.providerService.Create(ctx, p); err != nil { s.logger.Error(ctx, "failed to create provider", "provider_urn", p.URN, "type", p.Type, "error", err) - return nil, status.Errorf(codes.Internal, "failed to create provider: %v", err) + return nil, s.internalError(ctx, "failed to create provider: %v", err) } providerProto, err := s.adapter.ToProviderProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse provider: %v", err) + return nil, s.internalError(ctx, "failed to parse provider: %v", err) } return &guardianv1beta1.CreateProviderResponse{ @@ -112,12 +112,12 @@ func (s *GRPCServer) UpdateProvider(ctx context.Context, req *guardianv1beta1.Up if err := s.providerService.Update(ctx, p); err != nil { s.logger.Error(ctx, "failed to update provider", "provider_id", id, "provider_urn", p.URN, "type", p.Type, "error", err) - return nil, status.Errorf(codes.Internal, "failed to update provider: %v", err) + return nil, s.internalError(ctx, "failed to update provider: %v", err) } providerProto, err := s.adapter.ToProviderProto(p) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse provider: %v", err) + return nil, s.internalError(ctx, "failed to parse provider: %v", err) } return &guardianv1beta1.UpdateProviderResponse{ @@ -130,7 +130,7 @@ func (s *GRPCServer) DeleteProvider(ctx context.Context, req *guardianv1beta1.De if errors.Is(err, provider.ErrRecordNotFound) { return nil, status.Errorf(codes.NotFound, "provider not found") } - return nil, status.Errorf(codes.Internal, "failed to delete provider: %v", err) + return nil, s.internalError(ctx, "failed to delete provider: %v", err) } return &guardianv1beta1.DeleteProviderResponse{}, nil @@ -139,14 +139,14 @@ func (s *GRPCServer) DeleteProvider(ctx context.Context, req *guardianv1beta1.De func (s *GRPCServer) ListRoles(ctx context.Context, req *guardianv1beta1.ListRolesRequest) (*guardianv1beta1.ListRolesResponse, error) { roles, err := s.providerService.GetRoles(ctx, req.GetId(), req.GetResourceType()) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to list roles: %v", err) + return nil, s.internalError(ctx, "failed to list roles: %v", err) } roleProtos := []*guardianv1beta1.Role{} for _, r := range roles { role, err := s.adapter.ToRole(r) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err) + return nil, s.internalError(ctx, "failed to parse proto: %v", err) } roleProtos = append(roleProtos, role) diff --git a/api/handler/v1beta1/resource.go b/api/handler/v1beta1/resource.go index e6e536eca..3169c80d9 100644 --- a/api/handler/v1beta1/resource.go +++ b/api/handler/v1beta1/resource.go @@ -59,7 +59,7 @@ func (s *GRPCServer) listResources(ctx context.Context, filter domain.ListResour eg.Go(func() error { resourceRecords, err := s.resourceService.Find(ctx, filter) if err != nil { - return status.Errorf(codes.Internal, "failed to get resource list: %s", err) + return s.internalError(ctx, "failed to get resource list: %s", err) } resources = resourceRecords return nil @@ -67,7 +67,7 @@ func (s *GRPCServer) listResources(ctx context.Context, filter domain.ListResour eg.Go(func() error { totalRecord, err := s.resourceService.GetResourcesTotalCount(ctx, filter) if err != nil { - return status.Errorf(codes.Internal, "failed to get resource total count: %s", err) + return s.internalError(ctx, "failed to get resource total count: %s", err) } total = totalRecord return nil @@ -80,7 +80,7 @@ func (s *GRPCServer) listResources(ctx context.Context, filter domain.ListResour for i, r := range resources { resourceProto, err := s.adapter.ToResourceProto(resources[i]) if err != nil { - return nil, 0, status.Errorf(codes.Internal, "failed to parse resource %v: %v", r.Name, err) + return nil, 0, s.internalError(ctx, "failed to parse resource %v: %v", r.Name, err) } resourceProtos = append(resourceProtos, resourceProto) } @@ -95,13 +95,13 @@ func (s *GRPCServer) GetResource(ctx context.Context, req *guardianv1beta1.GetRe case resource.ErrRecordNotFound: return nil, status.Error(codes.NotFound, "resource not found") default: - return nil, status.Errorf(codes.Internal, "failed to retrieve resource: %v", err) + return nil, s.internalError(ctx, "failed to retrieve resource: %v", err) } } resourceProto, err := s.adapter.ToResourceProto(r) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse resource: %v", err) + return nil, s.internalError(ctx, "failed to parse resource: %v", err) } return &guardianv1beta1.GetResourceResponse{ @@ -122,12 +122,12 @@ func (s *GRPCServer) UpdateResource(ctx context.Context, req *guardianv1beta1.Up if errors.Is(err, resource.ErrRecordNotFound) { return nil, status.Error(codes.NotFound, "resource not found") } - return nil, status.Errorf(codes.Internal, "failed to update resource: %v", err) + return nil, s.internalError(ctx, "failed to update resource: %v", err) } resourceProto, err := s.adapter.ToResourceProto(r) if err != nil { - return nil, status.Errorf(codes.Internal, "failed to parse resource: %v", err) + return nil, s.internalError(ctx, "failed to parse resource: %v", err) } return &guardianv1beta1.UpdateResourceResponse{ @@ -140,7 +140,7 @@ func (s *GRPCServer) DeleteResource(ctx context.Context, req *guardianv1beta1.De if errors.Is(err, resource.ErrRecordNotFound) { return nil, status.Errorf(codes.NotFound, "resource not found") } - return nil, status.Errorf(codes.Internal, "failed to update resource: %v", err) + return nil, s.internalError(ctx, "failed to update resource: %v", err) } return &guardianv1beta1.DeleteResourceResponse{}, nil diff --git a/core/appeal/errors.go b/core/appeal/errors.go index e8d76b5fe..dd96ef2e0 100644 --- a/core/appeal/errors.go +++ b/core/appeal/errors.go @@ -12,46 +12,42 @@ var ( 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") - ErrAppealInvalidExtensionDuration = errors.New("invalid appeal extension duration") + ErrAppealDuplicate = errors.New("appeal with identical account_id, resource, and role already exists") + ErrAppealInvalidExtensionDuration = errors.New("invalid configured appeal extension duration") ErrAppealFoundActiveGrant = errors.New("user still have an active grant") - ErrGrantNotEligibleForExtension = errors.New("existing grant is not eligible for extension") + 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") + 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") - ErrProviderTypeNotFound = errors.New("provider is not registered") - ErrProviderURNNotFound = errors.New("provider with specified urn is not registered") - ErrResourceTypeNotFound = errors.New("unable to find matching resource config for specified resource type") + ErrProviderNotFound = errors.New("provider not found") + ErrInvalidResourceType = errors.New("invalid resource type") ErrOptionsExpirationDateOptionNotFound = errors.New("expiration date is required, unable to find expiration date option") ErrInvalidRole = errors.New("invalid role") ErrExpirationDateIsRequired = errors.New("having permanent access to this resource is not allowed, access duration is required") - ErrPolicyIDNotFound = errors.New("unable to find approval policy for specified id") - ErrPolicyVersionNotFound = errors.New("unable to find approval policy for specified version") + ErrPolicyNotFound = errors.New("policy not found") ErrResourceNotFound = errors.New("resource not found") + ErrResourceDeleted = errors.New("resource has been deleted") ErrAppealNotFound = errors.New("appeal not found") - ErrResourceIsDeleted = errors.New("resource is deleted") - ErrOptionsDurationNotFound = errors.New("duration option not found") + 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 04b5413dd..4209e637b 100644 --- a/core/appeal/service.go +++ b/core/appeal/service.go @@ -216,11 +216,11 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ... return err } if err := addResource(appeal, resources); err != nil { - return fmt.Errorf("retrieving resource details for %s: %w", appeal.ResourceID, err) + return fmt.Errorf("couldn't find resource with id %q: %w", appeal.ResourceID, err) } provider, err := getProvider(appeal, providers) if err != nil { - return fmt.Errorf("retrieving provider: %w", err) + return err } var policy *domain.Policy @@ -229,7 +229,7 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ... } else { policy, err = getPolicy(appeal, provider, policies) if err != nil { - return fmt.Errorf("retrieving policy: %w", err) + return err } } @@ -240,12 +240,12 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ... if activeGrant != nil { if err := s.checkExtensionEligibility(appeal, provider, policy, activeGrant); err != nil { - return fmt.Errorf("checking grant extension eligibility: %w", err) + return err } } if err := s.providerService.ValidateAppeal(ctx, appeal, provider, policy); err != nil { - return fmt.Errorf("validating appeal based on provider: %w", err) + return fmt.Errorf("provider validation: %w", err) } strPermissions, err := s.getPermissions(ctx, provider.Config, appeal.Resource.Type, appeal.Role) @@ -255,23 +255,23 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ... appeal.Permissions = strPermissions if err := validateAppealDurationConfig(appeal, policy); err != nil { - return fmt.Errorf("validating appeal duration: %w", err) + return err } if err := validateAppealOnBehalf(appeal, policy); err != nil { - return fmt.Errorf("validating cross-individual appeal: %w", err) + return err } if err := s.addCreatorDetails(ctx, appeal, policy); err != nil { - return fmt.Errorf("retrieving creator details: %w", err) + return fmt.Errorf("getting creator details: %w", err) } if err := appeal.ApplyPolicy(policy); err != nil { - return fmt.Errorf("populating approvals: %w", err) + return err } if err := appeal.AdvanceApproval(policy); err != nil { - return fmt.Errorf("initializing approval step statuses: %w", err) + return fmt.Errorf("initializing approvals: %w", err) } appeal.Policy = nil @@ -422,7 +422,7 @@ func validateAppealDurationConfig(appeal *domain.Appeal, policy *domain.Policy) } } - return fmt.Errorf("%w: %s", ErrOptionsDurationNotFound, appeal.Options.Duration) + return fmt.Errorf("invalid duration: %w: %q", ErrDurationNotAllowed, appeal.Options.Duration) } func validateAppealOnBehalf(a *domain.Appeal, policy *domain.Policy) error { @@ -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 { @@ -1040,31 +1032,31 @@ func (s *Service) GrantAccessToProvider(ctx context.Context, a *domain.Appeal, o } func (s *Service) checkExtensionEligibility(a *domain.Appeal, p *domain.Provider, policy *domain.Policy, activeGrant *domain.Grant) error { - AllowActiveAccessExtensionIn := "" + allowActiveAccessExtensionIn := "" // Default to use provider config if policy config is not set if p.Config.Appeal != nil { - AllowActiveAccessExtensionIn = p.Config.Appeal.AllowActiveAccessExtensionIn + allowActiveAccessExtensionIn = p.Config.Appeal.AllowActiveAccessExtensionIn } // Use policy config if set if policy != nil && policy.AppealConfig != nil && policy.AppealConfig.AllowActiveAccessExtensionIn != "" { - AllowActiveAccessExtensionIn = policy.AppealConfig.AllowActiveAccessExtensionIn + allowActiveAccessExtensionIn = policy.AppealConfig.AllowActiveAccessExtensionIn } - if AllowActiveAccessExtensionIn == "" { + if allowActiveAccessExtensionIn == "" { return ErrAppealFoundActiveGrant } - extensionDurationRule, err := time.ParseDuration(AllowActiveAccessExtensionIn) + extensionDurationRule, err := time.ParseDuration(allowActiveAccessExtensionIn) if err != nil { - return fmt.Errorf("%w: %v: %v", ErrAppealInvalidExtensionDuration, AllowActiveAccessExtensionIn, err) + return fmt.Errorf("%w: %q: %v", ErrAppealInvalidExtensionDuration, allowActiveAccessExtensionIn, err) } if !activeGrant.IsEligibleForExtension(extensionDurationRule) { - return ErrGrantNotEligibleForExtension + return fmt.Errorf("%w: extension is allowed %q before grant expiration", ErrGrantNotEligibleForExtension, allowActiveAccessExtensionIn) } return nil } @@ -1078,17 +1070,15 @@ func getPolicy(a *domain.Appeal, p *domain.Provider, policiesMap map[string]map[ } } if resourceConfig == nil { - return nil, ErrResourceTypeNotFound + return nil, fmt.Errorf("%w: couldn't find %q resource type in the provider config", ErrInvalidResourceType, a.Resource.Type) } - policyConfig := resourceConfig.Policy - if policiesMap[policyConfig.ID] == nil { - return nil, ErrPolicyIDNotFound - } else if policiesMap[policyConfig.ID][uint(policyConfig.Version)] == nil { - return nil, ErrPolicyVersionNotFound - } - return policiesMap[policyConfig.ID][uint(policyConfig.Version)], nil + policy, ok := policiesMap[policyConfig.ID][uint(policyConfig.Version)] + if !ok { + return nil, fmt.Errorf("couldn't find details for policy %q: %w", fmt.Sprintf("%s@%v", policyConfig.ID, policyConfig.Version), ErrPolicyNotFound) + } + return policy, nil } func (s *Service) addCreatorDetails(ctx context.Context, a *domain.Appeal, p *domain.Policy) error { @@ -1098,20 +1088,20 @@ func (s *Service) addCreatorDetails(ctx context.Context, a *domain.Appeal, p *do iamConfig, err := s.iam.ParseConfig(p.IAM) if err != nil { - return fmt.Errorf("parsing iam config: %w", err) + return fmt.Errorf("parsing policy.iam config: %w", err) } iamClient, err := s.iam.GetClient(iamConfig) if err != nil { - return fmt.Errorf("getting iam client: %w", err) + return fmt.Errorf("initializing iam client: %w", err) } userDetails, err := iamClient.GetUser(a.CreatedBy) if err != nil { if p.AppealConfig != nil && p.AppealConfig.AllowCreatorDetailsFailure { - s.logger.Warn(ctx, "fetching creator's user iam", "error", err) + s.logger.Warn(ctx, "unable to get creator details", "error", err) return nil } - return fmt.Errorf("fetching creator's user iam: %w", err) + return fmt.Errorf("unable to get creator details: %w", err) } userDetailsMap, ok := userDetails.(map[string]interface{}) @@ -1151,7 +1141,7 @@ func addResource(a *domain.Appeal, resourcesMap map[string]*domain.Resource) err if r == nil { return ErrResourceNotFound } else if r.IsDeleted { - return ErrResourceIsDeleted + return ErrResourceDeleted } a.Resource = r @@ -1159,13 +1149,11 @@ func addResource(a *domain.Appeal, resourcesMap map[string]*domain.Resource) err } func getProvider(a *domain.Appeal, providersMap map[string]map[string]*domain.Provider) (*domain.Provider, error) { - if providersMap[a.Resource.ProviderType] == nil { - return nil, ErrProviderTypeNotFound - } else if providersMap[a.Resource.ProviderType][a.Resource.ProviderURN] == nil { - return nil, ErrProviderURNNotFound + provider, ok := providersMap[a.Resource.ProviderType][a.Resource.ProviderURN] + if !ok { + return nil, fmt.Errorf("couldn't find details for provider %q: %w", a.Resource.ProviderType+" - "+a.Resource.ProviderURN, ErrProviderNotFound) } - - return providersMap[a.Resource.ProviderType][a.Resource.ProviderURN], nil + return provider, nil } func validateAppeal(a *domain.Appeal, pendingAppealsMap map[string]map[string]map[string]*domain.Appeal) error { diff --git a/core/appeal/service_test.go b/core/appeal/service_test.go index b705f868f..2e5998aa1 100644 --- a/core/appeal/service_test.go +++ b/core/appeal/service_test.go @@ -298,7 +298,18 @@ func (s *ServiceTestSuite) TestCreate() { }}, providers: []*domain.Provider{testProvider}, appeals: []*domain.Appeal{{ResourceID: "1"}}, - expectedError: appeal.ErrProviderTypeNotFound, + expectedError: appeal.ErrProviderNotFound, + }, + { + name: "provider urn not found", + resources: []*domain.Resource{{ + ID: "1", + ProviderType: "provider_type", + ProviderURN: "invalid_provider_urn", + }}, + providers: []*domain.Provider{testProvider}, + appeals: []*domain.Appeal{{ResourceID: "1"}}, + expectedError: appeal.ErrProviderNotFound, }, { name: "user still have active grant", @@ -394,35 +405,44 @@ func (s *ServiceTestSuite) TestCreate() { expectedError: appeal.ErrGrantNotEligibleForExtension, }, { - name: "provider urn not found", + name: "duration not found when the appeal config prevents permanent access", resources: []*domain.Resource{{ ID: "1", ProviderType: "provider_type", - ProviderURN: "invalid_provider_urn", + ProviderURN: "provider_urn", + Type: "resource_type", }}, - providers: []*domain.Provider{testProvider}, - appeals: []*domain.Appeal{{ResourceID: "1"}}, - expectedError: appeal.ErrProviderURNNotFound, + policies: []*domain.Policy{{ID: "policy_id", Version: 1}}, + providers: []*domain.Provider{testProvider}, + callMockValidateAppeal: true, + expectedAppealValidationError: provider.ErrAppealValidationDurationNotSpecified, + appeals: []*domain.Appeal{{ + ResourceID: "1", + }}, + expectedError: provider.ErrAppealValidationDurationNotSpecified, }, { - name: "duration not found when the appeal config prevents permanent access", + name: "empty duration option", resources: []*domain.Resource{{ ID: "1", ProviderType: "provider_type", ProviderURN: "provider_urn", Type: "resource_type", }}, - policies: []*domain.Policy{{ID: "policy_id", Version: 1}}, + policies: testPolicies, providers: []*domain.Provider{testProvider}, callMockValidateAppeal: true, - expectedAppealValidationError: provider.ErrOptionsDurationNotFound, + expectedAppealValidationError: provider.ErrAppealValidationEmptyDuration, appeals: []*domain.Appeal{{ ResourceID: "1", + Options: &domain.AppealOptions{ + Duration: "", + }, }}, - expectedError: appeal.ErrOptionsDurationNotFound, + expectedError: provider.ErrAppealValidationEmptyDuration, }, { - name: "empty duration option", + name: "invalid duration value", resources: []*domain.Resource{{ ID: "1", ProviderType: "provider_type", @@ -432,14 +452,14 @@ func (s *ServiceTestSuite) TestCreate() { policies: testPolicies, providers: []*domain.Provider{testProvider}, callMockValidateAppeal: true, - expectedAppealValidationError: provider.ErrDurationIsRequired, + expectedAppealValidationError: provider.ErrAppealValidationInvalidDurationValue, appeals: []*domain.Appeal{{ ResourceID: "1", Options: &domain.AppealOptions{ - Duration: "", + Duration: "invalid-duration", }, }}, - expectedError: appeal.ErrDurationIsRequired, + expectedError: provider.ErrAppealValidationInvalidDurationValue, }, { name: "invalid role", @@ -473,7 +493,7 @@ func (s *ServiceTestSuite) TestCreate() { policies: testPolicies, providers: []*domain.Provider{testProvider}, appeals: []*domain.Appeal{{ResourceID: "1"}}, - expectedError: appeal.ErrResourceTypeNotFound, + expectedError: appeal.ErrInvalidResourceType, }, { name: "policy id not found", @@ -491,7 +511,7 @@ func (s *ServiceTestSuite) TestCreate() { ExpirationDate: &timeNow, }, }}, - expectedError: appeal.ErrPolicyIDNotFound, + expectedError: appeal.ErrPolicyNotFound, }, { name: "policy version not found", @@ -512,7 +532,7 @@ func (s *ServiceTestSuite) TestCreate() { ExpirationDate: &timeNow, }, }}, - expectedError: appeal.ErrPolicyVersionNotFound, + expectedError: appeal.ErrPolicyNotFound, }, { name: "appeal duration not found in policy appeal config", @@ -545,7 +565,7 @@ func (s *ServiceTestSuite) TestCreate() { Duration: "100h", }, }}, - expectedError: appeal.ErrOptionsDurationNotFound, + expectedError: appeal.ErrDurationNotAllowed, }, } @@ -1606,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() { @@ -1617,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() { @@ -1627,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", @@ -1655,7 +1680,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusPending, }, }, - expectedError: appeal.ErrApprovalDependencyIsPending, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "found one previous approval is reject", @@ -1670,7 +1695,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusPending, }, }, - expectedError: appeal.ErrAppealStatusRejected, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "invalid approval status", @@ -1700,7 +1725,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusApproved, }, }, - expectedError: appeal.ErrApprovalStatusApproved, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "approval step already rejected", @@ -1715,7 +1740,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusRejected, }, }, - expectedError: appeal.ErrApprovalStatusRejected, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "approval step already skipped", @@ -1730,7 +1755,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() { Status: domain.ApprovalStatusSkipped, }, }, - expectedError: appeal.ErrApprovalStatusSkipped, + expectedError: appeal.ErrApprovalNotEligibleForAction, }, { name: "invalid approval status", @@ -1793,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 deleted file mode 100644 index 70157d395..000000000 --- a/core/approval/errors.go +++ /dev/null @@ -1,35 +0,0 @@ -package approval - -import "errors" - -var ( - ErrPolicyNotFound = errors.New("policy not found") - ErrDependencyApprovalStepNotFound = errors.New("unable to resolve approval step dependency") - ErrApprovalStepConditionNotFound = errors.New("unable to resolve designated condition") - 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") - ErrAppealInvalidExtensionDuration = errors.New("invalid appeal extension duration") - ErrAppealFoundActiveGrant = errors.New("user still have an active grant") - 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") - - ErrActionForbidden = errors.New("user is not allowed to make action on this approval step") - ErrActionInvalidValue = errors.New("invalid action value") -) diff --git a/core/provider/errors.go b/core/provider/errors.go index 860b640db..3ed442206 100644 --- a/core/provider/errors.go +++ b/core/provider/errors.go @@ -8,17 +8,23 @@ var ( // ErrEmptyIDParam is the error value if the policy id is empty ErrEmptyIDParam = errors.New("id can't be empty") // ErrRecordNotFound is the error value if the designated record id is not exists - ErrRecordNotFound = errors.New("record not found") - ErrEmptyProviderType = errors.New("provider type can't be nil") - ErrEmptyProviderURN = errors.New("provider urn can't be nil") - ErrNilAppeal = errors.New("appeal can't be nil") - ErrNilResource = errors.New("resource can't be nil") - ErrInvalidResourceType = errors.New("invalid resource type") - ErrInvalidRole = errors.New("invalid role") - ErrDurationIsRequired = errors.New("having permanent access to this resource is not allowed, access duration is required") - ErrOptionsDurationNotFound = errors.New("duration option not found") + ErrRecordNotFound = errors.New("record not found") + ErrEmptyProviderType = errors.New("provider type can't be nil") + ErrEmptyProviderURN = errors.New("provider urn can't be nil") + ErrNilAppeal = errors.New("appeal can't be nil") + ErrNilResource = errors.New("resource can't be nil") + ErrInvalidResourceType = errors.New("invalid resource type") + ErrInvalidRole = errors.New("invalid role") ErrUnimplementedMethod = errors.New("method is not yet implemented") ErrImportActivitiesMethodNotSupported = errors.New("import activities is not supported for this provider type") ErrGetActivityMethodNotSupported = errors.New("get activity is not supported for this provider type") + + ErrAppealValidationInvalidAccountType = errors.New("invalid account type") + ErrAppealValidationInvalidRole = errors.New("invalid role") + ErrAppealValidationDurationNotSpecified = errors.New("appeal duration is required") + ErrAppealValidationEmptyDuration = errors.New("permanent access is not allowed") + ErrAppealValidationInvalidDurationValue = errors.New("invalid duration value") + ErrAppealValidationMissingRequiredParameter = errors.New("missing required parameter") + ErrAppealValidationMissingRequiredQuestion = errors.New("missing required question") ) diff --git a/core/provider/service.go b/core/provider/service.go index 341763430..d97ead7c5 100644 --- a/core/provider/service.go +++ b/core/provider/service.go @@ -302,7 +302,7 @@ func (s *Service) ValidateAppeal(ctx context.Context, a *domain.Appeal, p *domai if !utils.ContainsString(p.Config.AllowedAccountTypes, a.AccountType) { allowedAccountTypesStr := strings.Join(p.Config.AllowedAccountTypes, ", ") - return fmt.Errorf("invalid account type: %v. allowed account types for %v: %v", a.AccountType, p.Type, allowedAccountTypesStr) + return fmt.Errorf("%w: %q. allowed account types: %v", ErrAppealValidationInvalidAccountType, a.AccountType, allowedAccountTypesStr) } roles, err := c.GetRoles(p.Config, resourceType) @@ -319,7 +319,7 @@ func (s *Service) ValidateAppeal(ctx context.Context, a *domain.Appeal, p *domai } if !isRoleExists { - return ErrInvalidRole + return fmt.Errorf("%w: %q", ErrAppealValidationInvalidRole, a.Role) } // Default to use provider config if policy config is not set @@ -334,15 +334,15 @@ func (s *Service) ValidateAppeal(ctx context.Context, a *domain.Appeal, p *domai if !AllowPermanentAccess { if a.Options == nil { - return ErrOptionsDurationNotFound + return ErrAppealValidationDurationNotSpecified } if a.Options.Duration == "" { - return ErrDurationIsRequired + return ErrAppealValidationEmptyDuration } if err := validateDuration(a.Options.Duration); err != nil { - return fmt.Errorf("invalid duration: %v", err) + return fmt.Errorf("%w: %q", ErrAppealValidationInvalidDurationValue, a.Options.Duration) } } @@ -360,15 +360,16 @@ func (*Service) validateQuestionsAndParameters(a *domain.Appeal, p *domain.Provi if p != nil && p.Config.Parameters != nil { for _, param := range p.Config.Parameters { if param.Required && !utils.ContainsString(parameterKeys, param.Key) { - return fmt.Errorf(`parameter "%s" is required`, param.Key) + return fmt.Errorf("%w: %q", ErrAppealValidationMissingRequiredParameter, fmt.Sprintf("details.%s.%s", ReservedDetailsKeyProviderParameters, param.Key)) } } } + // TODO: do validation outside of provider.ValidateAppeal if policy != nil && policy.AppealConfig != nil && len(policy.AppealConfig.Questions) > 0 { for _, question := range policy.AppealConfig.Questions { if question.Required && !utils.ContainsString(questionKeys, question.Key) { - return fmt.Errorf(`question "%s" is required`, question.Key) + return fmt.Errorf("%w: %q", ErrAppealValidationMissingRequiredQuestion, fmt.Sprintf("details.%s.%s", ReservedDetailsKeyPolicyQuestions, question.Key)) } } } @@ -670,10 +671,8 @@ func (s *Service) validateAppealConfig(a *domain.AppealConfig) error { } func validateDuration(d string) error { - if _, err := time.ParseDuration(d); err != nil { - return fmt.Errorf("parsing duration: %v", err) - } - return nil + _, err := time.ParseDuration(d) + return err } func flattenResources(resources []*domain.Resource) []*domain.Resource { diff --git a/core/provider/service_test.go b/core/provider/service_test.go index 3404070e1..31869091e 100644 --- a/core/provider/service_test.go +++ b/core/provider/service_test.go @@ -846,7 +846,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { } policy := &domain.Policy{} - expectedError := fmt.Errorf("invalid account type: %v. allowed account types for %v: %v", appeal.AccountType, mockProviderType, provider.Config.AllowedAccountTypes[0]) + expectedError := fmt.Errorf("invalid account type: %q. allowed account types: %v", appeal.AccountType, provider.Config.AllowedAccountTypes[0]) actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) @@ -878,7 +878,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { }) s.Run("should return error if get roles not exist", func() { - expectedError := provider.ErrInvalidRole + expectedError := provider.ErrAppealValidationInvalidRole appeal := &domain.Appeal{ AccountType: "test", @@ -900,11 +900,11 @@ func (s *ServiceTestSuite) TestValidateAppeal() { actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) - s.EqualError(actualError, expectedError.Error()) + s.ErrorIs(actualError, expectedError) }) s.Run("should return error if not allow permanent access and duration option not found", func() { - expectedError := provider.ErrOptionsDurationNotFound + expectedError := provider.ErrAppealValidationDurationNotSpecified role1 := &domain.Role{ID: "role-1"} appeal := &domain.Appeal{ @@ -933,7 +933,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { }) s.Run("should return error if not allow permanent access and duration is empty", func() { - expectedError := provider.ErrDurationIsRequired + expectedError := provider.ErrAppealValidationEmptyDuration role1 := &domain.Role{ID: "role-1"} appeal := &domain.Appeal{ @@ -964,7 +964,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { s.EqualError(actualError, expectedError.Error()) }) - s.Run("should return error if not allow permanent access and duration is empty", func() { + s.Run("should return error if not allow permanent access and duration value is invalid", func() { role1 := &domain.Role{ID: "role-1"} appeal := &domain.Appeal{ AccountType: "test", @@ -976,7 +976,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { Duration: "invalid-duration", }, } - expectedError := fmt.Errorf("invalid duration: parsing duration: time: invalid duration \"invalid-duration\"") + expectedError := provider.ErrAppealValidationInvalidDurationValue provider := &domain.Provider{ Config: &domain.ProviderConfig{ @@ -993,43 +993,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) - s.EqualError(actualError, expectedError.Error()) - }) - - s.Run("should return error if not allow permanent access and duration is empty", func() { - role1 := &domain.Role{ID: "role-1"} - appeal := &domain.Appeal{ - AccountType: "test", - Resource: &domain.Resource{ - ProviderType: mockProviderType, - }, - Role: role1.ID, - Options: &domain.AppealOptions{ - Duration: "invalid-duration", - }, - } - expectedError := fmt.Errorf("invalid duration: parsing duration: time: invalid duration \"invalid-duration\"") - - provider := &domain.Provider{ - Config: &domain.ProviderConfig{ - AllowedAccountTypes: []string{"test"}, - Appeal: &domain.AppealConfig{ - AllowPermanentAccess: true, - }, - }, - Type: mockProviderType, - } - policy := &domain.Policy{ - AppealConfig: &domain.PolicyAppealConfig{ - AllowPermanentAccess: false, - }, - } - - s.mockProvider.On("GetRoles", mock.Anything, mock.Anything).Return([]*domain.Role{role1}, nil).Once() - - actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) - - s.EqualError(actualError, expectedError.Error()) + s.ErrorIs(actualError, expectedError) }) s.Run("should return error when required provider parameter not present", func() { @@ -1043,9 +1007,14 @@ func (s *ServiceTestSuite) TestValidateAppeal() { Options: &domain.AppealOptions{ Duration: "24h", }, + Details: map[string]interface{}{ + provider.ReservedDetailsKeyProviderParameters: map[string]interface{}{ + "username": "", + }, + }, } - expectedError := fmt.Errorf(`parameter "%s" is required`, "username") + expectedError := provider.ErrAppealValidationMissingRequiredParameter provider := &domain.Provider{ Config: &domain.ProviderConfig{ @@ -1070,7 +1039,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) - s.EqualError(actualError, expectedError.Error()) + s.ErrorIs(actualError, expectedError) }) s.Run("should return error when required policy question not present", func() { @@ -1086,7 +1055,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { }, } - expectedError := fmt.Errorf(`question "%s" is required`, "team") + expectedError := provider.ErrAppealValidationMissingRequiredQuestion provider := &domain.Provider{ Config: &domain.ProviderConfig{ @@ -1114,7 +1083,7 @@ func (s *ServiceTestSuite) TestValidateAppeal() { actualError := s.service.ValidateAppeal(context.Background(), appeal, provider, policy) - s.EqualError(actualError, expectedError.Error()) + s.ErrorIs(actualError, expectedError) }) s.Run("should return nil when all valids", func() { diff --git a/domain/appeal.go b/domain/appeal.go index cb455f939..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" ) @@ -28,7 +29,10 @@ const ( ) var ( - ErrApproverInvalidType = errors.New("invalid approver type, expected an email string or array of email string") + ErrFailedToGetApprovers = errors.New("failed to get approvers") + ErrApproversNotFound = errors.New("approvers not found") + ErrUnexpectedApproverType = errors.New("unexpected approver type") + ErrInvalidApproverValue = errors.New("approver value is not a valid email") ) // AppealOptions @@ -276,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"` diff --git a/domain/appeal_test.go b/domain/appeal_test.go index 53ba30e7b..ce5b93d65 100644 --- a/domain/appeal_test.go +++ b/domain/appeal_test.go @@ -1,6 +1,7 @@ package domain_test import ( + "errors" "reflect" "testing" "time" @@ -731,3 +732,68 @@ func TestAppeal_ApplyPolicy(t *testing.T) { }) } } + +func TestApprovalAction_Validate(t *testing.T) { + testCases := []struct { + name string + aa domain.ApprovalAction + want error + }{ + { + name: "PassingValidation", + aa: domain.ApprovalAction{ + AppealID: "appeal-1", + ApprovalName: "approval-1", + Actor: "user@example.com", + Action: "approve", + }, + want: nil, + }, + { + name: "EmptyAppealID", + aa: domain.ApprovalAction{ + AppealID: "", + }, + want: errors.New("appeal id is required"), + }, + { + name: "EmptyApprovalName", + aa: domain.ApprovalAction{ + AppealID: "appeal-1", + ApprovalName: "", + }, + want: errors.New("approval name is required"), + }, + { + name: "InvalidActor", + aa: domain.ApprovalAction{ + AppealID: "appeal-1", + ApprovalName: "approval-1", + Actor: "invalid-email", + }, + want: errors.New(`actor is not a valid email: "invalid-email"`), + }, + { + name: "InvalidAction", + aa: domain.ApprovalAction{ + AppealID: "appeal-1", + ApprovalName: "approval-1", + Actor: "user@example.com", + Action: "invalid-action", + }, + want: errors.New(`invalid action: "invalid-action"`), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actualError := tc.aa.Validate() + if tc.want == nil { + assert.NoError(t, actualError) + } else { + assert.Error(t, actualError) + assert.Equal(t, tc.want.Error(), actualError.Error()) + } + }) + } +} diff --git a/domain/policy.go b/domain/policy.go index 70f39d7b0..6f8ac030e 100644 --- a/domain/policy.go +++ b/domain/policy.go @@ -22,7 +22,8 @@ const ( var ( ErrInvalidConditionField = errors.New("unable to parse condition's field") - validate = validator.New() + + validate = validator.New() ) type ApprovalStepStrategy string @@ -129,9 +130,9 @@ func (s Step) ResolveApprovers(a *Appeal) ([]string, error) { approversValue, err := evaluator.Expression(expr).EvaluateWithVars(params) if err != nil { - return nil, fmt.Errorf("evaluating aprrovers expression: %w", err) + return nil, fmt.Errorf("%w: %s", ErrFailedToGetApprovers, err) } else if approversValue == nil { - return nil, fmt.Errorf("evaluating aprrovers expression: evaluation result is nil") + return nil, ErrApproversNotFound } value := reflect.ValueOf(approversValue) @@ -145,27 +146,29 @@ func (s Step) ResolveApprovers(a *Appeal) ([]string, error) { case reflect.String: approvers = append(approvers, itemValue.String()) default: - return nil, fmt.Errorf(`%w: %s`, ErrApproverInvalidType, itemValue.Type().Kind()) + return nil, fmt.Errorf(`%w: %q`, ErrUnexpectedApproverType, itemValue.Type().Kind()) } } default: - return nil, fmt.Errorf(`%w: %s`, ErrApproverInvalidType, value.Type().Kind()) + return nil, fmt.Errorf(`%w: %q`, ErrUnexpectedApproverType, value.Type().Kind()) } } } - distinctApprovers := slices.UniqueStringSlice(approvers) - if err := validate.Var(distinctApprovers, "dive,email"); err != nil { - return nil, err + uniqueApprovers := slices.UniqueStringSlice(approvers) + for _, approverEmail := range uniqueApprovers { + if err := validate.Var(approverEmail, "email"); err != nil { + return nil, fmt.Errorf("%w: %q", ErrInvalidApproverValue, approverEmail) + } } - return distinctApprovers, nil + return uniqueApprovers, nil } func (s Step) ToApproval(a *Appeal, p *Policy, index int) (*Approval, error) { approvers, err := s.ResolveApprovers(a) if err != nil { - return nil, fmt.Errorf("resolving approvers `%s`: %w", s.Approvers, err) + return nil, fmt.Errorf("unable to set approvers for %q: %w", s.Name, err) } approval := &Approval{ diff --git a/internal/server/server.go b/internal/server/server.go index 54648a879..1f89e48a8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -10,9 +10,6 @@ import ( "github.com/goto/guardian/domain" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" - "github.com/go-playground/validator/v10" handlerv1beta1 "github.com/goto/guardian/api/handler/v1beta1" guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1" @@ -33,6 +30,8 @@ import ( "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "google.golang.org/api/idtoken" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/encoding/protojson" )