Cleanly handle player mailling lists (#140)

* Fix server hanging for certain recipient names

* Disallow recursive maillist inclusion

* Disallow sending to empty recipient

* Complement testcases

api.lua

@@ -51,6 +51,8 @@ function mail.send(m)
 			table.insert(undeliverable_reason, reason)
 		end
 		return false, table.concat(undeliverable_reason, "\n")
+	elseif not next(recipients) then
+		return false, S("You did not specify any valid recipient.")
 	end
 
 	local extra = {}

api.spec.lua

@@ -7,6 +7,12 @@ mail.register_recipient_handler(function(_, name)
         return false, "It works (?)"
     end
 end)
+mail.update_maillist("player1", {
+	owner = "player1",
+	name = "recursive",
+	desc = "",
+	players = {"@recursive", "player1"},
+}, "recursive")
 
 local received_count = {}
 mail.register_on_player_receive(function(player)
@@ -27,30 +33,44 @@ local function assert_inbox_count(player_name, count)
     assert(player_received == count, ("incorrect receive count: %d expected, got %d"):format(count, player_received))
 end
 
+local function assert_send(expected_success, ...)
+	local success, err = mail.send(...)
+	if expected_success then
+		assert(success, ("expected mail to be sent, got error message: %s"):format(err))
+		assert(not err, ("unexpected message after sending mail: %s"):format(err))
+	else
+		assert(not success, "expected mail to be rejected, mail was sent")
+		assert(type(err) == "string", ("expected error message, got datum of type %s"):format(type(err)))
+	end
+end
+
 mtt.register("send mail", function(callback)
+    -- local maillists
+    assert_send(true, {from = "player1", to = "@recursive", subject = "hello recursion", body = "blah"})
+    assert_inbox_count("player1", 1)
+    assert(sent_count == 1)
+
+    -- do not allow empty recipients
+    assert_send(false, {from = "player1", to = "@doesnotexist", subject = "should not be sent", body = "blah"})
+    assert(sent_count == 1)
+
     -- send a mail to a list
-    local success, err = mail.send({from = "player1", to = "list/test", subject = "something", body = "blah"})
-    assert(success)
-    assert(not err)
+    assert_send(true, {from = "player1", to = "list/test", subject = "something", body = "blah"})
     assert_inbox_count("player2", 1)
-    assert_inbox_count("player1", 0)
-    assert(sent_count == 1)
+    assert_inbox_count("player1", 1)
+    assert(sent_count == 2)
 
     -- send a second mail to the list and also the sender
-    success, err = mail.send({from = "player1", to = "list/test, alias/player1", subject = "something", body = "blah"})
-    assert(success)
-    assert(not err)
+    assert_send(true, {from = "player1", to = "list/test, alias/player1", subject = "something", body = "blah"})
     assert_inbox_count("player2", 2)
-    assert_inbox_count("player1", 1)
-    assert(sent_count == 2)
+    assert_inbox_count("player1", 2)
+    assert(sent_count == 3)
 
     -- send a mail to list/reject - the mail should be rejected
-    success, err = mail.send({from = "player1", to = "list/reject", subject = "something", body = "NO"})
-    assert(not success)
-    assert(type(err) == "string")
+    assert_send(false, {from = "player1", to = "list/reject", subject = "something", body = "NO"})
     assert_inbox_count("player2", 2)
-    assert_inbox_count("player1", 1)
-    assert(sent_count == 2)
+    assert_inbox_count("player1", 2)
+    assert(sent_count == 3)
 
     callback()
 end)

storage.lua

@@ -404,30 +404,37 @@ function mail.delete_maillist(playername, listname)
 	end
 end
 
-function mail.extractMaillists(receivers_string, maillists_owner)
-	local receivers = mail.parse_player_list(receivers_string) -- extracted receivers
-
-	-- extract players from mailing lists
-	while string.find(receivers_string, "@") do
-		local globalReceivers = mail.parse_player_list(receivers_string) -- receivers including maillists
-		receivers = {}
-		for _, receiver in ipairs(globalReceivers) do
-			local receiverInfo = receiver:split("@") -- @maillist
-			if receiverInfo[1] and receiver == "@" .. receiverInfo[1] then
-				local maillist = mail.get_maillist_by_name(maillists_owner, receiverInfo[1])
-				if maillist then
-					for _, playername in ipairs(maillist.players) do
-						table.insert(receivers, playername)
-					end
+local function extract_maillists_main(receivers, maillists_owner, expanded_receivers, seen)
+	if type(receivers) == "string" then
+		receivers = mail.parse_player_list(receivers)
+	end
+
+	for _, receiver in pairs(receivers) do
+		if seen[receiver] then
+			-- Do not add/expand this receiver as it is already seen
+			minetest.log("verbose", ("mail: ignoring duplicate receiver %q during maillist expansion"):format(receiver))
+		elseif string.find(receiver, "^@") then
+			seen[receiver] = true
+			local listname = string.sub(receiver, 2)
+			local maillist = mail.get_maillist_by_name(maillists_owner, listname)
+			if maillist then
+				minetest.log("verbose", ("mail: expanding maillist %q"):format(listname))
+				for _, entry in ipairs(maillist.players) do
+					extract_maillists_main(entry, maillists_owner, expanded_receivers, seen)
 				end
-			else -- in case of player
-				table.insert(receivers, receiver)
 			end
+		else
+			seen[receiver] = true
+			minetest.log("verbose", ("mail: adding %q to receiver list during maillist expansion"):format(receiver))
+			table.insert(expanded_receivers, receiver)
 		end
-		receivers_string = mail.concat_player_list(receivers)
 	end
+end
 
-	return receivers
+function mail.extractMaillists(receivers, maillists_owner)
+	local expanded_receivers = {}
+	extract_maillists_main(receivers, maillists_owner, expanded_receivers, {})
+	return expanded_receivers
 end
 
 function mail.get_setting_default_value(key)