diff --git a/backend/app/auth.py b/backend/app/auth.py index 48152cc1..29f664af 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -92,6 +92,36 @@ def is_allowed(db: Session, email: str) -> bool: return email in settings.allowed_email_set or email in settings.admin_email_set +def find_user_by_email(db: Session, email: str) -> User | None: + """The account someone means when they type an address at sign-in, password-reset or + verify-resend. None when there isn't exactly one answer. + + Exact spelling wins. Failing that, the address is resolved to its mailbox + (utils.canonical_email) — so a password chosen at registration still works when the person types + another spelling of their own Gmail, and so the three flows that look an account up by typed + address stop being dead ends. Before this, requesting a reset for `xy@gmail.com` on an account + registered as `x.y@gmail.com` issued nothing and said nothing: the endpoint answers uniformly by + design, so the user got silence and no way in. + + SEVERAL canonical candidates return None rather than a guess. That case is real — prod carries two + accounts for one mailbox — and picking one of them for a password sign-in would hand someone the + wrong account. The callers all treat None as "no such account", which is the uniform, + non-enumerating answer they already gave. + """ + if not email: + return None + email = email.strip().lower() + exact = db.execute(select(User).where(User.email == email)).scalar_one_or_none() + if exact is not None: + return exact + candidates = ( + db.execute(select(User).where(User.email_canonical == canonical_email(email))) + .scalars() + .all() + ) + return candidates[0] if len(candidates) == 1 else None + + def get_or_create_demo_user(db: Session) -> User: """The one shared demo user, created on first use. No OAuth token / YouTube scope, so has_read_scope/has_write_scope are False and every YouTube-touching path is closed to it.""" diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index a5930d6c..bd262239 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -25,6 +25,7 @@ from app.auth import ( READ_SCOPES, WRITE_SCOPES, current_user, + find_user_by_email, get_or_create_demo_user, google_enabled, is_allowed, @@ -350,13 +351,7 @@ async def callback( log.info("Suspended account blocked at Google login: %s", email) _notify_suspended(background, user.email) return RedirectResponse(url="/?login=suspended") - # Keep the email in sync with Google, but never overwrite it with one another account already - # owns — that would hit the unique constraint and 500, wedging this user's sign-in (rare: a - # Google-side email change to a value that collides with a different local account). - if user.email != email: - clash = db.query(User).filter(User.email == email, User.id != user.id).one_or_none() - if clash is None: - user.email = email + _sync_google_email(db, user, email) user.display_name = userinfo.get("name") user.avatar_url = userinfo.get("picture") # ADMIN_EMAILS (env) is the bootstrap admin list and always wins, so the configured admin can't @@ -395,6 +390,30 @@ async def callback( return RedirectResponse(url="/") +def _sync_google_email(db: Session, user: User, email: str) -> None: + """Follow a genuine Google-side email change — and ONLY that. + + A different SPELLING of the same mailbox is the user's own choice and must survive. People + register as `me+siftlode@gmail.com` precisely so a later leak from this site is identifiable, and + silently rewriting that to `me@gmail.com` destroys the thing it was for. It also locked them out + of the spelling they chose, because sign-in matches the stored address first. Google returns the + account's PRIMARY address, which need not be what they registered with — and can never contain a + `+` at all — so without this guard every plus-tagged registration lost its tag on the first Google + sign-in. (E9.5 S3 made that reachable by matching adoption across spellings; the rewrite itself is + older.) + + Never overwrite with an address another account already owns either: that would hit the unique + constraint and 500, wedging this user's sign-in. + """ + if not email or user.email == email: + return + if canonical_email(user.email) == canonical_email(email): + return # same mailbox, their spelling — leave it alone + clash = db.query(User).filter(User.email == email, User.id != user.id).one_or_none() + if clash is None: + user.email = email + + def _store_token(db: Session, user: User, token: dict) -> None: """Persist the OAuth grant on the user's single token row, creating it if absent. Google only returns a refresh_token on (re)consent, so the previous one is kept otherwise. @@ -740,10 +759,13 @@ def _resend_verify_if_eligible(email: str) -> None: if not email_mod.email_enabled(): return with SessionLocal() as db: - user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() + user = find_user_by_email(db, email) if user is not None and not user.email_verified and not user.is_demo: raw = _issue_token(db, user, "verify", VERIFY_TTL) - email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}") + # To the address ON THE ACCOUNT, not the one just typed: the account's address is the one + # its owner chose to be reachable at, and a `+tag` there is usually deliberate (it tells + # them which site leaked, if one ever does). Same mailbox either way for a Gmail alias. + email_mod.send_verify_email(user.email, f"{_app_base()}/#verify={raw}") @router.post("/password-login") @@ -760,7 +782,7 @@ def password_login( raise HTTPException(status_code=429, detail="Too many attempts. Try again shortly.") email = (body.email or "").strip().lower() password = body.password or "" - user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() + user = find_user_by_email(db, email) # Always run the KDF (against a decoy hash for an unknown / passwordless / demo account) so the # response time is the same whether or not the email exists — no timing-based enumeration. candidate_hash = user.password_hash if (user and not user.is_demo) else None @@ -816,12 +838,13 @@ def _send_reset_if_eligible(email: str) -> None: ineligible on purpose: its owner adds one from Settings (/auth/set-password) while signed in, which proves identity through the session rather than by mail.""" with SessionLocal() as db: - user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() + user = find_user_by_email(db, email) if user is None or user.is_demo: return if user.password_hash or user.password_revoked_at: raw = _issue_token(db, user, "reset", RESET_TTL) - email_mod.send_password_reset(email, f"{_app_base()}/#reset={raw}") + # The account's own address, as above — never the spelling that was typed into the form. + email_mod.send_password_reset(user.email, f"{_app_base()}/#reset={raw}") @router.post("/password-reset/confirm") diff --git a/backend/tests/test_email_canonical.py b/backend/tests/test_email_canonical.py index a0cf4fb6..814ebe22 100644 --- a/backend/tests/test_email_canonical.py +++ b/backend/tests/test_email_canonical.py @@ -156,3 +156,140 @@ def test_manually_whitelisting_another_spelling_reuses_the_row(db): rows = db.execute(select(Invite)).scalars().all() assert len(rows) == 1 assert rows[0].email == "hartmann.peter1986@gmail.com" # the original spelling is preserved + + +def _capture_reset_links(monkeypatch) -> list[str]: + """Patch the reset mailer and collect the recipients it is asked to write to.""" + from app.routes import auth as auth_mod + + sent: list[str] = [] + monkeypatch.setattr(auth_mod.email_mod, "send_password_reset", lambda to, link: sent.append(to)) + return sent + + +# --- signing in and recovering an account, whichever spelling of it the person types ------------- +# +# The three flows that look an account up BY TYPED ADDRESS used to match literally, so a Gmail user +# who registered `x.y@` and typed `xy@` met "Invalid email or password" at sign-in — and, worse, +# total silence from password-reset and verify-resend, which answer uniformly by design. + + +def test_the_password_works_with_any_spelling_of_the_same_mailbox(db): + from app.auth import find_user_by_email + + make = User( + email="t.e.st+tag@gmail.com", email_verified=True, is_active=True, password_hash="x" + ) + db.add(make) + db.commit() + + for typed in ( + "t.e.st+tag@gmail.com", # as registered + "test@gmail.com", # the mailbox itself + "t.e.st@gmail.com", # dots only + "TEST@googlemail.com", # the alias domain + ): + assert find_user_by_email(db, typed) is make, typed + + +def test_an_unrelated_address_still_finds_nothing(db): + from app.auth import find_user_by_email + + db.add(User(email="someone@gmail.com", email_verified=True, is_active=True)) + db.commit() + + assert find_user_by_email(db, "someoneelse@gmail.com") is None + assert find_user_by_email(db, "some.one@example.com") is None # dots matter off Gmail + + +def test_the_exact_spelling_wins_when_two_accounts_share_a_mailbox(db): + # Prod carries such a pair (ids 13 and 14). Whoever types their own address in full must land on + # their own account, not on whichever row happens to come first. + from app.auth import find_user_by_email + + dotted = User(email="npeter83.temu@gmail.com", email_verified=True, is_active=True) + plain = User(email="npeter83temu@gmail.com", email_verified=True, is_active=True) + db.add_all([dotted, plain]) + db.commit() + + assert find_user_by_email(db, "npeter83.temu@gmail.com") is dotted + assert find_user_by_email(db, "npeter83temu@gmail.com") is plain + + +def test_an_ambiguous_spelling_refuses_rather_than_guessing(db): + # Neither spelling is exact, and both are the same mailbox. Handing a password sign-in one of + # them at random would let someone into the wrong account, so this answers "no such account" — + # the same uniform reply the caller already gives. + from app.auth import find_user_by_email + + db.add(User(email="npeter83.temu@gmail.com", email_verified=True, is_active=True)) + db.add(User(email="npeter83temu@gmail.com", email_verified=True, is_active=True)) + db.commit() + + assert find_user_by_email(db, "n.peter83.temu+x@gmail.com") is None + + +def test_a_reset_link_goes_to_the_address_on_the_account(db, monkeypatch): + """Typing another spelling must still reach the owner — and the mail must go to the address they + registered with, tag and all, not to the one typed into the form.""" + from app.routes.auth import _send_reset_if_eligible + + db.add( + User( + email="me+siftlode@gmail.com", + email_verified=True, + is_active=True, + password_hash="x", + ) + ) + db.commit() + sent = _capture_reset_links(monkeypatch) + + _send_reset_if_eligible("me@gmail.com") + + assert sent == ["me+siftlode@gmail.com"] + + +# --- a Google sign-in must not rewrite the address its owner chose ------------------------------ + + +def test_a_google_sign_in_keeps_a_plus_tagged_address(db): + """People register as `me+siftlode@gmail.com` so a later leak from this site is identifiable. + Google returns the account's primary address, which can never contain `+`, so following it + blindly destroyed the tag on the first Google sign-in — and, since sign-in matches the stored + address first, stopped them signing in with the address they had chosen.""" + from app.routes.auth import _sync_google_email + + user = User(email="me+siftlode@gmail.com", email_verified=True, is_active=True) + db.add(user) + db.commit() + + _sync_google_email(db, user, "me@gmail.com") + + assert user.email == "me+siftlode@gmail.com" + + +def test_a_google_sign_in_follows_a_real_mailbox_change(db): + # The case the sync exists for: the Google account genuinely moved to a different mailbox. + from app.routes.auth import _sync_google_email + + user = User(email="old.address@gmail.com", email_verified=True, is_active=True) + db.add(user) + db.commit() + + _sync_google_email(db, user, "brand-new@example.com") + + assert user.email == "brand-new@example.com" + + +def test_a_mailbox_change_is_refused_when_another_account_owns_it(db): + from app.routes.auth import _sync_google_email + + db.add(User(email="taken@example.com", email_verified=True, is_active=True)) + user = User(email="mover@gmail.com", email_verified=True, is_active=True) + db.add(user) + db.commit() + + _sync_google_email(db, user, "taken@example.com") + + assert user.email == "mover@gmail.com" # unique constraint would have 500'd the sign-in