Skip to content

Commit

Permalink
fix: bug in appeal creation
Browse files Browse the repository at this point in the history
- bug: if a policy only has auto approval steps, and the appeal gets auto-approved, then with the current logic the appeal is approved but the grant never gets created. Fix this by adding a check
  • Loading branch information
bsushmith committed Sep 20, 2023
1 parent 1d897e8 commit ea12aab
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 1 deletion.
2 changes: 1 addition & 1 deletion core/appeal/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func (s *Service) Create(ctx context.Context, appeals []*domain.Appeal, opts ...
appeal.Policy = nil

for _, approval := range appeal.Approvals {
if approval.Index == len(appeal.Approvals)-1 && approval.Status == domain.ApprovalStatusApproved {
if approval.Index == len(appeal.Approvals)-1 && (approval.Status == domain.ApprovalStatusApproved || appeal.Status == domain.AppealStatusApproved) {
newGrant, revokedGrant, err := s.prepareGrant(ctx, appeal)
if err != nil {
return fmt.Errorf("preparing grant: %w", err)
Expand Down
32 changes: 32 additions & 0 deletions core/appeal/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,13 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
AllowFailed: false,
ApproveIf: "1==1",
},
{
Name: "step_2",
Strategy: "manual",
When: "1==0",
AllowFailed: false,
Approvers: []string{"[email protected]"},
},
},
IAM: &domain.IAMConfig{
Provider: "http",
Expand Down Expand Up @@ -1188,6 +1195,13 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
Status: domain.ApprovalStatusApproved,
PolicyID: "policy_1",
PolicyVersion: 1,
}, {
Name: "step_2",
Index: 1,
Status: domain.ApprovalStatusSkipped,
PolicyID: "policy_1",
PolicyVersion: 1,
Approvers: []string{"[email protected]"},
},
},
Grant: &domain.Grant{
Expand Down Expand Up @@ -1225,6 +1239,15 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
PolicyID: "policy_1",
PolicyVersion: 1,
},
{
ID: "2",
Name: "step_2",
Index: 1,
Status: domain.ApprovalStatusSkipped,
PolicyID: "policy_1",
PolicyVersion: 1,
Approvers: []string{"[email protected]"},
},
},
Grant: &domain.Grant{
ResourceID: "1",
Expand Down Expand Up @@ -1271,6 +1294,15 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithExistingAppealAndWithAutoApprov
OrderBy: []string{"updated_at:desc"},
}).
Return(expectedExistingGrants, nil).Once()
// duplicate call with slight change in filters but the code needs it in order to work. appeal create code needs to be refactored.
s.mockGrantService.EXPECT().
List(mock.AnythingOfType("*context.emptyCtx"), domain.ListGrantsFilter{
Statuses: []string{string(domain.GrantStatusActive)},
AccountIDs: []string{appeals[0].AccountID},
ResourceIDs: []string{appeals[0].ResourceID},
Permissions: []string{"test-permission"},
}).
Return(expectedExistingGrants, nil).Once()
s.mockProviderService.On("ValidateAppeal", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
s.mockProviderService.On("GetPermissions", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return([]interface{}{"test-permission"}, nil)
Expand Down

0 comments on commit ea12aab

Please sign in to comment.