aliases: Exists does not need to return the "clean" address

The aliases.Resolver.Exists function currently returns the "clean"
address (with the drop characters and suffixes removed), which is relied
upon in its only caller.

That, however, makes the logic more difficult to follow, hiding some
of the address manipulation behind what should be a read-only check.

So this patch reorganizes that code a little bit, removing the
"cleaning" of the address as part of Exists, and making it explicit when
needed instead.

This patch does not have any user-visible change in behaviour, it is
just internal reorganization.

This is in preparation for further patches which will improve the
handling of some aliases corner cases.

internal/aliases/aliases.go

@@ -151,28 +151,25 @@ func (v *Resolver) Resolve(tr *trace.Trace, addr string) ([]Recipient, error) {
 	return v.resolve(0, addr, tr)
 }
 
-// Exists check that the address exists in the database.
-// It returns the cleaned address, and a boolean indicating the result.
-// The clean address can be used to look it up in other databases, even if it
-// doesn't exist. It must only be called for local addresses.
-func (v *Resolver) Exists(tr *trace.Trace, addr string) (string, bool) {
+// Exists check that the address exists in the database.  It must only be
+// called for local addresses.
+func (v *Resolver) Exists(tr *trace.Trace, addr string) bool {
 	tr = tr.NewChild("Alias.Exists", addr)
 	defer tr.Finish()
 
-	addr = v.cleanIfLocal(addr)
-
+	addr = v.RemoveDropsAndSuffix(addr)
 	rcpts, _ := v.lookup(addr, tr)
 	if len(rcpts) > 0 {
-		return addr, true
+		return true
 	}
 
 	domain := envelope.DomainOf(addr)
 	catchAll, _ := v.lookup("*@"+domain, tr)
 	if len(catchAll) > 0 {
-		return addr, true
+		return true
 	}
 
-	return addr, false
+	return false
 }
 
 func (v *Resolver) lookup(addr string, tr *trace.Trace) ([]Recipient, error) {
@@ -209,7 +206,7 @@ func (v *Resolver) resolve(rcount int, addr string, tr *trace.Trace) ([]Recipien
 	// Drop suffixes and chars to get the "clean" address before resolving.
 	// This also means that we will return the clean version if there's no
 	// match, which our callers can rely upon.
-	addr = v.cleanIfLocal(addr)
+	addr = v.RemoveDropsAndSuffix(addr)
 
 	// Lookup in the aliases database.
 	rcpts, err := v.lookup(addr, tr)
@@ -278,17 +275,8 @@ func (v *Resolver) resolve(rcount int, addr string, tr *trace.Trace) ([]Recipien
 	return ret, nil
 }
 
-func (v *Resolver) cleanIfLocal(addr string) string {
+func (v *Resolver) RemoveDropsAndSuffix(addr string) string {
 	user, domain := envelope.Split(addr)
-
-	v.mu.Lock()
-	isLocal := v.domains[domain]
-	v.mu.Unlock()
-
-	if !isLocal {
-		return addr
-	}
-
 	user = removeAllAfter(user, v.SuffixSep)
 	user = removeChars(user, v.DropChars)
 	user, _ = normalize.User(user)

internal/aliases/aliases_test.go

@@ -41,7 +41,7 @@ func mustExist(t *testing.T, r *Resolver, addrs ...string) {
 	tr := trace.New("test", "mustExist")
 	defer tr.Finish()
 	for _, addr := range addrs {
-		if _, ok := r.Exists(tr, addr); !ok {
+		if ok := r.Exists(tr, addr); !ok {
 			t.Errorf("address %q does not exist, it should", addr)
 		}
 	}
@@ -52,7 +52,7 @@ func mustNotExist(t *testing.T, r *Resolver, addrs ...string) {
 	tr := trace.New("test", "mustNotExist")
 	defer tr.Finish()
 	for _, addr := range addrs {
-		if _, ok := r.Exists(tr, addr); ok {
+		if ok := r.Exists(tr, addr); ok {
 			t.Errorf("address %q exists, it should not", addr)
 		}
 	}
@@ -88,7 +88,7 @@ func TestBasic(t *testing.T) {
 	resolver.aliases = map[string][]Recipient{
 		"a@localA":   {{"c@d", EMAIL}, {"e@localB", EMAIL}},
 		"e@localB":   {{"cmd", PIPE}},
-		"cmd":      {{"x@y", EMAIL}}, // it's a trap!
+		"cmd@localA": {{"x@y", EMAIL}},
 	}
 
 	cases := Cases{
@@ -98,7 +98,7 @@ func TestBasic(t *testing.T) {
 	}
 	cases.check(t, resolver)
 
-	mustExist(t, resolver, "a@localA", "e@localB", "cmd")
+	mustExist(t, resolver, "a@localA", "e@localB", "cmd@localA")
 	mustNotExist(t, resolver, "x@y")
 }
 
@@ -195,7 +195,7 @@ func TestAddrRewrite(t *testing.T) {
 	cases.check(t, resolver)
 }
 
-func TestExistsRewrite(t *testing.T) {
+func TestExists(t *testing.T) {
 	resolver := NewResolver(allUsersExist)
 	resolver.AddDomain("def")
 	resolver.AddDomain("p-q.com")
@@ -207,35 +207,53 @@ func TestExistsRewrite(t *testing.T) {
 	resolver.DropChars = ".~"
 	resolver.SuffixSep = "-+"
 
-	mustExist(t, resolver, "abc@def", "a.bc+blah@def", "ño.ño@def")
-	mustNotExist(t, resolver, "abc@d.ef", "nothere@def")
+	mustExist(t, resolver,
+		"abc@def",
+		"abc+blah@def",
+		"a.bc+blah@def",
+		"a.b~c@def",
+		"ñoño@def",
+		"ño.ño@def",
+		"recu@def",
+		"re.cu@def")
+	mustNotExist(t, resolver,
+		"abc@d.ef",
+		"nothere@def",
+		"ex@def",
+		"a.bc@unknown",
+		"x.yz@def",
+		"x.yz@d.ef",
+		"abc@d.ef")
+}
 
-	tr := trace.New("test", "TestExistsRewrite")
-	defer tr.Finish()
+func TestRemoveDropsAndSuffix(t *testing.T) {
+	resolver := NewResolver(allUsersExist)
+	resolver.AddDomain("def")
+	resolver.AddDomain("p-q.com")
+	resolver.aliases = map[string][]Recipient{
+		"abc@def":  {{"x@y", EMAIL}},
+		"ñoño@def": {{"x@y", EMAIL}},
+		"recu@def": {{"ab+cd@p-q.com", EMAIL}},
+	}
+	resolver.DropChars = ".~"
+	resolver.SuffixSep = "-+"
 
 	cases := []struct {
 		addr string
-		expectAddr   string
-		expectExists bool
+		want string
 	}{
-		{"abc@def", "abc@def", true},
-		{"abc+blah@def", "abc@def", true},
-		{"a.b~c@def", "abc@def", true},
-		{"a.bc+blah@def", "abc@def", true},
-
-		{"a.bc@unknown", "a.bc@unknown", false},
-		{"x.yz@def", "xyz@def", false},
-		{"x.yz@d.ef", "x.yz@d.ef", false},
+		{"abc@def", "abc@def"},
+		{"abc+blah@def", "abc@def"},
+		{"a.b~c@def", "abc@def"},
+		{"a.bc+blah@def", "abc@def"},
+		{"x.yz@def", "xyz@def"},
+		{"x.yz@d.ef", "xyz@d.ef"},
 	}
 	for _, c := range cases {
-		addr, exists := resolver.Exists(tr, c.addr)
-		if addr != c.expectAddr {
-			t.Errorf("%q: expected addr %q, got %q",
-				c.addr, c.expectAddr, addr)
-		}
-		if exists != c.expectExists {
-			t.Errorf("%q: expected exists %v, got %v",
-				c.addr, c.expectExists, exists)
+		addr := resolver.RemoveDropsAndSuffix(c.addr)
+		if addr != c.want {
+			t.Errorf("RemoveDropsAndSuffix(%q): want %q, got %q",
+				c.addr, c.want, addr)
 		}
 	}
 }

internal/smtpsrv/conn.go

@@ -591,7 +591,7 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 			return 550, "5.1.3 Destination address is invalid"
 		}
 
-		ok, err := c.userExists(addr)
+		ok, err := c.localUserExists(addr)
 		if err != nil {
 			c.tr.Errorf("error checking if user %q exists: %v", addr, err)
 			maillog.Rejected(c.remoteAddr, c.mailFrom, []string{addr},
@@ -1096,17 +1096,14 @@ func (c *Conn) resetEnvelope() {
 	c.spfError = nil
 }
 
-func (c *Conn) userExists(addr string) (bool, error) {
-	var ok bool
-	addr, ok = c.aliasesR.Exists(c.tr, addr)
-	if ok {
+func (c *Conn) localUserExists(addr string) (bool, error) {
+	if c.aliasesR.Exists(c.tr, addr) {
 		return true, nil
 	}
 
-	// Note we used the address returned by the aliases resolver, which has
-	// cleaned it up. This means that a check for "us.er@domain" will have us
-	// look up "user" in our databases if the domain is local, which is what
-	// we want.
+	// Remove the drop chars and suffixes, if any, so the database lookup is
+	// on a "clean" address.
+	addr = c.aliasesR.RemoveDropsAndSuffix(addr)
 	user, domain := envelope.Split(addr)
 	return c.authr.Exists(c.tr, user, domain)
 }