fix(auth): match identity on a canonical email, so one Gmail mailbox is one account (E9.5 S3)
Gmail delivers x.y@, xy@ and xy+tag@ to one inbox; the invite gate only lowercased. On 2026-09-03 that turned an approved user's Google sign-in into a silent SECOND access request -- OAuth returns the account's stored primary, which need not be the spelling they typed -- and left prod with two user rows for one mailbox (ids 13, 14). The +tag case fails every time: a Google account cannot contain +, so OAuth can only ever answer with the base address. Mail still goes to the address AS TYPED; only identity matching uses the canonical form. utils.canonical_email is the single definition, and a model validator derives users.email_canonical / invites.email_canonical on every write, so no call site can forget it. Scope is narrow on purpose: dots are significant on most providers and the Gmail rule does not hold for Workspace custom domains. Swept every sibling of the same principle, not just the reported one: is_allowed, upsert_pending_invite, the Google-callback adoption, _register_account (so no new duplicate pair can be created), _activate_user_by_email, admin _accounts_for (an invite backing a live user under another spelling used to look deletable) and add_invite, plus purge_user, which now erases every canonically-equal invite instead of leaving one behind that still grants access. Migration 0079 backfills both columns and indexes them NON-uniquely: prod already carries canonical duplicates (approved invite pairs since 2026-06-16), and merging accounts is a judgement call about people's data, not something a migration does on its own. A test pins the backfill SQL against the Python helper.
This commit is contained in:
@@ -0,0 +1,65 @@
|
|||||||
|
"""email_canonical on users + invites, for matching one mailbox across Gmail spellings
|
||||||
|
|
||||||
|
Revision ID: 0079_email_canonical
|
||||||
|
Revises: 0078_password_revoked_at
|
||||||
|
Create Date: 2026-09-04
|
||||||
|
|
||||||
|
Gmail delivers `x.y@gmail.com`, `xy@gmail.com` and `xy+tag@gmail.com` to ONE mailbox; the invite gate
|
||||||
|
only lowercased. On 2026-09-03 that turned an approved user's Google sign-in into a silent second
|
||||||
|
access request (OAuth returns the account's stored primary, which need not be the spelling they typed)
|
||||||
|
and left prod with two user rows for one mailbox.
|
||||||
|
|
||||||
|
The backfill below reproduces `app.utils.canonical_email` in SQL. That duplication is deliberate — a
|
||||||
|
migration must not import application code that will keep changing — but it means the two have to
|
||||||
|
agree; `test_canonical_email_backfill_matches_the_helper` pins them together.
|
||||||
|
|
||||||
|
Deliberately NOT unique. Prod already carries canonical duplicates: two approved invites for one
|
||||||
|
mailbox dating back to 2026-06-16, and two active user rows from the 2026-09-03 walkthrough. Merging
|
||||||
|
accounts is a judgement call about people's data, not something a migration gets to do on its own, so
|
||||||
|
this migration only records the canonical form and leaves the duplicates in place for a human.
|
||||||
|
"""
|
||||||
|
|
||||||
|
import sqlalchemy as sa
|
||||||
|
from alembic import op
|
||||||
|
|
||||||
|
revision: str = "0079_email_canonical"
|
||||||
|
down_revision: str | None = "0078_password_revoked_at"
|
||||||
|
branch_labels: str | None = None
|
||||||
|
depends_on: str | None = None
|
||||||
|
|
||||||
|
# Mirrors app.utils.canonical_email: Gmail (and its googlemail alias) loses dots and any `+tag` from
|
||||||
|
# the local part and lands on gmail.com; every other domain is only lowercased, because the dot rule
|
||||||
|
# is not guaranteed elsewhere — and does not hold for Workspace custom domains unless their admin
|
||||||
|
# enabled it.
|
||||||
|
_CANON_SQL = """
|
||||||
|
CASE
|
||||||
|
WHEN split_part(lower({col}), '@', 2) IN ('gmail.com', 'googlemail.com')
|
||||||
|
-- an empty local part after stripping falls through to plain lowercase, as the helper does
|
||||||
|
AND replace(split_part(split_part(lower({col}), '@', 1), '+', 1), '.', '') <> ''
|
||||||
|
THEN replace(split_part(split_part(lower({col}), '@', 1), '+', 1), '.', '') || '@gmail.com'
|
||||||
|
ELSE lower({col})
|
||||||
|
END
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
|
def _backfill(table: str) -> None:
|
||||||
|
op.execute(
|
||||||
|
f"UPDATE {table} SET email_canonical = {_CANON_SQL.format(col='email')} "
|
||||||
|
f"WHERE email IS NOT NULL"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade() -> None:
|
||||||
|
for table in ("users", "invites"):
|
||||||
|
op.add_column(
|
||||||
|
table,
|
||||||
|
sa.Column("email_canonical", sa.String(length=320), nullable=False, server_default=""),
|
||||||
|
)
|
||||||
|
_backfill(table)
|
||||||
|
op.create_index(f"ix_{table}_email_canonical", table, ["email_canonical"])
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade() -> None:
|
||||||
|
for table in ("users", "invites"):
|
||||||
|
op.drop_index(f"ix_{table}_email_canonical", table_name=table)
|
||||||
|
op.drop_column(table, "email_canonical")
|
||||||
+25
-5
@@ -12,6 +12,7 @@ from app.config import settings
|
|||||||
from app.db import get_db
|
from app.db import get_db
|
||||||
from app.models import Invite, User
|
from app.models import Invite, User
|
||||||
from app.security import decrypt_optional
|
from app.security import decrypt_optional
|
||||||
|
from app.utils import canonical_email
|
||||||
|
|
||||||
PASSWORD_MIN_LEN = 10
|
PASSWORD_MIN_LEN = 10
|
||||||
|
|
||||||
@@ -65,13 +66,29 @@ def has_write_scope(user: User) -> bool:
|
|||||||
|
|
||||||
def is_allowed(db: Session, email: str) -> bool:
|
def is_allowed(db: Session, email: str) -> bool:
|
||||||
"""May this email sign in? An approved Invite is the source of truth; the env
|
"""May this email sign in? An approved Invite is the source of truth; the env
|
||||||
ALLOWED_EMAILS/ADMIN_EMAILS remain a bootstrap fallback (e.g. a fresh DB)."""
|
ALLOWED_EMAILS/ADMIN_EMAILS remain a bootstrap fallback (e.g. a fresh DB).
|
||||||
|
|
||||||
|
Matched on the CANONICAL form (utils.canonical_email), not the literal string. Gmail delivers
|
||||||
|
every dotted / `+tagged` spelling to one mailbox, so approving one spelling always meant approving
|
||||||
|
that mailbox — matching literally only produced the 2026-09-03 lockout, where an approved user's
|
||||||
|
Google sign-in became a silent second access request because OAuth returned the undotted primary.
|
||||||
|
|
||||||
|
One canonical form can match SEVERAL invite rows (prod carries approved pairs for one mailbox,
|
||||||
|
the oldest from 2026-06-16). An approved row among them grants access: the API cannot flip an
|
||||||
|
approved invite to denied (`_decide` only accepts a pending one), so approved-beside-denied means
|
||||||
|
the admin approved that mailbox under a different spelling, not that they revoked it. Revoking is
|
||||||
|
deleting the invite row or suspending the account — both still work, since a deleted row is no
|
||||||
|
longer here to match and suspension is enforced at sign-in.
|
||||||
|
"""
|
||||||
if not email:
|
if not email:
|
||||||
return False
|
return False
|
||||||
|
canon = canonical_email(email)
|
||||||
|
statuses = (
|
||||||
|
db.execute(select(Invite.status).where(Invite.email_canonical == canon)).scalars().all()
|
||||||
|
)
|
||||||
|
if statuses:
|
||||||
|
return "approved" in statuses
|
||||||
email = email.lower()
|
email = email.lower()
|
||||||
inv = db.execute(select(Invite).where(Invite.email == email)).scalar_one_or_none()
|
|
||||||
if inv is not None:
|
|
||||||
return inv.status == "approved"
|
|
||||||
return email in settings.allowed_email_set or email in settings.admin_email_set
|
return email in settings.allowed_email_set or email in settings.admin_email_set
|
||||||
|
|
||||||
|
|
||||||
@@ -130,7 +147,10 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None:
|
|||||||
)
|
)
|
||||||
# Raw delete so Postgres applies ON DELETE CASCADE / SET NULL on every dependent table.
|
# Raw delete so Postgres applies ON DELETE CASCADE / SET NULL on every dependent table.
|
||||||
db.execute(delete(User).where(User.id == user_id))
|
db.execute(delete(User).where(User.id == user_id))
|
||||||
db.execute(delete(Invite).where(Invite.email == email))
|
# Every canonically-equal invite goes, not just the literal spelling: is_allowed now matches on
|
||||||
|
# the canonical form, so leaving a sibling row behind would keep granting access to a mailbox
|
||||||
|
# whose account was just erased — and prod really does carry approved pairs for one mailbox.
|
||||||
|
db.execute(delete(Invite).where(Invite.email_canonical == canonical_email(email)))
|
||||||
db.commit()
|
db.commit()
|
||||||
if google_token:
|
if google_token:
|
||||||
background.add_task(revoke_google_token, google_token)
|
background.add_task(revoke_google_token, google_token)
|
||||||
|
|||||||
+27
-3
@@ -17,9 +17,10 @@ from sqlalchemy import (
|
|||||||
func,
|
func,
|
||||||
)
|
)
|
||||||
from sqlalchemy.dialects.postgresql import JSONB, TSVECTOR
|
from sqlalchemy.dialects.postgresql import JSONB, TSVECTOR
|
||||||
from sqlalchemy.orm import Mapped, mapped_column, relationship
|
from sqlalchemy.orm import Mapped, mapped_column, relationship, validates
|
||||||
|
|
||||||
from app.db import Base
|
from app.db import Base
|
||||||
|
from app.utils import canonical_email
|
||||||
|
|
||||||
|
|
||||||
class TimestampMixin:
|
class TimestampMixin:
|
||||||
@@ -29,6 +30,21 @@ class TimestampMixin:
|
|||||||
created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now())
|
created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now())
|
||||||
|
|
||||||
|
|
||||||
|
class CanonicalEmailMixin:
|
||||||
|
"""Keeps `email_canonical` derived from `email` on every write.
|
||||||
|
|
||||||
|
The invariant — "identity is matched on the canonical form, mail is sent to the typed one" —
|
||||||
|
lives HERE, in code, instead of in a comment every write site has to remember. That distinction
|
||||||
|
is what the 2026-09-03 lockout was made of: one gate lowercased, nothing else did, and an
|
||||||
|
approved user's Google sign-in silently became a second access request.
|
||||||
|
"""
|
||||||
|
|
||||||
|
@validates("email")
|
||||||
|
def _derive_email_canonical(self, _key: str, value: str | None) -> str | None:
|
||||||
|
self.email_canonical = canonical_email(value)
|
||||||
|
return value
|
||||||
|
|
||||||
|
|
||||||
class UpdatedAtMixin:
|
class UpdatedAtMixin:
|
||||||
"""Adds an `updated_at` that server-defaults on insert and bumps on every UPDATE."""
|
"""Adds an `updated_at` that server-defaults on insert and bumps on every UPDATE."""
|
||||||
|
|
||||||
@@ -37,7 +53,7 @@ class UpdatedAtMixin:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
class User(Base, TimestampMixin):
|
class User(Base, TimestampMixin, CanonicalEmailMixin):
|
||||||
__tablename__ = "users"
|
__tablename__ = "users"
|
||||||
|
|
||||||
id: Mapped[int] = mapped_column(primary_key=True)
|
id: Mapped[int] = mapped_column(primary_key=True)
|
||||||
@@ -45,6 +61,11 @@ class User(Base, TimestampMixin):
|
|||||||
# multiple NULLs under a unique index). Set on Google sign-in / in-app linking.
|
# multiple NULLs under a unique index). Set on Google sign-in / in-app linking.
|
||||||
google_sub: Mapped[str | None] = mapped_column(String(64), unique=True, index=True)
|
google_sub: Mapped[str | None] = mapped_column(String(64), unique=True, index=True)
|
||||||
email: Mapped[str] = mapped_column(String(320), unique=True, index=True)
|
email: Mapped[str] = mapped_column(String(320), unique=True, index=True)
|
||||||
|
# The same mailbox, in the form used to MATCH identities (utils.canonical_email) — Gmail dots and
|
||||||
|
# `+tags` folded away. Kept by the validator below so no write site can forget it. Deliberately
|
||||||
|
# NOT unique: prod already carries two rows for one mailbox (a password registration and the
|
||||||
|
# Google sign-in that failed to find it), and a migration must not merge accounts on its own.
|
||||||
|
email_canonical: Mapped[str] = mapped_column(String(320), index=True, server_default="")
|
||||||
# argon2id hash for email+password login; NULL for Google-only / demo accounts.
|
# argon2id hash for email+password login; NULL for Google-only / demo accounts.
|
||||||
password_hash: Mapped[str | None] = mapped_column(String(255))
|
password_hash: Mapped[str | None] = mapped_column(String(255))
|
||||||
# When SEC-10 dropped a preset password on Google adoption (see the single caller of
|
# When SEC-10 dropped a preset password on Google adoption (see the single caller of
|
||||||
@@ -121,7 +142,7 @@ class AuthToken(Base, TimestampMixin):
|
|||||||
used_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True))
|
used_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True))
|
||||||
|
|
||||||
|
|
||||||
class Invite(Base):
|
class Invite(Base, CanonicalEmailMixin):
|
||||||
"""Access-request / whitelist row. Source of truth for who may sign in; the env
|
"""Access-request / whitelist row. Source of truth for who may sign in; the env
|
||||||
ALLOWED_EMAILS/ADMIN_EMAILS act only as a bootstrap fallback (see auth.is_allowed)."""
|
ALLOWED_EMAILS/ADMIN_EMAILS act only as a bootstrap fallback (see auth.is_allowed)."""
|
||||||
|
|
||||||
@@ -129,6 +150,9 @@ class Invite(Base):
|
|||||||
|
|
||||||
id: Mapped[int] = mapped_column(primary_key=True)
|
id: Mapped[int] = mapped_column(primary_key=True)
|
||||||
email: Mapped[str] = mapped_column(String(320), unique=True, index=True)
|
email: Mapped[str] = mapped_column(String(320), unique=True, index=True)
|
||||||
|
# See User.email_canonical. Not unique here either: prod has approved invites for both spellings
|
||||||
|
# of the same mailbox, dating back to 2026-06-16.
|
||||||
|
email_canonical: Mapped[str] = mapped_column(String(320), index=True, server_default="")
|
||||||
# pending | approved | denied
|
# pending | approved | denied
|
||||||
status: Mapped[str] = mapped_column(String(16), default="pending", server_default="pending")
|
status: Mapped[str] = mapped_column(String(16), default="pending", server_default="pending")
|
||||||
note: Mapped[str | None] = mapped_column(Text)
|
note: Mapped[str | None] = mapped_column(Text)
|
||||||
|
|||||||
+33
-10
@@ -27,20 +27,27 @@ from app.models import (
|
|||||||
Invite,
|
Invite,
|
||||||
User,
|
User,
|
||||||
)
|
)
|
||||||
from app.utils import iso, valid_email
|
from app.utils import canonical_email, iso, valid_email
|
||||||
|
|
||||||
router = APIRouter(prefix="/api/admin", tags=["admin"])
|
router = APIRouter(prefix="/api/admin", tags=["admin"])
|
||||||
|
|
||||||
|
|
||||||
def _accounts_for(db: Session, emails: Iterable[str]) -> set[str]:
|
def _accounts_for(db: Session, emails: Iterable[str]) -> set[str]:
|
||||||
"""Which of these addresses already have an account. Batched, so listing the invites costs one
|
"""Which of these addresses already have an account, as a set of CANONICAL forms. Batched, so
|
||||||
extra query. An invite whose address has an account must not be deletable: `is_allowed` reads
|
listing the invites costs one extra query. An invite whose address has an account must not be
|
||||||
this row, so removing it would silently revoke a live user's access."""
|
deletable: `is_allowed` reads this row, so removing it would silently revoke a live user's access.
|
||||||
wanted = {e.lower() for e in emails if e}
|
|
||||||
|
Canonical on both sides, because `is_allowed` matches that way: an invite for `x.y@gmail.com`
|
||||||
|
whose account registered as `xy@gmail.com` would otherwise look account-less, offer the admin a
|
||||||
|
delete button, and take away a live user's access — precisely the accident this guard exists to
|
||||||
|
prevent."""
|
||||||
|
wanted = {canonical_email(e) for e in emails if e}
|
||||||
if not wanted:
|
if not wanted:
|
||||||
return set()
|
return set()
|
||||||
rows = db.execute(select(User.email).where(User.email.in_(wanted))).scalars()
|
rows = db.execute(
|
||||||
return {e.lower() for e in rows if e}
|
select(User.email_canonical).where(User.email_canonical.in_(wanted))
|
||||||
|
).scalars()
|
||||||
|
return {e for e in rows if e}
|
||||||
|
|
||||||
|
|
||||||
def _env_allowed(email: str) -> bool:
|
def _env_allowed(email: str) -> bool:
|
||||||
@@ -82,7 +89,7 @@ def list_invites(
|
|||||||
order = {"pending": 0, "approved": 1, "denied": 2}
|
order = {"pending": 0, "approved": 1, "denied": 2}
|
||||||
rows.sort(key=lambda i: order.get(i.status, 9))
|
rows.sort(key=lambda i: order.get(i.status, 9))
|
||||||
accounts = _accounts_for(db, (i.email for i in rows))
|
accounts = _accounts_for(db, (i.email for i in rows))
|
||||||
return [_serialize(i, has_account=(i.email or "").lower() in accounts) for i in rows]
|
return [_serialize(i, has_account=canonical_email(i.email) in accounts) for i in rows]
|
||||||
|
|
||||||
|
|
||||||
def _activate_user_by_email(db: Session, email: str) -> None:
|
def _activate_user_by_email(db: Session, email: str) -> None:
|
||||||
@@ -97,7 +104,18 @@ def _activate_user_by_email(db: Session, email: str) -> None:
|
|||||||
DEFAULT experience, because approve-before-verify is the normal ordering. The revoke stays on
|
DEFAULT experience, because approve-before-verify is the normal ordering. The revoke stays on
|
||||||
the Google-adoption path (auth.revoke_preset_password_if_unverified), where the real owner does
|
the Google-adoption path (auth.revoke_preset_password_if_unverified), where the real owner does
|
||||||
attest ownership and the preset password must not survive."""
|
attest ownership and the preset password must not survive."""
|
||||||
user = db.execute(select(User).where(User.email == email.lower())).scalar_one_or_none()
|
# Canonical, with the literal spelling preferred when both exist: approving
|
||||||
|
# `npeter83.temu@gmail.com` must activate the account that registered as `npeter83temu@gmail.com`,
|
||||||
|
# since Gmail delivers both to one mailbox and the admin decided about the mailbox.
|
||||||
|
email = email.lower()
|
||||||
|
candidates = (
|
||||||
|
db.execute(
|
||||||
|
select(User).where(User.email_canonical == canonical_email(email)).order_by(User.id)
|
||||||
|
)
|
||||||
|
.scalars()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
user = next((u for u in candidates if u.email == email), None) or next(iter(candidates), None)
|
||||||
if user is not None and not user.is_active:
|
if user is not None and not user.is_active:
|
||||||
user.is_active = True
|
user.is_active = True
|
||||||
db.commit()
|
db.commit()
|
||||||
@@ -169,7 +187,12 @@ def add_invite(
|
|||||||
email = body.email.strip().lower()
|
email = body.email.strip().lower()
|
||||||
if not valid_email(email):
|
if not valid_email(email):
|
||||||
raise HTTPException(status_code=400, detail="Enter a valid email address.")
|
raise HTTPException(status_code=400, detail="Enter a valid email address.")
|
||||||
inv = db.execute(select(Invite).where(Invite.email == email)).scalar_one_or_none()
|
# Reuse a canonically-equal row instead of adding a second one for the same mailbox. Prod carries
|
||||||
|
# exactly that pair for one person from 2026-06-16 (hartmann.peter1986@ / hartmannpeter1986@),
|
||||||
|
# which is how an admin ends up approving the same human twice and still seeing them bounce.
|
||||||
|
canon = canonical_email(email)
|
||||||
|
rows = db.execute(select(Invite).where(Invite.email_canonical == canon)).scalars().all()
|
||||||
|
inv = next((r for r in rows if r.email == email), None) or next(iter(rows), None)
|
||||||
if inv is None:
|
if inv is None:
|
||||||
inv = Invite(email=email)
|
inv = Invite(email=email)
|
||||||
db.add(inv)
|
db.add(inv)
|
||||||
|
|||||||
@@ -37,7 +37,7 @@ from app.db import SessionLocal, get_db
|
|||||||
from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User
|
from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User
|
||||||
from app.ratelimit import RateLimiter
|
from app.ratelimit import RateLimiter
|
||||||
from app.security import encrypt, hash_password, hash_token, verify_password
|
from app.security import encrypt, hash_password, hash_token, verify_password
|
||||||
from app.utils import valid_email
|
from app.utils import canonical_email, valid_email
|
||||||
|
|
||||||
VERIFY_TTL = timedelta(hours=24)
|
VERIFY_TTL = timedelta(hours=24)
|
||||||
RESET_TTL = timedelta(hours=1)
|
RESET_TTL = timedelta(hours=1)
|
||||||
@@ -149,11 +149,21 @@ def _client_ip(request: Request) -> str:
|
|||||||
|
|
||||||
def upsert_pending_invite(db: Session, email: str) -> Invite | None:
|
def upsert_pending_invite(db: Session, email: str) -> Invite | None:
|
||||||
"""Idempotently record an access request. Returns the Invite if a *new* pending row
|
"""Idempotently record an access request. Returns the Invite if a *new* pending row
|
||||||
was created (so the caller can notify admins), else None for already-known emails."""
|
was created (so the caller can notify admins), else None for already-known emails.
|
||||||
|
|
||||||
|
"Already known" is decided on the CANONICAL form: `npeter83.temu@` and `npeter83temu@` are one
|
||||||
|
Gmail mailbox, and treating them as two is what put a second, silent access request in the admin
|
||||||
|
queue on 2026-09-03 for an already-approved user. An approved sibling wins — re-requesting must
|
||||||
|
not reopen a decision that has already been made for that mailbox. The row is still STORED under
|
||||||
|
the address as typed, because that is where mail has to go.
|
||||||
|
"""
|
||||||
email = (email or "").lower()
|
email = (email or "").lower()
|
||||||
if not valid_email(email):
|
if not valid_email(email):
|
||||||
return None
|
return None
|
||||||
inv = db.execute(select(Invite).where(Invite.email == email)).scalar_one_or_none()
|
canon = canonical_email(email)
|
||||||
|
rows = db.execute(select(Invite).where(Invite.email_canonical == canon)).scalars().all()
|
||||||
|
# Prefer a decided row over a pending one, so a denied duplicate can't mask an approval.
|
||||||
|
inv = next((r for r in rows if r.status == "approved"), None) or next(iter(rows), None)
|
||||||
if inv is not None:
|
if inv is not None:
|
||||||
# Let a previously-denied person ask again; don't disturb approved/pending rows.
|
# Let a previously-denied person ask again; don't disturb approved/pending rows.
|
||||||
if inv.status == "denied":
|
if inv.status == "denied":
|
||||||
@@ -292,11 +302,21 @@ async def callback(
|
|||||||
|
|
||||||
user = db.query(User).filter(User.google_sub == sub).one_or_none()
|
user = db.query(User).filter(User.google_sub == sub).one_or_none()
|
||||||
if user is None:
|
if user is None:
|
||||||
# No account carries this Google identity yet. If one already exists for this email —
|
# No account carries this Google identity yet. If one already exists for this MAILBOX —
|
||||||
# e.g. an email+password registration — adopt the Google identity onto it instead of
|
# e.g. an email+password registration — adopt the Google identity onto it instead of
|
||||||
# inserting a duplicate (which would collide on the unique email and 500). Google has
|
# inserting a duplicate. Google has verified the address, so this safely unifies password +
|
||||||
# verified the address, so this safely unifies password + Google sign-in on one account.
|
# Google sign-in on one account.
|
||||||
existing = db.query(User).filter(User.email == email).one_or_none()
|
#
|
||||||
|
# Matched canonically, and that is not cosmetic: on 2026-09-03 a registration as
|
||||||
|
# `npeter83.temu@gmail.com` met an OAuth claim of `npeter83temu@gmail.com`, the literal
|
||||||
|
# lookup found nothing, and prod ended up with TWO active user rows for one Gmail mailbox
|
||||||
|
# (ids 13 and 14). Exact spelling still wins when both exist, so an account that already
|
||||||
|
# matches literally is never passed over for a canonical sibling.
|
||||||
|
canon = canonical_email(email)
|
||||||
|
candidates = db.query(User).filter(User.email_canonical == canon).order_by(User.id).all()
|
||||||
|
existing = next((u for u in candidates if u.email == email), None) or next(
|
||||||
|
iter(candidates), None
|
||||||
|
)
|
||||||
if existing is not None:
|
if existing is not None:
|
||||||
if existing.google_sub is not None and existing.google_sub != sub:
|
if existing.google_sub is not None and existing.google_sub != sub:
|
||||||
# A different Google identity is already bound to this email — refuse rather than
|
# A different Google identity is already bound to this email — refuse rather than
|
||||||
@@ -615,7 +635,13 @@ def _register_account(email: str, password: str) -> None:
|
|||||||
notice) for a genuinely new email; no-op for an already-registered one. Runs after the response
|
notice) for a genuinely new email; no-op for an already-registered one. Runs after the response
|
||||||
with its own DB session, so the request's timing never reveals whether the email already exists."""
|
with its own DB session, so the request's timing never reveals whether the email already exists."""
|
||||||
with SessionLocal() as db:
|
with SessionLocal() as db:
|
||||||
if db.execute(select(User).where(User.email == email)).scalar_one_or_none() is not None:
|
# Canonical, so a second spelling of one Gmail mailbox cannot open a SECOND account. Prod
|
||||||
|
# already carries such a pair (user ids 13 and 14, both for npeter83temu@gmail.com); this is
|
||||||
|
# the end that stops new ones appearing, while the Google callback copes with the existing.
|
||||||
|
existing = db.execute(
|
||||||
|
select(User).where(User.email_canonical == canonical_email(email))
|
||||||
|
).first()
|
||||||
|
if existing is not None:
|
||||||
return # already registered — nothing to do (the uniform "ok" was already returned)
|
return # already registered — nothing to do (the uniform "ok" was already returned)
|
||||||
# Without working SMTP we can't deliver a verification link, so email ownership can't gate
|
# Without working SMTP we can't deliver a verification link, so email ownership can't gate
|
||||||
# sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so
|
# sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so
|
||||||
|
|||||||
@@ -22,6 +22,39 @@ def valid_email(value: str | None) -> bool:
|
|||||||
return bool(value and _EMAIL_RE.match(value))
|
return bool(value and _EMAIL_RE.match(value))
|
||||||
|
|
||||||
|
|
||||||
|
# Gmail (and its googlemail.com alias) delivers to one mailbox regardless of dots in the local part
|
||||||
|
# and of any `+tag`; nobody else is guaranteed to. So the canonical form below strips those for
|
||||||
|
# Gmail only, and merely lowercases everything else.
|
||||||
|
_GMAIL_DOMAINS = {"gmail.com", "googlemail.com"}
|
||||||
|
|
||||||
|
|
||||||
|
def canonical_email(value: str | None) -> str:
|
||||||
|
"""The form used to decide whether two addresses are the SAME MAILBOX. Never used for sending —
|
||||||
|
mail always goes to the address as the user typed it.
|
||||||
|
|
||||||
|
Why this exists (observed on prod, 2026-09-03): someone registered as `npeter83.temu@gmail.com`
|
||||||
|
and was approved, but Google's OAuth `email` claim came back as the account's stored primary,
|
||||||
|
`npeter83temu@gmail.com`. The invite gate only lowercased, found no approved row for that literal
|
||||||
|
string, and turned an approved user's sign-in into a silent SECOND access request — plus a second
|
||||||
|
User row. The `+tag` case is worse and fails every time: a Google account cannot contain `+`, so
|
||||||
|
OAuth can only ever return the base address, meaning anyone who registers with
|
||||||
|
`user+siftlode@gmail.com` (a common throwaway habit) is guaranteed to be denied after approval.
|
||||||
|
|
||||||
|
Scope is deliberately narrow. The dot rule does NOT hold for Google Workspace custom domains
|
||||||
|
unless the admin turned it on, and dots are significant on most providers, so anything outside
|
||||||
|
gmail.com/googlemail.com is only lowercased. Approving one Gmail spelling always did mean
|
||||||
|
approving that mailbox, so collapsing them is the honest reading of an invite, not a widening
|
||||||
|
of it.
|
||||||
|
"""
|
||||||
|
email = (value or "").strip().lower()
|
||||||
|
local, sep, domain = email.partition("@")
|
||||||
|
if not sep or domain not in _GMAIL_DOMAINS:
|
||||||
|
return email
|
||||||
|
local = local.partition("+")[0].replace(".", "")
|
||||||
|
# Normalise the alias domain too, so googlemail and gmail spellings meet.
|
||||||
|
return f"{local}@gmail.com" if local else email
|
||||||
|
|
||||||
|
|
||||||
def now_utc() -> datetime:
|
def now_utc() -> datetime:
|
||||||
"""Current time as a timezone-aware UTC datetime."""
|
"""Current time as a timezone-aware UTC datetime."""
|
||||||
return datetime.now(timezone.utc)
|
return datetime.now(timezone.utc)
|
||||||
|
|||||||
@@ -0,0 +1,158 @@
|
|||||||
|
"""One mailbox, many spellings — the canonical-email identity rule (E9.5 S3).
|
||||||
|
|
||||||
|
Gmail delivers `x.y@gmail.com`, `xy@gmail.com` and `xy+tag@gmail.com` to ONE inbox. The invite gate
|
||||||
|
used to compare literal strings, so on 2026-09-03 an approved user's Google sign-in became a silent
|
||||||
|
SECOND access request: they had registered as `npeter83.temu@gmail.com`, and OAuth returned the
|
||||||
|
account's stored primary `npeter83temu@gmail.com`. The `+tag` case fails every time — a Google account
|
||||||
|
cannot contain `+`, so OAuth can only ever answer with the base address.
|
||||||
|
|
||||||
|
The rule is narrow on purpose: dots are significant on most providers, and the Gmail dot rule does not
|
||||||
|
hold for Google Workspace custom domains unless their admin enabled it.
|
||||||
|
"""
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from sqlalchemy import select, text
|
||||||
|
|
||||||
|
from app.auth import is_allowed
|
||||||
|
from app.models import Invite, User
|
||||||
|
from app.utils import canonical_email
|
||||||
|
|
||||||
|
# (input, expected canonical) — the matrix the sprint promised, plus the shapes that must NOT fold.
|
||||||
|
CASES = [
|
||||||
|
# Gmail: dots and +tags are noise…
|
||||||
|
("npeter83.temu@gmail.com", "npeter83temu@gmail.com"),
|
||||||
|
("npeter83temu@gmail.com", "npeter83temu@gmail.com"),
|
||||||
|
("n.p.e.t.e.r@gmail.com", "npeter@gmail.com"),
|
||||||
|
("user+siftlode@gmail.com", "user@gmail.com"),
|
||||||
|
("u.ser+a+b@gmail.com", "user@gmail.com"),
|
||||||
|
("USer.Name@GMail.COM", "username@gmail.com"),
|
||||||
|
# …and googlemail is the same mailbox as gmail.
|
||||||
|
("user.name@googlemail.com", "username@gmail.com"),
|
||||||
|
# Everyone else: lowercase ONLY. Dots stay significant.
|
||||||
|
("first.last@example.com", "first.last@example.com"),
|
||||||
|
("First.Last@Example.COM", "first.last@example.com"),
|
||||||
|
("first+tag@example.com", "first+tag@example.com"),
|
||||||
|
# A Workspace domain is not Gmail, however Gmail-ish it looks.
|
||||||
|
("first.last@gmail.company.com", "first.last@gmail.company.com"),
|
||||||
|
# Degenerate input must not explode or invent an address.
|
||||||
|
("", ""),
|
||||||
|
(None, ""),
|
||||||
|
("not-an-email", "not-an-email"),
|
||||||
|
(".@gmail.com", ".@gmail.com"),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("raw,expected", CASES)
|
||||||
|
def test_canonical_email(raw, expected):
|
||||||
|
assert canonical_email(raw) == expected
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_migration_sql_agrees_with_the_helper(db):
|
||||||
|
"""The 0079 backfill reimplements canonical_email in SQL (a migration must not import app code
|
||||||
|
that keeps changing). That duplication is only safe while the two agree, so pin them together —
|
||||||
|
a drift here means rows backfilled into a form the application will never look up."""
|
||||||
|
import importlib.util
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
path = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0079_email_canonical.py"
|
||||||
|
spec = importlib.util.spec_from_file_location("m0079", path)
|
||||||
|
module = importlib.util.module_from_spec(spec)
|
||||||
|
spec.loader.exec_module(module)
|
||||||
|
|
||||||
|
sql = module._CANON_SQL.format(col=":addr")
|
||||||
|
for raw, expected in CASES:
|
||||||
|
if raw is None:
|
||||||
|
continue # the column is NOT NULL; the backfill skips NULL emails explicitly
|
||||||
|
got = db.execute(text(f"SELECT {sql}"), {"addr": raw}).scalar_one()
|
||||||
|
assert got == expected, f"SQL and helper disagree on {raw!r}: {got!r} != {expected!r}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_column_is_kept_in_sync_by_writing_the_email(db):
|
||||||
|
"""No write site has to remember: the model validator derives it. If this breaks, every lookup
|
||||||
|
below silently stops matching."""
|
||||||
|
user = User(email="Dotted.Name+tag@GoogleMail.com")
|
||||||
|
db.add(user)
|
||||||
|
db.commit()
|
||||||
|
assert user.email == "Dotted.Name+tag@GoogleMail.com" # stored AS TYPED — mail goes here
|
||||||
|
assert user.email_canonical == "dottedname@gmail.com" # …matched on this
|
||||||
|
|
||||||
|
user.email = "someone.else@example.com"
|
||||||
|
db.commit()
|
||||||
|
assert user.email_canonical == "someone.else@example.com"
|
||||||
|
|
||||||
|
|
||||||
|
def test_an_approval_covers_every_spelling_of_the_same_gmail_mailbox(db):
|
||||||
|
# THE 2026-09-03 lockout, in one assertion: approved under the dotted spelling, signing in with
|
||||||
|
# the primary Google returns.
|
||||||
|
db.add(Invite(email="npeter83.temu@gmail.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
assert is_allowed(db, "npeter83temu@gmail.com") is True
|
||||||
|
assert is_allowed(db, "npeter83.temu@gmail.com") is True
|
||||||
|
assert is_allowed(db, "npeter83.temu+throwaway@gmail.com") is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_plus_tagged_registration_is_not_locked_out_after_approval(db):
|
||||||
|
# The case that ALWAYS broke: a Google account cannot contain `+`, so OAuth can only return the
|
||||||
|
# base address, and a literal match could never find the invite.
|
||||||
|
db.add(Invite(email="someone+siftlode@gmail.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
assert is_allowed(db, "someone@gmail.com") is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_dots_stay_significant_outside_gmail(db):
|
||||||
|
db.add(Invite(email="first.last@example.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
assert is_allowed(db, "first.last@example.com") is True
|
||||||
|
assert is_allowed(db, "firstlast@example.com") is False # a DIFFERENT mailbox elsewhere
|
||||||
|
|
||||||
|
|
||||||
|
def test_an_approved_row_wins_over_a_denied_sibling(db):
|
||||||
|
# Prod carries approved pairs for one mailbox (oldest 2026-06-16). The API cannot flip an approved
|
||||||
|
# invite to denied, so approved-beside-denied means the admin approved that mailbox under another
|
||||||
|
# spelling — not that they revoked it. Revoking is deleting the row or suspending the account.
|
||||||
|
db.add(Invite(email="two.spellings@gmail.com", status="denied"))
|
||||||
|
db.add(Invite(email="twospellings@gmail.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
assert is_allowed(db, "twospellings@gmail.com") is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_denied_mailbox_stays_denied(db):
|
||||||
|
db.add(Invite(email="no.thanks@gmail.com", status="denied"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
assert is_allowed(db, "nothanks@gmail.com") is False
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_second_spelling_does_not_open_a_second_access_request(db):
|
||||||
|
"""The other half of the 2026-09-03 report: the admin queue got a duplicate request for a user
|
||||||
|
who was already approved."""
|
||||||
|
from app.routes.auth import upsert_pending_invite
|
||||||
|
|
||||||
|
db.add(Invite(email="already.approved@gmail.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
created = upsert_pending_invite(db, "alreadyapproved@gmail.com")
|
||||||
|
|
||||||
|
assert created is None # nothing new to notify an admin about
|
||||||
|
assert len(db.execute(select(Invite)).scalars().all()) == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_manually_whitelisting_another_spelling_reuses_the_row(db):
|
||||||
|
"""How prod ended up with hartmann.peter1986@ AND hartmannpeter1986@ both approved on the same
|
||||||
|
day: the admin typed the other spelling and got a second row instead of a no-op."""
|
||||||
|
from app.routes.admin import add_invite, AddInviteIn
|
||||||
|
|
||||||
|
admin = User(email="admin@example.com", role="admin", is_active=True, email_verified=True)
|
||||||
|
db.add(admin)
|
||||||
|
db.add(Invite(email="hartmann.peter1986@gmail.com", status="approved"))
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
add_invite(AddInviteIn(email="hartmannpeter1986@gmail.com"), admin=admin, db=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
|
||||||
Reference in New Issue
Block a user