diff --git a/CHANGELOG.md b/CHANGELOG.md index c7e50d2b4..279a1e664 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,28 @@ All notable changes to Massing. Releases are signed, auto-updating desktop build (Windows / macOS / Linux); the updater always serves the latest. Format loosely follows [Keep a Changelog](https://keepachangelog.com/). +## Unreleased — R24-REPORTS-BY-MOMENT: a finished pack can be sent, not only downloaded + +`POST /projects/{pid}/jobs/{job_id}/deliver` emails any finished job's artifact to named +recipients, surfaced as **Send** beside **Download** in the job tray. + +**The roadmap named the wrong blocker, one layer too high.** It said this "still wants a delivery +surface and SMTP" — both of which already shipped: `mailer.py` sends real mail, and the notification +digest is a working assemble-then-send surface. What was actually missing was smaller and more +specific: **the mailer could not carry a file**. `build_message` gained attachments, and the order +matters — `add_alternative` must run before `add_attachment` or Python refuses outright, which the +test asserts rather than assumes. + +Refusals mirror the download route exactly (404 wrong project, 409 while queued/running, 404 with no +artifact), so a caller does not learn two answers to "is this artifact ready", plus two of its own: +an empty recipient list is 422 rather than a silent success, and over 15 MB is 413 rather than a +per-recipient error from a server that would have bounced it. A deployment with no SMTP configured +returns 200 with every recipient `disabled`, so the UI reads `smtp_configured` before claiming a +send. The delivery is audited — a file leaving the system is what an audit log is for. + +**Not shipped, deliberately: the SCHEDULED half.** There is no scheduler of any kind in this tree, +so choosing in-process versus external cron is a deployment decision, not a wiring task. + ## v0.3.1143 (2026-09-01) — SCALE-SEAM ㉝, Last-Planner onto schedule.ts Six methods out of `client.ts` into the existing `apps/web/src/api/schedule.ts` mixin diff --git a/apps/web/public/wasm/web-ifc-mt.wasm b/apps/web/public/wasm/web-ifc-mt.wasm old mode 100644 new mode 100755 diff --git a/apps/web/public/wasm/web-ifc.wasm b/apps/web/public/wasm/web-ifc.wasm old mode 100644 new mode 100755 diff --git a/apps/web/src/api/routines.ts b/apps/web/src/api/routines.ts index 8bdda385a..1aab3382a 100644 --- a/apps/web/src/api/routines.ts +++ b/apps/web/src/api/routines.ts @@ -115,5 +115,15 @@ export function withRoutines>(Base: TBase) { jobArtifactUrl(pid: string, jobId: string): string { return this.url(`/projects/${pid}/jobs/${jobId}/artifact`); } + /** R24-REPORTS-BY-MOMENT — mail a finished job's artifact to recipients: the "shared, not just + * downloaded" half. Same refusals as the artifact URL above (404 / 409 while running / 404 with + * no artifact), plus 422 on no recipients and 413 over the 15 MB cap. On a deployment with no + * SMTP configured this SUCCEEDS with every recipient reported `disabled` — check + * `smtp_configured` before telling the user it was sent. */ + deliverJobArtifact(pid: string, jobId: string, to: string[], note = "") { + return this.json<{ smtp_configured: boolean; filename: string; bytes: number; + results: Record }>( + `/projects/${pid}/jobs/${jobId}/deliver`, { method: "POST", body: JSON.stringify({ to, note }) }); + } }; } diff --git a/apps/web/src/api/surface.test.ts b/apps/web/src/api/surface.test.ts index 058563ae4..59282383b 100644 --- a/apps/web/src/api/surface.test.ts +++ b/apps/web/src/api/surface.test.ts @@ -205,7 +205,7 @@ describe("the API client's public surface", () => { "escalationsScan", "sendDigest", "notificationStream", // 18 overdue / digest "reviewModelVersion", "modelVersions", "versionDiff", // 19 publish history "importClashXlsx", "importClashXml", // 20 clash import - "enqueueJob", "jobs", "jobArtifactUrl", // 21 job tray + "enqueueJob", "jobs", "jobArtifactUrl", "deliverJobArtifact", // 21 job tray "projects", "createProject", "importBundle", // 22 project catalog "integrations", "license", "capabilities", // 23 deploy entitle "siteContext", "parcelAnalyze", "parcelsScreen", // 24 land around site diff --git a/apps/web/src/main.ts b/apps/web/src/main.ts index 0fe227415..7db55cc4c 100644 --- a/apps/web/src/main.ts +++ b/apps/web/src/main.ts @@ -2171,6 +2171,23 @@ const _jobs = _embed ? null : mountJobTray({ host: toolbar, fetch: () => (projectId ? api.jobs(projectId, 25) : Promise.resolve([])), artifactUrl: (j) => api.jobArtifactUrl(projectId!, j.id), + // R24-REPORTS-BY-MOMENT — "shared, not just downloaded". `prompt` rather than a modal on purpose: + // the recipient list is the whole input, and a dialog for one text field is chrome. An + // unconfigured deployment answers 200 with every recipient `disabled`, which is why the notice + // below reads `smtp_configured` instead of assuming a 200 means the mail went. + onSend: (j) => { + const to = window.prompt("Email this artifact to (comma-separated addresses):", ""); + if (to === null) return; + const addrs = to.split(",").map((a) => a.trim()).filter(Boolean); + if (!addrs.length) { notify("No recipients — nothing sent.", "error"); return; } + void api.deliverJobArtifact(projectId!, j.id, addrs) + .then((r) => notify( + r.smtp_configured + ? `${r.filename} sent to ${(r.results.sent ?? []).length} of ${addrs.length}` + : "Email is not configured on this server — nothing was sent.", + r.smtp_configured && (r.results.sent ?? []).length ? "success" : "error")) + .catch((e: Error) => notify(`Send failed — ${e.message}`, "error")); + }, // The completion notice is the point of the tray: it is what makes leaving safe. onSettled: (j) => notify( j.state === "error" ? `${jobLabel(j.kind)} failed — ${j.error ?? "no detail"}` : `${jobLabel(j.kind)} finished`, diff --git a/apps/web/src/ui/jobTray.test.ts b/apps/web/src/ui/jobTray.test.ts index 574181fc1..c7d05263f 100644 --- a/apps/web/src/ui/jobTray.test.ts +++ b/apps/web/src/ui/jobTray.test.ts @@ -305,4 +305,32 @@ describe("the tray is actually reachable", () => { document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" })); expect(btn().getAttribute("aria-expanded")).toBe("false"); }); + + /** + * R24-REPORTS-BY-MOMENT — the send affordance is gated on there being a file, exactly like the + * download link beside it. Asserted rather than assumed because the two gates are written + * separately, and a "Send" on a running job offers to mail something that does not exist yet. + */ + it("offers Send only on rows that actually have an artifact", () => { + const host = document.createElement("div"); + const sent: string[] = []; + renderJobTray(host, [ + J({ id: "running", state: "running" }), + J({ id: "noart", state: "done", result: {} }), + J({ id: "ready", state: "done", result: { artifact_key: "k" } }), + ], { onSend: (j) => sent.push(j.id) }); + + const buttons = [...host.querySelectorAll("button")].filter((b) => b.textContent === "Send"); + expect(buttons.length).toBe(1); + buttons[0]!.click(); + expect(sent).toEqual(["ready"]); + }); + + /** Omitting `onSend` must offer no button at all — the same contract `artifactUrl` already has, + * so a host that cannot deliver does not show a control that would throw. */ + it("offers no Send affordance when onSend is omitted", () => { + const host = document.createElement("div"); + renderJobTray(host, [J({ state: "done", result: { artifact_key: "k" } })], {}); + expect([...host.querySelectorAll("button")].some((b) => b.textContent === "Send")).toBe(false); + }); }); diff --git a/apps/web/src/ui/jobTray.ts b/apps/web/src/ui/jobTray.ts index 0c3c92e98..d46b15ef7 100644 --- a/apps/web/src/ui/jobTray.ts +++ b/apps/web/src/ui/jobTray.ts @@ -137,6 +137,9 @@ const STATE_COLOR: Record = { export interface JobTrayOpts { /** Absolute href for a finished job's artifact. Omitted → no download affordance is offered. */ artifactUrl?: (j: Job) => string; + /** R24-REPORTS-BY-MOMENT — mail a finished artifact to recipients ("shared, not just + * downloaded"). Omitted → no send affordance, exactly like `artifactUrl`. */ + onSend?: (j: Job) => void; /** Remove a finished/failed row from view. Client-side only — the server keeps its history. */ onDismiss?: (j: Job) => void; } @@ -206,6 +209,20 @@ export function renderJobTray(host: HTMLElement, jobs: readonly Job[], opts: Job row.appendChild(a); } + // Sending sits beside downloading because they answer the same question — "the pack is ready, + // now what" — and a report pack that can only be downloaded still has to be forwarded by hand. + // Gated on `hasArtifact` for the same reason the link is: there is nothing to send until there + // is a file. + if (opts.onSend && hasArtifact(j)) { + const b = document.createElement("button"); + b.type = "button"; + b.textContent = "Send"; + b.title = "Email this artifact to recipients"; + b.style.cssText = "font-size:11px;flex:0 0 auto"; + b.onclick = () => opts.onSend!(j); + row.appendChild(b); + } + // Only finished rows can be dismissed. Hiding a running job would leave work in flight with no // way back to it, which is the exact failure the tray exists to fix. if (opts.onDismiss && !isActive(j)) { @@ -312,6 +329,8 @@ export function mountJobTray(opts: { host: HTMLElement; fetch: () => Promise; artifactUrl?: (j: Job) => string; + /** R24-REPORTS-BY-MOMENT — see JobTrayOpts.onSend. Passed straight through to each row. */ + onSend?: (j: Job) => void; onSettled?: (j: Job) => void; /** * R24-RUNS-INBOX — open the run history. A footer row rather than a header button, because the @@ -359,6 +378,7 @@ export function mountJobTray(opts: { if (!panel.hidden) { renderJobTray(panel, shown, { artifactUrl: opts.artifactUrl, + onSend: opts.onSend, onDismiss: (j) => { dismissed.add(j.id); draw(); }, }); if (opts.onHistory) { diff --git a/docs/roadmap.md b/docs/roadmap.md index 704245368..c9d4b679e 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -2062,10 +2062,20 @@ refute one, so this goes first even though it is the least visible. heading below. `reportMoments.test.ts` reads `reports.py` and fails the build if a package names an id the server no longer defines; without that, a renamed report shortens a package silently on the Friday it is due. - **Still open: "scheduled and shared, not just downloaded."** Assemble is a job - (`report_package` in `services/api/src/aec_api/jobs.py`, **Assemble** in `apps/web/src/reportCenter.ts`). - Making it a *scheduled deliverable* — sent to a recipient on a date — still wants a delivery surface - and SMTP. The Job row is already the record that a pack ran. + **SHARED shipped; SCHEDULED still open — and the blocker was never the one written here.** This + entry said making a pack a scheduled deliverable "still wants a delivery surface and SMTP". + **Both already existed** when that was written: `services/api/src/aec_api/mailer.py` sends real mail + (stdlib `smtplib`, a Settings "Test connection" button), and `POST …/notifications/digest` is a + working assemble-then-send surface returning a per-recipient status map. What was actually missing + was one size smaller — **the mailer could not carry a file**. `POST …/jobs/{job_id}/deliver` now + mails any finished job's artifact (`services/api/test_artifact_deliver.py`), surfaced as **Send** + beside **Download** in the job tray. *Naming the blocker one layer too high is what let it sit: the + two named things were present, so every look confirmed the entry and nobody checked the layer below.* + **What genuinely remains is SCHEDULED, and it needs a runner.** There is no scheduler of any kind in + this tree — no APScheduler, no croniter, no cron — so the existing digest is admin-triggered and + nothing runs on a date. Choosing in-process versus external cron hitting an endpoint is a + **deployment decision with different operational consequences, not a wiring task**, which is why it + is not taken here. The Job row is already the record that a pack ran. - **R24-TERMS** *(S)* — the remaining long tail (element/component and estimate/budget/cost pairs are a user decision; storey/floor settled v0.3.945). diff --git a/services/api/run_tests.py b/services/api/run_tests.py index d3db2341b..5db239565 100644 --- a/services/api/run_tests.py +++ b/services/api/run_tests.py @@ -86,7 +86,7 @@ "test_markup", "test_route_authz", "test_resource_id_authz", "test_route_reachability", "test_resumable_upload", "test_model_align", "test_ifc_parse_gate", "test_plugin_isolation", "test_body_pid_authz", "test_global_authz", "test_protected_prefix_coverage", "test_baseline", "test_global_mutating_authz", "test_ref_counter", "test_audit_coverage", "test_bsdd", "test_openbim_registry", "test_waterfall", "test_waterfall_cents", "test_sessions", "test_mfa", "test_stored_ids", "test_cobie", "test_fts_index", "test_scim", "test_scim_provision_race", "test_saml", "test_responsibility", "test_array_live", "test_assemblies", "test_dxf_takeoff", "test_qto_class_match", "test_georef", "test_scene_package", "test_clash_bvh", "test_model_qa", "test_model_health", "test_roundtrip_qa", "test_stakeholder", "test_prioritization", "test_ai_readiness", - "test_scan_deviation", "test_plan_to_bim", "test_errorlog", "test_import_cycles", "test_tenant_scoping", "test_schedule_risk_single", "test_carbon_compliance", "test_permit_check", "test_drawing_qa", "test_element_5d", "test_authoring_matrix", "test_option_missing", "test_option_score", "test_plugin_registry", "test_jobs", "test_clash_federated_job", "test_inbox_jobs", "test_job_kind_labels", "test_worker_split", "test_job_orphan_scope", "test_job_stall", "test_pid_lock_xproc", "test_pid_lock_surface", "test_sheet_layout", "test_dim_component", "test_sheet_recover", "test_firm_standards", "test_site_context", "test_risk_board", "test_env_wind", "test_model_options", "test_doc_text", "test_escalation", "test_query_dsl", "test_rule_library", "test_schedule_baselines", "test_model_ci", "test_xlsx_roundtrip", "test_geometric_rules", "test_rebar_rules", "test_cx", "test_distwaterfall", "test_license_cloud", "test_smart_views", "test_view_delete", "test_version_approve_identity", "test_upload_streaming", "test_lod_aspects", "test_lod_element_table", "test_publish_reconvert", "test_model_cache_seed", "test_model_cache_mutation", "test_mutating_readers", "test_adopt_guid", "test_ifcpatch", "test_bcf_api", "test_coordination_fresh", "test_assemblies_cost", "test_fem_export", "test_subset_export", "test_norm_valid", "test_schema_diag", "test_revision_delta", "test_bep", "test_pm_close", "test_itp", "test_quality_chain", "test_quality_chain_route", "test_meeting_links", "test_est_bands", "test_scope_gap", "test_golden_thread", "test_clash_xml_import", "test_gis_out", "test_cbs", "test_mep_graph", "test_model_warnings", "test_schedule_options", "test_master_builder", "test_master_builder_scope", "test_get_commits", "test_project_pulse", "test_client_portal", "test_selections", "test_margin", "test_model_assets", "test_macros", "test_layout_options", "test_equipment", "test_space_util", "test_design_metrics", "test_mep_fittings", "test_prod_actuals", "test_pipeline_allocate", "test_production", "test_procure_level", "test_adjacency", "test_supply_chain", "test_invisible_unicode", "test_cited_answer", "test_est_confidence", "test_buyout_schedule", "test_scope_register", "test_permit_timeline", "test_absorption", "test_progress_rollup", "test_fill_matrix", "test_parcel_geometry", "test_assembly_thermal", "test_portal_txn", "test_persona_answer", "test_boe_ledger", "test_assumption_provenance", "test_assumption_provenance_route", "test_concept_budget", "test_topic_board", "test_roof_window", "test_topic_lifecycle", "test_comment_promote", "test_calc_fields", "test_constraints", "test_element_lookup", "test_cli", "test_view_templates", "test_type_catalogs", "test_password_policy", "test_stepup_single_verifier", "test_fin_gov", "test_fin_calc", "test_fin_ingest", "test_fin_portfolio", "test_level_move", "test_instance_props", "test_roundtrip", "test_wall_joins", "test_composite_family", "test_shared_params", "test_version_values", "test_ifcpatch_transforms", "test_bcf3", "test_energy_export", "test_net_effective", "test_cre_deal_desk", "test_cre_governance", "test_cre_tier3", "test_family_geometry", "test_demo_seed", "test_cost_spine", "test_commercial_drift", "test_family_shapes", "test_workflow_config", "test_option_takeoff", "test_option_carbon", "test_option_carbon_route", "test_option_economics", "test_option_economics_route", "test_option_object", "test_option_object_route", "test_family_coverage", "test_section_annotation", "test_lod500_readiness", "test_scan_to_lod500", "test_egress_routes", "test_status_workflow_parity", "test_section_hatch", "test_section_keynotes", "test_detail_refs", "test_vg_overrides", "test_revit_export_cfg", "test_soft_clash", "test_sequence_clash", "test_element_tags", "test_cost_ifc", "test_fived", "test_health_consistency", "test_module_rooms", "test_modules_response_complete", "test_lifecycle_strip", "test_family_merge", "test_element_facts", "test_consistency", "test_work_queue", "test_task_bind", "test_qto_wire", "test_estimate_diff", "test_dim_constraints", "test_sov_build", "test_takeoff_scope", "test_r37_wire_routes", "test_r37_consolidate", "test_r37_contract", "test_export_promises", "test_pdf_ingest_gate", "test_roadmap_status", "test_claim_type", "test_risk_calibrate", "test_schedule_status", "test_engine_routes", "test_reachable", "test_money_wire", "test_license_gate", "test_license_lock_gate", "test_lock_advisories", "test_npm_advisories", "test_perf_budget", "test_perf_rate", "test_cache_key", "test_oauth_providers", "test_qto_measured_area", "test_lod_census", "test_lod_proxy", "test_model_ensure", "test_support_graph", "test_export_colour_stable", "test_stair_ramp", "test_profile_dims", "test_eot", "test_eot_methods", "test_eot_sourced", "test_shared_model", "test_plan_identity", "test_axon_view", "test_view_kind_dispatch", "test_photo_cv", "test_photo_detect", "test_photo_duplicate", "test_pipeline_scales", "test_plan_pins", "test_plan_cut_quality", "test_pins_unified", "test_index_freshness", "test_bake_budget", "test_geom_slots", "test_bake_shared", "test_geo_ref", "test_asset_verify", "test_folder_owner", "test_file_sizes", "test_declared_imports", "test_ruff_scope", "test_delete_ratchet", "test_doc_substance", "test_claude_md_gates", "test_cors_expose_headers", "test_open_redirect", "test_mp_engine", "test_upload_cap", "test_vitals", "test_samples", "test_bundle_index", + "test_scan_deviation", "test_plan_to_bim", "test_errorlog", "test_import_cycles", "test_tenant_scoping", "test_schedule_risk_single", "test_carbon_compliance", "test_permit_check", "test_drawing_qa", "test_element_5d", "test_authoring_matrix", "test_option_missing", "test_option_score", "test_plugin_registry", "test_jobs", "test_clash_federated_job", "test_inbox_jobs", "test_job_kind_labels", "test_worker_split", "test_job_orphan_scope", "test_job_stall", "test_pid_lock_xproc", "test_pid_lock_surface", "test_sheet_layout", "test_dim_component", "test_sheet_recover", "test_firm_standards", "test_site_context", "test_risk_board", "test_env_wind", "test_model_options", "test_doc_text", "test_escalation", "test_query_dsl", "test_rule_library", "test_schedule_baselines", "test_model_ci", "test_xlsx_roundtrip", "test_geometric_rules", "test_rebar_rules", "test_cx", "test_distwaterfall", "test_license_cloud", "test_smart_views", "test_view_delete", "test_version_approve_identity", "test_upload_streaming", "test_lod_aspects", "test_lod_element_table", "test_publish_reconvert", "test_model_cache_seed", "test_model_cache_mutation", "test_mutating_readers", "test_adopt_guid", "test_ifcpatch", "test_bcf_api", "test_coordination_fresh", "test_assemblies_cost", "test_fem_export", "test_subset_export", "test_norm_valid", "test_schema_diag", "test_revision_delta", "test_bep", "test_pm_close", "test_itp", "test_quality_chain", "test_quality_chain_route", "test_meeting_links", "test_est_bands", "test_scope_gap", "test_golden_thread", "test_clash_xml_import", "test_gis_out", "test_cbs", "test_mep_graph", "test_model_warnings", "test_schedule_options", "test_master_builder", "test_master_builder_scope", "test_get_commits", "test_project_pulse", "test_client_portal", "test_selections", "test_margin", "test_model_assets", "test_macros", "test_layout_options", "test_equipment", "test_space_util", "test_design_metrics", "test_mep_fittings", "test_prod_actuals", "test_pipeline_allocate", "test_production", "test_procure_level", "test_adjacency", "test_supply_chain", "test_invisible_unicode", "test_cited_answer", "test_est_confidence", "test_buyout_schedule", "test_scope_register", "test_permit_timeline", "test_absorption", "test_progress_rollup", "test_fill_matrix", "test_parcel_geometry", "test_assembly_thermal", "test_portal_txn", "test_persona_answer", "test_boe_ledger", "test_assumption_provenance", "test_assumption_provenance_route", "test_concept_budget", "test_topic_board", "test_roof_window", "test_topic_lifecycle", "test_comment_promote", "test_artifact_deliver", "test_calc_fields", "test_constraints", "test_element_lookup", "test_cli", "test_view_templates", "test_type_catalogs", "test_password_policy", "test_stepup_single_verifier", "test_fin_gov", "test_fin_calc", "test_fin_ingest", "test_fin_portfolio", "test_level_move", "test_instance_props", "test_roundtrip", "test_wall_joins", "test_composite_family", "test_shared_params", "test_version_values", "test_ifcpatch_transforms", "test_bcf3", "test_energy_export", "test_net_effective", "test_cre_deal_desk", "test_cre_governance", "test_cre_tier3", "test_family_geometry", "test_demo_seed", "test_cost_spine", "test_commercial_drift", "test_family_shapes", "test_workflow_config", "test_option_takeoff", "test_option_carbon", "test_option_carbon_route", "test_option_economics", "test_option_economics_route", "test_option_object", "test_option_object_route", "test_family_coverage", "test_section_annotation", "test_lod500_readiness", "test_scan_to_lod500", "test_egress_routes", "test_status_workflow_parity", "test_section_hatch", "test_section_keynotes", "test_detail_refs", "test_vg_overrides", "test_revit_export_cfg", "test_soft_clash", "test_sequence_clash", "test_element_tags", "test_cost_ifc", "test_fived", "test_health_consistency", "test_module_rooms", "test_modules_response_complete", "test_lifecycle_strip", "test_family_merge", "test_element_facts", "test_consistency", "test_work_queue", "test_task_bind", "test_qto_wire", "test_estimate_diff", "test_dim_constraints", "test_sov_build", "test_takeoff_scope", "test_r37_wire_routes", "test_r37_consolidate", "test_r37_contract", "test_export_promises", "test_pdf_ingest_gate", "test_roadmap_status", "test_claim_type", "test_risk_calibrate", "test_schedule_status", "test_engine_routes", "test_reachable", "test_money_wire", "test_license_gate", "test_license_lock_gate", "test_lock_advisories", "test_npm_advisories", "test_perf_budget", "test_perf_rate", "test_cache_key", "test_oauth_providers", "test_qto_measured_area", "test_lod_census", "test_lod_proxy", "test_model_ensure", "test_support_graph", "test_export_colour_stable", "test_stair_ramp", "test_profile_dims", "test_eot", "test_eot_methods", "test_eot_sourced", "test_shared_model", "test_plan_identity", "test_axon_view", "test_view_kind_dispatch", "test_photo_cv", "test_photo_detect", "test_photo_duplicate", "test_pipeline_scales", "test_plan_pins", "test_plan_cut_quality", "test_pins_unified", "test_index_freshness", "test_bake_budget", "test_geom_slots", "test_bake_shared", "test_geo_ref", "test_asset_verify", "test_folder_owner", "test_file_sizes", "test_declared_imports", "test_ruff_scope", "test_delete_ratchet", "test_doc_substance", "test_claude_md_gates", "test_cors_expose_headers", "test_open_redirect", "test_mp_engine", "test_upload_cap", "test_vitals", "test_samples", "test_bundle_index", # R41-TEST-RESIDUE — the residue sweep must never propose a database it does not own: "test_sweep_guard", # R23-DIGEST — the deterministic model digest and its two routes: diff --git a/services/api/src/aec_api/mailer.py b/services/api/src/aec_api/mailer.py index 802762656..371f094ed 100644 --- a/services/api/src/aec_api/mailer.py +++ b/services/api/src/aec_api/mailer.py @@ -12,6 +12,7 @@ import logging import smtplib +import ssl from email.message import EmailMessage from . import settings_store @@ -36,7 +37,7 @@ def smtp_test() -> dict: try: with smtplib.SMTP(host, port, timeout=15) as s: if settings_store.get("AEC_SMTP_TLS", "1") == "1": - s.starttls() + s.starttls(context=ssl.create_default_context()) # verified — see send_email user, pw = settings_store.get("AEC_SMTP_USER"), settings_store.get("AEC_SMTP_PASSWORD") if user and pw: s.login(user, pw) @@ -45,8 +46,14 @@ def smtp_test() -> dict: return {"ok": False, "message": f"SMTP failed: {str(e)[:140]}"} -def build_message(to: str, subject: str, body_text: str, body_html: str | None = None) -> EmailMessage: - """Construct a well-formed (optionally multipart) message — pure, no I/O (testable).""" +def build_message(to: str, subject: str, body_text: str, body_html: str | None = None, + attachments: list[tuple[str, bytes, str]] | None = None) -> EmailMessage: + """Construct a well-formed (optionally multipart) message — pure, no I/O (testable). + + `attachments` are `(filename, data, mime)` triples. Order matters: `add_alternative` must run + BEFORE `add_attachment`, or the html alternative lands inside the mixed part and clients show + the attachment where the body should be. + """ msg = EmailMessage() msg["From"] = _from_addr() msg["To"] = to @@ -54,27 +61,61 @@ def build_message(to: str, subject: str, body_text: str, body_html: str | None = msg.set_content(body_text) if body_html: msg.add_alternative(body_html, subtype="html") + for filename, data, mime in attachments or []: + maintype, _, subtype = mime.partition("/") + msg.add_attachment(data, maintype=maintype or "application", + subtype=subtype or "octet-stream", filename=filename) return msg -def send_email(to: str, subject: str, body_text: str, body_html: str | None = None) -> str: +def send_email(to: str, subject: str, body_text: str, body_html: str | None = None, + attachments: list[tuple[str, bytes, str]] | None = None) -> str: """Send one message. Returns "sent" | "disabled" | "error". Never raises (so a digest run can't be broken by one bad address / transient SMTP failure).""" - msg = build_message(to, subject, body_text, body_html) if not smtp_configured(): - _log.info("email disabled (no AEC_SMTP_HOST) — would send %r to %s", subject, to) + # Still built, so an unconfigured deployment fails on a malformed address the same way a + # configured one does — a bad recipient must not become visible only in production. + try: + build_message(to, subject, body_text, body_html, attachments) + except Exception as e: # noqa: BLE001 + # %r, not %s: `to` is attacker-influenced and a CR/LF in it writes literal newlines + # into the log stream, so a recipient can forge whole log lines (CWE-117). repr escapes + # them. Same at the send handler below. + _log.warning("email not built for %r: %s", to, e) + return "error" + _log.info("email disabled (no AEC_SMTP_HOST) — would send %r to %r", subject, to) return "disabled" - host = settings_store.get("AEC_SMTP_HOST") - port = int(settings_store.get("AEC_SMTP_PORT", "587")) try: + # EVERYTHING that can raise belongs inside this boundary, not just the message build. The + # first fix moved `build_message` in and left `int(AEC_SMTP_PORT)` outside — and settings are + # stored as arbitrary strings (`settings_store.set_value(db, k, str(v))`, no numeric check), + # so a mistyped port raised ValueError one line above the guard that exists to prevent + # exactly that. Treating the instance rather than the class is what left it; the rule is that + # this function returns a status for ANY input, configuration included. + host = settings_store.get("AEC_SMTP_HOST") + port = int(settings_store.get("AEC_SMTP_PORT", "587")) + msg = build_message(to, subject, body_text, body_html, attachments) with smtplib.SMTP(host, port, timeout=15) as s: if settings_store.get("AEC_SMTP_TLS", "1") == "1": - s.starttls() + # An explicit verified context. `starttls()` with no argument uses + # `ssl._create_stdlib_context()`, which on this interpreter reports + # verify_mode=0 / check_hostname=False — no certificate check at all, so the + # artifact and the SMTP credentials go up unauthenticated. + s.starttls(context=ssl.create_default_context()) user, pw = settings_store.get("AEC_SMTP_USER"), settings_store.get("AEC_SMTP_PASSWORD") if user and pw: + if settings_store.get("AEC_SMTP_TLS", "1") != "1": + # Deliberately a loud warning, not a refusal. `AEC_SMTP_TLS=0` is a documented + # deployment choice for a self-hosted product relaying through localhost or a + # trusted internal MTA, where cleartext is not an exposure; hard-refusing would + # break those installs to protect against a risk they do not have. What is not + # defensible is doing it SILENTLY, so the operator is told each time. + _log.warning("SMTP auth over cleartext: AEC_SMTP_TLS=0 and a password is set, " + "so the credential leaves this host unprotected. Set AEC_SMTP_TLS=1 " + "unless the relay is local or on a trusted network.") s.login(user, pw) s.send_message(msg) return "sent" except Exception as e: # noqa: BLE001 — one bad send must not abort a batch - _log.warning("email send failed to %s: %s", to, e) + _log.warning("email send failed to %r: %s", to, e) return "error" diff --git a/services/api/src/aec_api/routers/jobs.py b/services/api/src/aec_api/routers/jobs.py index e7e46cac1..8048e2bca 100644 --- a/services/api/src/aec_api/routers/jobs.py +++ b/services/api/src/aec_api/routers/jobs.py @@ -5,7 +5,7 @@ from sqlalchemy import select from sqlalchemy.orm import Session -from .. import rbac +from .. import audit, rbac from ..db import get_db from ..models import Job from ..rbac import require_role @@ -92,6 +92,81 @@ def job_artifact(pid: str, job_id: str, db: Session = Depends(get_db), headers={"Content-Disposition": f'inline; filename="{fname}"'}) +# R24-REPORTS-BY-MOMENT — "scheduled AND SHARED, not just downloaded" is the entry's own remainder, +# and the two halves have different blockers. SHARED is unblocked and lands here: the mailer already +# ships (stdlib smtplib, a Settings "Test connection" button, a digest route that sends real mail), +# it just had no way to carry a file. SCHEDULED is not here on purpose — it needs a recurring-trigger +# record AND a runner, and there is no scheduler of any kind in this tree (no APScheduler, croniter +# or cron), so choosing one is a deployment decision rather than a wiring task. +_DELIVER_MAX_BYTES = 15 * 1024 * 1024 +_DELIVER_MAX_RECIPIENTS = 25 + + +@router.post("/projects/{pid}/jobs/{job_id}/deliver") +def deliver_artifact(pid: str, job_id: str, to: list[str] = Body(..., embed=True), + note: str = Body("", embed=True), db: Session = Depends(get_db), + user: str = Depends(require_role("editor"))): + """Email a finished job's artifact to named recipients — the "shared, not just downloaded" half. + + Mirrors `job_artifact` exactly on lookup and refusal (404 wrong project, 409 while queued/running, + 404 when the job produced no artifact), because a caller should not have to learn two different + answers to "is this artifact ready". Delivery then adds two refusals of its own: an empty + recipient list is 422 rather than a silent no-op, and an artifact over 15 MB is 413 rather than a + per-recipient "error" from a server that would have rejected it anyway. + + Returns a per-recipient status map (`sent` / `disabled` / `error`) in the same shape as the + notification digest, so an unconfigured deployment reports `disabled` instead of failing. + """ + from .. import mailer, storage + j = db.get(Job, job_id) + if j is None or j.project_id != pid: + raise HTTPException(404, "job not found") + if j.state in ("queued", "running"): + raise HTTPException(409, f"job is {j.state} — poll until done") + res = j.result or {} + key = res.get("artifact_key") if isinstance(res, dict) else None + if j.state != "done" or not key or not storage.exists(key): + raise HTTPException(404, "job has no artifact" + (f" (state {j.state}: {j.error})" if j.error else "")) + # Normalise, de-duplicate (case-insensitively — SMTP domains are not case-sensitive and the + # local part is not worth guessing at), and preserve the caller's order so the response reads + # the way the request was written. `dict.fromkeys` does both in one pass. + addrs = list(dict.fromkeys(a.strip() for a in to if isinstance(a, str) and a.strip()).keys()) + seen: set[str] = set() + addrs = [a for a in addrs if not (a.lower() in seen or seen.add(a.lower()))] + if not addrs: + raise HTTPException(422, "at least one recipient is required") + # Each address is a SYNCHRONOUS SMTP conversation with a 15-second timeout, so an unbounded + # list is a request that occupies a worker for hours. The cap is a refusal, not a silent trim: + # quietly dropping recipients is the failure the 422 above exists to avoid, one level up. + if len(addrs) > _DELIVER_MAX_RECIPIENTS: + raise HTTPException(422, f"at most {_DELIVER_MAX_RECIPIENTS} recipients per delivery " + f"({len(addrs)} given)") + + # Size BEFORE read. `storage.get` materialises the whole object, and an artifact job can park a + # large geometry export, so checking `len(data)` afterwards means the memory has already been + # spent on exactly the payload being refused — and concurrent callers multiply it. + nbytes = storage.size(key) + if nbytes > _DELIVER_MAX_BYTES: + raise HTTPException(413, f"artifact is {nbytes} bytes; the delivery cap is {_DELIVER_MAX_BYTES}") + data = storage.get(key) + fname = res.get("filename") or "artifact.bin" + subject = f"{j.kind.replace('_', ' ')}: {fname}" + body = (f"{user} sent you {fname} from project {pid}.\n\n" + + (note.strip() + "\n\n" if note.strip() else "") + + f"Generated by job {job_id} ({j.kind}).\n") + att = [(fname, data, res.get("media_type") or "application/octet-stream")] + results: dict[str, list[str]] = {} + for addr in addrs: + results.setdefault(mailer.send_email(addr, subject, body, None, att), []).append(addr) + audit.record(db, action="job.artifact.deliver", actor=user, method="POST", + path=f"/projects/{pid}/jobs/{job_id}/deliver", + detail={"kind": j.kind, "filename": fname, "bytes": len(data), + "recipients": len(addrs)}) + db.commit() + return {"smtp_configured": mailer.smtp_configured(), "filename": fname, + "bytes": len(data), "results": results} + + @router.get("/projects/{pid}/jobs") def list_jobs(pid: str, limit: int = 50, db: Session = Depends(get_db), _: str = Depends(require_role("viewer"))): diff --git a/services/api/test_artifact_deliver.py b/services/api/test_artifact_deliver.py new file mode 100644 index 000000000..23e5fb183 --- /dev/null +++ b/services/api/test_artifact_deliver.py @@ -0,0 +1,266 @@ +"""R24-REPORTS-BY-MOMENT — "shared, not just downloaded": mail a finished job's artifact. + +The entry's remainder reads "scheduled and shared, not just downloaded", and the two halves have +DIFFERENT blockers. Its own wording says this "still wants a delivery surface and SMTP" — both of +which already ship: `mailer.py` sends real mail and `POST .../notifications/digest` is a working +assemble-then-send surface. What was actually missing was smaller and more specific: the mailer had +no way to carry a FILE. That is what this covers. + +The SCHEDULED half is deliberately not here: it needs a recurring-trigger record and a runner, and +this tree has no scheduler of any kind, so picking one is a deployment decision. + +Run: PYTHONPATH=src ./.venv/bin/python test_artifact_deliver.py""" +import os + +os.environ["DATABASE_URL"] = "sqlite:///./test_artifact_deliver.db" +# setdefault, not assignment: run_tests.py assigns STORAGE_DIR=./_storage_{test} and sweeps exactly +# that path afterwards. Overwriting it sent this test's 15 MiB blob to a directory the runner does +# not own, which is what the suite footer means by "dir(s) this runner does not own". +os.environ.setdefault("STORAGE_DIR", "./_storage_test_artifact_deliver") +os.environ.pop("AEC_RBAC", None) +os.environ.pop("AEC_SMTP_HOST", None) # unconfigured: sends must report "disabled", not fail +for _f in ("./test_artifact_deliver.db",): + if os.path.exists(_f): + os.remove(_f) + +from fastapi.testclient import TestClient # noqa: E402 + +from aec_api import mailer, storage # noqa: E402 +from aec_api.db import SessionLocal # noqa: E402 +from aec_api.main import app # noqa: E402 +from aec_api.models import AuditLog, Job # noqa: E402 + +# --- the pure half first: build_message must actually carry the bytes --------------------------- +# `build_message` is documented as pure and testable, so the attachment shape is checked without +# any SMTP at all. The ORDER matters and is the reason this is asserted rather than eyeballed: +# add_alternative has to run before add_attachment, or the html body lands inside the mixed part +# and mail clients render the attachment where the message should be. +msg = mailer.build_message("a@example.com", "Subj", "plain body", "

html body

", + [("pack.pdf", b"%PDF-1.4 fake", "application/pdf")]) +atts = list(msg.iter_attachments()) +assert len(atts) == 1, [p.get_content_type() for p in msg.walk()] +assert atts[0].get_filename() == "pack.pdf", atts[0].get_filename() +assert atts[0].get_content_type() == "application/pdf", atts[0].get_content_type() +assert atts[0].get_payload(decode=True) == b"%PDF-1.4 fake" +body = msg.get_body(preferencelist=("html",)) +assert body is not None and "html body" in body.get_content(), "the html body must survive attaching" + +# a message with no attachments must be byte-identical in shape to before — the parameter is +# additive, and an existing digest send must not silently become multipart/mixed. +plain = mailer.build_message("a@example.com", "S", "t") +assert not list(plain.iter_attachments()), "no attachments must mean no mixed part" + +with TestClient(app) as c: + pid = c.post("/projects", json={"name": "Deliver P"}).json()["id"] + + # --- a finished artifact job, built the way the job runner leaves one ---------------------- + key = f"{pid}/jobs/deadbeef-owner-monthly.pdf" + storage.put(key, b"%PDF-1.4 owner monthly package") + with SessionLocal() as s: + s.add(Job(id="job-done", project_id=pid, kind="report_package", state="done", + params={}, result={"artifact_key": key, "media_type": "application/pdf", + "filename": "owner-monthly.pdf", + "bytes": 30, "reports": ["r1"]})) + s.add(Job(id="job-running", project_id=pid, kind="report_package", state="running", + params={})) + s.add(Job(id="job-noart", project_id=pid, kind="report_package", state="done", + params={}, result={})) + s.commit() + + # --- the delivery itself: unconfigured SMTP reports "disabled", it does not 500 ------------- + # This is the shape the digest route already returns, on purpose: an operator who has not set + # AEC_SMTP_HOST gets a truthful per-recipient status rather than an error that reads like a bug. + r = c.post(f"/projects/{pid}/jobs/job-done/deliver", + json={"to": ["owner@example.com", "lender@example.com"], "note": "Draw 7 pack."}) + assert r.status_code == 200, (r.status_code, r.text) + out = r.json() + assert out["smtp_configured"] is False, out + assert out["filename"] == "owner-monthly.pdf", out + assert out["bytes"] == len(b"%PDF-1.4 owner monthly package"), out + assert sorted(out["results"]["disabled"]) == ["lender@example.com", "owner@example.com"], out + + # --- it is audited: who sent what to how many people --------------------------------------- + # A file leaving the system is exactly the event an audit log exists for. + with SessionLocal() as s: + ev = [a for a in s.query(AuditLog).all() if a.action == "job.artifact.deliver"] + assert len(ev) == 1, [(a.action) for a in ev] + assert ev[0].detail["recipients"] == 2, ev[0].detail + assert ev[0].detail["filename"] == "owner-monthly.pdf", ev[0].detail + assert ev[0].detail["bytes"] == len(b"%PDF-1.4 owner monthly package"), ev[0].detail + + # --- refusals: same answers as the download route, plus delivery's own two ----------------- + # Mirroring job_artifact matters — a caller should not learn two different answers to + # "is this artifact ready". + assert c.post(f"/projects/{pid}/jobs/nope/deliver", + json={"to": ["a@example.com"]}).status_code == 404 + assert c.post(f"/projects/{pid}/jobs/job-running/deliver", + json={"to": ["a@example.com"]}).status_code == 409 + assert c.post(f"/projects/{pid}/jobs/job-noart/deliver", + json={"to": ["a@example.com"]}).status_code == 404 + + # an empty recipient list is a refusal, not a silent success — otherwise a UI bug that drops + # the address field reports "sent" and the pack goes nowhere. + for empty in ([], ["", " "]): + r = c.post(f"/projects/{pid}/jobs/job-done/deliver", json={"to": empty}) + assert r.status_code == 422, (empty, r.status_code, r.text) + + # oversize is refused up front rather than as a per-recipient "error" from a server that + # would have bounced it anyway. + big = f"{pid}/jobs/big.pdf" + storage.put(big, b"x" * (15 * 1024 * 1024 + 1)) + with SessionLocal() as s: + s.add(Job(id="job-big", project_id=pid, kind="report_package", state="done", params={}, + result={"artifact_key": big, "media_type": "application/pdf", + "filename": "big.pdf"})) + s.commit() + r = c.post(f"/projects/{pid}/jobs/job-big/deliver", json={"to": ["a@example.com"]}) + assert r.status_code == 413, (r.status_code, r.text) + + # --- a malformed recipient is that recipient's error, not everyone's ----------------------- + # `EmailMessage` rejects a header value containing CR/LF with ValueError. `send_email` is + # documented to NEVER raise; built outside its try block it did, which aborted the delivery loop + # after earlier recipients had already been served and before the audit row was written — so the + # audit disagreed with what actually happened. The bad address must degrade to "error" alone. + assert mailer.send_email("bad@example.com\r\nBcc: injected@example.com", "S", "b") == "error" + r = c.post(f"/projects/{pid}/jobs/job-done/deliver", + json={"to": ["good@example.com", "bad@example.com\r\nBcc: x@example.com"]}) + assert r.status_code == 200, (r.status_code, r.text) + res = r.json()["results"] + assert res.get("disabled") == ["good@example.com"], res + assert res.get("error") == ["bad@example.com\r\nBcc: x@example.com"], res + + # --- recipients are de-duplicated, case-insensitively -------------------------------------- + # Every retained address is a synchronous SMTP conversation, so a duplicate is not merely untidy. + r = c.post(f"/projects/{pid}/jobs/job-done/deliver", + json={"to": ["a@example.com", "A@Example.com", " a@example.com "]}) + assert r.status_code == 200, (r.status_code, r.text) + assert r.json()["results"]["disabled"] == ["a@example.com"], r.json()["results"] + + # --- the recipient cap REFUSES, it does not silently trim ---------------------------------- + # Trimming would be the same silent-success failure the empty-list 422 exists to prevent. + many = [f"u{i}@example.com" for i in range(26)] + r = c.post(f"/projects/{pid}/jobs/job-done/deliver", json={"to": many}) + assert r.status_code == 422, (r.status_code, r.text) + assert "25" in r.text, r.text + + # --- oversize is refused from the STORED SIZE, without materialising the object ------------- + # storage.get() pulls the whole artifact into memory; checking len() afterwards spends exactly + # the memory being refused. Patching get() to explode proves the refusal happens before it. + _boom = storage.get + storage.get = lambda k: (_ for _ in ()).throw(AssertionError(f"materialised {k}")) + try: + r = c.post(f"/projects/{pid}/jobs/job-big/deliver", json={"to": ["a@example.com"]}) + assert r.status_code == 413, (r.status_code, r.text) + finally: + storage.get = _boom + + # a job in ANOTHER project is not reachable through this project's path. + pid2 = c.post("/projects", json={"name": "Other"}).json()["id"] + assert c.post(f"/projects/{pid2}/jobs/job-done/deliver", + json={"to": ["a@example.com"]}).status_code == 404 + +# --- STARTTLS must present a VERIFYING context --------------------------------------------------- +# `starttls()` with no argument uses `ssl._create_stdlib_context()`, which on this interpreter +# reports verify_mode=CERT_NONE and check_hostname=False — the artifact and the SMTP password go up +# with no certificate check. Asserted through a fake SMTP rather than by reading the source, so the +# test measures what is passed at the call, not what the file appears to say. +import ssl # noqa: E402 + + +class _FakeSMTP: + captured: list = [] + + def __init__(self, host, port, timeout=None): + pass + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + def starttls(self, context=None): + _FakeSMTP.captured.append(context) + + def login(self, u, p): + pass + + def send_message(self, m): + pass + + +_real_smtp, _real_get = mailer.smtplib.SMTP, mailer.settings_store.get +mailer.smtplib.SMTP = _FakeSMTP +mailer.settings_store.get = lambda k, d=None: {"AEC_SMTP_HOST": "smtp.example.com", + "AEC_SMTP_PORT": "587", + "AEC_SMTP_TLS": "1"}.get(k, d) +try: + assert mailer.send_email("a@example.com", "S", "b") == "sent" +finally: + mailer.smtplib.SMTP, mailer.settings_store.get = _real_smtp, _real_get + +assert len(_FakeSMTP.captured) == 1, _FakeSMTP.captured +_ctx = _FakeSMTP.captured[0] +assert _ctx is not None, "starttls() was called with no context — that context does NOT verify" +assert _ctx.verify_mode == ssl.CERT_REQUIRED, _ctx.verify_mode +assert _ctx.check_hostname is True, _ctx.check_hostname + +# --- a mistyped port is a status, not an escaped exception --------------------------------------- +# The first fix moved build_message inside the boundary and left `int(AEC_SMTP_PORT)` outside it — +# the same defect class, one line above the guard. Settings are stored as arbitrary strings +# (settings_store.set_value(db, k, str(v)), no numeric validation), so a typo in the Settings form +# raised ValueError straight through a function documented never to raise, aborting the delivery +# loop before its audit row exactly as the CR/LF recipient did. +_real_get = mailer.settings_store.get +mailer.settings_store.get = lambda k, d=None: {"AEC_SMTP_HOST": "h", + "AEC_SMTP_PORT": "not-a-number"}.get(k, d) +try: + assert mailer.send_email("a@example.com", "S", "b") == "error" +finally: + mailer.settings_store.get = _real_get + +# --- an attacker-influenced recipient cannot forge log lines (CWE-117) -------------------------- +# `%s` writes a literal CR/LF into the stream, so a recipient can append whatever it likes as a +# separate, plausible-looking log record. `%r` escapes it. +import io as _io # noqa: E402 +import logging as _logging # noqa: E402 + +_buf = _io.StringIO() +_h = _logging.StreamHandler(_buf) +_ml = _logging.getLogger("aec.mail") +_saved, _prop = _ml.handlers[:], _ml.propagate +_ml.handlers[:] = [_h] +_ml.propagate = False +try: + mailer.send_email("v@x.test\r\nFAKE: forged log line", "S", "b") +finally: + _ml.handlers[:], _ml.propagate = _saved, _prop +_out = _buf.getvalue() +assert "FAKE: forged log line" in _out, _out # the value is still reported... +assert "\nFAKE: forged log line" not in _out, repr(_out) # ...but never as its own line + +# --- cleartext SMTP auth is allowed but never silent ------------------------------------------- +# AEC_SMTP_TLS=0 is a documented deployment choice (a local or trusted-network relay), so this is a +# warning rather than a refusal — but sending a credential unprotected without telling anyone is +# what would be indefensible. +_FakeSMTP.captured.clear() +_buf2 = _io.StringIO() +_h2 = _logging.StreamHandler(_buf2) +_saved, _prop = _ml.handlers[:], _ml.propagate +_ml.handlers[:] = [_h2] +_ml.propagate = False +_real_smtp, _real_get = mailer.smtplib.SMTP, mailer.settings_store.get +mailer.smtplib.SMTP = _FakeSMTP +mailer.settings_store.get = lambda k, d=None: {"AEC_SMTP_HOST": "h", "AEC_SMTP_PORT": "587", + "AEC_SMTP_TLS": "0", "AEC_SMTP_USER": "u", + "AEC_SMTP_PASSWORD": "hunter2-secret"}.get(k, d) +try: + assert mailer.send_email("a@example.com", "S", "b") == "sent" # still allowed +finally: + mailer.smtplib.SMTP, mailer.settings_store.get = _real_smtp, _real_get + _ml.handlers[:], _ml.propagate = _saved, _prop +assert "cleartext" in _buf2.getvalue(), _buf2.getvalue() +assert "hunter2-secret" not in _buf2.getvalue(), "the password must never be logged" +assert not _FakeSMTP.captured, "starttls must not run when TLS is off" + +print("test_artifact_deliver OK")