From b4970af3b80f48d69f95ca5251ffbfd6f6c131ec Mon Sep 17 00:00:00 2001 From: Ashwin Date: Thu, 13 Aug 2026 22:23:05 +0530 Subject: [PATCH] feat: Add email factor for MFA and enhance enforcement options - Introduced an email-based one-time code factor for MFA, allowing users to receive codes via email as a fallback option. - Updated documentation to reflect the new email factor, including its usage, security considerations, and configuration settings. - Implemented enforcement settings to require MFA for specific users or views, enhancing security policies. - Added new system checks to validate the configuration of MFA requirements. - Bumped version to 4.1.0 to reflect the new features and changes. --- CHANGELOG.md | 91 +++ README.md | 18 +- django_mfa/adapters/__init__.py | 2 + django_mfa/adapters/email.py | 315 +++++++++++ django_mfa/adapters/recovery_codes.py | 4 + django_mfa/apps.py | 3 + django_mfa/checks.py | 30 + django_mfa/conf.py | 7 + django_mfa/decorators.py | 71 +++ django_mfa/events.py | 44 ++ django_mfa/middleware.py | 71 ++- django_mfa/migrations/0008_email_factor.py | 50 ++ django_mfa/models.py | 3 +- django_mfa/notifications.py | 98 ++++ django_mfa/policy.py | 86 +++ django_mfa/ratelimit.py | 50 +- django_mfa/registry.py | 51 ++ django_mfa/signals.py | 11 + django_mfa/static/django_mfa/style.css | 4 + .../django_mfa/email/factor_added.txt | 3 + .../django_mfa/email/factor_added_subject.txt | 1 + .../django_mfa/email/factor_removed.txt | 3 + .../email/factor_removed_subject.txt | 1 + .../django_mfa/email/mfa_disabled.txt | 3 + .../django_mfa/email/mfa_disabled_subject.txt | 1 + .../templates/django_mfa/email/otp_code.txt | 5 + .../django_mfa/email/otp_code_subject.txt | 1 + .../django_mfa/email/recovery_code_used.txt | 5 + .../email/recovery_code_used_subject.txt | 1 + .../templates/django_mfa/enroll_email.html | 35 ++ django_mfa/templates/django_mfa/security.html | 9 +- .../templates/django_mfa/verify_email.html | 36 ++ django_mfa/templatetags/otp_tags.py | 15 + django_mfa/tests/test_adapter_email.py | 518 ++++++++++++++++++ django_mfa/tests/test_decorators.py | 143 +++++ django_mfa/tests/test_enforcement.py | 155 ++++++ django_mfa/tests/test_events.py | 221 ++++++++ django_mfa/tests/test_migrations.py | 20 + django_mfa/tests/test_models.py | 36 +- django_mfa/tests/test_notifications.py | 154 ++++++ django_mfa/tests/test_passwordless.py | 60 ++ django_mfa/tests/test_policy.py | 110 ++++ django_mfa/tests/test_ratelimit.py | 39 ++ django_mfa/tests/test_registry.py | 45 ++ django_mfa/utils.py | 42 ++ django_mfa/views/enroll.py | 7 +- django_mfa/views/manage.py | 33 +- django_mfa/views/verify.py | 16 +- docs/api.md | 55 +- docs/contributing.md | 3 +- docs/custom_factors.md | 129 +++-- docs/customizing.md | 7 + docs/enforcement.md | 140 +++++ docs/index.md | 1 + docs/mfa_flow.md | 17 +- docs/recipes.md | 36 +- docs/security.md | 38 ++ docs/settings.md | 69 ++- docs/upgrading.md | 2 +- pyproject.toml | 2 +- uv.lock | 2 +- 61 files changed, 3089 insertions(+), 139 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 django_mfa/adapters/email.py create mode 100644 django_mfa/decorators.py create mode 100644 django_mfa/events.py create mode 100644 django_mfa/migrations/0008_email_factor.py create mode 100644 django_mfa/notifications.py create mode 100644 django_mfa/policy.py create mode 100644 django_mfa/templates/django_mfa/email/factor_added.txt create mode 100644 django_mfa/templates/django_mfa/email/factor_added_subject.txt create mode 100644 django_mfa/templates/django_mfa/email/factor_removed.txt create mode 100644 django_mfa/templates/django_mfa/email/factor_removed_subject.txt create mode 100644 django_mfa/templates/django_mfa/email/mfa_disabled.txt create mode 100644 django_mfa/templates/django_mfa/email/mfa_disabled_subject.txt create mode 100644 django_mfa/templates/django_mfa/email/otp_code.txt create mode 100644 django_mfa/templates/django_mfa/email/otp_code_subject.txt create mode 100644 django_mfa/templates/django_mfa/email/recovery_code_used.txt create mode 100644 django_mfa/templates/django_mfa/email/recovery_code_used_subject.txt create mode 100644 django_mfa/templates/django_mfa/enroll_email.html create mode 100644 django_mfa/templates/django_mfa/verify_email.html create mode 100644 django_mfa/tests/test_adapter_email.py create mode 100644 django_mfa/tests/test_decorators.py create mode 100644 django_mfa/tests/test_events.py create mode 100644 django_mfa/tests/test_notifications.py create mode 100644 django_mfa/tests/test_policy.py create mode 100644 docs/enforcement.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..e1bb9b5 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,91 @@ +# Changelog + +Notable changes to django-mfa, newest first. + +This file starts at 4.1.0. For the 2.x/3.x → 4.0 rewrite — which was a +ground-up rebuild with breaking changes to models, URLs, session keys and +settings — see [docs/upgrading.md](docs/upgrading.md); it is far more than a +changelog entry could carry. Releases before 4.1.0 are on the +[GitHub releases page](https://github.com/MicroPyramid/django-mfa/releases). + +Versions follow [PEP 440](https://peps.python.org/pep-0440/). The version in +`pyproject.toml` is the only place it is written; the git tag and the GitHub +Release are derived from it (see [docs/contributing.md](docs/contributing.md)). + +## 4.1.0 + +Three additions, all opt-in. **An install that sets none of the new settings +behaves identically to 4.0.1** — the only required step is running the new +migration. + +### Added + +- **`MFA_REQUIRED` — require a second factor.** Until now enrollment was + entirely voluntary: the middleware only challenged users who had *already* + enrolled, so anyone who never opted in was never prompted, and there was no + way to require MFA of staff. Accepts `False` (default), `True`, a callable + taking a user, or a dotted path to one; `django_mfa.policy` supplies + `is_staff` and `in_groups(*names)`. A required user holding no primary factor + is walled to the security page — only the enroll pages, recovery codes and + `MFA_EXEMPT_PATHS` stay reachable — until they enroll. See + [docs/enforcement.md](docs/enforcement.md). +- **`@mfa_required` and `MfaRequiredMixin`** (`django_mfa.decorators`) for + per-view enforcement regardless of `MFA_REQUIRED`. The setting picks users, + the decorator picks views, and neither can express the other. +- **System check `django_mfa.E004`**, rejecting an unimportable or + non-callable `MFA_REQUIRED` at `manage.py check` rather than from inside + middleware on a user's first live request. Unlike E001–E003 it is not gated + on WebAuthn being active. +- **An emailed one-time-code factor** (`"email"`) — the only built-in that does + not assume the user still holds a device they enrolled earlier, which makes + it the lost-phone path. **Not in the `MFA_FACTORS` default**: add it + explicitly, so that upgrading cannot silently acquire a factor that sends + mail through a backend this package does not control. New settings + `MFA_EMAIL_CODE_LENGTH` (6), `MFA_EMAIL_CODE_VALIDITY` (300s), + `MFA_EMAIL_SEND_RATE_LIMIT` (`"3/5m"`), `MFA_EMAIL_SUBJECT`, and + `MFA_FROM_EMAIL`. +- **Five signals** — `factor_added`, `factor_removed`, `mfa_verified`, + `mfa_verification_failed`, `recovery_code_used` — importable from + `django_mfa.signals`, always on. All sent with `send_robust()`, so a raising + receiver cannot break a security action such as removing a compromised key. + `mfa_verification_failed` also fires for attempts the rate limiter refuses: a + brute-force detector needs the refused attempts, not only the evaluated ones. + See [docs/api.md](docs/api.md). +- **`MFA_NOTIFY_ON_CHANGE`** (default `False`) — emails the user when a factor + is added or removed, when a recovery code is spent, and when their last + factor goes. Sending is synchronous and best-effort: a failure is logged, + never raised, because a mail outage must not turn "remove this key I think is + compromised" into a 500. For async delivery or non-email routing, connect + your own receiver to the signals above and leave this off — that is why the + signals ship independently of the emails. +- **`Registry.has_primary_factor(user)`** — the boolean form of + `primary_enabled_for()` in one query instead of one per registered adapter. + Both derive from `Adapter.counts_as_primary_factor`, so they cannot disagree. +- New documentation page, **Enforcement**. + +### Changed + +- `Adapter.complete_enroll()` **must return the created `Authenticator`**. This + was always true of the built-ins, but it is now a documented contract: the + `factor_added` signal carries the return value, so a custom adapter returning + `None` silently degrades every host project's audit trail for that factor + type. +- `ratelimit.parse/check/record_failure` gained a `setting=` keyword argument so + a second budget (emailed-code sends) can be counted against its own setting. + Existing positional calls are unaffected; `record_failure` is now an alias of + the more general `record`. +- `docs/custom_factors.md`'s worked example is now a printed-backup-token + factor. Its previous example was an emailed-code factor, which now ships as a + built-in, so the page had begun documenting how to reimplement something the + package provides. + +### Upgrading + +Run `manage.py migrate django_mfa`. Migration `0008_email_factor` adds the +`email` factor type and extends the `mfa_one_singleton_authenticator_per_user` +constraint to cover it — a singleton factor missing from that condition would +not actually be constrained. Unlike `0007`, it is reversible. + +Nothing else is required. `MFA_REQUIRED`, `MFA_NOTIFY_ON_CHANGE` and the +absence of `"email"` from `MFA_FACTORS`'s default are what keep existing +behaviour unchanged. diff --git a/README.md b/README.md index c2e5f6e..81521f3 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,7 @@ Django's own `user_logged_in` signal. | 🔑 **Passkeys & security keys** | WebAuthn/FIDO2 — Touch ID, Windows Hello, Face ID, YubiKey. Usable as a second factor *or* for full passwordless login, with no username typed. | | 📱 **Authenticator apps** | Standard TOTP (RFC 6238) — Google Authenticator, 1Password, Aegis, anything. QR code rendered server-side as inline SVG; no third-party service ever sees your users' secrets. | | 🧾 **Recovery codes** | Ten single-use codes, hashed at rest, shown exactly once. The answer to "I lost my phone" that isn't a support ticket. | +| ✉️ **Emailed codes** | Opt-in (`"email"` in `MFA_FACTORS`): a one-time code sent to the address on file, for a user who's lost everything else. Not in the default factor list — an existing install has to opt in. | | 🖥️ **Remember this browser** | Optional, off by default. Trust a browser for N days after one successful challenge. | | ➕ **Several keys at once** | A user can register a work laptop's Touch ID *and* a backup YubiKey, each with its own name. | @@ -116,9 +117,12 @@ built-in view, allauth, or your own SSO handler, all django-mfa needs is that `login()` gets called. A `user_logged_in` receiver marks the session pending, and the middleware takes it from there. -**Users without a second factor are never blocked.** Someone with no factor enrolled -logs in exactly as before. Enforcement applies only to users who actually have one, so -you can roll MFA out gradually instead of on a flag day. +**Users without a second factor are never blocked, unless you ask for it.** Someone +with no factor enrolled logs in exactly as before, so you can roll MFA out gradually +instead of on a flag day. Want to *require* it instead — for everyone, for staff, for +one group — set `MFA_REQUIRED`; a required user with no factor is walled to the +security page until they enroll one. See +[Enforcing MFA](http://django-mfa.readthedocs.io/en/latest/enforcement.html). **The screens are yours.** Every page extends `MFA_BASE_TEMPLATE`, so pointing that at your own base template is usually all the theming you need. Want more? Shadow any @@ -163,7 +167,7 @@ The parts that are easy to get subtly wrong, done deliberately: ### It tells you when you've misconfigured it -Three system checks run on `manage.py check` (and therefore on `migrate` and +Four system checks run on `manage.py check` (and therefore on `migrate` and `runserver`), because each one guards a failure that is otherwise *silent in production*: @@ -172,6 +176,7 @@ production*: | `django_mfa.E001` | `MFA_FIDO2_RP_ID` is unset | | `django_mfa.E002` | `MFA_FIDO2_RP_ID` doesn't match any `ALLOWED_HOSTS` entry | | `django_mfa.E003` | `WebAuthnBackend` is missing from `AUTHENTICATION_BACKENDS` | +| `django_mfa.E004` | `MFA_REQUIRED` is a dotted path that fails to import, or resolves to something that isn't callable | `E003` is the instructive one. Passwordless login calls `login()` with an explicit `backend=`, which succeeds no matter what `AUTHENTICATION_BACKENDS` says. One request @@ -196,8 +201,8 @@ class Adapter: Add `enroll_.html` and `verify_.html`, register the adapter, and it appears in the security page, the picker, and the middleware's exempt set automatically. The -three built-ins (`totp`, `webauthn`, `recovery_codes`) are written against this same -API — there's no privileged path. +four built-ins (`totp`, `webauthn`, `recovery_codes`, `email`) are written against +this same API — there's no privileged path. ## Compatibility @@ -217,6 +222,7 @@ outside the source tree. - [Getting started](http://django-mfa.readthedocs.io/en/latest/installation_setup.html) — install and wire it up in five minutes - [Settings reference](http://django-mfa.readthedocs.io/en/latest/settings.html) — every setting, its default, and what it does - [Customizing the UI](http://django-mfa.readthedocs.io/en/latest/customizing.html) — templates, context, and the WebAuthn JS contract +- [Enforcing MFA](http://django-mfa.readthedocs.io/en/latest/enforcement.html) — requiring it for some or all users, and per-view enforcement - [Integration recipes](http://django-mfa.readthedocs.io/en/latest/recipes.html) — allauth, passkey buttons, APIs, testing, troubleshooting - [Writing a custom factor](http://django-mfa.readthedocs.io/en/latest/custom_factors.html) — the Adapter API, with a worked example - [Security model](http://django-mfa.readthedocs.io/en/latest/security.html) — controls, non-goals, and a production checklist diff --git a/django_mfa/adapters/__init__.py b/django_mfa/adapters/__init__.py index 55a704e..f538a4e 100644 --- a/django_mfa/adapters/__init__.py +++ b/django_mfa/adapters/__init__.py @@ -19,6 +19,7 @@ from django_mfa.conf import settings as mfa_settings from django_mfa.registry import registry +from .email import EmailAdapter from .recovery_codes import RecoveryCodesAdapter from .totp import TOTPAdapter from .webauthn import WebAuthnAdapter @@ -29,6 +30,7 @@ "totp": TOTPAdapter, "recovery_codes": RecoveryCodesAdapter, "webauthn": WebAuthnAdapter, + "email": EmailAdapter, } diff --git a/django_mfa/adapters/email.py b/django_mfa/adapters/email.py new file mode 100644 index 0000000..73ad293 --- /dev/null +++ b/django_mfa/adapters/email.py @@ -0,0 +1,315 @@ +"""A one-time code delivered by email. + +The only factor here that does not assume the user still holds a device they +enrolled earlier, which makes it the "lost my phone" path. It is an ordinary +enrolled factor: the user opts in, confirms a code, and gets an Authenticator +row they can see and remove like any other. + +NOT in the MFA_FACTORS default. An existing install must not silently acquire +a factor that sends mail through a backend this package does not control. +""" + +import logging +import secrets +import time + +from django.conf import settings as django_settings +from django.core.mail import send_mail +from django.template.loader import render_to_string +from django.utils.crypto import salted_hmac + +from django_mfa import ratelimit +from django_mfa.conf import settings as mfa_settings +from django_mfa.models import Authenticator +from django_mfa.registry import Adapter +from django_mfa.utils import mask_email, strings_equal, user_email + +logger = logging.getLogger(__name__) + +#: Ceremony state keys. Separate for enrollment and verification for the same +#: reason webauthn.py keeps AUTH_STATE_KEY and PASSKEY_STATE_KEY apart: a code +#: issued to confirm a new address must not be spendable as a challenge +#: answer, or vice versa. +ENROLL_STATE_KEY = "django_mfa_email_enroll" +VERIFY_STATE_KEY = "django_mfa_email_verify" + +#: Rate-limit scope for *sending*, distinct from the per-factor verification +#: budget. Colon-namespaced so it cannot collide with a factor type, which is +#: a URL segment and never contains one. +SEND_SCOPE = "email:send" +SEND_SETTING = "MFA_EMAIL_SEND_RATE_LIMIT" + +#: Wrong guesses allowed against one issued *code* before that code is +#: discarded and a fresh one is required on the next begin_enroll/ +#: begin_verify call. Not zero: closing on the first typo would force that +#: next call to mint a resend for an ordinary fumble-fingered digit, instead +#: of just re-checking the same still-good code. +#: +#: This bounds brute-forcing of one issued code -- it does NOT by itself end +#: a guessing session. views/verify.py (and enroll.py) re-call begin_verify/ +#: begin_enroll to re-render the challenge page after every failed POST, and +#: once this cap pops the ceremony state, that very call sees no state left, +#: mints a fresh code, and mails it -- attempts reset to zero (see +#: _ensure_code). What actually bounds an attacker across repeated +#: rotations like that is the pair of rate limits: MFA_VERIFY_RATE_LIMIT +#: (wrong POSTs per user+factor) and MFA_EMAIL_SEND_RATE_LIMIT (fresh codes +#: minted per user) -- either one running out stops the run. Do not read +#: this constant as "the ceremony locks after N attempts"; only those two +#: settings make that true. +MAX_ATTEMPTS = 3 + + +def _issue_code(): + length = mfa_settings.MFA_EMAIL_CODE_LENGTH + return f"{secrets.randbelow(10 ** length):0{length}d}" + + +def _hash(code, salt): + """Key a code's stored digest off SECRET_KEY, so the session itself can't + be used to recover it. + + The session is not a safe place for the plaintext: under + SESSION_ENGINE="django.contrib.sessions.backends.signed_cookies" it + round-trips through the client, and signed is not encrypted -- the payload + is readable base64. But hashing alone doesn't fix that if the hash is a + plain, unkeyed digest of ``(salt, code)``: both salt and digest sit in + that same readable session, so the same client that receives them can + recompute sha256(f"{salt}:{code}") for all 10**length candidates entirely + offline, with no server round-trip to rate-limit or a validity window to + race -- a single-threaded pure-Python loop clears a 6-digit space in a + fraction of a second. MFA_EMAIL_CODE_VALIDITY and MAX_ATTEMPTS only bind + guesses made *against the server*; they do nothing once the attacker has + a local copy of the digest and can check candidates without it. + + salted_hmac() closes that: it mixes in settings.SECRET_KEY, a value the + client never sees, so the digest is unforgeable without it -- there is no + offline computation the holder of (salt, digest) alone can run. The + per-issue salt is kept anyway, as HMAC's "key_salt" input, so two codes + issued to the same user never hash identically. One consequence worth + knowing: rotating SECRET_KEY invalidates every in-flight code, since the + HMAC output changes with it -- harmless at this construction's 300s TTL, + but worth knowing before you're debugging it during a key rotation. + """ + return salted_hmac(salt, code, algorithm="sha256").hexdigest() + + +def _is_fresh(state): + return (time.time() - state.get("issued_at", 0) + < mfa_settings.MFA_EMAIL_CODE_VALIDITY) + + +class EmailAdapter(Adapter): + type = Authenticator.Type.EMAIL + verbose_name = "Emailed code" + supports_multiple = False + supports_enroll = True + counts_as_primary_factor = True + + # --- availability ------------------------------------------------------ + + def is_available(self, user): + """Offering a factor that cannot possibly deliver is worse than not + offering it, so an account with no address never sees this one. + + Gated on mask_email() rather than a bare truthiness check on + user_email() -- the same predicate begin_enroll/begin_verify use to + decide whether to show the code-entry form. A profile field holding + a string with no "@" in it (never validated by this package) used to + pass the old truthiness check, so a code was minted and mailed + against it while the template's own `{% if address %}` -- fed by + mask_email(), which returns "" for exactly that input -- refused to + render anything to type it into. One predicate, used everywhere, + keeps "deliverable" and "displayable" from disagreeing. + """ + if not mask_email(user_email(user)): + return False + return super().is_available(user) + + # --- sending ----------------------------------------------------------- + + def _send(self, user, address, code): + context = { + "code": code, + "user": user, + "validity_minutes": max( + 1, mfa_settings.MFA_EMAIL_CODE_VALIDITY // 60), + } + subject = mfa_settings.MFA_EMAIL_SUBJECT or render_to_string( + "django_mfa/email/otp_code_subject.txt", context) + # A subject with a newline in it is a header-injection vector; a + # template file almost always ends with one. + subject = " ".join(subject.split()) + body = render_to_string("django_mfa/email/otp_code.txt", context) + send_mail( + subject, body, + mfa_settings.MFA_FROM_EMAIL or django_settings.DEFAULT_FROM_EMAIL, + [address], + ) + + def _ensure_code(self, request, user, address, key): + """Make sure a live code exists for this ceremony, sending one if not. + + Called from begin_enroll/begin_verify, which run on every GET of the + page -- including a refresh. Reusing a still-valid code rather than + issuing a new one is what stops an ordinary refresh from spending + send budget (or, without a throttle, from being an email bomb aimed + at a third party). + + Past the limit, or when the mail backend itself fails to deliver, + this returns having done nothing usable: no signal to the caller + distinguishing "throttled" from "sent" from "delivery failed" -- + see begin_verify's docstring for why the page must look identical + in every one of those cases. + """ + state = request.session.get(key) + if state and _is_fresh(state) and state.get("address") == address: + return + if not ratelimit.check(user, SEND_SCOPE, setting=SEND_SETTING): + return + code = _issue_code() + salt = secrets.token_hex(8) + request.session[key] = { + "hash": _hash(code, salt), + "salt": salt, + "issued_at": time.time(), + "address": address, + "attempts": 0, + } + # The budget is spent here, before the send is even attempted, and + # stays spent even if _send() below fails: the costly thing being + # budgeted is the outbound call to the mail provider, which has + # already happened by the time _send() can raise. Not moving this + # after a successful send is what stops a persistent backend outage + # from turning into an unthrottled loop of real network calls to + # that provider on every single page load. + ratelimit.record(user, SEND_SCOPE, setting=SEND_SETTING) + try: + self._send(user, address, code) + except Exception: + # A mail-backend outage must not surface as an unhandled 500 on + # the challenge/enroll page -- and, for the same reason a + # throttled send must not reveal itself (see begin_verify's + # docstring), it must not be visibly *different* from one + # either: both end up "no usable code, identical response". + # Pop the state written above so a later complete_*() correctly + # reports "invalid" instead of validating a code that was never + # actually delivered. + # + # Not swallowed silently, though: this is the one place in the + # request that would otherwise know delivery failed, so it logs + # at ERROR -- a host project with its own error monitoring on + # `django_mfa` sees every outage even though the user never does. + logger.exception( + "django_mfa: failed to send a one-time code to user pk=%s", + user.pk) + request.session.pop(key, None) + + def _spend(self, request, key, address, submitted): + """Check ``submitted`` against the stored ceremony. True or False. + + Pops the state on success, and on the last allowed attempt against + that code (MAX_ATTEMPTS) -- so one issued code is single-use in + both directions: it cannot be replayed after it works, and it + cannot be brute-forced past MAX_ATTEMPTS guesses. See MAX_ATTEMPTS's + own comment for why that cap bounds one code, not a whole guessing + session, and for which two settings bound the session itself. + """ + state = request.session.get(key) + if not state or not _is_fresh(state) or state.get("address") != address: + request.session.pop(key, None) + return False + + if strings_equal(_hash(submitted, state["salt"]), state["hash"]): + request.session.pop(key, None) + return True + + attempts = state.get("attempts", 0) + 1 + if attempts >= MAX_ATTEMPTS: + request.session.pop(key, None) + else: + # __setitem__ already flips request.session.modified; no + # separate assignment needed. + request.session[key] = {**state, "attempts": attempts} + return False + + # --- enrollment -------------------------------------------------------- + + def begin_enroll(self, request): + """Send a confirmation code to the account's address. + + Returns address=None rather than raising when there is no address: + views.enroll.enroll_factor calls this outside its try/except, so an + exception here is a 500 on a hand-typed URL. is_available() already + keeps the factor off the security page for such an account. + + Gated on mask_email(address), not the raw address -- see + is_available()'s docstring for why "deliverable" and "displayable" + have to be the same check. Includes code_length so the template's + maxlength/label can track MFA_EMAIL_CODE_LENGTH instead of assuming + it's 6: a mismatch there means a correctly-emailed code can never be + typed into the truncated input at all. + """ + address = user_email(request.user) + masked = mask_email(address) + if not masked: + return {"address": None} + self._ensure_code(request, request.user, address, ENROLL_STATE_KEY) + return {"address": masked, + "code_length": mfa_settings.MFA_EMAIL_CODE_LENGTH} + + def complete_enroll(self, request, data): + address = user_email(request.user) + if not address or not self._spend( + request, ENROLL_STATE_KEY, address, data.get("code", "")): + raise ValueError("Verification code is expired or invalid.") + return Authenticator.objects.create( + user=request.user, type=self.type, data={"address": address}) + + # --- verification ------------------------------------------------------ + + def _address_for(self, user): + """The address this user's factor was enrolled against. + + Deliberately the stored one, not user_email(user): a factor is + possession of a specific mailbox, and silently following a mutable + profile field means whoever can change that field can redirect the + factor. Falls back to the profile address only for a row that + predates the field being stored. + """ + authenticator = self.get_instances(user).first() + if authenticator is None: + return "" + return authenticator.data.get("address") or user_email(user) + + def begin_verify(self, request, user): + """Send a challenge code. + + A throttled send renders exactly as a successful one does -- same + copy, same status, no mail. Telling the user they were throttled + would hand an attacker a free counter of how many sends remain, and + the legitimate user's remedy (wait, or pick another method) is the + same either way. The same now holds for a mail-backend failure -- + see _ensure_code. + + Gated on mask_email(address), matching begin_enroll -- see + is_available()'s docstring. Includes code_length for the same + reason begin_enroll does. + """ + address = self._address_for(user) + masked = mask_email(address) + if not masked: + return {"address": None} + self._ensure_code(request, user, address, VERIFY_STATE_KEY) + return {"address": masked, + "code_length": mfa_settings.MFA_EMAIL_CODE_LENGTH} + + def complete_verify(self, request, user, data): + address = self._address_for(user) + if not address or not self._spend( + request, VERIFY_STATE_KEY, address, data.get("code", "")): + return False + authenticator = self.get_instances(user).first() + if authenticator is None: + return False + authenticator.record_usage() + return True diff --git a/django_mfa/adapters/recovery_codes.py b/django_mfa/adapters/recovery_codes.py index 2ac3931..ac7f27c 100644 --- a/django_mfa/adapters/recovery_codes.py +++ b/django_mfa/adapters/recovery_codes.py @@ -4,6 +4,7 @@ from django.contrib.auth.hashers import check_password, make_password +from django_mfa import events from django_mfa.atomic import update_data from django_mfa.models import Authenticator from django_mfa.registry import Adapter @@ -78,4 +79,7 @@ def spend(current): if not update_data(auth, spend): return False auth.record_usage() + events.recovery_code_used.send_robust( + sender=type(self), user=user, + remaining=self.remaining(user), request=request) return True diff --git a/django_mfa/apps.py b/django_mfa/apps.py index d9bc57f..cff8ab7 100644 --- a/django_mfa/apps.py +++ b/django_mfa/apps.py @@ -10,13 +10,16 @@ class DjangoMfaAppConfig(AppConfig): def ready(self): from django_mfa.checks import ( check_fido2_rp_id, + check_mfa_required_predicate, check_webauthn_backend_configured, ) register(check_fido2_rp_id) register(check_webauthn_backend_configured) + register(check_mfa_required_predicate) from django_mfa import ( adapters, # noqa: F401 (registers built-ins) + notifications, # noqa: F401 (connects notification receivers) signals, # noqa: F401 ) diff --git a/django_mfa/checks.py b/django_mfa/checks.py index 5c9718a..ed3263e 100644 --- a/django_mfa/checks.py +++ b/django_mfa/checks.py @@ -115,3 +115,33 @@ def check_webauthn_backend_configured(app_configs, **kwargs): f"request. Add {backend_path!r} to AUTHENTICATION_BACKENDS.", id="django_mfa.E003", )] + + +def check_mfa_required_predicate(app_configs, **kwargs): + """``MFA_REQUIRED`` must be something policy.resolve() can use. + + Deliberately NOT gated on ``_webauthn_active()``. That gate exists so a + TOTP-only project isn't asked for WebAuthn settings; MFA_REQUIRED is not + WebAuthn-specific and applies to every install. + + Without this check the failure surfaces as an ImportError or TypeError + raised from inside MfaMiddleware, on a user's first request after + deploy, on every request -- i.e. a total outage discovered in production + rather than a refused `manage.py check`. + """ + from django.core.exceptions import ImproperlyConfigured + + from django_mfa import policy + + try: + policy.resolve() + except (ImportError, ImproperlyConfigured, TypeError) as exc: + return [Error( + f"MFA_REQUIRED is not usable: {exc}", + hint="Set it to False (nobody), True (everyone), a callable " + "taking a user and returning a bool, or a dotted path to " + "one. django_mfa.policy.is_staff and " + "django_mfa.policy.in_groups(...) are supplied.", + id="django_mfa.E004", + )] + return [] diff --git a/django_mfa/conf.py b/django_mfa/conf.py index a20b13b..1b5fa8c 100644 --- a/django_mfa/conf.py +++ b/django_mfa/conf.py @@ -7,9 +7,11 @@ "MFA_BASE_TEMPLATE": "django_mfa/base.html", "MFA_SECRET_ENCRYPTION_KEYS": None, "MFA_VERIFY_RATE_LIMIT": "5/5m", + "MFA_EMAIL_SEND_RATE_LIMIT": "3/5m", "MFA_QUICKLOGIN": False, "MFA_OWNED_BY_ENTERPRISE": False, "MFA_FACTORS": ["totp", "recovery_codes", "webauthn"], + "MFA_REQUIRED": False, "MFA_FIDO2_RP_ID": None, "MFA_FIDO2_RP_NAME": "django-mfa", "MFA_FIDO2_RESIDENT_KEY": "preferred", @@ -17,6 +19,11 @@ "MFA_FIDO2_USER_VERIFICATION": "preferred", "MFA_FIDO2_ATTESTATION_PREFERENCE": "none", "MFA_EXEMPT_PATHS": [], + "MFA_EMAIL_CODE_LENGTH": 6, + "MFA_EMAIL_CODE_VALIDITY": 300, + "MFA_EMAIL_SUBJECT": None, + "MFA_FROM_EMAIL": None, + "MFA_NOTIFY_ON_CHANGE": False, } diff --git a/django_mfa/decorators.py b/django_mfa/decorators.py new file mode 100644 index 0000000..3d1dfcf --- /dev/null +++ b/django_mfa/decorators.py @@ -0,0 +1,71 @@ +"""Per-view MFA enforcement. + +MFA_REQUIRED (django_mfa.policy) decides which *users* must hold a factor. +These decide which *views* require one, for everybody, which a user predicate +cannot express: "MFA on the billing flow" is a property of the view. + +Share two destinations with MfaMiddleware, in the same order: pending -> +the verify picker, no primary factor -> the security page. The second +rung differs in what triggers it, though: the middleware fires it only +for a user MFA_REQUIRED applies to, while these fire it for anyone who +reaches a decorated view -- that is the whole point of having both. +Add a rung the middleware doesn't have: unauthenticated -> LOGIN_URL. MfaMiddleware +passes an unauthenticated request straight through (it isn't a login +gate); a view behind only this decorator may have no other login gate in +front of it, so the decorator supplies that rung itself. +""" + +from functools import wraps + +from django.contrib.auth.views import redirect_to_login +from django.shortcuts import resolve_url +from django.urls import reverse + +from django_mfa import session + + +def _enforce(request): + """Return a redirect response, or None to let the request through.""" + from django_mfa.registry import registry + + user = request.user + if not user.is_authenticated: + return redirect_to_login(request.get_full_path()) + if session.is_pending(request): + return redirect_to_login(request.get_full_path(), + resolve_url(reverse("mfa:verify")), "next") + if not registry.has_primary_factor(user): + # has_primary_factor(), not enabled_for(): a user holding only + # recovery codes is not protected, and recovery codes must never be + # somebody's sole second factor. has_primary_factor(), not + # primary_enabled_for(): only the yes/no answer is needed here, and + # this runs on every request to a decorated view. + return redirect_to_login( + request.get_full_path(), + resolve_url(reverse("mfa:security_settings")), "next") + return None + + +def mfa_required(view_func): + """Require a verified second factor for this view.""" + @wraps(view_func) + def _wrapped(request, *args, **kwargs): + response = _enforce(request) + if response is not None: + return response + return view_func(request, *args, **kwargs) + + return _wrapped + + +class MfaRequiredMixin: + """Class-based-view form of ``mfa_required``. + + Mix in FIRST, so dispatch() runs before the view's own. + """ + + def dispatch(self, request, *args, **kwargs): + response = _enforce(request) + if response is not None: + return response + return super().dispatch(request, *args, **kwargs) diff --git a/django_mfa/events.py b/django_mfa/events.py new file mode 100644 index 0000000..97ddb5b --- /dev/null +++ b/django_mfa/events.py @@ -0,0 +1,44 @@ +"""Signals django_mfa emits, for host projects to hook. + +Deliberately a separate module from django_mfa/signals.py, and deliberately +importing nothing from django_mfa. signals.py imports django_mfa.views at +module scope (for verify_rmb_cookie), so an *adapter* that imported +django_mfa.signals to send an event would close the cycle +adapters -> signals -> views -> registry -> adapters. A module with no +internal imports cannot participate in a cycle at all. + +signals.py re-exports every name below, so `from django_mfa.signals import +factor_added` -- where a Django developer looks first -- also works. That +re-export is one-way (signals imports events, never the reverse) and safe. + +`sender` is uniformly the Adapter *class* for the factor involved, so a +receiver can narrow with `sender=TOTPAdapter`. `request` is always supplied: +every emission point sits inside a request path. +""" + +import django.dispatch + +#: A user enrolled a factor. kwargs: user, authenticator, request. +#: `authenticator` is the created Authenticator row -- every Adapter's +#: complete_enroll() returns it (see Adapter.complete_enroll's docstring). +factor_added = django.dispatch.Signal() + +#: A user removed a factor. kwargs: user, factor_type, name, request. +#: The row is already gone by the time this fires, so its type and name are +#: passed by value rather than as an instance. +factor_removed = django.dispatch.Signal() + +#: A second-factor challenge succeeded. kwargs: user, method, request. +mfa_verified = django.dispatch.Signal() + +#: A second-factor challenge failed. kwargs: user, method, request. +#: Fires on every failure path in views/verify.py -- a wrong code, an adapter +#: ceremony error, AND an attempt refused by the rate limiter. A receiver +#: watching for brute force needs to see the attempts that were refused, not +#: only the ones that were evaluated. +mfa_verification_failed = django.dispatch.Signal() + +#: A recovery code was spent. kwargs: user, remaining, request. +#: Emitted from the adapter rather than the view: only the adapter knows how +#: many codes are left. +recovery_code_used = django.dispatch.Signal() diff --git a/django_mfa/middleware.py b/django_mfa/middleware.py index be26162..b45fd64 100644 --- a/django_mfa/middleware.py +++ b/django_mfa/middleware.py @@ -20,6 +20,10 @@ def exempt_paths(self): Without it, a pending user who cannot complete their second factor (lost device, no recovery codes, etc.) has no way out of the redirect loop this middleware creates — they can't even log out. + + Deliberately does NOT include the enroll pages. See + enrollment_exempt_paths() below for why the two sets must stay + separate. """ from django_mfa.conf import settings as mfa_settings from django_mfa.registry import registry @@ -30,15 +34,70 @@ def exempt_paths(self): paths.update(mfa_settings.MFA_EXEMPT_PATHS) return paths + def enrollment_exempt_paths(self): + """Paths reachable while a REQUIRED user has not enrolled anything. + + A different set from exempt_paths() above, and merging the two is a + vulnerability rather than a tidy-up: views.enroll.enroll_factor calls + session.mark_verified() on success, so a *pending* user allowed onto + an enroll page could enroll a fresh TOTP with a secret of their own + choosing and satisfy the session without ever presenting the factor + they already hold. The pending set must exclude enrollment; this set + is almost entirely enrollment. + + MFA_EXEMPT_PATHS applies here too: without it a required user who + cannot enroll (no phone, no security key to hand) is trapped with no + way even to log out. + """ + from django_mfa.conf import settings as mfa_settings + from django_mfa.registry import registry + + paths = {reverse("mfa:security_settings"), reverse("mfa:recovery_codes")} + for adapter in registry.all(): + if adapter.supports_enroll: + paths.add(reverse("mfa:enroll_factor", args=[adapter.type])) + paths.update(mfa_settings.MFA_EXEMPT_PATHS) + return paths + + @staticmethod + def _is_exempt(path, paths): + """One comparison for both sets, so they cannot come to disagree + about what counts as the same path.""" + return path in paths + def process_request(self, request): + from django_mfa import policy + from django_mfa.registry import registry + if not request.user.is_authenticated: return None - if not session.is_pending(request): - return None - if request.path in self.exempt_paths(): - return None - return redirect_to_login(request.get_full_path(), - resolve_url(reverse("mfa:verify")), "next") + + if session.is_pending(request): + if self._is_exempt(request.path, self.exempt_paths()): + return None + return redirect_to_login(request.get_full_path(), + resolve_url(reverse("mfa:verify")), "next") + + # A required user with no primary factor is never *pending*: + # signals.stamp_pending_verification only stamps a session when + # registry.primary_enabled_for() is non-empty. That is precisely why + # this second rung exists -- without it, "MFA is required" would have + # no effect on the one user it needs to reach. + # + # has_primary_factor(), not primary_enabled_for(): this rung only + # needs the yes/no answer, and primary_enabled_for() would run one + # .exists() query per registered adapter (via enabled_for()) on every + # single authenticated request just to build a list this branch + # immediately discards. + if (policy.mfa_required_for(request.user) + and not registry.has_primary_factor(request.user)): + if self._is_exempt(request.path, self.enrollment_exempt_paths()): + return None + return redirect_to_login( + request.get_full_path(), + resolve_url(reverse("mfa:security_settings")), "next") + + return None def process_response(self, request, response): """Finish applying the MFA_QUICKLOGIN hint cookie set/cleared by the diff --git a/django_mfa/migrations/0008_email_factor.py b/django_mfa/migrations/0008_email_factor.py new file mode 100644 index 0000000..000584d --- /dev/null +++ b/django_mfa/migrations/0008_email_factor.py @@ -0,0 +1,50 @@ +# Adds the "email" factor type. +# +# Two operations, and the second is the one that is easy to miss: the +# singleton constraint's condition is a literal list of type strings, so a new +# singleton factor that isn't added to it is simply not constrained. The +# adapter's supports_multiple = False would then be a promise nothing keeps, +# and a second row would race EmailAdapter.get_instances(user).first() -- +# making which address receives the code depend on row order. +# +# Reversible, unlike 0007: nothing here destroys data. Reversing restores the +# previous choices and constraint; any "email" rows already created would then +# violate nothing (the constraint simply stops covering them) but would be +# unreadable by an adapter that no longer exists. +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("django_mfa", "0007_drop_legacy_models"), + ] + + operations = [ + migrations.AlterField( + model_name="authenticator", + name="type", + field=models.CharField( + choices=[ + ("totp", "Authenticator app"), + ("webauthn", "Security key or passkey"), + ("recovery_codes", "Recovery codes"), + ("email", "Emailed code"), + ], + max_length=32, + ), + ), + migrations.RemoveConstraint( + model_name="authenticator", + name="mfa_one_singleton_authenticator_per_user", + ), + migrations.AddConstraint( + model_name="authenticator", + constraint=models.UniqueConstraint( + condition=models.Q( + ("type__in", ["totp", "recovery_codes", "email"])), + fields=("user", "type"), + name="mfa_one_singleton_authenticator_per_user", + ), + ), + ] diff --git a/django_mfa/models.py b/django_mfa/models.py index 7edb2e3..e6980b5 100644 --- a/django_mfa/models.py +++ b/django_mfa/models.py @@ -22,6 +22,7 @@ class Type(models.TextChoices): TOTP = "totp", "Authenticator app" WEBAUTHN = "webauthn", "Security key or passkey" RECOVERY_CODES = "recovery_codes", "Recovery codes" + EMAIL = "email", "Emailed code" user = models.ForeignKey(settings.AUTH_USER_MODEL, related_name="mfa_authenticators", @@ -38,7 +39,7 @@ class Meta: constraints = [ models.UniqueConstraint( fields=["user", "type"], - condition=models.Q(type__in=["totp", "recovery_codes"]), + condition=models.Q(type__in=["totp", "recovery_codes", "email"]), name="mfa_one_singleton_authenticator_per_user", ), ] diff --git a/django_mfa/notifications.py b/django_mfa/notifications.py new file mode 100644 index 0000000..66d9e23 --- /dev/null +++ b/django_mfa/notifications.py @@ -0,0 +1,98 @@ +"""Opt-in emails telling a user their second factor changed. + +Off by default (MFA_NOTIFY_ON_CHANGE). The signals in django_mfa.events fire +either way -- a project that wants delivery off the request path, or wants to +route these somewhere other than email, connects its own receiver and leaves +this switched off. That split is why the signals ship as their own module. + +Templates follow the convention django-allauth established, so overriding is +familiar: shadow django_mfa/email/.txt and _subject.txt. +""" + +import logging + +from django.conf import settings as django_settings +from django.core.mail import send_mail +from django.dispatch import receiver +from django.template.loader import render_to_string + +from django_mfa import events +from django_mfa.conf import settings as mfa_settings +from django_mfa.utils import user_email + +logger = logging.getLogger(__name__) + + +def _notify(user, template, context): + """Render and send one notification. Never raises. + + A mail outage must not turn "remove this security key I think is + compromised" into a 500 -- the security action is more urgent than the + message about it. Failures are logged at ERROR and dropped, which + docs/security.md states plainly so nobody reads a delivered notification + as a guarantee. + """ + if not mfa_settings.MFA_NOTIFY_ON_CHANGE: + return + + address = user_email(user) + if not address: + return + + try: + subject = render_to_string( + f"django_mfa/email/{template}_subject.txt", context) + subject = " ".join(subject.split()) + body = render_to_string(f"django_mfa/email/{template}.txt", context) + send_mail( + subject, body, + mfa_settings.MFA_FROM_EMAIL or django_settings.DEFAULT_FROM_EMAIL, + [address], + ) + except Exception: + logger.exception( + "django-mfa could not send the %r notification to user %s", + template, getattr(user, "pk", "?")) + + +@receiver(events.factor_added) +def notify_factor_added(sender, user, authenticator, request=None, **kwargs): + _notify(user, "factor_added", { + "user": user, + "factor": authenticator.get_type_display(), + "name": authenticator.name, + }) + + +@receiver(events.factor_removed) +def notify_factor_removed(sender, user, factor_type, name, request=None, + **kwargs): + # _notify() already no-ops on this setting, but checking it again here, + # before doing anything else, matters: without this early return, every + # factor removal -- on a default install, with notifications off -- still + # paid for registry.has_primary_factor()'s query just to compute an + # mfa_disabled context that _notify() would immediately discard. This is + # what keeps a default install exactly as cheap as before notifications + # existed at all. + if not mfa_settings.MFA_NOTIFY_ON_CHANGE: + return + + from django_mfa.models import Authenticator + from django_mfa.registry import registry + + label = dict(Authenticator.Type.choices).get(factor_type, factor_type) + _notify(user, "factor_removed", { + "user": user, "factor": label, "name": name}) + + # "I am no longer protected" is the state that matters to a user, and no + # single signal expresses it -- it is a property of what is left, so it is + # derived here rather than emitted as its own event. has_primary_factor, + # not primary_enabled_for: only the yes/no answer is needed to decide + # whether to send mfa_disabled. + if not registry.has_primary_factor(user): + _notify(user, "mfa_disabled", {"user": user}) + + +@receiver(events.recovery_code_used) +def notify_recovery_code_used(sender, user, remaining, request=None, **kwargs): + _notify(user, "recovery_code_used", {"user": user, "remaining": remaining}) diff --git a/django_mfa/policy.py b/django_mfa/policy.py new file mode 100644 index 0000000..f9bc0c6 --- /dev/null +++ b/django_mfa/policy.py @@ -0,0 +1,86 @@ +"""Who is required to hold a second factor. + +``MFA_REQUIRED`` answers that question in one of four shapes: + + MFA_REQUIRED = False # the default: nobody + MFA_REQUIRED = True # every authenticated user + MFA_REQUIRED = "myapp.policy.needs_mfa" # dotted path to predicate(user) + MFA_REQUIRED = some_callable # the same, imported yourself + +Enrollment is otherwise entirely voluntary: MfaMiddleware only challenges a +user who already holds a factor, so without this setting a user who never +enrolls is never prompted. + +This module answers "is this user *required* to have MFA". Whether they +actually have it is registry.primary_enabled_for(user), which stays the +single source of truth for that -- do not add a second definition here. +""" + +from functools import cache + +from django.core.exceptions import ImproperlyConfigured +from django.utils.module_loading import import_string + +from django_mfa.conf import settings as mfa_settings + + +def is_staff(user): + """Predicate: require MFA of anyone who can reach the Django admin.""" + return bool(getattr(user, "is_staff", False)) + + +def in_groups(*names): + """Return a predicate requiring MFA of anyone in one of ``names``. + + A factory, not a predicate -- call it and assign the result: + + MFA_REQUIRED = in_groups("admins", "finance") + """ + def predicate(user): + return user.groups.filter(name__in=names).exists() + + predicate.mfa_group_names = tuple(names) + return predicate + + +@cache +def _import(path): + """Import a dotted path once per distinct string. + + Keyed on the path itself rather than cached as a single module-level + value, so override_settings() in tests (and a host project that changes + the setting between requests) is honoured rather than pinned to whatever + was resolved first. + """ + return import_string(path) + + +def resolve(): + """Return ``MFA_REQUIRED`` as a callable predicate, or None when off. + + Raises ImproperlyConfigured (or ImportError, from _import) for a value it + cannot use. Both are caught at startup by checks.check_mfa_required_ + predicate (django_mfa.E004) so the failure surfaces from `manage.py + check` rather than from inside middleware on a user's first request. + """ + value = mfa_settings.MFA_REQUIRED + if value is None or value is False: + return None + if value is True: + return lambda user: True + if isinstance(value, str): + value = _import(value) + if not callable(value): + raise ImproperlyConfigured( + f"MFA_REQUIRED must be a bool, a callable, or a dotted path to " + f"one -- got {value!r}." + ) + return value + + +def mfa_required_for(user): + """Is this user required to hold a primary second factor?""" + if not getattr(user, "is_authenticated", False): + return False + predicate = resolve() + return bool(predicate and predicate(user)) diff --git a/django_mfa/ratelimit.py b/django_mfa/ratelimit.py index 69b6896..cb84b61 100644 --- a/django_mfa/ratelimit.py +++ b/django_mfa/ratelimit.py @@ -8,10 +8,10 @@ PATTERN = re.compile(r"^(\d+)/(\d+)([smh])$") -def parse(spec): +def parse(spec, setting="MFA_VERIFY_RATE_LIMIT"): match = PATTERN.match(spec) if not match: - raise ValueError(f"Invalid MFA_VERIFY_RATE_LIMIT: {spec!r}") + raise ValueError(f"Invalid {setting}: {spec!r}") count, amount, unit = match.groups() count, window = int(count), int(amount) * UNITS[unit] # A zero limit locks every user out permanently — check() compares @@ -20,27 +20,41 @@ def parse(spec): # Both are almost certainly typos, so refuse them loudly. if count < 1: raise ValueError( - f"MFA_VERIFY_RATE_LIMIT count must be at least 1, got {spec!r} — " + f"{setting} count must be at least 1, got {spec!r} — " "a zero limit would lock out every user permanently." ) if window < 1: raise ValueError( - f"MFA_VERIFY_RATE_LIMIT window must be at least 1 second, got {spec!r}" + f"{setting} window must be at least 1 second, got {spec!r}" ) return count, window -def _key(user, factor_type): - return f"django_mfa:rl:{user.pk}:{factor_type}" +def _key(user, scope): + """Cache key for one user's counter in one rate-limit ``scope``. + + ``scope`` is either a bare factor type (``"totp"``, from + views/verify.py's per-factor MFA_VERIFY_RATE_LIMIT budget -- always a URL + segment, e.g. from ``verify_factor(request, factor_type)``, so it can + never contain a colon) or a colon-namespaced string an adapter defines + for its own separate budget (e.g. adapters/email.py's SEND_SCOPE = + "email:send", which throttles *sending* a code rather than guessing one). + Because a factor-type scope is exactly one of the fixed, colon-free + values the URL conf can dispatch to, and a namespaced scope always + contains at least one colon, the two shapes can never collide inside the + same ``f"...:{scope}"`` key -- there is no factor type and no adapter + namespace that produce the same string. + """ + return f"django_mfa:rl:{user.pk}:{scope}" -def check(user, factor_type): - limit, _window = parse(mfa_settings.MFA_VERIFY_RATE_LIMIT) - return cache.get(_key(user, factor_type), 0) < limit +def check(user, scope, setting="MFA_VERIFY_RATE_LIMIT"): + limit, _window = parse(getattr(mfa_settings, setting), setting) + return cache.get(_key(user, scope), 0) < limit -def record_failure(user, factor_type): - """Count one failed attempt against ``user``'s budget for ``factor_type``. +def record(user, scope, setting="MFA_VERIFY_RATE_LIMIT"): + """Count one event against ``user``'s budget for ``scope``. add()-then-incr(), not get()-then-set(). The latter is a read-modify-write and loses increments whenever two attempts interleave -- which is not a @@ -59,8 +73,8 @@ def record_failure(user, factor_type): the ordinary reading of "5/5m" and is the stricter-to-reason-about of the two; the previous behaviour extended the lockout on every attempt. """ - _limit, window = parse(mfa_settings.MFA_VERIFY_RATE_LIMIT) - key = _key(user, factor_type) + _limit, window = parse(getattr(mfa_settings, setting), setting) + key = _key(user, scope) try: cache.incr(key) return @@ -84,5 +98,11 @@ def record_failure(user, factor_type): cache.set(key, 1, window) -def clear(user, factor_type): - cache.delete(_key(user, factor_type)) +#: The original name, kept because views/verify.py and its tests call it and +#: this refactor must not change the verification path at all. "failure" is +#: accurate for that caller; the generic name is `record`. +record_failure = record + + +def clear(user, scope): + cache.delete(_key(user, scope)) diff --git a/django_mfa/registry.py b/django_mfa/registry.py index 1c4437b..3d0b78a 100644 --- a/django_mfa/registry.py +++ b/django_mfa/registry.py @@ -59,6 +59,15 @@ def begin_enroll(self, request): raise NotImplementedError def complete_enroll(self, request, data): + """Validate ``data`` and create the Authenticator row. + + MUST return the created ``Authenticator``: ``views.enroll`` passes it + straight to the ``factor_added`` signal, so a factor that returns + ``None`` here silently degrades every host project's audit log and + notification for that factor type. Raise ``ValueError`` to reject the + submission -- the view turns that into the same generic 400 a wrong + code gets. + """ raise NotImplementedError def begin_verify(self, request, user): @@ -123,9 +132,51 @@ def primary_enabled_for(self, user): Use this to decide whether to CHALLENGE a user. Use enabled_for() to decide what to OFFER them once challenged — that one includes recovery codes, which are a valid way to verify but not a factor in themselves. + + Runs one .exists() query per registered adapter (via enabled_for()), + because it hands back actual Adapter instances. A caller that only + needs the yes/no answer — "is this user protected at all" — should + use has_primary_factor() below instead: same predicate, one query + total. The two must keep agreeing, since counts_as_primary_factor is + the single source of truth for both. """ return [a for a in self.enabled_for(user) if a.counts_as_primary_factor] + def has_primary_factor(self, user): + """True if this user holds at least one factor that counts as + primary — the boolean counterpart to primary_enabled_for(), for + callers (MfaMiddleware, the enforcement decorator, the security-page + context, the notifications module) that only need the yes/no answer + and would otherwise pay for one .exists() query per registered + adapter just to throw the list away. + + One query total: Authenticator.objects.filter(user=user, + type__in=[...]).exists(). The type list is derived from the registry + at call time (``[a.type for a in self.all() if + a.counts_as_primary_factor]``) rather than hardcoded here, which is + deliberate and load-bearing — Adapter.counts_as_primary_factor is the + single, explicit source of truth for what counts as a primary + factor (see its docstring). This project once had a second, + independent definition of that list (models.PRIMARY_FACTOR_TYPES) + that silently drifted out of sync with the registry and was removed + for exactly that reason; do not reintroduce one here by hardcoding + `type__in=["totp", "webauthn", ...]`. + + Callers that need the actual adapter list, not just whether it's + non-empty, must keep using primary_enabled_for() instead — this + method throws that information away by design, in exchange for the + single query. + + signals.stamp_pending_verification and views/verify.py:verify_factor + still call primary_enabled_for() even though both use the result as a + bare boolean. That is not because they need the list: they run once + per login rather than once per request, so the query saving does not + arise there, and leaving them alone kept this change off the + authentication path. Switching them later is safe. + """ + types = [a.type for a in self.all() if a.counts_as_primary_factor] + return Authenticator.objects.filter(user=user, type__in=types).exists() + def available_for(self, user): """Adapters this user could add right now, via the enroll flow. diff --git a/django_mfa/signals.py b/django_mfa/signals.py index 655a29d..b3aa2f8 100644 --- a/django_mfa/signals.py +++ b/django_mfa/signals.py @@ -3,6 +3,17 @@ from django_mfa import session from django_mfa.conf import settings as mfa_settings + +# Re-exported so `from django_mfa.signals import factor_added` works -- the +# import path a Django developer tries first. The definitions live in +# django_mfa/events.py; see that module's docstring for why. +from django_mfa.events import ( # noqa: F401 + factor_added, + factor_removed, + mfa_verification_failed, + mfa_verified, + recovery_code_used, +) from django_mfa.registry import registry from django_mfa.views import verify_rmb_cookie diff --git a/django_mfa/static/django_mfa/style.css b/django_mfa/static/django_mfa/style.css index 4946a42..09710b7 100644 --- a/django_mfa/static/django_mfa/style.css +++ b/django_mfa/static/django_mfa/style.css @@ -254,6 +254,10 @@ kbd { font-size: 0.875rem; } +.mfa-notice { border-radius: 6px; padding: 12px 16px; margin: 0 0 20px; } +.mfa-notice--warning { background: #fff8e1; border: 1px solid #f0c36d; color: #6b4e00; } +.mfa-notice p { margin: 4px 0 0; } + /* -------------------------------------------------------------------------- * Lists of enrolled methods / pickable methods * ---------------------------------------------------------------------- */ diff --git a/django_mfa/templates/django_mfa/email/factor_added.txt b/django_mfa/templates/django_mfa/email/factor_added.txt new file mode 100644 index 0000000..b8bd3ec --- /dev/null +++ b/django_mfa/templates/django_mfa/email/factor_added.txt @@ -0,0 +1,3 @@ +{% load i18n %}{% blocktrans %}A new two-factor method was added to your account: {{ factor }}{% endblocktrans %}{% if name %} ({{ name }}){% endif %} + +{% trans "If this wasn't you, remove it and change your password immediately." %} diff --git a/django_mfa/templates/django_mfa/email/factor_added_subject.txt b/django_mfa/templates/django_mfa/email/factor_added_subject.txt new file mode 100644 index 0000000..e164c9f --- /dev/null +++ b/django_mfa/templates/django_mfa/email/factor_added_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% trans "A new two-factor method was added to your account" %} diff --git a/django_mfa/templates/django_mfa/email/factor_removed.txt b/django_mfa/templates/django_mfa/email/factor_removed.txt new file mode 100644 index 0000000..6cfe747 --- /dev/null +++ b/django_mfa/templates/django_mfa/email/factor_removed.txt @@ -0,0 +1,3 @@ +{% load i18n %}{% blocktrans %}This two-factor method was removed from your account: {{ factor }}{% endblocktrans %}{% if name %} ({{ name }}){% endif %} + +{% trans "If this wasn't you, change your password immediately." %} diff --git a/django_mfa/templates/django_mfa/email/factor_removed_subject.txt b/django_mfa/templates/django_mfa/email/factor_removed_subject.txt new file mode 100644 index 0000000..3797778 --- /dev/null +++ b/django_mfa/templates/django_mfa/email/factor_removed_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% trans "A two-factor method was removed from your account" %} diff --git a/django_mfa/templates/django_mfa/email/mfa_disabled.txt b/django_mfa/templates/django_mfa/email/mfa_disabled.txt new file mode 100644 index 0000000..adc32ae --- /dev/null +++ b/django_mfa/templates/django_mfa/email/mfa_disabled.txt @@ -0,0 +1,3 @@ +{% load i18n %}{% trans "Your account is no longer protected by two-factor authentication. A password is now all that's needed to sign in." %} + +{% trans "If this wasn't you, change your password and set up two-factor authentication again immediately." %} diff --git a/django_mfa/templates/django_mfa/email/mfa_disabled_subject.txt b/django_mfa/templates/django_mfa/email/mfa_disabled_subject.txt new file mode 100644 index 0000000..5a4ca5c --- /dev/null +++ b/django_mfa/templates/django_mfa/email/mfa_disabled_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% trans "Two-factor authentication is off for your account" %} diff --git a/django_mfa/templates/django_mfa/email/otp_code.txt b/django_mfa/templates/django_mfa/email/otp_code.txt new file mode 100644 index 0000000..8c68c37 --- /dev/null +++ b/django_mfa/templates/django_mfa/email/otp_code.txt @@ -0,0 +1,5 @@ +{% load i18n %}{% blocktrans %}Your sign-in code is {{ code }}.{% endblocktrans %} + +{% blocktrans %}It expires in {{ validity_minutes }} minutes and can be used once.{% endblocktrans %} + +{% trans "If you didn't try to sign in, someone may know your password. Change it." %} diff --git a/django_mfa/templates/django_mfa/email/otp_code_subject.txt b/django_mfa/templates/django_mfa/email/otp_code_subject.txt new file mode 100644 index 0000000..32c6cbd --- /dev/null +++ b/django_mfa/templates/django_mfa/email/otp_code_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% trans "Your sign-in code" %} diff --git a/django_mfa/templates/django_mfa/email/recovery_code_used.txt b/django_mfa/templates/django_mfa/email/recovery_code_used.txt new file mode 100644 index 0000000..3b97eef --- /dev/null +++ b/django_mfa/templates/django_mfa/email/recovery_code_used.txt @@ -0,0 +1,5 @@ +{% load i18n %}{% trans "A recovery code was just used to sign in to your account." %} + +{% blocktrans %}You have {{ remaining }} recovery codes left.{% endblocktrans %} + +{% trans "If this wasn't you, change your password and generate a fresh set of codes." %} diff --git a/django_mfa/templates/django_mfa/email/recovery_code_used_subject.txt b/django_mfa/templates/django_mfa/email/recovery_code_used_subject.txt new file mode 100644 index 0000000..2706094 --- /dev/null +++ b/django_mfa/templates/django_mfa/email/recovery_code_used_subject.txt @@ -0,0 +1 @@ +{% load i18n %}{% trans "A recovery code was used to sign in" %} diff --git a/django_mfa/templates/django_mfa/enroll_email.html b/django_mfa/templates/django_mfa/enroll_email.html new file mode 100644 index 0000000..03f138f --- /dev/null +++ b/django_mfa/templates/django_mfa/enroll_email.html @@ -0,0 +1,35 @@ +{% extends base_template %} +{% load i18n %} +{% block content %} +
+

{% trans "Set up email codes" %}

+ {% if address %} +

+ {% blocktrans %}We've emailed a code to {{ address }}. Enter it below to turn on email codes.{% endblocktrans %} +

+ +
+ {% csrf_token %} +
+
+
+ +

{% if error_message %}{{ error_message }}{% endif %}

+
+
+
+ + {% trans "Cancel" %} +
+
+ {% else %} +

{% trans "Your account has no email address, so codes can't be delivered. Add one to your profile first." %}

+
+ {% trans "Back" %} +
+ {% endif %} +
+{% endblock %} diff --git a/django_mfa/templates/django_mfa/security.html b/django_mfa/templates/django_mfa/security.html index 85b2a4a..551a1ce 100755 --- a/django_mfa/templates/django_mfa/security.html +++ b/django_mfa/templates/django_mfa/security.html @@ -1,9 +1,16 @@ {% extends base_template %} {% load i18n %} +{% load otp_tags %} {% block main_modifier %} mfa-main--wide{% endblock %} {% block content %}

{% trans "Two-factor authentication" %}

{% trans "Add a second step to your sign-in so a stolen password isn't enough on its own." %}

+{% if mfa_enrollment_required %} + +{% endif %}
@@ -17,7 +24,7 @@

{% trans "Your methods" %}

