From 53d543f49c74a0c6a5abc7ae5191da485ddd69af Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 29 Mar 2026 18:28:52 +0000 Subject: [PATCH] Add report deduplication and single-report deletion Agent-Logs-Url: https://github.com/christianlouis/dmarq/sessions/37d68c2c-7cc8-45e4-bac3-e2e6f1611c6a Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- backend/app/api/api_v1/endpoints/reports.py | 46 ++++++- backend/app/services/report_store.py | 138 ++++++++++++++------ backend/app/tests/test_report_store.py | 64 +++++++++ backend/app/tests/test_reports_api.py | 63 +++++++++ 4 files changed, 269 insertions(+), 42 deletions(-) diff --git a/backend/app/api/api_v1/endpoints/reports.py b/backend/app/api/api_v1/endpoints/reports.py index a83d5bf..521a41a 100644 --- a/backend/app/api/api_v1/endpoints/reports.py +++ b/backend/app/api/api_v1/endpoints/reports.py @@ -178,8 +178,19 @@ async def upload_report(file: UploadFile = File(...)): detail=f"Invalid domain in report: {error_msg}", ) - # Store the report + # Check for duplicate report before storing store = ReportStore.get_instance() + report_id = report.get("report_id", "") + if report_id and store.has_report(domain, report_id): + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"Report '{report_id}' for domain '{domain}' has already been uploaded. " + "Duplicate reports are not stored to keep statistics accurate." + ), + ) + + # Store the report store.add_report(report) processed_records = report.get("summary", {}).get("total_count", 0) @@ -330,3 +341,36 @@ async def get_domain_reports_paginated( return PaginatedReportResponse( total=total, page=page, page_size=page_size, total_pages=total_pages, reports=report_entries ) + + +class DeleteReportResponse(BaseModel): + """Response model for report deletion""" + + success: bool + message: str + + +@router.delete( + "/domain/{domain}/reports/{report_id}", + response_model=DeleteReportResponse, +) +async def delete_report(domain: str, report_id: str): + """ + Delete a single DMARC report for a domain. + + Removes the report from the store and recomputes all domain statistics so + that aggregated numbers remain accurate after deletion. + """ + store = ReportStore.get_instance() + deleted = store.delete_report(domain, report_id) + + if not deleted: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Report '{report_id}' not found for domain '{domain}'.", + ) + + return DeleteReportResponse( + success=True, + message=f"Report '{report_id}' for domain '{domain}' deleted successfully.", + ) diff --git a/backend/app/services/report_store.py b/backend/app/services/report_store.py index d62e220..d60fd49 100644 --- a/backend/app/services/report_store.py +++ b/backend/app/services/report_store.py @@ -33,6 +33,69 @@ class ReportStore: # Domain -> sources (sending IPs) self.domain_sources: Dict[str, Dict[str, Dict[str, Any]]] = {} + def has_report(self, domain: str, report_id: str) -> bool: + """ + Check whether a report with the given report_id already exists for a domain. + + Args: + domain: Domain name + report_id: Report identifier from the DMARC report metadata + + Returns: + True if the report already exists, False otherwise + """ + return any( + r.get("report_id") == report_id for r in self.domain_reports.get(domain, []) + ) + + def _recompute_domain_stats(self, domain: str) -> None: + """ + Recompute summary stats and source data for a domain from its current report list. + + Args: + domain: Domain name whose stats should be recalculated + """ + reports = self.domain_reports.get(domain, []) + + summary: Dict[str, Any] = { + "total_count": 0, + "passed_count": 0, + "failed_count": 0, + "reports_processed": len(reports), + } + sources: Dict[str, Dict[str, Any]] = {} + + for report in reports: + report_summary = report.get("summary", {}) + summary["total_count"] += report_summary.get("total_count", 0) + summary["passed_count"] += report_summary.get("passed_count", 0) + summary["failed_count"] += report_summary.get("failed_count", 0) + + if "policy" in report: + summary["policy"] = report["policy"] + + for record in report.get("records", []): + source_ip = record.get("source_ip", "unknown") + if source_ip not in sources: + sources[source_ip] = { + "count": 0, + "spf_result": "unknown", + "dkim_result": "unknown", + "disposition": "none", + } + sources[source_ip]["count"] += record.get("count", 0) + sources[source_ip]["spf_result"] = record.get("spf", "unknown") + sources[source_ip]["dkim_result"] = record.get("dkim", "unknown") + sources[source_ip]["disposition"] = record.get("disposition", "none") + + total = summary["total_count"] + summary["compliance_rate"] = ( + round(summary["passed_count"] / total * 100, 1) if total > 0 else 0 + ) + + self.domain_summary[domain] = summary + self.domain_sources[domain] = sources + def add_report(self, report: Dict[str, Any]) -> None: """ Add a new report to the store @@ -56,47 +119,8 @@ class ReportStore: # Add the new report self.domain_reports[domain].append(report) - # Update summary stats for this domain - summary = report.get("summary", {}) - self.domain_summary[domain]["total_count"] += summary.get("total_count", 0) - self.domain_summary[domain]["passed_count"] += summary.get("passed_count", 0) - self.domain_summary[domain]["failed_count"] += summary.get("failed_count", 0) - self.domain_summary[domain]["reports_processed"] += 1 - - # Set policy from the latest report - if "policy" in report: - self.domain_summary[domain]["policy"] = report["policy"] - - # Update source data - report_records = report.get("records", []) - for record in report_records: - source_ip = record.get("source_ip", "unknown") - if source_ip not in self.domain_sources[domain]: - self.domain_sources[domain][source_ip] = { - "count": 0, - "spf_result": "unknown", - "dkim_result": "unknown", - "disposition": "none", - } - - # Update source counts and results - self.domain_sources[domain][source_ip]["count"] += record.get("count", 0) - self.domain_sources[domain][source_ip]["spf_result"] = record.get("spf", "unknown") - self.domain_sources[domain][source_ip]["dkim_result"] = record.get("dkim", "unknown") - self.domain_sources[domain][source_ip]["disposition"] = record.get( - "disposition", "none" - ) - - # Calculate compliance rate (percentage of passing emails) - if self.domain_summary[domain]["total_count"] > 0: - pass_rate = ( - self.domain_summary[domain]["passed_count"] - / self.domain_summary[domain]["total_count"] - * 100 - ) - self.domain_summary[domain]["compliance_rate"] = round(pass_rate, 1) - else: - self.domain_summary[domain]["compliance_rate"] = 0 + # Recompute all summary stats from the full list to keep them consistent + self._recompute_domain_stats(domain) def get_domains(self) -> List[str]: """ @@ -187,6 +211,38 @@ class ReportStore: self.domain_summary = {} self.domain_sources = {} + def delete_report(self, domain: str, report_id: str) -> bool: + """ + Delete a single report from the store and recompute domain statistics. + + If the domain has no remaining reports after deletion, the domain entry + is removed entirely from all internal data structures. + + Args: + domain: Domain name + report_id: Report identifier to delete + + Returns: + True if the report was found and deleted, False otherwise + """ + reports = self.domain_reports.get(domain, []) + original_len = len(reports) + self.domain_reports[domain] = [r for r in reports if r.get("report_id") != report_id] + + if len(self.domain_reports[domain]) == original_len: + # Nothing was removed + return False + + if not self.domain_reports[domain]: + # Domain has no remaining reports – clean up entirely + self.domain_reports.pop(domain, None) + self.domain_summary.pop(domain, None) + self.domain_sources.pop(domain, None) + else: + self._recompute_domain_stats(domain) + + return True + def delete_domain_with_cleanup(self, domain: str) -> bool: """ Delete a domain and all its associated data diff --git a/backend/app/tests/test_report_store.py b/backend/app/tests/test_report_store.py index 63bbc06..5dbace5 100644 --- a/backend/app/tests/test_report_store.py +++ b/backend/app/tests/test_report_store.py @@ -77,3 +77,67 @@ class TestReportStore: def test_delete_nonexistent_domain(self): store = ReportStore.get_instance() assert store.delete_domain_with_cleanup("nope.com") is False + + def test_has_report_returns_true_for_existing(self): + store = ReportStore.get_instance() + store.add_report(_sample_report("test.com")) + assert store.has_report("test.com", "rpt-001") is True + + def test_has_report_returns_false_for_missing_report_id(self): + store = ReportStore.get_instance() + store.add_report(_sample_report("test.com")) + assert store.has_report("test.com", "rpt-999") is False + + def test_has_report_returns_false_for_unknown_domain(self): + store = ReportStore.get_instance() + assert store.has_report("nobody.com", "rpt-001") is False + + def test_delete_report_removes_report_and_updates_stats(self): + store = ReportStore.get_instance() + store.add_report(_sample_report("test.com")) + + result = store.delete_report("test.com", "rpt-001") + assert result is True + # Domain should be gone entirely when no reports remain + assert "test.com" not in store.get_domains() + + def test_delete_report_with_remaining_reports_recomputes_stats(self): + store = ReportStore.get_instance() + report_a = _sample_report("test.com") + report_a["report_id"] = "rpt-001" + + report_b = _sample_report("test.com") + report_b["report_id"] = "rpt-002" + report_b["summary"] = {"total_count": 3, "passed_count": 1, "failed_count": 2} + + store.add_report(report_a) + store.add_report(report_b) + + assert store.get_domain_summary("test.com")["reports_processed"] == 2 + + result = store.delete_report("test.com", "rpt-001") + assert result is True + + summary = store.get_domain_summary("test.com") + assert summary["reports_processed"] == 1 + # Stats should now reflect only report_b + assert summary["total_count"] == 3 + assert summary["passed_count"] == 1 + + def test_delete_report_nonexistent_returns_false(self): + store = ReportStore.get_instance() + assert store.delete_report("test.com", "rpt-999") is False + + def test_delete_report_unknown_domain_returns_false(self): + store = ReportStore.get_instance() + assert store.delete_report("nobody.com", "rpt-001") is False + + def test_recompute_stats_after_add(self): + """_recompute_domain_stats is called on add; compliance_rate must be correct.""" + store = ReportStore.get_instance() + report = _sample_report("test.com") + report["summary"] = {"total_count": 10, "passed_count": 8, "failed_count": 2} + store.add_report(report) + + summary = store.get_domain_summary("test.com") + assert summary["compliance_rate"] == 80.0 diff --git a/backend/app/tests/test_reports_api.py b/backend/app/tests/test_reports_api.py index fb77ccf..195cdba 100644 --- a/backend/app/tests/test_reports_api.py +++ b/backend/app/tests/test_reports_api.py @@ -69,3 +69,66 @@ def test_upload_and_get_domain_summary(client: TestClient): assert data["domain"] == "example.com" assert data["total_count"] == 2 assert data["reports_processed"] == 1 + + +def test_duplicate_upload_returns_409(client: TestClient): + """Uploading the same report twice returns 409 Conflict.""" + zip_bytes = _make_zip(SAMPLE_XML) + + first = client.post( + "/api/v1/reports/upload", + files={"file": ("report.zip", zip_bytes, "application/zip")}, + ) + assert first.status_code == 200 + + second = client.post( + "/api/v1/reports/upload", + files={"file": ("report.zip", zip_bytes, "application/zip")}, + ) + assert second.status_code == 409 + assert "already been uploaded" in second.json()["detail"].lower() + + +def test_delete_report_success(client: TestClient): + """Deleting an existing report returns 200 and removes it from the store.""" + zip_bytes = _make_zip(SAMPLE_XML) + client.post( + "/api/v1/reports/upload", + files={"file": ("report.zip", zip_bytes, "application/zip")}, + ) + + # Confirm the domain exists first + assert client.get("/api/v1/reports/domain/example.com/summary").status_code == 200 + + # Delete the report (report_id comes from SAMPLE_XML: "123456789") + response = client.delete("/api/v1/reports/domain/example.com/reports/123456789") + assert response.status_code == 200 + data = response.json() + assert data["success"] is True + + # Domain should be gone now + assert client.get("/api/v1/reports/domain/example.com/summary").status_code == 404 + + +def test_delete_nonexistent_report_returns_404(client: TestClient): + """Deleting a report that does not exist returns 404.""" + response = client.delete("/api/v1/reports/domain/example.com/reports/no-such-id") + assert response.status_code == 404 + + +def test_upload_after_delete_succeeds(client: TestClient): + """After deleting a report, the same report can be uploaded again.""" + zip_bytes = _make_zip(SAMPLE_XML) + + client.post( + "/api/v1/reports/upload", + files={"file": ("report.zip", zip_bytes, "application/zip")}, + ) + client.delete("/api/v1/reports/domain/example.com/reports/123456789") + + response = client.post( + "/api/v1/reports/upload", + files={"file": ("report.zip", zip_bytes, "application/zip")}, + ) + assert response.status_code == 200 + assert response.json()["success"] is True