amadan

carlos / platform Public

mail: fix round 2 — non-vacuous region-guard tests, full guard coverage, key-cap positive test

Round-2 review findings, all closed:

- TestEnsureIdentityRefusesAMismatchedRegion was vacuous: it passed
  even with checkRegion's call site deleted, because the bare stub's
  zero-value success carried no DKIM tokens and fix #3's zero-token
  guard fired instead, satisfying "an error came back" for the wrong
  reason. checkRegion now wraps a sentinel (errRegionMismatch), and the
  replacement TestRegionGuardRefusesAMismatch stubs every SES call with
  an unconditional, well-formed success (three DKIM tokens included) so
  the guard is the ONLY thing that can produce an error, and asserts
  errors.Is against the sentinel rather than mere non-nilness. Verified
  by removing all four checkRegion call sites, confirming the new test
  fails on all four (EnsureConfigSet, EnsureIdentity, IdentityStatus,
  DeleteIdentity), and restoring the file — diff against the pre-
  experiment version came back identical.
- Added TestRegionGuardPassesAnEmptyRegionThrough covering the same
  four methods with region="", pinning the deliberate "caller doesn't
  care" escape that nothing exercised before.
- Added TestMemMintKeyEnforcesTheTwoKeyCap: asserts both halves — two
  keys mint successfully (a cap misfiring at two would break ordinary
  rotation) and a third is refused, matching IAM's real per-user limit.
Paul Campbell 47dbfff479dcb2b39310b85556423259e46b4aa9 parent 3af5674
3 files changed, +131 −9
  • internal/mail/aws.go +6 −1
  • internal/mail/aws_test.go +103 −8
  • internal/mail/memprovisioner_test.go +22 −0
