fix(test): MockAuthService discards constraint arguments — third divergence, and the justifying comment is now false

[cristim]

Summary

MockAuthService.grantPermissionsScoped (internal/api/mocks_test.go:403-417) wires both HasPermissionAPI and HasPermissionForConstraintsAPI to the same closure:

decide := func(action, resource string) bool {
    return authCtx.HasPermission(action, resource)
}
m.On("HasPermissionAPI", ...).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.
m.On("HasPermissionForConstraintsAPI", ...).Return(decide, nil).Maybe()

decide takes (action, resource) and never looks at the constraint arguments. So the constraint dimension — providers, regions, MaxPurchaseAmount — is not modelled at all: every handler test that appears to exercise it is measuring the mock.

The comment justifying it is now false

The quoted justification holds only while no test principal holds a carved-out verb. PR #1758 introduces exactly such a principal: execute:ri-exchange joins adminCarvedOuts, and membership in the seeded RI Exchanger group (migration 000096) grants it explicitly.

A stale justification is worse than none — the next reader stops at the comment and concludes the shortcut is sound.

Evidence (execution-verified)

Measured against real permissionsAllow in internal/auth, compared to what the mock's decide closure answers for the same principal:

held:    execute:ri-exchange constrained to Providers: ["aws"]
request: Providers: ["azure"]

production (permissionsAllow):  false
mock      (decide closure):     true
divergent:                      true

Production refuses the cross-provider request; the mock allows it. Any handler test asserting a constraint refusal for such a principal is green regardless of whether the handler enforces constraints at all.

This is the third divergence in this mock today — the pattern is the design, not three bugs

#	divergence	status
1	grantAdmin stubbed the authorization decision, answering true for carved-out verbs production denies	fixed in #1744
2	empty constraintSets: production fails closed, the mock answered true	fixed in #1744
3	this one: HasPermissionForConstraintsAPI discards the constraint arguments entirely	open

Three in one day in one mock is a signal about its shape: after #1744 it models the permission set faithfully (via the real AuthContext.HasPermission) and the constraint answer not at all. The two halves of the authorization decision are held to different standards, and the constraint half has no backing implementation.

That changes what a fix should look like. Patching the third case in isolation invites a fourth. The direction that removes the class is to make HasPermissionForConstraintsAPI delegate to the same real code path production uses — Service.permissionsAllow(perms, action, resource, constraints) — so the mock models the principal rather than the answer, exactly as #1744 did for the permission half.

Suggested work

1. Wire HasPermissionForConstraintsAPI to real constraint evaluation rather than to decide(action, resource).
2. Delete or correct the justification comment at mocks_test.go:410-412.
3. Add a regression test that a constrained holder of a carved-out verb is refused a request outside its constraints, and confirm it fails against the current stub.

Expect fallout: handler tests that currently pass because constraints are ignored may start failing, and each such failure is a genuine gap worth triaging rather than papering over.

References

• fix(test): make grantAdmin model the principal instead of stubbing the authorization decision #1744 — fixed divergences 1 and 2 (grantAdmin decision stub)
• fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore #1740 — name-only AssertNotCalled is vacuous on every mock type
• fix(test): grantAdmin authorization-decision stub — remainder after #1744 (pair pinning, .Maybe() per-handler, scoped-account conversion) #1596 — the permission-pair remainder after fix(test): make grantAdmin model the principal instead of stubbing the authorization decision #1744 (distinct dimension: that issue is about which (action, resource) a handler asks for; this one is about the constraint arguments being discarded)
• PR sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group #1758 — introduces the principal that invalidates the current justification

Found during independent adversarial review of PR #1758.

[cristim]

Starting scaffolding for whoever picks this up

I filed this but am handing it off rather than implementing, so here are the baseline rows already measured against the real decision function. Start from these rather than re-deriving — and note that a probe routed through internal/api's grantPermissions reports "allowed" for both rows, which is the bug itself, so any verification has to run in internal/auth against Service.permissionsAllow.

Held permission for all rows:

{Action: ActionExecute, Resource: ResourceRIExchange,
 Constraints: &PermissionConstraints{
     Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 100,
 }}

Measured at f8545146e:

request	permissionsAllow (production)	what MockAuthService answers
azure / eastus / $999999 (out of constraint)	false	true
aws / us-east-1 / $50 (in constraint)	true	true

The mock answers true for both, because decide(action, resource) never sees the constraint arguments. So the mock is right by coincidence on the second row and wrong on the first — which is why a test suite built on it looks healthy.

Both directions the fix needs to preserve are therefore pinned: out-of-constraint must become false, in-constraint must stay true. A fix that simply tightens the mock until the first row passes, without the second, would swap one wrong answer for another.

Reproduce with a throwaway test in internal/auth:

func TestProbeBaseline_ConstraintDirections(t *testing.T) {
	s := &Service{}
	held := []Permission{{
		Action: ActionExecute, Resource: ResourceRIExchange,
		Constraints: &PermissionConstraints{
			Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 100,
		},
	}}
	out := &PermissionConstraints{Providers: []string{"azure"}, Regions: []string{"eastus"}, MaxPurchaseAmount: 999999}
	in := &PermissionConstraints{Providers: []string{"aws"}, Regions: []string{"us-east-1"}, MaxPurchaseAmount: 50}

	t.Logf("out-of-constraint = %v (must be false)", s.permissionsAllow(held, ActionExecute, ResourceRIExchange, out))
	t.Logf("in-constraint     = %v (must be true)",  s.permissionsAllow(held, ActionExecute, ResourceRIExchange, in))

	authCtx := &AuthContext{Permissions: held}
	t.Logf("what the mock answers for BOTH = %v", authCtx.HasPermission(ActionExecute, ResourceRIExchange))
}

