From 91d736d780772461b25fabd67f6136bc736b8303 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 14:12:40 +0100 Subject: [PATCH] test(purchase): pin case-insensitive email matching in 4-eyes and approver checks Every email literal in the 4-eyes self-approval tests and the SQS approver-matching tests was lowercase and byte-identical on both sides, so replacing strings.EqualFold with == in checkDifferentApprover, or dropping either strings.ToLower in matchActorAgainstApprovers, left the whole suite green. - TestManager_ApproveExecution_FourEyesOn_SQSActorEqualsCreator_Denied is now table-driven: the creator's stored email and the actor email each differ only in case in one row, and both must still be denied. - TestProcessMessage_ApproveFourEyesOn_SelfApproveDenied gains a row where the creator and contact_email are stored mixed case and the SQS actor_email is lowercase, so both folds run end to end. - New TestProcessMessage_ApproverMatchIgnoresEmailCase covers each side of the approver comparison. Mocks in the new rows are bound with .Test(t) so one regressed row fails on its own instead of panicking and skipping the rest. No production code changes: the comparisons are already case-insensitive. Closes #307 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/approvals_test.go | 61 +++++++---- internal/purchase/coverage_extra_test.go | 131 +++++++++++++++++------ 2 files changed, 139 insertions(+), 53 deletions(-) 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