diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 064ca9001..4ad4c3f6a 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1055,7 +1055,7 @@ func (h *Handler) authorizeScopedSessionApprove(ctx context.Context, session *Se func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { // The stateless admin API key has full access and no user row to resolve // permissions from. Administrators-group users fall through and pass via - // the approve-any HasPermissionAPI check below, since {admin, *} matches + // the approve-any check below, since {admin, *} matches // any requested permission. if session.UserID == apiKeyAdminUserID { return nil @@ -1064,17 +1064,17 @@ func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session, return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasAny { return wrapConstraintDenied(h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveAny, execution.Recommendations)) } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { return NewClientError(403, "permission denied: requires approve-any or approve-own on purchases") @@ -1113,16 +1113,16 @@ func (h *Handler) approveConstraintsForSession(ctx context.Context, session *Ses // session holds (approve-any first, then approve-own). bound is false when no // verb applies to this row, so the caller falls back to the plan cap. func (h *Handler) enforceHeldApproveConstraints(ctx context.Context, session *Session, execution *config.PurchaseExecution) (bound bool, err error) { - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveAny) if err != nil { - return false, fmt.Errorf("permission check failed: %w", err) + return false, err } if hasAny { return true, h.enforcePurchaseConstraints(ctx, session, auth.ActionApproveAny, execution.Recommendations) } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveOwn) if err != nil { - return false, fmt.Errorf("permission check failed: %w", err) + return false, err } // A plan-step row has no creator (plan_step_attach.go), so its approve-own // holder is bound by their Constraints regardless of it. @@ -1401,7 +1401,7 @@ func (h *Handler) requireDifferentApprover(ctx context.Context, session *Session // component is a 500 as per feedback_fail_closed_middleware.md. func (h *Handler) authorizeSessionExecuteDirect(ctx context.Context, session *Session, creatorID string, recs []config.RecommendationRecord) error { // Stateless admin API key: full access, no user row. Administrators-group - // users pass via the execute-any HasPermissionAPI check below, since + // users pass via the execute-any check below, since // {admin, *} matches any requested permission. if session.UserID == apiKeyAdminUserID { return nil @@ -1410,17 +1410,21 @@ func (h *Handler) authorizeSessionExecuteDirect(ctx context.Context, session *Se return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionExecuteAny, auth.ResourcePurchases) + // Both verbs are probed on the principal enforcePurchaseConstraints + // judges: the key (intersected with its owner) for a user API key, so a + // key holding execute-own is not routed to its owner's execute-any and + // then refused on the key's (issue #1005). + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionExecuteAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasAny { return h.enforcePurchaseConstraints(ctx, session, auth.ActionExecuteAny, recs) } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionExecuteOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionExecuteOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { return NewClientError(403, "permission denied: requires execute-any or execute-own on purchases") @@ -1935,7 +1939,7 @@ func (h *Handler) authorizeCancelSession(ctx context.Context, session *Session, // Constraints-checked. Returns a 403 ClientError otherwise. func (h *Handler) authorizeSessionCancel(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { // Stateless admin API key: full access, no user row. Administrators-group - // users pass via the cancel-any HasPermissionAPI check below. + // users pass via the cancel-any check below. if session.UserID == apiKeyAdminUserID { return nil } @@ -1943,9 +1947,9 @@ func (h *Handler) authorizeSessionCancel(ctx context.Context, session *Session, return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionCancelAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionCancelAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } var anyErr error if hasAny { @@ -1962,9 +1966,9 @@ func (h *Handler) authorizeSessionCancel(ctx context.Context, session *Session, // session has no cancel-any); it is returned instead of the generic denials so // a constraint refusal stays terminal. func (h *Handler) authorizeCancelOwn(ctx context.Context, session *Session, execution *config.PurchaseExecution, anyErr error) error { - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionCancelOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionCancelOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { if anyErr != nil { @@ -2519,7 +2523,7 @@ var errRetryLadderTrancheSettled = errors.New("ladder tranche is settled") // non-admins, same as the cancel path). func (h *Handler) authorizeSessionRetry(ctx context.Context, session *Session, execution *config.PurchaseExecution) error { // Stateless admin API key: full access, no user row. Administrators-group - // users pass via the retry-any HasPermissionAPI check below. + // users pass via the retry-any check below. if session.UserID == apiKeyAdminUserID { return nil } @@ -2527,17 +2531,17 @@ func (h *Handler) authorizeSessionRetry(ctx context.Context, session *Session, e return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRetryAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionRetryAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasAny { return nil } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRetryOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionRetryOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { return NewClientError(403, "permission denied: requires retry-any or retry-own on purchases") diff --git a/internal/api/handler_purchases_cancel_constraints_test.go b/internal/api/handler_purchases_cancel_constraints_test.go index d58535f73..e403ba5a7 100644 --- a/internal/api/handler_purchases_cancel_constraints_test.go +++ b/internal/api/handler_purchases_cancel_constraints_test.go @@ -161,8 +161,10 @@ func TestAuthorizeSessionCancel_AdminAndAPIKeys(t *testing.T) { t.Parallel() for _, allowed := range []bool{false, true} { mockAuth := new(MockAuthService) - mockAuth.On("HasPermissionAPI", ctx, revokeConstraintsUser, auth.ActionCancelAny, auth.ResourcePurchases).Return(true, nil) - mockAuth.On("HasPermissionAPI", ctx, revokeConstraintsUser, auth.ActionCancelOwn, auth.ResourcePurchases).Return(false, nil).Maybe() + // The verbs are probed on the key, not the owner (issue #1005). + probe := []auth.PermissionConstraints{{}} + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionCancelAny, auth.ResourcePurchases, probe).Return(true, nil) + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionCancelOwn, auth.ResourcePurchases, probe).Return(false, nil).Maybe() mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionCancelAny, auth.ResourcePurchases, mock.Anything).Return(allowed, nil).Once() h := &Handler{auth: mockAuth, config: &MockConfigStore{}} err := h.authorizeSessionCancel(ctx, &Session{UserID: revokeConstraintsUser, UserAPIKeyID: "key-1"}, exec) diff --git a/internal/api/handler_purchases_owner_key_probe_helpers_test.go b/internal/api/handler_purchases_owner_key_probe_helpers_test.go new file mode 100644 index 000000000..335955338 --- /dev/null +++ b/internal/api/handler_purchases_owner_key_probe_helpers_test.go @@ -0,0 +1,181 @@ +package api //nolint:revive // var-naming: package name "api" is intentional for handler package + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #1005, helper level: each purchase authorization helper must decide +// a user-API-key session's full-scope *-any verb on the KEY (intersected +// with its owner), never on the owner alone, so that the verb check and the +// Constraints check that follows judge the same principal. Apart from +// direct execute, no route hands these helpers a key session today +// (TestSessionOnlyPurchaseRoutes_RefuseAUserAPIKey), so this pins the +// helpers themselves. + +var errOKPUnbound = errors.New("no held approve verb binds this row") + +func okpRecs() []config.RecommendationRecord { + return []config.RecommendationRecord{{ + ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", Count: 1, UpfrontCost: 100, + }} +} + +func okpExecution(creator string) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: okpRowID, Status: "failed", CreatedByUserID: &creator, Recommendations: okpRecs(), + } +} + +type okpHelperCase struct { + name, anyVerb, ownVerb string + // narrow is a key scope holding no full-scope verb; defaults to ownVerb. + narrow []string + call func(h *Handler, s *Session, creator string) error +} + +var okpHelperCases = []okpHelperCase{ + {name: "approve", anyVerb: auth.ActionApproveAny, ownVerb: auth.ActionApproveOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionApprove(context.Background(), s, okpExecution(creator)) + }}, + {name: "approve token constraints", anyVerb: auth.ActionApproveAny, ownVerb: auth.ActionApproveOwn, + call: func(h *Handler, s *Session, creator string) error { + bound, err := h.enforceHeldApproveConstraints(context.Background(), s, okpExecution(creator)) + if err == nil && !bound { + return errOKPUnbound + } + return err + }}, + {name: "execute direct", anyVerb: auth.ActionExecuteAny, ownVerb: auth.ActionExecuteOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionExecuteDirect(context.Background(), s, creator, okpRecs()) + }}, + {name: "cancel", anyVerb: auth.ActionCancelAny, ownVerb: auth.ActionCancelOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionCancel(context.Background(), s, okpExecution(creator)) + }}, + {name: "retry", anyVerb: auth.ActionRetryAny, ownVerb: auth.ActionRetryOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionRetry(context.Background(), s, okpExecution(creator)) + }}, + {name: "revoke scheduled", anyVerb: auth.ActionRevokeAny, ownVerb: auth.ActionRevokeOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionRevokeExecution(context.Background(), s, okpExecution(creator)) + }}, + // History rows carry no creator (ownership is account access, and the + // owner's scope is unrestricted here), so the narrow key holds no revoke + // verb at all and creator is ignored. + {name: "revoke history", anyVerb: auth.ActionRevokeAny, ownVerb: auth.ActionRevokeOwn, narrow: []string{auth.ActionExecute}, + call: func(h *Handler, s *Session, _ string) error { + acct := "acct-1005" + return h.authorizeSessionRevoke(context.Background(), s, &config.PurchaseHistoryRecord{ + PurchaseID: okpRowID, CloudAccountID: &acct, Provider: "azure", Service: "compute", Region: "westeurope", + }) + }}, + // The pre-fetch probe has no row, so the narrow key holds no approve verb. + {name: "ri exchange approve right", anyVerb: auth.ActionApproveAny, ownVerb: auth.ActionApproveOwn, narrow: []string{auth.ActionExecute}, + call: func(h *Handler, s *Session, _ string) error { + return h.sessionHasApproveRight(context.Background(), s) + }}, + {name: "ri exchange approve", anyVerb: auth.ActionApproveAny, ownVerb: auth.ActionApproveOwn, + call: func(h *Handler, s *Session, creator string) error { + return h.authorizeSessionApproveRIExchange(context.Background(), s, &config.RIExchangeRecord{ + ID: okpRowID, Status: "pending", CreatedByUserID: &creator, + }) + }}, +} + +func okpHelperHandler(a AuthServiceInterface) *Handler { + store := new(MockConfigStore) + store.On("ListCloudAccounts", mock.Anything, mock.Anything).Return([]config.CloudAccount{}, nil).Maybe() + return &Handler{auth: a, config: store} +} + +func okpKeySession() *Session { return &Session{UserID: okpOwnerID, UserAPIKeyID: okpKeyID} } + +// TestPurchaseAuthzHelpers_FullScopeFollowsTheAPIKey: the owner holds every +// *-any verb throughout, so a helper that probed the owner would let the +// narrow key through (or wrongly route its own-row request to *-any). +func TestPurchaseAuthzHelpers_FullScopeFollowsTheAPIKey(t *testing.T) { + for _, tc := range okpHelperCases { + narrow := tc.narrow + if narrow == nil { + narrow = []string{tc.ownVerb} + } + t.Run(tc.name+"/narrow key on another user's row is refused", func(t *testing.T) { + err := tc.call(okpHelperHandler(newOKPAuth(narrow...)), okpKeySession(), okpVictimID) + assert.Error(t, err) + }) + t.Run(tc.name+"/key with neither verb on its owner's row is refused", func(t *testing.T) { + err := tc.call(okpHelperHandler(newOKPAuth(auth.ActionExecute)), okpKeySession(), okpOwnerID) + assert.Error(t, err) + }) + t.Run(tc.name+"/key "+tc.ownVerb+" on its owner's row is allowed", func(t *testing.T) { + err := tc.call(okpHelperHandler(newOKPAuth(tc.ownVerb)), okpKeySession(), okpOwnerID) + assert.NoError(t, err) + }) + t.Run(tc.name+"/key "+tc.anyVerb+" on another user's row is allowed", func(t *testing.T) { + err := tc.call(okpHelperHandler(newOKPAuth(tc.anyVerb)), okpKeySession(), okpVictimID) + assert.NoError(t, err) + }) + t.Run(tc.name+"/owner session on another user's row is allowed", func(t *testing.T) { + err := tc.call(okpHelperHandler(newOKPAuth()), &Session{UserID: okpOwnerID, Email: "owner@example.com"}, okpVictimID) + assert.NoError(t, err) + }) + } +} + +// TestPurchaseAuthzHelpers_APIKeyVerbProbeErrorRefuses: an error probing +// the key's *-any verb, or its *-own verb once *-any is not held, is +// returned, never read as "not held". Every other answer allows, including +// the owner's own grants, so a helper that swallowed the error or probed the +// owner would succeed instead. +func TestPurchaseAuthzHelpers_APIKeyVerbProbeErrorRefuses(t *testing.T) { + probe := []auth.PermissionConstraints{{}} + for _, tc := range okpHelperCases { + for _, failing := range []string{tc.anyVerb, tc.ownVerb} { + t.Run(tc.name+"/"+failing, func(t *testing.T) { + boom := errors.New("key store down") + m := new(MockAuthService) + if failing == tc.ownVerb { + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, okpKeyID, okpOwnerID, tc.anyVerb, auth.ResourcePurchases, probe). + Return(false, nil) + } + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, okpKeyID, okpOwnerID, failing, auth.ResourcePurchases, probe). + Return(false, boom) + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, okpKeyID, okpOwnerID, mock.Anything, auth.ResourcePurchases, + mock.Anything).Return(true, nil).Maybe() + m.On("HasPermissionAPI", mock.Anything, okpOwnerID, mock.Anything, auth.ResourcePurchases).Return(true, nil).Maybe() + m.On("GetAllowedAccountsAPI", mock.Anything, okpOwnerID).Return([]string{}, nil).Maybe() + + err := tc.call(okpHelperHandler(m), okpKeySession(), okpOwnerID) + require.ErrorIs(t, err, boom) + }) + } + } +} + +// TestPurchaseAuthzHelpers_AdminAPIKeyUnchanged: the stateless admin API key +// still short-circuits every helper before any permission probe. +func TestPurchaseAuthzHelpers_AdminAPIKeyUnchanged(t *testing.T) { + for _, tc := range okpHelperCases { + if tc.name == "approve token constraints" { + continue // approveConstraintsForSession exempts the admin key before this helper runs + } + t.Run(tc.name, func(t *testing.T) { + m := new(MockAuthService) // no expectations: any probe would panic + err := tc.call(okpHelperHandler(m), &Session{UserID: apiKeyAdminUserID}, okpVictimID) + assert.NoError(t, err) + assert.Empty(t, m.Calls) + }) + } +} diff --git a/internal/api/handler_purchases_owner_key_probe_test.go b/internal/api/handler_purchases_owner_key_probe_test.go new file mode 100644 index 000000000..43495e7aa --- /dev/null +++ b/internal/api/handler_purchases_owner_key_probe_test.go @@ -0,0 +1,266 @@ +package api //nolint:revive // var-naming: package name "api" is intentional for handler package + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "errors" + "net/http" + "testing" + "time" + + "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/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Issue #1005: the purchase authorization helpers asked HasPermissionAPI +// whether the key's OWNER held the full-scope *-any verb, while the +// Constraints that follow are judged on the key itself. These tests drive +// the decision into a real auth.Service backed by auth.MockStore (key +// permissions intersected with the owner's), the same shape #1001 uses for +// the planned-purchase routes. + +const ( + okpOwnerID = "aaaa1005-0000-4000-8000-00000000a005" + okpVictimID = "bbbb1005-0000-4000-8000-00000000b005" + okpRowID = "cccc1005-0000-4000-8000-00000000c005" + okpKeyID = "key-1005" + okpRawKey = "raw-user-key-1005" + okpToken = "owner-session-1005" + okpGroupID = "group-1005" +) + +var errOKPReached = errors.New("action reached") + +// okpAuth sends every permission decision and API-key validation to a real +// auth.Service; session validation, CSRF and account scope stay mocked. +type okpAuth struct { + *MockAuthService + service *auth.Service +} + +func (a *okpAuth) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (any, any, error) { + key, user, err := a.service.ValidateUserAPIKeyAPI(ctx, apiKey) + if err != nil { + return nil, nil, err + } + return key, user, nil +} + +func (a *okpAuth) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID, keyID string, allowed bool, err error) { + return a.service.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) +} + +func (a *okpAuth) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, sets []auth.PermissionConstraints) (bool, error) { + return a.service.HasAPIKeyPermissionForConstraintsAPI(ctx, keyID, userID, action, resource, sets) +} + +func (a *okpAuth) HasPermissionAPI(ctx context.Context, userID, action, resource string) (bool, error) { + return a.service.HasPermissionAPI(ctx, userID, action, resource) +} + +func (a *okpAuth) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, sets []auth.PermissionConstraints) (bool, error) { + return a.service.HasPermissionForConstraintsAPI(ctx, userID, action, resource, sets) +} + +func okpPerms(verbs ...string) []auth.Permission { + perms := make([]auth.Permission, 0, len(verbs)) + for _, v := range verbs { + perms = append(perms, auth.Permission{Action: v, Resource: auth.ResourcePurchases}) + } + return perms +} + +// okpOwnerVerbs is the owner's group: every verb the helpers probe, +// including each full-scope *-any. +var okpOwnerVerbs = []string{ + auth.ActionExecute, auth.ActionExecuteAny, auth.ActionExecuteOwn, + auth.ActionApproveAny, auth.ActionApproveOwn, + auth.ActionCancelAny, auth.ActionCancelOwn, + auth.ActionRetryAny, auth.ActionRetryOwn, + auth.ActionRevokeAny, auth.ActionRevokeOwn, +} + +// newOKPAuth builds the owner (holding okpOwnerVerbs) and a user API key +// scoped to keyVerbs. +func newOKPAuth(keyVerbs ...string) *okpAuth { + sum := sha256.Sum256([]byte(okpRawKey)) + expires := time.Now().Add(24 * time.Hour) + key := &auth.UserAPIKey{ + ID: okpKeyID, UserID: okpOwnerID, KeyHash: base64.RawURLEncoding.EncodeToString(sum[:]), + IsActive: true, ExpiresAt: &expires, Permissions: okpPerms(keyVerbs...), + } + store := new(auth.MockStore) + store.On("GetAPIKeyByHash", mock.Anything, key.KeyHash).Return(key, nil).Maybe() + store.On("GetAPIKeyByID", mock.Anything, okpKeyID).Return(key, nil).Maybe() + store.On("GetUserByID", mock.Anything, okpOwnerID).Return(&auth.User{ + ID: okpOwnerID, Email: "owner@example.com", Active: true, GroupIDs: []string{okpGroupID}, + }, nil).Maybe() + store.On("GetGroup", mock.Anything, okpGroupID).Return(&auth.Group{ + ID: okpGroupID, Permissions: okpPerms(okpOwnerVerbs...), + }, nil).Maybe() + + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, okpToken).Return(&Session{UserID: okpOwnerID, Email: "owner@example.com"}, nil).Maybe() + m.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + m.On("GetAllowedAccountsAPI", mock.Anything, okpOwnerID).Return([]string{}, nil).Maybe() + return &okpAuth{MockAuthService: m, service: auth.NewService(auth.ServiceConfig{Store: store, CSRFKey: auth.TestCSRFKey()})} +} + +func okpKeyHeaders() map[string]string { return map[string]string{"X-API-Key": okpRawKey} } + +func okpOwnerHeaders() map[string]string { + return map[string]string{"Authorization": "Bearer " + okpToken, "X-CSRF-Token": "csrf-1005"} +} + +func okpRequest(method, path, body string, headers map[string]string) *events.LambdaFunctionURLRequest { + headers["Content-Type"] = "application/json" + return &events.LambdaFunctionURLRequest{ + Headers: headers, + Body: body, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: method, Path: path}, + }, + } +} + +// serveOKPExecute posts a direct execute through HandleRequest and reports +// the status and whether the purchase reached DirectExecute (which answers +// errOKPReached, so an allowed request ends in an error after the call). +func serveOKPExecute(t *testing.T, a *okpAuth, headers map[string]string) (int, bool) { + t.Helper() + store := new(MockConfigStore) + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil).Maybe() + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + expectStoredRecs(store, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) + purchaseMgr := new(MockPurchaseManager) + purchaseMgr.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil, "", errOKPReached).Maybe() + + h := &Handler{auth: a, config: store, purchase: purchaseMgr} + resp, err := h.HandleRequest(context.Background(), okpRequest(http.MethodPost, "/api/purchases/execute", directExecRecBody, headers)) + require.NoError(t, err) + require.NotNil(t, resp) + + reached := false + for _, c := range purchaseMgr.Calls { + reached = reached || c.Method == "DirectExecute" + } + if !reached { + purchaseMgr.AssertNotCalled(t, "DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + } + return resp.StatusCode, reached +} + +// TestExecuteDirect_FullScopeFollowsTheAPIKey: direct execute is the one +// site an API-key session reaches (POST /api/purchases/execute accepts +// X-API-Key). The row is always created by the caller, so there is no other +// user's row to reach; the owner-probe bug made a key holding execute-own +// take the execute-any branch whenever its owner held execute-any, and then +// fail the key's execute-any Constraints check. +func TestExecuteDirect_FullScopeFollowsTheAPIKey(t *testing.T) { + t.Run("key without execute-any or execute-own is refused", func(t *testing.T) { + code, reached := serveOKPExecute(t, newOKPAuth(auth.ActionExecute), okpKeyHeaders()) + assert.Equal(t, http.StatusForbidden, code) + assert.False(t, reached, "a refused direct execute must not reach the purchase") + }) + t.Run("key with execute-own executes its own submission", func(t *testing.T) { + _, reached := serveOKPExecute(t, newOKPAuth(auth.ActionExecute, auth.ActionExecuteOwn), okpKeyHeaders()) + assert.True(t, reached, "execute-own on the key must not be judged by the owner's execute-any") + }) + t.Run("key with execute-any executes", func(t *testing.T) { + _, reached := serveOKPExecute(t, newOKPAuth(auth.ActionExecute, auth.ActionExecuteAny), okpKeyHeaders()) + assert.True(t, reached) + }) + t.Run("owner session executes", func(t *testing.T) { + _, reached := serveOKPExecute(t, newOKPAuth(), okpOwnerHeaders()) + assert.True(t, reached) + }) +} + +// TestExecuteDirect_APIKeyFullScopeProbeErrorRefuses: an error probing the +// key's execute-any refuses the request instead of being read as "not held". +func TestExecuteDirect_APIKeyFullScopeProbeErrorRefuses(t *testing.T) { + boom := errors.New("key store down") + store := new(MockConfigStore) + store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + store.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + expectStoredRecs(store, config.RecommendationRecord{ + ID: "rec-1", Provider: "aws", Service: "ec2", Count: 1, Term: 1, Payment: "all-upfront", UpfrontCost: 500, Savings: 100, + }) + purchaseMgr := new(MockPurchaseManager) + purchaseMgr.On("DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil, "", errOKPReached).Maybe() + + m := new(MockAuthService) + m.On("HasAPIKeyPermissionAPI", mock.Anything, okpRawKey, auth.ActionExecute, auth.ResourcePurchases). + Return(okpOwnerID, okpKeyID, true, nil) + m.On("GetAllowedAccountsAPI", mock.Anything, okpOwnerID).Return([]string{}, nil) + probe := []auth.PermissionConstraints{{}} + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, okpKeyID, okpOwnerID, auth.ActionExecuteAny, auth.ResourcePurchases, probe). + Return(false, boom) + // Reached only if the error were swallowed or the owner probed instead: + // every other answer allows, so a regression would execute. + m.On("HasPermissionAPI", mock.Anything, okpOwnerID, mock.Anything, auth.ResourcePurchases).Return(true, nil).Maybe() + m.On("HasAPIKeyPermissionForConstraintsAPI", mock.Anything, okpKeyID, okpOwnerID, mock.Anything, auth.ResourcePurchases, mock.Anything). + Return(true, nil).Maybe() + + h := &Handler{auth: m, config: store, purchase: purchaseMgr} + _, err := h.executePurchase(context.Background(), &events.LambdaFunctionURLRequest{Headers: okpKeyHeaders(), Body: directExecRecBody}) + require.ErrorIs(t, err, boom) + purchaseMgr.AssertNotCalled(t, "DirectExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// TestSessionOnlyPurchaseRoutes_RefuseAUserAPIKey documents why the other +// sites were not exploitable: approve, cancel, email revoke, retry, revoke +// and RI-exchange approve resolve their session through requireSession or +// tryGetSession, which only accept a bearer session (never a user-API-key +// session), so an X-API-Key request is refused before any helper runs, +// whatever the key or its owner holds. The helpers now probe the key anyway +// (see handler_purchases_owner_key_probe_helpers_test.go). +func TestSessionOnlyPurchaseRoutes_RefuseAUserAPIKey(t *testing.T) { + routes := []struct { + name, path, anyVerb, ownVerb string + }{ + {"approve", "/api/purchases/approve/" + okpRowID, auth.ActionApproveAny, auth.ActionApproveOwn}, + {"cancel", "/api/purchases/cancel/" + okpRowID, auth.ActionCancelAny, auth.ActionCancelOwn}, + {"email revoke", "/api/purchases/revoke/" + okpRowID, auth.ActionCancelAny, auth.ActionCancelOwn}, + {"retry", "/api/purchases/retry/" + okpRowID, auth.ActionRetryAny, auth.ActionRetryOwn}, + {"revoke", "/api/purchases/" + okpRowID + "/revoke", auth.ActionRevokeAny, auth.ActionRevokeOwn}, + {"ri exchange approve", "/api/ri-exchange/approve/" + okpRowID, auth.ActionApproveAny, auth.ActionApproveOwn}, + } + for _, rt := range routes { + for _, verb := range []string{rt.ownVerb, rt.anyVerb} { + t.Run(rt.name+"/key "+verb+" on another user's row", func(t *testing.T) { + creator := okpVictimID + store := new(MockConfigStore) + store.On("GetExecutionByID", mock.Anything, okpRowID).Return(&config.PurchaseExecution{ + ExecutionID: okpRowID, Status: "failed", CreatedByUserID: &creator, + }, nil).Maybe() + store.On("GetRIExchangeRecord", mock.Anything, okpRowID).Return(&config.RIExchangeRecord{ + ID: okpRowID, Status: "pending", CreatedByUserID: &creator, + }, nil).Maybe() + purchaseMgr := new(MockPurchaseManager) + + h := &Handler{auth: newOKPAuth(verb), config: store, purchase: purchaseMgr} + resp, err := h.HandleRequest(context.Background(), okpRequest(http.MethodPost, rt.path, "{}", okpKeyHeaders())) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "a user API key is not a session on this route") + assert.Empty(t, purchaseMgr.Calls, "a refused request must not reach the purchase manager") + for _, c := range store.Calls { + assert.Contains(t, []string{"GetExecutionByID", "GetRIExchangeRecord"}, c.Method, "only lookups may run") + } + }) + } + } +} diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index b3947b1fe..a5ae9b027 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -311,9 +311,9 @@ func (h *Handler) authorizeSessionRevokeExecution(ctx context.Context, session * return nil } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionRevokeAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } var anyErr error if hasAny { @@ -323,9 +323,9 @@ func (h *Handler) authorizeSessionRevokeExecution(ctx context.Context, session * } } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionRevokeOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { if anyErr != nil { @@ -406,15 +406,15 @@ func (h *Handler) dispatchProviderRevoke(ctx context.Context, record *config.Pur func (h *Handler) authorizeSessionRevoke(ctx context.Context, session *Session, record *config.PurchaseHistoryRecord) error { // The stateless admin API key has full access and no user row to resolve // permissions from. Administrators-group users fall through and pass via - // the revoke-any HasPermissionAPI check below, since {admin, *} matches + // the revoke-any check below, since {admin, *} matches // any requested permission. if session.UserID == apiKeyAdminUserID { return nil } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionRevokeAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } var anyErr error if hasAny { @@ -424,9 +424,9 @@ func (h *Handler) authorizeSessionRevoke(ctx context.Context, session *Session, } } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionRevokeOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionRevokeOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { if anyErr != nil { diff --git a/internal/api/handler_purchases_revoke_constraints_test.go b/internal/api/handler_purchases_revoke_constraints_test.go index 13f3af546..39c870348 100644 --- a/internal/api/handler_purchases_revoke_constraints_test.go +++ b/internal/api/handler_purchases_revoke_constraints_test.go @@ -181,8 +181,11 @@ func TestAuthorizeSessionRevoke_APIKeySessionUsesKeyConstraints(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) - mockAuth.On("HasPermissionAPI", ctx, revokeConstraintsUser, auth.ActionRevokeAny, auth.ResourcePurchases).Return(true, nil) - mockAuth.On("HasPermissionAPI", ctx, revokeConstraintsUser, auth.ActionRevokeOwn, auth.ResourcePurchases).Return(false, nil) + // The verbs are probed on the key, not the owner (issue #1005): the key + // holds revoke-any but not revoke-own, and its revoke-any Constraints deny. + probe := []auth.PermissionConstraints{{}} + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionRevokeAny, auth.ResourcePurchases, probe).Return(true, nil) + mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionRevokeOwn, auth.ResourcePurchases, probe).Return(false, nil) mockAuth.On("GetAllowedAccountsAPI", ctx, revokeConstraintsUser).Return([]string{}, nil) mockAuth.On("HasAPIKeyPermissionForConstraintsAPI", ctx, "key-1", revokeConstraintsUser, auth.ActionRevokeAny, auth.ResourcePurchases, mock.Anything).Return(false, nil) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 181190fd7..b4f67b75f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2419,23 +2419,23 @@ func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Sess // to approveRIExchangeViaSession before fetching the record. func (h *Handler) sessionHasApproveRight(ctx context.Context, session *Session) error { // Stateless admin API key: full access, no user row. Administrators-group - // users pass via the approve-any HasPermissionAPI check below. + // users pass via the approve-any check below. if session.UserID == apiKeyAdminUserID { return nil } if h.auth == nil { return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasAny { return nil } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasOwn { return nil @@ -2452,7 +2452,7 @@ func (h *Handler) sessionHasApproveRight(ctx context.Context, session *Session) // which prefers reusing the existing verbs to keep the matrix small. func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { // Stateless admin API key: full access, no user row. Administrators-group - // users pass via the approve-any HasPermissionAPI check below. + // users pass via the approve-any check below. if session.UserID == apiKeyAdminUserID { return nil } @@ -2460,17 +2460,17 @@ func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session return NewClientError(500, "authentication service not configured") } - hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveAny, auth.ResourcePurchases) + hasAny, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveAny) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if hasAny { return nil } - hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionApproveOwn, auth.ResourcePurchases) + hasOwn, err := h.sessionHoldsVerb(ctx, session, auth.ActionApproveOwn) if err != nil { - return fmt.Errorf("permission check failed: %w", err) + return err } if !hasOwn { return NewClientError(403, "permission denied: requires approve-any or approve-own on purchases")