From dcb8f58d2564b4b7f52d5c2f6523ffc831d92abf Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 11:52:24 +0100 Subject: [PATCH] fix(auth): enforce constraints on an admin-scoped permission on every check permissionsAllow applied the Constraints of an {admin, *} permission only when the request set StrictScope. A key created as {admin, *} with accounts=[A] or a max_amount therefore passed every non-strict constraint check (requirePermissionConstraints -> HasAPIKeyPermissionForConstraintsAPI) for account B or for any amount, so the scoping its creator asked for was silently ignored. The owner gate still bounded it, but an Administrators owner holds an unconstrained {admin, *} and passes everything. An admin permission's Constraints now narrow it on every check, the same as any other permission. Unconstrained admin grants (the seeded Administrators group) and the money-verb carve-outs are unchanged. Closes #956 Co-Authored-By: Claude Opus 5.5 --- internal/auth/admin_constraints_test.go | 126 ++++++++++++++++++ internal/auth/service_apikeys.go | 7 + internal/auth/service_group.go | 10 +- .../server/apikey_admin_constraints_test.go | 73 ++++++++++ 4 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 internal/auth/admin_constraints_test.go create mode 100644 internal/server/apikey_admin_constraints_test.go diff --git a/internal/auth/admin_constraints_test.go b/internal/auth/admin_constraints_test.go new file mode 100644 index 000000000..b17dbf9b8 --- /dev/null +++ b/internal/auth/admin_constraints_test.go @@ -0,0 +1,126 @@ +package auth + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #956: an {admin, *} permission that carries Constraints must be +// narrowed by them on every check, not only under StrictScope. Before the +// fix, permissionsAllow applied an admin permission's Constraints only when +// the request set StrictScope, so a key created as {admin, *} with +// accounts=[A] or a max_amount passed every non-strict check for account B +// or for any amount. + +func constrainedAdmin(c *PermissionConstraints) Permission { + return Permission{Action: ActionAdmin, Resource: ResourceAll, Constraints: c} +} + +func TestPermissionsAllow_ConstrainedAdminNonStrict(t *testing.T) { + t.Parallel() + held := constrainedAdmin(&PermissionConstraints{ + AccountIDs: []string{"acct-a"}, Providers: []string{"aws"}, Services: []string{"ec2"}, + Regions: []string{"us-east-1"}, MaxPurchaseAmount: 500, + }) + cases := []struct { + name string + req PermissionConstraints + want bool + }{ + {"other account denied", PermissionConstraints{AccountIDs: []string{"acct-b"}}, false}, + {"own account allowed", PermissionConstraints{AccountIDs: []string{"acct-a"}}, true}, + {"amount over cap denied", PermissionConstraints{MaxPurchaseAmount: 501}, false}, + {"amount at cap allowed", PermissionConstraints{MaxPurchaseAmount: 500}, true}, + {"other provider denied", PermissionConstraints{Providers: []string{"azure"}}, false}, + {"other service denied", PermissionConstraints{Services: []string{"rds"}}, false}, + {"other region denied", PermissionConstraints{Regions: []string{"eu-west-1"}}, false}, + {"full in-scope request allowed", PermissionConstraints{ + AccountIDs: []string{"acct-a"}, Providers: []string{"aws"}, Services: []string{"ec2"}, + Regions: []string{"us-east-1"}, MaxPurchaseAmount: 100, + }, true}, + // The empty probe sessionHoldsVerb sends must still report the verb + // as held: an empty request dimension is "not specified". + {"empty holds-verb probe allowed", PermissionConstraints{}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := PermissionsAllowForConstraintSets([]Permission{held}, ActionExecuteAny, ResourcePurchases, []PermissionConstraints{tc.req}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestPermissionsAllow_AdminWithoutConstraintsUnchanged(t *testing.T) { + t.Parallel() + admin := []Permission{{Action: ActionAdmin, Resource: ResourceAll}} + req := &PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 1e6} + assert.True(t, permissionsAllow(admin, ActionExecuteAny, ResourcePurchases, req)) + assert.True(t, permissionsAllow(admin, ActionUpdate, ResourceConfig, nil)) + // Carve-outs still apply to an admin permission, constrained or not. + assert.False(t, permissionsAllow(admin, ActionExecute, ResourcePurchases, req)) + constrained := []Permission{constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}})} + assert.False(t, permissionsAllow(constrained, ActionExecute, ResourcePurchases, &PermissionConstraints{AccountIDs: []string{"acct-a"}})) +} + +// A constrained admin grant is narrowed even when another grant in the set +// matches the action: the decision is per permission, and an in-scope +// explicit grant still allows. +func TestPermissionsAllow_ConstrainedAdminAlongsideExplicitGrant(t *testing.T) { + t.Parallel() + perms := []Permission{ + constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}}), + {Action: ActionUpdate, Resource: ResourceConfig, Constraints: &PermissionConstraints{AccountIDs: []string{"acct-c"}}}, + } + assert.False(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-b"}})) + assert.True(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-a"}})) + assert.True(t, permissionsAllow(perms, ActionUpdate, ResourceConfig, &PermissionConstraints{AccountIDs: []string{"acct-c"}})) +} + +// End to end through the service: an Administrators-group owner holds an +// unconstrained {admin, *}, so the owner gate passes everything; only the +// key's own constraints can refuse account B or an amount over the cap. +func TestHasAPIKeyPermissionForConstraintsAPI_ConstrainedAdminKey(t *testing.T) { + key := effKey(constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 500})) + ownerPerms := []Permission{{Action: ActionAdmin, Resource: ResourceAll}} + cases := []struct { + name string + req PermissionConstraints + want bool + }{ + {"account B refused", PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 100}, false}, + {"amount over key cap refused", PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 1000}, false}, + {"in scope allowed", PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 100}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := &Service{store: effStore(t, key, effOwner(), ownerPerms)} + got, err := s.HasAPIKeyPermissionForConstraintsAPI(context.Background(), effKeyID, effOwnerID, + ActionExecuteAny, ResourcePurchases, []PermissionConstraints{tc.req}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +// The same rule holds for a group-granted constrained admin permission on the +// session path (HasPermissionForConstraintsAPI). +func TestHasPermissionForConstraintsAPI_ConstrainedAdminGroup(t *testing.T) { + groupPerms := []Permission{constrainedAdmin(&PermissionConstraints{AccountIDs: []string{"acct-a"}})} + s := &Service{store: effStore(t, nil, effOwner(), groupPerms)} + ctx := context.Background() + + got, err := s.HasPermissionForConstraintsAPI(ctx, effOwnerID, ActionUpdate, ResourceConfig, + []PermissionConstraints{{AccountIDs: []string{"acct-b"}}}) + require.NoError(t, err) + assert.False(t, got, "a group's {admin, *} constrained to acct-a must not authorize acct-b") + + got, err = s.HasPermissionForConstraintsAPI(ctx, effOwnerID, ActionUpdate, ResourceConfig, + []PermissionConstraints{{AccountIDs: []string{"acct-a"}}}) + require.NoError(t, err) + assert.True(t, got) +} diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go index e097223aa..03bde2bf8 100644 --- a/internal/auth/service_apikeys.go +++ b/internal/auth/service_apikeys.go @@ -182,6 +182,13 @@ func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string, // user's permissions. Administrators-group members hold {admin, *}, so the // per-permission HasPermission check below already passes for every requested // permission; no role-based short-circuit is needed. +// +// Only action and resource are checked here. A key permission's Constraints +// are not compared with the owner's at creation; they are enforced at check +// time, where HasAPIKeyPermissionForConstraintsAPI requires every request to +// pass both the key's constraints and the owner's. That holds for an +// {admin, *} key permission too: permissionsAllow applies its Constraints on +// every check, not only under StrictScope (issue #956). func (s *Service) validateAPIKeyPermissions(ctx context.Context, user *User, permissions []Permission) error { // Get user's auth context to check their permissions authCtx, err := s.GetAuthContext(ctx, user.ID) diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 5cc3f618c..bb6cb9919 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -323,6 +323,14 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st // adminCarvedOuts (separation of duties, issue #923). For those, admin falls // through to the explicit-permission check below instead of short-circuiting. // +// An admin:* permission that carries Constraints is narrowed by them on every +// check, strict or not, exactly like any other permission (issue #956). It +// used to apply them only under StrictScope, so a key created as {admin, *} +// with accounts=[A] or a max_amount passed a non-strict check for account B +// or for any amount: the scoping its creator asked for was silently ignored. +// An unconstrained admin:* (the seeded Administrators group) is unaffected, +// since checkPermissionConstraints passes a permission with no Constraints. +// // A package function rather than a Service method: the decision reads nothing // off the receiver, and PermissionsAllowForConstraintSets exposes it to // callers that hold an already-resolved permission set but no Service. @@ -332,7 +340,7 @@ func permissionsAllow(permissions []Permission, action, resource string, constra if coversCarvedOut(Permission{Action: action, Resource: resource}) { continue } - if constraints != nil && constraints.StrictScope && !checkPermissionConstraints(perm, constraints) { + if !checkPermissionConstraints(perm, constraints) { continue } return true diff --git a/internal/server/apikey_admin_constraints_test.go b/internal/server/apikey_admin_constraints_test.go new file mode 100644 index 000000000..1e115b451 --- /dev/null +++ b/internal/server/apikey_admin_constraints_test.go @@ -0,0 +1,73 @@ +package server + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #956: a key created through the API as {admin, *} with accounts=[A] +// and a max_amount keeps those constraints on create (#957), and they must +// also be enforced on the non-strict constraint check the API layer runs for +// user-API-key sessions (requirePermissionConstraints -> +// HasAPIKeyPermissionForConstraintsAPI). The owner is an Administrators +// member with an unconstrained {admin, *}, so the owner gate passes +// everything and only the key's own constraints can refuse the request. +func TestAuthServiceAdapter_ConstrainedAdminKeyRefusedOutOfScope(t *testing.T) { + adapter, mockStore := createMockAuthService(t) + ctx := context.Background() + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + const ownerPass = "CorrectHorse@123" // throwaway fixture credential + owner := &auth.User{ + ID: "user-1", Email: "test@example.com", Active: true, + PasswordHash: auth.TestPasswordHash(t, ownerPass), + GroupIDs: []string{auth.DefaultAdminGroupID}, + } + mockStore.On("GetUserByID", ctx, "user-1").Return(owner, nil) + mockStore.On("GetGroup", ctx, auth.DefaultAdminGroupID).Return(&auth.Group{ + ID: auth.DefaultAdminGroupID, + Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + }, nil) + var stored *auth.UserAPIKey + mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")). + Run(func(args mock.Arguments) { stored = args.Get(1).(*auth.UserAPIKey) }).Return(nil) + + expiresAt := time.Now().Add(24 * time.Hour) + _, err := adapter.CreateAPIKeyAPI(ctx, "user-1", api.CreateAPIKeyRequest{ + Name: "admin-scoped", + Password: ownerPass, + ExpiresAt: &expiresAt, + Permissions: []api.Permission{{ + Action: auth.ActionAdmin, Resource: auth.ResourceAll, + Constraints: &api.PermissionConstraint{Accounts: []string{"acct-a"}, MaxAmount: 500}, + }}, + }) + require.NoError(t, err) + require.NotNil(t, stored) + mockStore.On("GetAPIKeyByID", ctx, stored.ID).Return(stored, nil) + + cases := []struct { + name string + req auth.PermissionConstraints + want bool + }{ + {"account B refused", auth.PermissionConstraints{AccountIDs: []string{"acct-b"}, MaxPurchaseAmount: 100}, false}, + {"amount over key cap refused", auth.PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 501}, false}, + {"in scope allowed", auth.PermissionConstraints{AccountIDs: []string{"acct-a"}, MaxPurchaseAmount: 500}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := adapter.HasAPIKeyPermissionForConstraintsAPI(ctx, stored.ID, "user-1", + auth.ActionExecuteAny, auth.ResourcePurchases, []auth.PermissionConstraints{tc.req}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +}