fix(auth): reject an explicit zero spend cap on group writes

CHANGELOG.md

@@ -32,6 +32,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- Group writes reject an explicit `max_amount: 0` spend cap with a 400 instead of storing it as no cap; omit the field for no limit. API-key creation over HTTP is not covered yet (#980) (#133)
 - Remove debug console.log from frontend recommendation handler
 - Align pre-commit gocyclo threshold (10) with CI pipeline
 - Pin tool versions in GitHub Actions for reproducible builds

internal/api/group_explicit_zero_cap_test.go

@@ -0,0 +1,93 @@
+package api
+
+// Issue #133: POST/PUT /api/groups reject an explicit max_amount of 0. The
+// handler decodes the body itself, so this drives the real Handler into a real
+// auth.Service (MockStore) to prove the explicit-zero signal survives the
+// handler's JSON decode. The mock-auth handler tests cannot show that.
+
+import (
+	"context"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
+	"github.com/aws/aws-lambda-go/events"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// MockStore aliases auth.MockStore so the mock assertion guard, which only
+// resolves receivers built from a bare type name, can check this file's
+// AssertNotCalled sites.
+type MockStore = auth.MockStore
+
+const zeroCapGroupID = "33333333-3333-3333-3333-333333333333"
+
+// realGroupAuth routes the group writes to a real auth.Service and leaves
+// session and permission checks on the shared mock.
+type realGroupAuth struct {
+	*MockAuthService
+	svc *auth.Service
+}
+
+func (a *realGroupAuth) CreateGroupAPI(ctx context.Context, actor string, req any) (any, error) {
+	return a.svc.CreateGroupAPI(ctx, actor, req)
+}
+
+func (a *realGroupAuth) UpdateGroupAPI(ctx context.Context, actor, id string, req any) (any, error) {
+	return a.svc.UpdateGroupAPI(ctx, actor, id, req)
+}
+
+func TestGroupWritesHTTP_ExplicitZeroSpendCap(t *testing.T) {
+	ctx := context.Background()
+	body := func(constraints string) string {
+		return `{"name":"Team","permissions":[{"action":"view","resource":"plans","constraints":` + constraints + `}]}`
+	}
+	cases := []struct {
+		name        string
+		constraints string
+		wantStatus  int
+	}{
+		{"explicit zero", `{"max_amount":0}`, 400},
+		{"explicit 0.0", `{"max_amount":0.0}`, 400},
+		{"absent", `{"regions":["us-east-1"]}`, 0},
+		{"positive", `{"max_amount":5000}`, 0},
+	}
+	for _, tc := range cases {
+		run := func(t *testing.T, write func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error)) {
+			store := new(MockStore)
+			store.On("GetGroup", mock.Anything, zeroCapGroupID).Return(&auth.Group{ID: zeroCapGroupID, Name: "Team"}, nil).Maybe()
+			store.On("CreateGroup", mock.Anything, mock.Anything).Return(nil).Maybe()
+			store.On("UpdateGroup", mock.Anything, mock.Anything).Return(nil).Maybe()
+			mockAuth := new(MockAuthService)
+			mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: auth.AdminAPIKeyActorID}, nil)
+			mockAuth.grantAdmin()
+			h := &Handler{auth: &realGroupAuth{MockAuthService: mockAuth, svc: auth.NewService(auth.ServiceConfig{Store: store})}}
+			req := &events.LambdaFunctionURLRequest{
+				Headers: map[string]string{"Authorization": "Bearer admin-token"},
+				Body:    body(tc.constraints),
+			}
+
+			_, err := write(h, req)
+
+			if tc.wantStatus == 0 {
+				require.NoError(t, err)
+				return
+			}
+			var ce *clientError
+			require.ErrorAs(t, err, &ce)
+			assert.Equal(t, tc.wantStatus, ce.code)
+			assert.Contains(t, ce.message, "max_amount must be greater than 0")
+			store.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything)
+			store.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+		}
+		t.Run("POST "+tc.name, func(t *testing.T) {
+			run(t, func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) { return h.createGroup(ctx, req) })
+		})
+		t.Run("PUT "+tc.name, func(t *testing.T) {
+			run(t, func(h *Handler, req *events.LambdaFunctionURLRequest) (any, error) {
+				return h.updateGroup(ctx, req, zeroCapGroupID)
+			})
+		})
+	}
+}

