fix(auth): honour an approval granted before sign-up instead of parking the account
Follow-up to the commit fix, from the deep-review. Persisting the account was only half the story: when the admin had approved the address BEFORE the person registered, the account landed is_active=False against an invite that is no longer pending — absent from the Access-requests queue, and approve_invite 409s on an already-decided invite. The sign-up worked and still could not sign in. Derive activation from is_allowed() at the moment ownership is attested: - verify_email_confirm activates a pre-approved account when the emailed link is confirmed (only the mailbox owner can follow it), so SEC-10 stays intact; - _register_account activates only when there is no SMTP to verify with, where the admin whitelist is by design the sole gate — the same one add_invite already activates on. Tests: the pre-approved no-SMTP case and the verify-activates case are real regression tests (both fail without this change); guards cover the unknown-email and denied->pending branches, verification of an unapproved account, and the first end-to-end coverage of the /auth/register route and its allow_registration gate. Release notes rewritten — the previous entry miscounted its regression tests and advertised an email fix this release has no code for.
This commit is contained in:
@@ -637,6 +637,15 @@ def _register_account(email: str, password: str) -> None:
|
||||
db.rollback()
|
||||
return
|
||||
upsert_pending_invite(db, email) # admin-approval gate (commits only when it writes a row)
|
||||
# Honour an approval the admin had ALREADY given for this address. Without this the account
|
||||
# lands is_active=False against a non-pending invite: it never shows up in the Access-requests
|
||||
# queue (which lists pending only) and approve_invite 409s on an already-decided invite, so
|
||||
# the admin has no obvious way to let the person in. Attestation still gates it — with SMTP on
|
||||
# the address is unverified at this point, so activation waits for the verify link (see
|
||||
# verify_email_confirm). With no SMTP there is no attestation to wait for and the admin's
|
||||
# whitelist is by design the only gate, the very one add_invite already activates on.
|
||||
if not email_ok and is_allowed(db, email):
|
||||
user.is_active = True
|
||||
# Persist the account ourselves. upsert_pending_invite commits only when it actually inserts
|
||||
# (or revives) an invite row, and _issue_token below only runs with SMTP on — so for an email
|
||||
# the admin had ALREADY approved, on an instance with no SMTP, nothing along this path
|
||||
@@ -663,6 +672,12 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict:
|
||||
status_code=400, detail="This verification link is invalid or has expired."
|
||||
)
|
||||
user.email_verified = True
|
||||
# Ownership is attested now (only the mailbox owner can follow this link), so an approval the
|
||||
# admin granted BEFORE the person signed up can finally be honoured. Otherwise they sit at
|
||||
# is_active=False behind a request no admin screen shows — the invite is already decided, so it
|
||||
# is absent from the pending queue and approve_invite would 409 on it.
|
||||
if not user.is_active and is_allowed(db, user.email):
|
||||
user.is_active = True
|
||||
db.commit()
|
||||
return {"ok": True}
|
||||
|
||||
|
||||
@@ -145,8 +145,9 @@ def _register_and_fetch(db, email: str):
|
||||
|
||||
|
||||
def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp(db, monkeypatch):
|
||||
# The regression: an admin pre-approves an address, the person then registers, and the account
|
||||
# silently never existed. upsert_pending_invite returns early WITHOUT committing for an
|
||||
# THE regression (the only one of these that fails without the explicit commit in
|
||||
# _register_account): an admin pre-approves an address, the person then registers, and the
|
||||
# account silently never existed. upsert_pending_invite returns early WITHOUT committing for an
|
||||
# already-approved email, and the only other commit on this path (_issue_token) is gated on
|
||||
# SMTP — so on a no-SMTP instance the flushed user was rolled back at session close.
|
||||
from app.models import Invite
|
||||
@@ -159,16 +160,21 @@ def test_registration_of_a_pre_approved_email_persists_the_account_without_smtp(
|
||||
user, invite = _register_and_fetch(db, "roland@example.com")
|
||||
|
||||
assert user is not None, "pre-approved registration was rolled back — the account is gone"
|
||||
assert user.is_active is False # admin approval still gates sign-in
|
||||
assert user.password_hash is not None
|
||||
assert user.email_verified is True # no SMTP => ownership can't be proven by link
|
||||
assert invite.status == "approved" # an existing approval is never disturbed
|
||||
# With no SMTP there is no attestation to wait for, so the admin's standing approval IS the gate
|
||||
# and the account is usable at once. Otherwise it would sit inactive behind an invite that is no
|
||||
# longer pending — invisible in the Access-requests queue, and approve_invite 409s on it.
|
||||
assert user.is_active is True
|
||||
|
||||
|
||||
def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite(
|
||||
db, monkeypatch
|
||||
):
|
||||
# The branch that always worked (upsert INSERTs the invite and commits) must keep working.
|
||||
# Guard, not a regression test: this branch always worked (upsert INSERTs the invite and commits
|
||||
# the same transaction). It must keep working — and the account must stay inactive, since nobody
|
||||
# has approved this address.
|
||||
from app.routes import auth as auth_mod
|
||||
|
||||
monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False)
|
||||
@@ -176,11 +182,33 @@ def test_registration_of_an_unknown_email_still_persists_account_and_pending_inv
|
||||
user, invite = _register_and_fetch(db, "stranger@example.com")
|
||||
|
||||
assert user is not None
|
||||
assert user.is_active is False
|
||||
assert invite is not None and invite.status == "pending"
|
||||
|
||||
|
||||
def test_registration_of_a_previously_denied_email_revives_the_request(db, monkeypatch):
|
||||
# Guard: a denied person may ask again — upsert_pending_invite flips the invite back to pending
|
||||
# (and commits) so the admin sees a fresh request. The stale decision must not leak into the new
|
||||
# account's state, and re-registering must not be a dead end.
|
||||
from app.models import Invite
|
||||
from app.routes import auth as auth_mod
|
||||
|
||||
monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False)
|
||||
db.add(Invite(email="denis@example.com", status="denied", decided_by="admin@example.com"))
|
||||
db.commit()
|
||||
|
||||
user, invite = _register_and_fetch(db, "denis@example.com")
|
||||
|
||||
assert user is not None
|
||||
assert user.is_active is False
|
||||
assert invite.status == "pending"
|
||||
assert invite.decided_at is None and invite.decided_by is None
|
||||
|
||||
|
||||
def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db, monkeypatch):
|
||||
# Same pre-approved path with SMTP on: the account must persist AND get a verification link.
|
||||
# Guard, not a regression test: with SMTP on, _issue_token's own commit already persisted the
|
||||
# user, so this branch was never broken and must stay unbroken. Here the account waits for the
|
||||
# verify link (see the activation test below), so it is inactive and unverified.
|
||||
from app.models import Invite
|
||||
from app.routes import auth as auth_mod
|
||||
|
||||
@@ -195,4 +223,86 @@ def test_registration_of_a_pre_approved_email_persists_the_account_with_smtp(db,
|
||||
|
||||
assert user is not None
|
||||
assert user.email_verified is False # must confirm the address
|
||||
assert user.is_active is False
|
||||
assert sent == ["rita@example.com"]
|
||||
|
||||
|
||||
def test_verifying_an_email_activates_a_pre_approved_account(client, db, make_user):
|
||||
# Confirming the emailed link attests ownership, so an approval the admin granted before the
|
||||
# person signed up is honoured right there. Without this the SMTP-on pre-approved registrant is
|
||||
# stuck: their invite is already decided, so it never reaches the Access-requests queue and
|
||||
# approve_invite 409s on it.
|
||||
from app.models import Invite
|
||||
from app.routes.auth import _issue_token
|
||||
|
||||
user = make_user(email="rita@example.com", password=_PW, email_verified=False, is_active=False)
|
||||
db.add(Invite(email="rita@example.com", status="approved", decided_by="admin@example.com"))
|
||||
db.commit()
|
||||
raw = _issue_token(db, user, "verify", timedelta(hours=1))
|
||||
|
||||
assert client.post("/auth/verify", json={"token": raw}).status_code == 200
|
||||
|
||||
db.expire_all()
|
||||
assert user.email_verified is True
|
||||
assert user.is_active is True
|
||||
assert _login(client, "rita@example.com", _PW).status_code == 200
|
||||
|
||||
|
||||
def test_verifying_an_email_does_not_activate_an_unapproved_account(client, db, make_user):
|
||||
# The mirror guard: verification proves the address, it does not grant access. With no approved
|
||||
# invite the account stays inactive and sign-in is still refused.
|
||||
from app.routes.auth import _issue_token
|
||||
|
||||
user = make_user(
|
||||
email="nobody@example.com", password=_PW, email_verified=False, is_active=False
|
||||
)
|
||||
raw = _issue_token(db, user, "verify", timedelta(hours=1))
|
||||
|
||||
assert client.post("/auth/verify", json={"token": raw}).status_code == 200
|
||||
|
||||
db.expire_all()
|
||||
assert user.email_verified is True
|
||||
assert user.is_active is False
|
||||
assert _login(client, "nobody@example.com", _PW).status_code == 403
|
||||
|
||||
|
||||
def test_register_route_creates_the_account_end_to_end(client, db, monkeypatch):
|
||||
# The tests above call _register_account directly, so nothing covered the ROUTE: its
|
||||
# allow_registration gate, its rate limiter and the background.add_task wiring that makes the
|
||||
# whole flow reachable at all. TestClient runs BackgroundTasks after the response, so a single
|
||||
# POST exercises the real path end to end.
|
||||
from sqlalchemy import select as _select
|
||||
|
||||
from app.models import User
|
||||
from app.routes import auth as auth_mod
|
||||
|
||||
monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False)
|
||||
|
||||
r = client.post("/auth/register", json={"email": "newbie@example.com", "password": _PW})
|
||||
|
||||
assert r.status_code == 200 and r.json() == {"status": "ok"}
|
||||
db.expire_all()
|
||||
user = db.execute(_select(User).where(User.email == "newbie@example.com")).scalar_one_or_none()
|
||||
assert user is not None, "POST /auth/register answered ok but created nothing"
|
||||
assert user.password_hash is not None
|
||||
|
||||
|
||||
def test_register_route_is_refused_when_registration_is_closed(client, db, monkeypatch):
|
||||
# The gate is an explicit 403 with nothing created — not a uniform "ok" that silently drops it.
|
||||
from sqlalchemy import select as _select
|
||||
|
||||
from app import sysconfig
|
||||
from app.models import User
|
||||
from app.routes import auth as auth_mod
|
||||
|
||||
monkeypatch.setattr(auth_mod.email_mod, "email_enabled", lambda: False)
|
||||
sysconfig.set_value(db, "allow_registration", False)
|
||||
|
||||
r = client.post("/auth/register", json={"email": "closed@example.com", "password": _PW})
|
||||
|
||||
assert r.status_code == 403
|
||||
db.expire_all()
|
||||
assert (
|
||||
db.execute(_select(User).where(User.email == "closed@example.com")).scalar_one_or_none()
|
||||
is None
|
||||
)
|
||||
|
||||
@@ -18,13 +18,14 @@ export const RELEASE_NOTES: ReleaseEntry[] = [
|
||||
version: "0.78.5",
|
||||
date: "2026-09-01",
|
||||
summary:
|
||||
"Account emails are being delivered again — and a sign-up by someone the admin had already approved no longer vanishes without a trace.",
|
||||
"Signing up with an address the admin had already approved now works end to end — the account is created, and confirming your email lets you straight in instead of parking you behind an approval that already happened.",
|
||||
fixes: [
|
||||
"Email is going out again. Verification links, password-reset links, approval and denial notices, and the “new access request” alert to the admin were all being silently dropped because the mail settings were incomplete — the app skipped each message instead of sending it. Check Configuration → Email and use “Send test email” if yours has ever gone quiet.",
|
||||
"Signing up with an address an admin had already approved now actually creates the account. The sign-up reported success but the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.",
|
||||
"Signing up with an address an admin had already approved now actually creates the account. Before, the sign-up reported success and the account was discarded on the spot, so the person could neither sign in nor ask for access again — they simply had no account at all.",
|
||||
"Such an account no longer waits for an approval it already has. Confirming the emailed link now activates it straight away, and on an instance that sends no email the admin's whitelist alone is enough — previously the sign-up landed in a state the Access-requests screen never showed, because the request had already been decided.",
|
||||
],
|
||||
chores: [
|
||||
"Registration now commits the new account explicitly. For an already-approved email, upsert_pending_invite() returns without committing, and the only other commit on that path is gated on working SMTP — so with mail unavailable the flushed user row was rolled back when the background task's session closed. Covered by three DB-lane regression tests: pre-approved without SMTP, unknown email, and pre-approved with SMTP.",
|
||||
"Registration commits the new account explicitly: for an already-approved email, upsert_pending_invite() returns without committing and the only other commit on that path is gated on working SMTP, so with mail unavailable the flushed user row was rolled back when the background task's session closed. Activation is now derived from is_allowed() at the moment ownership is attested — at email verification, or at registration when the instance has no email to verify with. Covered by one regression test plus guards for every other branch of the worker, and the first end-to-end tests of the /auth/register route itself.",
|
||||
"Operator note: an incomplete SMTP setting makes Siftlode skip every message silently by design. If your instance has never delivered mail, check Configuration → Email and use “Send test email”.",
|
||||
],
|
||||
},
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user