One expectation to set before starting: wiring HasPermissionForConstraintsAPI to real constraint evaluation will turn some internal/api handler tests red. Those are not regressions from the fix — they are gaps the mock was hiding, and each needs its own fix-or-file decision. Budget for that triage rather than treating the first red test as a sign the approach is wrong. #1596's remainder is the closest precedent for how large a grantAdmin-adjacent fan-out can get (235 call sites across 26 files for the permission-pair dimension), though the constraint dimension is likely much narrower since far fewer tests pass constraints at all.

[cristim]

Premise correction from implementation

Implemented. The central claim holds and is if anything understated, but the supporting argument about #1758 does not survive measurement, and the "every handler test" characterisation overstates the blast radius. Recording both here so the next reader does not inherit them.

1. The central claim is correct, and broader than stated

Before editing anything I measured real permissionsAllow against the mock's exact decide closure (AuthContext.HasPermission) for the same principal, on unmodified d9d1c1302:

DIVERGENT real=false mock=true   ri-exchange constrained Providers:[aws], request Providers:[azure]
  same    real=true  mock=true   control: same permission, request Providers:[aws]
DIVERGENT real=false mock=true   execute:purchases capped MaxPurchaseAmount:1000, request 5000
  same    real=true  mock=true   control: same cap, request 500
DIVERGENT real=false mock=true   Regions:[eastus] permitted, request Regions:[eastus,westus]
DIVERGENT real=false mock=true   AccountIDs:[acct-a] permitted, request AccountIDs:[acct-b]
DIVERGENT real=false mock=true   Services:[ec2] permitted, request Services:[rds]
  same    real=false mock=false  admin:* alone on execute:ri-exchange
  same    real=true  mock=true   admin:* on a non-carved verb with a constraint
DIVERGENT real=false mock=true   admin:* PLUS constrained ri-exchange, request Providers:[azure]

total cases=10 divergent=6

The issue cites one case. All five constraint dimensions diverge, not just Providers. The money one is the second row: a MaxPurchaseAmount of 1000 did not stop a request for 5000. Divergence is also strictly one-directional; the mock never denied where production allowed, so it was uniformly over-permissive, which is the worse direction for an authorization double.

2. The #1758 argument does not hold

The quoted justification holds only while no test principal holds a carved-out verb. PR #1758 introduces exactly such a principal

It does not falsify the comment, and the measurement shows it directly: row 8 above, admin:* alone on execute:ri-exchange, is real=false mock=false. Same.

The old comment said "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." Adding a pair to adminCarvedOuts makes the second clause more true, not false. For an admin-only principal the two sides are structurally identical: AuthContext.HasPermission (internal/auth/types.go:146) takes no constraint parameter at all, and permissionsAllow (internal/auth/service_group.go:329) short-circuits on checkAdminPermission before checkPermissionConstraints is ever reached. Both continue on a carve-out hit and fall through to false. They agreed before #1758 and agree after. #1758 only changed which verbs they deny in lockstep.

Worth being precise about the mechanism, because it is a security-relevant detail: they ignore the constraint arguments for an admin:* permission unconditionally, not because that permission happens to carry no Constraints. Attaching Constraints to an admin:* grant would not make the constraint check bind.

What actually made the comment false: its premise ("a principal holding only admin:*") describes grantAdmin, but the comment sits on grantPermissionsScoped, the shared implementation grantPermissions also drives with an arbitrary permission set. It was false from the day it was written, for a reason unrelated to #1758.

What #1758 genuinely changed is that it made "admin plus a separate explicit grant on a money verb" a real operator shape, since execute:ri-exchange now needs the RI Exchanger group. Related: migration 000096_seed_ri_exchanger_group.up.sql:48-56 documents that seeded grant as deliberately UNCONSTRAINED. A constrained execute:ri-exchange grant is a legitimate operator configuration, not what #1758 shipped.

3. The divergence was latent, not actively false-greening

every handler test that appears to exercise it is measuring the mock

This overstates it. No pre-existing test changed behaviour when the mock stopped rubber-stamping; all 2170 internal/api tests passed unchanged on the first run after the rewiring.

Verified against d9d1c1302 rather than a working tree: concatenating every internal/api/*_test.go at that commit and grepping for the struct field Constraints: returns 0 hits in the entire file set. No pre-existing api test constructs an auth.Permission carrying Constraints at all. All 259 grant* call sites pass Constraints == nil permissions, for which checkPermissionConstraints returns true unconditionally, so the old and new mock answer identically for every principal any existing test builds.

This was already known and written down. grantadmin_carveout_test.go:176 says: "Harmless today - every current grant helper passes only Constraints == nil permissions, so no test exercises the divergence - but a trap for the first constrained-permission test that reaches HasPermissionForConstraintsAPI through the auto-answering path." Someone reached the same conclusion reviewing #1596.

The trap is real and worth closing. But it was a loaded gun, not a fired one, and describing it as fired invites dismissal by counter-example, which would leave the gun loaded.

4. Scope of what was fixed

The session branch of requirePermissionConstraints only. The user-API-key branch (handler.go:506-522, session.UserAPIKeyID != "") calls HasAPIKeyPermissionForConstraintsAPI, which the mock still stubs to a constant via allowConstraintChecks. That is the same defect one branch over, on the same money paths. Modelling it faithfully needs the key-and-owner permission intersection production computes (service_apikeys_api.go:405-411), which is a distinct change; left for a follow-up.