fix(auth): becoming admin through ADMIN_EMAILS drops the password, on any sign-in
The promotion revoke guessed from today's SMTP state whether the password was trustworthy, and ran only when the account was adopted. Both guesses failed: a row verified while SMTP was off stays "verified" after SMTP is configured, and a later sign-in matched by Google identity promoted the account with no revoke at all. Either way a stranger's password and session could become admin. Now one rule, where the promotion happens: when ADMIN_EMAILS turns a non-admin account into an admin and it holds a password, the password goes and every session ends; the owner, signed in through Google at that moment, sets a new one in Settings. An account that already is admin keeps its password. Tests: the activated squat, the SMTP case, the later sign-in (all three red without the rule) and an admin's own sign-in.
This commit is contained in:
+26
-25
@@ -446,11 +446,7 @@ 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,
|
||||
becomes_admin=user.role != "admin"
|
||||
and canonical_email(email) in env_admin_canonicals(),
|
||||
)
|
||||
revoke_untrusted_preset_password(user)
|
||||
# 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
|
||||
@@ -481,7 +477,16 @@ async def callback(
|
||||
# default for a brand-new account (its role attribute is still unset before flush).
|
||||
# Canonical, like every other read of the env lists: a dotted or `+tag`ged ADMIN_EMAILS entry
|
||||
# would otherwise never bootstrap its admin, because Google answers with the plain primary.
|
||||
#
|
||||
# Becoming admin this way drops any password the account holds, on whichever sign-in it
|
||||
# happens. Nothing here can tell who chose that password: without SMTP registration marks the
|
||||
# address verified with no proof (and SMTP configured later never revisits it), and an admin's
|
||||
# Activate vouches for access, not for the password. So a stranger who registered the address
|
||||
# first, and was let in, would otherwise see their own live session become an admin one. The
|
||||
# real owner, signed in through Google right now, sets a new password in Settings.
|
||||
if canonical_email(email) in env_admin_canonicals():
|
||||
if user.role != "admin" and user.password_hash is not None:
|
||||
_revoke_password(user)
|
||||
user.role = "admin"
|
||||
elif not user.role:
|
||||
user.role = "user"
|
||||
@@ -1144,24 +1149,18 @@ def bump_session_epoch(user: User) -> None:
|
||||
user.session_epoch = (user.session_epoch or 0) + 1
|
||||
|
||||
|
||||
def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False) -> None:
|
||||
def revoke_untrusted_preset_password(user: User) -> 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.
|
||||
|
||||
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.
|
||||
A NEVER-ACTIVATED account is untrusted too, verified or not: without SMTP, registration marks the
|
||||
address verified with no proof at all. 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
|
||||
with their own password still working. (An account the callback makes ADMIN loses its password
|
||||
whatever its state — see the ADMIN_EMAILS block there.)
|
||||
|
||||
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
|
||||
@@ -1183,14 +1182,16 @@ def revoke_untrusted_preset_password(user: User, *, becomes_admin: bool = False)
|
||||
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)
|
||||
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)
|
||||
if not user.email_verified or (not user.is_active and user.password_hash is not None):
|
||||
_revoke_password(user)
|
||||
|
||||
|
||||
def _revoke_password(user: User) -> None:
|
||||
"""Drop the account's password, stamp why (so password-reset still accepts it) and end every
|
||||
session it signed in."""
|
||||
user.password_hash = None
|
||||
user.password_revoked_at = datetime.now(timezone.utc)
|
||||
bump_session_epoch(user)
|
||||
|
||||
|
||||
@router.post("/logout-others")
|
||||
|
||||
Reference in New Issue
Block a user