Skip to content

test(api): newAzureRegionScopedHandler re-implements the all-match region rule instead of asserting it #208

Description

@cristim

Summary

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:

permits := func(sets []auth.PermissionConstraints) bool {
	*captured = sets
	for _, s := range sets {
		for _, r := range s.Regions {
			if r != permittedRegion {
				return false
			}
		}
	}
	return true
}

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.

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.:

mock.MatchedBy(func(sets []auth.PermissionConstraints) bool {
	return len(sets) == 1 && sets[0].MaxPurchaseAmount == math.MaxFloat64
})).Return(false, nil)

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.

Suggested work

  1. Rewrite newAzureRegionScopedHandler to grant a constrained permission through grantPermissions and let the mock answer from auth.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-writing permits. Keep captured so the tests can still assert which regions the handler thought the operation touches: that part is a real assertion about handler behaviour.
  2. Confirm the rewritten tests fail if matchAllRegionsConstraint is regressed to containsAny. They do not today.
  3. 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.

References

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions