diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 78a2d8fe0..a8b4cab0c 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -663,30 +663,51 @@ func TestManager_ApproveAndExecute_FourEyesOff_AllowsSelfApprove(t *testing.T) { // awareness of 4-eyes mode at all, so a replayed or forwarded token+actor // matching the creator's own email would approve the row despite mode being // on. +// +// The case-differing rows (issue #307) pin the case-insensitive comparison: +// email addresses are matched case-insensitively everywhere else in the +// approval flow, so a creator whose stored email differs from the actor's +// only in letter case is the same person and must still be denied. A +// regression that swaps strings.EqualFold for == fails those rows. func TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied(t *testing.T) { - ctx := context.Background() - manager, store, _ := newApproveManager(t) - - creatorID := "user-creator" - creatorEmail := "creator@example.com" - execution := &config.PurchaseExecution{ - ExecutionID: "exec-sqs-self", - PlanID: "plan-fourEyes", - Status: "pending", - ApprovalToken: config.HashApprovalToken("valid-token"), - CreatedByUserID: &creatorID, + cases := []struct { + name string + creatorEmail string + actorEmail string + }{ + {name: "byte-identical emails", creatorEmail: "creator@example.com", actorEmail: "creator@example.com"}, + {name: "creator stored mixed case", creatorEmail: "Creator@Example.COM", actorEmail: "creator@example.com"}, + {name: "actor mixed case", creatorEmail: "creator@example.com", actorEmail: "CREATOR@Example.com"}, } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + manager, store, _ := newApproveManager(t) + // Fail this row on an unexpected call instead of panicking, so + // one regressed row doesn't abort the remaining rows. + store.Test(t) - store.On("GetExecutionByID", ctx, "exec-sqs-self").Return(execution, nil) - store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) - store.On("GetUserEmailByID", ctx, creatorID).Return(creatorEmail, nil) + creatorID := "user-creator" + execution := &config.PurchaseExecution{ + ExecutionID: "exec-sqs-self", + PlanID: "plan-fourEyes", + Status: "pending", + ApprovalToken: config.HashApprovalToken("valid-token"), + CreatedByUserID: &creatorID, + } - _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", creatorEmail, nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") - store.AssertNotCalled(t, "TransitionExecutionStatus", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - store.AssertExpectations(t) + store.On("GetExecutionByID", ctx, "exec-sqs-self").Return(execution, nil) + store.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + store.On("GetUserEmailByID", ctx, creatorID).Return(tc.creatorEmail, nil) + + _, err := manager.ApproveExecution(ctx, "exec-sqs-self", "valid-token", tc.actorEmail, nil) + require.ErrorIs(t, err, ErrFourEyesDenied) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + store.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertExpectations(t) + }) + } } // TestManager_ApproveExecution_FourEyesOn_DifferentActor_Allowed is the diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index e7654c46e..afdd840c2 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -3,6 +3,7 @@ package purchase import ( "context" "errors" + "fmt" "testing" "time" @@ -437,45 +438,109 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { // enabled. Pre-fix this test proceeds straight through to // TransitionExecutionStatus (proven by stashing the fix and re-running); // post-fix ProcessMessage returns the 4-eyes denial before any state change. +// +// The case-differing row (issue #307) runs both email folds on the SQS path +// end to end: the actor must match the mixed-case per-account contact_email +// case-insensitively to get past verifyAsyncApprovalActor, and must then be +// recognized as the creator despite the case difference so 4-eyes denies. func TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied(t *testing.T) { - ctx := context.Background() - mockStore := new(MockConfigStore) - mockEmail := new(MockEmailSender) - - accountID := "acct-1" - creatorID := "user-creator" - creatorEmail := "owner@example.com" - exec := &config.PurchaseExecution{ - ExecutionID: "exec-appv-self", - Status: "pending", - ApprovalToken: config.HashApprovalToken("correct-token"), - CreatedByUserID: &creatorID, - Recommendations: []config.RecommendationRecord{ - {CloudAccountID: &accountID}, - }, + cases := []struct { + name string + storedEmail string // creator's user email and the account contact_email + messageActor string + }{ + {name: "byte-identical emails", storedEmail: "owner@example.com", messageActor: "owner@example.com"}, + {name: "stored mixed case, actor lowercase", storedEmail: "Owner@Example.COM", messageActor: "owner@example.com"}, } - account := &config.CloudAccount{ID: accountID, ContactEmail: creatorEmail} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockStore.Test(t) // fail this row on an unexpected call, don't panic + mockEmail := new(MockEmailSender) - // verifyAsyncApprovalActor + the manager's own fetch inside ApproveExecution - // both load the execution; enforceFourEyesPolicy loads it again. - mockStore.On("GetExecutionByID", ctx, "exec-appv-self").Return(exec, nil) - mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) - mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) - mockStore.On("GetUserEmailByID", ctx, creatorID).Return(creatorEmail, nil) + accountID := "acct-1" + creatorID := "user-creator" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-appv-self", + Status: "pending", + ApprovalToken: config.HashApprovalToken("correct-token"), + CreatedByUserID: &creatorID, + Recommendations: []config.RecommendationRecord{ + {CloudAccountID: &accountID}, + }, + } + account := &config.CloudAccount{ID: accountID, ContactEmail: tc.storedEmail} - manager := &Manager{ - config: mockStore, - email: mockEmail, - dashboardURL: "https://dashboard.example.com", + // verifyAsyncApprovalActor + the manager's own fetch inside + // ApproveExecution both load the execution; enforceFourEyesPolicy + // loads it again. + mockStore.On("GetExecutionByID", ctx, "exec-appv-self").Return(exec, nil) + mockStore.On("GetCloudAccount", ctx, accountID).Return(account, nil) + mockStore.On("GetGlobalConfig", ctx).Return(fourEyesCfgOnForManager(), nil) + mockStore.On("GetUserEmailByID", ctx, creatorID).Return(tc.storedEmail, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + msg := fmt.Sprintf(`{"type":"approve","execution_id":"exec-appv-self","token":"correct-token","actor_email":%q}`, tc.messageActor) + err := manager.ProcessMessage(ctx, msg) + require.ErrorIs(t, err, ErrFourEyesDenied) + assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) + }) } +} - err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv-self","token":"correct-token","actor_email":"owner@example.com"}`) - require.Error(t, err) - assert.Contains(t, err.Error(), "4-eyes mode requires a different approver") - mockStore.AssertNotCalled(t, "TransitionExecutionStatus", - mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) - mockStore.AssertExpectations(t) - mockEmail.AssertExpectations(t) +// TestProcessMessage_ApproverMatchIgnoresEmailCase pins the case-insensitive +// comparison in matchActorAgainstApprovers (issue #307): an SQS approve or +// cancel message whose actor_email differs from the account's stored +// contact_email only in letter case is the authorized approver and must get +// past verifyAsyncApprovalActor. One row per side of the comparison, so +// dropping the strings.ToLower on either the actor or the stored address +// fails a row. The non-matching control +// (TestProcessMessage_ApproveRejectsNonMatchingActor) keeps an "accept +// everyone" regression from passing. +func TestProcessMessage_ApproverMatchIgnoresEmailCase(t *testing.T) { + cases := []struct { + name string + contactEmail string + messageActor string + }{ + {name: "contact_email mixed case, actor lowercase", contactEmail: "Owner@Example.COM", messageActor: "owner@example.com"}, + {name: "contact_email lowercase, actor mixed case", contactEmail: "owner@example.com", messageActor: "OWNER@Example.com"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockStore.Test(t) // fail this row on an unexpected call, don't panic + + accountID := "acct-1" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-case", + Status: "pending", + ApprovalToken: config.HashApprovalToken("correct-token"), + Recommendations: []config.RecommendationRecord{ + {CloudAccountID: &accountID}, + }, + } + mockStore.On("GetExecutionByID", ctx, "exec-case").Return(exec, nil) + mockStore.On("GetCloudAccount", ctx, accountID). + Return(&config.CloudAccount{ID: accountID, ContactEmail: tc.contactEmail}, nil) + + manager := &Manager{config: mockStore, email: new(MockEmailSender)} + msg := &AsyncMessage{ExecutionID: "exec-case", Token: "correct-token", ActorEmail: tc.messageActor} + require.NoError(t, manager.verifyAsyncApprovalActor(ctx, msg)) + mockStore.AssertExpectations(t) + }) + } } // TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds is the