diff --git a/.env.example b/.env.example index 634ca2b0..40250d38 100644 --- a/.env.example +++ b/.env.example @@ -48,10 +48,9 @@ PUSHOVER_TOKEN= # Comma-separated to notify multiple users/groups, e.g. , PUSHOVER_USER_KEY= -# ── hCaptcha / Rapid API ─────────────────────────────────────────────────────── +# ── hCaptcha / UserCheck ─────────────────────────────────────────────────────── HCAPTCHA_SECRET_KEY= -RAPID_API_KEY= -RAPID_API_HOST= +USERCHECK_API_KEY= # ── OpenCollective donor sync ────────────────────────────────────────────────── OPENCOLLECTIVE_API_KEY= diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 81df6d9f..6f707919 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,8 +69,7 @@ jobs: PUSHOVER_TOKEN: "dummy" PUSHOVER_USER_KEY: "dummy" HCAPTCHA_SECRET_KEY: "dummy" - RAPID_API_KEY: "dummy" - RAPID_API_HOST: "dummy" + USERCHECK_API_KEY: "dummy" OPENCOLLECTIVE_API_KEY: "dummy" STATIC_ROOT: "/tmp/static" MEDIA_ROOT: "/tmp/media" diff --git a/locale/it/LC_MESSAGES/django.po b/locale/it/LC_MESSAGES/django.po index fbebbc32..fe138715 100644 --- a/locale/it/LC_MESSAGES/django.po +++ b/locale/it/LC_MESSAGES/django.po @@ -6515,9 +6515,22 @@ msgstr "L'email esiste già" msgid "Temporary email addresses are not allowed." msgstr "Gli indirizzi email temporanei non sono ammessi." -#: users/forms.py:61 -msgid "Error creating account. Contact the site administrator." -msgstr "Errore nella creazione dell'account. Contatta l'amministratore del sito." +#: users/forms.py +msgid "Email forwarding addresses are not allowed." +msgstr "Gli indirizzi email di inoltro non sono ammessi." + +#: users/forms.py +#, python-format +msgid "Did you mean %(suggestion)s?" +msgstr "Intendevi %(suggestion)s?" + +#: users/forms.py +msgid "" +"We couldn't verify your email address right now. Please try again in a " +"moment." +msgstr "" +"Al momento non è stato possibile verificare il tuo indirizzo email. Riprova " +"tra qualche istante." #: users/forms.py:78 msgid "Required. Enter a valid email address." @@ -6593,6 +6606,10 @@ msgstr "" "Al momento non è stato possibile verificare il captcha. Riprova tra qualche " "minuto." +#: users/views.py +msgid "Please complete the captcha and try again." +msgstr "Completa il captcha e riprova." + #: users/login_throttle.py msgid "Too many failed attempts. Please try again in 15 minutes." msgstr "Troppi tentativi non riusciti. Riprova tra 15 minuti." diff --git a/metron.env.example b/metron.env.example index 1bea6524..6d3725db 100644 --- a/metron.env.example +++ b/metron.env.example @@ -46,10 +46,9 @@ EMAIL_FROM= PUSHOVER_TOKEN= PUSHOVER_USER_KEY= -# ── hCaptcha / Rapid API ─────────────────────────────────────────────────────── +# ── hCaptcha / UserCheck ─────────────────────────────────────────────────────── HCAPTCHA_SECRET_KEY= -RAPID_API_KEY= -RAPID_API_HOST= +USERCHECK_API_KEY= # ── OpenCollective donor sync ────────────────────────────────────────────────── OPENCOLLECTIVE_API_KEY= diff --git a/metron/settings.py b/metron/settings.py index d8444c34..2f5aa537 100644 --- a/metron/settings.py +++ b/metron/settings.py @@ -368,8 +368,8 @@ } HCAPTCHA_SECRET_KEY = config("HCAPTCHA_SECRET_KEY") -RAPID_API_KEY = config("RAPID_API_KEY") -RAPID_API_HOST = config("RAPID_API_HOST") +# Optional: without a key, signups are checked against the local blocklists only. +USERCHECK_API_KEY = config("USERCHECK_API_KEY", default="") OPENCOLLECTIVE_API_KEY = config("OPENCOLLECTIVE_API_KEY") OPENCOLLECTIVE_SLUG = config("OPENCOLLECTIVE_SLUG", default="metron") diff --git a/pyproject.toml b/pyproject.toml index dc00dc86..41e5d69b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,6 +6,7 @@ dependencies = [ "bleach[css]", "boto3", "chartkick", + "disposable-email-domains>=0.0.281", "django", "django-countries", "django-filter", diff --git a/tests/conftest.py b/tests/conftest.py index f66ecaa3..3aebd54f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -57,6 +57,31 @@ def isolated_login_throttle(): yield +CLEAN_EMAIL_VERDICT = { + "disposable": False, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, +} + + +@pytest.fixture(autouse=True) +def isolated_usercheck(): + """Keep tests off the real UserCheck API, and give each test its own verdict + cache, alert flag and credit status in the shared Redis cache. Forms see a clean verdict for + any address not caught by the local blocklists; tests that need another verdict + re-patch users.forms.check_email_domain, and the inner patch takes precedence.""" + suffix = uuid.uuid4().hex + with ( + patch("users.utils.CACHE_PREFIX", f"usercheck:test:{suffix}"), + patch("users.utils.ALERT_KEY", f"usercheck:alerted:test:{suffix}"), + patch("users.utils.CREDITS_KEY", f"usercheck:has_credits:test:{suffix}"), + patch("users.forms.check_email_domain", return_value=CLEAN_EMAIL_VERDICT), + ): + yield + + @pytest.fixture(autouse=True) def isolated_api_throttle(): """Give each test its own DRF throttle history. It's keyed by user pk in the diff --git a/tests/users/test_email_change.py b/tests/users/test_email_change.py index e2b98287..91c41e56 100644 --- a/tests/users/test_email_change.py +++ b/tests/users/test_email_change.py @@ -15,7 +15,7 @@ HTML_OK_CODE = 200 HTTP_REDIRECT_FOUND_CODE = 302 -NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the RapidAPI check. +NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the UserCheck lookup. def _profile_data(user, **overrides): @@ -138,15 +138,32 @@ def test_rejects_disposable_address(self, auto_login_user, test_password): def test_runs_domain_check_for_unlisted_domains(self, auto_login_user, test_password): client, user = auto_login_user() - blocked = {"block": True, "disposable": True} + blocked = { + "disposable": True, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, + } with patch("users.forms.check_email_domain", return_value=blocked) as mock_check: resp = client.post( reverse("change_profile"), _profile_data(user, email="x@example.org", current_password=test_password), ) - mock_check.assert_called_once_with("x@example.org") + mock_check.assert_called_once_with("x@example.org", "example.org") assert resp.context["form"].errors["email"] + @pytest.mark.parametrize("password", ["", "not-my-password"]) + def test_lookup_waits_for_correct_password(self, auto_login_user, password): + client, user = auto_login_user() + with patch("users.forms.check_email_domain") as mock_check: + resp = client.post( + reverse("change_profile"), + _profile_data(user, email="x@example.org", current_password=password), + ) + mock_check.assert_not_called() + assert resp.context["form"].errors["current_password"] + def test_send_failure_saves_nothing(self, auto_login_user, test_password, test_email): client, user = auto_login_user() with patch( diff --git a/tests/users/test_email_validation.py b/tests/users/test_email_validation.py new file mode 100644 index 00000000..f8348477 --- /dev/null +++ b/tests/users/test_email_validation.py @@ -0,0 +1,103 @@ +"""Tests for users.forms.validate_email_allowed, shared by signup and email changes.""" + +from unittest.mock import patch + +import pytest +from django.core.exceptions import ValidationError + +from users.forms import validate_email_allowed + +CHECK = "users.forms.check_email_domain" +CLEAN = { + "disposable": False, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, +} + +pytestmark = pytest.mark.django_db + + +def _error(email, verdict=CLEAN): + with patch(CHECK, return_value=verdict) as mock_check: + try: + validate_email_allowed(email) + except ValidationError as exc: + return exc.messages[0], mock_check + return None, mock_check + + +@pytest.mark.parametrize( + "email", + [ + "x@mailinator.com", # disposable-email-domains package + "x@inbox.mailinator.com", # subdomain of a package-listed domain + "x@duck.com", # hand-kept blocklist + "x@Duck.COM", + ], +) +def test_local_blocklists_reject_without_lookup(email): + message, mock_check = _error(email) + assert message == "Temporary email addresses are not allowed." + mock_check.assert_not_called() + + +def test_allowlisted_domain_skips_lookup(): + message, mock_check = _error("x@Gmail.com") + assert message is None + mock_check.assert_not_called() + + +def test_clean_verdict_is_allowed(): + message, mock_check = _error("x@example.org") + assert message is None + mock_check.assert_called_once_with("x@example.org", "example.org") + + +def test_lookup_gets_normalized_domain(): + message, mock_check = _error("x@Example.ORG") + assert message is None + mock_check.assert_called_once_with("x@Example.ORG", "example.org") + + +def test_fails_open_without_verdict(): + message, _mock = _error("x@example.org", verdict=None) + assert message is None + + +@pytest.mark.parametrize( + ("overrides", "expected"), + [ + ({"disposable": True}, "Temporary email addresses are not allowed."), + ({"spam": True}, "Temporary email addresses are not allowed."), + ({"relay_domain": True}, "Email forwarding addresses are not allowed."), + ({"mx": False}, "Email address is not valid."), + ({"mx": False, "suggested_domain": "gmail.com"}, "Did you mean x@gmail.com?"), + # An unknown MX result isn't grounds to reject. + ({"mx": None}, None), + # A suggestion alone doesn't block an address that can receive mail. + ({"suggested_domain": "gmail.com"}, None), + ], +) +def test_verdict_mapping(overrides, expected): + message, _mock = _error("x@example.org", verdict={**CLEAN, **overrides}) + assert message == expected + + +def test_malformed_address_is_rejected(): + message, _mock = _error("x@example.org", verdict={"invalid": True}) + assert message == "Email address is not valid." + + +def test_rate_limited_lookup_asks_user_to_retry(): + message, _mock = _error("x@example.org", verdict={"rate_limited": True}) + assert message == ( + "We couldn't verify your email address right now. Please try again in a moment." + ) + + +def test_address_without_domain_is_rejected(): + message, mock_check = _error("no-at-sign") + assert message == "Email address is not valid." + mock_check.assert_not_called() diff --git a/tests/users/test_login_throttle.py b/tests/users/test_login_throttle.py index 572e15e2..34480073 100644 --- a/tests/users/test_login_throttle.py +++ b/tests/users/test_login_throttle.py @@ -14,7 +14,7 @@ HTML_OK_CODE = 200 HTTP_REDIRECT_FOUND_CODE = 302 LOCKED_TEXT = "Too many failed attempts" -NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the RapidAPI check. +NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the UserCheck lookup. def _login(client, username, password, ip="203.0.113.10"): diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index 9b689b9c..09495992 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -4,54 +4,310 @@ from unittest.mock import MagicMock, patch import pytest +from django.core.cache import cache -from users.utils import check_email_domain +from users.utils import CACHE_TIMEOUT, NO_MX_CACHE_TIMEOUT, check_email_domain HTTPS_CONNECTION = "users.utils.http.client.HTTPSConnection" +SLEEP = "users.utils.time.sleep" +PUSHOVER = "users.utils.send_pushover" +USERCHECK_BODY = { + "status": 200, + "email": "x@example.org", + "domain": "example.org", + "mx": True, + "disposable": False, + "public_domain": False, + "relay_domain": False, + "spam": False, + "did_you_mean": None, +} -def _mock_connection(status=200, body=None): + +@pytest.fixture(autouse=True) +def usercheck_api_key(settings): + settings.USERCHECK_API_KEY = "test-key" + + +def _mock_connection(*statuses, body=None): + """A connection whose successive responses have the given statuses (default: one 200). + + ``body`` replaces every response's body (default: a clean verdict for a 200, a + generic error otherwise). + """ conn = MagicMock() - response = conn.getresponse.return_value - response.status = status - response.reason = "OK" if status == 200 else "Error" - response.read.return_value = json.dumps(body or {"block": False}).encode() + responses = [] + for status in statuses or (200,): + response = MagicMock() + response.status = status + response.reason = "OK" if status == 200 else "Error" + if body is not None: + content = body + elif status == 200: + content = USERCHECK_BODY + else: + content = {"status": status, "error": "Error"} + response.read.return_value = json.dumps(content).encode() + responses.append(response) + conn.getresponse.side_effect = responses return conn @pytest.mark.parametrize( - ("email", "expected_query"), + ("email", "expected_path"), [ - ("someone@example.org", "domain=someone%40example.org"), - # A quoted local part may contain characters that are special in a query. - ('"a&b=c#d"@example.org', "domain=%22a%26b%3Dc%23d%22%40example.org"), - ('"first last"@example.org', "domain=%22first+last%22%40example.org"), + ("someone@example.org", "/email/someone%40example.org"), + # A quoted local part may contain characters that are special in a path. + ('"a/b?c#d"@example.org', "/email/%22a%2Fb%3Fc%23d%22%40example.org"), + ('"first last"@example.org', "/email/%22first%20last%22%40example.org"), ], ) -def test_check_email_domain_encodes_query(email, expected_query): +def test_check_email_domain_encodes_path(email, expected_path): conn = _mock_connection() with patch(HTTPS_CONNECTION, return_value=conn): - check_email_domain(email) + check_email_domain(email, "example.org") method, path = conn.request.call_args.args assert method == "GET" - assert path == f"/?{expected_query}" + assert path == expected_path + + +def test_check_email_domain_sends_bearer_token(settings): + settings.USERCHECK_API_KEY = "secret" + conn = _mock_connection() + with patch(HTTPS_CONNECTION, return_value=conn): + check_email_domain("x@example.org", "example.org") + + assert conn.request.call_args.kwargs["headers"] == {"Authorization": "Bearer secret"} -def test_check_email_domain_returns_parsed_response(): - body = {"block": True, "disposable": True} +def test_check_email_domain_returns_normalized_verdict(): + body = { + **USERCHECK_BODY, + "disposable": True, + "relay_domain": True, + "mx": False, + "did_you_mean": "x@gmail.com", + } with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): - assert check_email_domain("x@example.org") == body + assert check_email_domain("x@gmial.com", "gmial.com") == { + "disposable": True, + "spam": False, + "relay_domain": True, + "mx": False, + "suggested_domain": "gmail.com", + } + + +def test_check_email_domain_caches_verdict_per_domain(): + conn = _mock_connection() + with patch(HTTPS_CONNECTION, return_value=conn) as mock_conn: + first = check_email_domain("x@example.org", "example.org") + # Another mailbox on the same domain hits the cache. + second = check_email_domain("y@example.org", "example.org") + + assert first == second + mock_conn.assert_called_once() + + +@pytest.mark.parametrize( + ("mx", "expected_timeout"), + [(True, CACHE_TIMEOUT), (None, CACHE_TIMEOUT), (False, NO_MX_CACHE_TIMEOUT)], +) +def test_check_email_domain_caches_missing_mx_briefly(mx, expected_timeout): + body = {**USERCHECK_BODY, "mx": mx} + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)), + patch.object(cache, "set") as mock_set, + ): + check_email_domain("x@example.org", "example.org") + assert mock_set.call_args.kwargs["timeout"] == expected_timeout + + +def test_check_email_domain_does_not_cache_failures(): + with patch(HTTPS_CONNECTION, side_effect=[_mock_connection(500), _mock_connection()]): + assert check_email_domain("x@example.org", "example.org") is None + assert check_email_domain("x@example.org", "example.org") is not None + + +def test_check_email_domain_flags_malformed_address(): + body = {"status": 400, "error": "The email address is invalid."} + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(400, body=body)), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") == {"invalid": True} + mock_pushover.assert_not_called() + + +@pytest.mark.parametrize( + "body", + [ + {"status": 400, "error": "Missing or malformed path."}, + {"status": 400}, + "Bad Request", + [], + ], +) +def test_check_email_domain_fails_open_on_other_400s(body): + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(400, body=body)), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + mock_pushover.assert_called_once() + + +def test_check_email_domain_retries_once_when_rate_limited(): + with ( + patch(HTTPS_CONNECTION, side_effect=[_mock_connection(429), _mock_connection()]), + patch(SLEEP) as mock_sleep, + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is not None + + mock_sleep.assert_called_once() + mock_pushover.assert_not_called() + + +def _mock_status(remaining=None, status=200): + """A connection answering the /status endpoint with ``remaining`` credits.""" + conn = MagicMock() + response = conn.getresponse.return_value + response.status = status + response.reason = "OK" if status == 200 else "Error" + response.read.return_value = json.dumps({"usage": {"remaining": remaining}}).encode() + return conn + + +def test_check_email_domain_rejects_when_rate_limited_with_credits_left(): + status_conn = _mock_status(remaining=500) + with ( + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), status_conn], + ), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") == {"rate_limited": True} + + assert status_conn.request.call_args.args[:2] == ("GET", "/status") + mock_pushover.assert_not_called() + + +@pytest.mark.parametrize( + "status_conn", + [ + _mock_status(remaining=0), # quota exhausted + _mock_status(status=500), # /status itself failing + _mock_status(remaining="lots"), # unexpected body + ], +) +def test_check_email_domain_alerts_when_rate_limited_without_known_credits(status_conn): + with ( + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), status_conn], + ), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + + mock_pushover.assert_called_once() + + +def test_check_email_domain_alerts_when_status_lookup_errors(): + with ( + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), OSError("unreachable")], + ), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + + mock_pushover.assert_called_once() -def test_check_email_domain_returns_none_on_error_status(): - with patch(HTTPS_CONNECTION, return_value=_mock_connection(status=500)): - assert check_email_domain("x@example.org") is None +def test_check_email_domain_caches_credit_status(): + with ( + patch( + HTTPS_CONNECTION, + side_effect=[ + _mock_connection(429), + _mock_connection(429), + _mock_status(remaining=500), + # The second lookup reuses the cached credit status: no /status call. + _mock_connection(429), + _mock_connection(429), + ], + ) as mock_conn, + patch(SLEEP), + ): + assert check_email_domain("x@example.org", "example.org") == {"rate_limited": True} + assert check_email_domain("y@example.net", "example.net") == {"rate_limited": True} + + assert mock_conn.call_count == 5 + + +def test_check_email_domain_alerts_at_most_once_per_interval(): + with ( + patch(HTTPS_CONNECTION, side_effect=lambda *a, **kw: _mock_connection(500)), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + assert check_email_domain("y@example.net", "example.net") is None + + mock_pushover.assert_called_once() def test_check_email_domain_returns_none_on_network_error(): conn = _mock_connection() conn.request.side_effect = TimeoutError("timed out") - with patch(HTTPS_CONNECTION, return_value=conn): - assert check_email_domain("x@example.org") is None + with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: + assert check_email_domain("x@example.org", "example.org") is None conn.close.assert_called_once() + mock_pushover.assert_called_once() + + +def test_check_email_domain_returns_none_when_connection_fails(): + with patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")): + assert check_email_domain("x@example.org", "example.org") is None + + +def test_check_email_domain_skips_lookup_without_api_key(settings): + settings.USERCHECK_API_KEY = "" + with patch(HTTPS_CONNECTION) as mock_conn, patch(PUSHOVER) as mock_pushover: + assert check_email_domain("x@example.org", "example.org") is None + mock_conn.assert_not_called() + mock_pushover.assert_not_called() + + +def test_check_email_domain_fails_open_when_alert_fails(): + with ( + patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")), + patch(PUSHOVER, side_effect=OSError("pushover unreachable")) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + mock_pushover.assert_called_once() + + +@pytest.mark.parametrize("body", [[], None, "ok"]) +def test_check_email_domain_returns_none_on_unexpected_response_shape(body): + conn = _mock_connection() + conn.getresponse.side_effect = None + conn.getresponse.return_value.status = 200 + conn.getresponse.return_value.read.return_value = json.dumps(body).encode() + with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: + assert check_email_domain("x@example.org", "example.org") is None + mock_pushover.assert_called_once() + + +def test_check_email_domain_ignores_non_string_suggestion(): + body = {**USERCHECK_BODY, "did_you_mean": {"email": "x@gmail.com"}} + with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): + assert check_email_domain("x@example.org", "example.org")["suggested_domain"] is None diff --git a/tests/users/test_views.py b/tests/users/test_views.py index f788b750..c435939b 100644 --- a/tests/users/test_views.py +++ b/tests/users/test_views.py @@ -240,7 +240,7 @@ def test_signup_hcaptcha_unavailable_shows_error(db, client): assert resp.status_code == HTML_OK_CODE assertTemplateUsed(resp, "registration/signup.html") - assert "unable to verify the captcha" in str(resp.context["form"].non_field_errors()) + assert "unable to verify the captcha" in resp.content.decode() assert CustomUser.objects.count() == user_count_before # The outage doesn't count against the IP limit, so a retry still works. @@ -250,13 +250,59 @@ def test_signup_hcaptcha_unavailable_shows_error(db, client): assert CustomUser.objects.count() == user_count_before + 1 -def test_signup_failed_hcaptcha_still_redirects_silently(db, client): +def test_signup_failed_captcha_skips_email_lookup(db, client): + user_count_before = CustomUser.objects.count() + payload = _signup_payload("bot") + with ( + patch("users.views.get_recaptcha_auth", return_value={"success": False}), + patch("users.forms.check_email_domain") as mock_check, + ): + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + # Rendering the form must not validate it either. + content = resp.content.decode() + + mock_check.assert_not_called() + assert payload["email"] in content + assert CustomUser.objects.count() == user_count_before + + +def test_signup_hcaptcha_unavailable_skips_email_lookup(db, client): + unavailable = {"success": False, "error-codes": [HCAPTCHA_UNAVAILABLE]} + payload = _signup_payload("outage-lookup") + with ( + patch("users.views.get_recaptcha_auth", return_value=unavailable), + patch("users.forms.check_email_domain") as mock_check, + ): + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + # Rendering the form's errors must not validate it either. + content = resp.content.decode() + + mock_check.assert_not_called() + assert payload["email"] in content + + +def test_signup_invalid_form_after_captcha_shows_errors(db, client): + user_count_before = CustomUser.objects.count() + payload = {**_signup_payload("mismatch"), "password2": "something-else-entirely-2"} + with patch("users.views.get_recaptcha_auth", return_value={"success": True}): + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + + assert resp.status_code == HTML_OK_CODE + assert resp.context["form"].errors["password2"] + assert CustomUser.objects.count() == user_count_before + + +def test_signup_failed_hcaptcha_asks_to_complete_captcha(db, client): user_count_before = CustomUser.objects.count() failed = {"success": False, "error-codes": ["invalid-input-response"]} with patch("users.views.get_recaptcha_auth", return_value=failed): resp = client.post(reverse("signup"), _signup_payload("bot"), REMOTE_ADDR=_unique_ip()) - assert resp.status_code == HTTP_REDIRECT_FOUND_CODE + # Not the "activation email sent" redirect: no account was created, so a real + # user would otherwise wait for an email that never comes. + assert resp.status_code == HTML_OK_CODE + assertTemplateUsed(resp, "registration/signup.html") + assert "Please complete the captcha and try again." in resp.content.decode() assert CustomUser.objects.count() == user_count_before diff --git a/users/forms.py b/users/forms.py index 35815fcc..d6c26b74 100644 --- a/users/forms.py +++ b/users/forms.py @@ -1,6 +1,7 @@ import logging from typing import Any +from disposable_email_domains import blocklist as DISPOSABLE_DOMAINS # noqa: N812 from django.contrib.admin.forms import AdminAuthenticationForm from django.contrib.auth.forms import AuthenticationForm, UserChangeForm, UserCreationForm from django.core.exceptions import ValidationError @@ -20,7 +21,8 @@ LOGGER = logging.getLogger(__name__) -# Providers that hand out temporary email addresses. +# Providers that hand out temporary email addresses, on top of the +# disposable-email-domains package's list. EMAIL_DOMAIN_BLOCKLIST = { "cloudinbox.top", "duck.com", @@ -37,11 +39,15 @@ EMAIL_DOMAIN_ALLOWLIST = {"gmail.com", "yahoo.com", "proton.me"} -def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: +def validate_email_allowed( + email: str, exclude_pk: int | None = None, *, remote: bool = True +) -> None: """Raise ValidationError unless ``email`` may be used for an account. Shared by signup and email changes so a profile edit can't bypass the signup checks. ``exclude_pk`` skips the user changing their own address. + With ``remote=False`` only the local checks run, and the caller must call + validate_email_remotely() itself once it's ok to spend a (metered) lookup. """ others = CustomUser.objects.filter(email__iexact=email) if exclude_pk is not None: @@ -50,26 +56,68 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: LOGGER.warning("'%s' already exists", email) raise ValidationError(_("Email already exists")) - try: - _unused, domain = email.split("@") - except ValueError as exc: - LOGGER.warning("Email: %s | Error: %s", email, exc) - raise ValidationError(_("Email address is not valid.")) from exc - - if domain in EMAIL_DOMAIN_BLOCKLIST: + _local, domain = _split_email(email) + if _is_blocklisted(domain): LOGGER.warning("'%s' is a temporary email address.", email) raise ValidationError(_("Temporary email addresses are not allowed.")) - if domain not in EMAIL_DOMAIN_ALLOWLIST: - resp = check_email_domain(email) - if resp is None: - raise ValidationError(_("Error creating account. Contact the site administrator.")) - if resp["block"] is True: - if resp["disposable"] is True: - LOGGER.warning("'%s' is a temporary email address.", email) - raise ValidationError(_("Temporary email addresses are not allowed.")) - LOGGER.warning("'%s' is not a valid email address.", email) - raise ValidationError(_("Email address is not valid.")) + if remote: + validate_email_remotely(email) + + +def validate_email_remotely(email: str) -> None: + """Raise ValidationError if UserCheck says ``email`` can't be used for an account.""" + local, domain = _split_email(email) + if domain in EMAIL_DOMAIN_ALLOWLIST: + return + + verdict = check_email_domain(email, domain) + if verdict is None: + # Fail open: the address already passed the local blocklists, and + # check_email_domain has alerted us that the remote check is down. + LOGGER.warning("Allowing '%s' without a UserCheck verdict", email) + return + if verdict.get("rate_limited"): + LOGGER.warning("'%s' couldn't be checked: UserCheck is rate-limiting lookups", email) + raise ValidationError( + _("We couldn't verify your email address right now. Please try again in a moment.") + ) + if verdict.get("invalid"): + LOGGER.warning("'%s' is not a valid email address.", email) + raise ValidationError(_("Email address is not valid.")) + if verdict["disposable"] or verdict["spam"]: + LOGGER.warning("'%s' is a temporary or spam email address.", email) + raise ValidationError(_("Temporary email addresses are not allowed.")) + if verdict["relay_domain"]: + LOGGER.warning("'%s' is an email forwarding address.", email) + raise ValidationError(_("Email forwarding addresses are not allowed.")) + if verdict["mx"] is False: + LOGGER.warning("'%s' can't receive mail.", email) + if suggested := verdict["suggested_domain"]: + raise ValidationError( + _("Did you mean %(suggestion)s?"), params={"suggestion": f"{local}@{suggested}"} + ) + raise ValidationError(_("Email address is not valid.")) + + +def _split_email(email: str) -> tuple[str, str]: + """Split ``email`` into its local part and lowercased domain.""" + # Split on the last "@": a quoted local part may itself legally contain one. + local, sep, domain = email.rpartition("@") + if not sep or not local or not domain: + LOGGER.warning("'%s' has no local part or domain", email) + raise ValidationError(_("Email address is not valid.")) + return local, domain.lower() + + +def _is_blocklisted(domain: str) -> bool: + """Whether ``domain``, or any domain it's a subdomain of, is a known throwaway provider.""" + labels = domain.split(".") + for i in range(len(labels) - 1): + candidate = ".".join(labels[i:]) + if candidate in EMAIL_DOMAIN_BLOCKLIST or candidate in DISPOSABLE_DOMAINS: + return True + return False class CustomUserCreationForm(UserCreationForm): @@ -135,7 +183,8 @@ def pending_email(self) -> str | None: def clean_email(self): email = self.cleaned_data["email"] if email != self.original_email: - validate_email_allowed(email, exclude_pk=self.instance.pk) + # The UserCheck lookup waits for clean(), once the password checks out. + validate_email_allowed(email, exclude_pk=self.instance.pk, remote=False) return email def clean(self): @@ -156,6 +205,13 @@ def clean(self): "current_password", _("Enter your current password to change your email address."), ) + else: + # Only spend a (metered) UserCheck lookup on a confirmed password, so + # a logged-in session alone can't be used to burn through credits. + try: + validate_email_remotely(self.pending_email) + except ValidationError as e: + self.add_error("email", e) return cleaned_data def save(self, commit=True): diff --git a/users/utils.py b/users/utils.py index c412f62a..1be33664 100644 --- a/users/utils.py +++ b/users/utils.py @@ -2,14 +2,38 @@ import json import logging import ssl +import time import urllib.parse from django.conf import settings - -from metron.settings import RAPID_API_HOST, RAPID_API_KEY +from django.core.cache import cache LOGGER = logging.getLogger(__name__) +USERCHECK_HOST = "api.usercheck.com" +# Lookups run inside the signup request, so keep this short: a healthy lookup +# takes well under a second, and if it's slower we fail open anyway. +REQUEST_TIMEOUT = 3 +# Verdicts are about the domain, not the mailbox, so one lookup covers every +# address on it - caching them saves credits on the (metered) UserCheck plan. +CACHE_PREFIX = "usercheck:domain" +CACHE_TIMEOUT = 60 * 60 * 24 * 30 +# A missing MX record can be a passing DNS failure, and caching it for the full +# period would block every address on the domain until it expired. +NO_MX_CACHE_TIMEOUT = 60 * 60 +# The free plan allows 1 request/second, so a 429 is often just two lookups +# landing in the same second; one retry after this delay covers that. +RETRY_DELAY = 1 +ALERT_KEY = "usercheck:alerted" +ALERT_INTERVAL = 60 * 60 +# Whether the plan has credits left, from the /status endpoint - only consulted +# when lookups keep getting a 429, so a short cache is plenty. +CREDITS_KEY = "usercheck:has_credits" +CREDITS_CACHE_TIMEOUT = 60 * 10 +# The error UserCheck documents for its only 400: a malformed address. Any other +# 400 points at our request (or an API change), which mustn't block the user. +MALFORMED_EMAIL_ERROR = "The email address is invalid." + def client_ip(request): # nginx (nginx/nginx.conf) always overwrites X-Real-IP with its own @@ -20,37 +44,148 @@ def client_ip(request): return request.META.get("HTTP_X_REAL_IP") or request.META.get("REMOTE_ADDR", "unknown") -def check_email_domain(email: str): - result = None +def _alert_usercheck_failure(reason: str) -> None: + LOGGER.error("UserCheck lookup failed: %s", reason) + # cache.add only succeeds once per interval, so an outage (or running out of + # credits) sends one notification rather than one per signup attempt. + if cache.add(ALERT_KEY, True, timeout=ALERT_INTERVAL): + # This runs when the network is likely unhealthy, so Pushover may be + # failing too - that mustn't turn the caller's fail-open into a 500. + try: + send_pushover( + f"UserCheck email lookups are failing ({reason}). " + "Signups are being allowed without the remote check." + ) + except http.client.HTTPException, OSError: + LOGGER.exception("Failed to send UserCheck failure alert") + + +def _usercheck_request(path: str) -> tuple[int, str, bytes]: + conn = None try: # Without a timeout a stalled API blocks the gunicorn worker indefinitely. - conn = http.client.HTTPSConnection("mailcheck.p.rapidapi.com", timeout=10) + conn = http.client.HTTPSConnection(USERCHECK_HOST, timeout=REQUEST_TIMEOUT) + headers = {"Authorization": f"Bearer {settings.USERCHECK_API_KEY}"} + conn.request("GET", path, headers=headers) + res = conn.getresponse() + return res.status, res.reason, res.read() + finally: + if conn is not None: + conn.close() - headers = { - "X-RapidAPI-Key": RAPID_API_KEY, - "X-RapidAPI-Host": RAPID_API_HOST, - } - # Encode the address: a quoted local part may legally contain "&", "#", "=" - # or spaces, which would otherwise alter or break the query string. - query = urllib.parse.urlencode({"domain": email}) - conn.request("GET", f"/?{query}", headers=headers) +def _has_credits() -> bool | None: + """Whether the UserCheck plan has credits left, or None if that can't be told. + + The /email endpoint answers 429 both for the per-second limit and for an + exhausted monthly quota, with identical bodies; /status tells them apart. + """ + if (cached := cache.get(CREDITS_KEY)) is not None: + return cached + try: + status, reason, body = _usercheck_request("/status") + data = json.loads(body.decode("utf-8")) if status == 200 else None # noqa: PLR2004 + except (http.client.HTTPException, OSError, ValueError) as e: + LOGGER.warning("UserCheck status lookup failed: %s: %s", type(e).__name__, e) + return None + usage = data.get("usage") if isinstance(data, dict) else None + remaining = usage.get("remaining") if isinstance(usage, dict) else None + if not isinstance(remaining, int | float): + LOGGER.warning("UserCheck status lookup failed: %s %s", status, reason) + return None + has_credits = remaining > 0 + cache.set(CREDITS_KEY, has_credits, timeout=CREDITS_CACHE_TIMEOUT) + return has_credits + + +def _is_malformed_email_error(body: bytes) -> bool: + try: + data = json.loads(body.decode("utf-8")) + except ValueError: + return False + return isinstance(data, dict) and data.get("error") == MALFORMED_EMAIL_ERROR + + +def _parse_verdict(body: bytes) -> dict | None: + try: + data = json.loads(body.decode("utf-8")) + except ValueError as e: + _alert_usercheck_failure(f"invalid JSON: {e}") + return None + if not isinstance(data, dict): + _alert_usercheck_failure(f"unexpected response: {type(data).__name__}") + return None + did_you_mean = data.get("did_you_mean") + if not isinstance(did_you_mean, str): + did_you_mean = None + return { + "disposable": bool(data.get("disposable")), + "spam": bool(data.get("spam")), + "relay_domain": bool(data.get("relay_domain")), + # None means UserCheck couldn't tell, which isn't grounds to reject. + "mx": data.get("mx"), + "suggested_domain": did_you_mean.rsplit("@", 1)[-1] if did_you_mean else None, + } + + +def check_email_domain(email: str, domain: str) -> dict | None: + """Look up ``email`` with UserCheck and return a verdict for its domain. + + ``domain`` is the caller's already-normalized (lowercased) domain of ``email``, + used as the cache key so it matches what the caller checked its blocklists with. + + The verdict has ``disposable``, ``spam``, ``relay_domain``, ``mx`` and + ``suggested_domain`` keys, or is ``{"invalid": True}`` for an address UserCheck + rejects as malformed, or ``{"rate_limited": True}`` when lookups are still being + throttled after a retry although the plan has credits left. Returns None when no + answer could be had (network error, out of credits, ...), leaving the caller to + decide whether to fail open. + """ + if not settings.USERCHECK_API_KEY: + LOGGER.warning("USERCHECK_API_KEY is not set; skipping UserCheck lookup") + return None + + cache_key = f"{CACHE_PREFIX}:{domain}" + if (cached := cache.get(cache_key)) is not None: + return cached + + # Encode the address: a quoted local part may legally contain "/", "?", "#" + # or spaces, which would otherwise alter or break the path. + path = f"/email/{urllib.parse.quote(email, safe='')}" + try: + status, reason, body = _usercheck_request(path) + if status == 429: # noqa: PLR2004 + time.sleep(RETRY_DELAY) + status, reason, body = _usercheck_request(path) + except (http.client.HTTPException, OSError) as e: + _alert_usercheck_failure(f"{type(e).__name__}: {e}") + return None + + return _verdict_from_response(status, reason, body, cache_key) - res = conn.getresponse() - match res.status: - case 200: - data = res.read() - result = json.loads(data.decode("utf-8")) - case _: - LOGGER.error("Bad response from RapidAPI: %s %s", res.status, res.reason) - except http.client.HTTPException as e: - LOGGER.error("HTTP error: %s", e) - except Exception as e: # NOQA: BLE001 - LOGGER.error("An error occurred: %s", e) - finally: - conn.close() - return result +def _verdict_from_response(status: int, reason: str, body: bytes, cache_key: str) -> dict | None: + match status: + case 200: + verdict = _parse_verdict(body) + if verdict is not None: + timeout = NO_MX_CACHE_TIMEOUT if verdict["mx"] is False else CACHE_TIMEOUT + cache.set(cache_key, verdict, timeout=timeout) + return verdict + case 400 if _is_malformed_email_error(body): + # Specific to the address, so not cached. + return {"invalid": True} + case 429 if _has_credits(): + # Credits remain, so this is the per-second limit rather than an exhausted + # quota. Failing open here would let anyone who can trigger it (by flooding + # lookups) skip the check, so the caller asks the user to retry instead. + LOGGER.warning("UserCheck lookups are still rate-limited after a retry") + return {"rate_limited": True} + case _: + # Includes a 429 from an exhausted quota (or one /status couldn't explain), + # and a 400 for anything other than a malformed address. + _alert_usercheck_failure(f"{status} {reason}") + return None def send_pushover(message): @@ -59,17 +194,20 @@ def send_pushover(message): user_keys = [key.strip() for key in settings.PUSHOVER_USER_KEY.split(",") if key.strip()] for user_key in user_keys: context = ssl.create_default_context() - conn = http.client.HTTPSConnection("api.pushover.net:443", context=context) - conn.request( - "POST", - "/1/messages.json", - urllib.parse.urlencode( - { - "token": settings.PUSHOVER_TOKEN, - "user": user_key, - "message": message, - } - ), - {"Content-type": "application/x-www-form-urlencoded"}, - ) - conn.getresponse() + conn = http.client.HTTPSConnection("api.pushover.net:443", context=context, timeout=10) + try: + conn.request( + "POST", + "/1/messages.json", + urllib.parse.urlencode( + { + "token": settings.PUSHOVER_TOKEN, + "user": user_key, + "message": message, + } + ), + {"Content-type": "application/x-www-form-urlencoded"}, + ) + conn.getresponse() + finally: + conn.close() diff --git a/users/views.py b/users/views.py index 3fa760ce..fd36abdb 100644 --- a/users/views.py +++ b/users/views.py @@ -175,7 +175,65 @@ def activate(request, uidb64, token): return redirect("home") -def signup(request): # sourcery skip: extract-method +def _create_account(request, form): + """Create an inactive account from a valid signup ``form`` and email its activation link. + + Returns False, with an error added to ``form``, if the email couldn't be sent. + """ + user: CustomUser = form.save(commit=False) + user.is_active = False + user.save() + context = { + "user": user, + "uid": urlsafe_base64_encode(force_bytes(user.pk)), + "token": account_activation_token.make_token(user), + } + try: + _send_email( + request, + _("Activate Your Metron Account"), + "registration/account_activation_email", + context, + user.email, + ) + except smtplib.SMTPException, OSError: + # There's no way to resend the activation email, so don't leave + # behind an inactive account holding the username/email, and + # don't count it against the IP limit, so the user can retry. + logger.exception("Failed to send activation email to %s", user.username) + user.delete() + form.add_error( + None, + _("We were unable to send your activation email. Please try again in at a later time."), + ) + return False + _record_signup(request, user.username) + # Let's send a pushover notice that a user requested an account. + send_pushover(f"{user} signed up for an account on Metron.") + ip = client_ip(request) + logger.info( + "User signed up for an account on Metron (user=%s, ip=%s)", + user.username, + ip, + extra={"username": user.username, "ip": ip}, + ) + return True + + +def _rerender_signup(request, message): + """Show the signup page again with ``message``, keeping the username and email. + + The form is unbound: displaying a bound form's errors would validate it, which + looks the email up with UserCheck - the lookup a failed captcha mustn't reach. + """ + form = CustomUserCreationForm( + initial={field: request.POST.get(field, "") for field in ("username", "email")} + ) + messages.error(request, message) + return render(request, "registration/signup.html", {"form": form}) + + +def signup(request): settings_obj = SignupSettings.get_solo() if not settings_obj.signups_enabled: return render( @@ -189,64 +247,25 @@ def signup(request): # sourcery skip: extract-method _notify_signup_rate_limit_hit(request) return render(request, "registration/signup_rate_limited.html", status=429) - form = CustomUserCreationForm(request.POST) - if form.is_valid(): - result = get_recaptcha_auth(request) - - if HCAPTCHA_UNAVAILABLE in result.get("error-codes", []): - # An hCaptcha outage, not a failed check: tell the user to retry - # rather than falling through to "activation email sent". - form.add_error( - None, - _( - "We were unable to verify the captcha right now. " - "Please try again in a few minutes." - ), - ) - return render(request, "registration/signup.html", {"form": form}) - - if result["success"]: - user: CustomUser = form.save(commit=False) - user.is_active = False - user.save() - context = { - "user": user, - "uid": urlsafe_base64_encode(force_bytes(user.pk)), - "token": account_activation_token.make_token(user), - } - try: - _send_email( - request, - _("Activate Your Metron Account"), - "registration/account_activation_email", - context, - user.email, - ) - except smtplib.SMTPException, OSError: - # There's no way to resend the activation email, so don't leave - # behind an inactive account holding the username/email, and - # don't count it against the IP limit, so the user can retry. - logger.exception("Failed to send activation email to %s", user.username) - user.delete() - form.add_error( - None, - _( - "We were unable to send your activation email. " - "Please try again in at a later time." - ), - ) - return render(request, "registration/signup.html", {"form": form}) - _record_signup(request, user.username) - # Let's send a pushover notice that a user requested an account. - send_pushover(f"{user} signed up for an account on Metron.") - ip = client_ip(request) - logger.info( - "User signed up for an account on Metron (user=%s, ip=%s)", - user.username, - ip, - extra={"username": user.username, "ip": ip}, - ) + # Verify the captcha before validating the form: validation looks the email + # up with UserCheck, so checking first makes every (metered) lookup cost a + # solved captcha rather than just a POST. + result = get_recaptcha_auth(request) + + if HCAPTCHA_UNAVAILABLE in result.get("error-codes", []): + # An hCaptcha outage, not a failed check, so the user can just retry. + return _rerender_signup( + request, + _( + "We were unable to verify the captcha right now. " + "Please try again in a few minutes." + ), + ) + if not result["success"]: + return _rerender_signup(request, _("Please complete the captcha and try again.")) + form = CustomUserCreationForm(request.POST) + if form.is_valid() and _create_account(request, form): return redirect("account_activation_sent") else: form = CustomUserCreationForm() diff --git a/uv.lock b/uv.lock index 0bc26942..f1eb256d 100644 --- a/uv.lock +++ b/uv.lock @@ -201,6 +201,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/b5/a5/36ebf05de5ae042cc10923f8d78189364076ed5c3f58f0dfe1e639d3ef10/cssbeautifier-2.0.3-py3-none-any.whl", hash = "sha256:14650d9c8a47adcb80a7697a866a3921fe0332ef74a92e1039d09086aa0c5001", size = 122645, upload-time = "2026-06-30T15:41:10.519Z" }, ] +[[package]] +name = "disposable-email-domains" +version = "0.0.281" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/03/7e/c4c7f9672e7c2ef813d79dedc70a0b1226b124cf48f5ca420fc9475f5dd8/disposable_email_domains-0.0.281.tar.gz", hash = "sha256:9fc890a1708c17399b7125de8f67624541601918bb5f3545d14d552151f39d4b", size = 119652, upload-time = "2026-10-16T17:02:52.27Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/7b/5d/75be3e22fd06dc0cc692bde8d23ad341ca9c1b3305e9ab0e15a9252ffb66/disposable_email_domains-0.0.281-py3-none-any.whl", hash = "sha256:872cdbce38e163788cc7d699256c2a2c9ad9f5dba2a088cfc3951522377765d9", size = 61473, upload-time = "2026-10-16T17:02:50.643Z" }, +] + [[package]] name = "distlib" version = "0.4.3" @@ -668,6 +677,7 @@ dependencies = [ { name = "bleach", extra = ["css"] }, { name = "boto3" }, { name = "chartkick" }, + { name = "disposable-email-domains" }, { name = "django" }, { name = "django-countries" }, { name = "django-filter" }, @@ -714,6 +724,7 @@ requires-dist = [ { name = "bleach", extras = ["css"] }, { name = "boto3" }, { name = "chartkick" }, + { name = "disposable-email-domains", specifier = ">=0.0.281" }, { name = "django" }, { name = "django-countries" }, { name = "django-filter" },