internal/api/openapi.yaml

@@ -2722,6 +2722,12 @@ components:
         max_amount:
           type: number
           format: double
+          minimum: 0
+          exclusiveMinimum: true
+          description: >-
+            Spend cap per purchase. Omit for no cap. An explicit 0 is rejected
+            with 400 on group writes (create and update); API-key creation
+            over HTTP does not enforce this yet.
 
     # -- API Keys -----------------------------------------------------------
     CreateAPIKeyRequest:

internal/auth/explicit_zero_cap_test.go

@@ -0,0 +1,111 @@
+package auth
+
+// An explicit max_amount of 0 on a group write is rejected (issue #133). Absent
+// or null still means "no cap". Only wire JSON can say "explicit", so every
+// case here decodes a JSON body the way the HTTP handlers do.
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+func groupBodyWithCap(t *testing.T, constraints string) APICreateGroupRequest {
+	t.Helper()
+	var req APICreateGroupRequest
+	body := `{"name":"Team","permissions":[{"action":"view","resource":"plans","constraints":` + constraints + `}]}`
+	require.NoError(t, json.Unmarshal([]byte(body), &req))
+	return req
+}
+
+func TestGroupWrite_RejectsExplicitZeroMaxAmount(t *testing.T) {
+	ctx := context.Background()
+	for _, raw := range []string{`0`, `0.0`, `-0`, `0e5`} {
+		constraints := `{"max_amount":` + raw + `}`
+
+		t.Run("create "+raw, func(t *testing.T) {
+			mockStore := new(MockStore)
+			svc := newCeilingService(t, mockStore)
+			stubActorPermissionsMaybe(ctx, mockStore, adminOnly)
+			mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe()
+
+			_, err := svc.CreateGroupAPI(ctx, ceilingActorID, groupBodyWithCap(t, constraints))
+
+			require.Error(t, err)
+			assert.ErrorIs(t, err, ErrInvalidPermission)
+			assert.Contains(t, err.Error(), "max_amount must be greater than 0")
+			assert.Contains(t, err.Error(), "entry 0")
+			mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything)
+		})
+
+		t.Run("update "+raw, func(t *testing.T) {
+			mockStore := new(MockStore)
+			svc := newCeilingService(t, mockStore)
+			stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"})
+			stubActorPermissionsMaybe(ctx, mockStore, adminOnly)
+			mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Maybe()
+			var req APIUpdateGroupRequest
+			require.NoError(t, json.Unmarshal([]byte(
+				`{"permissions":[{"action":"view","resource":"plans","constraints":`+constraints+`}]}`), &req))
+
+			_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, req)
+
+			require.Error(t, err)
+			assert.ErrorIs(t, err, ErrInvalidPermission)
+			mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+		})
+	}
+}
+
+// The shared adapter also guards CreateAPIKeyAPI for a caller that decodes
+// auth.APICreateAPIKeyRequest itself. The HTTP key handler does not (#980).
+func TestCreateAPIKeyAPI_RejectsDecodedExplicitZeroMaxAmount(t *testing.T) {
+	var req APICreateAPIKeyRequest
+	require.NoError(t, json.Unmarshal([]byte(
+		`{"name":"k","permissions":[{"action":"view","resource":"plans","constraints":{"max_amount":0}}]}`), &req))
+	svc := newCeilingService(t, new(MockStore))
+
+	_, err := svc.CreateAPIKeyAPI(context.Background(), "user-1", req)
+
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrInvalidPermission)
+}
+
+func TestGroupWrite_AbsentNullAndPositiveMaxAmountAreNotRejected(t *testing.T) {
+	ctx := context.Background()
+	cases := map[string]float64{
+		`{}`:                    0,
+		`{"max_amount":null}`:   0,
+		`{"max_amount":5000}`:   5000,
+		`{"max_amount":1234.5}`: 1234.5,
+	}
+	for constraints, want := range cases {
+		t.Run(constraints, func(t *testing.T) {
+			mockStore := new(MockStore)
+			svc := newCeilingService(t, mockStore)
+			stubActorPermissions(ctx, mockStore, adminOnly)
+			var saved *Group
+			mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).
+				Run(func(args mock.Arguments) { saved, _ = args.Get(1).(*Group) }).Return(nil).Once()
+
+			_, err := svc.CreateGroupAPI(ctx, ceilingActorID, groupBodyWithCap(t, constraints))
+
+			require.NoError(t, err)
+			require.NotNil(t, saved)
+			assert.Equal(t, want, saved.Permissions[0].Constraints.MaxPurchaseAmount)
+		})
+	}
+}
+
+// The response type must not grow a wire field for the decode-only flag.
+func TestAPIPermissionConstraint_MarshalOmitsUnsetCap(t *testing.T) {
+	var c APIPermissionConstraint
+	require.NoError(t, json.Unmarshal([]byte(`{"max_amount":0,"regions":["us-east-1"]}`), &c))
+	b, err := json.Marshal(c)
+	require.NoError(t, err)
+	assert.JSONEq(t, `{"regions":["us-east-1"]}`, string(b))
+}

