feat: update approval check for self approval (#178)

* feat: update approval check for self approval

* fix: test cases

* Fix: remove unwanted code

* Resolve comments

* Updated proton commit

Makefile

@@ -3,7 +3,7 @@ COMMIT := $(shell git rev-parse --short HEAD)
 TAG := "$(shell git rev-list --tags --max-count=1)"
 VERSION := "$(shell git describe --tags ${TAG})-next"
 BUILD_DIR=dist
-PROTON_COMMIT := "9a99d4894eba2ae6319da2667838121209da9ec8"
+PROTON_COMMIT := "293abecafe5eda9ea683ced75644ded070134b2f"
 
 .PHONY: all build clean test tidy vet proto setup format generate
 

api/handler/v1beta1/adapter.go

@@ -233,14 +233,15 @@ func (a *adapter) FromPolicyProto(p *guardianv1beta1.Policy) *domain.Policy {
 		var steps []*domain.Step
 		for _, s := range p.GetSteps() {
 			steps = append(steps, &domain.Step{
-				Name:            s.GetName(),
-				Description:     s.GetDescription(),
-				When:            s.GetWhen(),
-				Strategy:        domain.ApprovalStepStrategy(s.GetStrategy()),
-				RejectionReason: s.GetRejectionReason(),
-				ApproveIf:       s.GetApproveIf(),
-				AllowFailed:     s.GetAllowFailed(),
-				Approvers:       s.GetApprovers(),
+				Name:                  s.GetName(),
+				Description:           s.GetDescription(),
+				When:                  s.GetWhen(),
+				Strategy:              domain.ApprovalStepStrategy(s.GetStrategy()),
+				RejectionReason:       s.GetRejectionReason(),
+				ApproveIf:             s.GetApproveIf(),
+				AllowFailed:           s.GetAllowFailed(),
+				Approvers:             s.GetApprovers(),
+				DontAllowSelfApproval: s.GetDontAllowSelfApproval(),
 			})
 		}
 		policy.Steps = steps
@@ -375,14 +376,15 @@ func (a *adapter) ToPolicyProto(p *domain.Policy) (*guardianv1beta1.Policy, erro
 		var steps []*guardianv1beta1.Policy_ApprovalStep
 		for _, s := range p.Steps {
 			steps = append(steps, &guardianv1beta1.Policy_ApprovalStep{
-				Name:            s.Name,
-				Description:     s.Description,
-				When:            s.When,
-				Strategy:        string(s.Strategy),
-				RejectionReason: s.RejectionReason,
-				ApproveIf:       s.ApproveIf,
-				AllowFailed:     s.AllowFailed,
-				Approvers:       s.Approvers,
+				Name:                  s.Name,
+				Description:           s.Description,
+				When:                  s.When,
+				Strategy:              string(s.Strategy),
+				RejectionReason:       s.RejectionReason,
+				ApproveIf:             s.ApproveIf,
+				AllowFailed:           s.AllowFailed,
+				Approvers:             s.Approvers,
+				DontAllowSelfApproval: s.DontAllowSelfApproval,
 			})
 		}
 		policyProto.Steps = steps

core/appeal/errors.go

@@ -49,8 +49,10 @@ var (
 	ErrAppealNotEligibleForApproval = errors.New("appeal status not eligible for approval")
 	ErrApprovalNotEligibleForAction = errors.New("approval not eligible for action")
 
-	ErrAppealStatusInvalid = errors.New("invalid appeal status")
-	ErrNoChanges           = errors.New("no changes found")
+	ErrAppealStatusInvalid    = errors.New("invalid appeal status")
+	ErrNoChanges              = errors.New("no changes found")
+	ErrNoPolicyStepFound      = errors.New("no policy step found")
+	ErrSelfApprovalNotAllowed = errors.New("requestor is not allowed to approve their own request")
 )
 
 type InvalidError struct {

core/appeal/service.go

@@ -315,7 +315,7 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
 		if err := s.addCreatorDetails(ctx, appeal, policy); err != nil {
 			return fmt.Errorf("getting creator details: %w", err)
 		}
-		
+
 		if err := s.populateAppealMetadata(ctx, appeal, policy); err != nil {
 			return fmt.Errorf("getting appeal metadata: %w", err)
 		}
@@ -876,6 +876,25 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr
 	currentApproval.Reason = approvalAction.Reason
 	currentApproval.UpdatedAt = TimeNow()
 	if approvalAction.Action == domain.AppealActionNameApprove {
+		if appeal.Policy == nil {
+			appeal.Policy, err = s.policyService.GetOne(ctx, appeal.PolicyID, appeal.PolicyVersion)
+			if err != nil {
+				return nil, err
+			}
+		}
+
+		policyStep := appeal.Policy.GetStepByName(currentApproval.Name)
+		if policyStep == nil {
+			return nil, fmt.Errorf("%w: %q for appeal %q", ErrNoPolicyStepFound, approvalAction.ApprovalName, appeal.ID)
+		}
+
+		// check if user is self approving the appeal
+		if policyStep.DontAllowSelfApproval {
+			if approvalAction.Actor == appeal.CreatedBy {
+				return nil, ErrSelfApprovalNotAllowed
+			}
+		}
+
 		currentApproval.Approve()
 
 		// mark next approval as pending
@@ -884,12 +903,6 @@ func (s *Service) UpdateApproval(ctx context.Context, approvalAction domain.Appr
 			nextApproval.Status = domain.ApprovalStatusPending
 		}
 
-		if appeal.Policy == nil {
-			appeal.Policy, err = s.policyService.GetOne(ctx, appeal.PolicyID, appeal.PolicyVersion)
-			if err != nil {
-				return nil, err
-			}
-		}
 		if err := appeal.AdvanceApproval(appeal.Policy); err != nil {
 			return nil, err
 		}

core/appeal/service_test.go

@@ -5252,8 +5252,8 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 					tc.expectedAppealDetails.Policy == nil {
 					mockPolicy := &domain.Policy{
 						Steps: []*domain.Step{
-							{Name: "step-1"},
-							{Name: "step-2"},
+							{Name: "approval_0"},
+							{Name: "approval_1"},
 						},
 					}
 					h.mockPolicyService.EXPECT().
@@ -5294,6 +5294,257 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 			})
 		}
 	})
+	s.Run("should check reject for DontAllowSelfApproval flag", func() {
+		creator := "[email protected]"
+		user := "[email protected]"
+		testCases := []struct {
+			name                   string
+			expectedApprovalAction domain.ApprovalAction
+			expectedAppealDetails  *domain.Appeal
+			expectedResult         *domain.Appeal
+			expectedNotifications  []domain.Notification
+			expectedGrant          *domain.Grant
+		}{
+			{
+				name: "rejected even when no self approval is allowed in policy step",
+				expectedApprovalAction: domain.ApprovalAction{
+					AppealID:     appealID,
+					ApprovalName: "approval_1",
+					Actor:        "[email protected]",
+					Action:       domain.AppealActionNameReject,
+					Reason:       "test-reason",
+				},
+				expectedAppealDetails: &domain.Appeal{
+					ID:         validApprovalActionParam.AppealID,
+					AccountID:  "[email protected]",
+					CreatedBy:  creator,
+					ResourceID: "1",
+					Role:       "test-role",
+					Resource: &domain.Resource{
+						ID:           "1",
+						URN:          "urn",
+						Name:         "test-resource-name",
+						ProviderType: "test-provider",
+					},
+					Status: domain.AppealStatusPending,
+					Approvals: []*domain.Approval{
+						{
+							Name:   "approval_0",
+							Index:  0,
+							Status: domain.ApprovalStatusApproved,
+						},
+						{
+							Name:      "approval_1",
+							Index:     1,
+							Status:    domain.ApprovalStatusPending,
+							Approvers: []string{"[email protected]"},
+						},
+					},
+					Policy: &domain.Policy{
+						Steps: []*domain.Step{
+							{Name: "approval_0"},
+							{Name: "approval_1", DontAllowSelfApproval: true},
+						},
+					},
+				},
+				expectedResult: &domain.Appeal{
+					ID:         validApprovalActionParam.AppealID,
+					AccountID:  "[email protected]",
+					CreatedBy:  creator,
+					ResourceID: "1",
+					Role:       "test-role",
+					Resource: &domain.Resource{
+						ID:           "1",
+						URN:          "urn",
+						Name:         "test-resource-name",
+						ProviderType: "test-provider",
+					},
+					Status: domain.AppealStatusRejected,
+					Approvals: []*domain.Approval{
+						{
+							Name:   "approval_0",
+							Index:  0,
+							Status: domain.ApprovalStatusApproved,
+						},
+						{
+							Name:      "approval_1",
+							Index:     1,
+							Status:    domain.ApprovalStatusRejected,
+							Approvers: []string{"[email protected]"},
+							Actor:     &user,
+							Reason:    "test-reason",
+							UpdatedAt: timeNow,
+						},
+					},
+					Policy: &domain.Policy{
+						Steps: []*domain.Step{
+							{Name: "approval_0"},
+							{Name: "approval_1", DontAllowSelfApproval: true},
+						},
+					},
+				},
+				expectedNotifications: []domain.Notification{
+					{
+						User: creator,
+						Message: domain.NotificationMessage{
+							Type: domain.NotificationTypeAppealRejected,
+							Variables: map[string]interface{}{
+								"resource_name": "test-resource-name (test-provider: urn)",
+								"role":          "test-role",
+							},
+						},
+					},
+				},
+			},
+		}
+		for _, tc := range testCases {
+			s.Run(tc.name, func() {
+				h := newServiceTestHelper()
+
+				h.mockRepository.EXPECT().
+					GetByID(h.ctxMatcher, validApprovalActionParam.AppealID).
+					Return(tc.expectedAppealDetails, nil).Once()
+
+				if tc.expectedApprovalAction.Action == domain.AppealActionNameApprove &&
+					tc.expectedAppealDetails.Policy == nil {
+					mockPolicy := &domain.Policy{
+						Steps: []*domain.Step{
+							{Name: "approval_0"},
+							{Name: "approval_1"},
+						},
+					}
+					h.mockPolicyService.EXPECT().
+						GetOne(mock.Anything, tc.expectedAppealDetails.PolicyID, tc.expectedAppealDetails.PolicyVersion).
+						Return(mockPolicy, nil).Once()
+					tc.expectedResult.Policy = mockPolicy
+				}
+
+				if tc.expectedGrant != nil {
+					h.mockProviderService.EXPECT().
+						IsExclusiveRoleAssignment(mock.Anything, mock.Anything, mock.Anything).
+						Return(false).Once()
+					h.mockGrantService.EXPECT().
+						List(mock.Anything, domain.ListGrantsFilter{
+							AccountIDs:  []string{tc.expectedAppealDetails.AccountID},
+							ResourceIDs: []string{tc.expectedAppealDetails.ResourceID},
+							Statuses:    []string{string(domain.GrantStatusActive)},
+							Permissions: tc.expectedAppealDetails.Permissions,
+						}).Return([]domain.Grant{}, nil).Once()
+					h.mockGrantService.EXPECT().
+						Prepare(mock.Anything, mock.Anything).Return(tc.expectedGrant, nil).Once()
+
+					h.mockProviderService.EXPECT().GrantAccess(mock.Anything, *tc.expectedGrant).Return(nil).Once()
+				}
+
+				h.mockRepository.EXPECT().Update(h.ctxMatcher, tc.expectedResult).Return(nil).Once()
+				h.mockNotifier.EXPECT().Notify(h.ctxMatcher, mock.Anything).Return(nil).Once()
+				h.mockAuditLogger.EXPECT().Log(h.ctxMatcher, mock.Anything, mock.Anything).
+					Return(nil).Once()
+
+				actualResult, actualError := h.service.UpdateApproval(context.Background(), tc.expectedApprovalAction)
+				s.NoError(actualError)
+				tc.expectedResult.Policy = actualResult.Policy
+				s.Equal(tc.expectedResult, actualResult)
+
+				time.Sleep(time.Millisecond)
+				h.assertExpectations(s.T())
+			})
+		}
+	})
+
+	s.Run("should check approve for DontAllowSelfApproval flag", func() {
+		creator := "[email protected]"
+		testCases := []struct {
+			name                   string
+			expectedApprovalAction domain.ApprovalAction
+			expectedAppealDetails  *domain.Appeal
+			expectedResult         *domain.Appeal
+			expectedNotifications  []domain.Notification
+			expectedGrant          *domain.Grant
+			expectedError          error
+		}{
+			{
+				name:                   "not approved when no self approval is allowed in policy step",
+				expectedApprovalAction: validApprovalActionParam,
+				expectedAppealDetails: &domain.Appeal{
+					ID:         validApprovalActionParam.AppealID,
+					AccountID:  "[email protected]",
+					CreatedBy:  creator,
+					ResourceID: "1",
+					Role:       "test-role",
+					Resource: &domain.Resource{
+						ID:           "1",
+						URN:          "urn",
+						Name:         "test-resource-name",
+						ProviderType: "test-provider",
+					},
+					Status: domain.AppealStatusPending,
+					Approvals: []*domain.Approval{
+						{
+							Name:   "approval_0",
+							Status: domain.ApprovalStatusApproved,
+							Index:  0,
+						},
+						{
+							Name:      "approval_1",
+							Status:    domain.ApprovalStatusPending,
+							Index:     1,
+							Approvers: []string{"[email protected]"},
+						},
+					},
+				},
+				expectedResult: nil,
+				expectedError:  appeal.ErrSelfApprovalNotAllowed,
+			},
+		}
+		for _, tc := range testCases {
+			s.Run(tc.name, func() {
+				h := newServiceTestHelper()
+
+				h.mockRepository.EXPECT().
+					GetByID(h.ctxMatcher, validApprovalActionParam.AppealID).
+					Return(tc.expectedAppealDetails, nil).Once()
+
+				if tc.expectedApprovalAction.Action == domain.AppealActionNameApprove &&
+					tc.expectedAppealDetails.Policy == nil {
+					mockPolicy := &domain.Policy{
+						Steps: []*domain.Step{
+							{Name: "approval_0"},
+							{Name: "approval_1", DontAllowSelfApproval: true},
+						},
+					}
+					h.mockPolicyService.EXPECT().
+						GetOne(mock.Anything, tc.expectedAppealDetails.PolicyID, tc.expectedAppealDetails.PolicyVersion).
+						Return(mockPolicy, nil).Once()
+				}
+
+				h.mockProviderService.EXPECT().
+					IsExclusiveRoleAssignment(mock.Anything, mock.Anything, mock.Anything).
+					Return(false).Once()
+				h.mockGrantService.EXPECT().
+					List(mock.Anything, domain.ListGrantsFilter{
+						AccountIDs:  []string{tc.expectedAppealDetails.AccountID},
+						ResourceIDs: []string{tc.expectedAppealDetails.ResourceID},
+						Statuses:    []string{string(domain.GrantStatusActive)},
+						Permissions: tc.expectedAppealDetails.Permissions,
+					}).Return([]domain.Grant{}, nil).Once()
+				h.mockGrantService.EXPECT().
+					Prepare(mock.Anything, mock.Anything).Return(nil, nil).Once()
+
+				h.mockProviderService.EXPECT().GrantAccess(mock.Anything, mock.Anything).Return(nil).Once()
+
+				h.mockRepository.EXPECT().Update(h.ctxMatcher, mock.Anything).Return(nil).Once()
+				h.mockNotifier.EXPECT().Notify(h.ctxMatcher, mock.Anything).Return(nil).Once()
+				h.mockAuditLogger.EXPECT().Log(h.ctxMatcher, mock.Anything, mock.Anything).
+					Return(nil).Once()
+
+				actualResult, actualError := h.service.UpdateApproval(context.Background(), tc.expectedApprovalAction)
+				s.Nil(actualResult)
+				s.ErrorIs(actualError, tc.expectedError)
+			})
+		}
+	})
+
 }
 
 func (s *ServiceTestSuite) TestGrantAccessToProvider() {