Round 6's two remaining nits. The operator note said "Configuration → Email"
but the group renders as "Email / SMTP" in both locales, and
test_an_admin_can_activate_a_pre_approved_registrant requested make_user without
using it — a leftover from the reverted rounds that made the test look like it
seeded an admin when it did not.
Round 5's correctness/security pass came back clean; what was left was text that
said more than the code does.
- The register-path notify comment (and its test) claimed the admin "is still
told". The call fires, but on a no-SMTP instance _send drops it — which is the
open half of the problem, not something this branch fixed. Both now say so.
- The release notes tell admins what to actually do: an account whose address was
whitelisted in advance has no pending request to approve, so the whitelist box
is what activates it. That instruction only existed in the roadmap and in
chores, which the dialog does not render.
- "That one line is the whole production change" now accounts for the test-only
RateLimiter.reset() that also ships.
- The rate-limiter test claimed to pin that blocked attempts are not counted, on
an assertion that held either way. Replaced with one that drives the clock
across the window boundary and fails if hammering extends it.
Round 4 showed the `inv is not None` gate added in b5a1002 removed a signal that
matters: for an address the admin had already approved, that email is their only
notice that someone has now created an account on it — the invite is not pending
so nothing lands in the Access-requests queue either. Suppressing it left the
registrant at "awaiting admin approval" with the admin unaware they existed. The
duplicate it avoided (Request access, then sign up) is mild noise by comparison.
The production change is now one db.commit(), which is all the reported bug ever
needed. Release notes corrected again: a previously-denied address was also
unaffected, the pending-request case never lost its queue entry (only the
account behind it), and the test inventory was missing a guard.
Round 3 of the review killed the reopen from b5a1002: flipping an approved
invite back to pending let an unauthenticated POST /auth/register REVOKE a
standing approval for any whitelisted-but-unclaimed address, which then fails
is_allowed() and locks the real owner out of Google sign-in — the primary way
people get in here. It also re-armed the pre-emption window it was meant to
close, because with no SMTP email_verified is set True and the SEC-10 guard the
design leaned on never fires.
Two attempts at fixing the stranded-account UX in the same change both came back
worse than the wart. What remains is the narrow, real bug: the account was
silently rolled back and is now committed. An admin still binds the approval to
the account, exactly as before this branch. UX-25 in the roadmap records the
follow-up and both dead ends so they are not retried.
Also from round 3: the upsert docstring no longer claims the admin UI cannot
reach these accounts (re-entering the address in the same panel's whitelist box
does, and a test now pins it), the release notes are scoped to what the diff
actually changes with the operator note moved where the dialog renders it, and
RateLimiter has unit tests so reset() is not production code only a fixture
calls.
Replaces the activation added in ea931dc, which round 2 of the review showed was
a real weakening: on a no-SMTP instance it made a standing whitelist entry a
bearer grant — the first anonymous party to register an approved-but-unclaimed
address got an immediately usable account under that identity, with no
attestation, no admin action and no signal anywhere. The verify-time activation
had the same shape one step removed, and neither site ran the SEC-10 preset-
password guard or left an audit trail.
Keep a human in the loop instead. An approval granted before any account existed
was a decision about the ADDRESS, so registration reopens the invite
(upsert_pending_invite gains revive_approved, used only by the register worker).
The request lands in the Access-requests queue, approving it runs
_activate_user_by_email — and with it revoke_preset_password_if_unverified — and
the account activates. One mechanism, already audited, no new grant path, and
the "awaiting admin approval" copy is honest again.
The explicit commit from the previous commit still matters: with an invite
already pending, upsert returns without committing and _issue_token is gated on
SMTP, so a no-SMTP registration was still being rolled back.
Also from round 2: gate the admin notice on a fresh decision (matching the two
other upsert callers) so a retry no longer re-pings every admin, and reset the
module-level rate limiters between tests — the suite pooled every login into one
300s window and was two logins short of failing an unrelated test with a 429.
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.
_register_account() flushed the new User but relied on someone else to commit:
upsert_pending_invite() commits only when it actually writes an invite row, and
_issue_token() runs only when SMTP is enabled. For an email an admin had ALREADY
approved, on an instance where email is unavailable, neither fired — the session
closed and the account was rolled back. The would-be user got the uniform "ok"
and no account, could not sign in, and re-registering was a silent no-op.
Commit explicitly after the invite upsert (a no-op when upsert already committed
the same transaction). Three DB-lane regression tests cover the pre-approved
no-SMTP path (fails without the fix), the unknown-email path that always worked,
and the pre-approved path with SMTP on.
SEC-10 (account pre-emption / "pre-hijack"): a password set on an account whose
email was never verified is untrusted — whoever registered the address (to
pre-empt the real owner) could have chosen it. Both the Google-adoption callback
and admin activation (_activate_user_by_email) now drop that pre-set password and
bump the session epoch when the account isn't email_verified, so a later Google
login / admin approval can't hand the pre-set password a working login. No-op for
a verified account (the legit no-SMTP flow). +2 tests.
SEC-11 (link/upgrade bound to the session default): the OAuth link/upgrade GETs
resolved to the session DEFAULT account, so a tab acting as a secondary account
would link Google / the YouTube grant to the wrong account. The frontend now
threads the active account through the navigation URL (withActiveAccount ->
?account=<active>), and _complete_link accepts any wallet account, not only the
default, so the identity lands on the account that started the flow.
Address the /code-review findings on the S2 test-net:
- as_user no longer sets a global `lambda: user` override that two users in one
test would collide on; the shared current_user override now resolves the caller
from an x-test-user header, and each returned client tags its own requests, so
as_user(a)/as_user(b) are independent (#1).
- override_dep records the prior value per dependency key and restores it on
teardown instead of a blanket dependency_overrides.clear(), so fixtures compose
without clobbering each other (#5).
- a _configured fixture bakes the post-setup state into client/as_user, and the
per-test inline monkeypatch / autouse duplicates are removed (#4).
- the feed keyset walk asserts all seeded rows surfaced before comparing, so a
seeding regression can't make the sorts pass vacuously (#3).
The auth security mag had zero tests. Add a DB-lane suite over the real
auth/session stack (TestClient carries the signed cookie): password login ->
/api/me authenticated; a wrong password and an unknown email return a
byte-identical 401 (no enumeration oracle); an X-Siftlode-Account header naming
a non-wallet account falls back to the default (impersonation gate); logout-others
bumps the epoch so a second session is rejected (401 on a protected endpoint)
while the caller stays in; and password-reset/confirm is single-use (replay ->
400) and bumps the epoch so pre-reset sessions die.
peter