Merge branch 'feature/fullemail-33' for #33

cmd/inbucket/main.go

@@ -118,8 +118,8 @@ func main() {
 		startupLog.Fatal().Err(err).Str("module", "storage").Msg("Fatal storage error")
 	}
 	msgHub := msghub.New(rootCtx, conf.Web.MonitorHistory)
-	addrPolicy := &policy.Addressing{Config: conf.SMTP}
-	mmanager := &message.StoreManager{Store: store, Hub: msgHub}
+	addrPolicy := &policy.Addressing{Config: conf}
+	mmanager := &message.StoreManager{AddrPolicy: addrPolicy, Store: store, Hub: msgHub}
 	// Start Retention scanner.
 	retentionScanner := storage.NewRetentionScanner(conf.Storage, store, shutdownChan)
 	retentionScanner.Start()

pkg/config/config.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"fmt"
 	"log"
 	"os"
 	"strings"
@@ -29,13 +30,37 @@ var (
 	BuildDate = ""
 )
 
-// Root wraps all other configurations.
+// mbNaming represents a mailbox naming strategy.
+type mbNaming int
+
+// Mailbox naming strategies.
+const (
+	UnknownNaming mbNaming = iota
+	LocalNaming
+	FullNaming
+)
+
+// Decode a naming strategy from string.
+func (n *mbNaming) Decode(v string) error {
+	switch strings.ToLower(v) {
+	case "local":
+		*n = LocalNaming
+	case "full":
+		*n = FullNaming
+	default:
+		return fmt.Errorf("Unknown MailboxNaming strategy: %q", v)
+	}
+	return nil
+}
+
+// Root contains global configuration, and structs with for specific sub-systems.
 type Root struct {
-	LogLevel string `required:"true" default:"info" desc:"debug, info, warn, or error"`
-	SMTP     SMTP
-	POP3     POP3
-	Web      Web
-	Storage  Storage
+	LogLevel      string   `required:"true" default:"info" desc:"debug, info, warn, or error"`
+	MailboxNaming mbNaming `required:"true" default:"local" desc:"local or full"`
+	SMTP          SMTP
+	POP3          POP3
+	Web           Web
+	Storage       Storage
 }
 
 // SMTP contains the SMTP server configuration.

pkg/message/manager.go