- {{ authenticator.get_type_display }}{% if authenticator.name %} — {{ authenticator.name }}{% endif %} + {{ authenticator.get_type_display }}{% if authenticator.name %} — {{ authenticator.name }}{% endif %}{% if authenticator.type == "email" and authenticator.data.address %} — {{ authenticator.data.address|mask_email }}{% endif %} {% blocktrans with created=authenticator.created_at|date:"j M Y" %}Added {{ created }}{% endblocktrans %} diff --git a/django_mfa/templates/django_mfa/verify_email.html b/django_mfa/templates/django_mfa/verify_email.html new file mode 100644 index 0000000..240be5a --- /dev/null +++ b/django_mfa/templates/django_mfa/verify_email.html @@ -0,0 +1,36 @@ +{% extends base_template %} +{% load i18n %} +{% block content %} +
+

{% trans "Enter your code" %}

+ {% if address %} +

+ {% blocktrans %}We've emailed a code to {{ address }}. It expires shortly.{% endblocktrans %} +

+ +
+ {% csrf_token %} + +
+
+
+ +

{% if error_message %}{{ error_message }}{% endif %}

+
+
+
+ + {% trans "Use another method" %} +
+
+ {% else %} +

{% trans "This account has no email code to verify. Use another method, or contact support if you believe this is a mistake." %}

