From 7c39c154db4fad571ed840438c606522c82ae42d Mon Sep 17 00:00:00 2001 From: Brennan Kinney <5098581+polarathene@users.noreply.github.com> Date: Sat, 20 Jan 2024 19:59:28 +1300 Subject: [PATCH] Apply suggestions from code review - `_mid` / `MID` => `_msgid` / `MSG_ID` - Revised documentation / tooltip comments --- test/helper/log_and_filtering.bash | 33 +++++++++---------- test/helper/sending.bash | 28 +++++++++------- .../parallel/set1/spam_virus/postscreen.bats | 4 +-- .../parallel/set1/spam_virus/rspamd_full.bats | 10 +++--- 4 files changed, 40 insertions(+), 35 deletions(-) diff --git a/test/helper/log_and_filtering.bash b/test/helper/log_and_filtering.bash index f6c05a42..b7b63018 100644 --- a/test/helper/log_and_filtering.bash +++ b/test/helper/log_and_filtering.bash @@ -7,8 +7,8 @@ # shellcheck disable=SC2034,SC2155 -# Assert that the number of lines output by a previous command matches the given -# amount (${1}). `lines` is a special BATS variable updated via `run`. +# Assert that the number of lines output by a previous command matches the given amount (${1}). +# `lines` is a special BATS variable updated via `run`. # # @param ${1} = number of lines that the output should have function _should_output_number_of_lines() { @@ -16,8 +16,7 @@ function _should_output_number_of_lines() { assert_equal "${#lines[@]}" "${1:?Number of lines not provided}" } -# Filters a service's logs (under `/var/log/supervisor/.log`) given -# a specific string. +# Filters a service's logs (under `/var/log/supervisor/.log`) given a specific string. # # @param ${1} = service name # @param ${2} = string to filter by @@ -71,22 +70,23 @@ function _service_log_should_not_contain_string() { assert_failure } -# Filters the mail log according to MID (Message-ID) and prints lines -# of the mail log that fit Postfix's queue ID for the given message ID. +# Filters the mail log by the given MSG_ID (Message-ID) parameter, +# printing log lines which include the associated Postfix Queue ID. # -# @param ${1} = message ID part before '@' -function _print_mail_log_of_queue_id_from_mid() { - # The unique ID Postfix (and other services) use may be different in length - # on different systems. Hence, we use a range to safely capture it. +# @param ${1} = The local-part of a Message-ID header value (``) +function _print_mail_log_of_queue_id_from_msgid() { + # A unique ID Postfix generates for tracking queued mail as it's processed. + # The length can vary (as per the postfix docs). Hence, we use a range to safely capture it. + # https://github.com/docker-mailserver/docker-mailserver/pull/3747#discussion_r1446679671 local QUEUE_ID_REGEX='[A-Z0-9]{9,12}' - local MID=$(__construct_mid "${1:?Left-hand side of MID missing}") + local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}") shift 1 _wait_for_empty_mail_queue_in_container QUEUE_ID=$(_exec_in_container tac /var/log/mail.log \ - | grep -E "postfix/cleanup.*: ${QUEUE_ID_REGEX}:.*message-id=${MID}" \ + | grep -E "postfix/cleanup.*: ${QUEUE_ID_REGEX}:.*message-id=${MSG_ID}" \ | grep -E --only-matching --max-count 1 "${QUEUE_ID_REGEX}" || :) # We perform plausibility checks on the IDs. @@ -97,13 +97,12 @@ function _print_mail_log_of_queue_id_from_mid() { _filter_service_log 'mail' "${QUEUE_ID}" } -# Filters the mail log according to MID (Message-ID) and prints lines -# of the mail log that fit lines with the pattern `msgid=${1}@dms-test`. +# A convenience method that filters for Dovecot specific logs with a `msgid` field that matches the MSG_ID input. # -# @param ${1} = message ID part before '@' +# @param ${1} = The local-part of a Message-ID header value (``) function _print_mail_log_for_msgid() { - local MID=$(__construct_mid "${1:?Left-hand side of MID missing}") + local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}") shift 1 - _filter_service_log 'mail' "msgid=${MID}" + _filter_service_log 'mail' "msgid=${MSG_ID}" } diff --git a/test/helper/sending.bash b/test/helper/sending.bash index bd5f8bc8..60a4e359 100644 --- a/test/helper/sending.bash +++ b/test/helper/sending.bash @@ -102,19 +102,25 @@ function _send_email() { return "${RETURN_VALUE}" } -# Contruct the message ID for the 'Message-ID' header. +# Construct the value for a 'Message-ID' header. +# For tests we use only the local-part to identify mail activity in logs. The rest of the value is fixed. # -# @param ${1} = message ID part before '@' (later used when filtering logs again) -function __construct_mid() { - echo "<${1:?Message-ID prefix is required}@dms-tests>" +# A Message-ID header value should be in the form of: `` +# https://en.wikipedia.org/wiki/Message-ID +# https://datatracker.ietf.org/doc/html/rfc5322#section-3.6.4 +# +# @param ${1} = The local-part of a Message-ID header value (``) +function __construct_msgid() { + local MSG_ID_LOCALPART=${1:?The local-part for MSG_ID was not provided} + echo "<${MSG_ID_LOCALPART}@dms-tests>" } -# Like `_send_email` but adds a "Message-Id: ${1}@dms-tests>" header, which -# allows for filtering logs later. +# Like `_send_email` but adds a "Message-ID: ${1}@dms-tests>" header, +# which allows for filtering logs later. # -# @param ${1} = message ID part before '@' (later used when filtering logs again) -function _send_email_with_mid() { - local MID=$(__construct_mid "${1:?Left-hand side of MID missing}") +# @param ${1} = The local-part of a Message-ID header value (``) +function _send_email_with_msgid() { + local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}") shift 1 _send_email "${@}" --header "Message-Id: ${MID}" @@ -122,9 +128,9 @@ function _send_email_with_mid() { # Send a spam e-mail by utilizing GTUBE. # -# Extra arguments given to this function will be supplied by `_send_email_with_mid` directly. +# Extra arguments given to this function will be supplied by `_send_email_with_msgid` directly. function _send_spam() { - _send_email_with_mid 'spam' "${@}" \ + _send_email_with_msgid 'spam' "${@}" \ --from 'spam@external.tld' \ --body 'XJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X' } diff --git a/test/tests/parallel/set1/spam_virus/postscreen.bats b/test/tests/parallel/set1/spam_virus/postscreen.bats index 78d8edaa..d1c308a7 100644 --- a/test/tests/parallel/set1/spam_virus/postscreen.bats +++ b/test/tests/parallel/set1/spam_virus/postscreen.bats @@ -57,8 +57,8 @@ function teardown_file() { # Configure `send_email()` to send from the mail client container (CONTAINER2_NAME) via ENV override, # mail is sent to the DMS server container (CONTAINER1_NAME) via `--server` parameter: CONTAINER_NAME=${CONTAINER2_NAME} _send_email --expect-rejection --server "${CONTAINER1_IP}" --port 25 --data 'postscreen.txt' - # TODO: Use _send_email_with_mid when proper resolution of domain names is possible: - # CONTAINER_NAME=${CONTAINER2_NAME} _send_email_with_mid 'postscreen' --server "${CONTAINER1_IP}" --data 'postscreen.txt' + # TODO: Use _send_email_with_msgid when proper resolution of domain names is possible: + # CONTAINER_NAME=${CONTAINER2_NAME} _send_email_with_msgid 'postscreen' --server "${CONTAINER1_IP}" --data 'postscreen.txt' # _print_mail_log_for_msgid 'postscreen' # assert_output --partial "stored mail into mailbox 'INBOX'" diff --git a/test/tests/parallel/set1/spam_virus/rspamd_full.bats b/test/tests/parallel/set1/spam_virus/rspamd_full.bats index 1ecedf25..875655d3 100644 --- a/test/tests/parallel/set1/spam_virus/rspamd_full.bats +++ b/test/tests/parallel/set1/spam_virus/rspamd_full.bats @@ -45,16 +45,16 @@ function setup_file() { # We will send 4 emails: # 1. The first one should pass just fine - _send_email_with_mid 'pass' + _send_email_with_msgid 'pass' # 2. The second one should be rejected (Rspamd-specific GTUBE pattern for rejection) _send_spam --expect-rejection # 3. The third one should be rejected due to a virus (ClamAV EICAR pattern) # shellcheck disable=SC2016 - _send_email_with_mid 'virus' --expect-rejection \ + _send_email_with_msgid 'virus' --expect-rejection \ --body 'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' # 4. The fourth one will receive an added header (Rspamd-specific GTUBE pattern for adding a spam header) # ref: https://rspamd.com/doc/gtube_patterns.html - _send_email_with_mid 'header' --body "YJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X" + _send_email_with_msgid 'header' --body "YJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X" _run_in_container cat /var/log/mail.log assert_success @@ -120,7 +120,7 @@ function teardown_file() { _default_teardown ; } _service_log_should_contain_string 'rspamd' 'S \(reject\)' _service_log_should_contain_string 'rspamd' 'reject "Gtube pattern"' - _print_mail_log_of_queue_id_from_mid 'spam' + _print_mail_log_of_queue_id_from_msgid 'spam' assert_output --partial 'milter-reject' assert_output --partial '5.7.1 Gtube pattern' refute_output --partial "stored mail into mailbox 'INBOX'" @@ -135,7 +135,7 @@ function teardown_file() { _default_teardown ; } _service_log_should_contain_string 'rspamd' 'T \(reject\)' _service_log_should_contain_string 'rspamd' 'reject "ClamAV FOUND VIRUS "Eicar-Signature"' - _print_mail_log_of_queue_id_from_mid 'virus' + _print_mail_log_of_queue_id_from_msgid 'virus' assert_output --partial 'milter-reject' assert_output --partial '5.7.1 ClamAV FOUND VIRUS "Eicar-Signature"' refute_output --partial "stored mail into mailbox 'INBOX'"