diff --git a/internal/mail/aws.go b/internal/mail/aws.go
index c6f4ed505..2e5b933de 100644
--- a/internal/mail/aws.go
+++ b/internal/mail/aws.go
@@ -129,9 +129,14 @@ func NewAWSProvisioner(ctx context.Context, region string) (Provisioner, error)
// CALLER's region while the identity itself was created in a different
// one). An empty region is accepted as "caller doesn't care" and treated as
// this provisioner's own region.
+// errRegionMismatch is the sentinel checkRegion wraps, so a test (or a
+// future caller) can check errors.Is(err, errRegionMismatch) rather than
+// matching against the formatted message.
+var errRegionMismatch = errors.New("mail: region mismatch")
+
func (p *awsProvisioner) checkRegion(region string) error {
if region != "" && region != p.region {
- return fmt.Errorf("mail: provisioner is bound to region %s, cannot act on %s", p.region, region)
+ return fmt.Errorf("mail: provisioner is bound to region %s, cannot act on %s: %w", p.region, region, errRegionMismatch)
}
return nil
}
diff --git a/internal/mail/aws_test.go b/internal/mail/aws_test.go
index 424f92416..ae714ffe9 100644
--- a/internal/mail/aws_test.go
+++ b/internal/mail/aws_test.go
@@ -170,14 +170,109 @@ func TestEnsureIdentityRefusesNoTokens(t *testing.T) {
}
}
-// TestEnsureIdentityRefusesAMismatchedRegion pins the region guard: an
-// awsProvisioner is bound to one client at construction, and a caller naming
-// a different region must be refused loudly rather than silently routed
-// through the wrong client.
-func TestEnsureIdentityRefusesAMismatchedRegion(t *testing.T) {
- p := &awsProvisioner{ses: &stubSES{}, account: "123456789012", region: "eu-west-1"}
- if _, err := p.EnsureIdentity(context.Background(), "us-east-1", "example.com", "mail.example.com"); err == nil {
- t.Fatal("EnsureIdentity accepted a region other than the one the provisioner is bound to")
+// wellFormedIdentityStub returns a stubSES whose CreateEmailIdentity and
+// PutEmailIdentityMailFromAttributes both succeed, with three non-empty
+// DKIM tokens. Used by the region-guard tests below so that success or
+// failure turns ENTIRELY on checkRegion and never on fix #3's zero-token
+// guard — round 1's TestEnsureIdentityRefusesAMismatchedRegion used a bare
+// &stubSES{}, whose zero-value CreateEmailIdentity success carries no
+// DkimAttributes; the zero-token guard fired and satisfied "an error came
+// back" even with checkRegion's call site deleted entirely, so the test
+// passed for the wrong reason. Confirmed empirically below (see the
+// round-2 fix report) by removing the checkRegion call and rerunning.
+func wellFormedIdentityStub() *stubSES {
+ return &stubSES{
+ createEmailIdentity: func(*sesv2.CreateEmailIdentityInput) (*sesv2.CreateEmailIdentityOutput, error) {
+ return &sesv2.CreateEmailIdentityOutput{
+ DkimAttributes: &sesv2types.DkimAttributes{Tokens: []string{"t1", "t2", "t3"}},
+ }, nil
+ },
+ }
+}
+
+// TestRegionGuardRefusesAMismatch covers all four SES-facing methods that
+// take a region parameter (EnsureConfigSet, EnsureIdentity, IdentityStatus,
+// DeleteIdentity): a caller naming a region other than the one the
+// provisioner is bound to must be refused, and the refusal must be
+// checkRegion's — not an error that happens to fire for an unrelated
+// reason. Every stub here answers with an unconditional success for
+// whatever SES call the guard is supposed to prevent from ever being
+// reached, so if the guard is silently removed the call SUCCEEDS instead of
+// merely "erroring somehow" — which is what makes the errors.Is assertion
+// meaningful rather than vacuous.
+func TestRegionGuardRefusesAMismatch(t *testing.T) {
+ const boundRegion = "eu-west-1"
+ const otherRegion = "us-east-1"
+
+ cases := []struct {
+ name string
+ call func(p *awsProvisioner) error
+ }{
+ {"EnsureConfigSet", func(p *awsProvisioner) error {
+ return p.EnsureConfigSet(context.Background(), otherRegion, "cfg", "arn:aws:sns:eu-west-1:123456789012:topic")
+ }},
+ {"EnsureIdentity", func(p *awsProvisioner) error {
+ _, err := p.EnsureIdentity(context.Background(), otherRegion, "example.com", "mail.example.com")
+ return err
+ }},
+ {"IdentityStatus", func(p *awsProvisioner) error {
+ _, err := p.IdentityStatus(context.Background(), otherRegion, "example.com")
+ return err
+ }},
+ {"DeleteIdentity", func(p *awsProvisioner) error {
+ return p.DeleteIdentity(context.Background(), otherRegion, "example.com")
+ }},
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ p := &awsProvisioner{ses: wellFormedIdentityStub(), account: "123456789012", region: boundRegion}
+ err := tc.call(p)
+ if err == nil {
+ t.Fatalf("%s accepted region %q against a provisioner bound to %q", tc.name, otherRegion, boundRegion)
+ }
+ if !errors.Is(err, errRegionMismatch) {
+ t.Fatalf("%s error = %v, want it to wrap errRegionMismatch", tc.name, err)
+ }
+ })
+ }
+}
+
+// TestRegionGuardPassesAnEmptyRegionThrough pins the deliberate escape:
+// checkRegion treats "" as "caller doesn't care" rather than a mismatch.
+// Each case uses a stub that succeeds unconditionally, so a passing test
+// here means the guard did not fire — not that some other stub default
+// happened to swallow an error.
+func TestRegionGuardPassesAnEmptyRegionThrough(t *testing.T) {
+ const boundRegion = "eu-west-1"
+
+ cases := []struct {
+ name string
+ call func(p *awsProvisioner) error
+ }{
+ {"EnsureConfigSet", func(p *awsProvisioner) error {
+ return p.EnsureConfigSet(context.Background(), "", "cfg", "arn:aws:sns:eu-west-1:123456789012:topic")
+ }},
+ {"EnsureIdentity", func(p *awsProvisioner) error {
+ _, err := p.EnsureIdentity(context.Background(), "", "example.com", "mail.example.com")
+ return err
+ }},
+ {"IdentityStatus", func(p *awsProvisioner) error {
+ _, err := p.IdentityStatus(context.Background(), "", "example.com")
+ return err
+ }},
+ {"DeleteIdentity", func(p *awsProvisioner) error {
+ return p.DeleteIdentity(context.Background(), "", "example.com")
+ }},
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ p := &awsProvisioner{ses: wellFormedIdentityStub(), account: "123456789012", region: boundRegion}
+ if err := tc.call(p); err != nil {
+ t.Fatalf("%s with an empty region was refused: %v", tc.name, err)
+ }
+ })
}
}
diff --git a/internal/mail/memprovisioner_test.go b/internal/mail/memprovisioner_test.go
index 7bc936b6b..c9b4a6c58 100644
--- a/internal/mail/memprovisioner_test.go
+++ b/internal/mail/memprovisioner_test.go
@@ -89,6 +89,28 @@ func TestMemReapKeysKeepsOnlyTheNamedOnes(t *testing.T) {
}
}
+// IAM hard-caps every user at two access keys. MintKey must enforce that
+// cap in the fake too, since a rotation walk (Task 13) mints a second key
+// while the first is still live, and a fake that let a third key through
+// would pass a test that fails the first time it runs against real IAM.
+// Both halves matter: the first two must succeed (a cap misapplied at two
+// would break ordinary rotation), and only the third must fail.
+func TestMemMintKeyEnforcesTheTwoKeyCap(t *testing.T) {
+ ctx, p := context.Background(), NewMemProvisioner("1")
+ if _, err := p.EnsureUser(ctx, "u", UserPath, []string{"arn:aws:ses:eu-west-1:1:identity/x.com"}, []string{"x.com"}); err != nil {
+ t.Fatalf("EnsureUser: %v", err)
+ }
+ if _, _, err := p.MintKey(ctx, "u"); err != nil {
+ t.Fatalf("first MintKey: %v, want success", err)
+ }
+ if _, _, err := p.MintKey(ctx, "u"); err != nil {
+ t.Fatalf("second MintKey: %v, want success — the cap must not fire before the third key", err)
+ }
+ if _, _, err := p.MintKey(ctx, "u"); err == nil {
+ t.Fatal("third MintKey succeeded, want IAM's two-key-per-user limit enforced")
+ }
+}
+
// EnsureUser must refuse an ungranted principal rather than create one with
// no policy — an unscoped sending user is the failure this package exists to
// prevent.