fix(email): detect and surface webhook send failures instead of claiming success
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
Aegis CI / lint-and-test (push) Has been cancelled
Snyk Security Scan / Python vulnerabilities (backend) (push) Has been cancelled
Snyk Security Scan / npm vulnerabilities (frontend) (push) Has been cancelled
Snyk Security Scan / Docker image vulnerabilities (backend) (push) Has been cancelled
send_webhook_email() only returned False on a network exception — a webhook that responded with a non-2xx status (bad URL, wrong API key, misconfigured Power Automate flow) was still reported as delivered. Now checks response.ok and returns False on rejection too. send_password_setup_email() also never checked the return value at all, so the admin's 'Send Email' action (including resending a password-reset email after the user already set their password) always showed success even when nothing was actually sent. Now raises a clear error instead.
This commit is contained in:
@@ -65,7 +65,11 @@ def send_password_setup_email(db: Session, user: User) -> None:
|
||||
f"{set_password_url}\n\n"
|
||||
"This link expires in 24 hours and can only be used once."
|
||||
)
|
||||
send_webhook_email(db, to=user.email, subject=subject, message=message, full_name=user.full_name)
|
||||
sent = send_webhook_email(db, to=user.email, subject=subject, message=message, full_name=user.full_name)
|
||||
if not sent:
|
||||
raise BusinessRuleViolation(
|
||||
"Failed to send the email — check the webhook URL/API key in Settings and the server logs."
|
||||
)
|
||||
|
||||
|
||||
def _get_valid_token_or_raise(db: Session, token: str) -> PasswordSetupToken:
|
||||
|
||||
@@ -159,8 +159,9 @@ def send_webhook_email(
|
||||
"""POST a notification email to the configured webhook.
|
||||
|
||||
Best-effort: returns False (and logs a warning) if no webhook is
|
||||
configured, *to* is empty, or the request fails — never raises, since
|
||||
a webhook outage must not break whatever triggered the notification.
|
||||
configured, *to* is empty, the request fails, or the webhook responds
|
||||
with a non-2xx status — never raises, since a webhook outage must not
|
||||
break whatever triggered the notification.
|
||||
"""
|
||||
if not to:
|
||||
return False
|
||||
@@ -174,12 +175,18 @@ def send_webhook_email(
|
||||
headers = {"x-api-key": api_key} if api_key else {}
|
||||
|
||||
try:
|
||||
requests.post(
|
||||
response = requests.post(
|
||||
webhook_url,
|
||||
json={"to": to, "subject": subject, "body": build_email_body(full_name, message)},
|
||||
headers=headers,
|
||||
timeout=10,
|
||||
)
|
||||
if not response.ok:
|
||||
logger.warning(
|
||||
"Email webhook rejected the request for %s: HTTP %d — %s",
|
||||
to, response.status_code, response.text[:500],
|
||||
)
|
||||
return False
|
||||
return True
|
||||
except requests.RequestException:
|
||||
logger.warning("Email webhook call failed for %s", to, exc_info=True)
|
||||
|
||||
@@ -84,6 +84,57 @@ def test_send_password_email_posts_to_webhook_and_issues_token(api, db, auth_hea
|
||||
assert token_row.used_at is None
|
||||
|
||||
|
||||
def test_send_password_email_fails_loudly_when_webhook_rejects(api, auth_headers, new_user_id):
|
||||
"""A webhook that's reachable but rejects the request (bad URL/API key,
|
||||
wrong Power Automate config, etc.) must not be reported as a success —
|
||||
previously the HTTP response status was never checked."""
|
||||
api(
|
||||
"patch", "/api/v1/system/email-webhook-config", auth_headers,
|
||||
json={"url": "https://example.com/power-automate-hook"},
|
||||
)
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
fake_response = MagicMock(ok=False, status_code=404, text="not found")
|
||||
with patch("app.services.webhook_email_service.requests.post", return_value=fake_response):
|
||||
resp = api("post", f"/api/v1/users/{new_user_id}/send-password-email", auth_headers)
|
||||
assert resp.status_code == 400
|
||||
assert "failed to send" in resp.text.lower()
|
||||
|
||||
|
||||
def test_send_password_email_resend_after_password_set_is_a_reset_email(api, db, auth_headers, new_user_id):
|
||||
"""Once a user has already set their password, hitting 'Send Email'
|
||||
again must still actually send — as a password-reset email — not
|
||||
silently no-op."""
|
||||
api(
|
||||
"patch", "/api/v1/system/email-webhook-config", auth_headers,
|
||||
json={"url": "https://example.com/power-automate-hook"},
|
||||
)
|
||||
|
||||
with patch("app.services.webhook_email_service.requests.post") as mock_post:
|
||||
resp = api("post", f"/api/v1/users/{new_user_id}/send-password-email", auth_headers)
|
||||
assert resp.status_code == 200, resp.text
|
||||
first_payload = mock_post.call_args.kwargs["json"]
|
||||
assert first_payload["subject"] == "Set Your Password"
|
||||
|
||||
from app.models.password_setup_token import PasswordSetupToken
|
||||
token_row = db.query(PasswordSetupToken).filter(
|
||||
PasswordSetupToken.user_id == uuid.UUID(new_user_id),
|
||||
).first()
|
||||
api(
|
||||
"post", "/api/v1/auth/set-password", {},
|
||||
json={"token": token_row.token, "new_password": "BrandNewPass123!@#"},
|
||||
)
|
||||
|
||||
# Now the user has must_change_password=False — resending must still
|
||||
# go through, this time as a reset email.
|
||||
with patch("app.services.webhook_email_service.requests.post") as mock_post_2:
|
||||
resp2 = api("post", f"/api/v1/users/{new_user_id}/send-password-email", auth_headers)
|
||||
assert resp2.status_code == 200, resp2.text
|
||||
mock_post_2.assert_called_once()
|
||||
second_payload = mock_post_2.call_args.kwargs["json"]
|
||||
assert second_payload["subject"] == "Reset Your Password"
|
||||
|
||||
def test_set_password_with_valid_token_succeeds(api, db, auth_headers, new_user_id):
|
||||
api(
|
||||
"patch", "/api/v1/system/email-webhook-config", auth_headers,
|
||||
|
||||
Reference in New Issue
Block a user