diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 0a87277b..4f34b3cc 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -612,15 +612,14 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // session. authorizeApprovalAction enforces the per-account // contact_email gate from PR #101; the purchase service // validates the token itself before mutating state. - // 3. token == "" → session-authed dashboard Approve button. - // approvePurchaseViaSession runs the approve-any / - // approve-own RBAC matrix and rejects sessions without it. + // 3. token == "" → a session denial above is final (issue #173); + // with no session, approvePurchaseViaSession returns 401. if session := h.tryGetSession(ctx, req); session != nil { switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: return h.approvePurchaseViaSession(ctx, req, execution) case fallsThroughToToken(err, token): - // Explicit 403, or an out-of-scope 404 with a token → fall + // Explicit 403 or out-of-scope 404, with a token → fall // through to the token branch so the contact_email gate gets a // chance (a logged-in user without approve-* or outside the // account scope may still be the per-account contact recipient). @@ -1215,18 +1214,16 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // 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. + // 3. token == "" → a session denial above is final (issue #173); + // with no session, cancelPurchaseViaSession returns 401. if session := h.tryGetSession(ctx, req); session != nil { switch err := h.authorizeCancelSession(ctx, session, execution); { case err == nil: // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) case fallsThroughToToken(err, token): - // Explicit "permission denied" (403), or an out-of-scope 404 - // with a token → fall through to the token branch so the + // Explicit "permission denied" (403) or out-of-scope 404, with + // a token → 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). @@ -2264,15 +2261,15 @@ func wrapConstraintDenied(err error) error { } // fallsThroughToToken reports whether a session-authorization failure should -// hand the request to the email-token branch: an RBAC denial (403), or an -// account-scope miss (404) when a token is present. The token branch's -// contact_email gate is deliberately not account-scoped (issue #92). Without -// a token the 404 is returned as-is, so the token branch's status guards -// cannot reveal an out-of-scope execution's existence or status. Both -// matches are strict, like isPermissionDenied, so a wrapped error from -// deeper in the chain propagates. +// hand the request to the email-token branch: an RBAC denial (403) or an +// account-scope miss (404), and only when a token is present. The token +// branch's contact_email gate is deliberately not account-scoped (issue #92). +// Without a token the denial is final (issue #173): nothing else can authorize +// the request, and the 404 must not let later status guards reveal an +// out-of-scope execution. Both matches are strict, like isPermissionDenied, +// so a wrapped error from deeper in the chain propagates. func fallsThroughToToken(err error, token string) bool { - return isPermissionDenied(err) || (token != "" && err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate + return token != "" && (isPermissionDenied(err) || err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate } // isPermissionDenied reports whether err is *directly* a 403 ClientError diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index fa4f6f00..49f3d4cf 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3245,7 +3245,7 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { } session := &Session{UserID: cancelCallerID} - handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, true, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) @@ -3274,7 +3274,7 @@ func TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus(t *testin } session := &Session{UserID: cancelCallerID} - handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, true, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) @@ -5869,9 +5869,9 @@ func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { } // TestHandler_revokePurchase_SessionNoPermissionNoToken verifies that a session -// lacking both cancel-any and cancel-own, with no token supplied, is denied: -// the session branch returns permission-denied and falls through to the -// tokenless 401. Covers the session-auth revoke branch CR finding (PR #889). +// lacking both cancel-any and cancel-own, with no token supplied, gets the +// session branch's 403: without a token the denial is final (issue #173). +// Covers the session-auth revoke branch CR finding (PR #889). func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { ctx := context.Background() execID := "88888888-8888-8888-8888-888888888888" @@ -5899,7 +5899,7 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { require.Error(t, err, "no permission and no token must be denied") ce, ok := IsClientError(err) require.True(t, ok, "expected a client error") - assert.Equal(t, 401, ce.code) + assert.Equal(t, 403, ce.code) mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) } diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 2185af80..bb994faf 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2166,8 +2166,8 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu // 2. token != "" -> legacy email-link flow. validateExchangeApproval enforces // the token-equality check; the permission-denied fall-through ensures a // logged-in user without approve-* can still use an email link they hold. -// 3. token == "" AND no qualifying session -> 403 via -// approveRIExchangeViaSession's requireSession gate. +// 3. token == "": a session denial is returned as-is (issue #173); with no +// session, approveRIExchangeViaSession's requireSession gate returns 401. func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunctionURLRequest, id, token string) (any, error) { if session := h.tryGetSession(ctx, req); session != nil { // Quick RBAC pre-check (no record fetch needed): does this session hold @@ -2193,7 +2193,7 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct if errors.Is(sessErr, errCSRFRejected) || token == "" || !isPermissionDenied(sessErr) { return nil, sessErr } - case isPermissionDenied(err): + case token != "" && isPermissionDenied(err): // Logged-in user without approve-* may still hold a valid email token. default: return nil, err diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 2273a0b1..8d816da8 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -306,10 +306,14 @@ func TestApproveViaSession_RequiresCSRF(t *testing.T) { mockConfig := new(MockConfigStore) exec := &config.PurchaseExecution{ - ExecutionID: execID, - ApprovalToken: "email-tok", - Status: "pending", - Recommendations: []config.RecommendationRecord{{ID: "r1"}}, + ExecutionID: execID, + ApprovalToken: "email-tok", + Status: "pending", + // Non-zero UpfrontCost so the approve-any Constraints check passes and + // the dispatcher hands off to approvePurchaseViaSession (issue #173). + Recommendations: []config.RecommendationRecord{ + {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100}, + }, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -319,7 +323,7 @@ func TestApproveViaSession_RequiresCSRF(t *testing.T) { // Authorization is group-membership-only after issue #907: the session must // pass the approve-* HasPermissionAPI check to reach the CSRF guard, since // the dispatcher authorizes before invoking approvePurchaseViaSession. - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // CSRF token is empty → ValidateCSRFToken must return an error so the // request is rejected. This is the critical regression assertion for #404. mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch")) diff --git a/internal/api/session_denial_final_test.go b/internal/api/session_denial_final_test.go new file mode 100644 index 00000000..ec2f09cf --- /dev/null +++ b/internal/api/session_denial_final_test.go @@ -0,0 +1,143 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #173: without a token a session denial is final. The context principal is an authorized +// admin distinct from the denied bearer session, so a fall-through reaches the mutation. + +const ( + denialTestExecID = "17317317-3173-1731-7317-317317317317" + denialTestUserID = "denied-uid" +) + +var errDenialTestReached = errors.New("mutation reached") + +func newDenialTestHandler(t *testing.T, store *MockConfigStore, mockPurchase *MockPurchaseManager) (*Handler, context.Context) { + t.Helper() + mockAuth := new(MockAuthService) + + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: denialTestUserID, Email: "denied@example.com"}, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, denialTestUserID, mock.Anything, mock.Anything).Return(false, nil) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, denialTestUserID).Return(nil, nil).Maybe() + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "csrf-ok").Return(nil).Maybe() + + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errDenialTestReached).Maybe() + store.On("TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errDenialTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errDenialTestReached).Maybe() + + ctx := contextWithPrincipal(context.Background(), &Principal{ + Kind: PrincipalSession, + Session: &Session{UserID: apiKeyAdminUserID, Email: "admin@example.com"}, + UserID: apiKeyAdminUserID, + }) + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, ctx +} + +func denialTestRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok", "x-csrf-token": "csrf-ok"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + } +} + +func requireForbidden(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code, "got: %v", err) +} + +func TestApprovePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + store, mockPurchase := new(MockConfigStore), new(MockPurchaseManager) + h, ctx := newDenialTestHandler(t, store, mockPurchase) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "pending", + Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "aws", UpfrontCost: 100}}, + }, nil) + + _, err := h.approvePurchase(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires approve-any or approve-own") + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestCancelPurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "pending", + }, nil) + + _, err := h.cancelPurchase(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") + store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestRevokePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "completed", + }, nil) + + _, err := h.revokeViaEmailToken(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") +} + +func TestApproveRIExchange_SessionDenialWithoutTokenIsFinal(t *testing.T) { + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) + store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ + ID: denialTestExecID, + Status: "pending", + ApprovalToken: config.HashApprovalToken("tok"), + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "10.00", + }, nil).Maybe() + + _, err := h.approveRIExchange(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires approve-any or approve-own") + store.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestApproveRIExchange_SessionDenialWithTokenUsesToken(t *testing.T) { + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) + store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ + ID: denialTestExecID, + Status: "pending", + ApprovalToken: config.HashApprovalToken("tok"), + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "10.00", + }, nil) + + _, err := h.approveRIExchange(ctx, denialTestRequest(), denialTestExecID, "tok") + + require.ErrorIs(t, err, errDenialTestReached) + store.AssertCalled(t, "TransitionRIExchangeStatus", mock.Anything, denialTestExecID, "pending", "processing", (*string)(nil)) +}