diff --git a/CHANGELOG.md b/CHANGELOG.md index 5466ca039..01744a46c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/api/group_explicit_zero_cap_test.go b/internal/api/group_explicit_zero_cap_test.go new file mode 100644 index 000000000..a90bbd2d9 --- /dev/null +++ b/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) + }) + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 15a10aeb4..974dc962f 100644 --- a/internal/api/openapi.yaml +++ b/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: diff --git a/internal/auth/explicit_zero_cap_test.go b/internal/auth/explicit_zero_cap_test.go new file mode 100644 index 000000000..654555e18 --- /dev/null +++ b/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)) +} diff --git a/internal/auth/purchase_amount_cap.go b/internal/auth/purchase_amount_cap.go index 4b99b8da0..d01cfbe40 100644 --- a/internal/auth/purchase_amount_cap.go +++ 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) +} diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 9a7c2d236..bcdfa753b 100644 --- a/internal/auth/service_api.go +++ b/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 } diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 2f5dc4cc5..dedf489c5 100644 --- a/internal/auth/service_apikeys_api.go +++ b/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 }