You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208
newAzureRegionScopedHandler (internal/api/handler_ri_exchange_azure_test.go:1910) hand-writes a closure that re-implements auth.matchAllRegionsConstraint's all-match rule, then feeds its own verdict back to the handler as the authorization answer:
Four tests build their handler through it (:1989, :2024, :2119), and its own docstring is explicit that this is what it does: "emulating auth.matchAllRegionsConstraint's all-match rule". So the region property those tests assert is decided by the test, not by the code that decides it in production. If matchAllRegionsConstraint regressed to containsAny semantics (the exact bug it was written to fix, where a caller permitted only in eastus could attach a westus target and the irreversible exchange would execute for both), these four tests would stay green.
Being precise, because the imprecise version of this claim is easy to dismiss. There are 14 mock.MatchedBy(func(sets []auth.PermissionConstraints) bool) closures across three files. Thirteen of them are not this problem. They assert constraint-set shape and hardcode the verdict, e.g.:
That is a legitimate and valuable assertion: it pins the handler's constraint-set construction, which is real logic worth testing (the non-USD math.MaxFloat64 sentinel, the region union across sources and targets, the unattributed-account fallback). Those should stay as they are.
Only the permits helper re-implements an auth rule and returns it as the decision.
The gap this leaves
Between the two styles, no test in internal/api composes both halves: a constraint set the handler actually built, evaluated by auth's real matcher. The shape tests cover "handler builds the right set" with a hardcoded verdict; LeanerCloud/cloud-commitments-cli#1762's new tests cover "auth's real matcher decides correctly" but with synthetic constraint sets rather than handler-built ones.
Confirm the rewritten tests fail if matchAllRegionsConstraint is regressed to containsAny. They do not today.
Leave the 13 shape-assertion closures alone.
Not in scope
HasAPIKeyPermissionForConstraintsAPI on MockAuthService is still constant-stubbed via allowConstraintChecks, so the user-API-key branch of requirePermissionConstraints (internal/api/handler.go:506-522) has no constraint modelling at all. That is a separate and larger piece: production intersects the key's own permissions with the owning user's group permissions (internal/auth/service_apikeys_api.go:405-411), so the mock needs two permission sets rather than one. Tracking it separately rather than folding it in here.
Summary
newAzureRegionScopedHandler(internal/api/handler_ri_exchange_azure_test.go:1910) hand-writes a closure that re-implementsauth.matchAllRegionsConstraint's all-match rule, then feeds its own verdict back to the handler as the authorization answer:Four tests build their handler through it (
:1989,:2024,:2119), and its own docstring is explicit that this is what it does: "emulatingauth.matchAllRegionsConstraint's all-match rule". So the region property those tests assert is decided by the test, not by the code that decides it in production. IfmatchAllRegionsConstraintregressed tocontainsAnysemantics (the exact bug it was written to fix, where a caller permitted only ineastuscould attach awestustarget and the irreversible exchange would execute for both), these four tests would stay green.Found while implementing LeanerCloud/cloud-commitments-cli#1762.
Scope: narrower than it first looks
Being precise, because the imprecise version of this claim is easy to dismiss. There are 14
mock.MatchedBy(func(sets []auth.PermissionConstraints) bool)closures across three files. Thirteen of them are not this problem. They assert constraint-set shape and hardcode the verdict, e.g.:That is a legitimate and valuable assertion: it pins the handler's constraint-set construction, which is real logic worth testing (the non-USD
math.MaxFloat64sentinel, the region union across sources and targets, the unattributed-account fallback). Those should stay as they are.Only the
permitshelper re-implements an auth rule and returns it as the decision.The gap this leaves
Between the two styles, no test in
internal/apicomposes both halves: a constraint set the handler actually built, evaluated by auth's real matcher. The shape tests cover "handler builds the right set" with a hardcoded verdict; LeanerCloud/cloud-commitments-cli#1762's new tests cover "auth's real matcher decides correctly" but with synthetic constraint sets rather than handler-built ones.Suggested work
newAzureRegionScopedHandlerto grant a constrained permission throughgrantPermissionsand let the mock answer fromauth.PermissionsAllowForConstraintSets(added in fix(test): MockAuthService discards constraint arguments — third divergence, and the justifying comment is now false cloud-commitments-cli#1762), instead of hand-writingpermits. Keepcapturedso the tests can still assert which regions the handler thought the operation touches: that part is a real assertion about handler behaviour.matchAllRegionsConstraintis regressed tocontainsAny. They do not today.Not in scope
HasAPIKeyPermissionForConstraintsAPIonMockAuthServiceis still constant-stubbed viaallowConstraintChecks, so the user-API-key branch ofrequirePermissionConstraints(internal/api/handler.go:506-522) has no constraint modelling at all. That is a separate and larger piece: production intersects the key's own permissions with the owning user's group permissions (internal/auth/service_apikeys_api.go:405-411), so the mock needs two permission sets rather than one. Tracking it separately rather than folding it in here.References
HasPermissionForConstraintsAPIdivergence this was found alongside; addedauth.PermissionsAllowForConstraintSets, the entry point item 1 would usegrantAdmindecision stub