smtp/policy: Store messages with full naming for #33

- Added some tests to make sure stores do not enforce their own naming.
- Improve recipient structured logging.

cmd/inbucket/main.go

@@ -119,7 +119,7 @@ func main() {
 	}
 	msgHub := msghub.New(rootCtx, conf.Web.MonitorHistory)
 	addrPolicy := &policy.Addressing{Config: conf}
-	mmanager := &message.StoreManager{Store: store, Hub: msgHub}
+	mmanager := &message.StoreManager{AddrPolicy: addrPolicy, Store: store, Hub: msgHub}
 	// Start Retention scanner.
 	retentionScanner := storage.NewRetentionScanner(conf.Storage, store, shutdownChan)
 	retentionScanner.Start()

pkg/message/manager.go

@@ -65,6 +65,7 @@ func (s *StoreManager) Deliver(
 			toaddr[i] = &torecip.Address
 		}
 	}
+	log.Debug().Str("module", "message").Str("mailbox", to.Mailbox).Msg("Delivering message")
 	delivery := &Delivery{
 		Meta: Metadata{
 			Mailbox: to.Mailbox,

pkg/policy/address.go

@@ -28,6 +28,9 @@ func (a *Addressing) ExtractMailbox(address string) (string, error) {
 	if a.Config.MailboxNaming == config.LocalNaming {
 		return local, nil
 	}
+	if a.Config.MailboxNaming != config.FullNaming {
+		return "", fmt.Errorf("Unknown MailboxNaming value: %v", a.Config.MailboxNaming)
+	}
 	if domain == "" {
 		return local, nil
 	}
@@ -43,7 +46,7 @@ func (a *Addressing) NewRecipient(address string) (*Recipient, error) {
 	if err != nil {
 		return nil, err
 	}
-	mailbox, err := a.ExtractMailbox(local)
+	mailbox, err := a.ExtractMailbox(address)
 	if err != nil {
 		return nil, err
 	}

pkg/server/smtp/handler.go

@@ -326,28 +326,25 @@ func (s *Session) mailHandler(cmd string, arg string) {
 			s.logger.Warn().Msgf("Bad RCPT argument: %q", arg)
 			return
 		}
-		// This trim is probably too forgiving
 		addr := strings.Trim(arg[3:], "<> ")
 		recip, err := s.addrPolicy.NewRecipient(addr)
 		if err != nil {
 			s.send("501 Bad recipient address syntax")
-			s.logger.Warn().Msgf("Bad address as RCPT arg: %q, %s", addr, err)
+			s.logger.Warn().Str("to", addr).Err(err).Msg("Bad address as RCPT arg")
 			return
 		}
 		if !recip.ShouldAccept() {
-			s.logger.Warn().Str("addr", addr).Msg("Rejecting recipient")
+			s.logger.Warn().Str("to", addr).Msg("Rejecting recipient domain")
 			s.send("550 Relay not permitted")
 			return
 		}
 		if len(s.recipients) >= s.config.MaxRecipients {
-			s.logger.Warn().Msgf("Maximum limit of %v recipients reached",
+			s.logger.Warn().Msgf("Limit of %v recipients exceeded", s.config.MaxRecipients)
-				s.config.MaxRecipients)
+			s.send(fmt.Sprintf("552 Limit of %v recipients exceeded", s.config.MaxRecipients))
-			s.send(fmt.Sprintf("552 Maximum limit of %v recipients reached",
-				s.config.MaxRecipients))
 			return
 		}
 		s.recipients = append(s.recipients, recip)
-		s.logger.Info().Msgf("Recipient: %v", addr)
+		s.logger.Debug().Str("to", addr).Msg("Recipient added")
 		s.send(fmt.Sprintf("250 I'll make sure <%v> gets this", addr))
 		return
 	case "DATA":
@@ -356,15 +353,14 @@ func (s *Session) mailHandler(cmd string, arg string) {
 			s.logger.Warn().Msgf("Got unexpected args on DATA: %q", arg)
 			return
 		}
-		if len(s.recipients) > 0 {
+		if len(s.recipients) == 0 {
-			// We have recipients, go to accept data
-			s.enterState(DATA)
-			return
-		}
 			// DATA out of sequence
 			s.ooSeq(cmd)
 			return
 		}
+		s.enterState(DATA)
+		return
+	}
 	s.ooSeq(cmd)
 }
 

pkg/server/smtp/handler_test.go

@@ -362,6 +362,7 @@ func (m *mockConn) SetWriteDeadline(t time.Time) error { return nil }
 
 func setupSMTPServer(ds storage.Store) (s *Server, buf *bytes.Buffer, teardown func()) {
 	cfg := &config.Root{
+		MailboxNaming: config.FullNaming,
 		SMTP: config.SMTP{
 			Addr:            "127.0.0.1:2500",
 			Domain:          "inbucket.local",

pkg/test/manager.go

@@ -56,7 +56,9 @@ func (m *ManagerStub) GetMetadata(mailbox string) ([]*message.Metadata, error) {
 
 // MailboxForAddress invokes policy.ParseMailboxName.
 func (m *ManagerStub) MailboxForAddress(address string) (string, error) {
-	addrPolicy := &policy.Addressing{Config: &config.Root{}}
+	addrPolicy := &policy.Addressing{Config: &config.Root{
+		MailboxNaming: config.FullNaming,
+	}}
 	return addrPolicy.ExtractMailbox(address)
 }
 

pkg/test/storage_suite.go

@@ -27,6 +27,7 @@ func StoreSuite(t *testing.T, factory StoreFactory) {
 		{"metadata", testMetadata, config.Storage{}},
 		{"content", testContent, config.Storage{}},
 		{"delivery order", testDeliveryOrder, config.Storage{}},
+		{"naming", testNaming, config.Storage{}},
 		{"size", testSize, config.Storage{}},
 		{"seen", testSeen, config.Storage{}},
 		{"delete", testDelete, config.Storage{}},
@@ -191,6 +192,13 @@ func testDeliveryOrder(t *testing.T, store storage.Store) {
 	}
 }
 
+// testNaming ensures the store does not enforce local part mailbox naming.
+func testNaming(t *testing.T, store storage.Store) {
+	DeliverToStore(t, store, "fred@fish.net", "disk #27", time.Now())
+	GetAndCountMessages(t, store, "fred", 0)
+	GetAndCountMessages(t, store, "fred@fish.net", 1)
+}
+
 // testSize verifies message contnet size metadata values.
 func testSize(t *testing.T, store storage.Store) {
 	mailbox := "fred"
@@ -406,7 +414,7 @@ func GetAndCountMessages(t *testing.T, s storage.Store, mailbox string, count in
 		t.Fatalf("Failed to GetMessages for %q: %v", mailbox, err)
 	}
 	if len(msgs) != count {
-		t.Errorf("Got %v messages, want: %v", len(msgs), count)
+		t.Errorf("Got %v messages for %q, want: %v", len(msgs), mailbox, count)
 	}
 	return msgs
 }