internal/auth/purchase_amount_cap.go

@@ -47,3 +47,17 @@ func validateMaxPurchaseAmount(permIndex int, p Permission) error {
 		"%w: entry %d (%s:%s) has an invalid max purchase amount %v; a spend cap must be zero (no cap) or positive",
 		ErrInvalidPermission, permIndex, p.Action, p.Resource, p.Constraints.MaxPurchaseAmount)
 }
+
+// validateExplicitZeroCap refuses a permission whose JSON carried an explicit
+// max_amount of 0 (issue #133). Zero stays "no cap" when the field is absent
+// or null, but typing 0 into a cap field reads as "spend nothing" and silently
+// granting unlimited spend is the opposite. Applied to group writes; the HTTP
+// API-key path erases an explicit 0 before it gets here (#980).
+func validateExplicitZeroCap(permIndex int, p APIPermission) error {
+	if p.Constraints == nil || !p.Constraints.maxAmountSet || p.Constraints.MaxAmount != 0 {
+		return nil
+	}
+	return fmt.Errorf(
+		"%w: entry %d (%s:%s) max_amount must be greater than 0; omit it for no limit",
+		ErrInvalidPermission, permIndex, p.Action, p.Resource)
+}

internal/auth/service_api.go

@@ -2,6 +2,7 @@ package auth
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"time"
 )
@@ -56,6 +57,28 @@ type APIPermissionConstraint struct {
 	Services  []string `json:"services,omitempty"`
 	Regions   []string `json:"regions,omitempty"`
 	MaxAmount float64  `json:"max_amount,omitempty"`
+
+	// maxAmountSet records that the decoded JSON carried a non-null
+	// max_amount, so an explicit 0 can be told from an absent field. Only
+	// UnmarshalJSON sets it; a Go-built literal is never "explicit".
+	maxAmountSet bool
+}
+
+// UnmarshalJSON decodes the constraint and records whether max_amount was
+// present and non-null (null counts as absent).
+func (c *APIPermissionConstraint) UnmarshalJSON(b []byte) error {
+	type plain APIPermissionConstraint // no methods, so no recursion
+	var probe struct {
+		MaxAmount *float64 `json:"max_amount"`
+	}
+	if err := json.Unmarshal(b, (*plain)(c)); err != nil {
+		return err
+	}
+	if err := json.Unmarshal(b, &probe); err != nil {
+		return err
+	}
+	c.maxAmountSet = probe.MaxAmount != nil
+	return nil
 }
 
 // APICreateUserRequest is the request type for creating users via API.
