diff --git a/CHANGELOG.md b/CHANGELOG.md index 06fb6063..844a0b1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - **`n_procs` parallel parsing now covers messages from mbox files and mailbox connections** ([#147](https://github.com/domainaware/parsedmarc/issues/147)), not just report files passed directly as CLI arguments: IMAP, Microsoft Graph, Gmail API, and Maildir connections, including watch mode. `get_dmarc_reports_from_mbox()`, `get_dmarc_reports_from_mailbox()`, and `watch_inbox()` all gained an `n_procs` keyword argument. Parallel workers are now a reused process pool for the whole run, with a bounded submission window that keeps at most roughly `2 * n_procs` messages in flight at a time so memory stays bounded even for huge mboxes; the main process sends periodic IMAP keepalives while workers parse. - **Added `ParserConfig`, a single frozen dataclass carrying every parsing/enrichment option** ([#503](https://github.com/domainaware/parsedmarc/issues/503)): offline mode, IP database path, reverse DNS map and PSL overrides paths/URLs, DNS nameservers/timeout/retries, `strip_attachment_payloads`, `normalize_timespan_threshold_hours`, and the three caches the parser and enrichment code share across calls (IP address info, seen aggregate report IDs, and the reverse DNS map). `parse_aggregate_report_xml()`, `parse_aggregate_report_file()`, `parse_failure_report()`, `parse_report_email()`, `parse_report_file()`, `get_dmarc_reports_from_mbox()`, `get_dmarc_reports_from_mailbox()`, and `watch_inbox()` all accept a keyword-only `config=` argument; when it's provided, the individual option keyword arguments are ignored in favor of the config's values, and every existing per-option keyword argument continues to work unchanged when `config=` is omitted. Every explicitly constructed `ParserConfig` owns fresh, isolated caches; omitting `config=` falls back to the existing process-wide shared default caches, unchanged and identity-preserved (`parsedmarc.IP_ADDRESS_CACHE`, `parsedmarc.SEEN_AGGREGATE_REPORT_IDS`, `parsedmarc.REVERSE_DNS_MAP`). Caches never cross multiprocessing worker boundaries: `ParserConfig.__getstate__` drops the three cache fields when a config is pickled for a worker, and each worker accumulates its own from that point on. `keep_alive` and `n_procs` remain separate keyword arguments — they control process/worker orchestration, not parsing or enrichment behavior, so they're deliberately not `ParserConfig` fields. See the new "Using parsedmarc as a library" section of the usage docs for an example. - **Added a "Domain policy" dropdown filter to the Splunk aggregate DMARC dashboard** ([#854](https://github.com/domainaware/parsedmarc/issues/854)): filters every panel on the published DMARC policy (`published_policy.p`) or subdomain policy (`published_policy.sp`), making it easy to see which domains with a `none` policy are ready to move to `quarantine` or `reject`. It sits immediately before the "Message disposition" dropdown and offers the same choices (`any`/`none`/`quarantine`/`reject`, defaulting to `any`). +- **New `[mailbox]` options `delete_aggregate`, `delete_failure`, `delete_smtp_tls`, and `delete_invalid`** ([#256](https://github.com/domainaware/parsedmarc/issues/256)): control message deletion per report type instead of all-or-nothing. Each defaults to the value of the overall `delete` option, so existing configurations behave exactly as before; set one to override just that type — for example `delete = True` with `delete_failure = False` deletes processed aggregate and SMTP TLS report messages while archiving failure report messages. `delete_invalid` covers messages that could not be parsed, so they can be kept in the `Invalid` archive subfolder for debugging even when everything else is deleted. `get_dmarc_reports_from_mailbox()` and `watch_inbox()` gained matching `delete_aggregate`, `delete_failure`, `delete_smtp_tls`, and `delete_invalid` keyword arguments, where `None` (the default) means "inherit `delete`". The mutual exclusion between deletion and `test` mode is now evaluated against the effective per-type flags: any type that would be deleted still raises a `ValueError` alongside `test`, but `delete = True` with all four per-type options explicitly `False` is now valid with `test` enabled, since nothing would be deleted. - **New `[general]` option `archive_directory`**: move successfully processed local report files into a dated archive tree (`////`); files that fail to parse as a report go to `/Invalid/`, while files that fail for other reasons (e.g. transient I/O errors) are left in place so a later run can retry them; existing files are never overwritten (numeric suffix); files already under the archive directory are excluded from later runs. Applies only to report files passed directly as local path arguments ([#570](https://github.com/domainaware/parsedmarc/issues/570)). ### Bug fixes diff --git a/docs/source/usage.md b/docs/source/usage.md index 9270d25a..d182ca17 100644 --- a/docs/source/usage.md +++ b/docs/source/usage.md @@ -216,6 +216,29 @@ The full set of configuration options are: messages as they arrive or poll MS Graph for new messages - `delete` - bool: Delete messages after processing them, instead of archiving them + - `delete_aggregate` - bool: Delete aggregate report messages + after processing them, instead of archiving them + (Default: the value of `delete`) + - `delete_failure` - bool: Delete failure report messages + after processing them, instead of archiving them + (Default: the value of `delete`) + - `delete_smtp_tls` - bool: Delete SMTP TLS report messages + after processing them, instead of archiving them + (Default: the value of `delete`) + - `delete_invalid` - bool: Delete messages that could not be + parsed, instead of archiving them in the `Invalid` + subfolder, where they can be inspected for debugging + (Default: the value of `delete`) + + :::{note} + Each of these four options overrides `delete` for one kind of + message only, and the other three keep inheriting `delete`. So + `delete = True` combined with `delete_failure = False` archives + failure report messages while deleting processed aggregate and + SMTP TLS report messages — and unparseable ones, unless + `delete_invalid = False` is set as well. + ::: + - `test` - bool: Do not move or delete messages - `batch_size` - int: Number of messages to read and process before saving. Default `10`. Use `0` for no limit. diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index 4148d679..c449af25 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -2243,6 +2243,8 @@ def _dispose_invalid_message( ) -> None: """Delete or move an unparseable message, per ``delete``. + Callers pass their effective ``delete_invalid`` value as ``delete``. + Shared, unmodified, by the sequential and parallel branches of ``get_dmarc_reports_from_mailbox`` so both dispose of invalid messages identically. @@ -2427,6 +2429,10 @@ def get_dmarc_reports_from_mailbox( reports_folder: str = "INBOX", archive_folder: str = "Archive", delete: bool = False, + delete_aggregate: bool | None = None, + delete_failure: bool | None = None, + delete_smtp_tls: bool | None = None, + delete_invalid: bool | None = None, test: bool = False, ip_db_path: str | None = None, always_use_local_files: bool = False, @@ -2452,7 +2458,23 @@ def get_dmarc_reports_from_mailbox( connection: A Mailbox connection object reports_folder (str): The folder where reports can be found archive_folder (str): The folder to move processed mail to - delete (bool): Delete messages after processing them + delete (bool): Delete messages after processing them + delete_aggregate (bool | None): Delete aggregate report messages + after processing them, instead of moving them to the + ``Aggregate`` archive subfolder; ``None`` (the default) inherits + the value of ``delete`` + delete_failure (bool | None): Delete failure report messages after + processing them, instead of moving them to the ``Failure`` + archive subfolder; ``None`` (the default) inherits the value of + ``delete`` + delete_smtp_tls (bool | None): Delete SMTP TLS report messages after + processing them, instead of moving them to the ``SMTP-TLS`` + archive subfolder; ``None`` (the default) inherits the value of + ``delete`` + delete_invalid (bool | None): Delete unparseable messages, instead + of moving them to the ``Invalid`` archive subfolder where they + can be inspected for debugging; ``None`` (the default) inherits + the value of ``delete`` test (bool): Do not move or delete messages after processing them ip_db_path (str): Path to a MMDB file from IPinfo, MaxMind, or DBIP always_use_local_files (bool): Do not download files @@ -2481,15 +2503,34 @@ def get_dmarc_reports_from_mailbox( interleaved message-by-message as it is when ``n_procs`` is 1. Not part of ``config``; always applies. config (ParserConfig): a single object carrying all parsing and - enrichment options plus the caches; when provided, the - individual option keyword arguments listed above are ignored in - favor of the config's values. + enrichment options plus the caches; when provided, it replaces + the individual parsing and enrichment option keyword arguments + listed above (DNS, GeoIP, offline mode, attachment payload + stripping, timespan normalization), whose values are then + ignored. The remaining keyword arguments control mailbox + handling and orchestration rather than parsing (the folder + names, the ``delete`` options, ``test``, ``since``, + ``batch_size``); they are not part of ``config`` and always + apply. Returns: dict: Lists of ``aggregate_reports``, ``failure_reports``, and ``smtp_tls_reports`` """ - if delete and test: - raise ValueError("delete and test options are mutually exclusive") + # Each per-report-type flag inherits the overall ``delete`` value when it + # is left unset (``None``). Resolve once, up front: every decision below + # reads only these four resolved flags. The raw ``delete`` parameter is + # still forwarded verbatim to the recursive self-call at the end of this + # function, where it is inert because all four resolved flags accompany + # it. + delete_aggregate = delete if delete_aggregate is None else delete_aggregate + delete_failure = delete if delete_failure is None else delete_failure + delete_smtp_tls = delete if delete_smtp_tls is None else delete_smtp_tls + delete_invalid = delete if delete_invalid is None else delete_invalid + + if test and ( + delete_aggregate or delete_failure or delete_smtp_tls or delete_invalid + ): + raise ValueError("delete options and test are mutually exclusive") if connection is None: raise ValueError("Must supply a connection") @@ -2646,7 +2687,10 @@ def get_dmarc_reports_from_mailbox( if not test: for invalid_message_id in invalid_msg_ids: _dispose_invalid_message( - connection, invalid_message_id, delete, invalid_reports_folder + connection, + invalid_message_id, + delete_invalid, + invalid_reports_folder, ) else: for i in range(message_limit): @@ -2678,73 +2722,57 @@ def get_dmarc_reports_from_mailbox( logger.warning(error.__str__()) if not test: _dispose_invalid_message( - connection, message_id, delete, invalid_reports_folder + connection, message_id, delete_invalid, invalid_reports_folder ) if not test: - if delete: - processed_messages = ( - aggregate_report_msg_uids + failure_report_msg_uids + smtp_tls_msg_uids - ) - - number_of_processed_msgs = len(processed_messages) - for i in range(number_of_processed_msgs): - msg_uid = processed_messages[i] - logger.debug( - f"Deleting message {i + 1} of {number_of_processed_msgs}: UID {msg_uid}" - ) - try: - connection.delete_message(msg_uid) - - except Exception as e: - message = "Error deleting message UID" - e = f"{message} {msg_uid}: {e}" - logger.error(f"Mailbox error: {e}") - else: - if len(aggregate_report_msg_uids) > 0: - log_message = "Moving aggregate report messages from" - logger.debug( - f"{log_message} {reports_folder} to {aggregate_reports_folder}" - ) - number_of_agg_report_msgs = len(aggregate_report_msg_uids) - for i in range(number_of_agg_report_msgs): - msg_uid = aggregate_report_msg_uids[i] + # Each report type is disposed of according to its own effective + # delete flag: deleted outright, or moved to its archive subfolder. + for msg_uids, delete_type, destination_folder, label in ( + ( + aggregate_report_msg_uids, + delete_aggregate, + aggregate_reports_folder, + "aggregate report", + ), + ( + failure_report_msg_uids, + delete_failure, + failure_reports_folder, + "failure report", + ), + ( + smtp_tls_msg_uids, + delete_smtp_tls, + smtp_tls_reports_folder, + "SMTP TLS report", + ), + ): + number_of_msgs = len(msg_uids) + if number_of_msgs == 0: + continue + if not delete_type: + message = f"Moving {label} messages from" + logger.debug(f"{message} {reports_folder} to {destination_folder}") + for i in range(number_of_msgs): + msg_uid = msg_uids[i] + if delete_type: logger.debug( - f"Moving message {i + 1} of {number_of_agg_report_msgs}: UID {msg_uid}" + f"Deleting message {i + 1} of {number_of_msgs}: UID {msg_uid}" ) try: - connection.move_message(msg_uid, aggregate_reports_folder) + connection.delete_message(msg_uid) except Exception as e: - message = "Error moving message UID" + message = "Error deleting message UID" e = f"{message} {msg_uid}: {e}" logger.error(f"Mailbox error: {e}") - if len(failure_report_msg_uids) > 0: - message = "Moving failure report messages from" - logger.debug(f"{message} {reports_folder} to {failure_reports_folder}") - number_of_failure_msgs = len(failure_report_msg_uids) - for i in range(number_of_failure_msgs): - msg_uid = failure_report_msg_uids[i] + else: message = "Moving message" logger.debug( - f"{message} {i + 1} of {number_of_failure_msgs}: UID {msg_uid}" + f"{message} {i + 1} of {number_of_msgs}: UID {msg_uid}" ) try: - connection.move_message(msg_uid, failure_reports_folder) - except Exception as e: - e = f"Error moving message UID {msg_uid}: {e}" - logger.error(f"Mailbox error: {e}") - if len(smtp_tls_msg_uids) > 0: - message = "Moving SMTP TLS report messages from" - logger.debug(f"{message} {reports_folder} to {smtp_tls_reports_folder}") - number_of_smtp_tls_uids = len(smtp_tls_msg_uids) - for i in range(number_of_smtp_tls_uids): - msg_uid = smtp_tls_msg_uids[i] - message = "Moving message" - logger.debug( - f"{message} {i + 1} of {number_of_smtp_tls_uids}: UID {msg_uid}" - ) - try: - connection.move_message(msg_uid, smtp_tls_reports_folder) + connection.move_message(msg_uid, destination_folder) except Exception as e: e = f"Error moving message UID {msg_uid}: {e}" logger.error(f"Mailbox error: {e}") @@ -2771,6 +2799,10 @@ def get_dmarc_reports_from_mailbox( reports_folder=reports_folder, archive_folder=archive_folder, delete=delete, + delete_aggregate=delete_aggregate, + delete_failure=delete_failure, + delete_smtp_tls=delete_smtp_tls, + delete_invalid=delete_invalid, test=test, results=results, since=current_time, @@ -2788,6 +2820,10 @@ def watch_inbox( reports_folder: str = "INBOX", archive_folder: str = "Archive", delete: bool = False, + delete_aggregate: bool | None = None, + delete_failure: bool | None = None, + delete_smtp_tls: bool | None = None, + delete_invalid: bool | None = None, test: bool = False, check_timeout: int = 30, ip_db_path: str | None = None, @@ -2815,7 +2851,23 @@ def watch_inbox( callback: The callback function to receive the parsing results reports_folder (str): The IMAP folder where reports can be found archive_folder (str): The folder to move processed mail to - delete (bool): Delete messages after processing them + delete (bool): Delete messages after processing them + delete_aggregate (bool | None): Delete aggregate report messages + after processing them, instead of moving them to the + ``Aggregate`` archive subfolder; ``None`` (the default) inherits + the value of ``delete`` + delete_failure (bool | None): Delete failure report messages after + processing them, instead of moving them to the ``Failure`` + archive subfolder; ``None`` (the default) inherits the value of + ``delete`` + delete_smtp_tls (bool | None): Delete SMTP TLS report messages after + processing them, instead of moving them to the ``SMTP-TLS`` + archive subfolder; ``None`` (the default) inherits the value of + ``delete`` + delete_invalid (bool | None): Delete unparseable messages, instead + of moving them to the ``Invalid`` archive subfolder where they + can be inspected for debugging; ``None`` (the default) inherits + the value of ``delete`` test (bool): Do not move or delete messages after processing them check_timeout (int): Number of seconds to wait for a IMAP IDLE response or the number of seconds until the next mail check @@ -2842,9 +2894,15 @@ def watch_inbox( parallel. Passed through to ``get_dmarc_reports_from_mailbox`` on each check. Not part of ``config``; always applies. config (ParserConfig): a single object carrying all parsing and - enrichment options plus the caches; when provided, the - individual option keyword arguments listed above are ignored in - favor of the config's values. + enrichment options plus the caches; when provided, it replaces + the individual parsing and enrichment option keyword arguments + listed above (DNS, GeoIP, offline mode, attachment payload + stripping, timespan normalization), whose values are then + ignored. The remaining keyword arguments control mailbox + handling and orchestration rather than parsing (the folder + names, the ``delete`` options, ``test``, ``since``, + ``batch_size``); they are not part of ``config`` and always + apply. """ cfg = _resolve_config( config, @@ -2866,6 +2924,10 @@ def watch_inbox( reports_folder=reports_folder, archive_folder=archive_folder, delete=delete, + delete_aggregate=delete_aggregate, + delete_failure=delete_failure, + delete_smtp_tls=delete_smtp_tls, + delete_invalid=delete_invalid, test=test, batch_size=batch_size, since=since, diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index e68dffaf..5970ce3c 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -783,6 +783,22 @@ def _parse_config(config: ConfigParser, opts): opts.mailbox_watch = bool(mailbox_config.getboolean("watch")) if "delete" in mailbox_config: opts.mailbox_delete = bool(mailbox_config.getboolean("delete")) + if "delete_aggregate" in mailbox_config: + opts.mailbox_delete_aggregate = bool( + mailbox_config.getboolean("delete_aggregate") + ) + if "delete_failure" in mailbox_config: + opts.mailbox_delete_failure = bool( + mailbox_config.getboolean("delete_failure") + ) + if "delete_smtp_tls" in mailbox_config: + opts.mailbox_delete_smtp_tls = bool( + mailbox_config.getboolean("delete_smtp_tls") + ) + if "delete_invalid" in mailbox_config: + opts.mailbox_delete_invalid = bool( + mailbox_config.getboolean("delete_invalid") + ) if "test" in mailbox_config: opts.mailbox_test = bool(mailbox_config.getboolean("test")) if "batch_size" in mailbox_config: @@ -2217,6 +2233,13 @@ def _main(): mailbox_archive_folder="Archive", mailbox_watch=False, mailbox_delete=False, + # None means "unset": each per-report-type flag inherits mailbox_delete + # in get_dmarc_reports_from_mailbox, so an explicit False (opting one + # type out of a global delete = true) stays distinct from being unset. + mailbox_delete_aggregate=None, + mailbox_delete_failure=None, + mailbox_delete_smtp_tls=None, + mailbox_delete_invalid=None, mailbox_test=False, mailbox_batch_size=10, mailbox_check_timeout=30, @@ -2686,7 +2709,21 @@ def _main(): exit(1) if opts.gmail_api_credentials_file: - if opts.mailbox_delete: + # Any effective delete flag needs the deletion scope: the per-report-type + # flags inherit mailbox_delete when unset (None), so this reduces to + # mailbox_delete alone when none of them is set. The scope is a + # mailbox-wide capability grant rather than a per-type one, so when it + # is missing every flag is turned off explicitly. + per_type_delete_opts = ( + "mailbox_delete_aggregate", + "mailbox_delete_failure", + "mailbox_delete_smtp_tls", + "mailbox_delete_invalid", + ) + if any( + opts.mailbox_delete if getattr(opts, name) is None else getattr(opts, name) + for name in per_type_delete_opts + ): if "https://mail.google.com/" not in opts.gmail_api_scopes: logger.error( "Message deletion requires scope" @@ -2695,6 +2732,8 @@ def _main(): "to acquire proper access." ) opts.mailbox_delete = False + for name in per_type_delete_opts: + setattr(opts, name, False) try: mailbox_connection = GmailConnection( @@ -2737,6 +2776,10 @@ def _main(): reports = get_dmarc_reports_from_mailbox( connection=mailbox_connection, delete=opts.mailbox_delete, + delete_aggregate=opts.mailbox_delete_aggregate, + delete_failure=opts.mailbox_delete_failure, + delete_smtp_tls=opts.mailbox_delete_smtp_tls, + delete_invalid=opts.mailbox_delete_invalid, batch_size=mailbox_batch_size_value, reports_folder=opts.mailbox_reports_folder, archive_folder=opts.mailbox_archive_folder, @@ -2859,6 +2902,10 @@ def _main(): reports_folder=opts.mailbox_reports_folder, archive_folder=opts.mailbox_archive_folder, delete=opts.mailbox_delete, + delete_aggregate=opts.mailbox_delete_aggregate, + delete_failure=opts.mailbox_delete_failure, + delete_smtp_tls=opts.mailbox_delete_smtp_tls, + delete_invalid=opts.mailbox_delete_invalid, test=opts.mailbox_test, check_timeout=mailbox_check_timeout_value, batch_size=mailbox_batch_size_value, diff --git a/tests/test_cli.py b/tests/test_cli.py index 6a126a5f..56463c0f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1552,6 +1552,146 @@ since = 2d self.assertEqual(mock_watch_inbox.call_args.kwargs.get("since"), "2d") +class TestMailboxPerTypeDeleteOptions(unittest.TestCase): + """The [mailbox] per-report-type delete options (issue #256) must reach + the library unchanged, including the difference between an explicit + ``false`` and an unset option (``None``, meaning "inherit ``delete``").""" + + def setUp(self): + from parsedmarc.log import logger as _logger + + _logger.disabled = True + self._stdout_patch = patch("sys.stdout", new_callable=io.StringIO) + self._stderr_patch = patch("sys.stderr", new_callable=io.StringIO) + self._stdout_patch.start() + self._stderr_patch.start() + + def tearDown(self): + from parsedmarc.log import logger as _logger + + _logger.disabled = False + self._stderr_patch.stop() + self._stdout_patch.stop() + + def _write_config(self, config_text): + with tempfile.NamedTemporaryFile("w", suffix=".ini", delete=False) as cfg: + cfg.write(config_text) + cfg_path = cfg.name + self.addCleanup(lambda: os.path.exists(cfg_path) and os.remove(cfg_path)) + return cfg_path + + IMAP_CONFIG = """[general] +silent = true + +[imap] +host = imap.example.com +user = user +password = pass + +[mailbox] +delete = true +delete_failure = false +""" + + def _assert_per_type_delete_kwargs(self, kwargs): + self.assertIs(kwargs.get("delete"), True) + self.assertIs(kwargs.get("delete_failure"), False) + # Unset options stay None so the library applies the inheritance. + self.assertIsNone(kwargs.get("delete_aggregate")) + self.assertIsNone(kwargs.get("delete_smtp_tls")) + self.assertIsNone(kwargs.get("delete_invalid")) + + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.watch_inbox") + @patch("parsedmarc.cli.IMAPConnection") + def testCliPassesPerTypeDeleteToWatchInbox( + self, mock_imap_connection, mock_watch_inbox, mock_get_mailbox_reports + ): + mock_imap_connection.return_value = object() + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + mock_watch_inbox.side_effect = FileExistsError("stop-watch-loop") + cfg_path = self._write_config(self.IMAP_CONFIG + "watch = true\n") + + with patch.object(sys, "argv", ["parsedmarc", "-c", cfg_path]): + with self.assertRaises(SystemExit) as system_exit: + parsedmarc.cli._main() + + self.assertEqual(system_exit.exception.code, 1) + self._assert_per_type_delete_kwargs(mock_watch_inbox.call_args.kwargs) + + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testCliPassesPerTypeDeleteToOneShotMailboxRun( + self, mock_imap_connection, mock_get_mailbox_reports + ): + mock_imap_connection.return_value = object() + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + cfg_path = self._write_config(self.IMAP_CONFIG) + + with patch.object(sys, "argv", ["parsedmarc", "-c", cfg_path]): + parsedmarc.cli._main() + + self._assert_per_type_delete_kwargs(mock_get_mailbox_reports.call_args.kwargs) + + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.GmailConnection") + def testGmailScopeGuardForcesAllDeleteFlagsOff( + self, mock_gmail_connection, mock_get_mailbox_reports + ): + """The Gmail deletion scope is a mailbox-wide capability grant, so a + per-report-type delete option alone (with ``delete`` unset) is enough + to trip the guard, and a missing scope turns every delete flag off.""" + mock_gmail_connection.return_value = MagicMock() + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + cfg_path = self._write_config("""[general] +silent = true + +[gmail_api] +credentials_file = /tmp/gmail-credentials.json +scopes = https://www.googleapis.com/auth/gmail.modify + +[mailbox] +delete_aggregate = true +""") + + from parsedmarc.log import logger as _logger + + _logger.disabled = False + try: + with patch.object(sys, "argv", ["parsedmarc", "-c", cfg_path]): + with self.assertLogs("parsedmarc.log", level="ERROR") as logs: + parsedmarc.cli._main() + finally: + _logger.disabled = True + + self.assertTrue( + any("Message deletion requires scope" in line for line in logs.output), + logs.output, + ) + kwargs = mock_get_mailbox_reports.call_args.kwargs + for option in ( + "delete", + "delete_aggregate", + "delete_failure", + "delete_smtp_tls", + "delete_invalid", + ): + with self.subTest(option=option): + self.assertIs(kwargs.get(option), False) + + class TestCliParserConfigWiring(unittest.TestCase): """Tests that _main() builds a single ParserConfig (via _build_parser_config) from parsed opts and passes it as ``config=`` to @@ -4484,6 +4624,68 @@ def _config_with(section: str, settings: dict) -> "ConfigParser": return cp +class TestParseConfigMailbox(unittest.TestCase): + """The [mailbox] section, including the per-report-type delete options + (issue #256). An option that is absent from the INI must be left alone + entirely: _main defaults the four per-type options to None so the library + inherits the overall ``delete`` value, and writing False for an absent key + would silently disable that inheritance.""" + + PER_TYPE_DELETE_OPTS = ( + "mailbox_delete_aggregate", + "mailbox_delete_failure", + "mailbox_delete_smtp_tls", + "mailbox_delete_invalid", + ) + + def test_mailbox_full_section(self): + from parsedmarc.cli import _parse_config + + cp = _config_with( + "mailbox", + { + "reports_folder": "Reports", + "archive_folder": "Processed", + "watch": "true", + "delete": "true", + "delete_aggregate": "true", + "delete_failure": "false", + "delete_smtp_tls": "true", + "delete_invalid": "false", + "test": "false", + "batch_size": "25", + "check_timeout": "60", + "since": "3d", + }, + ) + opts = _opts() + _parse_config(cp, opts) + self.assertEqual(opts.mailbox_reports_folder, "Reports") + self.assertEqual(opts.mailbox_archive_folder, "Processed") + self.assertIs(opts.mailbox_watch, True) + self.assertIs(opts.mailbox_delete, True) + self.assertIs(opts.mailbox_delete_aggregate, True) + # Explicitly false, not None: this type opts out of delete = true. + self.assertIs(opts.mailbox_delete_failure, False) + self.assertIs(opts.mailbox_delete_smtp_tls, True) + self.assertIs(opts.mailbox_delete_invalid, False) + self.assertIs(opts.mailbox_test, False) + self.assertEqual(opts.mailbox_batch_size, 25) + self.assertEqual(opts.mailbox_check_timeout, 60) + self.assertEqual(opts.mailbox_since, "3d") + + def test_mailbox_absent_per_type_delete_keys_are_not_set(self): + from parsedmarc.cli import _parse_config + + cp = _config_with("mailbox", {"delete": "true"}) + opts = _opts() + _parse_config(cp, opts) + self.assertIs(opts.mailbox_delete, True) + for option in self.PER_TYPE_DELETE_OPTS: + with self.subTest(option=option): + self.assertFalse(hasattr(opts, option)) + + class TestParseConfigGeneral(unittest.TestCase): """The [general] section sets dozens of flags. Hit a representative subset: filenames, save-toggles, DNS settings, output dir.""" diff --git a/tests/test_init.py b/tests/test_init.py index 9a79e51f..b571b08a 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -13,6 +13,7 @@ import logging import mailbox import os import unittest +from collections.abc import Callable from datetime import datetime, timedelta, timezone from glob import glob from io import BytesIO @@ -2963,6 +2964,38 @@ class TestGetDmarcReportsFromMailboxValidation(unittest.TestCase): ) self.assertIn("mutually exclusive", str(ctx.exception)) + def test_inherited_delete_with_test_raises(self): + """The guard checks the effective per-type flags, so delete=True still + raises alongside test=True when only *one* per-type flag opts out -- + the remaining three inherit the deletion.""" + with self.assertRaises(ValueError) as ctx: + parsedmarc.get_dmarc_reports_from_mailbox( + connection=MagicMock(), delete=True, delete_aggregate=False, test=True + ) + self.assertIn("mutually exclusive", str(ctx.exception)) + + def test_explicit_per_type_delete_with_test_raises(self): + """Each per-type delete flag on its own is enough to conflict with + test=True, with the overall delete option left False and the other + three flags explicitly False.""" + for flag in ( + "delete_aggregate", + "delete_failure", + "delete_smtp_tls", + "delete_invalid", + ): + with self.subTest(flag=flag): + with self.assertRaises(ValueError) as ctx: + parsedmarc.get_dmarc_reports_from_mailbox( + connection=MagicMock(), + test=True, + delete_aggregate=flag == "delete_aggregate", + delete_failure=flag == "delete_failure", + delete_smtp_tls=flag == "delete_smtp_tls", + delete_invalid=flag == "delete_invalid", + ) + self.assertIn("mutually exclusive", str(ctx.exception)) + def test_none_connection_raises(self): with self.assertRaises(ValueError) as ctx: parsedmarc.get_dmarc_reports_from_mailbox( @@ -3071,6 +3104,34 @@ class TestMigrateForensicArchiveFolderMaildir(unittest.TestCase): self.assertEqual(result["failure_reports"], []) +class _FailingDisposalMaildirConnection(MaildirConnection): + """A MaildirConnection whose first disposal call fails. + + Real mailbox backends can reject an individual delete or move (permission + denied, a UID that vanished, a dropped connection) while the rest of the + batch is still fine. Failing only the *first* call, then behaving + normally, is what makes that observable: the message the failure hit + stays put, and every message after it must still be disposed of. + """ + + def __init__(self, *args, fail_delete=False, fail_move=False, **kwargs): + super().__init__(*args, **kwargs) + self._fail_delete = fail_delete + self._fail_move = fail_move + + def delete_message(self, message_id): + if self._fail_delete: + self._fail_delete = False + raise RuntimeError("server said no") + super().delete_message(message_id) + + def move_message(self, message_id, folder_name): + if self._fail_move: + self._fail_move = False + raise RuntimeError("server said no") + super().move_message(message_id, folder_name) + + class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): """parsedmarc's real mailbox processing loop, end to end on an on-disk Maildir (mailsuite MaildirConnection, no mocks, offline parsing): fetch @@ -3184,6 +3245,113 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): self.assertEqual(conn.fetch_messages("Archive/Failure"), []) self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + def test_delete_aggregate_overrides_delete_false(self): + """delete_aggregate=True with delete left at its False default: only + the aggregate report message is deleted; the other two report types + (and the unparseable message) are still archived.""" + self._deliver(self.AGGREGATE) + self._deliver(self.FAILURE) + self._deliver(self.SMTP_TLS) + self._deliver(self.JUNK) + + conn, result = self._run(delete_aggregate=True) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + self.assertEqual(len(conn.fetch_messages("Archive/SMTP-TLS")), 1) + self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) + + def test_delete_failure_false_overrides_delete_true(self): + """delete=True with delete_failure=False: failure report messages are + kept (archived) while every other message -- aggregate, SMTP TLS, and + the unparseable one, all inheriting delete=True -- is deleted. This is + the motivating case from issue #256.""" + self._deliver(self.AGGREGATE) + self._deliver(self.FAILURE) + self._deliver(self.SMTP_TLS) + self._deliver(self.JUNK) + + conn, result = self._run(delete=True, delete_failure=False) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(len(result["failure_reports"]), 1) + self.assertEqual(len(result["smtp_tls_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + self.assertEqual(conn.fetch_messages("Archive/SMTP-TLS"), []) + self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) + + def test_delete_invalid_true_deletes_unparseable_messages(self): + """delete_invalid=True on its own: the unparseable message is deleted + while the parsed failure report is still archived.""" + self._deliver(self.FAILURE) + self._deliver(self.JUNK) + + conn, result = self._run(delete_invalid=True) + + self.assertEqual(len(result["failure_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(conn.fetch_messages("Archive/Invalid"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + + def test_delete_invalid_false_keeps_unparseable_when_delete_true(self): + """delete=True with delete_invalid=False: the unparseable message is + kept in Archive/Invalid for debugging while the parsed failure report + message is deleted.""" + self._deliver(self.FAILURE) + self._deliver(self.JUNK) + + conn, result = self._run(delete=True, delete_invalid=False) + + self.assertEqual(len(result["failure_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) + self.assertEqual(conn.fetch_messages("Archive/Failure"), []) + + def test_per_type_delete_flags_parallel(self): + """Same per-type semantics on the n_procs=2 branch, where + invalid-message disposition happens after the parse phase: the + failure report and the unparseable message are archived (both + explicitly False) while the aggregate report inherits delete=True and + is deleted.""" + self._deliver(self.AGGREGATE) + self._deliver(self.FAILURE) + self._deliver(self.JUNK) + + conn, result = self._run( + n_procs=2, delete=True, delete_failure=False, delete_invalid=False + ) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(len(result["failure_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + self.assertEqual(len(conn.fetch_messages("Archive/Invalid")), 1) + self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + + def test_test_mode_allowed_when_all_delete_flags_explicitly_false(self): + """The test/delete guard fires on the *effective* per-type flags, so + delete=True alongside all four per-type flags explicitly False is + valid with test=True: nothing would be deleted. The message is parsed + and left in the INBOX.""" + self._deliver(self.FAILURE) + + conn, result = self._run( + delete=True, + delete_aggregate=False, + delete_failure=False, + delete_smtp_tls=False, + delete_invalid=False, + test=True, + ) + + self.assertEqual(len(result["failure_reports"]), 1) + self.assertEqual(len(conn.fetch_messages("INBOX")), 1) + self.assertFalse(conn.folder_exists("Archive/Failure")) + def test_test_mode_parses_without_moving_or_creating_folders(self): """test=True: the report is parsed and returned, but the message stays in the INBOX and no archive folders are created/touched.""" @@ -3228,6 +3396,64 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): self.assertEqual(len(conn.fetch_messages("Archive/Aggregate")), 2) self.assertEqual(conn.fetch_messages("INBOX"), []) + def test_delete_error_is_logged_and_disposal_continues(self): + """A backend that rejects one delete must not abort the disposal of + the remaining messages: the error is logged and the loop moves on to + the next report type. The aggregate message's delete fails, so it + stays in the INBOX, while the SMTP TLS message is still deleted.""" + self._deliver(self.AGGREGATE) + self._deliver(self.SMTP_TLS) + conn = _FailingDisposalMaildirConnection( + self._maildir, maildir_create=True, fail_delete=True + ) + + with self.assertLogs("parsedmarc.log", level="ERROR") as cm: + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, offline=True, delete=True + ) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(len(result["smtp_tls_reports"]), 1) + self.assertTrue( + any( + "Mailbox error: Error deleting message UID" in line + for line in cm.output + ), + cm.output, + ) + # Only the message whose delete raised is left behind. + self.assertEqual(len(conn.fetch_messages("INBOX")), 1) + self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + self.assertEqual(conn.fetch_messages("Archive/SMTP-TLS"), []) + + def test_move_error_is_logged_and_disposal_continues(self): + """The same for the archiving half of the disposal loop: a rejected + move is logged and the loop continues, so the aggregate message stays + in the INBOX while the SMTP TLS message still reaches its archive + subfolder.""" + self._deliver(self.AGGREGATE) + self._deliver(self.SMTP_TLS) + conn = _FailingDisposalMaildirConnection( + self._maildir, maildir_create=True, fail_move=True + ) + + with self.assertLogs("parsedmarc.log", level="ERROR") as cm: + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, offline=True + ) + + self.assertEqual(len(result["aggregate_reports"]), 1) + self.assertEqual(len(result["smtp_tls_reports"]), 1) + self.assertTrue( + any( + "Mailbox error: Error moving message UID" in line for line in cm.output + ), + cm.output, + ) + self.assertEqual(len(conn.fetch_messages("INBOX")), 1) + self.assertEqual(conn.fetch_messages("Archive/Aggregate"), []) + self.assertEqual(len(conn.fetch_messages("Archive/SMTP-TLS")), 1) + class _MidRunArrivalMaildirConnection(MaildirConnection): """A MaildirConnection that delivers one extra message into the INBOX @@ -3301,6 +3527,104 @@ class TestGetDmarcReportsFromMailboxMaildirBatchSizeZeroRecursion(unittest.TestC self.assertEqual(len(result["smtp_tls_reports"]), 1) self.assertEqual(conn.fetch_messages("INBOX"), []) + def test_batch_size_zero_recursion_threads_per_type_delete(self): + """The per-report-type delete flags must be threaded through the + recursive re-check call too. The SMTP TLS report only "arrives" after + the main pass's fetch, so it is disposed of by the recursive call: with + delete_smtp_tls=True it must be deleted, not archived. Dropping the + kwarg from the recursive call archives it instead and fails here.""" + conn = _MidRunArrivalMaildirConnection( + self._maildir, maildir_create=True, extra_source=self.SMTP_TLS + ) + + result = parsedmarc.get_dmarc_reports_from_mailbox( + connection=conn, offline=True, batch_size=0, delete_smtp_tls=True + ) + + self.assertEqual(len(result["smtp_tls_reports"]), 1) + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(conn.fetch_messages("Archive/SMTP-TLS"), []) + # The types that inherit the (False) delete default are unaffected. + self.assertEqual(len(conn.fetch_messages("Archive/Aggregate")), 1) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + + +class _SingleCheckMaildirConnection(MaildirConnection): + """A MaildirConnection whose watch() performs exactly one check. + + mailsuite's own MaildirConnection.watch() loops forever (sleeping + check_timeout seconds between checks) and swallows every exception the + callback raises, so it cannot be driven from a test as-is. This override + keeps the same signature, invokes watch_inbox's check_callback once, lets + exceptions propagate, and returns -- so the real closure inside + watch_inbox runs against a real on-disk Maildir, with no parsedmarc + internals mocked. + """ + + def watch( + self, + check_callback: Callable[[parsedmarc.MailboxConnection], None], + check_timeout: int, + config_reloading: Callable[[], bool] | None = None, + ) -> None: + del check_timeout, config_reloading + check_callback(self) + + +class TestWatchInboxMaildir(unittest.TestCase): + """watch_inbox builds a check_callback closure that calls + get_dmarc_reports_from_mailbox on every mailbox check. Everything watch + mode does to messages therefore flows through that closure, which is + reached only via the backend's watch() -- so it needs a connection whose + watch() actually runs one check (see _SingleCheckMaildirConnection).""" + + FAILURE = "samples/failure/dmarc_ruf_report_linkedin.eml" + JUNK = b"From: noise@example.com\nSubject: not a report\n\nplain text\n" + + def setUp(self): + self._tmp = mkdtemp() + self.addCleanup(rmtree, self._tmp, ignore_errors=True) + parsedmarc.SEEN_AGGREGATE_REPORT_IDS.clear() + self._maildir = os.path.join(self._tmp, "Maildir") + inbox = mailbox.Maildir(self._maildir, create=True) + with open(self.FAILURE, "rb") as failure_file: + inbox.add(mailbox.MaildirMessage(failure_file.read())) + inbox.add(mailbox.MaildirMessage(self.JUNK)) + inbox.flush() + + def test_watch_inbox_forwards_per_type_delete_flags(self): + """Trip-wire for the per-report-type delete flags on watch_inbox's + inner call: delete=True with delete_failure=False must archive the + failure report message while the unparseable message, inheriting + delete=True, is deleted. Dropping delete_failure from the closure + would silently delete the failure report message instead -- watch + mode ignoring what the caller asked for -- and fail here.""" + conn = _SingleCheckMaildirConnection(self._maildir, maildir_create=True) + # watch_inbox passes create_folders=False (in real watch mode the + # archive folders already exist, created by the run's first pass), so + # the destination folder and its parent are seeded here. + conn.create_folder("Archive") + conn.create_folder("Archive/Failure") + received = [] + + parsedmarc.watch_inbox( + mailbox_connection=conn, + callback=received.append, + offline=True, + delete=True, + delete_failure=False, + ) + + # The results reach the callback through the same closure. + self.assertEqual(len(received), 1) + self.assertEqual(len(received[0]["failure_reports"]), 1) + + self.assertEqual(conn.fetch_messages("INBOX"), []) + self.assertEqual(len(conn.fetch_messages("Archive/Failure")), 1) + # delete_invalid inherited delete=True, so the unparseable message was + # deleted rather than filed in the Invalid subfolder. + self.assertFalse(conn.folder_exists("Archive/Invalid")) + class TestEmailResultsErrorBranches(unittest.TestCase): """email_results requires mail_to to be a list — this is enforced