From 931e0315404921360ec0dd03be5a98172a6af692 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 04:40:13 +0200 Subject: [PATCH] test(api): model the constraint dimension in MockAuthService grantPermissionsScoped wired HasPermissionForConstraintsAPI to the same decide(action, resource) closure as HasPermissionAPI, so the SEC-01 execution-time check answered without ever reading the constraint arguments. Measured against the real matchers, the mock allowed requests outside the permission's constraints on all five dimensions: Providers, Regions, Services, AccountIDs and MaxPurchaseAmount, the last of which is the spend guard. A handler test asserting a constraint refusal was green whether or not the handler enforced anything. Extract auth.PermissionsAllowForConstraintSets, the decision half of Service.HasPermissionForConstraintsAPI with the permission fetch removed, and answer the mock from it. permissionsAllow and the four constraint matchers become package functions: they read nothing off the *Service receiver, and the exported entry point needs them without one. Replace the justifying comment. It claimed a principal holding only admin:* satisfies any constraint set for the verbs it holds, which is true and stayed true through #1758: both halves short-circuit on the wildcard before any constraint comparison, so #1758's carve-out only changed which verbs they deny in lockstep. What it never covered is the helper it was attached to, since grantPermissionsScoped takes an arbitrary permission set and a permission carrying Constraints is exactly what the check bounds. Registering a constraint-blind func on that method now panics, so the divergence cannot be reintroduced there silently. Scope: this covers the session branch of requirePermissionConstraints. The user-API-key branch calls HasAPIKeyPermissionForConstraintsAPI, which the mock still stubs to a constant via allowConstraintChecks; modelling it faithfully needs the key-and-owner permission intersection production computes, so it is left for a follow-up. Closes #1762 --- internal/api/grantadmin_carveout_test.go | 164 +++++++++++++++++++++++ internal/api/mocks_test.go | 72 +++++++--- internal/auth/service_api.go | 38 +++++- internal/auth/service_apikeys_api.go | 6 +- internal/auth/service_group.go | 32 +++-- internal/auth/service_group_test.go | 28 ++-- internal/auth/service_user.go | 4 +- 7 files changed, 290 insertions(+), 54 deletions(-) diff --git a/internal/api/grantadmin_carveout_test.go b/internal/api/grantadmin_carveout_test.go index 4059fdc92..b887c3698 100644 --- a/internal/api/grantadmin_carveout_test.go +++ b/internal/api/grantadmin_carveout_test.go @@ -191,3 +191,167 @@ func TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets require.NoError(t, err) assert.True(t, has) } + +// constrainedGrantCase is one permission-side constraint plus a request that +// falls inside it and a request that falls outside it. Holding both directions +// in the same case is deliberate: a mock that refuses everyone passes an +// outside-only assertion, and the pre-fix mock that allowed everyone passes an +// inside-only one. Only the pair distinguishes them. +type constrainedGrantCase struct { + permConstraints *auth.PermissionConstraints + inside auth.PermissionConstraints + outside auth.PermissionConstraints + name string + action string + resource string +} + +// constrainedGrantCases covers all five constraint dimensions. Each case holds +// exactly one permission, so the dimension under test is the only thing that +// can decide the answer. +var constrainedGrantCases = []constrainedGrantCase{ + { + name: "providers", + action: auth.ActionExecute, + resource: auth.ResourceRIExchange, + permConstraints: &auth.PermissionConstraints{Providers: []string{"aws"}}, + inside: auth.PermissionConstraints{Providers: []string{"aws"}}, + outside: auth.PermissionConstraints{Providers: []string{"azure"}}, + }, + { + name: "regions require every requested region", + action: auth.ActionExecute, + resource: auth.ResourceRIExchange, + permConstraints: &auth.PermissionConstraints{Regions: []string{"eastus"}}, + inside: auth.PermissionConstraints{Regions: []string{"eastus"}}, + outside: auth.PermissionConstraints{Regions: []string{"eastus", "westus"}}, + }, + { + name: "services", + action: auth.ActionExecute, + resource: auth.ResourcePurchases, + permConstraints: &auth.PermissionConstraints{Services: []string{"ec2"}}, + inside: auth.PermissionConstraints{Services: []string{"ec2"}}, + outside: auth.PermissionConstraints{Services: []string{"rds"}}, + }, + { + name: "account IDs", + action: auth.ActionExecute, + resource: auth.ResourcePurchases, + permConstraints: &auth.PermissionConstraints{AccountIDs: []string{"acct-a"}}, + inside: auth.PermissionConstraints{AccountIDs: []string{"acct-a"}}, + outside: auth.PermissionConstraints{AccountIDs: []string{"acct-b"}}, + }, + { + name: "max purchase amount is the spend guard", + action: auth.ActionExecute, + resource: auth.ResourcePurchases, + permConstraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}, + inside: auth.PermissionConstraints{MaxPurchaseAmount: 500}, + outside: auth.PermissionConstraints{MaxPurchaseAmount: 5000}, + }, +} + +// TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced is the issue +// #1762 regression barrier: the mock's HasPermissionForConstraintsAPI must +// answer from the constraint arguments, not discard them. +// +// It fails on the pre-fix mock -- which wired that method to a closure taking +// only (action, resource) -- with every `outside` row returning true. Measured +// against auth.PermissionsAllowForConstraintSets before the fix, all five +// dimensions diverged, so a handler test asserting a constraint refusal was +// green whether or not the handler enforced anything. +func TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced(t *testing.T) { + ctx := context.Background() + + for _, tc := range constrainedGrantCases { + t.Run(tc.name, func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.grantPermissions([]auth.Permission{{ + Action: tc.action, Resource: tc.resource, Constraints: tc.permConstraints, + }}) + + has, err := mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", tc.action, tc.resource, + []auth.PermissionConstraints{tc.inside}) + require.NoError(t, err) + assert.True(t, has, "a request inside the permission's constraints must be allowed") + + has, err = mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", tc.action, tc.resource, + []auth.PermissionConstraints{tc.outside}) + require.NoError(t, err) + assert.False(t, has, "a request outside the permission's constraints must be refused") + }) + } +} + +// TestGrantPermissionsScoped_AdminDoesNotWidenAConstrainedCarveOut covers the +// principal shape PR #1758 made reachable: execute:ri-exchange is carved out +// of admin:*, so an administrator reaches it only by ALSO holding an explicit +// grant. Migration 000096 seeds that grant unconstrained, but an operator may +// configure a constrained one, and when they do the admin permission alongside +// it must not widen them back out. +func TestGrantPermissionsScoped_AdminDoesNotWidenAConstrainedCarveOut(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.grantPermissions([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + { + Action: auth.ActionExecute, Resource: auth.ResourceRIExchange, + Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}}, + }, + }) + + has, err := mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourceRIExchange, + []auth.PermissionConstraints{{Providers: []string{"aws"}}}) + require.NoError(t, err) + assert.True(t, has, "the explicit grant covers its own provider") + + has, err = mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourceRIExchange, + []auth.PermissionConstraints{{Providers: []string{"azure"}}}) + require.NoError(t, err) + assert.False(t, has, + "admin:* must not satisfy a carved-out verb the explicit grant constrains to another provider") +} + +// TestRequirePermissionConstraints_ConstrainedGrantIsBoundedAtHandler drives +// the same principal through the handler gate the money paths call, so the +// barrier covers requirePermissionConstraints' consumption of the answer and +// not only the mock's production of it. +func TestRequirePermissionConstraints_ConstrainedGrantIsBoundedAtHandler(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.grantPermissions([]auth.Permission{{ + Action: auth.ActionExecute, + Resource: auth.ResourceRIExchange, + Constraints: &auth.PermissionConstraints{Providers: []string{"aws"}, MaxPurchaseAmount: 1000}, + }}) + h := &Handler{auth: mockAuth} + session := &Session{UserID: "u1"} + + require.NoError(t, + h.requirePermissionConstraints(ctx, session, auth.ResourceRIExchange, []auth.PermissionConstraints{{ + Providers: []string{"aws"}, MaxPurchaseAmount: 500, + }}), + "a request inside every dimension must pass the gate") + + for _, tc := range []struct { + name string + request auth.PermissionConstraints + }{ + {"wrong provider", auth.PermissionConstraints{Providers: []string{"azure"}, MaxPurchaseAmount: 500}}, + {"over the spend cap", auth.PermissionConstraints{Providers: []string{"aws"}, MaxPurchaseAmount: 5000}}, + } { + t.Run(tc.name, func(t *testing.T) { + err := h.requirePermissionConstraints(ctx, session, auth.ResourceRIExchange, + []auth.PermissionConstraints{tc.request}) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got %T: %v", err, err) + assert.Equal(t, 403, ce.code) + assert.Contains(t, ce.Error(), "exceeds the constraints") + }) + } +} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 9f1be1c2f..781b3638b 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -2,7 +2,6 @@ package api import ( "context" - "fmt" "sync" "github.com/LeanerCloud/CUDly/internal/auth" @@ -294,21 +293,41 @@ func permissionDecision(args mock.Arguments, action, resource string) bool { func (m *MockAuthService) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { args := m.Called(ctx, userID, action, resource, constraintSets) - // The decision-function path (registered by grantPermissionsScoped) must - // mirror auth.Service.HasPermissionForConstraintsAPI's fail-closed - // contract: an empty constraintSets is a caller bug, not a grant. Explicit + // The decision-function path (registered by grantPermissionsScoped) reads + // the constraint arguments, so it answers from auth's real matchers rather + // than from a re-statement of their contract. Explicit // mock.On(...).Return(bool, err) expectations for constrained-permission // tests are untouched -- those already encode the intended outcome for // whatever constraintSets the test passes, via permissionDecision below. - if decide, ok := args.Get(0).(func(action, resource string) bool); ok { - if len(constraintSets) == 0 { - return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + if decide, ok := args.Get(0).(constraintDecisionFunc); ok { + if err := args.Error(1); err != nil { + return false, err } - return decide(action, resource), args.Error(1) + return decide(action, resource, constraintSets) + } + // permissionDecision's func(action, resource) bool answers from the verb + // alone. On THIS method that IS the #1762 defect: constraintSets is + // discarded, so every request outside the permission's constraints reads + // as allowed. Reject it here rather than letting a future registration + // reintroduce the divergence silently. Note that this aborts the test + // binary, so a wholesale revert of grantPermissionsScoped's wiring dies + // here instead of reporting the per-dimension failures in + // TestGrantPermissionsScoped_ConstraintDimensionsAreEnforced; the reach + // this adds is over a one-off inline registration, which no test asserts. + if _, blind := args.Get(0).(func(action, resource string) bool); blind { + panic("MockAuthService.HasPermissionForConstraintsAPI: a func(action, resource) bool return " + + "discards the constraint arguments (issue #1762); register a constraintDecisionFunc") } return permissionDecision(args, action, resource), args.Error(1) } +// constraintDecisionFunc is the constraint-AWARE mock return registered by +// grantPermissionsScoped, for HasPermissionForConstraintsAPI only. A named +// type rather than a bare signature so the registration site says which of +// the two decision shapes it means, and so the blind-signature check above +// has something unambiguous to reject. +type constraintDecisionFunc func(action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) + func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { args := m.Called(ctx, userID) return args.Get(0), args.Error(1) @@ -391,10 +410,13 @@ func (m *MockAuthService) grantPermissions(perms []auth.Permission) { m.grantPermissionsScoped(perms, nil) } -// grantPermissionsScoped is the shared implementation. The permission checks -// are answered by auth.AuthContext.HasPermission rather than by a constant, so -// a handler asking for a verb this principal does not hold is DENIED exactly -// as it would be in production. +// grantPermissionsScoped is the shared implementation. Both halves of the +// authorization decision are answered by the real auth code rather than by a +// constant: the verb check by auth.AuthContext.HasPermission, the constraint +// check by auth.PermissionsAllowForConstraintSets. A handler asking for a verb +// this principal does not hold, or for a request outside the Constraints +// configured on the permission that grants it, is DENIED exactly as it would +// be in production. // // .Maybe() is retained: many handlers legitimately return before reaching a // permission check (bad body, bad UUID). Asserting the check happened is a @@ -407,11 +429,29 @@ func (m *MockAuthService) grantPermissionsScoped(perms []auth.Permission, accoun } m.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(decide, nil).Maybe() - // The SEC-01 execution-time constraint check. A principal holding only - // admin:* satisfies any constraint set for the verbs it holds, and holds - // none of the carved-out verbs, so the same decision function applies. + // The SEC-01 execution-time constraint check. This previously reused + // `decide`, which takes only (action, resource) and therefore discarded + // the constraint arguments outright, so the mock answered "allowed" for + // all five constraint dimensions when the request fell OUTSIDE the + // permission's constraints (issue #1762). + // + // The shortcut was justified on the grounds that a principal holding only + // admin:* satisfies any constraint set for the verbs it holds. That is + // true, and stayed true through #1758: both halves short-circuit on the + // admin:* wildcard before reaching any constraint comparison, so for such + // a principal the constraint arguments are ignored whatever Constraints + // the admin permission itself carries, and #1758's carve-out of + // execute:ri-exchange only changed which verbs both DENY in lockstep. + // What the justification never covered is the helper it was attached to. + // grantPermissionsScoped takes an ARBITRARY permission set, and a + // permission carrying Constraints is exactly what the check exists to + // bound. + decideConstrained := constraintDecisionFunc( + func(action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { + return auth.PermissionsAllowForConstraintSets(perms, action, resource, constraintSets) + }) m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(decide, nil).Maybe() + Return(decideConstrained, nil).Maybe() m.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything). Return(accounts, nil).Maybe() } diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 48b78cb38..33816deb4 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -413,17 +413,49 @@ func (s *Service) HasPermissionAPI(ctx context.Context, userID, action, resource // union semantics of group grants). // // Fail closed: an empty constraintSets slice is a caller bug, not a grant - -// it returns an explicit error rather than allowing. +// it returns an explicit error rather than allowing. Checked here as well as +// in PermissionsAllowForConstraintSets so a caller bug never reaches the +// store, which the "empty constraint sets fail loud" case asserts by leaving +// the store mock with no registered expectation. func (s *Service) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []PermissionConstraints) (bool, error) { if len(constraintSets) == 0 { - return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + return false, errNoConstraintSets(action, resource) } permissions, err := s.GetUserPermissions(ctx, userID) if err != nil { return false, err } + return PermissionsAllowForConstraintSets(permissions, action, resource, constraintSets) +} + +// errNoConstraintSets is the shared fail-closed error for an empty +// constraint-set slice, so the three entry points that guard against it +// (this file's two, plus HasAPIKeyPermissionForConstraintsAPI) cannot drift +// apart in wording. +func errNoConstraintSets(action, resource string) error { + return fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) +} + +// PermissionsAllowForConstraintSets is the decision half of +// HasPermissionForConstraintsAPI with the permission fetch removed: it +// reports whether an already-resolved permission set grants action on +// resource for EVERY request-derived constraint set. +// +// Exported so a caller that already holds the effective permission set +// answers from this code rather than from a re-implementation of it. The +// api package's MockAuthService is the caller that motivated it: it modeled +// the permission set faithfully but discarded the constraint arguments +// entirely, so it answered "allowed" for requests on every one of the five +// constraint dimensions that this function denies, and a handler test +// asserting a constraint refusal was measuring the mock (issue #1762). +// +// Fail closed: an empty constraintSets slice is a caller bug, not a grant. +func PermissionsAllowForConstraintSets(permissions []Permission, action, resource string, constraintSets []PermissionConstraints) (bool, error) { + if len(constraintSets) == 0 { + return false, errNoConstraintSets(action, resource) + } for i := range constraintSets { - if !s.permissionsAllow(permissions, action, resource, &constraintSets[i]) { + if !permissionsAllow(permissions, action, resource, &constraintSets[i]) { return false, nil } } diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index 5772dbbdc..fa89e227a 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -377,7 +377,7 @@ func (s *Service) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, re // failure returns an error and callers must deny. func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyID, userID, action, resource string, constraintSets []PermissionConstraints) (bool, error) { if len(constraintSets) == 0 { - return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + return false, errNoConstraintSets(action, resource) } key, err := s.store.GetAPIKeyByID(ctx, keyID) if err != nil { @@ -405,10 +405,10 @@ func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyI // - The key's effective permissions (key's constraint limits, e.g. MaxPurchaseAmount). // - The owner's group permissions (owner's constraint limits). for i := range constraintSets { - if !s.permissionsAllow(perms, action, resource, &constraintSets[i]) { + if !permissionsAllow(perms, action, resource, &constraintSets[i]) { return false, nil } - if !s.permissionsAllow(ownerAuthCtx.Permissions, action, resource, &constraintSets[i]) { + if !permissionsAllow(ownerAuthCtx.Permissions, action, resource, &constraintSets[i]) { return false, nil } } diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index d4b85b7bc..994d22502 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -310,7 +310,7 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st return false, err } - return s.permissionsAllow(permissions, action, resource, constraints), nil + return permissionsAllow(permissions, action, resource, constraints), nil } // permissionsAllow reports whether any permission in the effective set grants @@ -322,7 +322,11 @@ func (s *Service) HasPermission(ctx context.Context, userID, action, resource st // admin:* wildcard grants everything EXCEPT the money-spending verbs in // adminCarvedOuts (separation of duties, issue #923). For those, admin falls // through to the explicit-permission check below instead of short-circuiting. -func (s *Service) permissionsAllow(permissions []Permission, action, resource string, constraints *PermissionConstraints) bool { +// +// 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. +func permissionsAllow(permissions []Permission, action, resource string, constraints *PermissionConstraints) bool { for _, perm := range permissions { if checkAdminPermission(perm) { if adminCarvedOuts[[2]string{action, resource}] { @@ -335,7 +339,7 @@ func (s *Service) permissionsAllow(permissions []Permission, action, resource st continue } - if !checkPermissionConstraints(s, perm, constraints) { + if !checkPermissionConstraints(perm, constraints) { continue } @@ -378,20 +382,20 @@ func checkPermissionMatch(perm Permission, action, resource string) bool { return true } -func checkPermissionConstraints(s *Service, perm Permission, constraints *PermissionConstraints) bool { +func checkPermissionConstraints(perm Permission, constraints *PermissionConstraints) bool { if constraints != nil && perm.Constraints != nil { - return s.matchConstraints(perm.Constraints, constraints) + return matchConstraints(perm.Constraints, constraints) } return true } // matchConstraints checks if permission constraints match request constraints. -func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionConstraints) bool { - return s.matchStringListConstraints(permConstraints.AccountIDs, reqConstraints.AccountIDs) && - s.matchStringListConstraints(permConstraints.Providers, reqConstraints.Providers) && - s.matchStringListConstraints(permConstraints.Services, reqConstraints.Services) && - s.matchAllRegionsConstraint(permConstraints.Regions, reqConstraints.Regions) && - s.matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount) +func matchConstraints(permConstraints, reqConstraints *PermissionConstraints) bool { + return matchStringListConstraints(permConstraints.AccountIDs, reqConstraints.AccountIDs) && + matchStringListConstraints(permConstraints.Providers, reqConstraints.Providers) && + matchStringListConstraints(permConstraints.Services, reqConstraints.Services) && + matchAllRegionsConstraint(permConstraints.Regions, reqConstraints.Regions) && + matchPurchaseAmountConstraint(permConstraints.MaxPurchaseAmount, reqConstraints.MaxPurchaseAmount) } // matchStringListConstraints checks if two string lists have any overlap. @@ -407,7 +411,7 @@ func (s *Service) matchConstraints(permConstraints, reqConstraints *PermissionCo // Callers that need "a constrained permission must only match an explicit // request value" should verify reqList is non-empty before calling, or add // a separate dimension-specific check. -func (s *Service) matchStringListConstraints(permList, reqList []string) bool { +func matchStringListConstraints(permList, reqList []string) bool { if len(permList) > 0 && len(reqList) > 0 { return containsAny(permList, reqList) } @@ -435,7 +439,7 @@ func (s *Service) matchStringListConstraints(permList, reqList []string) bool { // empty-list semantics are unchanged from matchStringListConstraints (an // unconstrained permission, or a request that does not name a region, still // matches). -func (s *Service) matchAllRegionsConstraint(permRegions, reqRegions []string) bool { +func matchAllRegionsConstraint(permRegions, reqRegions []string) bool { if len(permRegions) == 0 || len(reqRegions) == 0 { return true } @@ -452,7 +456,7 @@ func (s *Service) matchAllRegionsConstraint(permRegions, reqRegions []string) bo } // matchPurchaseAmountConstraint checks if requested amount is within permitted limit. -func (s *Service) matchPurchaseAmountConstraint(permMax, reqMax float64) bool { +func matchPurchaseAmountConstraint(permMax, reqMax float64) bool { if permMax > 0 && reqMax > permMax { return false } diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 1fccaf6c7..7ef508d4f 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -1309,12 +1309,10 @@ func TestService_HasPermission_Constraints(t *testing.T) { } func TestMatchConstraints(t *testing.T) { - service := &Service{} - t.Run("all empty constraints match", func(t *testing.T) { permConstraints := &PermissionConstraints{} reqConstraints := &PermissionConstraints{} - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("account IDs match when intersection exists", func(t *testing.T) { @@ -1324,7 +1322,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ AccountIDs: []string{"account-2"}, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("account IDs don't match when no intersection", func(t *testing.T) { @@ -1334,7 +1332,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ AccountIDs: []string{"account-3"}, } - assert.False(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.False(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("providers match when intersection exists", func(t *testing.T) { @@ -1344,7 +1342,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ Providers: []string{"azure"}, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("services match when intersection exists", func(t *testing.T) { @@ -1354,7 +1352,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ Services: []string{"ec2"}, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("regions match when intersection exists", func(t *testing.T) { @@ -1364,7 +1362,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ Regions: []string{"us-east-1"}, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("max purchase amount under limit", func(t *testing.T) { @@ -1374,7 +1372,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ MaxPurchaseAmount: 5000.00, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("max purchase amount over limit", func(t *testing.T) { @@ -1384,7 +1382,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ MaxPurchaseAmount: 15000.00, } - assert.False(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.False(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("max purchase amount at exact limit", func(t *testing.T) { @@ -1394,7 +1392,7 @@ func TestMatchConstraints(t *testing.T) { reqConstraints := &PermissionConstraints{ MaxPurchaseAmount: 10000.00, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("multiple constraint types combined", func(t *testing.T) { @@ -1412,7 +1410,7 @@ func TestMatchConstraints(t *testing.T) { Regions: []string{"us-east-1"}, MaxPurchaseAmount: 5000.00, } - assert.True(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.True(t, matchConstraints(permConstraints, reqConstraints)) }) t.Run("one non-matching constraint fails all", func(t *testing.T) { @@ -1430,7 +1428,7 @@ func TestMatchConstraints(t *testing.T) { Regions: []string{"us-east-1"}, MaxPurchaseAmount: 5000.00, } - assert.False(t, service.matchConstraints(permConstraints, reqConstraints)) + assert.False(t, matchConstraints(permConstraints, reqConstraints)) }) } @@ -1447,8 +1445,6 @@ func TestMatchConstraints(t *testing.T) { // so re-wiring the Regions dimension back to matchStringListConstraints // fails these tests rather than leaving them vacuously green. func TestMatchConstraints_RegionsRequireEveryRequestedRegion(t *testing.T) { - service := &Service{} - tests := []struct { name string permRegions []string @@ -1525,7 +1521,7 @@ func TestMatchConstraints_RegionsRequireEveryRequestedRegion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := service.matchConstraints( + got := matchConstraints( &PermissionConstraints{Regions: tt.permRegions}, &PermissionConstraints{Regions: tt.reqRegions}, ) diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 4951cd797..1fb02bf03 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -443,7 +443,7 @@ func (s *Service) guardSelfEscalation(ctx context.Context, prior, next []string) if err != nil { return fmt.Errorf("failed to verify manage-users permission: %w", err) } - if !s.permissionsAllow(heldBefore, ActionUpdate, ResourceUsers, nil) { + if !permissionsAllow(heldBefore, ActionUpdate, ResourceUsers, nil) { return ErrSelfEscalation } // Holding update:users is NOT enough to hand yourself the money verbs. @@ -551,7 +551,7 @@ func (s *Service) firstUnheldCarvedOut(group *Group, held []Permission) *Permiss if !adminCarvedOuts[[2]string{perm.Action, perm.Resource}] { continue } - if s.permissionsAllow(held, perm.Action, perm.Resource, nil) { + if permissionsAllow(held, perm.Action, perm.Resource, nil) { continue } return &group.Permissions[i]