@@ -292,15 +315,18 @@ func (s *Service) ChangePasswordAPI(ctx context.Context, userID, currentPassword
 // apiPermissionsToPermissions converts a request-side permission list. A nil
 // result for an empty input preserves the "not sent" signal APIUpdateGroupRequest
 // relies on.
-func apiPermissionsToPermissions(in []APIPermission) []Permission {
+func apiPermissionsToPermissions(in []APIPermission) ([]Permission, error) {
 	if len(in) == 0 {
-		return nil
+		return nil, nil
 	}
 	out := make([]Permission, len(in))
 	for i, p := range in {
+		if err := validateExplicitZeroCap(i, p); err != nil {
+			return nil, err
+		}
 		out[i] = apiPermissionToPermission(p)
 	}
-	return out
+	return out, nil
 }
 
 // CreateGroupAPI creates a new group via the API. actorUserID is the acting
@@ -312,7 +338,10 @@ func (s *Service) CreateGroupAPI(ctx context.Context, actorUserID string, reqInt
 	if !ok {
 		return nil, fmt.Errorf("invalid request type")
 	}
-	perms := apiPermissionsToPermissions(req.Permissions)
+	perms, err := apiPermissionsToPermissions(req.Permissions)
+	if err != nil {
+		return nil, err
+	}
 	// A new group has no permissions yet, so nothing can be "carried over":
 	// pass a nil existing set, which makes every carved-out verb a grant.
 	if err := s.checkGrantCeiling(ctx, actorUserID, perms, nil); err != nil {
@@ -356,6 +385,21 @@ func applyUpdateGroupRequest(group *Group, req APIUpdateGroupRequest, perms []Pe
 	}
 }
 
+// updateRequestPermissions converts an update request's permission list.
+func updateRequestPermissions(req APIUpdateGroupRequest) ([]Permission, error) {
+	// req.Permissions == nil means the field was omitted: leave the group's
+	// permissions unchanged (this endpoint's pre-existing "not sent"
+	// contract). A non-nil-but-empty slice means the client explicitly sent
+	// `"permissions": []` (encoding/json allocates the slice before leaving
+	// it empty, so this is distinguishable from omission after decode),
+	// which this endpoint has no way to honor as "clear all permissions" --
+	// refuse rather than silently no-op it (issue #237).
+	if req.Permissions != nil && len(req.Permissions) == 0 {
+		return nil, ErrEmptyPermissions
+	}
+	return apiPermissionsToPermissions(req.Permissions)
+}
+
 // UpdateGroupAPI updates a group via the API. It refuses to touch a
 // system-managed group at all, and checks any requested permission list
 // against the acting principal's own effective permissions before writing
@@ -377,17 +421,10 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID strin
 		return nil, fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name)
 	}
 
-	// req.Permissions == nil means the field was omitted: leave the group's
-	// permissions unchanged (this endpoint's pre-existing "not sent"
-	// contract). A non-nil-but-empty slice means the client explicitly sent
-	// `"permissions": []` (encoding/json allocates the slice before leaving
-	// it empty, so this is distinguishable from omission after decode),
-	// which this endpoint has no way to honor as "clear all permissions" --
-	// refuse rather than silently no-op it (issue #237).
-	if req.Permissions != nil && len(req.Permissions) == 0 {
-		return nil, ErrEmptyPermissions
+	perms, err := updateRequestPermissions(req)
+	if err != nil {
+		return nil, err
 	}
-	perms := apiPermissionsToPermissions(req.Permissions)
 	if err := s.checkGrantCeiling(ctx, actorUserID, perms, group.Permissions); err != nil {
 		return nil, err
 	}

internal/auth/service_apikeys_api.go

@@ -210,8 +210,12 @@ func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (
 		}
 	}
 
+	perms, err := apiPermissionsToPermissions(createReq.Permissions)
+	if err != nil {
+		return nil, err
+	}
 	// Create the API key
-	apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Password, apiPermissionsToPermissions(createReq.Permissions), createReq.ExpiresAt)
+	apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Password, perms, createReq.ExpiresAt)
 	if err != nil {
 		return nil, err
 	}