Misc changes associated with the SMTP and POP unit test code review.

7 files changed, 74 insertions, 86 deletions

diff --git a/check/magma/magma_check.h b/check/magma/magma_check.h
index d9f2c238..dcf3eb71 100644
--- a/check/magma/magma_check.h
+++ b/check/magma/magma_check.h
@@ -35,7 +35,7 @@
 #include "servers/pop/pop_check.h"
 #include "servers/imap/imap_check.h"
 #include "servers/http/http_check.h"
-#include <servers/camel/camel_check.h>
+#include "servers/camel/camel_check.h"
 #include "regression/regression_check.h"
 #include "config/config_check.h"
 
diff --git a/check/magma/regression/regression_check_helpers.c b/check/magma/regression/regression_check_helpers.c
index 96f5d381..bf2916bf 100644
--- a/check/magma/regression/regression_check_helpers.c
+++ b/check/magma/regression/regression_check_helpers.c
@@ -55,20 +55,19 @@ bool_t check_regression_smtp_dot_stuffing_sthread(stringer_t *errmsg) {
 
 	client_t *client = NULL;
 	server_t *server = NULL;
-	uint64_t message_num = 0;
+	uint64_t messagenum = 0;
 	stringer_t *mailfrom = "magma@lavabit.com", *rcptto = NULLER("princess@example.com"),
-		*message = NULLER(
-		"To: \"Magma\" <magma@lavabit.com>\r\n"\
-		"From: \"Princess\" <princess@example.com\r\n"\
-		"Subject: Dot Stuffing Regression Test\r\n"\
-		"This is an SMTP message whose body has a period at the start of a line\r\n"\
-		". In fact, there are two instances of this in the body of this message\r\n"\
-		". The SMTP client code should stuff an extra period after each of them.\r\n"\
+		*message = NULLER("To: \"Magma\" <magma@lavabit.com>\r\n" \
+		"From: \"Princess\" <princess@example.com>\r\n" \
+		"Subject: Dot Stuffing Regression Test\r\n\r\n" \
+		"This is an SMTP message whose body has a period at the start of a line\r\n" \
+		". In fact, there are two instances of this in the body of this message\r\n" \
+		". The SMTP client code should stuff an extra period after each of them.\r\n" \
 		".\r\n");
 
 	// First, send the message with periods at the beginning of lines in the body.
 	if (!(client = smtp_client_connect(0))) {
-		st_sprint(errmsg, "Failed to connect to an SMTP server.");
+		st_sprint(errmsg, "Failed to connect with the SMTP server.");
 		return false;
 	}
 	else if (smtp_client_send_helo(client) != 1) {
@@ -94,6 +93,9 @@ bool_t check_regression_smtp_dot_stuffing_sthread(stringer_t *errmsg) {
 
 	smtp_client_close(client);
 
+	/// LOW: Split this unit test in the SMTP client dot stuff test, and the POP dot stuff test. In theory the latter should
+	/// 	add a test message using mail_store_message() and then find the precise UID for said test message via POP.
+
 	// Next, check if the entire message was sent to the recipient.
 	if (!(server = servers_get_by_protocol(POP, false)) || !(client = client_connect("localhost", server->network.port)) ||
 		!net_set_timeout(client->sockd, 20, 20) || client_read_line(client) <= 0 || client_status(client) != 1 ||
@@ -108,14 +110,14 @@ bool_t check_regression_smtp_dot_stuffing_sthread(stringer_t *errmsg) {
 		client_close(client);
 		return false;
 	}
-	else if (client_write(client, PLACER("LIST\r\n", 6)) != 6 || (message_num = check_pop_client_read_list(client, errmsg)) == 0 ||
+	else if (client_write(client, PLACER("LIST\r\n", 6)) != 6 || (messagenum = check_pop_client_read_list(client, errmsg)) == 0 ||
 		client_status(client) != 1) {
 
 		if (st_empty(errmsg)) st_sprint(errmsg, "Failed to return successful state after LIST.");
 		client_close(client);
 		return false;
 	}
-	else if (client_print(client, "TOP %lu 0\r\n", message_num) != (uint16_digits(message_num) + 8) || client_status(client) != 1) {
+	else if (client_print(client, "TOP %lu 0\r\n", messagenum) != (uint16_digits(messagenum) + 8) || client_status(client) != 1) {
 
 		st_sprint(errmsg, "Failed to return successful status after TOP.");
 		client_close(client);
diff --git a/check/magma/servers/pop/pop_check.c b/check/magma/servers/pop/pop_check.c
index 6e3fb089..b3b1c45e 100644
--- a/check/magma/servers/pop/pop_check.c
+++ b/check/magma/servers/pop/pop_check.c
@@ -20,9 +20,6 @@ START_TEST (check_pop_network_basic_tcp_s) {
 	else if (status() && !check_pop_network_basic_sthread(errmsg, tcp->network.port, false)) {
 		outcome = false;
 	}
-	else {
-		errmsg = NULL;
-	}
 
 	log_test("POP / NETWORK / BASIC / TCP / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -42,7 +39,6 @@ START_TEST (check_pop_network_basic_tls_s) {
 	}
 	else if (status() && !check_pop_network_basic_sthread(errmsg, tls->network.port, true)) {
 		outcome = false;
-		errmsg = NULL;
 	}
 
 	log_test("POP / NETWORK / BASIC / TLS / SINGLE THREADED:", errmsg);
diff --git a/check/magma/servers/pop/pop_check_network.c b/check/magma/servers/pop/pop_check_network.c
index b45c477d..348e683d 100644
--- a/check/magma/servers/pop/pop_check_network.c
+++ b/check/magma/servers/pop/pop_check_network.c
@@ -12,61 +12,71 @@
  * @brief 	Calls client_read_line on a client until it reaches a period only line, returning
  * 			the number of messages in the inbox.
  *
- * @param 	client 	The client_t* to read from (which should be connected to a POP server)
- * @param	size	A uint64_t*. If not null, the total size of the lines read will be placed
+ * @param 	client 	The client_t pointer to read from (which should be connected to a POP server)
+ * @param	size	A uint64_t pointer. If not NULL, the total size of the lines read will be placed
  * 					at this address.
- * @param	token	If not NULL and size if not NULL, then size will only be incremented after
- * 					reaching a line that begins with token.
+ * @param	token	If not NULL, then the size variable will only include the number of bytes read after the token.
  * @return 	true if a line containing a single period is found, false if not.
  */
 bool_t check_pop_client_read_end(client_t *client, uint64_t *size, chr_t *token) {
 
-	if (size) *size = 0;
 	bool_t token_found = false;
 
+	if (size) *size = 0;
+	else if (!token) token_found = true;
+
+	// There shouldn't be a token, if we aren't also supposed to be counting the number of bytes.
+	else if (!size && token) return false;
+
 	while (client_read_line(client) > 0) {
 
+		// Break when a line with just a period is found.
 		if (!st_cmp_cs_eq(&(client->line), NULLER(".\r\n"))) return true;
-		else if (size && st_cmp_cs_starts(&(client->line), NULLER(token)) == 0) token_found = true;
+
+		// If we have a size and a token, then keep checking for the token until its found.
+		else if (size && token && !token_found && st_cmp_cs_starts(&(client->line), NULLER(token)) == 0) token_found = true;
 
 		if (size && token_found) *size += pl_length_get(client->line);
 	}
+
 	return false;
 }
 
 /**
- * Calls 	client_read_line on a client until it reaches a period only line, returning the
- * 			number of messages in the inbox.
+ * @brief	Calls client_read_line until it reaches a line containing only a period, then returns the number
+ * 			of messages it encountered.
  *
- * @param 	client 	The client_t* to read from (which should be connected to a POP server).
- * @param 	errmsg	The stringer_t* to which error messages will be printed in event of an error.
- * @return 	a uint32_t containing the number of messages in the inbox.
+ * @param 	client 	The client_t pointer to read from (which should be connected to a POP server).
+ * @param 	errmsg	The stringer_t pointer to which error messages will be printed in event of an error.
+ * @return 	an uint32_t containing the number of messages in the inbox.
  */
 uint64_t check_pop_client_read_list(client_t *client, stringer_t *errmsg) {
 
 	placer_t fragment = pl_null();
 	uint64_t counter = 1, sequence = 0;
 
-	client_read_line(client);
 
-	while (client_read_line(client) > 0) {
+	/// LOW: Parse out the total message number from the first line returned and check against that at the end of the
+	/// 	function, returning an error if it and the counter do not match.
+	if (client_read_line(client) <= 0 || !pl_starts_with_char(client->line, '+')) {
+		st_sprint(errmsg, "The message list response failed to return a valid response.");
+		return 0;
+	}
+	while (client_read_line(client) > 0 && !pl_starts_with_char(client->line, '.')) {
 
-		if (pl_starts_with_char(client->line, '.')) {
-			return counter-2;
-		}
-		else if (tok_get_st(&(client->line), ' ', 0, &fragment) >= 0 && !uint64_conv_pl(fragment, &sequence) == 0) {
-			if (sequence != counter) return 0;
-		}
-		else {
+		// If the sequence number doesn't match our counter variable, we'll indicate an error.
+		if (tok_get_st(&(client->line), ' ', 0, &fragment) >= 0 && !uint64_conv_pl(fragment, &sequence) == 0 && sequence != counter) {
+			st_sprint(errmsg, "The message sequence appears to have skipped, because the internal counter no longer matches the sequence.");
 			return 0;
 		}
 
 		counter++;
 	}
 
-	return 0;
+	return counter - 1;
 }
 
+/// LOW: This should use stringer parameters.
 bool_t check_pop_client_auth(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg) {
 
 	if (client_print(client, "USER %s\r\n", user) != (ns_length_get(user) + 7) || client_read_line(client) <= 0 ||
diff --git a/check/magma/servers/smtp/smtp_check.c b/check/magma/servers/smtp/smtp_check.c
index a0b7e8d3..836d3072 100644
--- a/check/magma/servers/smtp/smtp_check.c
+++ b/check/magma/servers/smtp/smtp_check.c
@@ -21,9 +21,6 @@ START_TEST (check_smtp_network_basic_tcp_s) {
 	else if (status() && !check_smtp_network_basic_sthread(errmsg, server->network.port, false)) {
 		outcome = false;
 	}
-	else {
-		errmsg = NULL;
-	}
 
 	log_test("SMTP / NETWORK / BASIC / TCP / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -44,9 +41,6 @@ START_TEST (check_smtp_network_basic_tls_s) {
 	else if (status() && !check_smtp_network_basic_sthread(errmsg, server->network.port, true)) {
 		outcome = false;
 	}
-	else {
-		errmsg = NULL;
-	}
 
 	log_test("SMTP / NETWORK / BASIC / TLS / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -59,8 +53,7 @@ START_TEST (check_smtp_accept_store_message_s) {
 	bool_t outcome = true;
 	stringer_t *errmsg = MANAGEDBUF(2048);
 
-	outcome = check_smtp_accept_message_sthread(errmsg);
-	if (outcome) errmsg = NULL;
+	if (status()) outcome = check_smtp_accept_message_sthread(errmsg);
 
 	log_test("SMTP / ACCEPT / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -73,8 +66,7 @@ START_TEST (check_smtp_checkers_greylist_s) {
 	bool_t outcome = true;
 	stringer_t *errmsg = MANAGEDBUF(1024);
 
-	outcome = check_smtp_checkers_greylist_sthread(errmsg);
-	if (outcome) errmsg = NULL;
+	if (status()) outcome = check_smtp_checkers_greylist_sthread(errmsg);
 
 	log_test("SMTP / CHECKERS / GREYLIST / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -93,8 +85,6 @@ START_TEST (check_smtp_checkers_filters_s) {
 	if (status() && outcome) outcome = check_smtp_checkers_filters_sthread(errmsg, SMTP_FILTER_ACTION_LABEL, 3);
 	if (status() && outcome) outcome = check_smtp_checkers_filters_sthread(errmsg, SMTP_FILTER_ACTION_MARK_READ, 4);
 
-	if (outcome) errmsg = NULL;
-
 	log_test("SMTP / CHECKERS / FILTERS / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
 }
@@ -114,9 +104,10 @@ START_TEST (check_smtp_network_auth_plain_s) {
 	else if (status() && !check_smtp_network_auth_sthread(errmsg, server->network.port, false)) {
 		outcome = false;
 	}
-	else {
-		errmsg = NULL;
-	}
+
+	/// LOW: Add a variation of this test which takes place over TCP and thus fails specifically because the connection
+	/// 	lacks transport security (aka TLS). In other words, test for valid credentials first, and that it works via TLS,
+	/// 	before ensuring the same inputs fail via TCP.
 
 	log_test("SMTP / NETWORK / AUTH PLAIN / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
@@ -137,9 +128,10 @@ START_TEST (check_smtp_network_auth_login_s) {
 	else if (status() && !check_smtp_network_auth_sthread(errmsg, server->network.port, true)) {
 		outcome = false;
 	}
-	else {
-		errmsg = NULL;
-	}
+
+	/// LOW: Add a variation of this test which takes place over TCP and thus fails specifically because the connection
+	/// 	lacks transport security (aka TLS). In other words, test for valid credentials first, and that it works via TLS,
+	/// 	before ensuring the same inputs fail via TCP.
 
 	log_test("SMTP / NETWORK / AUTH LOGIN / SINGLE THREADED:", errmsg);
 	ck_assert_msg(outcome, st_char_get(errmsg));
diff --git a/check/magma/servers/smtp/smtp_check.h b/check/magma/servers/smtp/smtp_check.h
index 0e55dfbf..845b9f43 100644
--- a/check/magma/servers/smtp/smtp_check.h
+++ b/check/magma/servers/smtp/smtp_check.h
@@ -18,8 +18,8 @@ bool_t check_smtp_checkers_filters_sthread(stringer_t *errmsg, int_t action, int
 /// smtp_check_network.c
 bool_t check_smtp_client_read_end(client_t *client);
 bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to, stringer_t *errmsg);
-bool_t check_smtp_client_auth_plain(client_t *client, chr_t *pass, stringer_t *errmsg);
-bool_t check_smtp_client_auth_login(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg);
+bool_t check_smtp_client_auth_plain(client_t *client, stringer_t *auth);
+bool_t check_smtp_client_auth_login(client_t *client, stringer_t *user, stringer_t *pass);
 bool_t check_smtp_client_quit(client_t *client, stringer_t *errmsg);
 bool_t check_smtp_network_basic_sthread(stringer_t *errmsg, uint32_t port, bool_t secure);
 bool_t check_smtp_network_auth_sthread(stringer_t *errmsg, uint32_t port, bool_t login);
diff --git a/check/magma/servers/smtp/smtp_check_network.c b/check/magma/servers/smtp/smtp_check_network.c
index 0a68fbed..2f75098f 100644
--- a/check/magma/servers/smtp/smtp_check_network.c
+++ b/check/magma/servers/smtp/smtp_check_network.c
@@ -34,7 +34,7 @@ bool_t check_smtp_client_read_end(client_t *client) {
 bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to, stringer_t *errmsg) {
 
 	chr_t *line_from = "MAIL FROM: <%s>\r\n", *line_to = "RCPT TO: <%s>\r\n";
-	size_t size_from = ns_length_get(line_from) + ns_length_get(from) -2, size_to = ns_length_get(line_to) + ns_length_get(to) -2;
+	size_t size_from = ns_length_get(line_from) + ns_length_get(from) - 2, size_to = ns_length_get(line_to) + ns_length_get(to) - 2;
 
 	// Issue MAIL command.
 	if (client_print(client, line_from, from) != size_from || !check_smtp_client_read_end(client) ||
@@ -69,15 +69,11 @@ bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to
  * 					and error
  * @return 	True if no errors, false otherwise
  */
-bool_t check_smtp_client_auth_plain(client_t *client, chr_t *auth, stringer_t *errmsg) {
+bool_t check_smtp_client_auth_plain(client_t *client, stringer_t *auth) {
 
-	chr_t *line_auth = "AUTH PLAIN %s\r\n";
-
-	if (client_print(client, line_auth, auth) != (ns_length_get(line_auth) + ns_length_get(auth)) -2 ||
+	if (client_print(client, "AUTH PLAIN %.*s\r\n", st_length_int(auth), st_char_get(auth)) != st_length_get(auth) + 13 ||
 		!check_smtp_client_read_end(client) || client_status(client) != 1 ||
 		st_cmp_cs_starts(&(client->line), NULLER("235"))) {
-
-		st_sprint(errmsg, "Failed to return a successful status after submitting credentials");
 		return false;
 	}
 
@@ -88,30 +84,22 @@ bool_t check_smtp_client_auth_plain(client_t *client, chr_t *auth, stringer_t *e
  * @brief 	Submits the AUTH LOGIN command to the passed client using the passed parameters
  * @param 	client 	A client_t* connected to an SMTP server that has had the HELO/EHLO command
  * 					already submitted
- * @param 	user 	A chr_t* containing the username of the user
- * @param 	pass 	A chr_t* containing the password of the user
- * @param 	errmsg 	A stringer_t* that will have the error message printed to it in the event of
- * 					and error
- * @return 	True if no errors, false otherwise
+ * @param 	user 	A NULL string containing the username of the user.
+ * @param 	pass 	A NULL string containing the password of the user.
+ * @return 	true if no errors, false otherwise
  */
-bool_t check_smtp_client_auth_login(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg) {
+bool_t check_smtp_client_auth_login(client_t *client, stringer_t *user, stringer_t *pass) {
 
 	if (client_write(client, PLACER("AUTH LOGIN\r\n", 12)) != 12 || !check_smtp_client_read_end(client) ||
 		client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) {
-
-		st_sprint(errmsg, "Failed to return a proceed status code after AUTH LOGIN.");
 		return false;
 	}
-	else if (client_print(client, "%s\r\n", user) != ns_length_get(user) + 2 || !check_smtp_client_read_end(client) ||
-		client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) {
-
-		st_sprint(errmsg, "Failed to return a proceed status code after submitting username.");
+	else if (client_print(client, "%.*s\r\n", st_length_int(user), st_char_get(user)) != st_length_get(user) + 2 ||
+		!check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) {
 		return false;
 	}
-	else if (client_print(client, "%s\r\n", pass) != ns_length_get(pass) + 2 || !check_smtp_client_read_end(client) ||
-		client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("235"))) {
-
-		st_sprint(errmsg, "Failed to return a successful status after submitting credentials.");
+	else if (client_print(client, "%.*s\r\n", st_length_int(pass), st_char_get(pass)) != st_length_get(pass) + 2 ||
+		!check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("235"))) {
 		return false;
 	}
 
@@ -250,16 +238,16 @@ bool_t check_smtp_network_auth_sthread(stringer_t *errmsg, uint32_t port, bool_t
 		return false;
 	}
 	// Issue AUTH with incorrect credentials.
-	else if ((login ? check_smtp_client_auth_login(client, "bWFnbWE=", "aW52YWxpZHBhc3N3b3Jk", errmsg)
-			: check_smtp_client_auth_plain(client, "bWFnbWEAbWFnbWEAaW52YWxpZHBhc3N3b3Jk", errmsg))) {
-
+	else if ((login ? check_smtp_client_auth_login(client,  NULLER("bWFnbWE="),  NULLER("aW52YWxpZHBhc3N3b3Jk"))
+			: check_smtp_client_auth_plain(client,  NULLER("bWFnbWEAbWFnbWEAaW52YWxpZHBhc3N3b3Jk")))) {
+		st_sprint(errmsg, "Invalid credentials appear to have authenticated when they should have failed.");
 		client_close(client);
 		return false;
 	}
 	// Issue AUTH with correct credentials.
-	else if (!(login ? check_smtp_client_auth_login(client, "bWFnbWE=", "cGFzc3dvcmQ=", errmsg)
-			: check_smtp_client_auth_plain(client, "bWFnbWEAbWFnbWEAcGFzc3dvcmQ=", errmsg))) {
-
+	else if (!(login ? check_smtp_client_auth_login(client, NULLER("bWFnbWE="),  NULLER("cGFzc3dvcmQ="))
+			: check_smtp_client_auth_plain(client,  NULLER("bWFnbWEAbWFnbWEAcGFzc3dvcmQ=")))) {
+		st_sprint(errmsg, "Failed to authenticate even though we supplied valid credentials.");
 		client_close(client);
 		return false;
 	}