+
+ {% trans "Use another method" %} +
+ {% endif %} +
+{% endblock %} diff --git a/django_mfa/templatetags/otp_tags.py b/django_mfa/templatetags/otp_tags.py index 399ee39..eac042b 100644 --- a/django_mfa/templatetags/otp_tags.py +++ b/django_mfa/templatetags/otp_tags.py @@ -22,9 +22,24 @@ from django import template from django.utils.html import conditional_escape, format_html +from django_mfa.utils import mask_email as _mask_email + register = template.Library() +@register.filter(name="mask_email") +def mask_email(address): + """Expose django_mfa.utils.mask_email() as a filter. + + security.html uses this to show which mailbox an email-type + Authenticator row is bound to (masked, not the full address) -- + docs/settings.md promises this and, until this filter existed, the + template had no way to keep that promise: authenticator.name is + WebAuthn-only and always blank for an emailed-code row. + """ + return _mask_email(address) + + @register.simple_tag(name="qrcode") def qrcode(value, alt=None): """Render an OTP provisioning URI as an inline SVG ````.""" diff --git a/django_mfa/tests/test_adapter_email.py b/django_mfa/tests/test_adapter_email.py new file mode 100644 index 0000000..dfbec23 --- /dev/null +++ b/django_mfa/tests/test_adapter_email.py @@ -0,0 +1,518 @@ +import hashlib +import time + +from django.contrib.auth.models import User +from django.core import mail +from django.core.cache import cache +from django.core.mail.backends.base import BaseEmailBackend +from django.test import Client, RequestFactory, TestCase, override_settings +from django.urls import reverse + +from django_mfa.adapters.email import ( + ENROLL_STATE_KEY, + VERIFY_STATE_KEY, + EmailAdapter, +) +from django_mfa.models import Authenticator +from django_mfa.registry import registry +from django_mfa.utils import mask_email + + +def request_for(user): + """A request with a real, mutable session -- the adapter stashes ceremony + state there, so SessionMiddleware's store is what the tests inspect.""" + from django.contrib.sessions.backends.db import SessionStore + + request = RequestFactory().get("/") + request.user = user + request.session = SessionStore() + return request + + +def code_from_last_mail(length=6): + """Pull the ``length``-digit code out of the message body rather than + reaching into the session, so the test proves the user could actually + have read it.""" + import re + + match = re.search(rf"\b(\d{{{length}}})\b", mail.outbox[-1].body) + assert match, f"no code in {mail.outbox[-1].body!r}" + return match.group(1) + + +class ExplodingEmailBackend(BaseEmailBackend): + """A minimal Django email backend that always raises on send. + + Simulates a downstream mail-provider outage without depending on + smtplib, a real network call, or mocking django.core.mail.send_mail + (which would test that a mock was called rather than that a real + failure is handled) -- see review finding 1. + """ + + def send_messages(self, email_messages): + raise RuntimeError("simulated mail outage") + + +class MaskEmailTests(TestCase): + def test_keeps_the_first_and_last_character(self): + self.assertEqual(mask_email("ashwin@example.com"), "a****n@example.com") + + def test_short_local_parts_are_fully_masked(self): + self.assertEqual(mask_email("ab@example.com"), "**@example.com") + self.assertEqual(mask_email("a@example.com"), "*@example.com") + + def test_garbage_in_empty_out(self): + self.assertEqual(mask_email(""), "") + self.assertEqual(mask_email("not-an-address"), "") + + def test_a_truthy_non_string_does_not_raise(self): + """Minor finding 9: `"@" not in address` raises TypeError for a + truthy non-string. Reachable via otp_tags.py's template filter if a + host project writes Authenticator.data directly -- that must return + "" like any other garbage input, not 500 the security page. + """ + self.assertEqual(mask_email(12345), "") + self.assertEqual(mask_email(["a@example.com"]), "") + self.assertEqual(mask_email({"address": "a@example.com"}), "") + + +class EnrollTests(TestCase): + def setUp(self): + cache.clear() + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + self.adapter = EmailAdapter() + + def test_begin_sends_a_code_and_masks_the_address(self): + request = request_for(self.user) + context = self.adapter.begin_enroll(request) + + self.assertEqual(len(mail.outbox), 1) + self.assertEqual(mail.outbox[0].to, ["ashwin@example.com"]) + self.assertEqual(context["address"], "a****n@example.com") + + def test_the_plaintext_code_is_never_stored_in_the_session(self): + """Under SESSION_ENGINE='signed_cookies' the session round-trips + through the client. Signed is not encrypted -- a plaintext code there + is handed straight to the person being challenged.""" + request = request_for(self.user) + self.adapter.begin_enroll(request) + code = code_from_last_mail() + + state = request.session[ENROLL_STATE_KEY] + self.assertNotIn(code, str(state)) + self.assertNotIn(code, str(dict(request.session.items()))) + self.assertIn("hash", state) + self.assertIn("salt", state) + + def test_the_stored_hash_is_not_reproducible_from_salt_and_code_alone(self): + """Pins the real property, not just the absence of the literal code. + + A plain, unkeyed sha256(f"{salt}:{code}") would still satisfy the + test above -- the code isn't a *substring* of that digest -- while + remaining fully recoverable offline: anyone holding (salt, hash), as + the challenged user does under SESSION_ENGINE='signed_cookies', can + just recompute that same digest for all 10**length candidates + without ever talking to the server, sidestepping both + MFA_EMAIL_CODE_VALIDITY and MAX_ATTEMPTS (review Critical 1). The + stored hash must depend on something the client never sees + (SECRET_KEY, via salted_hmac), so this unkeyed computation must + *not* match what got stored. + """ + request = request_for(self.user) + self.adapter.begin_enroll(request) + code = code_from_last_mail() + + state = request.session[ENROLL_STATE_KEY] + offline_guess = hashlib.sha256( + f"{state['salt']}:{code}".encode()).hexdigest() + self.assertNotEqual(offline_guess, state["hash"]) + + def test_a_correct_code_creates_the_authenticator(self): + request = request_for(self.user) + self.adapter.begin_enroll(request) + authenticator = self.adapter.complete_enroll( + request, {"code": code_from_last_mail()}) + + self.assertEqual(authenticator.type, "email") + self.assertEqual(authenticator.data["address"], "ashwin@example.com") + self.assertNotIn(ENROLL_STATE_KEY, request.session) + + def test_a_wrong_code_is_rejected_but_leaves_the_ceremony_open(self): + request = request_for(self.user) + self.adapter.begin_enroll(request) + + with self.assertRaises(ValueError): + self.adapter.complete_enroll(request, {"code": "000000"}) + + # Still open: three typos must not cost the user a fresh send, which + # the send throttle would then refuse. + authenticator = self.adapter.complete_enroll( + request, {"code": code_from_last_mail()}) + self.assertEqual(authenticator.type, "email") + + def test_the_ceremony_closes_after_too_many_attempts(self): + request = request_for(self.user) + self.adapter.begin_enroll(request) + for _ in range(3): + with self.assertRaises(ValueError): + self.adapter.complete_enroll(request, {"code": "000000"}) + + self.assertNotIn(ENROLL_STATE_KEY, request.session) + with self.assertRaises(ValueError): + self.adapter.complete_enroll(request, {"code": code_from_last_mail()}) + + @override_settings(MFA_EMAIL_CODE_VALIDITY=1) + def test_an_expired_code_is_rejected(self): + request = request_for(self.user) + self.adapter.begin_enroll(request) + code = code_from_last_mail() + request.session[ENROLL_STATE_KEY]["issued_at"] = time.time() - 5 + + with self.assertRaises(ValueError): + self.adapter.complete_enroll(request, {"code": code}) + + def test_no_address_means_the_factor_is_not_available(self): + user = User.objects.create_user("noemail", password="pw") + self.assertFalse(self.adapter.is_available(user)) + + def test_begin_enroll_without_an_address_does_not_explode(self): + """enroll_factor calls begin_enroll outside its try/except, so raising + here would be a 500 on a hand-typed URL.""" + user = User.objects.create_user("noemail", password="pw") + context = self.adapter.begin_enroll(request_for(user)) + self.assertIsNone(context["address"]) + self.assertEqual(mail.outbox, []) + + def test_an_at_sign_free_profile_value_is_treated_as_no_address(self): + """Review finding 7: begin_enroll used to gate on the raw address + being non-empty, while the template gated on mask_email() being + non-empty. For a profile email field holding a string with no "@" + (this package never validates that field), the two disagreed: a + code was minted and mailed, spending send budget, while the + rendered page said there was no address and showed no form to type + the code into at all -- an unrecoverable ceremony every time. + """ + user = User.objects.create_user( + "noatsign", email="not-an-address", password="pw") + context = self.adapter.begin_enroll(request_for(user)) + self.assertIsNone(context["address"]) + self.assertEqual(mail.outbox, []) + self.assertFalse(self.adapter.is_available(user)) + + +class VerifyTests(TestCase): + def setUp(self): + cache.clear() + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + self.adapter = EmailAdapter() + self.authenticator = Authenticator.objects.create( + user=self.user, type="email", + data={"address": "enrolled@example.com"}) + + def test_the_code_goes_to_the_enrolled_address_not_the_profile_one(self): + """A factor is possession of a specific mailbox. Following a mutable + profile field means whoever can change it can redirect the factor.""" + self.adapter.begin_verify(request_for(self.user), self.user) + self.assertEqual(mail.outbox[0].to, ["enrolled@example.com"]) + + def test_the_plaintext_code_is_never_stored_in_the_session(self): + """Mirror of EnrollTests' test of the same name (review finding 5). + The enroll-side ceremony proving this was not enough on its own: the + verify ceremony is the path an attacker who already holds the + password actually reaches, and it needs this guarantee pinned + independently rather than inferred from shared helper code -- a + change that broke it only on the verify side would otherwise pass + the whole suite. + """ + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + code = code_from_last_mail() + + state = request.session[VERIFY_STATE_KEY] + self.assertNotIn(code, str(state)) + self.assertNotIn(code, str(dict(request.session.items()))) + self.assertIn("hash", state) + self.assertIn("salt", state) + + def test_the_stored_hash_is_not_reproducible_from_salt_and_code_alone(self): + """Mirror of EnrollTests' test of the same name (Critical 1): the + verify ceremony is the path an attacker who already holds the + password actually reaches, so this needs pinning independently + rather than inferred from shared helper code. + """ + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + code = code_from_last_mail() + + state = request.session[VERIFY_STATE_KEY] + offline_guess = hashlib.sha256( + f"{state['salt']}:{code}".encode()).hexdigest() + self.assertNotEqual(offline_guess, state["hash"]) + + def test_a_correct_code_verifies(self): + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + result = self.adapter.complete_verify( + request, self.user, {"code": code_from_last_mail()}) + self.assertTrue(result) + + def test_a_code_cannot_be_replayed(self): + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + code = code_from_last_mail() + self.assertTrue(self.adapter.complete_verify(request, self.user, + {"code": code})) + self.assertFalse(self.adapter.complete_verify(request, self.user, + {"code": code})) + + def test_verifying_records_usage(self): + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + self.adapter.complete_verify(request, self.user, + {"code": code_from_last_mail()}) + self.authenticator.refresh_from_db() + self.assertIsNotNone(self.authenticator.last_used_at) + + +class SendThrottleTests(TestCase): + def setUp(self): + cache.clear() + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + self.adapter = EmailAdapter() + Authenticator.objects.create( + user=self.user, type="email", data={"address": "ashwin@example.com"}) + + def test_refreshing_the_page_reuses_the_code_and_sends_nothing_more(self): + request = request_for(self.user) + self.adapter.begin_verify(request, self.user) + self.adapter.begin_verify(request, self.user) + self.adapter.begin_verify(request, self.user) + self.assertEqual(len(mail.outbox), 1) + + @override_settings(MFA_EMAIL_SEND_RATE_LIMIT="2/5m") + def test_past_the_limit_no_mail_is_sent_and_nothing_is_revealed(self): + for _ in range(4): + request = request_for(self.user) # a fresh session each time + context = self.adapter.begin_verify(request, self.user) + self.assertEqual(context["address"], "a****n@example.com") + + self.assertEqual(len(mail.outbox), 2) + + @override_settings(MFA_EMAIL_SEND_RATE_LIMIT="1/5m") + def test_the_throttle_is_per_user(self): + other = User.objects.create_user( + "other", email="other@example.com", password="pw") + Authenticator.objects.create( + user=other, type="email", data={"address": "other@example.com"}) + + self.adapter.begin_verify(request_for(self.user), self.user) + self.adapter.begin_verify(request_for(other), other) + + self.assertEqual(len(mail.outbox), 2) + + +class SendFailureTests(TestCase): + """Review finding 1: send_mail's default fail_silently=False meant a + downstream mail outage propagated straight out of begin_enroll/ + begin_verify -- neither call site is wrapped in a try/except -- as an + unhandled 500 on every GET of the challenge/enroll page for as long as + the outage lasted. Worse, the send budget was spent *before* the send + was attempted, so once MFA_EMAIL_SEND_RATE_LIMIT ran out mid-outage the + same page started rendering a misleading 200 claiming a code had been + sent when none ever had -- the one branch that made a throttled send + distinguishable from a failing one. + """ + + def setUp(self): + cache.clear() + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + self.adapter = EmailAdapter() + + @override_settings( + EMAIL_BACKEND=( + "django_mfa.tests.test_adapter_email.ExplodingEmailBackend")) + def test_a_backend_failure_does_not_raise(self): + request = request_for(self.user) + context = self.adapter.begin_enroll(request) # must not raise + + self.assertEqual(context["address"], "a****n@example.com") + self.assertEqual(mail.outbox, []) + + @override_settings( + EMAIL_BACKEND=( + "django_mfa.tests.test_adapter_email.ExplodingEmailBackend")) + def test_a_failed_send_leaves_no_usable_ceremony_state(self): + """The state written just before the failed send must not survive + it -- otherwise a later complete_enroll() could be tricked into + validating against a code that was minted but never delivered.""" + request = request_for(self.user) + self.adapter.begin_enroll(request) + + self.assertNotIn(ENROLL_STATE_KEY, request.session) + with self.assertRaises(ValueError): + self.adapter.complete_enroll(request, {"code": "000000"}) + + @override_settings( + EMAIL_BACKEND=( + "django_mfa.tests.test_adapter_email.ExplodingEmailBackend")) + def test_verify_side_failure_is_the_same_shape(self): + Authenticator.objects.create( + user=self.user, type="email", data={"address": "ashwin@example.com"}) + request = request_for(self.user) + context = self.adapter.begin_verify(request, self.user) # must not raise + + self.assertEqual(context["address"], "a****n@example.com") + self.assertEqual(mail.outbox, []) + self.assertNotIn(VERIFY_STATE_KEY, request.session) + + +class ViewIntegrationTests(TestCase): + """Every other test in this module calls the adapter directly. The + entire premise of the adapter/registry design is that views/enroll.py + and views/verify.py drive EmailAdapter completely unmodified -- no + email-specific view or URL exists -- so at least one test needs to walk + the real, generic views (review finding 6). This class is also where + two other findings actually live: the code-length/template maxlength + mismatch (finding 2) and what three wrong guesses in a row actually do + once the view's own re-render-on-failure behaviour is in the loop + (finding 4). + + "email" is registered directly into the production registry singleton + for the duration of each test, mirroring the registry.unregister() / + addCleanup(registry.register, ...) pattern test_conf.py's ChecksTests + and WebAuthnBackendCheckTests already use for WebAuthn: MFA_FACTORS is + read once at app startup, so override_settings(MFA_FACTORS=...) after + that has no effect on the already-populated registry. + """ + + def setUp(self): + cache.clear() + registry.register(EmailAdapter()) + self.addCleanup(registry.unregister, "email") + + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + Authenticator.objects.create( + user=self.user, type="email", data={"address": "ashwin@example.com"}) + self.client = Client() + self.client.login(username="ashwin", password="pw") + + def test_get_sends_a_code_and_renders_the_default_length(self): + response = self.client.get(reverse("mfa:verify_factor", args=["email"])) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(mail.outbox), 1) + self.assertContains(response, 'maxlength="6"') + + @override_settings(MFA_EMAIL_CODE_LENGTH=8) + def test_a_non_default_code_length_is_still_submittable(self): + """Direct regression test for finding 2: with the template's + maxlength hardcoded to 6, an 8-digit code could never be typed into + the input at all -- every submission looked like an ordinary wrong + code and spent a verify-attempt for nothing. + """ + response = self.client.get(reverse("mfa:verify_factor", args=["email"])) + self.assertContains(response, 'maxlength="8"') + self.assertNotContains(response, 'maxlength="6"') + + code = code_from_last_mail(length=8) + response = self.client.post( + reverse("mfa:verify_factor", args=["email"]), {"code": code}) + self.assertEqual(response.status_code, 302) + self.assertTrue(self.client.session["mfa"]["verified"]) + + def test_three_wrong_posts_reuse_then_rotate_in_a_fresh_code(self): + """Direct regression test for finding 4, and the reason MAX_ATTEMPTS' + comment was corrected: the cap closes the issued *code*, not the + guessing session. verify_factor re-renders the challenge page (which + calls begin_verify again) after every failed POST, so the first two + wrong guesses are absorbed by the still-open ceremony -- no new mail + -- and the third, which pops the ceremony state, causes that very + re-render to mint and mail a fresh code with the attempt counter + back at zero. + """ + self.client.get(reverse("mfa:verify_factor", args=["email"])) + self.assertEqual(len(mail.outbox), 1) + + for _ in range(2): + response = self.client.post( + reverse("mfa:verify_factor", args=["email"]), {"code": "000000"}) + self.assertEqual(response.status_code, 400) + self.assertEqual(len(mail.outbox), 1, "still just the first code") + + response = self.client.post( + reverse("mfa:verify_factor", args=["email"]), {"code": "000000"}) + self.assertEqual(response.status_code, 400) + self.assertEqual( + len(mail.outbox), 2, + "the third wrong guess closed the ceremony, and the " + "re-render that follows minted a fresh code") + + # And that fresh code is a live, submittable ceremony -- the cap + # rotated the code, it didn't lock the factor. + code = code_from_last_mail() + response = self.client.post( + reverse("mfa:verify_factor", args=["email"]), {"code": code}) + self.assertEqual(response.status_code, 302) + self.assertTrue(self.client.session["mfa"]["verified"]) + + def test_security_page_shows_the_bound_address_masked(self): + """Direct regression test for finding 3: docs/settings.md promises + the security page shows the enrolled-with address masked, but + security.html only ever rendered authenticator.name -- always blank + for email, since complete_enroll never sets it. + + The session is marked verified by hand first: this user already + holds a primary factor, so login left the session pending, and + security_settings is not on MfaMiddleware's exempt list (by + design -- see enrollment_exempt_paths()'s docstring) while pending. + """ + session = self.client.session + session["mfa"] = {"verified": True, "method": "email", "at": 0} + session.save() + + response = self.client.get(reverse("mfa:security_settings")) + self.assertContains(response, "a****n@example.com") + self.assertNotContains(response, "ashwin@example.com") + + def test_verify_page_does_not_500_with_no_email_factor_enrolled(self): + """Minor finding 7: a hand-typed GET /verify/email/ from a user who + never enrolled the email factor gets begin_verify()'s + {"address": None} (no `code_length` either -- see begin_verify's + early return). Without the {% if address %} guard verify_email.html + mirrors from enroll_email.html, this used to render "We've emailed a + code to None", maxlength="" and a "-digit code" label instead of + failing safely. + """ + User.objects.create_user( + "noemailfactor", email="noemailfactor@example.com", password="pw") + client = Client() + client.login(username="noemailfactor", password="pw") + + response = client.get(reverse("mfa:verify_factor", args=["email"])) + + self.assertEqual(response.status_code, 200) + self.assertNotContains(response, "We've emailed a code to None") + self.assertNotContains(response, "emailed a code to None") + self.assertNotContains(response, 'maxlength=""') + self.assertEqual(mail.outbox, []) + + +class RegistrationTests(TestCase): + def test_email_is_not_registered_by_default(self): + """An existing install must not silently acquire a mail-sending + factor on upgrade.""" + from django_mfa.conf import DEFAULTS + + self.assertNotIn("email", DEFAULTS["MFA_FACTORS"]) + + def test_it_registers_when_asked_for(self): + from django_mfa.adapters import register_default_adapters + from django_mfa.registry import Registry + + registry = Registry() + register_default_adapters(registry, ["totp", "email"]) + self.assertEqual([a.type for a in registry.all()], ["totp", "email"]) diff --git a/django_mfa/tests/test_decorators.py b/django_mfa/tests/test_decorators.py new file mode 100644 index 0000000..27e9359 --- /dev/null +++ b/django_mfa/tests/test_decorators.py @@ -0,0 +1,143 @@ +from django.conf import settings as django_settings +from django.contrib.auth.models import User +from django.http import HttpResponse +from django.test import Client, TestCase, override_settings +from django.urls import include, path, reverse +from django.views.generic import View + +from django_mfa.decorators import MfaRequiredMixin, mfa_required +from django_mfa.models import Authenticator + + +@mfa_required +def protected(request): + return HttpResponse("protected") + + +class ProtectedView(MfaRequiredMixin, View): + def get(self, request): + return HttpResponse("protected cbv") + + +urlpatterns = [ + path("protected/", protected, name="protected"), + path("protected-cbv/", ProtectedView.as_view(), name="protected_cbv"), + path("", include("django_mfa.urls")), +] + +# MfaMiddleware is also installed (see test_runner.py's MIDDLEWARE) and +# would redirect a pending user itself, before the view -- and therefore +# before mfa_required/MfaRequiredMixin's own _enforce() -- ever runs, since +# neither /protected/ nor /protected-cbv/ is in its exempt_paths(). The +# pending-rung tests below need that middleware out of the stack, or they +# cannot tell _enforce()'s own pending check apart from the middleware's. +_MIDDLEWARE_WITHOUT_MFA = [ + m for m in django_settings.MIDDLEWARE + if m != "django_mfa.middleware.MfaMiddleware" +] + + +@override_settings(ROOT_URLCONF="django_mfa.tests.test_decorators", + LOGIN_URL="/login/") +class MfaRequiredDecoratorTests(TestCase): + url_name = "protected" + + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + self.client = Client() + + def url(self): + return reverse(self.url_name) + + def test_anonymous_goes_to_the_login_page(self): + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 302) + self.assertIn("/login/", response.url) + + def test_pending_user_goes_to_the_verify_picker(self): + Authenticator.objects.create(user=self.user, type="totp") + self.client.login(username="a@example.com", password="pw") + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:verify"), response.url) + + def test_user_with_no_factor_goes_to_the_security_page(self): + self.client.login(username="a@example.com", password="pw") + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:security_settings"), response.url) + + def test_recovery_codes_alone_are_not_enough(self): + Authenticator.objects.create(user=self.user, type="recovery_codes") + self.client.login(username="a@example.com", password="pw") + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:security_settings"), response.url) + + def test_verified_user_reaches_the_view(self): + Authenticator.objects.create(user=self.user, type="totp") + self.client.login(username="a@example.com", password="pw") + session = self.client.session + session["mfa"] = {"verified": True, "method": "totp", "at": 0} + session.save() + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 200) + + def test_next_is_preserved(self): + self.client.login(username="a@example.com", password="pw") + response = self.client.get(self.url()) + self.assertIn("next=", response.url) + + def test_the_decorator_keeps_the_view_name(self): + self.assertEqual(protected.__name__, "protected") + + +@override_settings(ROOT_URLCONF="django_mfa.tests.test_decorators", + LOGIN_URL="/login/") +class MfaRequiredMixinTests(MfaRequiredDecoratorTests): + """The mixin must behave identically to the decorator, so it reruns the + whole case list against the class-based view.""" + + url_name = "protected_cbv" + + def test_the_decorator_keeps_the_view_name(self): + self.skipTest("not applicable to the mixin") + + +@override_settings(ROOT_URLCONF="django_mfa.tests.test_decorators", + LOGIN_URL="/login/", MIDDLEWARE=_MIDDLEWARE_WITHOUT_MFA) +class PendingRungIsolatedFromMiddlewareTests(TestCase): + """_enforce()'s pending rung, proven independent of MfaMiddleware. + + MfaRequiredDecoratorTests.test_pending_user_goes_to_the_verify_picker + runs with MfaMiddleware still installed, so it cannot tell _enforce()'s + own pending check apart from the middleware's identical one: the + middleware redirects first and the view is never reached. Removing + MfaMiddleware from MIDDLEWARE for this class means the redirect can + only come from the decorator/mixin itself. + """ + + url_name = "protected" + + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + self.client = Client() + + def url(self): + return reverse(self.url_name) + + def test_pending_user_goes_to_the_verify_picker(self): + Authenticator.objects.create(user=self.user, type="totp") + self.client.login(username="a@example.com", password="pw") + response = self.client.get(self.url()) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:verify"), response.url) + + +@override_settings(ROOT_URLCONF="django_mfa.tests.test_decorators", + LOGIN_URL="/login/", MIDDLEWARE=_MIDDLEWARE_WITHOUT_MFA) +class MixinPendingRungIsolatedFromMiddlewareTests( + PendingRungIsolatedFromMiddlewareTests): + """Same proof, against the mixin's class-based view.""" + + url_name = "protected_cbv" diff --git a/django_mfa/tests/test_enforcement.py b/django_mfa/tests/test_enforcement.py index e7dcafe..6425d2f 100644 --- a/django_mfa/tests/test_enforcement.py +++ b/django_mfa/tests/test_enforcement.py @@ -2,10 +2,13 @@ from django.contrib.auth.models import User from django.contrib.sessions.backends.db import SessionStore from django.core.cache import cache +from django.db import connection from django.test import Client, RequestFactory, TestCase, override_settings +from django.test.utils import CaptureQueriesContext from django.urls import reverse from django_mfa import totp as totp_mod +from django_mfa.adapters.totp import generate_secret from django_mfa.models import Authenticator @@ -274,3 +277,155 @@ class CustomNonPrimaryAdapter(Adapter): client = Client() client.login(username="d@example.com", password="pw") self.assertNotIn("mfa", client.session) + + +class PendingUserCannotReachEnrollmentTests(TestCase): + """A pending user must never reach an enroll page. + + enroll_factor() calls session.mark_verified() on success -- correct on + its own terms, since enrolling proves possession. But it means a user who + is mid-challenge could enroll a *fresh* TOTP with a secret of their own + choosing and be marked verified without ever presenting the factor they + already hold. The pending exempt set and the enrollment exempt set are + therefore different sets, and unioning them is a vulnerability. + + This passes against the code as it was before the enrollment wall existed + and must keep passing after. + """ + + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + Authenticator.objects.create(user=self.user, type="totp") + self.client = Client() + self.client.login(username="a@example.com", password="pw") + + def test_enroll_page_redirects_a_pending_user_to_verify(self): + response = self.client.get(reverse("mfa:enroll_factor", args=["totp"])) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:verify"), response.url) + + def test_enrolling_cannot_be_posted_by_a_pending_user(self): + secret = totp_mod.TOTP(generate_secret()) + response = self.client.post( + reverse("mfa:enroll_factor", args=["totp"]), + {"secret_key": secret.secret, "code": secret.now()}) + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:verify"), response.url) + self.assertFalse(self.client.session["mfa"]["verified"]) + + +@override_settings(MFA_REQUIRED=True) +class EnrollmentWallTests(TestCase): + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + self.client = Client() + self.client.login(username="a@example.com", password="pw") + + def test_a_required_user_with_no_factors_is_walled(self): + response = self.client.get("/some/other/page/") + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:security_settings"), response.url) + + def test_the_wall_records_next_in_the_redirect_url(self): + """Only that the parameter is *recorded*, not that anything reads it + back -- neither enroll_factor nor security_settings look at `next`, + so a user who enrolls here does not land back on this page. See the + "What a required user sees" section of docs/enforcement.md. + """ + response = self.client.get("/some/other/page/") + # django.contrib.auth.views.redirect_to_login builds the querystring + # via QueryDict.urlencode(safe="/"), which deliberately leaves "/" + # unescaped -- so this is "/some/other/page/", not the %2F-escaped + # form. Confirmed against the real redirect: assert on the decoded + # form actually produced rather than a hand-guessed encoding. + self.assertIn("next=/some/other/page/", response.url) + + def test_security_settings_itself_is_reachable(self): + response = self.client.get(reverse("mfa:security_settings")) + self.assertEqual(response.status_code, 200) + + def test_enroll_pages_are_reachable(self): + response = self.client.get(reverse("mfa:enroll_factor", args=["totp"])) + self.assertEqual(response.status_code, 200) + + def test_recovery_codes_page_is_reachable(self): + response = self.client.get(reverse("mfa:recovery_codes")) + self.assertEqual(response.status_code, 200) + + @override_settings(MFA_EXEMPT_PATHS=["/logout/"]) + def test_exempt_paths_apply_to_the_wall_too(self): + """Without this a required user who cannot enroll -- no phone, no + security key -- is trapped with no way even to log out.""" + response = self.client.get("/logout/") + self.assertNotEqual(response.status_code, 302) + + def test_recovery_codes_alone_do_not_satisfy_the_requirement(self): + Authenticator.objects.create(user=self.user, type="recovery_codes") + response = self.client.get("/some/other/page/") + self.assertEqual(response.status_code, 302) + self.assertIn(reverse("mfa:security_settings"), response.url) + + def test_a_primary_factor_releases_the_wall(self): + Authenticator.objects.create(user=self.user, type="totp") + session = self.client.session + session["mfa"] = {"verified": True, "method": "totp", "at": 0} + session.save() + response = self.client.get("/some/other/page/") + self.assertEqual(response.status_code, 404) # walled off -> not found + + def test_the_security_page_explains_why(self): + response = self.client.get(reverse("mfa:security_settings")) + self.assertTrue(response.context["mfa_enrollment_required"]) + + +class WallIsOffByDefaultTests(TestCase): + def test_an_unrequired_user_with_no_factors_is_untouched(self): + User.objects.create_user("b@example.com", password="pw") + client = Client() + client.login(username="b@example.com", password="pw") + response = client.get("/some/other/page/") + self.assertEqual(response.status_code, 404) + + +class MfaRequiredQueryCostTests(TestCase): + """Important 4, end-to-end: the reviewer measured 2 queries for a + fully-enrolled, verified user requesting an unrelated page with + MFA_REQUIRED=False, and 5 with it on -- +3, one .exists() query per + registered adapter (totp, webauthn, recovery_codes here), from + MfaMiddleware's second rung calling registry.primary_enabled_for() where + only the yes/no answer was ever needed. + + registry.has_primary_factor() (Important 4's fix) must bring that delta + down to +1 -- has_primary_factor's own single .exists() query -- no + matter how many adapters are registered, since MFA_REQUIRED=True is the + only thing that makes the middleware's second rung run at all. + """ + + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + for factor_type in ("totp", "webauthn", "recovery_codes"): + Authenticator.objects.create(user=self.user, type=factor_type) + self.client = Client() + self.client.login(username="a@example.com", password="pw") + session = self.client.session + session["mfa"] = {"verified": True, "method": "totp", "at": 0} + session.save() + + def test_requiring_mfa_costs_exactly_one_extra_query(self): + with override_settings(MFA_REQUIRED=False): + with CaptureQueriesContext(connection) as unrequired: + response = self.client.get("/some/other/page/") + self.assertEqual(response.status_code, 404) + + with override_settings(MFA_REQUIRED=True): + with CaptureQueriesContext(connection) as required: + response = self.client.get("/some/other/page/") + self.assertEqual(response.status_code, 404) + + delta = len(required.captured_queries) - len(unrequired.captured_queries) + self.assertEqual( + delta, 1, + "MFA_REQUIRED=True should cost exactly one extra query over " + "MFA_REQUIRED=False (has_primary_factor's own .exists()), not " + "one per registered adapter. Queries when required: " + f"{[q['sql'] for q in required.captured_queries]}") diff --git a/django_mfa/tests/test_events.py b/django_mfa/tests/test_events.py new file mode 100644 index 0000000..516d5d5 --- /dev/null +++ b/django_mfa/tests/test_events.py @@ -0,0 +1,221 @@ +from django.contrib.auth.models import User +from django.core.cache import cache +from django.test import Client, TestCase +from django.urls import reverse + +from django_mfa import events +from django_mfa import totp as totp_mod +from django_mfa.adapters.recovery_codes import RecoveryCodesAdapter +from django_mfa.adapters.totp import generate_secret +from django_mfa.crypto import encrypt +from django_mfa.models import Authenticator +from django_mfa.registry import registry + + +class SignalRecorder: + """Collect every kwargs dict a signal is sent with. + + Connected with a strong reference held by the test, so the receiver is + not garbage-collected mid-test the way a bare local function would be. + """ + + def __init__(self, signal): + self.signal = signal + self.calls = [] + signal.connect(self) + + def __call__(self, sender, **kwargs): + self.calls.append({"sender": sender, **kwargs}) + + def disconnect(self): + self.signal.disconnect(self) + + +class EventTestCase(TestCase): + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + self.client = Client() + self.recorders = [] + + def tearDown(self): + for recorder in self.recorders: + recorder.disconnect() + + def record(self, signal): + recorder = SignalRecorder(signal) + self.recorders.append(recorder) + return recorder + + def enroll_totp(self): + secret = generate_secret() + Authenticator.objects.create( + user=self.user, type="totp", data={"secret": encrypt(secret)}) + return secret + + def verified_login(self): + self.client.login(username="a@example.com", password="pw") + session = self.client.session + session["mfa"] = {"verified": True, "method": "totp", "at": 0} + session.save() + + +class FactorAddedTests(EventTestCase): + def test_enrolling_totp_emits_factor_added(self): + recorder = self.record(events.factor_added) + self.client.login(username="a@example.com", password="pw") + secret = generate_secret() + response = self.client.post( + reverse("mfa:enroll_factor", args=["totp"]), + {"secret_key": secret, "code": totp_mod.TOTP(secret).now()}) + self.assertEqual(response.status_code, 302) + + self.assertEqual(len(recorder.calls), 1) + call = recorder.calls[0] + self.assertEqual(call["user"], self.user) + self.assertEqual(call["authenticator"].type, "totp") + self.assertIsNotNone(call["request"]) + + def test_generating_recovery_codes_emits_factor_added(self): + self.enroll_totp() + self.verified_login() + recorder = self.record(events.factor_added) + self.client.get(reverse("mfa:recovery_codes")) + + self.assertEqual(len(recorder.calls), 1) + self.assertEqual(recorder.calls[0]["authenticator"].type, "recovery_codes") + + def test_a_failed_enrollment_emits_nothing(self): + recorder = self.record(events.factor_added) + self.client.login(username="a@example.com", password="pw") + self.client.post(reverse("mfa:enroll_factor", args=["totp"]), + {"secret_key": generate_secret(), "code": "000000"}) + self.assertEqual(recorder.calls, []) + + +class FactorRemovedTests(EventTestCase): + def test_removal_carries_the_type_and_name_of_the_deleted_row(self): + auth = Authenticator.objects.create( + user=self.user, type="webauthn", name="Yubikey 5C", data={}) + self.enroll_totp() + self.verified_login() + recorder = self.record(events.factor_removed) + + self.client.post(reverse("mfa:manage"), {"pk": auth.pk}) + + self.assertEqual(len(recorder.calls), 1) + call = recorder.calls[0] + self.assertEqual(call["factor_type"], "webauthn") + self.assertEqual(call["name"], "Yubikey 5C") + self.assertFalse(Authenticator.objects.filter(pk=auth.pk).exists()) + + def test_removing_a_row_whose_type_is_no_longer_registered_still_works(self): + """A row can outlive its adapter's registration -- MFA_FACTORS + narrowed, or registry.unregister() (the WebAuthn opt-out checks.py + itself recommends). security_settings lists every row regardless of + the registry, so removing one of these is a supported action and + must not 500 after the delete has already committed.""" + adapter = registry.get("webauthn") + registry.unregister("webauthn") + self.addCleanup(registry.register, adapter) + + auth = Authenticator.objects.create( + user=self.user, type="webauthn", name="Orphaned key", data={}) + self.enroll_totp() + self.verified_login() + recorder = self.record(events.factor_removed) + + response = self.client.post(reverse("mfa:manage"), {"pk": auth.pk}) + + self.assertEqual(response.status_code, 302) + self.assertFalse(Authenticator.objects.filter(pk=auth.pk).exists()) + self.assertEqual(len(recorder.calls), 1) + self.assertIsNone(recorder.calls[0]["sender"]) + + +class VerificationTests(EventTestCase): + def setUp(self): + super().setUp() + cache.clear() + + def test_success_emits_mfa_verified(self): + secret = self.enroll_totp() + self.client.login(username="a@example.com", password="pw") + recorder = self.record(events.mfa_verified) + + self.client.post(reverse("mfa:verify_factor", args=["totp"]), + {"code": totp_mod.TOTP(secret).now()}) + + self.assertEqual(len(recorder.calls), 1) + self.assertEqual(recorder.calls[0]["method"], "totp") + + def test_wrong_code_emits_mfa_verification_failed(self): + self.enroll_totp() + self.client.login(username="a@example.com", password="pw") + recorder = self.record(events.mfa_verification_failed) + + self.client.post(reverse("mfa:verify_factor", args=["totp"]), + {"code": "000000"}) + + self.assertEqual(len(recorder.calls), 1) + self.assertEqual(recorder.calls[0]["method"], "totp") + + def test_missing_field_emits_mfa_verification_failed(self): + """The caught KeyError path, not just an ordinary wrong code.""" + self.enroll_totp() + self.client.login(username="a@example.com", password="pw") + recorder = self.record(events.mfa_verification_failed) + + self.client.post(reverse("mfa:verify_factor", args=["totp"]), {}) + + self.assertEqual(len(recorder.calls), 1) + + def test_rate_limited_attempt_still_emits_mfa_verification_failed(self): + """A refused attempt is exactly what a brute-force detector needs to + see. It must not change the response, which stays the same generic + 400 a wrong code gets.""" + self.enroll_totp() + self.client.login(username="a@example.com", password="pw") + url = reverse("mfa:verify_factor", args=["totp"]) + for _ in range(5): + self.client.post(url, {"code": "000000"}) + + recorder = self.record(events.mfa_verification_failed) + response = self.client.post(url, {"code": "000000"}) + + self.assertEqual(response.status_code, 400) + self.assertEqual(len(recorder.calls), 1) + cache.clear() + + +class RecoveryCodeTests(EventTestCase): + def test_spending_a_code_reports_how_many_remain(self): + codes = RecoveryCodesAdapter().generate(self.user) + self.enroll_totp() + self.client.login(username="a@example.com", password="pw") + recorder = self.record(events.recovery_code_used) + + self.client.post(reverse("mfa:verify_factor", args=["recovery_codes"]), + {"code": codes[0]}) + + self.assertEqual(len(recorder.calls), 1) + self.assertEqual(recorder.calls[0]["remaining"], 9) + + +class RobustnessTests(EventTestCase): + def test_a_raising_receiver_does_not_break_the_view(self): + """A host project's buggy receiver must never be able to stop a user + removing an authenticator they believe is compromised. Every + in-request emission uses send_robust() for exactly this.""" + def boom(sender, **kwargs): + raise RuntimeError("receiver is broken") + + auth = Authenticator.objects.create(user=self.user, type="webauthn", data={}) + self.enroll_totp() + self.verified_login() + events.factor_removed.connect(boom) + self.addCleanup(events.factor_removed.disconnect, boom) + + response = self.client.post(reverse("mfa:manage"), {"pk": auth.pk}) + + self.assertEqual(response.status_code, 302) + self.assertFalse(Authenticator.objects.filter(pk=auth.pk).exists()) diff --git a/django_mfa/tests/test_migrations.py b/django_mfa/tests/test_migrations.py index 4833014..14cb974 100644 --- a/django_mfa/tests/test_migrations.py +++ b/django_mfa/tests/test_migrations.py @@ -304,5 +304,25 @@ def test_forward_from_zero_still_applies_through_0007(self): "0005_migrate_to_authenticator", "0006_mfa_user_handle", "0007_drop_legacy_models", + "0008_email_factor", }, ) + + +class NoMissingMigrationsTests(TestCase): + """A model change without a migration is invisible until somebody + deploys. There is no manage.py here (test_runner.py calls + settings.configure()), so the command is driven through call_command.""" + + def test_makemigrations_has_nothing_to_do(self): + from io import StringIO + + from django.core.management import call_command + + out = StringIO() + try: + call_command("makemigrations", "django_mfa", check=True, + dry_run=True, stdout=out, verbosity=1) + except SystemExit: + self.fail(f"django_mfa has model changes with no migration:\n" + f"{out.getvalue()}") diff --git a/django_mfa/tests/test_models.py b/django_mfa/tests/test_models.py index dd4a15f..52f5702 100644 --- a/django_mfa/tests/test_models.py +++ b/django_mfa/tests/test_models.py @@ -1,5 +1,5 @@ from django.contrib.auth.models import User -from django.db.utils import IntegrityError +from django.db import IntegrityError, transaction from django.test import TestCase from django_mfa.models import Authenticator @@ -55,3 +55,37 @@ def test_authenticator_is_the_only_registered_model(self): names = {m.__name__ for m in apps.get_app_config("django_mfa").get_models()} self.assertEqual(names, {"Authenticator", "MfaUserHandle"}) + + +class EmailIsASingletonFactorTests(TestCase): + """supports_multiple = False is an adapter-level promise; this is the + database keeping it. Without the constraint covering "email", a second row + races the adapter's get_instances(user).first() and which address receives + the code becomes non-deterministic.""" + + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + + def test_a_second_email_authenticator_is_rejected(self): + Authenticator.objects.create( + user=self.user, type="email", data={"address": "a@example.com"}) + with self.assertRaises(IntegrityError): + with transaction.atomic(): + Authenticator.objects.create( + user=self.user, type="email", + data={"address": "other@example.com"}) + + def test_two_users_may_each_have_one(self): + other = User.objects.create_user("b@example.com", password="pw") + Authenticator.objects.create(user=self.user, type="email", data={}) + Authenticator.objects.create(user=other, type="email", data={}) + self.assertEqual(Authenticator.objects.filter(type="email").count(), 2) + + def test_webauthn_is_still_allowed_to_repeat(self): + Authenticator.objects.create(user=self.user, type="webauthn", data={}) + Authenticator.objects.create(user=self.user, type="webauthn", data={}) + self.assertEqual(Authenticator.objects.filter(type="webauthn").count(), 2) + + def test_email_is_a_declared_choice(self): + self.assertEqual(Authenticator.Type.EMAIL, "email") + self.assertIn("email", [value for value, _ in Authenticator.Type.choices]) diff --git a/django_mfa/tests/test_notifications.py b/django_mfa/tests/test_notifications.py new file mode 100644 index 0000000..14def1c --- /dev/null +++ b/django_mfa/tests/test_notifications.py @@ -0,0 +1,154 @@ +from unittest import mock + +from django.contrib.auth.models import User +from django.core import mail +from django.test import Client, TestCase, override_settings +from django.urls import reverse + +from django_mfa import events +from django_mfa.adapters.recovery_codes import RecoveryCodesAdapter +from django_mfa.adapters.totp import generate_secret +from django_mfa.crypto import encrypt +from django_mfa.models import Authenticator + + +class NotificationTestCase(TestCase): + def setUp(self): + self.user = User.objects.create_user( + "ashwin", email="ashwin@example.com", password="pw") + self.client = Client() + + def verified_login(self): + Authenticator.objects.create( + user=self.user, type="totp", + data={"secret": encrypt(generate_secret())}) + self.client.login(username="ashwin", password="pw") + session = self.client.session + session["mfa"] = {"verified": True, "method": "totp", "at": 0} + session.save() + + +class OffByDefaultTests(NotificationTestCase): + def test_nothing_is_sent_by_default(self): + """An upgrade must not start mailing a host project's users through a + backend django-mfa doesn't control.""" + self.verified_login() + self.client.get(reverse("mfa:recovery_codes")) + self.assertEqual(mail.outbox, []) + + def test_the_signal_still_fires(self): + """Signals are the always-on half. A project wanting async delivery + connects its own receiver and leaves MFA_NOTIFY_ON_CHANGE off.""" + calls = [] + + def receiver(sender, **kw): + calls.append(kw) + + # weak=False: Django holds receivers by weak reference by default, + # and a receiver with no other strong reference can be garbage + # collected before the signal fires -- making this test pass or fail + # on GC timing rather than on the behaviour it's checking. + events.factor_added.connect(receiver, weak=False) + self.addCleanup(events.factor_added.disconnect, receiver) + self.verified_login() + self.client.get(reverse("mfa:recovery_codes")) + self.assertEqual(len(calls), 1) + + def test_removing_a_factor_does_not_touch_the_registry_when_off(self): + """Minor finding 6: notify_factor_removed used to call + registry.primary_enabled_for(user) (3 queries) unconditionally, even + with MFA_NOTIFY_ON_CHANGE off -- _notify() would immediately discard + the mfa_disabled context built from it, but the query already ran. + The early return at the top of the receiver must make a default + install exactly as cheap as before notifications existed: the + registry is never even consulted. + """ + self.verified_login() + webauthn = Authenticator.objects.create( + user=self.user, type="webauthn", data={}) + + with mock.patch( + "django_mfa.registry.registry.has_primary_factor") as mocked: + self.client.post(reverse("mfa:manage"), {"pk": webauthn.pk}) + + mocked.assert_not_called() + + +@override_settings(MFA_NOTIFY_ON_CHANGE=True, + DEFAULT_FROM_EMAIL="security@example.com") +class NotificationContentTests(NotificationTestCase): + def test_adding_a_factor_notifies(self): + self.verified_login() + mail.outbox.clear() + self.client.get(reverse("mfa:recovery_codes")) + + self.assertEqual(len(mail.outbox), 1) + message = mail.outbox[0] + self.assertEqual(message.to, ["ashwin@example.com"]) + self.assertEqual(message.from_email, "security@example.com") + self.assertIn("Recovery codes", message.body) + + def test_removing_one_of_two_factors_notifies_removal_only(self): + self.verified_login() + extra = Authenticator.objects.create( + user=self.user, type="webauthn", name="Yubikey", data={}) + mail.outbox.clear() + + self.client.post(reverse("mfa:manage"), {"pk": extra.pk}) + + self.assertEqual(len(mail.outbox), 1) + self.assertIn("Yubikey", mail.outbox[0].body) + + def test_removing_the_last_primary_factor_also_says_mfa_is_off(self): + self.verified_login() + only = Authenticator.objects.get(user=self.user, type="totp") + mail.outbox.clear() + + self.client.post(reverse("mfa:manage"), {"pk": only.pk}) + + self.assertEqual(len(mail.outbox), 2) + bodies = " ".join(m.body for m in mail.outbox) + self.assertIn("no longer", bodies) + + def test_spending_a_recovery_code_reports_the_remaining_count(self): + codes = RecoveryCodesAdapter().generate(self.user) + Authenticator.objects.create( + user=self.user, type="totp", + data={"secret": encrypt(generate_secret())}) + self.client.login(username="ashwin", password="pw") + mail.outbox.clear() + + self.client.post(reverse("mfa:verify_factor", args=["recovery_codes"]), + {"code": codes[0]}) + + self.assertEqual(len(mail.outbox), 1) + self.assertIn("9", mail.outbox[0].body) + + def test_a_user_with_no_address_is_skipped(self): + self.user.email = "" + self.user.save() + self.verified_login() + mail.outbox.clear() + + self.client.get(reverse("mfa:recovery_codes")) + + self.assertEqual(mail.outbox, []) + + +@override_settings(MFA_NOTIFY_ON_CHANGE=True) +class MailFailureTests(NotificationTestCase): + def test_a_broken_mail_backend_does_not_block_the_security_action(self): + """Removing a key you believe is compromised is more urgent than the + notification about it.""" + self.verified_login() + extra = Authenticator.objects.create( + user=self.user, type="webauthn", data={}) + + with mock.patch("django_mfa.notifications.send_mail", + side_effect=OSError("smtp is down")): + with self.assertLogs("django_mfa.notifications", "ERROR"): + response = self.client.post(reverse("mfa:manage"), + {"pk": extra.pk}) + + self.assertEqual(response.status_code, 302) + self.assertFalse(Authenticator.objects.filter(pk=extra.pk).exists()) diff --git a/django_mfa/tests/test_passwordless.py b/django_mfa/tests/test_passwordless.py index ebc614b..5a23989 100644 --- a/django_mfa/tests/test_passwordless.py +++ b/django_mfa/tests/test_passwordless.py @@ -40,6 +40,7 @@ from django.urls import reverse from fido2.webauthn import AuthenticatorData +from django_mfa import events from django_mfa.backends import user_from_handle, user_handle_for from django_mfa.conf import settings as mfa_settings from django_mfa.middleware import MfaMiddleware @@ -145,6 +146,21 @@ def _enroll(self): def _handle_bytes(self, user): return user_handle_for(user).encode("utf-8") + def _record(self, signal): + """Collect every kwargs dict `signal` is sent with, for the duration + of one test. Mirrors test_events.SignalRecorder's shape (strong + reference, connect/disconnect) without importing that module's + Client-driven EventTestCase fixtures, which this file doesn't use. + """ + calls = [] + + def receiver(sender, **kwargs): + calls.append({"sender": sender, **kwargs}) + + signal.connect(receiver) + self.addCleanup(signal.disconnect, receiver) + return calls + # -- Core ceremony: property 2 (a valid assertion actually authenticates) -- def test_passkey_login_authenticates_without_a_password(self): @@ -351,6 +367,50 @@ def test_up_only_assertion_authenticates_but_leaves_second_factor_pending(self): # factor before letting them reach anything non-exempt. self.assertFalse(self.client.session["mfa"]["verified"]) + def test_uv_assertion_emits_mfa_verified(self): + """Review Important 2: a UV-carrying passkey login genuinely + satisfies the second factor (see + test_uv_assertion_marks_the_session_fully_verified above) and must + emit the same mfa_verified signal verify_factor's own success branch + does -- a host project doing audit logging off this signal would + otherwise have a silent hole for the strongest login path. + """ + calls = self._record(events.mfa_verified) + begin = self.client.get(reverse("mfa:passkey_begin")) + assertion = self.device.get(json.loads(begin.json()["options"])) + + response = self.client.post(reverse("mfa:passkey_complete"), + {"credential": json.dumps(assertion)}) + + self.assertEqual(response.status_code, 302) + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0]["user"], self.user) + self.assertEqual(calls[0]["method"], "webauthn") + self.assertIsNotNone(calls[0]["request"]) + + def test_up_only_assertion_does_not_emit_mfa_verified(self): + """The other half of Important 2: a UP-only passkey login logs the + user in but does NOT satisfy the second factor (see + test_up_only_assertion_authenticates_but_leaves_second_factor_pending + above), so it must not claim it did by emitting this signal either. + """ + up_only_device = _UpOnlyAuthenticator(origin="https://testserver", + rp_id="testserver") + up_only_device.private_key = self.device.private_key + up_only_device.credential_id = self.device.credential_id + up_only_device.sign_count = self.device.sign_count + + calls = self._record(events.mfa_verified) + begin = self.client.get(reverse("mfa:passkey_begin")) + assertion = up_only_device.get(json.loads(begin.json()["options"]), + user_handle=self._handle_bytes(self.user)) + response = self.client.post(reverse("mfa:passkey_complete"), + {"credential": json.dumps(assertion)}) + + self.assertEqual(response.status_code, 302) + self.assertFalse(self.client.session["mfa"]["verified"]) + self.assertEqual(calls, []) + # -- MFA_QUICKLOGIN rides on this same view; confirm it still works end # to end when enabled (unit-level coverage lives in QuickLoginTests # below). diff --git a/django_mfa/tests/test_policy.py b/django_mfa/tests/test_policy.py new file mode 100644 index 0000000..7025c1b --- /dev/null +++ b/django_mfa/tests/test_policy.py @@ -0,0 +1,110 @@ +from django.contrib.auth.models import Group, User +from django.core.exceptions import ImproperlyConfigured +from django.test import TestCase, override_settings + +from django_mfa import policy +from django_mfa.checks import check_mfa_required_predicate + + +def everyone(user): + return True + + +def nobody(user): + return False + + +not_callable = "this is a string, not a function" + + +class MfaRequiredForTests(TestCase): + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + + def test_default_requires_nobody(self): + self.assertFalse(policy.mfa_required_for(self.user)) + + @override_settings(MFA_REQUIRED=True) + def test_true_requires_every_authenticated_user(self): + self.assertTrue(policy.mfa_required_for(self.user)) + + @override_settings(MFA_REQUIRED=True) + def test_anonymous_is_never_required(self): + from django.contrib.auth.models import AnonymousUser + self.assertFalse(policy.mfa_required_for(AnonymousUser())) + + @override_settings(MFA_REQUIRED="django_mfa.tests.test_policy.everyone") + def test_dotted_path_is_imported_and_called(self): + self.assertTrue(policy.mfa_required_for(self.user)) + + @override_settings(MFA_REQUIRED="django_mfa.tests.test_policy.nobody") + def test_dotted_path_returning_false(self): + self.assertFalse(policy.mfa_required_for(self.user)) + + @override_settings(MFA_REQUIRED=everyone) + def test_a_plain_callable_works_too(self): + self.assertTrue(policy.mfa_required_for(self.user)) + + +class SuppliedPredicateTests(TestCase): + def setUp(self): + self.user = User.objects.create_user("a@example.com", password="pw") + + @override_settings(MFA_REQUIRED=policy.is_staff) + def test_is_staff(self): + self.assertFalse(policy.mfa_required_for(self.user)) + self.user.is_staff = True + self.assertTrue(policy.mfa_required_for(self.user)) + + def test_in_groups_matches_only_named_groups(self): + predicate = policy.in_groups("admins", "finance") + with override_settings(MFA_REQUIRED=predicate): + self.assertFalse(policy.mfa_required_for(self.user)) + self.user.groups.add(Group.objects.create(name="marketing")) + self.assertFalse(policy.mfa_required_for(self.user)) + self.user.groups.add(Group.objects.create(name="finance")) + self.assertTrue(policy.mfa_required_for(self.user)) + + def test_in_groups_costs_one_query(self): + predicate = policy.in_groups("admins") + with self.assertNumQueries(1): + predicate(self.user) + + +class CheckE004Tests(TestCase): + def test_default_passes(self): + self.assertEqual(check_mfa_required_predicate(None), []) + + @override_settings(MFA_REQUIRED="django_mfa.tests.test_policy.everyone") + def test_valid_dotted_path_passes(self): + self.assertEqual(check_mfa_required_predicate(None), []) + + @override_settings(MFA_REQUIRED="django_mfa.nonexistent.predicate") + def test_unimportable_path_is_an_error(self): + errors = check_mfa_required_predicate(None) + self.assertEqual([e.id for e in errors], ["django_mfa.E004"]) + + @override_settings(MFA_REQUIRED="django_mfa.tests.test_policy.not_callable") + def test_importable_but_not_callable_is_an_error(self): + errors = check_mfa_required_predicate(None) + self.assertEqual([e.id for e in errors], ["django_mfa.E004"]) + + @override_settings(MFA_REQUIRED=42) + def test_nonsense_value_is_an_error(self): + errors = check_mfa_required_predicate(None) + self.assertEqual([e.id for e in errors], ["django_mfa.E004"]) + + @override_settings(MFA_REQUIRED="django_mfa.nonexistent.predicate") + def test_the_check_is_not_gated_on_webauthn(self): + """E001-E003 are WebAuthn-only and skip for a TOTP-only project. + E004 is not WebAuthn-specific and must fire regardless.""" + with override_settings(MFA_QUICKLOGIN=False): + errors = check_mfa_required_predicate(None) + self.assertEqual([e.id for e in errors], ["django_mfa.E004"]) + + +class ResolveTests(TestCase): + @override_settings(MFA_REQUIRED=42) + def test_resolve_raises_on_a_nonsense_value(self): + with self.assertRaises(ImproperlyConfigured): + policy.resolve() diff --git a/django_mfa/tests/test_ratelimit.py b/django_mfa/tests/test_ratelimit.py index a9925ce..e6f6d0b 100644 --- a/django_mfa/tests/test_ratelimit.py +++ b/django_mfa/tests/test_ratelimit.py @@ -165,3 +165,42 @@ def test_the_window_is_still_applied_to_a_freshly_seeded_counter(self): self.assertEqual(self._count(), 1) if ttl is not None: self.assertGreater(ttl, 0) + + +class AlternateScopeTests(TestCase): + """A second budget, under a different setting, that cannot collide with + the verification budget for the same user.""" + + def setUp(self): + cache.clear() + self.user = User.objects.create_user("a@example.com", password="pw") + + @override_settings(MFA_EMAIL_SEND_RATE_LIMIT="2/5m") + def test_a_named_setting_supplies_the_limit(self): + setting = "MFA_EMAIL_SEND_RATE_LIMIT" + self.assertTrue(ratelimit.check(self.user, "email:send", setting=setting)) + ratelimit.record(self.user, "email:send", setting=setting) + self.assertTrue(ratelimit.check(self.user, "email:send", setting=setting)) + ratelimit.record(self.user, "email:send", setting=setting) + self.assertFalse(ratelimit.check(self.user, "email:send", setting=setting)) + + @override_settings(MFA_EMAIL_SEND_RATE_LIMIT="1/5m", + MFA_VERIFY_RATE_LIMIT="5/5m") + def test_scopes_do_not_share_a_counter(self): + ratelimit.record(self.user, "email:send", + setting="MFA_EMAIL_SEND_RATE_LIMIT") + self.assertFalse(ratelimit.check(self.user, "email:send", + setting="MFA_EMAIL_SEND_RATE_LIMIT")) + self.assertTrue(ratelimit.check(self.user, "email")) + + @override_settings(MFA_EMAIL_SEND_RATE_LIMIT="0/5m") + def test_the_error_names_the_setting_that_is_wrong(self): + with self.assertRaises(ValueError) as ctx: + ratelimit.check(self.user, "email:send", + setting="MFA_EMAIL_SEND_RATE_LIMIT") + self.assertIn("MFA_EMAIL_SEND_RATE_LIMIT", str(ctx.exception)) + self.assertNotIn("MFA_VERIFY_RATE_LIMIT", str(ctx.exception)) + + def test_record_failure_is_still_the_verify_budget(self): + """Existing callers (views/verify.py) are untouched by the refactor.""" + self.assertIs(ratelimit.record_failure, ratelimit.record) diff --git a/django_mfa/tests/test_registry.py b/django_mfa/tests/test_registry.py index 0fce29a..b838c87 100644 --- a/django_mfa/tests/test_registry.py +++ b/django_mfa/tests/test_registry.py @@ -96,6 +96,51 @@ def test_totp_is_enabled_and_primary(self): self.assertIn("totp", enabled) self.assertIn("totp", primary) + def test_has_primary_factor_agrees_with_primary_enabled_for(self): + """has_primary_factor() is the boolean counterpart to + primary_enabled_for() -- Important 4. The two must never disagree, + since counts_as_primary_factor on the adapters is the single source + of truth both derive their type list from. + """ + self.assertFalse(self.registry.has_primary_factor(self.user)) + self.assertEqual(self.registry.primary_enabled_for(self.user), []) + + # Recovery codes alone: enabled, but not primary -- must not flip + # has_primary_factor() to True. + Authenticator.objects.create(user=self.user, type="recovery_codes") + self.assertFalse(self.registry.has_primary_factor(self.user)) + self.assertEqual(self.registry.primary_enabled_for(self.user), []) + + # A real primary factor: both must now agree it's True. + Authenticator.objects.create(user=self.user, type="totp") + self.assertTrue(self.registry.has_primary_factor(self.user)) + self.assertTrue(self.registry.primary_enabled_for(self.user)) + + def test_has_primary_factor_runs_a_single_query(self): + """Important 4: primary_enabled_for() runs one .exists() query per + registered adapter (via enabled_for()), which on a fully-enrolled, + verified user cost +3 queries on every authenticated request under + MFA_REQUIRED=True. has_primary_factor() must cost exactly one query, + regardless of how many adapters are registered -- this registry has + three (totp, webauthn, recovery_codes). + """ + Authenticator.objects.create(user=self.user, type="totp") + with self.assertNumQueries(1): + self.registry.has_primary_factor(self.user) + + def test_primary_enabled_for_costs_one_query_per_adapter_by_contrast(self): + """Documents the exact cost has_primary_factor() replaces callers + away from, for the callers that only need the boolean: three + adapters registered here (totp, webauthn, recovery_codes) means + three .exists() queries, one per adapter, every time + primary_enabled_for() runs -- this is what MfaMiddleware, + the enforcement decorator, and the notifications module used to pay + on every relevant request before Important 4's fix. + """ + Authenticator.objects.create(user=self.user, type="totp") + with self.assertNumQueries(3): + self.registry.primary_enabled_for(self.user) + def test_unregister_removes_adapter(self): self.registry.unregister("totp") with self.assertRaises(KeyError): diff --git a/django_mfa/utils.py b/django_mfa/utils.py index 61f7ddb..4ff214e 100644 --- a/django_mfa/utils.py +++ b/django_mfa/utils.py @@ -91,3 +91,45 @@ def strings_equal(s1, s2): s1 = unicodedata.normalize('NFKC', str(s1)) s2 = unicodedata.normalize('NFKC', str(s2)) return compare_digest(s1, s2) + + +def user_email(user): + """The user's email address, via the swappable user model's own field. + + AUTH_USER_MODEL is swappable and a host project's user model need not + call the field "email" -- get_email_field_name() is the same indirection + django_mfa.admin already uses for USERNAME_FIELD. + """ + from django.contrib.auth import get_user_model + + field = get_user_model().get_email_field_name() + return (getattr(user, field, "") or "").strip() + + +def mask_email(address): + """Show enough of an address to recognise, not enough to learn. + + ashwin@example.com -> a****n@example.com + + A local part of one or two characters is masked completely rather than + partially revealed: "a*@example.com" gives away half of a two-character + mailbox name. Anything that isn't an address at all returns "", so a + template can test the value rather than rendering nonsense. + + isinstance-checked before the "@" test, not just falsiness-checked: this + is reachable from otp_tags.py's template filter on a value that came + straight out of Authenticator.data, a JSONField a host project could + write to directly. A truthy non-string there (an int, a list) is not + caught by `not address`, and "@" not in address raises TypeError on + anything that isn't a string or a container of strings -- an unhandled + 500 on the security page rather than the "" this function exists to + return for exactly this kind of garbage input. + """ + if not isinstance(address, str) or "@" not in address: + return "" + local, _, domain = address.partition("@") + if not local or not domain: + return "" + if len(local) <= 2: + return f"{'*' * len(local)}@{domain}" + return f"{local[0]}{'*' * (len(local) - 2)}{local[-1]}@{domain}" diff --git a/django_mfa/views/enroll.py b/django_mfa/views/enroll.py index 3835ac9..4b0c8e9 100644 --- a/django_mfa/views/enroll.py +++ b/django_mfa/views/enroll.py @@ -3,7 +3,7 @@ from django.shortcuts import redirect, render from django.urls import reverse -from django_mfa import session +from django_mfa import events, session from django_mfa.conf import settings as mfa_settings from django_mfa.models import Authenticator from django_mfa.views.verify import GENERIC_ERROR, _adapter_or_404 @@ -24,7 +24,7 @@ def enroll_factor(request, factor_type): if request.method == "POST": try: - adapter.complete_enroll(request, request.POST) + authenticator = adapter.complete_enroll(request, request.POST) except (ValueError, TypeError, KeyError): # ValueError: the adapter's own "this ceremony/code is invalid" # signal (e.g. TOTP's wrong code, or a stale/replayed WebAuthn @@ -46,6 +46,9 @@ def enroll_factor(request, factor_type): # Enrolling a factor satisfies this session's requirement — the user # just proved possession. session.mark_verified(request, factor_type) + events.factor_added.send_robust( + sender=type(adapter), user=request.user, + authenticator=authenticator, request=request) has_codes = Authenticator.objects.filter( user=request.user, type=Authenticator.Type.RECOVERY_CODES).exists() diff --git a/django_mfa/views/manage.py b/django_mfa/views/manage.py index a285b96..5bc3bd9 100644 --- a/django_mfa/views/manage.py +++ b/django_mfa/views/manage.py @@ -4,6 +4,7 @@ from django.shortcuts import get_object_or_404, redirect, render, resolve_url from django.urls import reverse +from django_mfa import events, policy from django_mfa.adapters.recovery_codes import RecoveryCodesAdapter from django_mfa.conf import settings as mfa_settings from django_mfa.models import Authenticator @@ -25,6 +26,15 @@ def security_settings(request): "recovery_codes_remaining": RecoveryCodesAdapter().remaining(request.user), "owned_by_enterprise": mfa_settings.MFA_OWNED_BY_ENTERPRISE, "base_template": mfa_settings.MFA_BASE_TEMPLATE, + # True only when this user is *both* required to hold a factor and + # holds none -- i.e. exactly when MfaMiddleware walled them here. + # has_primary_factor, not primary_enabled_for: only the yes/no answer + # is needed here, and this view is the one every enrollment-required + # user lands back on after every action, so it runs on every one of + # those requests. + "mfa_enrollment_required": ( + policy.mfa_required_for(request.user) + and not registry.has_primary_factor(request.user)), } return render(request, "django_mfa/security.html", context) @@ -45,7 +55,14 @@ def recovery_codes(request): """ has_codes = Authenticator.objects.filter( user=request.user, type=Authenticator.Type.RECOVERY_CODES).exists() - codes = None if has_codes else RecoveryCodesAdapter().generate(request.user) + codes = None + if not has_codes: + codes = RecoveryCodesAdapter().generate(request.user) + events.factor_added.send_robust( + sender=RecoveryCodesAdapter, user=request.user, + authenticator=Authenticator.objects.get( + user=request.user, type=Authenticator.Type.RECOVERY_CODES), + request=request) next_url = resolve_url(settings.LOGIN_REDIRECT_URL) return render(request, "django_mfa/recovery_codes.html", { "codes": codes, @@ -78,5 +95,19 @@ def manage_factors(request): "This security key is managed by your organization and cannot " "be removed here.") + factor_type, name = authenticator.type, authenticator.name authenticator.delete() + try: + sender = type(registry.get(factor_type)) + except KeyError: + # A row whose type is no longer registered -- MFA_FACTORS narrowed, + # or registry.unregister() (which checks.py recommends as the + # WebAuthn opt-out). security_settings lists every row regardless of + # the registry, so removing one of these is a supported action and + # must not 500 after the delete has already committed. There is no + # adapter class to name as the sender in that case. + sender = None + events.factor_removed.send_robust( + sender=sender, user=request.user, + factor_type=factor_type, name=name, request=request) return redirect(reverse("mfa:security_settings")) diff --git a/django_mfa/views/verify.py b/django_mfa/views/verify.py index 572252d..6cd3ece 100644 --- a/django_mfa/views/verify.py +++ b/django_mfa/views/verify.py @@ -9,7 +9,7 @@ from django.utils.http import url_has_allowed_host_and_scheme from fido2.webauthn import AuthenticationResponse -from django_mfa import ratelimit, session +from django_mfa import events, ratelimit, session from django_mfa.adapters.webauthn import AUTH_STATE_KEY, WebAuthnAdapter, get_server from django_mfa.backends import WebAuthnBackend, user_from_handle from django_mfa.conf import settings as mfa_settings @@ -112,6 +112,9 @@ def verify_factor(request, factor_type): if verified: ratelimit.clear(request.user, factor_type) session.mark_verified(request, factor_type) + events.mfa_verified.send_robust( + sender=type(adapter), user=request.user, + method=factor_type, request=request) # Trust this browser for MFA_REMEMBER_DAYS so a future login can # skip the challenge (see the user_logged_in signal in # signals.py, which checks verify_rmb_cookie()). update_rmb_cookie @@ -119,6 +122,14 @@ def verify_factor(request, factor_type): return update_rmb_cookie(request, redirect(next_url)) if allowed: ratelimit.record_failure(request.user, factor_type) + # Emitted for a refused (rate-limited) attempt too -- see the signal's + # own comment in events.py. This is below the `if allowed` guard on + # purpose: record_failure is budget accounting and must not run when + # the attempt was never evaluated, while the event is an observation + # and must fire either way. + events.mfa_verification_failed.send_robust( + sender=type(adapter), user=request.user, + method=factor_type, request=request) context["error_message"] = GENERIC_ERROR context.update(adapter.begin_verify(request, request.user)) return render(request, adapter.verify_template, context, status=400) @@ -273,6 +284,9 @@ def passkey_complete(request): # that point -- the user must still complete a second-factor challenge. if parsed.response.authenticator_data.is_user_verified(): session.mark_verified(request, "webauthn") + events.mfa_verified.send_robust( + sender=type(adapter), user=user, + method="webauthn", request=request) auth.login(request, user, backend=BACKEND_PATH) return redirect(_safe_next(request)) diff --git a/docs/api.md b/docs/api.md index f6cd41f..8f0eb33 100644 --- a/docs/api.md +++ b/docs/api.md @@ -38,6 +38,7 @@ singleton — do not construct your own: | Method | Returns | |---|---| | `primary_enabled_for(user)` | Adapters that mean this user **is protected**. Excludes recovery codes. **This is the predicate for "does this user have MFA".** | +| `has_primary_factor(user)` | The same question as a `bool`, in **one** query rather than one per registered adapter. Use this when you only need yes/no — the enrollment wall, `@mfa_required` and the notification receivers all do, and on a busy site the difference is per request. Both methods read `Adapter.counts_as_primary_factor`, so they cannot disagree. | | `enabled_for(user)` | Adapters this user can **verify with right now**. Includes recovery codes. This is what to offer on a challenge screen. | | `available_for(user)` | Adapters the user could still **add**. Excludes singletons they already hold and factors that aren't enrolled at all. | | `all()` | Every registered adapter. | @@ -159,6 +160,54 @@ need to reset it. ## Signals -django-mfa sends no signals of its own. It **receives** `user_logged_in` (to stamp -the session pending) and `user_logged_out` (to clear the quicklogin hint). To react -to enrollment or verification, use `post_save` on `Authenticator`. +django-mfa also **receives** `user_logged_in` (to stamp the session pending) and +`user_logged_out` (to clear the quicklogin hint) — you don't connect anything for +those, they're internal. + +It **sends** five signals of its own, defined in `django_mfa.events` and re-exported +from `django_mfa.signals` (either import path works): + +| Signal | kwargs | Fires when | +|---|---|---| +| `factor_added` | `user`, `authenticator`, `request` | An adapter's `complete_enroll()` succeeds — every ordinary enrollment, plus the first time a user generates recovery codes. `authenticator` is the created `Authenticator` row. | +| `factor_removed` | `user`, `factor_type`, `name`, `request` | `mfa:manage` deletes a row. `factor_type` and `name` are passed by value, not as an instance — the row is already gone by the time this fires. | +| `mfa_verified` | `user`, `method`, `request` | A second-factor challenge succeeds — from `verify_factor`'s success branch, and from `passkey_complete` when the passkey assertion carries User Verification (a UP-only passkey login logs the user in but has not satisfied a second factor, so it does not fire this). | +| `mfa_verification_failed` | `user`, `method`, `request` | A challenge fails: a wrong code, a caught adapter exception (`ValueError`/`TypeError`/`KeyError`), **or an attempt refused outright by the rate limiter** — no adapter call happens in that last case, so a receiver watching for brute force needs to see refused attempts too, not only evaluated ones. | +| `recovery_code_used` | `user`, `remaining`, `request` | A recovery code is spent. Sent from the adapter itself, not the view — only the adapter knows how many codes are left. | + +Read the user off the `user` kwarg, never off `request.user`. On the passkey path +`mfa_verified` fires between `session.mark_verified()` and `auth.login()` — the +order is forced, since the login signal's own receiver checks whether the session is +already verified — so `request.user` is still `AnonymousUser` there, while the `user` +kwarg is correct on every path. + +`sender` is the `Adapter` **class** for the factor involved (e.g. `TOTPAdapter`), so +a receiver can narrow with `sender=TOTPAdapter` — except on `factor_removed`, whose +`sender` is `None` when the removed row's type is no longer registered (`MFA_FACTORS` +was narrowed since it was enrolled, or a third-party adapter was unregistered): match +on the `factor_type` kwarg instead of `sender` if you need to handle that case. + +A receiver: + + import logging + + from django.dispatch import receiver + from django_mfa.signals import factor_removed + + logger = logging.getLogger("myapp.security") + + @receiver(factor_removed) + def audit_factor_removal(sender, user, factor_type, name, request, **kwargs): + logger.info("factor removed: user=%s type=%s name=%r", + user.pk, factor_type, name) + +All five are sent with `send_robust()`, not `send()`: a raising receiver cannot break +the security action it's observing — enrolling, verifying, or removing a factor +succeeds or fails independently of what your receiver does with the event. The other +side of that trade is that `send_robust()` catches and discards the exception rather +than letting it propagate, so a receiver that fails does so **silently** unless it +logs its own failure. + +To react with something other than a signal receiver, `post_save` on `Authenticator` +still works too — these signals are additional, not a replacement for the model +layer. diff --git a/docs/contributing.md b/docs/contributing.md index 663abbe..f809551 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -43,7 +43,8 @@ upload token for that workflow alone. To cut a release, in full: -1. Bump `version` in `pyproject.toml`, open a PR, merge it to `master`. +1. Bump `version` in `pyproject.toml` and add that version's section to + `CHANGELOG.md`, in the same PR. Merge it to `master`. There is no step 2. You never create a tag and you never create a GitHub Release -- `.github/workflows/tag-release.yml` sees the version change, creates diff --git a/docs/custom_factors.md b/docs/custom_factors.md index c110c4f..ad32a75 100644 --- a/docs/custom_factors.md +++ b/docs/custom_factors.md @@ -2,8 +2,8 @@ Adding a factor type means writing one class and registering it. You do not write views, URLs, or middleware changes — those are generic and dispatch to whatever the -registry holds. The three built-ins (`totp`, `webauthn`, `recovery_codes`) are -written against exactly the API below; there is no privileged path. +registry holds. The four built-ins (`totp`, `webauthn`, `recovery_codes`, `email`) +are written against exactly the API below; there is no privileged path. ## The shape of it @@ -26,83 +26,94 @@ The `begin_*` methods return a dict that is merged into the template context. Th Once registered, `type` is what appears in `/mfa/enroll//` and `/mfa/verify//`, and what gets stored in `Authenticator.type`. -## A worked example: email codes +## A worked example: a printed backup token -A complete second factor that emails a one-time code. Roughly 40 lines. +A factor django-mfa doesn't ship: one static, hashed backup token, handed to a user +in person or by post, entered once to activate it, and consumed the moment it's used. +Unlike a TOTP secret it never changes and needs no clock sync; unlike ten recovery +codes it's a single value, closer to what an administrator would issue someone who's +lost both their authenticator and their recovery codes. Roughly 40 lines. # myapp/mfa.py import secrets - from django.core.mail import send_mail - from django.utils import timezone + from django.contrib.auth.hashers import check_password, make_password from django_mfa.models import Authenticator from django_mfa.registry import Adapter - from django_mfa.utils import strings_equal - SESSION_KEY = "myapp_email_code" + SESSION_KEY = "myapp_backup_token" - class EmailAdapter(Adapter): - type = "email" - verbose_name = "Emailed code" + class BackupTokenAdapter(Adapter): + type = "backup_token" + verbose_name = "Backup token" - # One enrollment per user: the address lives on the row, and a second - # row would just be a duplicate. (This is the default; shown for - # clarity — set it True only if several instances make sense, the way - # several security keys do.) + # One at a time: once the token is spent, complete_verify() below + # deletes the row outright, so there's never a second one to hold. + # (This is the default; shown for clarity — set it True only if + # several instances make sense, the way several security keys do.) supports_multiple = False - def _issue(self, user): - code = f"{secrets.randbelow(1_000_000):06d}" - send_mail("Your sign-in code", f"Your code is {code}.", - None, [user.email]) - return code - def begin_enroll(self, request): - request.session[SESSION_KEY] = self._issue(request.user) - return {"email": request.user.email} + # Generated here to keep the example self-contained. Swap this for + # your own issuing step if you want tokens minted out of band by + # an administrator and handed to users in person or by post — + # begin_enroll would then look up an already-issued token instead + # of minting one, but complete_enroll's job (confirm they have it + # right) stays the same. + token = secrets.token_hex(8) + request.session[SESSION_KEY] = make_password(token) + return {"token": token} # shown once: "write this down" def complete_enroll(self, request, data): - expected = request.session.pop(SESSION_KEY, None) - if not expected or not strings_equal(data["code"], expected): - raise ValueError("Code did not match.") + # Confirms the user actually recorded the token shown above, the + # same way TOTP's enrollment confirms a scanned secret rather than + # trusting that the QR code was read correctly. + expected_hash = request.session.pop(SESSION_KEY, None) + if not expected_hash or not check_password( + data.get("token", ""), expected_hash): + raise ValueError("Token did not match.") return Authenticator.objects.create( - user=request.user, type=self.type, - data={"email": request.user.email}) + user=request.user, type=self.type, data={"hash": expected_hash}) def begin_verify(self, request, user): - request.session[SESSION_KEY] = self._issue(user) - return {"email": user.email} + # Nothing to send: unlike a delivery-based factor, the token was + # already handed to the user at enrollment, so re-rendering this + # page on a failed attempt has no side effect to worry about. + return {} def complete_verify(self, request, user, data): - expected = request.session.pop(SESSION_KEY, None) - if not expected or not strings_equal(data.get("code", ""), expected): - return False auth = self.get_instances(user).first() if auth is None: return False - auth.record_usage() + if not check_password(data.get("token", ""), auth.data["hash"]): + return False + # Single-use: spending it deletes the factor outright rather than + # marking it used, so a stolen-and-reused token is impossible and + # re-issuing one (self-service, or through an administrator) is + # the same complete_enroll() flow as the first time. + auth.delete() return True Two templates, which can be as short as this: - {% comment %} myapp/templates/django_mfa/verify_email.html {% endcomment %} + {% comment %} myapp/templates/django_mfa/verify_backup_token.html {% endcomment %} {% extends base_template %} {% block content %} -

