From df9bf82e04fe949ac06480d61ef5ea8a8d1041b7 Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:11:47 -0400 Subject: [PATCH] Post-review follow-ups for Graph send (#825/#826) and requests-to-httpx migration (#827) Follow-ups from the review of PR #825 (whose implementation had already landed on master via #826's stacked merge): - Honor the documented [smtp] attachment and [smtp] message options. Both were parsed into opts but never passed to either summary-email transport (also broken in released 10.2.2), so a configured custom attachment filename or message body was silently ignored. Both the SMTP and Microsoft Graph transports now receive them, and the missing smtp_attachment Namespace default is added (also covers SIGHUP reload, which rebuilds opts from the CLI Namespace). - Don't mislabel non-Graph mailbox errors as Microsoft Graph failures: the shared mailbox-fetch and watch handlers now log a generic "Mailbox Error" with traceback when the connection isn't Graph. - Declare microsoft-kiota-abstractions as a direct dependency (imported directly in cli.py for Graph error handling; previously transitive). Migrate all runtime HTTP from requests to httpx (webhook client, Splunk HEC client, and the PSL-overrides / IP-database / reverse-DNS-map / IPinfo-API fetches in utils.py): - follow_redirects=True everywhere to preserve requests' default redirect-following; httpx does not follow redirects by default. - The PSL-overrides and reverse-DNS-map fetches gain a 60s timeout (previously none), matching the IP-database fetch. - response.ok -> response.is_success; requests.RequestException -> httpx.HTTPError; raw string bodies use content= (httpx's data= is form-encoding only); Splunk HEC verification moves to client construction (httpx has no per-request verify). - requests drops out of [project] dependencies and moves to the [build] extra for the out-of-wheel maintainer script collect_domain_info.py, which deliberately stays on requests/urllib3 for its permissive-TLS adapter. - Remove the requests-era module-level urllib3.disable_warnings(InsecureRequestWarning) in splunk.py; httpx doesn't route through urllib3, so its only remaining effect was globally silencing insecure-TLS warnings from other urllib3-based components as an import side effect. Nothing imports urllib3 directly anymore, so it also leaves [project] dependencies. Tests: config-to-transport wiring for attachment/message on both transports (including defaults), non-Graph errors keep the generic log line, webhook/Splunk payload assertions moved to content=, and Splunk verify asserted at httpx.Client construction. 736 passed; ruff and pyright clean. Co-authored-by: Claude Fable 5 --- CHANGELOG.md | 2 + docs/source/usage.md | 3 +- parsedmarc/cli.py | 39 ++++++---- parsedmarc/splunk.py | 20 +++-- parsedmarc/utils.py | 32 +++++--- parsedmarc/webhook.py | 17 +++-- pyproject.toml | 14 +++- tests/test_cli.py | 170 ++++++++++++++++++++++++++++++++++++++++++ tests/test_splunk.py | 47 +++++++++--- tests/test_utils.py | 64 ++++++++-------- tests/test_webhook.py | 20 ++++- 11 files changed, 335 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5606ad0..fe48a70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,12 @@ - **The periodic summary email can now be sent via Microsoft Graph** (tracking [#472](https://github.com/domainaware/parsedmarc/issues/472)). Previously the summary email required `[smtp] host`, forcing M365 tenants that block legacy SMTP AUTH to stand up a separate SMTP relay just to send from the same mailbox they already read reports from. When `[smtp] host` is omitted and `[msgraph]` is configured, the summary is now sent through the same already-authenticated Graph mailbox connection (`/users/{mailbox}/sendMail`), saved to Sent Items. SMTP is preferred whenever `[smtp] host` is set, with no fallback to Graph on SMTP failure. `[smtp] host`/`user`/`password`/`from` are now conditionally required — only when `host` is present; `to` keeps its existing requirement either way. A new public `email_results_via_msgraph()` function shares its subject/message/zip-building logic with the existing `email_results()` via an extracted `_build_report_email_content()` helper, so both transports stay in lockstep. Note `[smtp] from` has no effect on the Graph path — the message's `From` is always the `[msgraph]` mailbox — and sending requires the Graph `Mail.Send` permission (`Mail.Send.Shared` for a shared mailbox under delegated auth); see the "Sending the summary email via Microsoft Graph" docs section for the full permission matrix. - **Microsoft Graph connection/fetch/send failures now log a single clear ERROR line** instead of a bare `logger.exception()` that hid the actual Azure/Graph error. The line identifies the mailbox, tenant ID, and auth method, and includes the Graph `request-id`/`client-request-id` when available (from the OData inner error or, failing that, the raw response headers) — details that matter when contacting Microsoft support. The full traceback is still preserved under `--debug`. - **Refreshed the `[msgraph]` documentation**: national/sovereign-cloud `graph_url` values with an explicit warning that the Entra ID auth endpoint isn't independently configurable, a minimal example config for every auth method, a reading-permission matrix alongside the existing sending one, an accurate note on the `parsedmarc`-named token cache (no migration needed — it's a deliberate backward-compatibility choice from the 9.11.0 `mailsuite` extraction, not something users have to act on), and a troubleshooting table distinguishing still-live failure modes (admin consent, uninitialized-mailbox folder resolution) from historical ones already fixed at this project's dependency floor (`Event loop is closed`, invalid ISO timestamps). +- **All runtime HTTP calls now use `httpx` instead of `requests`**, and the dependency set changed accordingly: `requests` is no longer a runtime dependency (it moved to the `[build]` extra, where it's still used by the out-of-wheel maintainer script `parsedmarc/resources/maps/collect_domain_info.py`), and `httpx` and `microsoft-kiota-abstractions` are now declared dependencies (`httpx` for all runtime HTTP calls, `microsoft-kiota-abstractions` for the Microsoft Graph error handling above). The migration covers the webhook output client, the Splunk HEC client, and the PSL-overrides / IP-database / reverse-DNS-map / IPinfo-API fetches in `utils.py`. Redirect-following is preserved everywhere (`requests` follows redirects by default; `httpx` requires `follow_redirects=True`, which is now passed explicitly). The PSL-overrides and reverse-DNS-map fetches, which previously had no timeout, now time out after 60 seconds, matching the existing IP-database fetch. Also removed the module-level `urllib3.disable_warnings(InsecureRequestWarning)` call in the Splunk HEC output: `httpx` doesn't route through `urllib3`, so it no longer affected the HEC client, and its only remaining effect was globally silencing insecure-TLS warnings from every other `urllib3`-based component (Elasticsearch, OpenSearch, boto3) as an import side effect. With that gone, nothing imports `urllib3` directly anymore, so it's also no longer a declared dependency (it remains transitively installed). ### Bug fixes - **`--watch` no longer crashes with a raw uncaught traceback on a Microsoft Graph error.** The continuous-mode loop previously caught only `FileExistsError`/`ParserError`; a Graph auth, API, or transport error during a long-running watch — arguably the most likely real-world failure point, since that's where token/certificate expiry actually surfaces — crashed uncaught. It now gets the same single formatted ERROR line as the other Graph call sites and exits cleanly. +- **The documented `[smtp] attachment` and `[smtp] message` options are now honored.** Both were parsed into `opts.smtp_attachment`/`opts.smtp_message` but never passed to either summary-email transport, so a configured custom attachment filename or message body was silently ignored. Both the SMTP (`email_results()`) and Microsoft Graph (`email_results_via_msgraph()`) transports now receive them. Visible side effect: the default email body for SMTP summaries is now the long-documented default `Please see the attached DMARC results.` instead of the previously hardcoded `DMARC results for `. ## 10.2.2 diff --git a/docs/source/usage.md b/docs/source/usage.md index 71e96f0..2d97dd2 100644 --- a/docs/source/usage.md +++ b/docs/source/usage.md @@ -546,8 +546,9 @@ The full set of configuration options are: - `subject` - str: The Subject header to use in the email (Default: `parsedmarc report`) - `attachment` - str: The ZIP attachment filenames + (Default: `DMARC-.zip`) - `message` - str: The email message - (Default: `Please see the attached parsedmarc report.`) + (Default: `Please see the attached DMARC results.`) :::{note} `%` characters must be escaped with another `%` character, diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index 14ccd73..406ff4a 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -2119,6 +2119,7 @@ def _main(): smtp_from=None, smtp_to=[], smtp_subject="parsedmarc report", + smtp_attachment=None, smtp_message="Please see the attached DMARC results.", s3_bucket=None, s3_path=None, @@ -2648,13 +2649,16 @@ def _main(): smtp_tls_reports += reports["smtp_tls_reports"] except (ClientAuthenticationError, APIError, httpx.HTTPError) as error: - _log_msgraph_failure( - error, - stage="mailbox fetch", - mailbox=opts.graph_mailbox or opts.graph_user, - tenant_id=opts.graph_tenant_id, - auth_method=opts.graph_auth_method, - ) + if msgraph_connection is None: + logger.exception("Mailbox Error") + else: + _log_msgraph_failure( + error, + stage="mailbox fetch", + mailbox=opts.graph_mailbox or opts.graph_user, + tenant_id=opts.graph_tenant_id, + auth_method=opts.graph_auth_method, + ) exit(1) except Exception: logger.exception("Mailbox Error") @@ -2694,6 +2698,8 @@ def _main(): password=opts.smtp_password, subject=opts.smtp_subject, require_encryption=opts.smtp_ssl, + attachment_filename=opts.smtp_attachment, + message=opts.smtp_message, ) except Exception: logger.exception("Failed to email results") @@ -2705,6 +2711,8 @@ def _main(): msgraph_connection, smtp_to_value, subject=opts.smtp_subject, + attachment_filename=opts.smtp_attachment, + message=opts.smtp_message, ) except (ClientAuthenticationError, APIError, httpx.HTTPError) as error: _log_msgraph_failure( @@ -2763,13 +2771,16 @@ def _main(): logger.error(error.__str__()) exit(1) except (ClientAuthenticationError, APIError, httpx.HTTPError) as error: - _log_msgraph_failure( - error, - stage="mailbox watch", - mailbox=opts.graph_mailbox or opts.graph_user, - tenant_id=opts.graph_tenant_id, - auth_method=opts.graph_auth_method, - ) + if msgraph_connection is None: + logger.exception("Mailbox Error") + else: + _log_msgraph_failure( + error, + stage="mailbox watch", + mailbox=opts.graph_mailbox or opts.graph_user, + tenant_id=opts.graph_tenant_id, + auth_method=opts.graph_auth_method, + ) exit(1) # Prioritize shutdown over reload if both flags are set (e.g. diff --git a/parsedmarc/splunk.py b/parsedmarc/splunk.py index 7898c84..ccb85cd 100644 --- a/parsedmarc/splunk.py +++ b/parsedmarc/splunk.py @@ -7,15 +7,12 @@ import socket from typing import Any from urllib.parse import urlparse -import requests -import urllib3 +import httpx from parsedmarc.constants import USER_AGENT from parsedmarc.log import logger from parsedmarc.utils import human_timestamp_to_unix_timestamp -urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) - class SplunkError(RuntimeError): """Raised when a Splunk API error occurs""" @@ -56,18 +53,19 @@ class HECClient(object): self.index = index self.host = socket.getfqdn() self.source = source - self.session = requests.Session() self.timeout = timeout self.verify = verify self._common_data: dict[str, str | int | float | dict] = dict( host=self.host, source=self.source, index=self.index ) - self.session.headers.update( - { + self.session = httpx.Client( + headers={ "User-Agent": USER_AGENT, "Authorization": "Splunk {0}".format(self.access_token), - } + }, + verify=self.verify, + follow_redirects=True, ) def save_aggregate_reports_to_splunk( @@ -135,7 +133,7 @@ class HECClient(object): logger.debug("Skipping certificate verification for Splunk HEC") try: response = self.session.post( - self.url, data=json_str, verify=self.verify, timeout=self.timeout + self.url, content=json_str, timeout=self.timeout ) response = response.json() except Exception as e: @@ -178,7 +176,7 @@ class HECClient(object): logger.debug("Skipping certificate verification for Splunk HEC") try: response = self.session.post( - self.url, data=json_str, verify=self.verify, timeout=self.timeout + self.url, content=json_str, timeout=self.timeout ) response = response.json() except Exception as e: @@ -217,7 +215,7 @@ class HECClient(object): logger.debug("Skipping certificate verification for Splunk HEC") try: response = self.session.post( - self.url, data=json_str, verify=self.verify, timeout=self.timeout + self.url, content=json_str, timeout=self.timeout ) response = response.json() except Exception as e: diff --git a/parsedmarc/utils.py b/parsedmarc/utils.py index 20481d2..ac36e0f 100644 --- a/parsedmarc/utils.py +++ b/parsedmarc/utils.py @@ -28,9 +28,9 @@ from importlib.resources import files import dns.exception import dns.resolver import dns.reversename +import httpx import maxminddb import publicsuffixlist -import requests from dateutil.parser import parse as parse_date import parsedmarc.resources.ipinfo @@ -103,10 +103,12 @@ def load_psl_overrides( try: logger.debug(f"Trying to fetch PSL overrides from {url}...") headers = {"User-Agent": USER_AGENT} - response = requests.get(url, headers=headers) + response = httpx.get( + url, headers=headers, timeout=60, follow_redirects=True + ) response.raise_for_status() _load_text(response.text) - except requests.exceptions.RequestException as e: + except httpx.HTTPError as e: logger.warning(f"Failed to fetch PSL overrides: {e}") if len(psl_overrides) == 0: @@ -447,7 +449,9 @@ def load_ip_db( try: logger.debug(f"Trying to fetch IP database from {url}...") headers = {"User-Agent": USER_AGENT} - response = requests.get(url, headers=headers, timeout=60) + response = httpx.get( + url, headers=headers, timeout=60, follow_redirects=True + ) response.raise_for_status() os.makedirs(cache_dir, exist_ok=True) tmp_path = cached_path + ".tmp" @@ -457,7 +461,7 @@ def load_ip_db( _IP_DB_PATH = cached_path logger.info("IP database updated successfully") return - except requests.exceptions.RequestException as e: + except httpx.HTTPError as e: logger.warning(f"Failed to fetch IP database: {e}") except Exception as e: logger.warning(f"Failed to save IP database: {e}") @@ -546,10 +550,14 @@ def _ipinfo_api_lookup(ip_address: str) -> _IPDatabaseRecord | None: params = {"token": _IPINFO_API_TOKEN} headers = {"User-Agent": USER_AGENT, "Accept": "application/json"} try: - response = requests.get( - url, headers=headers, params=params, timeout=_IPINFO_API_TIMEOUT + response = httpx.get( + url, + headers=headers, + params=params, + timeout=_IPINFO_API_TIMEOUT, + follow_redirects=True, ) - except requests.exceptions.RequestException as e: + except httpx.HTTPError as e: logger.debug(f"IPinfo API request for {ip_address} failed: {e}") return None @@ -557,7 +565,7 @@ def _ipinfo_api_lookup(ip_address: str) -> _IPDatabaseRecord | None: raise InvalidIPinfoAPIKey( f"IPinfo API rejected the configured token (HTTP {response.status_code})" ) - if not response.ok: + if not response.is_success: logger.debug( f"IPinfo API returned HTTP {response.status_code} for {ip_address}" ) @@ -800,12 +808,14 @@ def load_reverse_dns_map( try: logger.debug(f"Trying to fetch reverse DNS map from {url}...") headers = {"User-Agent": USER_AGENT} - response = requests.get(url, headers=headers) + response = httpx.get( + url, headers=headers, timeout=60, follow_redirects=True + ) response.raise_for_status() csv_file.write(response.text) csv_file.seek(0) load_csv(csv_file) - except requests.exceptions.RequestException as e: + except httpx.HTTPError as e: logger.warning(f"Failed to fetch reverse DNS map: {e}") except Exception: logger.warning("Not a valid CSV file") diff --git a/parsedmarc/webhook.py b/parsedmarc/webhook.py index 8bf0948..b554985 100644 --- a/parsedmarc/webhook.py +++ b/parsedmarc/webhook.py @@ -4,7 +4,7 @@ from __future__ import annotations from typing import Any -import requests +import httpx from parsedmarc import logger from parsedmarc.constants import USER_AGENT @@ -32,12 +32,12 @@ class WebhookClient(object): self.failure_url = failure_url self.smtp_tls_url = smtp_tls_url self.timeout = timeout - self.session = requests.Session() - self.session.headers.update( - { + self.session = httpx.Client( + headers={ "User-Agent": USER_AGENT, "Content-Type": "application/json", - } + }, + follow_redirects=True, ) def save_failure_report_to_webhook(self, report: str): @@ -56,7 +56,12 @@ class WebhookClient(object): # redundant try/except — removed because _send_to_webhook # already catches every Exception itself. try: - self.session.post(webhook_url, data=payload, timeout=self.timeout) + if isinstance(payload, dict): + # requests form-encoded dict payloads via data=; httpx does + # the same only via data= + self.session.post(webhook_url, data=payload, timeout=self.timeout) + else: + self.session.post(webhook_url, content=payload, timeout=self.timeout) except Exception as error_: logger.error("Webhook Error: {0}".format(error_.__str__())) diff --git a/pyproject.toml b/pyproject.toml index ab3f895..4f7fe20 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,16 +44,21 @@ dependencies = [ "dnspython>=2.0.0", "elasticsearch>=8.18,<9", "expiringdict>=1.1.4", + # The runtime HTTP library (utils.py fetches, webhook and Splunk HEC + # clients, Graph error handling in cli.py). The floor matches + # microsoft-kiota-http's own requirement. + "httpx>=0.25", "kafka-python>=2.3.2", "lxml>=4.4.0", "mailsuite[gmail,msgraph]>=2.2.2", "maxminddb>=2.0.0", + # Imported directly in cli.py for Graph error handling; otherwise + # only a transitive dep of mailsuite[msgraph] -> msgraph-sdk. + "microsoft-kiota-abstractions>=1.8.0", "opensearch-py>=2.4.2,<=4.0.0", "publicsuffixlist>=0.10.0", "pygelf>=0.4.2", - "requests>=2.22.0", "tqdm>=4.31.1", - "urllib3>=1.25.7", "xmltodict>=0.12.0", "PyYAML>=6.0.3" ] @@ -81,6 +86,11 @@ build = [ "pyright==1.1.410", "pytest", "pytest-cov", + # Used only by the out-of-wheel maintainer script + # parsedmarc/resources/maps/collect_domain_info.py, which deliberately + # stays on requests because its permissive-TLS fallback is built on + # urllib3's HTTPAdapter machinery. + "requests>=2.22.0", "ruff", "sphinx", "sphinx_rtd_theme", diff --git a/tests/test_cli.py b/tests/test_cli.py index 8b5dad8..097bfa4 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2044,6 +2044,38 @@ subject = DMARC Summary self.assertIn("request-id=rid-1", output) self.assertIn("client-request-id=crid-1", output) + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.MSGraphConnection") + def testCliPassesSmtpAttachmentAndMessageToMsGraphSend( + self, mock_graph_connection, mock_get_mailbox_reports + ): + """[smtp] attachment/message are parsed but were never wired + through to either summary-email transport. On the Microsoft + Graph path, the configured attachment filename and message + body must reach send_message().""" + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_text = ( + self.CERT_CONFIG + + """ +[smtp] +to = admin@example.com +attachment = custom-report.zip +message = Custom body text +""" + ) + cfg_path = self._write_config(config_text) + self._run_main(cfg_path) + + send_message = mock_graph_connection.return_value.send_message + send_message.assert_called_once() + call_kwargs = send_message.call_args.kwargs + self.assertEqual(call_kwargs["attachments"][0][0], "custom-report.zip") + self.assertEqual(call_kwargs["plain_message"], "Custom body text") + class TestMSGraphFailureLogging(unittest.TestCase): """Microsoft Graph connection/fetch/watch failures log a single @@ -2208,6 +2240,42 @@ certificate_password = s3cret-cert-pass self.assertIn("Microsoft Graph mailbox watch failed", output) self.assertIn("ConnectError", output) + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testNonGraphMailboxErrorIsNotMislabeledAsMsGraph( + self, mock_imap_connection, mock_get_mailbox_reports + ): + """The mailbox-fetch handler catches + (ClientAuthenticationError, APIError, httpx.HTTPError) on every + mailbox backend, not just Graph, since httpx.HTTPError can in + principle surface from any HTTP-backed connection. Before this + fix, such an error on a non-Graph connection (e.g. IMAP) still + went through _log_msgraph_failure() and logged a misleading + "Microsoft Graph ... failed (mailbox=None, tenant_id=None, + auth_method=None)" line. It must now log a generic + "Mailbox Error" instead.""" + mock_imap_connection.return_value = object() + mock_get_mailbox_reports.side_effect = httpx.ConnectError("boom") + + config_text = """[general] +silent = true + +[imap] +host = imap.example.com +user = test-user +password = test-password +""" + cfg_path = self._write_config(config_text) + + with self.assertLogs("parsedmarc.log", level="ERROR") as cm: + with self.assertRaises(SystemExit) as system_exit: + self._run_main(cfg_path) + + self.assertEqual(system_exit.exception.code, 1) + output = "\n".join(cm.output) + self.assertIn("Mailbox Error", output) + self.assertNotIn("Microsoft Graph", output) + class TestSighupReload(unittest.TestCase): """Tests for SIGHUP-driven configuration reload in watch mode.""" @@ -3637,6 +3705,108 @@ class TestParseConfigSmtp(unittest.TestCase): _parse_config(cp, _opts()) +class TestSmtpAttachmentAndMessageWiring(unittest.TestCase): + """[smtp] attachment/message are documented and parsed into + opts.smtp_attachment/opts.smtp_message, but were never passed to + email_results(), so a configured custom attachment filename or + message body was silently ignored on the SMTP transport.""" + + 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 + + def _run_main(self, cfg_path, *cli_args): + with patch.object(sys, "argv", ["parsedmarc", "-c", cfg_path, *cli_args]): + parsedmarc.cli._main() + + @patch("parsedmarc.cli.email_results") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testSmtpAttachmentAndMessageArePassedToEmailResults( + self, mock_imap_connection, mock_get_mailbox_reports, mock_email_results + ): + """A configured attachment filename and message body reach + email_results() rather than being silently dropped.""" + mock_imap_connection.return_value = object() + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_text = """[general] +silent = true + +[imap] +host = imap.example.com +user = test-user +password = test-password + +[smtp] +host = smtp.example.com +user = smtp-user +password = smtp-password +from = dmarc@example.com +to = admin@example.com +attachment = custom-report.zip +message = Custom body text +""" + cfg_path = self._write_config(config_text) + self._run_main(cfg_path) + + mock_email_results.assert_called_once() + call_kwargs = mock_email_results.call_args.kwargs + # The configured value passes through _expand_path(), which is a + # no-op here since the filename has no ~ or $VAR references. + self.assertTrue( + call_kwargs["attachment_filename"].endswith("custom-report.zip") + ) + self.assertEqual(call_kwargs["message"], "Custom body text") + + @patch("parsedmarc.cli.email_results") + @patch("parsedmarc.cli.get_dmarc_reports_from_mailbox") + @patch("parsedmarc.cli.IMAPConnection") + def testSmtpDefaultsFlowThroughWhenNotConfigured( + self, mock_imap_connection, mock_get_mailbox_reports, mock_email_results + ): + """When [smtp] attachment/message are not set, email_results() + still gets the documented defaults (None for the attachment + filename, and the long-documented default message body) rather + than being called with no attachment/message context at all.""" + mock_imap_connection.return_value = object() + mock_get_mailbox_reports.return_value = { + "aggregate_reports": [], + "failure_reports": [], + "smtp_tls_reports": [], + } + config_text = """[general] +silent = true + +[imap] +host = imap.example.com +user = test-user +password = test-password + +[smtp] +host = smtp.example.com +user = smtp-user +password = smtp-password +from = dmarc@example.com +to = admin@example.com +""" + cfg_path = self._write_config(config_text) + self._run_main(cfg_path) + + mock_email_results.assert_called_once() + call_kwargs = mock_email_results.call_args.kwargs + self.assertIsNone(call_kwargs["attachment_filename"]) + self.assertEqual( + call_kwargs["message"], "Please see the attached DMARC results." + ) + + class TestParseConfigS3(unittest.TestCase): def test_s3_complete(self): from parsedmarc.cli import _parse_config diff --git a/tests/test_splunk.py b/tests/test_splunk.py index d91d29a..ed50d46 100644 --- a/tests/test_splunk.py +++ b/tests/test_splunk.py @@ -3,7 +3,7 @@ import json import time import unittest -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from parsedmarc.splunk import HECClient, SplunkError from tests.tzutil import force_tz @@ -211,7 +211,7 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_aggregate_reports_to_splunk(report) - body = client.session.post.call_args.kwargs["data"] + body = client.session.post.call_args.kwargs["content"] events = [json.loads(line) for line in body.strip().split("\n")] self.assertEqual(len(events), 2) for event in events: @@ -225,7 +225,7 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_aggregate_reports_to_splunk(_aggregate_report()) - body = client.session.post.call_args.kwargs["data"] + body = client.session.post.call_args.kwargs["content"] event = json.loads(body.strip())["event"] self.assertEqual(event["source_ip_address"], "192.0.2.1") self.assertEqual(event["header_from"], "example.com") @@ -238,7 +238,7 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_aggregate_reports_to_splunk(_aggregate_report()) - event = json.loads(client.session.post.call_args.kwargs["data"].strip())[ + event = json.loads(client.session.post.call_args.kwargs["content"].strip())[ "event" ] self.assertEqual( @@ -258,7 +258,10 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.save_aggregate_reports_to_splunk([]) client.session.post.assert_not_called() - def test_post_uses_session_verify_and_timeout(self): + def test_post_uses_session_timeout(self): + """httpx has no per-request verify= kwarg — verification is set + at client construction (see test_client_constructed_with_verify_ + false below), so only the per-request timeout is asserted here.""" client = HECClient( url="https://h:8088", access_token="t", @@ -270,9 +273,25 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.session.post.return_value = _ok_response() client.save_aggregate_reports_to_splunk(_aggregate_report()) kwargs = client.session.post.call_args.kwargs - self.assertEqual(kwargs["verify"], False) + self.assertNotIn("verify", kwargs) self.assertEqual(kwargs["timeout"], 15) + def test_client_constructed_with_verify_false(self): + """verify=False must disable TLS verification on the underlying + httpx.Client at construction time, since httpx.Client does not + accept a per-request verify= kwarg like requests.Session did. + Mocked at the httpx SDK boundary (httpx.Client itself).""" + with patch("parsedmarc.splunk.httpx.Client") as mock_client_cls: + HECClient( + url="https://h:8088", + access_token="t", + index="dmarc", + verify=False, + timeout=15, + ) + _, kwargs = mock_client_cls.call_args + self.assertEqual(kwargs["verify"], False) + def test_non_zero_response_code_raises_splunk_error(self): """HEC returns code=0 on success and non-zero codes for token/index/format errors. The error text from HEC carries @@ -306,7 +325,9 @@ class TestSaveAggregateReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_aggregate_reports_to_splunk(_aggregate_report()) - event_wrapper = json.loads(client.session.post.call_args.kwargs["data"].strip()) + event_wrapper = json.loads( + client.session.post.call_args.kwargs["content"].strip() + ) self.assertEqual(event_wrapper["time"], 1704067200) @@ -318,7 +339,9 @@ class TestSaveFailureReportsToSplunk(unittest.TestCase): client.save_failure_reports_to_splunk([_failure_report(), _failure_report()]) events = [ json.loads(line) - for line in client.session.post.call_args.kwargs["data"].strip().split("\n") + for line in client.session.post.call_args.kwargs["content"] + .strip() + .split("\n") ] self.assertEqual(len(events), 2) for event in events: @@ -329,7 +352,7 @@ class TestSaveFailureReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_failure_reports_to_splunk(_failure_report()) - event = json.loads(client.session.post.call_args.kwargs["data"].strip())[ + event = json.loads(client.session.post.call_args.kwargs["content"].strip())[ "event" ] self.assertEqual(event["reported_domain"], "example.com") @@ -354,7 +377,7 @@ class TestSaveFailureReportsToSplunk(unittest.TestCase): client.session = MagicMock() client.session.post.return_value = _ok_response() client.save_failure_reports_to_splunk(_failure_report()) - event = json.loads(client.session.post.call_args.kwargs["data"].strip()) + event = json.loads(client.session.post.call_args.kwargs["content"].strip()) # Fixture arrival_date_utc is 2024-01-01 00:00:00 UTC. self.assertEqual(event["time"], 1704067200) @@ -397,7 +420,9 @@ class TestSaveSmtpTlsReportsToSplunk(unittest.TestCase): client.save_smtp_tls_reports_to_splunk([_smtp_tls_report()]) events = [ json.loads(line) - for line in client.session.post.call_args.kwargs["data"].strip().split("\n") + for line in client.session.post.call_args.kwargs["content"] + .strip() + .split("\n") ] self.assertEqual(len(events), 1) self.assertEqual(events[0]["sourcetype"], "smtp:tls") diff --git a/tests/test_utils.py b/tests/test_utils.py index 3d76968..cd475e8 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -12,7 +12,7 @@ from unittest.mock import MagicMock, patch import dns.exception import dns.resolver -import requests +import httpx from expiringdict import ExpiringDict import parsedmarc @@ -159,7 +159,7 @@ class Test(unittest.TestCase): def _mock_response(status_code, json_body=None): resp = MagicMock() resp.status_code = status_code - resp.ok = 200 <= status_code < 300 + resp.is_success = 200 <= status_code < 300 resp.json.return_value = json_body or {} return resp @@ -173,7 +173,7 @@ class Test(unittest.TestCase): "country_code": "US", } with patch( - "parsedmarc.utils.requests.get", + "parsedmarc.utils.httpx.get", return_value=_mock_response(200, api_json), ) as mock_get: configure_ipinfo_api("fake-token", probe=False) @@ -188,7 +188,7 @@ class Test(unittest.TestCase): # Invalid key: 401 raises a fatal exception even on a random lookup. with patch( - "parsedmarc.utils.requests.get", + "parsedmarc.utils.httpx.get", return_value=_mock_response(401), ): configure_ipinfo_api("bad-token", probe=False) @@ -198,7 +198,7 @@ class Test(unittest.TestCase): # Any other non-2xx (e.g. 500, 503) falls back to the MMDB silently. configure_ipinfo_api("fake-token", probe=False) with patch( - "parsedmarc.utils.requests.get", + "parsedmarc.utils.httpx.get", return_value=_mock_response(500), ): record = get_ip_address_db_record("8.8.8.8") @@ -419,7 +419,7 @@ class TestLoadPSLOverrides(unittest.TestCase): mock_response.text = fake_body mock_response.raise_for_status = MagicMock() with patch( - "parsedmarc.utils.requests.get", return_value=mock_response + "parsedmarc.utils.httpx.get", return_value=mock_response ) as mock_get: result = parsedmarc.utils.load_psl_overrides(url="https://example.test/ov") self.assertEqual(result, ["-fetched-brand.com", ".cdn-fetched.net"]) @@ -427,11 +427,9 @@ class TestLoadPSLOverrides(unittest.TestCase): def test_url_failure_falls_back_to_local(self): """A network error falls back to the bundled copy.""" - import requests - with patch( - "parsedmarc.utils.requests.get", - side_effect=requests.exceptions.ConnectionError("nope"), + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("nope"), ): result = parsedmarc.utils.load_psl_overrides(url="https://example.test/ov") # Bundled file still loaded. @@ -439,8 +437,8 @@ class TestLoadPSLOverrides(unittest.TestCase): self.assertIn(".linode.com", result) def test_always_use_local_skips_network(self): - """always_use_local_file=True must not call requests.get.""" - with patch("parsedmarc.utils.requests.get") as mock_get: + """always_use_local_file=True must not call httpx.get.""" + with patch("parsedmarc.utils.httpx.get") as mock_get: parsedmarc.utils.load_psl_overrides(always_use_local_file=True) mock_get.assert_not_called() @@ -1052,8 +1050,8 @@ class TestUtilsReverseDnsMap(unittest.TestCase): """load_reverse_dns_map falls back to bundled on network error""" rdns_map = {} with patch( - "parsedmarc.utils.requests.get", - side_effect=requests.exceptions.ConnectionError("no network"), + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), ): parsedmarc.utils.load_reverse_dns_map(rdns_map) self.assertTrue(len(rdns_map) > 0) @@ -1065,7 +1063,7 @@ class TestUtilsReverseDnsMap(unittest.TestCase): response.text = "not,the,map\nfoo,bar,baz\n" response.raise_for_status.return_value = None rdns_map = {} - with patch("parsedmarc.utils.requests.get", return_value=response): + with patch("parsedmarc.utils.httpx.get", return_value=response): with self.assertLogs("parsedmarc.log", level="WARNING") as cm: parsedmarc.utils.load_reverse_dns_map(rdns_map) self.assertTrue(any("Not a valid CSV file" in message for message in cm.output)) @@ -1172,7 +1170,7 @@ class TestQueryDnsRetries(unittest.TestCase): class TestLoadIpDb(unittest.TestCase): """Tests for the load_ip_db() download/cache/bundled fallback chain, - mocking at the requests SDK boundary.""" + mocking at the httpx SDK boundary.""" def setUp(self): old_ip_db_path = parsedmarc.utils._IP_DB_PATH @@ -1198,7 +1196,7 @@ class TestLoadIpDb(unittest.TestCase): local_path = os.path.join(self.tmp_dir, "local.mmdb") with open(local_path, "wb") as f: f.write(b"local db") - with patch("parsedmarc.utils.requests.get") as mock_get: + with patch("parsedmarc.utils.httpx.get") as mock_get: parsedmarc.utils.load_ip_db(local_file_path=local_path) mock_get.assert_not_called() self.assertEqual(parsedmarc.utils._IP_DB_PATH, local_path) @@ -1208,7 +1206,7 @@ class TestLoadIpDb(unittest.TestCase): response = MagicMock() response.content = b"downloaded db bytes" response.raise_for_status.return_value = None - with patch("parsedmarc.utils.requests.get", return_value=response) as mock_get: + with patch("parsedmarc.utils.httpx.get", return_value=response) as mock_get: parsedmarc.utils.load_ip_db(url="https://example.com/db.mmdb") self.assertEqual(mock_get.call_args.args[0], "https://example.com/db.mmdb") cached_path = os.path.join(self.tmp_dir, "parsedmarc", "ipinfo_lite.mmdb") @@ -1224,8 +1222,8 @@ class TestLoadIpDb(unittest.TestCase): with open(cached_path, "wb") as f: f.write(b"stale cached db") with patch( - "parsedmarc.utils.requests.get", - side_effect=requests.exceptions.ConnectionError("no network"), + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), ): with self.assertLogs("parsedmarc.log", level="WARNING") as cm: parsedmarc.utils.load_ip_db() @@ -1237,8 +1235,8 @@ class TestLoadIpDb(unittest.TestCase): def testDownloadFailureFallsBackToBundledCopy(self): """On a network error with no cached copy, the bundled db is used""" with patch( - "parsedmarc.utils.requests.get", - side_effect=requests.exceptions.ConnectionError("no network"), + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), ): parsedmarc.utils.load_ip_db() bundled = str(files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb")) @@ -1255,7 +1253,7 @@ class TestLoadIpDb(unittest.TestCase): response.content = b"downloaded db bytes" response.raise_for_status.return_value = None with patch("parsedmarc.utils.tempfile.gettempdir", return_value=blocker): - with patch("parsedmarc.utils.requests.get", return_value=response): + with patch("parsedmarc.utils.httpx.get", return_value=response): with self.assertLogs("parsedmarc.log", level="WARNING") as cm: parsedmarc.utils.load_ip_db() self.assertTrue( @@ -1275,7 +1273,7 @@ class TestConfigureIpinfoApiProbe(unittest.TestCase): def _response(status_code, json_body=None): response = MagicMock() response.status_code = status_code - response.ok = 200 <= status_code < 300 + response.is_success = 200 <= status_code < 300 response.json.return_value = json_body if json_body is not None else {} return response @@ -1283,7 +1281,7 @@ class TestConfigureIpinfoApiProbe(unittest.TestCase): """A successful probe logs that the API is configured""" api_json = {"ip": "1.1.1.1", "asn": "AS13335", "country_code": "US"} with patch( - "parsedmarc.utils.requests.get", + "parsedmarc.utils.httpx.get", return_value=self._response(200, api_json), ): with self.assertLogs("parsedmarc.log", level="INFO") as cm: @@ -1296,8 +1294,8 @@ class TestConfigureIpinfoApiProbe(unittest.TestCase): """A probe network error logs a warning but keeps the token so per-request fallback can take over later""" with patch( - "parsedmarc.utils.requests.get", - side_effect=requests.exceptions.ConnectionError("no network"), + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), ): with self.assertLogs("parsedmarc.log", level="WARNING") as cm: parsedmarc.utils.configure_ipinfo_api("fake-token", probe=True) @@ -1308,7 +1306,7 @@ class TestConfigureIpinfoApiProbe(unittest.TestCase): def testProbeInvalidKeyRaises(self): """A 401 during the probe raises InvalidIPinfoAPIKey""" - with patch("parsedmarc.utils.requests.get", return_value=self._response(401)): + with patch("parsedmarc.utils.httpx.get", return_value=self._response(401)): with self.assertRaises(parsedmarc.utils.InvalidIPinfoAPIKey): parsedmarc.utils.configure_ipinfo_api("bad-token", probe=True) @@ -1323,7 +1321,7 @@ class TestIpinfoApiLookupFallbacks(unittest.TestCase): def _assert_mmdb_fallback(self, response=None, side_effect=None): with patch( - "parsedmarc.utils.requests.get", + "parsedmarc.utils.httpx.get", return_value=response, side_effect=side_effect, ): @@ -1334,21 +1332,19 @@ class TestIpinfoApiLookupFallbacks(unittest.TestCase): self.assertEqual(record["asn"], 15169) def testNetworkErrorFallsBackToMmdb(self): - self._assert_mmdb_fallback( - side_effect=requests.exceptions.ConnectionError("no network") - ) + self._assert_mmdb_fallback(side_effect=httpx.ConnectError("no network")) def testNonJsonBodyFallsBackToMmdb(self): response = MagicMock() response.status_code = 200 - response.ok = True + response.is_success = True response.json.side_effect = ValueError("not JSON") self._assert_mmdb_fallback(response=response) def testNonDictPayloadFallsBackToMmdb(self): response = MagicMock() response.status_code = 200 - response.ok = True + response.is_success = True response.json.return_value = ["not", "a", "dict"] self._assert_mmdb_fallback(response=response) diff --git a/tests/test_webhook.py b/tests/test_webhook.py index 489af50..ad3a929 100644 --- a/tests/test_webhook.py +++ b/tests/test_webhook.py @@ -49,7 +49,7 @@ class TestWebhookClientSaveMethods(unittest.TestCase): client.session = MagicMock() client.save_aggregate_report_to_webhook('{"agg": 1}') client.session.post.assert_called_once_with( - "http://agg.example.com", data='{"agg": 1}', timeout=60 + "http://agg.example.com", content='{"agg": 1}', timeout=60 ) def test_failure_posts_to_failure_url(self): @@ -57,7 +57,7 @@ class TestWebhookClientSaveMethods(unittest.TestCase): client.session = MagicMock() client.save_failure_report_to_webhook('{"fail": 1}') client.session.post.assert_called_once_with( - "http://fail.example.com", data='{"fail": 1}', timeout=60 + "http://fail.example.com", content='{"fail": 1}', timeout=60 ) def test_smtp_tls_posts_to_smtp_tls_url(self): @@ -65,7 +65,21 @@ class TestWebhookClientSaveMethods(unittest.TestCase): client.session = MagicMock() client.save_smtp_tls_report_to_webhook('{"tls": 1}') client.session.post.assert_called_once_with( - "http://tls.example.com", data='{"tls": 1}', timeout=60 + "http://tls.example.com", content='{"tls": 1}', timeout=60 + ) + + +class TestWebhookClientDictPayload(unittest.TestCase): + """``_send_to_webhook`` accepts ``bytes | str | dict``. httpx only + form-encodes a dict via ``data=``; string/bytes payloads must use + ``content=`` since httpx's ``data=`` is form-encoding only.""" + + def test_dict_payload_uses_data_kwarg(self): + client = _client() + client.session = MagicMock() + client._send_to_webhook("http://agg.example.com", {"agg": 1}) + client.session.post.assert_called_once_with( + "http://agg.example.com", data={"agg": 1}, timeout=60 )