diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 70c932e3..81c8b0a8 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -121,6 +121,12 @@ func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // Session has no admin role / cancel permissions → cancelPurchase's + // session-authed pre-check falls through to authorizeApprovalAction → + // purchase manager invocation, which is what this test's + // "cancel failed" assertion exercises. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", ctx, execID, "tok", approver). diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 47af2e60..6709c91d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -304,19 +304,52 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction return nil, NewClientError(404, "execution not found") } - // Two-mode dispatch (issue #46): - // * token != "" → legacy email-link flow. Token possession proves - // intent; the handler still requires the session email (when one - // exists) to be on the authorised-approver list, then delegates - // to the purchase service which validates the token itself. - // * token == "" → session-authed dashboard Cancel button. The - // session must carry a permission allowing the cancel: - // - cancel-any:purchases (admin) → any pending execution; or - // - cancel-own:purchases (default user) AND the execution's - // created_by_user_id matches the session UserID. - // Legacy rows with NULL created_by_user_id are reachable only - // via cancel-any (admin) or via the email token already in the - // inbox — they do not become orphaned by this change. + // Three-mode dispatch: + // 1. Session present AND RBAC-authorized (admin / cancel-any / + // cancel-own match) → session-authed cancel, regardless of + // whether a token is in the URL. Restores parity with the + // History page Cancel button: a user who can cancel from the + // dashboard should also be able to cancel from the email-link + // flow, since the same email-link flow already requires a + // logged-in session before reaching this endpoint (see + // frontend purchases-deeplink.ts). Without this branch, an + // admin clicking Cancel from a notification email about an + // ambient-credentials execution (CloudAccountID == nil → no + // per-account contact_email available) is locked out of an + // action they can perform from the dashboard. The token in + // the URL is informational at that point — the session has + // already authenticated the actor. + // 2. token != "" → legacy email-link flow without a qualifying + // session. authorizeApprovalAction enforces the per-account + // contact_email gate from PR #101; the purchase service + // validates the token itself before mutating state. This + // preserves the security model for forwarded email / shared + // inbox / stolen link cases — a non-privileged session falls + // through to this branch. + // 3. token == "" → session-authed dashboard Cancel button (issue + // #46). Same path as branch (1) but reached without a URL + // token. cancelPurchaseViaSession runs the cancel-any / + // cancel-own RBAC matrix and rejects sessions without it. + if session := h.tryGetSession(ctx, req); session != nil { + switch err := h.authorizeSessionCancel(ctx, session, execution); { + case err == nil: + // Session is RBAC-authorized → run the session-authed cancel. + return h.cancelPurchaseViaSession(ctx, req, execution) + case isPermissionDenied(err): + // Explicit "permission denied" (403) → fall through to the + // token branch so the contact_email gate still gets a chance + // (a logged-in user without admin / cancel-* may still be + // the per-account contact email recipient). + default: + // Transient failure (auth-service down, HasPermissionAPI + // returning a wrapped error, h.auth==nil 500). Propagate + // instead of silently widening to the contact_email gate — + // a stale auth backend should not mask itself as a 403 about + // missing contact emails. CR feedback on PR #216. + return nil, err + } + } + if token != "" { actor, err := h.authorizeApprovalAction(ctx, req, execution) if err != nil { @@ -759,18 +792,49 @@ func (h *Handler) authorizeSessionRetry(ctx context.Context, session *Session, e // → session-authed call with ?token=…) can record per-user attribution // without changing the token-only fallback path used by message workers. func (h *Handler) tryResolveActorEmail(ctx context.Context, req *events.LambdaFunctionURLRequest) string { + if s := h.tryGetSession(ctx, req); s != nil { + return s.Email + } + return "" +} + +// isPermissionDenied reports whether err is *directly* a 403 ClientError +// (not merely something that wraps one). Used by the cancel-from-email +// session pre-check to distinguish a legitimate "your session lacks +// cancel-* permission" answer (fall through to the token branch's +// contact_email gate) from a transient auth-service failure (propagate so +// the caller sees the real cause). CR feedback on PR #216. +// +// Strict (un-wrapped) type assertion is deliberate: if a future caller +// wraps a 403 to add context (fmt.Errorf("permission check failed: %w", +// ...)), that wrapped error represents the *outer* failure mode (the +// wrapper's intent), not "this is still a 403". errors.As-style unwrapping +// would erase that distinction and silently route wrapped backend +// failures into the contact_email gate — exactly the misclassification +// the propagate-vs-fall-through split is meant to prevent. +func isPermissionDenied(err error) bool { + ce, ok := err.(*clientError) + return ok && ce.code == 403 +} + +// tryGetSession returns the validated session for the request, or nil when +// the request carries no Bearer token, the auth service isn't configured, +// or session validation fails. Mirrors tryResolveActorEmail's silent +// best-effort semantics so AuthPublic callers can opt into session-aware +// behaviour without forcing a 401 on tokenless flows. +func (h *Handler) tryGetSession(ctx context.Context, req *events.LambdaFunctionURLRequest) *Session { if req == nil || h.auth == nil { - return "" + return nil } bearer := h.extractBearerToken(req) if bearer == "" { - return "" + return nil } session, err := h.auth.ValidateSession(ctx, bearer) if err != nil || session == nil { - return "" + return nil } - return session.Email + return session } // buildPurchaseDetailsResponse builds the response map for a purchase execution. diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 096bd98c..fdeec1a7 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -80,6 +80,12 @@ func TestHandler_cancelPurchase(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // Session has no admin role and no cancel permissions, so cancelPurchase's + // session-authed pre-check (added to fix the deep-link contact_email gate) + // falls through to authorizeApprovalAction. The contact_email + globalNotify + // fixture above is what proves the legacy email-link path still works. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", ctx, execID, "valid-token", approver).Return(nil) @@ -1418,6 +1424,201 @@ func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) { assert.Contains(t, err.Error(), "no authorization token provided") } +// TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate is the +// regression for the cancel-from-email contact_email-gate UX bug: +// +// "Failed to cancel purchase: no per-account contact email configured for +// this execution; set the cloud account's contact_email before approving" +// +// The deep-link cancel flow (frontend purchases-deeplink.ts) always POSTs +// /api/purchases/cancel/:id with both an X-Authorization Bearer session AND +// the email-link's URL token. Before this fix, cancelPurchase always took +// the token branch → authorizeApprovalAction → 403 when the execution's +// recs had no per-account contact_email (e.g. AWS ambient-credentials +// rows where CloudAccountID is nil). The same admin could already cancel +// the same execution from the History page Cancel button (session-authed, +// no contact_email gate); the email-link path was the only place they +// were locked out. +// +// Fix: when the caller has a valid session AND authorizeSessionCancel +// passes (admin / cancel-any / cancel-own match), use the session-authed +// cancel path, regardless of whether a token is present. Token-only +// callers (no session, e.g. forwarded email or shared inbox) still hit +// the contact_email gate from PR #101. +func TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate(t *testing.T) { + // Ambient-credentials execution: CloudAccountID is nil, so + // gatherAccountContactEmails returns []. Pre-fix this would 403 even + // for admin sessions. Post-fix the session-authed pre-check fires and + // the cancel succeeds. + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: nil, // no creator binding either; admin can still cancel + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + var saved *config.PurchaseExecution + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + saved = args.Get(1).(*config.PurchaseExecution) + }). + Return(nil) + + // Token IS present in the URL — the deep-link flow always sends one. + // The fix's whole point is that the admin session takes the + // session-authed branch instead of routing through the token path. + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.NoError(t, err, "admin clicking Cancel from notification email must succeed even when no contact_email is configured") + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + + require.NotNil(t, saved, "session-authed branch must commit the status flip") + assert.Equal(t, "cancelled", saved.Status) + require.NotNil(t, saved.CancelledBy, "session-authed branch must stamp CancelledBy") + assert.Equal(t, session.Email, *saved.CancelledBy) + + // Critical security assertion: the token branch's contact_email gate + // (authorizeApprovalAction → GetGlobalConfig → resolveApprovalRecipients) + // was NOT consulted. If a regression re-routed admins through the + // token path, GetGlobalConfig would fire because the gate fetches + // the global notification email; asserting it didn't is the cleanest + // way to pin the new branch behaviour. + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + +// TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate is +// the cancel-own variant of the regression above: a non-admin user with +// cancel-own on a purchase they themselves created should be able to +// cancel it from the email link, bypassing the contact_email gate that +// would otherwise lock them out for ambient-credentials executions. +func TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate(t *testing.T) { + creator := cancelCallerID // session user is the creator + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false /*hasAny*/, true /*hasOwn*/) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + +// TestIsPermissionDenied pins the strict (un-wrapped) type-assertion +// invariant from CR pass 2 on PR #216: a wrapped 403 ClientError must +// NOT count as permission-denied because the wrapper changes the +// failure's outer category. Only an *exact* *clientError with code 403 +// triggers the fall-through to the contact_email gate. +func TestIsPermissionDenied(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil error is not denial", nil, false}, + {"plain 403 ClientError is denial", NewClientError(403, "permission denied"), true}, + {"500 ClientError is not denial", NewClientError(500, "auth service down"), false}, + {"401 ClientError is not denial", NewClientError(401, "no session"), false}, + {"non-ClientError is not denial", errors.New("auth backend timeout"), false}, + { + name: "wrapped 403 is NOT denial (errors.As-style unwrap is rejected)", + err: fmt.Errorf("permission check failed: %w", NewClientError(403, "denied")), + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isPermissionDenied(tc.err)) + }) + } +} + +// TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates pins the +// CR-feedback hardening on PR #216: when authorizeSessionCancel returns a +// non-403 error (auth service down, HasPermissionAPI wrapped error, +// h.auth nil), the pre-check MUST surface it instead of silently falling +// through to the contact_email gate. A stale auth backend should not +// disguise itself as a "set the contact_email" 403, which would mislead +// operators investigating the failure. +func TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates(t *testing.T) { + creator := cancelOtherID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) + // Simulate a transient auth-backend failure on the cancel-any check. + // authorizeSessionCancel wraps this as "permission check failed: …" + // — NOT a 403 ClientError. The pre-check must propagate. + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases"). + Return(false, errors.New("auth backend timeout")) + + handler := &Handler{config: mockConfig, auth: mockAuth} + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "permission check failed", + "transient auth-service errors must surface, not silently route to the contact_email gate") + assert.NotContains(t, err.Error(), "contact email", + "the contact_email message would mislead operators about the actual failure cause") + + // Token branch must NOT have been reached — GetGlobalConfig is the + // signature first call inside authorizeApprovalAction. + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + +// TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate +// pins the security-model invariant from PR #101: a logged-in user +// without admin / cancel-any / cancel-own permission MUST still go +// through authorizeApprovalAction and hit the contact_email gate when +// clicking a forwarded cancel link. The session-authed pre-check is +// strictly additive — it widens for privileged users only. +func TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate(t *testing.T) { + creator := cancelOtherID // someone else created it; no cancel-own match + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + ApprovalToken: "deep-link-token", + Status: "notified", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false /*hasAny*/, false /*hasOwn*/) + // Token branch fetches the global config to populate the Cc list. + mockConfig.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "no per-account contact email configured for this execution", + "non-privileged session must fall through to the contact_email gate from PR #101") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) +} + // ─── Session-authed Retry (issue #47) ────────────────────────────────────── // // Mirror image of the cancel matrix above. retryPurchase creates a NEW diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 7c78240a..658f753d 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -764,6 +764,12 @@ func TestHandler_HandleRequest_CancelPurchase(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) + // Session has no admin role / cancel permissions → cancelPurchase's + // session-authed pre-check falls through to authorizeApprovalAction → + // MockPurchaseManager.CancelExecution, exercising the legacy + // email-link path that this end-to-end test was always covering. + mockAuth.On("HasPermissionAPI", mock.Anything, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", mock.Anything, execID, "token456", approver).Return(nil)