Check your email

-

We sent a code to {{ email }}.

+

Enter your backup token

{% csrf_token %} - + {% if error_message %}

{{ error_message }}

{% endif %}
{% endblock %} -`enroll_email.html` is the same minus the `next` field. See {doc}`customizing` for -the context every page receives. +`enroll_backup_token.html` is the same minus the `next` field, plus showing `{{ +token }}` once so the user can copy it down before confirming. See {doc}`customizing` +for the context every page receives. Then register it once, at startup: @@ -114,13 +125,18 @@ Then register it once, at startup: def ready(self): from django_mfa.registry import registry - from myapp.mfa import EmailAdapter - registry.register(EmailAdapter()) + from myapp.mfa import BackupTokenAdapter + registry.register(BackupTokenAdapter()) That's the whole integration. The factor now appears on the security page, in the picker, in the middleware's exempt set, and under rate limiting — none of which you touched. +For a fuller worked example of a *delivery-based* factor — one with a send step, a +throttle on how often it can be resent, and a masked address shown back to the user — +see the built-in `django_mfa/adapters/email.py`, which this page used to reproduce +here before emailed codes shipped as `"email"` in `MFA_FACTORS`. + ## What you get for free Registering an adapter is enough for all of this: @@ -163,10 +179,11 @@ failure by raising `ValueError`; its return value is the created `Authenticator` different: enrollment has nothing meaningful to return on failure. **`begin_verify` runs again on failure.** After a wrong code the view re-renders the -challenge page, which means `begin_verify` is called a second time. If yours has a -side effect — sending an email, as above — every wrong code sends another one. That -may be what you want, or you may want to cache the issued code with a TTL and reuse -it; decide deliberately. +challenge page, which means `begin_verify` is called a second time. The worked +example above has no side effect to worry about, but a factor that sends something +(an email, an SMS) on `begin_verify` — see `django_mfa/adapters/email.py` — will send +another one on every wrong-code retry unless you cache the issued value with a TTL +and reuse it, the way `EmailAdapter._ensure_code()` does; decide deliberately. **Store state in `data`, not in new columns.** `Authenticator.data` is a `JSONField`. There is no per-factor table and adding one is not the intended extension point. @@ -183,17 +200,18 @@ it. For values you never need to read back, hash instead, as recovery codes do. Being honest about where a custom factor is a slightly second-class citizen: -- **`Authenticator.Type` is a fixed `TextChoices`** with the three built-ins. A row +- **`Authenticator.Type` is a fixed `TextChoices`** with the four built-ins. A row with a custom `type` saves and queries fine — Django only validates choices in `full_clean()`, which these code paths don't call — but `get_type_display()` returns the raw string rather than a label, so `security.html` shows `my_factor` instead of `My factor`. Override `security.html` and render `adapter.verbose_name` if that matters to you. - **The singleton database constraint names its types explicitly** - (`mfa_one_singleton_authenticator_per_user` covers `totp` and `recovery_codes`). - `supports_multiple = False` is enforced in `is_available()` at the application - level, not by your database. For most factors that's fine; if you need the - guarantee, add your own constraint in a migration in your app. + (`mfa_one_singleton_authenticator_per_user` covers `totp`, `recovery_codes`, and + `email`). `supports_multiple = False` is enforced in `is_available()` at the + application level, not by your database, for anything outside that list. For most + factors that's fine; if you need the guarantee, add your own constraint in a + migration in your app. - **`MFA_FACTORS` only controls the built-ins.** Your adapter is registered by your own `ready()`, so listing it there does nothing — and an unrecognized name raises `ImproperlyConfigured`. Gate registration on your own setting if you need it @@ -211,16 +229,17 @@ is the best reference. A minimal check that yours is wired up end to end: from django.test import TestCase from django_mfa.registry import registry - class EmailFactorTests(TestCase): + class BackupTokenFactorTests(TestCase): def setUp(self): self.user = get_user_model().objects.create_user( "u", "u@example.com", "pw") self.client.force_login(self.user) def test_enrolling_protects_the_user(self): - self.client.get("/mfa/enroll/email/") # issues a code - code = self.client.session["myapp_email_code"] - response = self.client.post("/mfa/enroll/email/", {"code": code}) + response = self.client.get("/mfa/enroll/backup_token/") # issues a token + token = response.context["token"] + response = self.client.post( + "/mfa/enroll/backup_token/", {"token": token}) self.assertEqual(response.status_code, 302) self.assertTrue(registry.primary_enabled_for(self.user)) diff --git a/docs/customizing.md b/docs/customizing.md index 17b2a4e..54a1b01 100644 --- a/docs/customizing.md +++ b/docs/customizing.md @@ -49,9 +49,11 @@ The full set: | `django_mfa/picker.html` | `mfa:verify` | at login, when the user holds more than one method | | `django_mfa/enroll_totp.html` | `mfa:enroll_factor` | setting up an authenticator app | | `django_mfa/enroll_webauthn.html` | `mfa:enroll_factor` | registering a security key or passkey | +| `django_mfa/enroll_email.html` | `mfa:enroll_factor` | confirming an emailed one-time code (`"email"` in `MFA_FACTORS`, off by default — see {doc}`security`) | | `django_mfa/verify_totp.html` | `mfa:verify_factor` | the TOTP challenge | | `django_mfa/verify_webauthn.html` | `mfa:verify_factor` | the WebAuthn challenge | | `django_mfa/verify_recovery_codes.html` | `mfa:verify_factor` | the recovery-code challenge | +| `django_mfa/verify_email.html` | `mfa:verify_factor` | the emailed-code challenge | | `django_mfa/recovery_codes.html` | `mfa:recovery_codes` | displaying freshly generated codes | Enroll and verify templates are resolved from the factor type, as @@ -73,6 +75,7 @@ what the `{% extends base_template %}` line at the top resolves. | `authenticators` | The individual `Authenticator` rows, ordered by type then creation. This — not `enabled_adapters` — is what lets you list three security keys separately so a user can tell them apart and remove exactly one. | | `recovery_codes_remaining` | Integer count of unused codes. | | `owned_by_enterprise` | The `MFA_OWNED_BY_ENTERPRISE` setting, so the template can hide the remove button for WebAuthn. | +| `mfa_enrollment_required` | `True` only when this user is both required to hold a factor (`MFA_REQUIRED`, see {doc}`enforcement`) and holds none yet — i.e. exactly when `MfaMiddleware` walled them onto this page. If you shadow this template, render something here: without it, a required user lands on a security page that gives no reason for the wall they just hit. | ### `picker.html` @@ -93,6 +96,8 @@ being seen. | *(TOTP)* `secret_key` | The freshly generated Base32 secret. Must be POSTed back in a hidden field — the server does not stash it in the session. | | *(TOTP)* `provisioning_uri` | The `otpauth://` URI. Render it with `{% qrcode provisioning_uri "alt text" %}`. | | *(WebAuthn)* `options` | JSON ceremony options. Render with `{% webauthn_options_script options "webauthn-options" %}`. | +| *(email)* `address` | The account's address, masked (`a****n@example.com`) — never the plaintext. `None` when the account has no usable address; `enroll_email.html` shows a "no address on file" message instead of the form in that case (guard on it if you shadow this page). | +| *(email)* `code_length` | Digit count of the code just emailed — tracks `MFA_EMAIL_CODE_LENGTH` rather than assuming 6, so the input's `maxlength` and label stay correct if you change that setting. Only present alongside a non-`None` `address`. | ### Verify pages @@ -103,6 +108,8 @@ being seen. | `error_message` | Present only after a failed attempt; the page re-renders with HTTP 400. | | *(recovery codes)* `remaining` | How many unused codes are left, so you can warn people running low. | | *(WebAuthn)* `options` | JSON ceremony options, as above. | +| *(email)* `address` | The enrolled address, masked — the address captured at enrollment time, not necessarily the account's current one (see {doc}`security`). `None` when there is no live ceremony to challenge (e.g. the factor was removed after this page was linked to); `verify_email.html` shows a fallback message instead of the form in that case (guard on it if you shadow this page). | +| *(email)* `code_length` | Digit count of the code just emailed, as on the enroll page above. Only present alongside a non-`None` `address`. | ### `recovery_codes.html` diff --git a/docs/enforcement.md b/docs/enforcement.md new file mode 100644 index 0000000..8e883ab --- /dev/null +++ b/docs/enforcement.md @@ -0,0 +1,140 @@ +# Enforcing MFA + +Enrollment is opt-in by default. `MfaMiddleware` only challenges a user who already +holds a factor, so someone who never enrolls is never prompted or blocked — see +{doc}`mfa_flow`. This page covers the two ways to change that: requiring MFA of some +or all users, and requiring it for specific views regardless of who's asking — plus +what a required user actually sees before they've enrolled anything. + +## Requiring MFA + +`MFA_REQUIRED` (`django_mfa.policy`) answers "who must hold a second factor", in one +of four shapes. + +**Nobody — the default:** + + MFA_REQUIRED = False + +**Every authenticated user:** + + MFA_REQUIRED = True + +**Anyone matching a predicate you write** — a callable taking a user and returning a +bool: + + def needs_mfa(user): + return user.profile.handles_billing + + MFA_REQUIRED = needs_mfa + +**A dotted path to one**, resolved with `django.utils.module_loading.import_string` — +useful when you'd rather not import the predicate's module at settings-load time: + + MFA_REQUIRED = "myapp.policy.needs_mfa" + +Two predicates ship for the common cases, in `django_mfa.policy`: + + from django_mfa.policy import is_staff, in_groups + + MFA_REQUIRED = is_staff # anyone who can reach /admin/ + MFA_REQUIRED = in_groups("admins", "finance") # anyone in either group + +`in_groups(...)` is a factory, not a predicate itself — call it and assign the +result, as above. It costs one query per request +(`user.groups.filter(name__in=names).exists()`), so if you need several different +group-based rules, write your own predicate that checks them together rather than +combining several `in_groups(...)` calls. + +`django_mfa.E004` fails `manage.py check` if `MFA_REQUIRED` is a string that doesn't +import, or resolves to something that isn't callable — catching a misconfigured +predicate at startup is a lot cheaper than discovering it from an exception raised +inside the middleware on some user's live request. + +## What a required user sees + +A user `MFA_REQUIRED` applies to, who holds no *primary* factor yet +(`registry.has_primary_factor(user)` is false — see {doc}`api`), is walled to the +security page (`mfa:security_settings`) on every request. The only other pages +reachable are the enroll pages, `mfa:recovery_codes`, and whatever you list in +`MFA_EXEMPT_PATHS`; everything else redirects to the security page with the +original destination recorded in a `next` query parameter. Nothing in django-mfa +reads it back, though: `enroll_factor` redirects unconditionally to +`mfa:recovery_codes` or `mfa:security_settings` once enrollment succeeds, and +`security_settings` never looks at `next` either — so a user does not land back +where they started once they've enrolled. The parameter is there to use if you +build your own post-enrollment redirect on top of these views; it is not honoured +by the ones django-mfa ships. + +**Recovery codes alone do not release the wall.** They're exhaustible +(`counts_as_primary_factor = False` — see {doc}`security`), so a required user who +only generates recovery codes is still walled. The same *rule* decides both whether +to *challenge* a user at login and whether the enrollment wall releases them — +recovery codes were never meant to be anyone's sole factor, in either sense. The two +read it through different methods for cost reasons (the login path asks +`registry.primary_enabled_for()`, which returns the adapter list; the wall asks +`registry.has_primary_factor()`, which answers the same question in one query +instead of one per registered adapter), but both derive it from +`Adapter.counts_as_primary_factor`, so they cannot disagree. The wall releases the moment they +enroll `totp`, `webauthn`, or any other adapter with `counts_as_primary_factor = +True` (the default). + +:::{warning} +**`MFA_EXEMPT_PATHS` must contain your logout URL for this wall too.** The +enrollment wall is a *second* lockout trap, separate from the one the +pending-verification wall creates: a user required to hold a factor who cannot +enroll one right now — no phone at their desk, no security key issued yet — has no +way out of the redirect loop unless their logout view is exempt. Both walls consult +the same setting; see the warning in {doc}`settings` for the full explanation of +why, and for the exact syntax. +::: + +## Per-view enforcement + +`MFA_REQUIRED` decides which *users* must hold a factor. `django_mfa.decorators` +decides which *views* require one, for everyone who reaches them — useful when the +requirement is a property of the view itself (a billing flow, an admin action) +rather than of the user asking for it: + + from django_mfa.decorators import mfa_required + + @mfa_required + def transfer_funds(request): + ... + +For a class-based view, mix in `MfaRequiredMixin` **first**, so its `dispatch()` +runs before the view's own: + + from django.views.generic import FormView + from django_mfa.decorators import MfaRequiredMixin + + class TransferFundsView(MfaRequiredMixin, FormView): + ... + +Both share the same two *destinations* `MfaMiddleware` redirects to — pending +session → the picker, no primary factor → the security page — and add a third the +middleware doesn't need: unauthenticated → `LOGIN_URL`. `MfaMiddleware` passes an +unauthenticated request straight through, since it isn't a login gate itself; a view +behind only `@mfa_required` may have no other login gate in front of it, so the +decorator supplies that rung on its own. + +The no-primary-factor rung is not applied the same way, though. `MfaMiddleware` +only redirects there when **both** `policy.mfa_required_for(user)` is true **and** +the user holds no primary factor — a project with `MFA_REQUIRED = False` (the +default) never reaches that check at all. `@mfa_required`/`MfaRequiredMixin` apply +the no-primary-factor rung unconditionally to anyone who reaches a decorated view, +regardless of `MFA_REQUIRED`: the decorator is itself the policy for that view, not +a mirror of the site-wide one. A view behind `@mfa_required` requires a primary +factor even on a `MFA_REQUIRED = False` project. + +## What is not enforced + +- **Unauthenticated requests.** Every wall above only applies once + `request.user.is_authenticated` is true. Your project's own login and + password-reset flows are still yours to protect — `MFA_REQUIRED` cannot make MFA a + precondition of authenticating in the first place, only of what an + already-authenticated session can go on to reach. +- **Step-up re-authentication.** Once a session is verified, it stays verified for + the life of that session; django-mfa does not re-challenge before a sensitive + action. If you want that — say, requiring a fresh code before changing a password + — build it yourself on `session.is_verified()` plus your own policy. See + {doc}`security`. diff --git a/docs/index.md b/docs/index.md index 03659ec..38b4307 100644 --- a/docs/index.md +++ b/docs/index.md @@ -30,6 +30,7 @@ mfa_flow :caption: Guides customizing +enforcement recipes custom_factors ``` diff --git a/docs/mfa_flow.md b/docs/mfa_flow.md index a1106e8..c2a1317 100644 --- a/docs/mfa_flow.md +++ b/docs/mfa_flow.md @@ -7,8 +7,8 @@ Once basic setup (see {doc}`installation_setup`) is done, django-mfa exposes the | `mfa:security_settings` | `security/` | Overview: enabled factors, factors still available to add, recovery codes remaining. | | `mfa:manage` | `manage/` | POST-only: delete one of the user's own authenticators. | | `mfa:verify` | `verify/` | The second-factor picker (see below). | -| `mfa:verify_factor` | `verify//` | Challenge screen for one specific factor (`totp`, `webauthn`, or `recovery_codes`). | -| `mfa:enroll_factor` | `enroll//` | Enrollment screen for one specific factor (`totp` or `webauthn` -- recovery codes are generated, not enrolled). | +| `mfa:verify_factor` | `verify//` | Challenge screen for one specific factor (`totp`, `webauthn`, `recovery_codes`, or the opt-in `email`). | +| `mfa:enroll_factor` | `enroll//` | Enrollment screen for one specific factor (`totp`, `webauthn`, or the opt-in `email` -- recovery codes are generated, not enrolled). | | `mfa:recovery_codes` | `recovery/codes/` | View (and, on first visit, generate) recovery codes. | | `mfa:passkey_begin` | `passkey/begin/` | GET: start a passwordless WebAuthn ceremony (see below). | | `mfa:passkey_complete` | `passkey/complete/` | POST: finish it and log the resolved user in. | @@ -23,13 +23,24 @@ Once basic setup (see {doc}`installation_setup`) is done, django-mfa exposes the ## Logging in with a second factor -1. The host project's own login view authenticates the user as usual (username/password, SSO, etc.) and calls `django.contrib.auth.login()`. A `user_logged_in` signal handler (`django_mfa/signals.py`) then marks the session pending a second factor, if -- and only if -- the user has at least one factor counted as a primary factor (`totp` or `webauthn`; recovery codes alone do not count, since they're exhaustible). This replaces the old contract where a host project's login view had to set session keys by hand. +1. The host project's own login view authenticates the user as usual (username/password, SSO, etc.) and calls `django.contrib.auth.login()`. A `user_logged_in` signal handler (`django_mfa/signals.py`) then marks the session pending a second factor, if -- and only if -- the user has at least one factor counted as a primary factor (`totp`, `webauthn`, or the opt-in `email` factor -- see {doc}`security`; recovery codes alone do not count, since they're exhaustible). This replaces the old contract where a host project's login view had to set session keys by hand. 2. From that point on, `MfaMiddleware` redirects every request from that session to `mfa:verify` except the picker itself, each registered factor's own verify page, and anything listed in `MFA_EXEMPT_PATHS` (see the warning in {doc}`settings` -- this should include your logout URL). 3. `mfa:verify` (the picker) looks at which factors this user actually holds. If there's exactly one, it redirects straight to `mfa:verify_factor//` -- no need to make a user with a single factor choose from a list of one. Otherwise it renders a list to choose from. 4. `GET mfa:verify_factor//` renders that factor's challenge screen; POSTing the code/assertion marks the session fully verified on success (or a generic error on failure -- see the rate-limiting note below) and redirects to `next` (validated against open redirects) or `LOGIN_REDIRECT_URL`. Failed attempts against a single factor are rate-limited (`MFA_VERIFY_RATE_LIMIT`, default 5 per 5 minutes per user per factor type); a locked-out attempt gets the exact same response as a wrong code, so the lockout itself never reveals whether MFA is even enabled for that account. +## Being required to enroll + +Everything above only applies to a user who already holds a factor -- enrollment is +opt-in by default, and a user who never enrolls is never prompted or blocked. A +separate, opt-in wall changes that: if `MFA_REQUIRED` (`django_mfa.policy`) applies +to a user and they hold no primary factor yet, `MfaMiddleware` redirects every +request from that user to `mfa:security_settings` instead of to the picker -- +there is nothing to challenge them on until they enroll something. See +{doc}`enforcement` for the settings, the exact pages still reachable while walled, +and the equivalent per-view `@mfa_required` decorator. + ## Passwordless (passkey) login A user whose only factor is a WebAuthn passkey doesn't have to go through "password, then second factor" at all -- see {doc}`installation_setup` for the `AUTHENTICATION_BACKENDS` setup this requires. diff --git a/docs/recipes.md b/docs/recipes.md index c7a228b..ed17bc7 100644 --- a/docs/recipes.md +++ b/docs/recipes.md @@ -23,34 +23,18 @@ have that code, delete it — see {doc}`upgrading`. ## Sending people to set up MFA -There is no built-in "you must enroll" enforcement, deliberately: whether MFA is -mandatory, and for whom, is a policy decision. Point users at `mfa:security_settings` -from your account area: +Whether MFA is mandatory, and for whom, is a policy decision — `MFA_REQUIRED` +answers it. See {doc}`enforcement` for the full reference: the four shapes the +setting takes (`True`, a predicate, `is_staff`, `in_groups(...)`), what a +required-but-unenrolled user can still reach before locking themselves out, and +`@mfa_required`/`MfaRequiredMixin` for requiring it per view instead of per user. +That page also covers `MFA_EXEMPT_PATHS`, which the enrollment wall needs the same +way the pending-verification wall does — skip it and you build a redirect loop. - Two-factor authentication - -To *require* it for some group, write a small middleware of your own. Note what has -to be exempt, or you build a redirect loop: +For MFA that's entirely opt-in (the default, and still the right choice for most +projects), just point users at the settings page from your account area: - from django.shortcuts import redirect - from django.urls import reverse - from django_mfa.registry import registry - - class RequireMfaForStaff: - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - user = request.user - if (user.is_authenticated and user.is_staff - and not registry.primary_enabled_for(user) - and not request.path.startswith("/mfa/") - and request.path != reverse("logout")): - return redirect("mfa:security_settings") - return self.get_response(request) - -Use `registry.primary_enabled_for()` rather than counting `Authenticator` rows — a -user holding only recovery codes has rows but is not protected. + Two-factor authentication ## Checking whether the current session passed MFA diff --git a/docs/security.md b/docs/security.md index 74962f1..fcdcd58 100644 --- a/docs/security.md +++ b/docs/security.md @@ -104,6 +104,44 @@ show. `counts_as_primary_factor = False`: a user holding only recovery codes is offered them at the picker but is never challenged on their strength alone. +### Emailed codes + +Emailing a one-time code (`"email"` in `MFA_FACTORS`, off by default — see +{doc}`settings`) is the "lost my phone" factor, but it inherits whatever the +mailbox it's sent to is worth as a credential. + +**It's usually also your password-reset channel.** An attacker who already has a +victim's password and can reach their inbox — a shared family computer, a mail +client left signed in, a compromised email provider — can satisfy both factors +through the same channel. Recommend it as a *fallback* for someone who has lost +their authenticator and their recovery codes, not as the factor you steer a +high-value account (an administrator, anyone with billing access) toward. TOTP and +WebAuthn don't share this property: neither can be satisfied by reading mail. + +**The enrolled address is fixed.** A code is sent to the address captured in +`Authenticator.data` at enrollment time, not to whatever `user.email` says right +now. A factor is possession of a specific mailbox; following a mutable profile +field would mean that changing it — by whatever means the host project allows — +silently redirects the factor to a mailbox chosen by whoever changed it. Changing +the enrolled address is therefore a remove-and-re-enroll, not an edit. + +### Notifications + +`MFA_NOTIFY_ON_CHANGE` (off by default) emails a user when a factor is added or +removed, when a recovery code is spent, or when their last remaining primary factor +goes. Treat it as a courtesy, not a control: + +- **Sending is best-effort.** A mail-backend failure is logged at `ERROR` and + dropped, never raised — the security action it's reporting on (removing a key you + believe is compromised, say) has already succeeded and must not be rolled back or + blocked by a mail outage. A notification not arriving in someone's inbox says + nothing about whether the underlying action happened. +- **Don't build alerting on it.** A delivered email is not a durable audit record, + and there is no retry or dead-letter queue behind it. The `django_mfa.events` + signals it's built on (see {doc}`api`) fire unconditionally, whether or not + `MFA_NOTIFY_ON_CHANGE` is on — connect your own receiver to a logging pipeline or + a queue if you need something you can actually alert on. + ### Secrets at rest TOTP secrets are stored in plaintext. WebAuthn stores only a public key, so there is diff --git a/docs/settings.md b/docs/settings.md index 38c65cf..480b278 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -18,8 +18,9 @@ WebAuthn** — see {doc}`installation_setup`. | Setting | Default | Purpose | |---|---|---| | `MFA_FACTORS` | `["totp", "recovery_codes", "webauthn"]` | Which built-in factor adapters get registered at startup. See [Choosing which factors to offer](#choosing-which-factors-to-offer) below. | +| `MFA_REQUIRED` | `False` | Who must hold a second factor. `False` (nobody), `True` (every authenticated user), a callable taking a user and returning a bool, or a dotted path to one. A required user with no factor is walled to the security page until they enroll — see {doc}`enforcement`. | | `MFA_ISSUER_NAME` | `None` | Issuer label shown next to the username in the user's authenticator app when enrolling TOTP. Set it to your product name; without it the app shows the username alone, which is confusing for anyone with more than one account. | -| `MFA_EXEMPT_PATHS` | `[]` | URL paths reachable while a session is pending a second factor. **Must include your logout URL** — see the warning below. | +| `MFA_EXEMPT_PATHS` | `[]` | URL paths reachable through either of `MfaMiddleware`'s walls: a session pending a second factor, and — when `MFA_REQUIRED` applies — a required user who hasn't enrolled one yet. **Must include your logout URL** — see the warning below. | ## Appearance @@ -40,8 +41,28 @@ WebAuthn** — see {doc}`installation_setup`. | Setting | Default | Purpose | |---|---|---| | `MFA_VERIFY_RATE_LIMIT` | `"5/5m"` | Failed second-factor attempts allowed per user per factor type, as `"/"` where unit is `s`, `m`, or `h`. See {doc}`security`. | +| `MFA_EMAIL_SEND_RATE_LIMIT` | `"3/5m"` | How many emailed codes a user can be sent per window, same `"/"` grammar as above. Refreshing a challenge page reuses a still-valid code rather than spending budget; past the limit no mail is sent and the page looks exactly the same. Only consulted when `"email"` is in `MFA_FACTORS`. | | `MFA_SECRET_ENCRYPTION_KEYS` | `None` | **Deprecated, and does not encrypt.** List of keys used to *sign* TOTP secrets at rest. The first key signs; every key is tried when reading. See [Signing stored secrets](#signing-stored-secrets-deprecated). | | `MFA_OWNED_BY_ENTERPRISE` | `False` | When `True`, users cannot remove their own WebAuthn authenticators from the security settings page — e.g. an organization-issued security key an administrator manages instead. | +| `MFA_NOTIFY_ON_CHANGE` | `False` | When `True`, emails the user when a factor is added or removed, when a recovery code is spent, and when their last factor goes. Off by default so an upgrade doesn't start sending mail unannounced. The `django_mfa.events` signals fire either way — connect your own receiver for async delivery or for routing somewhere other than email. Sending is synchronous and best-effort: a failure is logged, never raised. | + +## Email codes + +Only consulted when `"email"` is in `MFA_FACTORS`, which it is not by default — +add it to offer emailed one-time codes. + +| Setting | Default | Purpose | +|---|---|---| +| `MFA_EMAIL_CODE_LENGTH` | `6` | Digits in an emailed code. | +| `MFA_EMAIL_CODE_VALIDITY` | `300` | How long an emailed code stays usable, in seconds. | +| `MFA_EMAIL_SUBJECT` | `None` | Subject line for the code email. `None` renders `django_mfa/email/otp_code_subject.txt`, which you can override instead. | +| `MFA_FROM_EMAIL` | `None` | From address for every email django-mfa sends. `None` falls back to Django's `DEFAULT_FROM_EMAIL`. | + +Codes go to the address the factor was **enrolled with**, which the security +page shows masked — not to whatever `user.email` currently says. A factor is +possession of a specific mailbox; following a mutable profile field would mean +that whoever can change that field can redirect the factor. Changing the +address therefore means removing the factor and enrolling it again. ## WebAuthn (security keys and passkeys) @@ -100,13 +121,22 @@ narrow the list, not after. :::{warning} **Set `MFA_EXEMPT_PATHS` to include your project's logout URL.** -`MfaMiddleware` redirects any authenticated, not-yet-verified request to the -second-factor picker, for every path except the ones it derives automatically (the -picker itself and each registered factor's verify page) plus whatever you list here. -A user who cannot complete their second factor — lost device, no recovery codes left -— and whose logout view isn't exempt has **no way to log out**: every request they -make, including to `/logout/`, bounces back to the picker. django-mfa cannot know -your logout URL on its own; list it explicitly: +`MfaMiddleware` enforces two separate walls, and `MFA_EXEMPT_PATHS` is consulted by +both of them: + +- An authenticated, not-yet-verified session is redirected to the second-factor + picker, for every path except the ones derived automatically (the picker itself + and each registered factor's verify page) plus whatever you list here. +- When `MFA_REQUIRED` applies to a user holding no primary factor, they are instead + redirected to the security page, for every path except the enroll pages, recovery + codes, and — again — whatever you list here. See {doc}`enforcement`. + +Either wall traps a user with **no way to log out** unless your logout URL is +exempt: a pending user who cannot complete their second factor (lost device, no +recovery codes left), or a required user who cannot enroll one yet (no phone at +their desk, no security key issued). Every request they make, including to +`/logout/`, bounces back to a page that isn't where they were headed. django-mfa +cannot know your logout URL on its own; list it explicitly: MFA_EXEMPT_PATHS = ["/logout/"] @@ -117,19 +147,23 @@ mounted, with its trailing slash. ## System checks django-mfa registers system checks that run on `manage.py check` — and therefore on -`migrate` and `runserver`, which run checks first — to catch the WebAuthn -misconfigurations above before they can affect a real user. +`migrate` and `runserver`, which run checks first — to catch the misconfigurations +described above before they can affect a real user. -All three are WebAuthn-only. They return no errors at all unless WebAuthn is +E001–E003 are WebAuthn-only: they return no errors at all unless WebAuthn is actually switched on for this install, meaning `MFA_QUICKLOGIN` is on or a WebAuthn adapter is registered (true by default). A project with -`MFA_FACTORS = ["totp", "recovery_codes"]` never trips any of them. +`MFA_FACTORS = ["totp", "recovery_codes"]` never trips any of them. `E004` is not +gated the same way — `MFA_REQUIRED` is not a WebAuthn setting, so there is nothing +to gate on, and it applies to every install regardless of which factors are +registered. | Check ID | Severity | Condition | |---|---|---| | `django_mfa.E001` | Error | WebAuthn is active and `MFA_FIDO2_RP_ID` is unset. | | `django_mfa.E002` | Error | WebAuthn is active and `MFA_FIDO2_RP_ID` is not a suffix of any `ALLOWED_HOSTS` entry. | | `django_mfa.E003` | Error | WebAuthn is active and `django_mfa.backends.WebAuthnBackend` is missing from `AUTHENTICATION_BACKENDS`. | +| `django_mfa.E004` | Error | `MFA_REQUIRED` is a dotted path that fails to import, or resolves to a value that isn't callable. | `E003` exists because the failure it prevents is otherwise completely silent. Passwordless login logs a user in by calling `django.contrib.auth.login()` with an @@ -140,8 +174,15 @@ resolves `request.user` to `AnonymousUser` — no exception, no log line, just a who was "logged in" a moment ago and is now anonymous again. Catching this at startup is far cheaper than a support ticket. -All three are `Error` rather than `Warning` deliberately: each guards a failure mode -that is otherwise silent in production, not a style nit. If a check fires for a +All four are `Error` rather than `Warning` deliberately, though what each guards +against differs slightly. E001–E003 guard a failure mode that is otherwise silent +in production (see E003's own explanation below). A misconfigured `MFA_REQUIRED` +is not silent even without E004 — `policy.resolve()` raises `ImproperlyConfigured` +or `ImportError` the first time `mfa_required_for()` runs, which is a loud 500 on +whichever live request gets there first. What E004 changes is *when* that failure +surfaces: at `manage.py check` (and therefore at `migrate`/`runserver`, and in CI if +you run checks there), before any request has been served, rather than as a 500 on +some user's request in production. If a check fires for a reason you understand and have already accounted for — say you provision `MFA_FIDO2_RP_ID` from a source Django's check framework can't see at check time — the standard Django escape hatch applies: diff --git a/docs/upgrading.md b/docs/upgrading.md index 9144d9d..2d68668 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -14,7 +14,7 @@ U2F (the `U2FKey` model, `python-u2flib-server` dependency, and every `u2f`-pref The three old models are gone from `django_mfa/models.py`. In their place: -- `Authenticator` -- one polymorphic model for every factor type (`type` is `"totp"`, `"webauthn"`, or `"recovery_codes"`), factor-specific data in a `data` `JSONField`, and a `user` FK. A user can hold multiple `Authenticator` rows (e.g. several WebAuthn keys), but at most one `totp` and one `recovery_codes` row each (a database constraint enforces this). +- `Authenticator` -- one polymorphic model for every factor type (`type` is `"totp"`, `"webauthn"`, `"recovery_codes"`, or the opt-in `"email"`), factor-specific data in a `data` `JSONField`, and a `user` FK. A user can hold multiple `Authenticator` rows (e.g. several WebAuthn keys), but at most one `totp`, one `recovery_codes`, and one `email` row each (a database constraint enforces this; migration `0008_email_factor` extended it from `totp`/`recovery_codes` to also cover `email` when that factor was added). - `MfaUserHandle` (`django_mfa/handles.py`) -- a new model, one row per user, holding the stable opaque WebAuthn "user handle" passkey ceremonies use to resolve a user before any password is typed. Created lazily, the first time a user's handle is needed. Migration `0005_migrate_to_authenticator` copies every existing `UserOTP` row to an `Authenticator` row of type `totp`, and every user's `UserRecoveryCodes` rows to a single `Authenticator` row of type `recovery_codes` holding all of that user's codes together (see item 10 below for what state those codes are in afterwards). Migration `0007` then drops the old tables -- see item 8. diff --git a/pyproject.toml b/pyproject.toml index dd539bd..604a740 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "django-mfa" -version = "4.0.1" +version = "4.1.0" description = "Second-factor authentication for Django: authenticator apps (TOTP), security keys and passkeys (WebAuthn), and recovery codes." readme = "README.md" requires-python = ">=3.10" diff --git a/uv.lock b/uv.lock index 0d1fed6..472f5d6 100644 --- a/uv.lock +++ b/uv.lock @@ -566,7 +566,7 @@ wheels = [ [[package]] name = "django-mfa" -version = "4.0.1" +version = "4.1.0" source = { editable = "." } dependencies = [ { name = "django", version = "5.2.17", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" },