@@ -35,8 +35,9 @@ type Manager interface {
 
 // StoreManager is a message Manager backed by the storage.Store.
 type StoreManager struct {
-	Store storage.Store
-	Hub   *msghub.Hub
+	AddrPolicy *policy.Addressing
+	Store      storage.Store
+	Hub        *msghub.Hub
 }
 
 // Deliver submits a new message to the store.
@@ -64,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,
@@ -110,7 +112,7 @@ func (s *StoreManager) GetMetadata(mailbox string) ([]*Metadata, error) {
 // GetMessage returns the specified message.
 func (s *StoreManager) GetMessage(mailbox, id string) (*Message, error) {
 	sm, err := s.Store.GetMessage(mailbox, id)
-	if err != nil {
+	if err != nil || sm == nil {
 		return nil, err
 	}
 	r, err := sm.Source()
@@ -146,7 +148,7 @@ func (s *StoreManager) RemoveMessage(mailbox, id string) error {
 // SourceReader allows the stored message source to be read.
 func (s *StoreManager) SourceReader(mailbox, id string) (io.ReadCloser, error) {
 	sm, err := s.Store.GetMessage(mailbox, id)
-	if err != nil {
+	if err != nil || sm == nil {
 		return nil, err
 	}
 	return sm.Source()
@@ -154,7 +156,7 @@ func (s *StoreManager) SourceReader(mailbox, id string) (io.ReadCloser, error) {
 
 // MailboxForAddress parses an email address to return the canonical mailbox name.
 func (s *StoreManager) MailboxForAddress(mailbox string) (string, error) {
-	return policy.ParseMailboxName(mailbox)
+	return s.AddrPolicy.ExtractMailbox(mailbox)
 }
 
 // makeMetadata populates Metadata from a storage.Message.

pkg/policy/address.go

@@ -12,7 +12,32 @@ import (
 
 // Addressing handles email address policy.
 type Addressing struct {
-	Config config.SMTP
+	Config *config.Root
+}
+
+// ExtractMailbox extracts the mailbox name from a partial email address.
+func (a *Addressing) ExtractMailbox(address string) (string, error) {
+	local, domain, err := parseEmailAddress(address)
+	if err != nil {
+		return "", err
+	}
+	local, err = parseMailboxName(local)
+	if err != nil {
+		return "", err
+	}
+	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
+	}
+	if !ValidateDomainPart(domain) {
+		return "", fmt.Errorf("Domain part %q in %q failed validation", domain, address)
+	}
+	return local + "@" + domain, nil
 }
 
 // NewRecipient parses an address into a Recipient.
@@ -21,7 +46,7 @@ func (a *Addressing) NewRecipient(address string) (*Recipient, error) {
 	if err != nil {
 		return nil, err
 	}
-	mailbox, err := ParseMailboxName(local)
+	mailbox, err := a.ExtractMailbox(address)
 	if err != nil {
 		return nil, err
 	}
@@ -41,10 +66,12 @@ func (a *Addressing) NewRecipient(address string) (*Recipient, error) {
 // ShouldAcceptDomain indicates if Inbucket accepts mail destined for the specified domain.
 func (a *Addressing) ShouldAcceptDomain(domain string) bool {
 	domain = strings.ToLower(domain)
-	if a.Config.DefaultAccept && !stringutil.SliceContains(a.Config.RejectDomains, domain) {
+	if a.Config.SMTP.DefaultAccept &&
+		!stringutil.SliceContains(a.Config.SMTP.RejectDomains, domain) {
 		return true
 	}
-	if !a.Config.DefaultAccept && stringutil.SliceContains(a.Config.AcceptDomains, domain) {
+	if !a.Config.SMTP.DefaultAccept &&
+		stringutil.SliceContains(a.Config.SMTP.AcceptDomains, domain) {
 		return true
 	}
 	return false
@@ -53,48 +80,84 @@ func (a *Addressing) ShouldAcceptDomain(domain string) bool {
 // ShouldStoreDomain indicates if Inbucket stores mail destined for the specified domain.
 func (a *Addressing) ShouldStoreDomain(domain string) bool {
 	domain = strings.ToLower(domain)
-	if a.Config.DefaultStore && !stringutil.SliceContains(a.Config.DiscardDomains, domain) {
+	if a.Config.SMTP.DefaultStore &&
+		!stringutil.SliceContains(a.Config.SMTP.DiscardDomains, domain) {
 		return true
 	}
-	if !a.Config.DefaultStore && stringutil.SliceContains(a.Config.StoreDomains, domain) {
+	if !a.Config.SMTP.DefaultStore &&
+		stringutil.SliceContains(a.Config.SMTP.StoreDomains, domain) {
 		return true
 	}
 	return false
 }
 
-// ParseMailboxName takes a localPart string (ex: "user+ext" without "@domain")
-// and returns just the mailbox name (ex: "user").  Returns an error if
-// localPart contains invalid characters; it won't accept any that must be
-// quoted according to RFC3696.
-func ParseMailboxName(localPart string) (result string, err error) {
-	if localPart == "" {
-		return "", fmt.Errorf("Mailbox name cannot be empty")
-	}
-	result = strings.ToLower(localPart)
-	invalid := make([]byte, 0, 10)
-	for i := 0; i < len(result); i++ {
-		c := result[i]
-		switch {
-		case 'a' <= c && c <= 'z':
-		case '0' <= c && c <= '9':
-		case bytes.IndexByte([]byte("!#$%&'*+-=/?^_`.{|}~"), c) >= 0:
-		default:
-			invalid = append(invalid, c)
-		}
-	}
-	if len(invalid) > 0 {
-		return "", fmt.Errorf("Mailbox name contained invalid character(s): %q", invalid)
-	}
-	if idx := strings.Index(result, "+"); idx > -1 {
-		result = result[0:idx]
-	}
-	return result, nil
-}
-
 // ParseEmailAddress unescapes an email address, and splits the local part from the domain part.
 // An error is returned if the local or domain parts fail validation following the guidelines
 // in RFC3696.
 func ParseEmailAddress(address string) (local string, domain string, err error) {
+	local, domain, err = parseEmailAddress(address)
+	if err != nil {
+		return "", "", err
+	}
+	if !ValidateDomainPart(domain) {
+		return "", "", fmt.Errorf("Domain part validation failed")
+	}
+	return local, domain, nil
+}
+
+// ValidateDomainPart returns true if the domain part complies to RFC3696, RFC1035. Used by
+// ParseEmailAddress().
+func ValidateDomainPart(domain string) bool {
+	if len(domain) == 0 {
+		return false
+	}
+	if len(domain) > 255 {
+		return false
+	}
+	if domain[len(domain)-1] != '.' {
+		domain += "."
+	}
+	prev := '.'
+	labelLen := 0
+	hasAlphaNum := false
+	for _, c := range domain {
+		switch {
+		case ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') ||
+			('0' <= c && c <= '9') || c == '_':
+			// Must contain some of these to be a valid label.
+			hasAlphaNum = true
+			labelLen++
+		case c == '-':
+			if prev == '.' {
+				// Cannot lead with hyphen.
+				return false
+			}
+		case c == '.':
+			if prev == '.' || prev == '-' {
+				// Cannot end with hyphen or double-dot.
+				return false
+			}
+			if labelLen > 63 {
+				return false
+			}
+			if !hasAlphaNum {
+				return false
+			}
+			labelLen = 0
+			hasAlphaNum = false
+		default:
+			// Unknown character.
+			return false
+		}
+		prev = c
+	}
+	return true
+}
+
+// parseEmailAddress unescapes an email address, and splits the local part from the domain part.  An
+// error is returned if the local part fails validation following the guidelines in RFC3696. The
+// domain part is optional and not validated.
+func parseEmailAddress(address string) (local string, domain string, err error) {
 	if address == "" {
 		return "", "", fmt.Errorf("Empty address")
 	}
@@ -205,57 +268,34 @@ LOOP:
 	if inStringQuote {
 		return "", "", fmt.Errorf("Cannot end address with unterminated string quote")
 	}
-	if !ValidateDomainPart(domain) {
-		return "", "", fmt.Errorf("Domain part validation failed")
-	}
 	return buf.String(), domain, nil
 }
 
-// ValidateDomainPart returns true if the domain part complies to RFC3696, RFC1035. Used by
-// ParseEmailAddress().
-func ValidateDomainPart(domain string) bool {
-	if len(domain) == 0 {
-		return false
+// ParseMailboxName takes a localPart string (ex: "user+ext" without "@domain")
+// and returns just the mailbox name (ex: "user").  Returns an error if
+// localPart contains invalid characters; it won't accept any that must be
+// quoted according to RFC3696.
+func parseMailboxName(localPart string) (result string, err error) {
+	if localPart == "" {
+		return "", fmt.Errorf("Mailbox name cannot be empty")
 	}
-	if len(domain) > 255 {
-		return false
-	}
-	if domain[len(domain)-1] != '.' {
-		domain += "."
-	}
-	prev := '.'
-	labelLen := 0
-	hasAlphaNum := false
-	for _, c := range domain {
+	result = strings.ToLower(localPart)
+	invalid := make([]byte, 0, 10)
+	for i := 0; i < len(result); i++ {
+		c := result[i]
 		switch {
-		case ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') ||
-			('0' <= c && c <= '9') || c == '_':
-			// Must contain some of these to be a valid label.
-			hasAlphaNum = true
-			labelLen++
-		case c == '-':
-			if prev == '.' {
-				// Cannot lead with hyphen.
-				return false
-			}
-		case c == '.':
-			if prev == '.' || prev == '-' {
-				// Cannot end with hyphen or double-dot.
-				return false
-			}
-			if labelLen > 63 {
-				return false
-			}
-			if !hasAlphaNum {
-				return false
-			}
-			labelLen = 0
-			hasAlphaNum = false
+		case 'a' <= c && c <= 'z':
+		case '0' <= c && c <= '9':
+		case bytes.IndexByte([]byte("!#$%&'*+-=/?^_`.{|}~"), c) >= 0:
 		default:
-			// Unknown character.
-			return false
+			invalid = append(invalid, c)
 		}
-		prev = c
 	}
-	return true
+	if len(invalid) > 0 {
+		return "", fmt.Errorf("Mailbox name contained invalid character(s): %q", invalid)
+	}
+	if idx := strings.Index(result, "+"); idx > -1 {
+		result = result[0:idx]
+	}
+	return result, nil
 }

pkg/policy/address_test.go

@@ -11,9 +11,11 @@ import (
 func TestShouldAcceptDomain(t *testing.T) {
 	// Test with default accept.
 	ap := &policy.Addressing{
-		Config: config.SMTP{
-			DefaultAccept: true,
-			RejectDomains: []string{"a.deny.com", "deny.com"},
+		Config: &config.Root{
+			SMTP: config.SMTP{
+				DefaultAccept: true,
+				RejectDomains: []string{"a.deny.com", "deny.com"},
+			},
 		},
 	}
 	testCases := []struct {
@@ -36,9 +38,11 @@ func TestShouldAcceptDomain(t *testing.T) {
 	}
 	// Test with default reject.
 	ap = &policy.Addressing{
-		Config: config.SMTP{
-			DefaultAccept: false,
-			AcceptDomains: []string{"a.allow.com", "allow.com"},
+		Config: &config.Root{
+			SMTP: config.SMTP{
+				DefaultAccept: false,
+				AcceptDomains: []string{"a.allow.com", "allow.com"},
+			},
 		},
 	}
 	testCases = []struct {
@@ -64,9 +68,11 @@ func TestShouldAcceptDomain(t *testing.T) {
 func TestShouldStoreDomain(t *testing.T) {
 	// Test with storage enabled.
 	ap := &policy.Addressing{
-		Config: config.SMTP{
-			DefaultStore: false,
-			StoreDomains: []string{"store.com", "a.store.com"},
+		Config: &config.Root{
+			SMTP: config.SMTP{
+				DefaultStore: false,
+				StoreDomains: []string{"store.com", "a.store.com"},
+			},
 		},
 	}
 	testCases := []struct {
@@ -89,9 +95,11 @@ func TestShouldStoreDomain(t *testing.T) {
 	}
 	// Test with storage disabled.
 	ap = &policy.Addressing{
-		Config: config.SMTP{
-			DefaultStore:   true,
-			DiscardDomains: []string{"discard.com", "a.discard.com"},
+		Config: &config.Root{
+			SMTP: config.SMTP{
+				DefaultStore:   true,
+				DiscardDomains: []string{"discard.com", "a.discard.com"},
+			},
 		},
 	}
 	testCases = []struct {
@@ -114,49 +122,170 @@ func TestShouldStoreDomain(t *testing.T) {
 	}
 }
 
-func TestParseMailboxName(t *testing.T) {
-	var validTable = []struct {
-		input  string
-		expect string
+func TestExtractMailboxValid(t *testing.T) {
+	localPolicy := policy.Addressing{Config: &config.Root{MailboxNaming: config.LocalNaming}}
+	fullPolicy := policy.Addressing{Config: &config.Root{MailboxNaming: config.FullNaming}}
+
+	testTable := []struct {
+		input string // Input to test
+		local string // Expected output when mailbox naming = local
+		full  string // Expected output when mailbox naming = full
 	}{
-		{"mailbox", "mailbox"},
-		{"user123", "user123"},
-		{"MailBOX", "mailbox"},
-		{"First.Last", "first.last"},
-		{"user+label", "user"},
-		{"chars!#$%", "chars!#$%"},
-		{"chars&'*-", "chars&'*-"},
-		{"chars=/?^", "chars=/?^"},
-		{"chars_`.{", "chars_`.{"},
-		{"chars|}~", "chars|}~"},
+		{
+			input: "mailbox",
+			local: "mailbox",
+			full:  "mailbox",
+		},
+		{
+			input: "user123",
+			local: "user123",
+			full:  "user123",
+		},
+		{
+			input: "MailBOX",
+			local: "mailbox",
+			full:  "mailbox",
+		},
+		{
+			input: "First.Last",
+			local: "first.last",
+			full:  "first.last",
+		},
+		{
+			input: "user+label",
+			local: "user",
+			full:  "user",
+		},
+		{
+			input: "chars!#$%",
+			local: "chars!#$%",
+			full:  "chars!#$%",
+		},
+		{
+			input: "chars&'*-",
+			local: "chars&'*-",
+			full:  "chars&'*-",
+		},
+		{
+			input: "chars=/?^",
+			local: "chars=/?^",
+			full:  "chars=/?^",
+		},
+		{
+			input: "chars_`.{",
+			local: "chars_`.{",
+			full:  "chars_`.{",
+		},
+		{
+			input: "chars|}~",
+			local: "chars|}~",
+			full:  "chars|}~",
+		},
+		{
+			input: "mailbox@domain.com",
+			local: "mailbox",
+			full:  "mailbox@domain.com",
+		},
+		{
+			input: "user123@domain.com",
+			local: "user123",
+			full:  "user123@domain.com",
+		},
+		{
+			input: "MailBOX@domain.com",
+			local: "mailbox",
+			full:  "mailbox@domain.com",
+		},
+		{
+			input: "First.Last@domain.com",
+			local: "first.last",
+			full:  "first.last@domain.com",
+		},
+		{
+			input: "user+label@domain.com",
+			local: "user",
+			full:  "user@domain.com",
+		},
+		{
+			input: "chars!#$%@domain.com",
+			local: "chars!#$%",
+			full:  "chars!#$%@domain.com",
+		},
+		{
+			input: "chars&'*-@domain.com",
+			local: "chars&'*-",
+			full:  "chars&'*-@domain.com",
+		},
+		{
+			input: "chars=/?^@domain.com",
+			local: "chars=/?^",
+			full:  "chars=/?^@domain.com",
+		},
+		{
+			input: "chars_`.{@domain.com",
+			local: "chars_`.{",
+			full:  "chars_`.{@domain.com",
+		},
+		{
+			input: "chars|}~@domain.com",
+			local: "chars|}~",
+			full:  "chars|}~@domain.com",
+		},
 	}
-	for _, tt := range validTable {
-		if result, err := policy.ParseMailboxName(tt.input); err != nil {
-			t.Errorf("Error while parsing %q: %v", tt.input, err)
+	for _, tc := range testTable {
+		if result, err := localPolicy.ExtractMailbox(tc.input); err != nil {
+			t.Errorf("Error while parsing with local naming %q: %v", tc.input, err)
 		} else {
-			if result != tt.expect {
-				t.Errorf("Parsing %q, expected %q, got %q", tt.input, tt.expect, result)
+			if result != tc.local {
+				t.Errorf("Parsing %q, expected %q, got %q", tc.input, tc.local, result)
+			}
+		}
+		if result, err := fullPolicy.ExtractMailbox(tc.input); err != nil {
+			t.Errorf("Error while parsing with full naming %q: %v", tc.input, err)
+		} else {
+			if result != tc.full {
+				t.Errorf("Parsing %q, expected %q, got %q", tc.input, tc.full, result)
 			}
 		}
 	}
-	var invalidTable = []struct {
+}
+
+func TestExtractMailboxInvalid(t *testing.T) {
+	localPolicy := policy.Addressing{Config: &config.Root{MailboxNaming: config.LocalNaming}}
+	fullPolicy := policy.Addressing{Config: &config.Root{MailboxNaming: config.FullNaming}}
+	// Test local mailbox naming policy.
+	localInvalidTable := []struct {
 		input, msg string
 	}{
 		{"", "Empty mailbox name is not permitted"},
-		{"user@host", "@ symbol not permitted"},
 		{"first last", "Space not permitted"},
 		{"first\"last", "Double quote not permitted"},
 		{"first\nlast", "Control chars not permitted"},
 	}
-	for _, tt := range invalidTable {
-		if _, err := policy.ParseMailboxName(tt.input); err == nil {
-			t.Errorf("Didn't get an error while parsing %q: %v", tt.input, tt.msg)
+	for _, tt := range localInvalidTable {
+		if _, err := localPolicy.ExtractMailbox(tt.input); err == nil {
+			t.Errorf("Didn't get an error while parsing in local mode %q: %v", tt.input, tt.msg)
+		}
+	}
+	// Test full mailbox naming policy.
+	fullInvalidTable := []struct {
+		input, msg string
+	}{
+		{"", "Empty mailbox name is not permitted"},
+		{"user@host@domain.com", "@ symbol not permitted"},
+		{"first last@domain.com", "Space not permitted"},
+		{"first\"last@domain.com", "Double quote not permitted"},
+		{"first\nlast@domain.com", "Control chars not permitted"},
+	}
+	for _, tt := range fullInvalidTable {
+		if _, err := fullPolicy.ExtractMailbox(tt.input); err == nil {
+			t.Errorf("Didn't get an error while parsing in full mode %q: %v", tt.input, tt.msg)
 		}
 	}
 }
 
 func TestValidateDomain(t *testing.T) {
-	var testTable = []struct {
+	testTable := []struct {
 		input  string
 		expect bool
 		msg    string
@@ -187,7 +316,7 @@ func TestValidateDomain(t *testing.T) {
 }
 
 func TestValidateLocal(t *testing.T) {
-	var testTable = []struct {
+	testTable := []struct {
 		input  string
 		expect bool
 		msg    string
@@ -203,12 +332,12 @@ func TestValidateLocal(t *testing.T) {
 		{"first..last", false, "Sequence of periods is not allowed"},
 		{".user", false, "Cannot lead with a period"},
 		{"user.", false, "Cannot end with a period"},
-		{"james@mail", false, "Unquoted @ not permitted"},
+		// {"james@mail", false, "Unquoted @ not permitted"},
 		{"first last", false, "Unquoted space not permitted"},
 		{"tricky\\. ", false, "Unquoted space not permitted"},
 		{"no,commas", false, "Unquoted comma not allowed"},
 		{"t[es]t", false, "Unquoted square brackets not allowed"},
-		{"james\\", false, "Cannot end with backslash quote"},
+		// {"james\\", false, "Cannot end with backslash quote"},
 		{"james\\@mail", true, "Quoted @ permitted"},
 		{"quoted\\ space", true, "Quoted space permitted"},
 		{"no\\,commas", true, "Quoted comma is OK"},

pkg/rest/apiv1_controller.go

@@ -53,14 +53,13 @@ func MailboxShowV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) (
 		return err
 	}
 	msg, err := ctx.Manager.GetMessage(name, id)
-	if err == storage.ErrNotExist {
+	if err != nil && err != storage.ErrNotExist {
+		return fmt.Errorf("GetMessage(%q) failed: %v", id, err)
+	}
+	if msg == nil {
 		http.NotFound(w, req)
 		return nil
 	}
-	if err != nil {
-		// This doesn't indicate empty, likely an IO error
-		return fmt.Errorf("GetMessage(%q) failed: %v", id, err)
-	}
 	attachParts := msg.Attachments()
 	attachments := make([]*model.JSONMessageAttachmentV1, len(attachParts))
 	for i, part := range attachParts {
@@ -146,14 +145,13 @@ func MailboxSourceV1(w http.ResponseWriter, req *http.Request, ctx *web.Context)
 		return err
 	}
 	r, err := ctx.Manager.SourceReader(name, id)
-	if err == storage.ErrNotExist {
+	if err != nil && err != storage.ErrNotExist {
+		return fmt.Errorf("SourceReader(%q) failed: %v", id, err)
+	}
+	if r == nil {
 		http.NotFound(w, req)
 		return nil
 	}
-	if err != nil {
-		// This doesn't indicate missing, likely an IO error
-		return fmt.Errorf("SourceReader(%q) failed: %v", id, err)
-	}
 	// Output message source
 	w.Header().Set("Content-Type", "text/plain")
 	_, err = io.Copy(w, r)

pkg/rest/apiv1_controller_test.go

@@ -37,7 +37,7 @@ func TestRestMailboxList(t *testing.T) {
 	logbuf := setupWebServer(mm)
 
 	// Test invalid mailbox name
-	w, err := testRestGet(baseURL + "/mailbox/foo@bar")
+	w, err := testRestGet(baseURL + "/mailbox/foo%20bar")
 	expectCode := 500
 	if err != nil {
 		t.Fatal(err)
@@ -139,7 +139,7 @@ func TestRestMessage(t *testing.T) {
 	logbuf := setupWebServer(mm)
 
 	// Test invalid mailbox name
-	w, err := testRestGet(baseURL + "/mailbox/foo@bar/0001")
+	w, err := testRestGet(baseURL + "/mailbox/foo%20bar/0001")
 	expectCode := 500
 	if err != nil {
 		t.Fatal(err)

pkg/server/smtp/handler.go

@@ -16,10 +16,13 @@ import (
 	"github.com/rs/zerolog/log"
 )
 
-// State tracks the current mode of our SMTP state machine
+// State tracks the current mode of our SMTP state machine.
 type State int
 
 const (
+	// timeStampFormat to use in Received header.
+	timeStampFormat = "Mon, 02 Jan 2006 15:04:05 -0700 (MST)"
+
 	// GREET State: Waiting for HELO
 	GREET State = iota
 	// READY State: Got HELO, waiting for MAIL
@@ -32,7 +35,11 @@ const (
 	QUIT
 )
 
-const timeStampFormat = "Mon, 02 Jan 2006 15:04:05 -0700 (MST)"
+// fromRegex captures the from address and optional BODY=8BITMIME clause.  Matches FROM, while
+// accepting '>' as quoted pair and in double quoted strings (?i) makes the regex case insensitive,
+// (?:) is non-grouping sub-match
+var fromRegex = regexp.MustCompile(
+	"(?i)^FROM:\\s*<((?:\\\\>|[^>])+|\"[^\"]+\"@[^>]+)>( [\\w= ]+)?$")
 
 func (s State) String() string {
 	switch s {
@@ -265,10 +272,8 @@ func parseHelloArgument(arg string) (string, error) {
 // READY state -> waiting for MAIL
 func (s *Session) readyHandler(cmd string, arg string) {
 	if cmd == "MAIL" {
-		// Match FROM, while accepting '>' as quoted pair and in double quoted strings
-		// (?i) makes the regex case insensitive, (?:) is non-grouping sub-match
-		re := regexp.MustCompile("(?i)^FROM:\\s*<((?:\\\\>|[^>])+|\"[^\"]+\"@[^>]+)>( [\\w= ]+)?$")
-		m := re.FindStringSubmatch(arg)
+		// Capture group 1: from address.  2: optional params.
+		m := fromRegex.FindStringSubmatch(arg)
 		if m == nil {
 			s.send("501 Was expecting MAIL arg syntax of FROM:<address>")
 			s.logger.Warn().Msgf("Bad MAIL argument: %q", arg)
@@ -321,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.config.MaxRecipients)
-			s.send(fmt.Sprintf("552 Maximum limit of %v recipients reached",
-				s.config.MaxRecipients))
+			s.logger.Warn().Msgf("Limit of %v recipients exceeded", s.config.MaxRecipients)
+			s.send(fmt.Sprintf("552 Limit of %v recipients exceeded", 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":
@@ -351,13 +353,12 @@ 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 {
-			// We have recipients, go to accept data
-			s.enterState(DATA)
+		if len(s.recipients) == 0 {
+			// DATA out of sequence
+			s.ooSeq(cmd)
 			return
 		}
-		// DATA out of sequence
-		s.ooSeq(cmd)
+		s.enterState(DATA)
 		return
 	}
 	s.ooSeq(cmd)

pkg/server/smtp/handler_test.go

@@ -361,41 +361,40 @@ func (m *mockConn) SetReadDeadline(t time.Time) error  { return nil }
 func (m *mockConn) SetWriteDeadline(t time.Time) error { return nil }
 
 func setupSMTPServer(ds storage.Store) (s *Server, buf *bytes.Buffer, teardown func()) {
-	// Test Server Config
-	cfg := config.SMTP{
-		Addr:            "127.0.0.1:2500",
-		Domain:          "inbucket.local",
-		MaxRecipients:   5,
-		MaxMessageBytes: 5000,
-		DefaultAccept:   true,
-		RejectDomains:   []string{"deny.com"},
-		Timeout:         5,
+	cfg := &config.Root{
+		MailboxNaming: config.FullNaming,
+		SMTP: config.SMTP{
+			Addr:            "127.0.0.1:2500",
+			Domain:          "inbucket.local",
+			MaxRecipients:   5,
+			MaxMessageBytes: 5000,
+			DefaultAccept:   true,
+			RejectDomains:   []string{"deny.com"},
+			Timeout:         5,
+		},
 	}
-
-	// Capture log output
+	// Capture log output.
 	buf = new(bytes.Buffer)
 	log.SetOutput(buf)
-
-	// Create a server, don't start it
+	// Create a server, don't start it.
 	shutdownChan := make(chan bool)
 	teardown = func() {
 		close(shutdownChan)
 	}
-	apolicy := &policy.Addressing{Config: cfg}
+	addrPolicy := &policy.Addressing{Config: cfg}
 	manager := &message.StoreManager{Store: ds}
-	s = NewServer(cfg, shutdownChan, manager, apolicy)
+	s = NewServer(cfg.SMTP, shutdownChan, manager, addrPolicy)
 	return s, buf, teardown
 }
 
 var sessionNum int
 
 func setupSMTPSession(server *Server) net.Conn {
-	// Pair of pipes to communicate
+	// Pair of pipes to communicate.
 	serverConn, clientConn := net.Pipe()
-	// Start the session
+	// Start the session.
 	server.wg.Add(1)
 	sessionNum++
 	go server.startSession(sessionNum, &mockConn{serverConn})
-
 	return clientConn
 }

pkg/storage/file/fstore.go

@@ -10,7 +10,6 @@ import (
 	"time"
 
 	"github.com/jhillyerd/inbucket/pkg/config"
-	"github.com/jhillyerd/inbucket/pkg/policy"
 	"github.com/jhillyerd/inbucket/pkg/storage"
 	"github.com/jhillyerd/inbucket/pkg/stringutil"
 	"github.com/rs/zerolog/log"
@@ -66,10 +65,7 @@ func New(cfg config.Storage) (storage.Store, error) {
 
 // AddMessage adds a message to the specified mailbox.
 func (fs *Store) AddMessage(m storage.Message) (id string, err error) {
-	mb, err := fs.mbox(m.Mailbox())
-	if err != nil {
-		return "", err
-	}
+	mb := fs.mbox(m.Mailbox())
 	mb.Lock()
 	defer mb.Unlock()
 	r, err := m.Source()
@@ -127,10 +123,7 @@ func (fs *Store) AddMessage(m storage.Message) (id string, err error) {
 
 // GetMessage returns the messages in the named mailbox, or an error.
 func (fs *Store) GetMessage(mailbox, id string) (storage.Message, error) {
-	mb, err := fs.mbox(mailbox)
-	if err != nil {
-		return nil, err
-	}
+	mb := fs.mbox(mailbox)
 	mb.RLock()
 	defer mb.RUnlock()
 	return mb.getMessage(id)
@@ -138,10 +131,7 @@ func (fs *Store) GetMessage(mailbox, id string) (storage.Message, error) {
 
 // GetMessages returns the messages in the named mailbox, or an error.
 func (fs *Store) GetMessages(mailbox string) ([]storage.Message, error) {
-	mb, err := fs.mbox(mailbox)
-	if err != nil {
-		return nil, err
-	}
+	mb := fs.mbox(mailbox)
 	mb.RLock()
 	defer mb.RUnlock()
 	return mb.getMessages()
@@ -149,10 +139,7 @@ func (fs *Store) GetMessages(mailbox string) ([]storage.Message, error) {
 
 // MarkSeen flags the message as having been read.
 func (fs *Store) MarkSeen(mailbox, id string) error {
-	mb, err := fs.mbox(mailbox)
-	if err != nil {
-		return err
-	}
+	mb := fs.mbox(mailbox)
 	mb.Lock()
 	defer mb.Unlock()
 	if !mb.indexLoaded {
@@ -175,10 +162,7 @@ func (fs *Store) MarkSeen(mailbox, id string) error {
 
 // RemoveMessage deletes a message by ID from the specified mailbox.
 func (fs *Store) RemoveMessage(mailbox, id string) error {
-	mb, err := fs.mbox(mailbox)
-	if err != nil {
-		return err
-	}
+	mb := fs.mbox(mailbox)
 	mb.Lock()
 	defer mb.Unlock()
 	return mb.removeMessage(id)
@@ -186,10 +170,7 @@ func (fs *Store) RemoveMessage(mailbox, id string) error {
 
 // PurgeMessages deletes all messages in the named mailbox, or returns an error.
 func (fs *Store) PurgeMessages(mailbox string) error {
-	mb, err := fs.mbox(mailbox)
-	if err != nil {
-		return err
-	}
+	mb := fs.mbox(mailbox)
 	mb.Lock()
 	defer mb.Unlock()
 	return mb.purge()
@@ -241,12 +222,8 @@ func (fs *Store) VisitMailboxes(f func([]storage.Message) (cont bool)) error {
 }
 
 // mbox returns the named mailbox.
-func (fs *Store) mbox(mailbox string) (*mbox, error) {
-	name, err := policy.ParseMailboxName(mailbox)
-	if err != nil {
-		return nil, err
-	}
-	hash := stringutil.HashMailboxName(name)
+func (fs *Store) mbox(mailbox string) *mbox {
+	hash := stringutil.HashMailboxName(mailbox)
 	s1 := hash[0:3]
 	s2 := hash[0:6]
 	path := filepath.Join(fs.mailPath, s1, s2, hash)
@@ -254,11 +231,11 @@ func (fs *Store) mbox(mailbox string) (*mbox, error) {
 	return &mbox{
 		RWMutex:   fs.hashLock.Get(hash),
 		store:     fs,
-		name:      name,
+		name:      mailbox,
 		dirName:   hash,
 		path:      path,
 		indexPath: indexPath,
-	}, nil
+	}
 }
 
 // mboxFromPath constructs a mailbox based on name hash.

pkg/storage/mem/store.go

@@ -93,7 +93,11 @@ func (s *Store) AddMessage(message storage.Message) (id string, err error) {
 // GetMessage gets a mesage.
 func (s *Store) GetMessage(mailbox, id string) (m storage.Message, err error) {
 	s.withMailbox(mailbox, false, func(mb *mbox) {
-		m = mb.messages[id]
+		var ok bool
+		m, ok = mb.messages[id]
+		if !ok {
+			m = nil
+		}
 	})
 	return m, err
 }

pkg/test/manager.go

@@ -3,6 +3,7 @@ package test
 import (
 	"errors"
 
+	"github.com/jhillyerd/inbucket/pkg/config"
 	"github.com/jhillyerd/inbucket/pkg/message"
 	"github.com/jhillyerd/inbucket/pkg/policy"
 	"github.com/jhillyerd/inbucket/pkg/storage"
@@ -55,7 +56,10 @@ func (m *ManagerStub) GetMetadata(mailbox string) ([]*message.Metadata, error) {
 
 // MailboxForAddress invokes policy.ParseMailboxName.
 func (m *ManagerStub) MailboxForAddress(address string) (string, error) {
-	return policy.ParseMailboxName(address)
+	addrPolicy := &policy.Addressing{Config: &config.Root{
+		MailboxNaming: config.FullNaming,
+	}}
+	return addrPolicy.ExtractMailbox(address)
 }
 
 // MarkSeen marks a message as having been read.

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
 }