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]