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

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/highSignificant harmSignificant harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/internalTeam-internal onlyTeam-internal only

   effort/mDaysDays

   type/bugDefectDefect

   on Aug 8, 2026
2. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   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.
3. cristim commented on Aug 19, 2026

   

   cristim

   on Aug 19, 2026

   MemberAuthor

   More actions

   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.
4. cristim

   closed this as completedin #1859on Aug 19, 2026