fix(auth): a Google sign-in that makes an account admin drops a password nobody proved
The never-activated rule left one order of events open: on an instance without SMTP, a stranger who registered an ADMIN_EMAILS address (or a sibling spelling) and was then activated by another admin kept their password and live session, and the owner's first Google sign-in raised that row to admin, the stranger's cookie with it. Activation vouches for access, not for who chose the password, and without SMTP nothing else ever proved the mailbox. When ADMIN_EMAILS promotes an adopted account that holds a password on an instance without SMTP, the password goes and the session epoch is bumped. With SMTP the verified flag is a proven mailbox, and an account the sign-in does not promote keeps its password, so the "your password was cleared" detour stays off the default path. Tests: the activated squat (red without the rule; the squatter's session is signed out) and the SMTP case that keeps the password.
This commit is contained in:
@@ -446,7 +446,11 @@ async def callback(
|
||||
# SEC-10: drop any password nobody proved the mailbox for, now that Google attests
|
||||
# ownership (bumps the epoch to revoke any session it minted). Runs before the
|
||||
# activation below, which is what it reads.
|
||||
revoke_untrusted_preset_password(user)
|
||||
revoke_untrusted_preset_password(
|
||||
user,
|
||||
becomes_admin=user.role != "admin"
|
||||
and canonical_email(email) in env_admin_canonicals(),
|
||||
)
|
||||
# Google verified the email and is_allowed granted access, so finish activating a
|
||||
# previously pending password registration (otherwise current_user would bounce it).
|
||||
user.email_verified = True
|
||||
@@ -1140,18 +1144,24 @@ def bump_session_epoch(user: User) -> None:
|
||||
user.session_epoch = (user.session_epoch or 0) + 1
|
||||
|
||||
|
||||
def revoke_untrusted_preset_password(user: User) -> None:
|
||||
def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False) -> None:
|
||||
"""SEC-10 (account pre-emption): 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. When
|
||||
the real owner attests ownership through Google, drop that password and bump the epoch to revoke any
|
||||
session it minted; the owner re-sets one via password-reset, or from Settings while the Google
|
||||
session lasts. No-op for a verified account that was already let in.
|
||||
session lasts.
|
||||
|
||||
A NEVER-ACTIVATED account is untrusted too, verified or not. Without SMTP, registration marks
|
||||
the address verified with no proof at all, so "verified" says nothing about who chose the
|
||||
password; activation is what an admin (or this very sign-in) grants. For an ADMIN_EMAILS /
|
||||
ALLOWED_EMAILS address no invite row gates the registration, so a stranger who registered it
|
||||
first would otherwise be activated here — as admin — with their own password still working.
|
||||
Two more cases are untrusted, because without SMTP registration marks the address verified with
|
||||
no proof at all, so "verified" says nothing about who chose the password:
|
||||
- A NEVER-ACTIVATED account. For an ADMIN_EMAILS / ALLOWED_EMAILS address no invite row gates the
|
||||
registration, so a stranger who registered it first would otherwise be activated here — as
|
||||
admin — with their own password still working.
|
||||
- An account this sign-in is about to make ADMIN (`becomes_admin`: ADMIN_EMAILS names it) on an
|
||||
instance without SMTP, even one an admin already activated. Activation is the admin vouching
|
||||
for access, not for who chose the password, and ADMIN_EMAILS would otherwise hand the admin
|
||||
role to a stranger's live session. With SMTP the verified flag is a proven mailbox, and an
|
||||
account that is neither of these keeps its password: clearing it for everyone is the default
|
||||
detour rejected below.
|
||||
|
||||
The Google-adoption callback is the ONLY caller. Admin approval used to call this too, and
|
||||
dropping it was right for the reason given — the revoke destroyed the password a legitimate
|
||||
@@ -1173,7 +1183,11 @@ def revoke_untrusted_preset_password(user: User) -> None:
|
||||
revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and
|
||||
_send_reset_if_eligible keyed off exactly that, so a revoked account silently got no reset link and
|
||||
was locked out for good (it could not sign in either). The stamp separates the two cases."""
|
||||
if not user.email_verified or (not user.is_active and user.password_hash is not None):
|
||||
if (
|
||||
not user.email_verified
|
||||
or (not user.is_active and user.password_hash is not None)
|
||||
or (becomes_admin and user.password_hash is not None and not email_mod.email_enabled())
|
||||
):
|
||||
user.password_hash = None
|
||||
user.password_revoked_at = datetime.now(timezone.utc)
|
||||
bump_session_epoch(user)
|
||||
|
||||
Reference in New Issue
Block a user