diff --git a/backend/app/api/api_v1/endpoints/mail_sources.py b/backend/app/api/api_v1/endpoints/mail_sources.py index 088017b..fa197c1 100644 --- a/backend/app/api/api_v1/endpoints/mail_sources.py +++ b/backend/app/api/api_v1/endpoints/mail_sources.py @@ -143,6 +143,161 @@ def _redact_sensitive_text(value: object) -> str: return redact_sensitive_text(value) +DIAGNOSTIC_COPY: Dict[str, Dict[str, Any]] = { + "ok": { + "summary": "Connection test completed successfully.", + "recovery_steps": [], + }, + "auth_required": { + "summary": "The mailbox has not been connected yet.", + "recovery_steps": [ + "Use the Connect Gmail action to complete authorization.", + "Confirm the authorized mailbox is the one that receives DMARC aggregate reports.", + ], + }, + "auth_expired": { + "summary": "The saved authorization is expired, revoked, or no longer accepted.", + "recovery_steps": [ + "Reconnect the mailbox from Mail Sources.", + "If your provider shows a consent screen, approve Gmail read-only access again.", + ], + }, + "authentication": { + "summary": "The server rejected the username, password, app password, or OAuth token.", + "recovery_steps": [ + "Verify the username and use an app-specific password when the provider requires one.", + "Reconnect OAuth sources if the provider recently changed account security settings.", + ], + }, + "permissions": { + "summary": "The account is connected but does not have enough mailbox access.", + "recovery_steps": [ + "Grant read access for the mailbox that receives DMARC reports.", + "For Gmail, reconnect and approve the requested Gmail read-only scope.", + ], + }, + "connectivity": { + "summary": "DMARQ could not reach the mail server reliably.", + "recovery_steps": [ + "Check the server hostname, port, TLS setting, and any firewall allowlists.", + "Use port 993 with SSL for most IMAP providers.", + ], + }, + "mailbox_not_found": { + "summary": "The configured mailbox folder could not be opened.", + "recovery_steps": [ + "Choose one of the available mailbox names returned by the test.", + "Check capitalization and nested folder separators such as Archive/DMARC.", + ], + }, + "throttling": { + "summary": "The mail provider is rate limiting or temporarily refusing requests.", + "recovery_steps": [ + "Wait a few minutes and retry the test.", + "Increase the polling interval if repeated imports trigger provider limits.", + ], + }, + "missing_config": { + "summary": "Required connection settings are missing.", + "recovery_steps": [ + "Fill in the server, username, and password or complete Gmail authorization.", + "Save the source before running stored-source tests.", + ], + }, + "not_implemented": { + "summary": "This connection method cannot be tested from this screen yet.", + "recovery_steps": [ + "Use IMAP or Gmail API for mailbox ingestion.", + "Keep unsupported sources disabled until a test path is implemented.", + ], + }, + "unknown": { + "summary": "The connection failed, but DMARQ could not classify the provider response.", + "recovery_steps": [ + "Retry the test once to rule out a transient provider issue.", + "Check the latest import history and server logs for the sanitized provider response.", + ], + }, +} + + +def _diagnostic_category(message: str, details: Optional[object] = None) -> str: + """Map provider-specific failures to operator-friendly categories.""" + text = f"{message} {_redact_sensitive_text(details or '')}".lower() + if any(term in text for term in ("not yet authorised", "not yet authorized", "complete oauth")): + return "auth_required" + if any( + term in text for term in ("expired", "revoked", "invalid_grant", "refresh token", "oauth") + ): + return "auth_expired" + if any(term in text for term in ("rate", "quota", "throttl", "too many", "429")): + return "throttling" + if any(term in text for term in ("scope", "permission", "access denied", "insufficient")): + return "permissions" + if any(term in text for term in ("mailbox", "folder", "select failed", "does not exist")): + return "mailbox_not_found" + if any(term in text for term in ("credential", "password", "auth", "login", "invalid token")): + return "authentication" + if any( + term in text + for term in ( + "timeout", + "timed out", + "dns", + "resolve", + "refused", + "network", + "ssl", + "certificate", + ) + ): + return "connectivity" + if "not yet implemented" in text: + return "not_implemented" + if any(term in text for term in ("not fully configured", "missing", "required")): + return "missing_config" + return "unknown" + + +def _connection_diagnostic( + success: bool, message: str, details: Optional[object] = None +) -> Dict[str, Any]: + """Build sanitized connection diagnostics for API responses and UI recovery copy.""" + category = "ok" if success else _diagnostic_category(message, details) + diagnostic_copy = DIAGNOSTIC_COPY[category] + diagnostic: Dict[str, Any] = { + "category": category, + "summary": diagnostic_copy["summary"], + "recovery_steps": diagnostic_copy["recovery_steps"], + } + if details and not success: + diagnostic["details"] = _redact_sensitive_text(details) + return diagnostic + + +def _connection_test_response( + success: bool, + message: str, + stats: Optional[Dict[str, Any]] = None, + details: Optional[object] = None, +) -> Dict[str, Any]: + """Normalize stored and ad-hoc mailbox test responses.""" + stats = stats or {} + diagnostic = _connection_diagnostic(success, message, details or stats.get("diagnostic_detail")) + return { + "success": success, + "message": _redact_sensitive_text(message), + "message_count": stats.get("message_count", 0), + "unread_count": stats.get("unread_count", 0), + "dmarc_count": stats.get("dmarc_count", 0), + "available_mailboxes": stats.get("available_mailboxes", []), + "diagnostic": diagnostic, + "diagnostic_category": diagnostic["category"], + "recovery_steps": diagnostic["recovery_steps"], + "timestamp": datetime.now().isoformat(), + } + + def _get_source_or_404(source_id: int, db: Session) -> MailSource: source = db.query(MailSource).filter(MailSource.id == source_id).first() if source is None: @@ -479,12 +634,11 @@ async def test_stored_mail_source( if source.method == "GMAIL_API": if not source.gmail_access_token: - return { - "success": False, - "message": "Gmail API source is not yet authorised. " - "Use the 'Connect Gmail' button to complete OAuth2 authorisation.", - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response( + False, + "Gmail API source is not yet authorised. " + "Use the Connect Gmail button to complete OAuth2 authorisation.", + ) try: gmail_client = GmailClient( client_id=source.gmail_client_id or "", @@ -497,29 +651,27 @@ async def test_stored_mail_source( service.users().getProfile(userId="me").execute() source.last_checked = datetime.utcnow() db.commit() - return { - "success": True, - "message": f"Gmail API credentials are valid (account: {source.gmail_email or 'unknown'}).", - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response( + True, + f"Gmail API credentials are valid (account: {source.gmail_email or 'unknown'}).", + ) except Exception as exc: # pylint: disable=broad-exception-caught logger.error( "Gmail API test failed for source id=%d: %s", int(source_id), _redact_sensitive_text(exc), ) - return { - "success": False, - "message": "Gmail API test failed. Check server logs for details.", - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response( + False, + "Gmail API test failed. The saved authorization may need attention.", + details=exc, + ) if source.method != "IMAP": - return { - "success": False, - "message": f"Connection testing for method '{source.method}' is not yet implemented.", - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response( + False, + f"Connection testing for method '{source.method}' is not yet implemented.", + ) imap_client = IMAPClient( server=source.server, @@ -534,15 +686,7 @@ async def test_stored_mail_source( source.last_checked = datetime.utcnow() db.commit() - return { - "success": success, - "message": message, - "message_count": stats.get("message_count", 0), - "unread_count": stats.get("unread_count", 0), - "dmarc_count": stats.get("dmarc_count", 0), - "available_mailboxes": stats.get("available_mailboxes", []), - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response(success, message, stats) @router.post("/test-connection", response_model=Dict[str, Any]) @@ -558,11 +702,10 @@ async def test_connection_adhoc( method = request.method.upper() if method != "IMAP": - return { - "success": False, - "message": f"Connection testing for method '{method}' is not yet implemented.", - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response( + False, + f"Connection testing for method '{method}' is not yet implemented.", + ) imap_client = IMAPClient( server=request.server, @@ -572,15 +715,7 @@ async def test_connection_adhoc( ) success, message, stats = imap_client.test_connection() - return { - "success": success, - "message": message, - "message_count": stats.get("message_count", 0), - "unread_count": stats.get("unread_count", 0), - "dmarc_count": stats.get("dmarc_count", 0), - "available_mailboxes": stats.get("available_mailboxes", []), - "timestamp": datetime.now().isoformat(), - } + return _connection_test_response(success, message, stats) # --------------------------------------------------------------------------- diff --git a/backend/app/services/imap_client.py b/backend/app/services/imap_client.py index d83ce35..bb58a3b 100644 --- a/backend/app/services/imap_client.py +++ b/backend/app/services/imap_client.py @@ -102,7 +102,11 @@ class IMAPClient: - stats: Dictionary with mailbox statistics (if successful) """ if not all([self.server, self.username, self.password]): - return False, "IMAP credentials not fully configured", {} + return ( + False, + "IMAP credentials not fully configured.", + {"diagnostic_detail": "missing server, username, or password"}, + ) try: # Create IMAP4 connection @@ -119,13 +123,23 @@ class IMAPClient: message_count = 0 unread_count = 0 - if status == "OK": - message_count = int(data[0]) + if status != "OK": + mail.logout() + return ( + False, + "Configured mailbox folder could not be opened.", + { + "available_mailboxes": available_mailboxes, + "diagnostic_detail": f"select failed for folder {self.folder}", + }, + ) - # Count unread messages - status, data = mail.search(None, "UNSEEN") - if status == "OK": - unread_count = len(data[0].split()) + message_count = int(data[0]) + + # Count unread messages + status, data = mail.search(None, "UNSEEN") + if status == "OK": + unread_count = len(data[0].split()) # Gather some stats about potential DMARC reports dmarc_count = 0 @@ -148,9 +162,27 @@ class IMAPClient: } return True, "Connection successful", stats + except imaplib.IMAP4.error as e: + logger.error("IMAP connection test failed: %s", str(e)) + return ( + False, + "IMAP authentication failed or the mailbox server rejected the request.", + {"diagnostic_detail": str(e)}, + ) + except (TimeoutError, OSError) as e: + logger.error("IMAP connection test failed: %s", str(e)) + return ( + False, + "Could not reach the IMAP server.", + {"diagnostic_detail": str(e)}, + ) except Exception as e: # pylint: disable=broad-exception-caught logger.error("IMAP connection test failed: %s", str(e)) - return False, "Connection failed. Check server address and credentials.", {} + return ( + False, + "Connection failed. Check mailbox settings and try again.", + {"diagnostic_detail": str(e)}, + ) def _process_single_email(self, mail, email_id: bytes, stats: dict) -> None: """Fetch, parse, and store DMARC attachments from one email message.""" diff --git a/backend/app/templates/mail_sources.html b/backend/app/templates/mail_sources.html index 4119bde..aba217a 100644 --- a/backend/app/templates/mail_sources.html +++ b/backend/app/templates/mail_sources.html @@ -33,12 +33,23 @@ @@ -440,7 +451,15 @@ @@ -554,8 +573,8 @@ function mailSourcesApp() { historySource: null, importHistory: [], historyLoading: false, - feedback: { message: '', type: '' }, - testResult: { message: '', success: false }, + feedback: { message: '', type: '', diagnostic_summary: '', recovery_steps: [] }, + testResult: { message: '', success: false, diagnostic_summary: '', recovery_steps: [] }, gmailConnected: false, gmailEmail: '', @@ -699,6 +718,22 @@ function mailSourcesApp() { return 'badge-outline'; }, + emptyTestResult() { + return { message: '', success: false, diagnostic_summary: '', recovery_steps: [] }; + }, + + emptyFeedback() { + return { message: '', type: '', diagnostic_summary: '', recovery_steps: [] }; + }, + + diagnosticFromResult(result) { + const diagnostic = result.diagnostic || {}; + return { + diagnostic_summary: diagnostic.summary || '', + recovery_steps: result.recovery_steps || diagnostic.recovery_steps || [], + }; + }, + openAddForm() { this.editingId = null; this.gmailConnected = false; @@ -717,7 +752,7 @@ function mailSourcesApp() { gmail_client_id: '', gmail_client_secret: '', }; - this.testResult = { message: '', success: false }; + this.testResult = this.emptyTestResult(); this.showForm = true; }, @@ -739,14 +774,14 @@ function mailSourcesApp() { gmail_client_id: source.gmail_client_id || '', gmail_client_secret: '', // never pre-fill client secret }; - this.testResult = { message: '', success: false }; + this.testResult = this.emptyTestResult(); this.showForm = true; }, closeForm() { this.showForm = false; this.editingId = null; - this.testResult = { message: '', success: false }; + this.testResult = this.emptyTestResult(); }, async connectGmail() { @@ -869,20 +904,23 @@ function mailSourcesApp() { async testSource(id) { this.testing[id] = true; - this.feedback = { message: '', type: '' }; + this.feedback = this.emptyFeedback(); try { const resp = await fetch(`/api/v1/mail-sources/${id}/test`, { method: 'POST' }); const result = await resp.json(); + const diagnostic = this.diagnosticFromResult(result); if (result.success) { this.feedback = { message: `Connection test successful for source #${id}: ${result.message}`, type: 'success', + ...diagnostic, }; await this.loadSources(); } else { this.feedback = { message: `Connection test failed for source #${id}: ${result.message}`, type: 'error', + ...diagnostic, }; } } catch (e) { @@ -894,7 +932,7 @@ function mailSourcesApp() { async testAdHoc() { this.isTesting = true; - this.testResult = { message: '', success: false }; + this.testResult = this.emptyTestResult(); try { const payload = { server: this.form.server, @@ -910,11 +948,13 @@ function mailSourcesApp() { body: JSON.stringify(payload), }); const result = await resp.json(); + const diagnostic = this.diagnosticFromResult(result); this.testResult = { success: result.success, message: result.success ? `✓ Connected. ${result.message_count || 0} messages, ${result.dmarc_count || 0} potential DMARC reports.` : `✗ ${result.message}`, + ...diagnostic, }; } catch (e) { this.testResult = { success: false, message: `Error: ${e.message}` }; diff --git a/backend/app/tests/test_imap_client.py b/backend/app/tests/test_imap_client.py index 3068b83..dceca58 100644 --- a/backend/app/tests/test_imap_client.py +++ b/backend/app/tests/test_imap_client.py @@ -253,7 +253,7 @@ class TestTestConnection: success, message, stats = client.test_connection() assert success is False assert "not fully configured" in message - assert stats == {} + assert stats["diagnostic_detail"] == "missing server, username, or password" def test_successful_connection(self): client = self._make_client() @@ -296,7 +296,8 @@ class TestTestConnection: with patch("imaplib.IMAP4_SSL", side_effect=ConnectionRefusedError("refused")): success, message, stats = client.test_connection() assert success is False - assert stats == {} + assert "IMAP server" in message + assert stats["diagnostic_detail"] == "refused" def test_list_status_not_ok_returns_empty_mailboxes(self): client = self._make_client() @@ -312,19 +313,21 @@ class TestTestConnection: assert success is True assert stats["available_mailboxes"] == [] - def test_select_not_ok_skips_message_count(self): + def test_select_not_ok_returns_mailbox_diagnostic(self): client = self._make_client() mock_mail = MagicMock() mock_mail.login.return_value = None - mock_mail.list.return_value = ("OK", []) + mock_mail.list.return_value = ("OK", [b'(\\HasNoChildren) "/" INBOX']) mock_mail.select.return_value = ("NO", []) mock_mail.search.return_value = ("OK", [b""]) with patch("imaplib.IMAP4_SSL", return_value=mock_mail): success, message, stats = client.test_connection() - assert success is True - assert stats["message_count"] == 0 + assert success is False + assert "folder" in message + assert stats["available_mailboxes"] == ["INBOX"] + assert "select failed" in stats["diagnostic_detail"] # --------------------------------------------------------------------------- diff --git a/backend/app/tests/test_mail_sources.py b/backend/app/tests/test_mail_sources.py index b325a67..2ce5ab4 100644 --- a/backend/app/tests/test_mail_sources.py +++ b/backend/app/tests/test_mail_sources.py @@ -15,6 +15,7 @@ from fastapi.testclient import TestClient from sqlalchemy import text from sqlalchemy.orm import Session +from app.api.api_v1.endpoints import mail_sources as mail_sources_endpoint from app.core.credential_encryption import is_encrypted_secret from app.models.mail_source import MailSource from app.models.mail_source_import import MailSourceImport @@ -640,6 +641,32 @@ class TestMailSourcesAPIAuthed: assert resp.status_code == 200 data = resp.json() assert data["success"] is False + assert data["diagnostic_category"] == "authentication" + assert data["diagnostic"]["summary"] + assert data["recovery_steps"] + + def test_test_stored_imap_source_sanitizes_diagnostics(self, authed_client: TestClient): + create_resp = authed_client.post( + "/api/v1/mail-sources", + json={"name": "IMAP Secret Fail", "method": "IMAP", "server": "bad.host"}, + ) + source_id = create_resp.json()["id"] + + mock_client = MagicMock() + mock_client.test_connection.return_value = ( + False, + "Connection failed", + {"diagnostic_detail": "password=secret-token refused"}, + ) + + with patch("app.api.api_v1.endpoints.mail_sources.IMAPClient", return_value=mock_client): + resp = authed_client.post(f"/api/v1/mail-sources/{source_id}/test") + + data = resp.json() + assert data["success"] is False + assert data["diagnostic_category"] == "authentication" + assert "secret-token" not in json.dumps(data) + assert "password=**redacted**" in data["diagnostic"]["details"] def test_test_stored_non_imap_source(self, authed_client: TestClient): create_resp = authed_client.post( @@ -700,6 +727,7 @@ class TestMailSourcesAPIAuthed: assert resp.status_code == 200 data = resp.json() assert data["success"] is False + assert data["diagnostic_category"] == "not_implemented" assert "not yet implemented" in data["message"] def test_adhoc_gmail_api_returns_not_implemented(self, authed_client: TestClient): @@ -707,8 +735,23 @@ class TestMailSourcesAPIAuthed: resp = authed_client.post("/api/v1/mail-sources/test-connection", json=payload) assert resp.status_code == 200 assert resp.json()["success"] is False + assert resp.json()["diagnostic_category"] == "not_implemented" assert "not yet implemented" in resp.json()["message"] + def test_diagnostic_category_common_failure_modes(self): + cases = { + "invalid_grant refresh token expired": "auth_expired", + "insufficient permission for gmail scope": "permissions", + "rate limit 429 too many requests": "throttling", + "select failed for folder DMARC": "mailbox_not_found", + "login failed invalid password": "authentication", + "dns timeout refused": "connectivity", + "server is required": "missing_config", + } + + for message, expected in cases.items(): + assert mail_sources_endpoint._diagnostic_category(message) == expected + # --------------------------------------------------------------------------- # Gmail API-specific tests @@ -750,6 +793,8 @@ class TestGmailAPIMailSource: data = resp.json() assert data["success"] is False assert "not yet authorised" in data["message"].lower() or "oauth" in data["message"].lower() + assert data["diagnostic_category"] == "auth_required" + assert any("Connect Gmail" in step for step in data["recovery_steps"]) def test_gmail_source_test_with_valid_token(self, authed_client: TestClient): """Test a GMAIL_API source that has valid OAuth tokens (mocked).""" @@ -1703,7 +1748,8 @@ class TestGmailTestConnectionFailure: # Stack-trace / raw exception text must NOT be exposed to callers assert "internal oauth error" not in data["message"] assert "token expired" not in data["message"] - assert "check server logs" in data["message"].lower() + assert "authorization may need attention" in data["message"].lower() + assert data["diagnostic_category"] == "auth_expired" # ---------------------------------------------------------------------------