Repository: inbucket/inbucket
Pull request #539 (2024-10-06 00:46:43)

538 add smtp error code response type get rid of bool pointers
Introduces a `SMTPResponse` type for extensions, which is exposed to Lua by way of three constructors:

```lua
smtp.defer()
smtp.allow()
smtp.deny(code, message) -- code and message have defaults
```

coveralls: ## Pull Request Test Coverage Report for [Build 11197584550](https://coveralls.io/builds/70183467)


### Details

* **48** of **57**   **(84.21%)**  changed or added relevant lines in **4** files are covered.
* **1** unchanged line in **1** file lost coverage.
* Overall coverage increased (+**0.2%**) to **56.191%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [pkg/extension/luahost/lua.go](https://coveralls.io/builds/70183467/source?filename=pkg%2Fextension%2Fluahost%2Flua.go#L178) | 5 | 6 | 83.33%
| [pkg/extension/luahost/bind_smtpresponse.go](https://coveralls.io/builds/70183467/source?filename=pkg%2Fextension%2Fluahost%2Fbind_smtpresponse.go#L53) | 33 | 41 | 80.49%
<!-- | **Total:** | **48** | **57** | **84.21%** | -->

|  Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [pkg/extension/luahost/lua.go](https://coveralls.io/builds/70183467/source?filename=pkg%2Fextension%2Fluahost%2Flua.go#L179) | 1 | 62.5% |
<!-- | **Total:** | **1** |  | -->

|  Totals | [![Coverage Status](https://coveralls.io/builds/70183467/badge)](https://coveralls.io/builds/70183467) |
| :-- | --: |
| Change from base [Build 11190398005](https://coveralls.io/builds/70179949): |  0.2% |
| Covered Lines: | 2741 |
| Relevant Lines: | 4878 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/.luarc.json b/.luarc.json
new file mode 100644
index 0000000..e645f99
--- /dev/null
+++ b/.luarc.json
@@ -0,0 +1,9 @@
+{
+	"runtime.version": "Lua 5.1",
+	"diagnostics": {
+		"globals": [
+			"inbucket",
+			"smtp"
+		]
+	}
+}
diff --git a/pkg/extension/event/events.go b/pkg/extension/event/events.go
index c59f6bf..14a7535 100644
--- a/pkg/extension/event/events.go
+++ b/pkg/extension/event/events.go
@@ -5,6 +5,15 @@ import (
 	"time"
 )
 
+const (
+	// ActionDefer defers decision to built-in Inbucket logic.
+	ActionDefer = iota
+	// ActionAllow explicitly allows this event.
+	ActionAllow
+	// ActionDeny explicitly deny this event, typically with specified SMTP error.
+	ActionDeny
+)
+
 // AddressParts contains the local and domain parts of an email address.
 type AddressParts struct {
 	Local  string
@@ -31,3 +40,10 @@ type MessageMetadata struct {
 	Size    int64
 	Seen    bool
 }
+
+// SMTPResponse describes the response to an SMTP policy check.
+type SMTPResponse struct {
+	Action    int    // ActionDefer, ActionAllow, etc.
+	ErrorCode int    // SMTP error code to respond with on deny.
+	ErrorMsg  string // SMTP error message to respond with on deny.
+}
diff --git a/pkg/extension/host.go b/pkg/extension/host.go
index c22ff1e..38d68e8 100644
--- a/pkg/extension/host.go
+++ b/pkg/extension/host.go
@@ -22,7 +22,7 @@ type Host struct {
 type Events struct {
 	AfterMessageDeleted AsyncEventBroker[event.MessageMetadata]
 	AfterMessageStored  AsyncEventBroker[event.MessageMetadata]
-	BeforeMailAccepted  EventBroker[event.AddressParts, bool]
+	BeforeMailAccepted  EventBroker[event.AddressParts, event.SMTPResponse]
 	BeforeMessageStored EventBroker[event.InboundMessage, event.InboundMessage]
 }
 
diff --git a/pkg/extension/luahost/bind_policy.go b/pkg/extension/luahost/bind_policy.go
deleted file mode 100644
index 8a42060..0000000
--- a/pkg/extension/luahost/bind_policy.go
+++ /dev/null
@@ -1,17 +0,0 @@
-package luahost
-
-import (
-	lua "github.com/yuin/gopher-lua"
-)
-
-const policyName = "policy"
-
-func registerPolicyType(ls *lua.LState) {
-	mt := ls.NewTypeMetatable(policyName)
-	ls.SetGlobal(policyName, mt)
-
-	// Static attributes.
-	ls.SetField(mt, "allow", lua.LTrue)
-	ls.SetField(mt, "deny", lua.LFalse)
-	ls.SetField(mt, "defer", lua.LNil)
-}
diff --git a/pkg/extension/luahost/bind_smtpresponse.go b/pkg/extension/luahost/bind_smtpresponse.go
new file mode 100644
index 0000000..dd420fe
--- /dev/null
+++ b/pkg/extension/luahost/bind_smtpresponse.go
@@ -0,0 +1,54 @@
+package luahost
+
+import (
+	"fmt"
+
+	"github.com/inbucket/inbucket/v3/pkg/extension/event"
+	lua "github.com/yuin/gopher-lua"
+)
+
+const smtpResponseName = "smtp"
+
+func registerSMTPResponseType(ls *lua.LState) {
+	mt := ls.NewTypeMetatable(smtpResponseName)
+	ls.SetGlobal(smtpResponseName, mt)
+
+	// Static attributes.
+	ls.SetField(mt, "allow", ls.NewFunction(newSMTPResponse(event.ActionAllow)))
+	ls.SetField(mt, "defer", ls.NewFunction(newSMTPResponse(event.ActionDefer)))
+	ls.SetField(mt, "deny", ls.NewFunction(newSMTPResponse(event.ActionDeny)))
+}
+
+func newSMTPResponse(action int) func(*lua.LState) int {
+	return func(ls *lua.LState) int {
+		val := &event.SMTPResponse{Action: action}
+
+		if action == event.ActionDeny {
+			// Optionally accept error code and message.
+			val.ErrorCode = ls.OptInt(1, 550)
+			val.ErrorMsg = ls.OptString(2, "Mail denied by policy")
+		}
+
+		ud := wrapSMTPResponse(ls, val)
+		ls.Push(ud)
+		return 1
+	}
+}
+
+func wrapSMTPResponse(ls *lua.LState, val *event.SMTPResponse) *lua.LUserData {
+	ud := ls.NewUserData()
+	ud.Value = val
+	ls.SetMetatable(ud, ls.GetTypeMetatable(smtpResponseName))
+
+	return ud
+}
+
+func unwrapSMTPResponse(lv lua.LValue) (*event.SMTPResponse, error) {
+	if ud, ok := lv.(*lua.LUserData); ok {
+		if v, ok := ud.Value.(*event.SMTPResponse); ok {
+			return v, nil
+		}
+	}
+
+	return nil, fmt.Errorf("expected SMTPResponse, got %q", lv.Type().String())
+}
diff --git a/pkg/extension/luahost/lua.go b/pkg/extension/luahost/lua.go
index 189726f..8a49dd6 100644
--- a/pkg/extension/luahost/lua.go
+++ b/pkg/extension/luahost/lua.go
@@ -148,7 +148,7 @@ func (h *Host) handleAfterMessageStored(msg event.MessageMetadata) {
 	}
 }
 
-func (h *Host) handleBeforeMailAccepted(addr event.AddressParts) *bool {
+func (h *Host) handleBeforeMailAccepted(addr event.AddressParts) *event.SMTPResponse {
 	logger, ls, ib, ok := h.prepareInbucketFuncCall("before.mail_accepted")
 	if !ok {
 		return nil
@@ -169,16 +169,16 @@ func (h *Host) handleBeforeMailAccepted(addr event.AddressParts) *bool {
 	ls.Pop(1)
 	logger.Debug().Msgf("Lua function returned %q (%v)", lval, lval.Type().String())
 
-	if lval.Type() == lua.LTNil {
+	if lval.Type() == lua.LTNil || lua.LVIsFalse(lval) {
 		return nil
 	}
 
-	result := true
-	if lua.LVIsFalse(lval) {
-		result = false
+	result, err := unwrapSMTPResponse(lval)
+	if err != nil {
+		logger.Error().Err(err).Msg("Bad response from Lua Function")
 	}
 
-	return &result
+	return result
 }
 
 func (h *Host) handleBeforeMessageStored(msg event.InboundMessage) *event.InboundMessage {
diff --git a/pkg/extension/luahost/pool.go b/pkg/extension/luahost/pool.go
index 65a379d..3ad48fd 100644
--- a/pkg/extension/luahost/pool.go
+++ b/pkg/extension/luahost/pool.go
@@ -48,7 +48,7 @@ func (lp *statePool) newState() (*lua.LState, error) {
 	registerInbucketTypes(ls)
 	registerMailAddressType(ls)
 	registerMessageMetadataType(ls)
-	registerPolicyType(ls)
+	registerSMTPResponseType(ls)
 
 	// Run compiled script.
 	ls.Push(ls.NewFunctionFromProto(lp.funcProto))
diff --git a/pkg/server/smtp/handler.go b/pkg/server/smtp/handler.go
index 2713fe8..4fd4c05 100644
--- a/pkg/server/smtp/handler.go
+++ b/pkg/server/smtp/handler.go
@@ -446,10 +446,14 @@ func (s *Session) parseMailFromCmd(arg string) {
 	}
 
 	// Process through extensions.
+	extAction := event.ActionDefer
 	extResult := s.extHost.Events.BeforeMailAccepted.Emit(
 		&event.AddressParts{Local: localpart, Domain: domain})
-	if extResult != nil && !*extResult {
-		s.send("550 Mail denied by policy")
+	if extResult != nil {
+		extAction = extResult.Action
+	}
+	if extAction == event.ActionDeny {
+		s.send(fmt.Sprintf("%03d %s", extResult.ErrorCode, extResult.ErrorMsg))
 		s.logger.Warn().Msgf("Extension denied mail from <%v>", from)
 		return
 	}
@@ -462,7 +466,8 @@ func (s *Session) parseMailFromCmd(arg string) {
 		return
 	}
 	s.from = origin
-	if !s.from.ShouldAccept() {
+	// Ignore ShouldAccept if extensions explicitly allowed this From.
+	if extAction == event.ActionDefer && !s.from.ShouldAccept() {
 		s.send("501 Unauthorized domain")
 		s.logger.Warn().Msgf("Bad domain sender %s", domain)
 		return


Test patch:
diff --git a/pkg/extension/luahost/bind_smtpresponse_test.go b/pkg/extension/luahost/bind_smtpresponse_test.go
new file mode 100644
index 0000000..6abeb50
--- /dev/null
+++ b/pkg/extension/luahost/bind_smtpresponse_test.go
@@ -0,0 +1,40 @@
+package luahost
+
+import (
+	"testing"
+
+	"github.com/inbucket/inbucket/v3/pkg/extension/event"
+	"github.com/inbucket/inbucket/v3/pkg/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSMTPResponseConstructors(t *testing.T) {
+	check := func(script string, want event.SMTPResponse) {
+		t.Helper()
+		ls, _ := test.NewLuaState()
+		registerSMTPResponseType(ls)
+		require.NoError(t, ls.DoString(script))
+
+		got, err := unwrapSMTPResponse(ls.Get(-1))
+		require.NoError(t, err)
+		assert.Equal(t, &want, got)
+	}
+
+	check("return smtp.defer()", event.SMTPResponse{Action: event.ActionDefer})
+	check("return smtp.allow()", event.SMTPResponse{Action: event.ActionAllow})
+
+	// Verify deny() has default code & msg.
+	check("return smtp.deny()", event.SMTPResponse{
+		Action:    event.ActionDeny,
+		ErrorCode: 550,
+		ErrorMsg:  "Mail denied by policy",
+	})
+
+	// Verify defaults can be overridden.
+	check("return smtp.deny(123, 'bacon')", event.SMTPResponse{
+		Action:    event.ActionDeny,
+		ErrorCode: 123,
+		ErrorMsg:  "bacon",
+	})
+}
diff --git a/pkg/extension/luahost/lua_test.go b/pkg/extension/luahost/lua_test.go
index dd626b3..fe8bb54 100644
--- a/pkg/extension/luahost/lua_test.go
+++ b/pkg/extension/luahost/lua_test.go
@@ -109,29 +109,36 @@ func TestBeforeMailAccepted(t *testing.T) {
 	// Register lua event listener.
 	script := `
 		function inbucket.before.mail_accepted(localpart, domain)
-			return localpart == "from" and domain == "test"
+			if localpart == "from" and domain == "test" then
+				logger.info("allowing message", {})
+				return smtp.allow()
+			else
+				logger.info("denying message", {})
+				return smtp.deny()
+			end
 		end
 	`
 	extHost := extension.NewHost()
-	_, err := luahost.NewFromReader(consoleLogger, extHost, strings.NewReader(script), "test.lua")
+	_, err := luahost.NewFromReader(
+		consoleLogger, extHost, strings.NewReader(test.LuaInit+script), "test.lua")
 	require.NoError(t, err)
 
 	// Send event to be accepted.
 	addr := &event.AddressParts{Local: "from", Domain: "test"}
 	got := extHost.Events.BeforeMailAccepted.Emit(addr)
-	want := true
+	want := event.ActionAllow
 	require.NotNil(t, got, "Expected result from Emit()")
-	if *got != want {
-		t.Errorf("Got %v, wanted %v for addr %v", *got, want, addr)
+	if got.Action != want {
+		t.Errorf("Got %v, wanted %v for addr %v", got.Action, want, addr)
 	}
 
 	// Send event to be denied.
 	addr = &event.AddressParts{Local: "reject", Domain: "me"}
 	got = extHost.Events.BeforeMailAccepted.Emit(addr)
-	want = false
+	want = event.ActionDeny
 	require.NotNil(t, got, "Expected result from Emit()")
-	if *got != want {
-		t.Errorf("Got %v, wanted %v for addr %v", *got, want, addr)
+	if got.Action != want {
+		t.Errorf("Got %v, wanted %v for addr %v", got.Action, want, addr)
 	}
 }
 
diff --git a/pkg/server/smtp/handler_test.go b/pkg/server/smtp/handler_test.go
index b0681c5..a0f9db6 100644
--- a/pkg/server/smtp/handler_test.go
+++ b/pkg/server/smtp/handler_test.go
@@ -393,9 +393,9 @@ func TestBeforeMailAcceptedEventEmitted(t *testing.T) {
 	var got *event.AddressParts
 	extHost.Events.BeforeMailAccepted.AddListener(
 		"test",
-		func(addr event.AddressParts) *bool {
+		func(addr event.AddressParts) *event.SMTPResponse {
 			got = &addr
-			return nil
+			return &event.SMTPResponse{Action: event.ActionDefer}
 		})
 
 	// Play and verify SMTP session.
@@ -416,32 +416,34 @@ func TestBeforeMailAcceptedEventResponse(t *testing.T) {
 	extHost := extension.NewHost()
 	server := setupSMTPServer(ds, extHost)
 
-	var shouldReturn *bool
+	var shouldReturn *event.SMTPResponse
 	var gotEvent *event.AddressParts
 	extHost.Events.BeforeMailAccepted.AddListener(
 		"test",
-		func(addr event.AddressParts) *bool {
+		func(addr event.AddressParts) *event.SMTPResponse {
 			gotEvent = &addr
 			return shouldReturn
 		})
 
-	allowRes := true
-	denyRes := false
 	tcs := map[string]struct {
-		script   scriptStep // Command to send and SMTP code expected.
-		eventRes *bool      // Response to send from event listener.
+		script   scriptStep         // Command to send and SMTP code expected.
+		eventRes event.SMTPResponse // Response to send from event listener.
 	}{
 		"allow": {
 			script:   scriptStep{"MAIL FROM:<john@gmail.com>", 250},
-			eventRes: &allowRes,
+			eventRes: event.SMTPResponse{Action: event.ActionAllow},
 		},
 		"deny": {
-			script:   scriptStep{"MAIL FROM:<john@gmail.com>", 550},
-			eventRes: &denyRes,
+			script: scriptStep{"MAIL FROM:<john@gmail.com>", 550},
+			eventRes: event.SMTPResponse{
+				Action:    event.ActionDeny,
+				ErrorCode: 550,
+				ErrorMsg:  "meh",
+			},
 		},
 		"defer": {
 			script:   scriptStep{"MAIL FROM:<john@gmail.com>", 250},
-			eventRes: nil,
+			eventRes: event.SMTPResponse{Action: event.ActionDefer},
 		},
 	}
 
@@ -449,7 +451,7 @@ func TestBeforeMailAcceptedEventResponse(t *testing.T) {
 		tc := tc
 		t.Run(name, func(t *testing.T) {
 			// Reset event listener.
-			shouldReturn = tc.eventRes
+			shouldReturn = &tc.eventRes
 			gotEvent = nil
 
 			// Play and verify SMTP session.