From 916351fe64932c17a70635372dcbef8bf8709e1e Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Tue, 29 Sep 2026 13:45:54 +0300 Subject: [PATCH 1/9] fix(auth): a sign-in email is proven, unique, and written in one place (abilityai/trinity-enterprise#720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every sign-in path resolves the account by email alone, so whoever holds an address on a users row holds that identity. These are the residual doors after #711: 1. Mailbox proof to bind. POST /api/users/me/email/code sends a 6-digit code to the NEW address (3 per 10 min). PUT /api/users/me/email requires {email, code}. Codes carry a purpose (email_login_codes.purpose): `email_bind:` completes only that account's bind; a bind code never signs in and a sign-in code never binds. Both routes are interactive-only. The one no-proof bind is the #82 transition on an install that cannot deliver mail (provider `console`): an interactive admin, audited as email_bind_unverified. Anyone else there gets 409 email_verification_unavailable. The onboarding step and the Settings card gain the code step (auth store `bindOwnEmail`). 2. Unique. idx_users_email_unique ON users(lower(email)) WHERE email IS NOT NULL, on both tracks (SQLite `ent720_email_identity`, Alembic `0081_ent720_email_identity`). Existing duplicates are resolved first: blank becomes NULL, and per address the earliest-created account keeps it while the rest become NULL, logged by username only. 3. One writer. create_user, update_user, the password upsert and email sign-in creation all write through `_insert_user` / `_update_user_row`. These refuse a held address (EmailInUseError → 409 email_in_use) and map a lost race on the index to the same refusal. A writer-census test enumerates them. get_user_by_email is case-insensitive. 4. A reclaimed username is not a 500. Email sign-in whose `username = email` is taken creates a suffixed username. Auth0 sign-in resolves the HOLDER of the address first and never hands it back to an account that re-bound away. 5. Redeemers honour suspension. Telegram and WhatsApp redemption, the MCP inline redeemer, email_has_agent_access and the per-message channel gate (open_access included) refuse a suspended account, through db.is_email_account_suspended. Second factor on the channel redeemers is out of scope: they cannot present a challenge. It is recorded in FR-4 as a follow-up. Tests: - test_ent720_email_binding (28 tests); - suspended cases in the Telegram, WhatsApp and router suites; - emailBindProof.spec.js (mounted). Mutations: bind-without-proof, no unique index, no username suffix, and suspension ignored each turn tests red. The related backend suites (67 files) pass on seeds 12345 and 99999 (1571 passed). Frontend: 181 files and 3728 tests pass, and the build is OK. Co-Authored-By: Claude Opus 5.5 --- .../requirements/lifecycle-observability.md | 35 ++ src/backend/adapters/message_router.py | 8 + src/backend/adapters/telegram_adapter.py | 6 + src/backend/adapters/whatsapp_adapter.py | 6 + src/backend/database.py | 11 +- src/backend/db/agent_settings/sharing.py | 6 + src/backend/db/email_auth.py | 42 +- src/backend/db/migrations.py | 57 +++ src/backend/db/schema.py | 9 +- src/backend/db/tables.py | 1 + src/backend/db/users.py | 178 +++++-- .../versions/0081_ent720_email_identity.py | 71 +++ src/backend/models.py | 10 + src/backend/routers/users.py | 127 ++++- src/backend/services/mcp_auth_service.py | 5 + .../components/onboarding/steps/StepEmail.vue | 69 ++- src/frontend/src/stores/auth.js | 26 +- src/frontend/src/utils/apiError.js | 4 + src/frontend/src/views/Settings.vue | 40 +- .../tests/unit/emailBindProof.spec.js | 111 +++++ tests/unit/test_ent720_email_binding.py | 434 ++++++++++++++++++ tests/unit/test_message_router_access_gate.py | 14 + tests/unit/test_telegram_login_gate.py | 18 + tests/unit/test_whatsapp_adapter.py | 19 + 24 files changed, 1204 insertions(+), 103 deletions(-) create mode 100644 src/backend/migrations/versions/0081_ent720_email_identity.py create mode 100644 src/frontend/tests/unit/emailBindProof.spec.js create mode 100644 tests/unit/test_ent720_email_binding.py diff --git a/docs/memory/requirements/lifecycle-observability.md b/docs/memory/requirements/lifecycle-observability.md index 6bffc438d..abf7da63e 100644 --- a/docs/memory/requirements/lifecycle-observability.md +++ b/docs/memory/requirements/lifecycle-observability.md @@ -341,6 +341,41 @@ password) is **Phase 2**, gated on a configured email provider and the existing (own-account scoped; 409 if the email belongs to another account), surfaced as an **Admin sign-in email** card in Settings → General. No verification email is sent; existing `admin`+password login keeps working until/unless an email is set. +- **FR-4 — A sign-in email is proven, unique, and written in one place + (trinity-enterprise#720, residual of #711).** Every sign-in path resolves the + account by email alone, so whoever holds an email on a `users` row holds that + identity — what is shared with the address, its Workspace threads, and the + real person's next sign-in. + - **Mailbox proof to bind.** `PUT /api/users/me/email` requires `{email, code}`, + where the code was sent to the NEW address by + `POST /api/users/me/email/code` (same rate limits as sign-in codes). Bind + codes live in `email_login_codes` with `purpose='email_bind'` and are tied to + the requesting account; a bind code never signs anyone in and a sign-in code + never binds. Missing/wrong/expired code → 400 `invalid_code`. + - **The one no-proof bind** is the #82 transition (FR-3) on an install that + cannot deliver mail (provider `console`): an **interactive admin** session may + bind without a code, audited as `email_bind_unverified`. Anyone else on such an + install gets 409 `email_verification_unavailable`. First-run setup (FR-2) is + unchanged — it binds before any provider can exist. + - **Unique.** `users.email` is unique on its lower-cased value (NULL allowed): + `idx_users_email_unique ON users(lower(email)) WHERE email IS NOT NULL`, on both + migration tracks. Pre-existing duplicates are resolved first: per address the + EARLIEST-created account keeps it; the others are set to NULL and logged by + username only. + - **One writer.** Every write of `users.email` (setup, the bind route, + `create_user`, `update_user`, the password upsert, Auth0, email sign-in + creation) goes through `db/users.py`'s checked write, which refuses an address + another account holds (`EmailInUseError` → 409 `email_in_use`) and maps a lost + race on the unique index to the same refusal. A test enumerates the writers. + - **A reclaimed username is not a 500.** Email sign-in creates the account with + `username = email`; when that username is already taken (an account that has + since re-bound away from the address), the new account gets a unique suffixed + username instead of an unhandled IntegrityError. + - **Redeemers honour suspension.** The Telegram, WhatsApp and MCP-inline code + redeemers and `email_has_agent_access` refuse an address whose account is + suspended (`users.suspended_at`), the same account-state rule `/verify` and + `get_current_user` apply. (Second-factor on those channels is out of scope: + they cannot present a challenge; tracked as a follow-up.) --- diff --git a/src/backend/adapters/message_router.py b/src/backend/adapters/message_router.py index d87cbe30f..331d2934f 100644 --- a/src/backend/adapters/message_router.py +++ b/src/backend/adapters/message_router.py @@ -1254,6 +1254,14 @@ async def _enforce_access_policy( if verified_email: verified_email = verified_email.strip().lower() or None + # ent#720: a verified email whose account is suspended reaches + # nothing — not even an open_access agent. An email verified BEFORE + # the suspension is still recorded on the channel link, so the check + # has to live here, per message, not only at code redemption. + if verified_email and db.is_email_account_suspended(verified_email): + logger.info(f"[ROUTER:{channel}] Access denied: account suspended (agent={agent_name})") + return False, None + require_email = policy.get("require_email", False) open_access = policy.get("open_access", False) diff --git a/src/backend/adapters/telegram_adapter.py b/src/backend/adapters/telegram_adapter.py index c7ab1a16e..8f38200c5 100644 --- a/src/backend/adapters/telegram_adapter.py +++ b/src/backend/adapters/telegram_adapter.py @@ -927,6 +927,12 @@ async def _handle_login_command( result = db.verify_login_code(pending_email, arg) if not result: return "❌ Invalid or expired code. Try again or request a new one." + # ent#720: a suspended account is refused here as on the web — the + # verified email is NOT recorded, so the open-access shortcut below + # and the per-message gate never see it. + if db.is_email_account_suspended(pending_email): + _clear_pending_login(binding["id"], message.sender_id) + return "❌ This account is suspended. Contact the agent's owner." db.set_telegram_verified_email(binding["id"], message.sender_id, pending_email) _clear_pending_login(binding["id"], message.sender_id) diff --git a/src/backend/adapters/whatsapp_adapter.py b/src/backend/adapters/whatsapp_adapter.py index 484f5904d..46a55ccab 100644 --- a/src/backend/adapters/whatsapp_adapter.py +++ b/src/backend/adapters/whatsapp_adapter.py @@ -672,6 +672,12 @@ async def _handle_login_command( result = db.verify_login_code(pending_email, arg) if not result: return "❌ Invalid or expired code. Try again or request a new one." + # ent#720: a suspended account is refused here as on the web — the + # verified email is NOT recorded, so the open-access shortcut below + # and the per-message gate never see it. + if db.is_email_account_suspended(pending_email): + _clear_pending_login(binding["id"], message.sender_id) + return "❌ This account is suspended. Contact the agent's owner." db.set_whatsapp_verified_email(binding["id"], message.sender_id, pending_email) _clear_pending_login(binding["id"], message.sender_id) diff --git a/src/backend/database.py b/src/backend/database.py index 66040b4fc..e27a24075 100644 --- a/src/backend/database.py +++ b/src/backend/database.py @@ -2638,11 +2638,14 @@ def remove_from_whitelist(self, email: str): def list_whitelist(self, limit: int = 100): return self._email_auth_ops.list_whitelist(limit) - def create_login_code(self, email: str, expiry_minutes: int = 10): - return self._email_auth_ops.create_login_code(email, expiry_minutes) + def create_login_code(self, email: str, expiry_minutes: int = 10, purpose=None): + return self._email_auth_ops.create_login_code(email, expiry_minutes, purpose=purpose) - def verify_login_code(self, email: str, code: str): - return self._email_auth_ops.verify_login_code(email, code) + def is_email_account_suspended(self, email: str) -> bool: + return self._user_ops.is_email_account_suspended(email) + + def verify_login_code(self, email: str, code: str, purpose=None): + return self._email_auth_ops.verify_login_code(email, code, purpose=purpose) def count_recent_code_requests(self, email: str, minutes: int = 10): return self._email_auth_ops.count_recent_code_requests(email, minutes) diff --git a/src/backend/db/agent_settings/sharing.py b/src/backend/db/agent_settings/sharing.py index 31ea9c927..972c68d38 100644 --- a/src/backend/db/agent_settings/sharing.py +++ b/src/backend/db/agent_settings/sharing.py @@ -244,6 +244,12 @@ def email_has_agent_access(self, agent_name: str, email: str) -> bool: if not normalized: return False user = self._user_ops.get_user_by_email(normalized) + # ent#720: a suspended account reaches nothing by channel either. The + # web path refuses it in `get_current_user`; the channel gates (Telegram, + # WhatsApp, the MCP inline redeemer) come here instead, and used to skip + # the check — a share to a suspended person's address kept working. + if user and user.get("suspended_at"): + return False if user: if user.get("role") == "admin": return True diff --git a/src/backend/db/email_auth.py b/src/backend/db/email_auth.py index 32ad849db..e633f7799 100644 --- a/src/backend/db/email_auth.py +++ b/src/backend/db/email_auth.py @@ -22,6 +22,7 @@ from .engine import get_engine from .tables import email_whitelist, email_login_codes, users from utils.helpers import utc_now_iso +from .users import EmailInUseError # Valid role values for new users. Kept local to avoid a circular import with # dependencies.py (which imports from database, which imports from this module). @@ -152,10 +153,15 @@ def list_whitelist(self, limit: int = 100) -> List[dict]: # Login Code Operations # ========================================================================= - def create_login_code(self, email: str, expiry_minutes: int = 10) -> dict: + def create_login_code(self, email: str, expiry_minutes: int = 10, + purpose: Optional[str] = None) -> dict: """ Generate a 6-digit login code for an email. + purpose (ent#720): NULL is a sign-in code. An email-bind code carries + `email_bind:` so it can only complete THAT account's bind, and + neither kind of code is accepted where the other is expected. + Returns: dict with code_id, code, expires_at """ @@ -172,6 +178,7 @@ def create_login_code(self, email: str, expiry_minutes: int = 10) -> dict: created_at=created_at.isoformat(), expires_at=expires_at.isoformat(), verified=0, + purpose=purpose, ) with get_engine().begin() as conn: conn.execute(stmt) @@ -184,9 +191,11 @@ def create_login_code(self, email: str, expiry_minutes: int = 10) -> dict: "expires_in_seconds": expiry_minutes * 60 } - def verify_login_code(self, email: str, code: str) -> Optional[dict]: + def verify_login_code(self, email: str, code: str, + purpose: Optional[str] = None) -> Optional[dict]: """ - Verify a login code for an email. + Verify a login code for an email. `purpose` must match exactly — the + default (None) accepts only sign-in codes (ent#720). Returns: dict with code verification result, or None if invalid @@ -205,6 +214,8 @@ def verify_login_code(self, email: str, code: str) -> Optional[dict]: func.lower(email_login_codes.c.email) == email.lower(), email_login_codes.c.code == code, email_login_codes.c.verified == 0, + (email_login_codes.c.purpose.is_(None) if purpose is None + else email_login_codes.c.purpose == purpose), ) ) .order_by(email_login_codes.c.created_at.desc()) @@ -283,20 +294,11 @@ def get_or_create_email_user(self, email: str) -> dict: if role not in _VALID_DEFAULT_ROLES: role = "user" - # Create new user - now = utc_now_iso() - # Username = email (lowercase) - username = email.lower() - - stmt = insert(users).values( - username=username, - email=email.lower(), - role=role, - created_at=now, - updated_at=now, - ) - with get_engine().begin() as conn: - conn.execute(stmt) - - # Return the created user - return self._user_ops.get_user_by_email(email) + # Create new user — through the one checked writer (ent#720), which + # also keeps a reclaimed `username = email` from becoming a 500. + try: + return self._user_ops.insert_email_user(email, role) + except EmailInUseError: + # Lost a race with a concurrent first sign-in for the same address: + # that account is this person's account. + return self._user_ops.get_user_by_email(email) diff --git a/src/backend/db/migrations.py b/src/backend/db/migrations.py index 59ca286bd..d9c7d6431 100644 --- a/src/backend/db/migrations.py +++ b/src/backend/db/migrations.py @@ -4765,6 +4765,62 @@ def _migrate_workspace_suggestion_feedback_table(cursor, conn): conn.commit() +def _migrate_ent720_email_identity(cursor, conn): + """trinity-enterprise#720 — a sign-in email is unique, and a code has a purpose. + + 1. `email_login_codes.purpose` (NULL = sign-in; `email_bind:` for + the mailbox proof a bind now requires). + 2. Resolve pre-existing duplicate `users.email` values, then add + `idx_users_email_unique ON users(lower(email)) WHERE email IS NOT NULL`. + Blank addresses become NULL first (they would collide under the index). + Per lower-cased address the EARLIEST-created account keeps it; the others + are set to NULL and named by USERNAME only in the log — the address itself + never is. Duplicates only exist because nothing stopped them; the oldest + row is the original owner. + + Idempotent; a fresh install whose tables do not exist yet on the first pass + gets both from `db/schema.py`. PostgreSQL half: Alembic + `0081_ent720_email_identity` (same decision function, `resolve_duplicate_emails`). + """ + cursor.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='email_login_codes'") + if cursor.fetchone(): + cursor.execute("PRAGMA table_info(email_login_codes)") + if "purpose" not in {r[1] for r in cursor.fetchall()}: + cursor.execute("ALTER TABLE email_login_codes ADD COLUMN purpose TEXT") + + cursor.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='users'") + if not cursor.fetchone(): + return + cursor.execute("UPDATE users SET email = NULL WHERE email IS NOT NULL AND TRIM(email) = ''") + cursor.execute("SELECT id, username, email, created_at FROM users WHERE email IS NOT NULL") + losers = resolve_duplicate_emails(cursor.fetchall()) + for user_id, username in losers: + cursor.execute("UPDATE users SET email = NULL WHERE id = ?", (user_id,)) + print(f"[ent#720] duplicate sign-in email: cleared on account '{username}' " + "(an earlier account holds it)") + cursor.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_unique " + "ON users(lower(email)) WHERE email IS NOT NULL" + ) + + +def resolve_duplicate_emails(rows): + """[(id, username, email, created_at)] → [(id, username)] that must LOSE + their email: per lower-cased address, all but the earliest-created account + (ties broken by the lower id). Shared by both migration tracks (ent#720).""" + groups = {} + for user_id, username, email, created_at in rows: + groups.setdefault((email or "").strip().lower(), []).append( + (created_at or "", user_id, username)) + losers = [] + for key, members in groups.items(): + if not key or len(members) < 2: + continue + members.sort() + losers += [(uid, uname) for _, uid, uname in members[1:]] + return losers + + def _migrate_agent_skill_sets(cursor, conn): """trinity-enterprise#530 — skill sets. @@ -5011,4 +5067,5 @@ def _migrate_auto_sync_enabled_backfill(cursor, conn): ("workspace_suggestion_feedback_table", _migrate_workspace_suggestion_feedback_table), ("telegram_group_context", _migrate_telegram_group_context), ("agent_skill_sets", _migrate_agent_skill_sets), + ("ent720_email_identity", _migrate_ent720_email_identity), ] diff --git a/src/backend/db/schema.py b/src/backend/db/schema.py index 8bac2e685..77d257160 100644 --- a/src/backend/db/schema.py +++ b/src/backend/db/schema.py @@ -186,7 +186,8 @@ created_at TEXT NOT NULL, expires_at TEXT NOT NULL, verified INTEGER DEFAULT 0, - used_at TEXT + used_at TEXT, + purpose TEXT ) """, @@ -1984,6 +1985,12 @@ "CREATE INDEX IF NOT EXISTS idx_users_username ON users(username)", "CREATE INDEX IF NOT EXISTS idx_users_auth0_sub ON users(auth0_sub)", "CREATE INDEX IF NOT EXISTS idx_users_email ON users(email)", + # trinity-enterprise#720 — a sign-in email belongs to ONE account. Every + # sign-in path resolves the account by email alone, so a duplicate is an + # identity coin toss. Lower-cased (addresses are compared that way), NULL + # allowed (password-only accounts, the pre-#82 placeholder). + "CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_unique " + "ON users(lower(email)) WHERE email IS NOT NULL", "CREATE INDEX IF NOT EXISTS idx_agent_ownership_owner ON agent_ownership(owner_id)", "CREATE INDEX IF NOT EXISTS idx_agent_ownership_name ON agent_ownership(agent_name)", # Issue #834: partial index for the retention sweep — narrow scan to diff --git a/src/backend/db/tables.py b/src/backend/db/tables.py index 951acd756..37847c327 100644 --- a/src/backend/db/tables.py +++ b/src/backend/db/tables.py @@ -194,6 +194,7 @@ def process_bind_param(self, value, dialect): Column("expires_at", Text), Column("verified", Integer), Column("used_at", Text), + Column("purpose", Text), # ent#720: NULL = sign-in; 'email_bind:' ) agent_schedules = Table( diff --git a/src/backend/db/users.py b/src/backend/db/users.py index c71ecf866..3f3cf171e 100644 --- a/src/backend/db/users.py +++ b/src/backend/db/users.py @@ -12,15 +12,49 @@ are unaffected. """ +import logging +import secrets from typing import Optional, Dict, List, Any -from sqlalchemy import insert, select, update +from sqlalchemy import func, insert, select, update +from sqlalchemy.exc import IntegrityError from .engine import get_engine from .tables import users from db_models import UserCreate from utils.helpers import utc_now_iso +logger = logging.getLogger(__name__) + +# trinity-enterprise#720 — `users.email` is unique on its lower-cased value. +# The index is created by both migration tracks and by `db/schema.py`. +EMAIL_UNIQUE_INDEX = "idx_users_email_unique" + + +class EmailInUseError(Exception): + """Another account already holds this sign-in email (ent#720). + + Every sign-in path resolves the account by email ALONE, so two rows with + one address make "who is this person" a coin toss — and the second writer + inherits whatever is shared with the address. Raised by the one checked + write below, never by a caller's own pre-check.""" + + def __init__(self, email: str): + super().__init__("That email is already associated with another account") + self.email = email + + +def normalize_email(email: Optional[str]) -> Optional[str]: + """The stored form of a sign-in email: trimmed, lower-cased, or None.""" + email = (email or "").strip().lower() + return email or None + + +def _is_email_conflict(exc: IntegrityError) -> bool: + """A lost race on the unique email index (not some other constraint).""" + text = str(getattr(exc, "orig", exc)).lower() + return EMAIL_UNIQUE_INDEX in text or "users.email" in text or "lower(email)" in text + class UserOperations: """User database operations.""" @@ -80,27 +114,87 @@ def get_user_by_id(self, user_id: int) -> Optional[Dict]: return self._get_user_by_field("id", user_id) def get_user_by_email(self, email: str) -> Optional[Dict]: - """Get user by email address.""" - return self._get_user_by_field("email", email) + """Get user by email address — case-insensitively, the way it is unique + (ent#720).""" + email = normalize_email(email) + if not email: + return None + stmt = select(*self._USER_COLUMNS).where(func.lower(users.c.email) == email) + with get_engine().connect() as conn: + row = conn.execute(stmt).mappings().first() + return self._row_to_user_dict(row) if row else None + + # ---------------------------------------------------------------- ent#720 + # THE writer of `users.email`. Every path that sets the column — insert or + # update — goes through `_insert_user` / `_update_user_row`, which normalise + # the address, refuse one another account holds, and turn a lost race on the + # unique index into the same refusal. `test_ent720_email_binding.py` + # enumerates the writers so a new one cannot bypass this. + + @staticmethod + def _assert_email_free(conn, email: Optional[str], username: str) -> None: + if not email: + return + taken = conn.execute( + select(users.c.username).where( + func.lower(users.c.email) == email, users.c.username != username) + ).first() + if taken: + raise EmailInUseError(email) + + def _insert_user(self, conn, values: Dict): + """Insert one users row through the email check. Returns the result.""" + values = dict(values) + if "email" in values: + values["email"] = normalize_email(values["email"]) + self._assert_email_free(conn, values["email"], values["username"]) + try: + with conn.begin_nested(): + return conn.execute(insert(users).values(**values)) + except IntegrityError as exc: + if _is_email_conflict(exc): + raise EmailInUseError(values.get("email") or "") from exc + raise + + def _update_user_row(self, conn, username: str, values: Dict): + """Update one users row through the email check. Returns the result.""" + values = dict(values) + if "email" in values: + values["email"] = normalize_email(values["email"]) + self._assert_email_free(conn, values["email"], username) + try: + with conn.begin_nested(): + return conn.execute(update(users).where(users.c.username == username).values(**values)) + except IntegrityError as exc: + if _is_email_conflict(exc): + raise EmailInUseError(values.get("email") or "") from exc + raise + + def is_email_account_suspended(self, email: str) -> bool: + """True when the account holding this sign-in email is suspended + (ent#720) — the account-state rule the channel redeemers share with + `get_current_user`. No account → False (nothing to suspend).""" + user = self.get_user_by_email(email) + return bool(user and user.get("suspended_at")) def create_user(self, user_data: UserCreate) -> Dict: """Create a new user.""" now = utc_now_iso() email = user_data.email or user_data.username # Use username as email if not provided - stmt = insert(users).values( - username=user_data.username, - password_hash=user_data.password, - role=user_data.role, - auth0_sub=user_data.auth0_sub, - name=user_data.name, - picture=user_data.picture, - email=email, - created_at=now, - updated_at=now, - ) + email = normalize_email(email) with get_engine().begin() as conn: - result = conn.execute(stmt) + result = self._insert_user(conn, dict( + username=user_data.username, + password_hash=user_data.password, + role=user_data.role, + auth0_sub=user_data.auth0_sub, + name=user_data.name, + picture=user_data.picture, + email=email, + created_at=now, + updated_at=now, + )) user_id = result.inserted_primary_key[0] return { @@ -128,9 +222,8 @@ def update_user(self, username: str, updates: Dict) -> Optional[Dict]: return self.get_user_by_username(username) values["updated_at"] = utc_now_iso() - stmt = update(users).where(users.c.username == username).values(**values) with get_engine().begin() as conn: - conn.execute(stmt) + self._update_user_row(conn, username, values) return self.get_user_by_username(username) @@ -159,18 +252,35 @@ def update_user_password(self, username: str, hashed_password: str) -> bool: return True # User doesn't exist - create it (for admin user during first-time setup) - result = conn.execute( - insert(users).values( - username=username, - password_hash=hashed_password, - role="admin", - email=username, - created_at=now, - updated_at=now, - ) - ) + result = self._insert_user(conn, dict( + username=username, + password_hash=hashed_password, + role="admin", + email=username, + created_at=now, + updated_at=now, + )) return result.rowcount > 0 + def insert_email_user(self, email: str, role: str) -> Dict: + """Create the account an email sign-in resolves to (ent#720). + + `username = email` as before — unless that username is already taken by + an account that has since re-bound away from this address, in which + case the new account gets a unique suffixed username. The insert used + to hit `username UNIQUE` and surface as an unhandled 500.""" + email = normalize_email(email) + now = utc_now_iso() + username = email + with get_engine().begin() as conn: + if conn.execute(select(users.c.id).where(users.c.username == username)).first(): + username = f"{email}~{secrets.token_hex(3)}" + logger.info("[ent#720] email sign-in username taken; created a suffixed username") + self._insert_user(conn, dict( + username=username, email=email, role=role, created_at=now, updated_at=now, + )) + return self.get_user_by_email(email) + def update_last_login(self, username: str): """Update user's last login timestamp.""" now = utc_now_iso() @@ -198,9 +308,19 @@ def get_or_create_auth0_user(self, auth0_sub: str, email: str, name: str = None, user = self.get_user_by_username(user["username"]) return user - # Try to find by email (username) - user = self.get_user_by_username(email) + # The HOLDER of the address first (ent#720): an account whose username + # is not its address (a re-bound one, a suffixed one) still owns this + # sign-in identity, and a second row for it is refused by the unique + # index. The legacy `username == email` match is only a fallback for a + # row with NO email — an account that re-bound AWAY from this address + # must not be handed it back through its old username. + user = self.get_user_by_email(email) + if not user: + legacy = self.get_user_by_username(email) + if legacy and not legacy.get("email"): + user = legacy if user: + email = user["username"] # Link auth0_sub to existing user stmt = ( update(users) diff --git a/src/backend/migrations/versions/0081_ent720_email_identity.py b/src/backend/migrations/versions/0081_ent720_email_identity.py new file mode 100644 index 000000000..bfcbee7a8 --- /dev/null +++ b/src/backend/migrations/versions/0081_ent720_email_identity.py @@ -0,0 +1,71 @@ +"""ent720_email_identity — sign-in email unique; codes carry a purpose. + +PostgreSQL half of the dual-track pair (trinity-enterprise#720); the SQLite half +is ``db/migrations.py::ent720_email_identity``. Both resolve duplicates with the +same rule; ``_resolve_duplicate_emails`` below is a frozen copy of +``db.migrations.resolve_duplicate_emails`` (a revision never imports app code), +pinned equal by ``tests/unit/test_ent720_email_binding.py``. + +1. ``email_login_codes.purpose`` — NULL is a sign-in code; ``email_bind:`` + is the mailbox proof a bind now requires. +2. Duplicate ``users.email`` values are resolved (blank → NULL; per lower-cased + address the earliest-created account keeps it, the rest → NULL, logged by + username only), then ``idx_users_email_unique`` is created. + +Revision ID: 0081_ent720_email_identity +Revises: 0080_agent_skill_sets +""" +from alembic import op +import sqlalchemy as sa + +revision = "0081_ent720_email_identity" +down_revision = "0080_agent_skill_sets" +branch_labels = None +depends_on = None + + +def _resolve_duplicate_emails(rows): + """[(id, username, email, created_at)] → [(id, username)] that must LOSE + their email: per lower-cased address, all but the earliest-created account + (ties broken by the lower id). Shared by both migration tracks (ent#720).""" + groups = {} + for user_id, username, email, created_at in rows: + groups.setdefault((email or "").strip().lower(), []).append( + (created_at or "", user_id, username)) + losers = [] + for key, members in groups.items(): + if not key or len(members) < 2: + continue + members.sort() + losers += [(uid, uname) for _, uid, uname in members[1:]] + return losers + + +def upgrade() -> None: + bind = op.get_bind() + insp = sa.inspect(bind) + if insp.has_table("email_login_codes") and "purpose" not in { + c["name"] for c in insp.get_columns("email_login_codes")}: + op.add_column("email_login_codes", sa.Column("purpose", sa.Text(), nullable=True)) + + if not insp.has_table("users"): + return + bind.execute(sa.text("UPDATE users SET email = NULL WHERE email IS NOT NULL AND TRIM(email) = ''")) + rows = bind.execute(sa.text( + "SELECT id, username, email, created_at FROM users WHERE email IS NOT NULL")).fetchall() + for user_id, username in _resolve_duplicate_emails([tuple(r) for r in rows]): + bind.execute(sa.text("UPDATE users SET email = NULL WHERE id = :id"), {"id": user_id}) + print(f"[ent#720] duplicate sign-in email: cleared on account '{username}' " + "(an earlier account holds it)") + op.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_unique " + "ON users(lower(email)) WHERE email IS NOT NULL" + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS idx_users_email_unique") + # The cleared duplicates are not restored — the honest inverse would be + # handing an identity back to an account that never proved it. + with op.batch_alter_table("email_login_codes") as batch: + batch.drop_column("purpose") diff --git a/src/backend/models.py b/src/backend/models.py index 10b3a90ce..0bdbedabc 100644 --- a/src/backend/models.py +++ b/src/backend/models.py @@ -4247,6 +4247,16 @@ class UserRoleUpdate(BaseModel): class UpdateMyEmailRequest(BaseModel): email: str + # trinity-enterprise#720: the 6-digit code sent to `email` by + # `POST /api/users/me/email/code` — proof the caller holds the mailbox. + # Optional only for the audited admin transition on an install that cannot + # deliver mail; everywhere else a missing code is a 400. + code: Optional[str] = None + + +class RequestEmailBindCodeRequest(BaseModel): + """`POST /api/users/me/email/code` — send a bind code to a NEW address (ent#720).""" + email: str class UserPreferenceWrite(BaseModel): diff --git a/src/backend/routers/users.py b/src/backend/routers/users.py index 5761baa97..523870da5 100644 --- a/src/backend/routers/users.py +++ b/src/backend/routers/users.py @@ -15,13 +15,16 @@ User, UserRoleUpdate, UpdateMyEmailRequest, + RequestEmailBindCodeRequest, GitHubPATRequest, UserPreferenceWrite, UserPreferenceRecord, UserPreferencesResponse, ) from database import db -from dependencies import require_admin, get_current_user, reject_non_interactive_principal +from db.users import EmailInUseError +from services.platform_audit_service import AuditEventType, platform_audit_service +from dependencies import require_admin, get_current_user, reject_non_interactive_principal, require_interactive from services import user_preferences_service from services.user_preferences_service import PreferenceConflict, PreferenceError @@ -30,39 +33,119 @@ VALID_ROLES = {"admin", "creator", "operator", "user"} # Permissive email-shape check (mirrors routers/setup.py): one @, a dot in the -# domain, no spaces. Identity binding only — no verification mail is sent. +# domain, no spaces. _EMAIL_RE = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s.]+$") +# trinity-enterprise#720 — binding a sign-in email requires proving the mailbox. +# Every sign-in path resolves the account by email ALONE, so whoever holds the +# address on a `users` row holds that identity: what is shared with it, its +# Workspace threads, and the real person's next sign-in. The 409 alone only saw +# `users` rows, so any signed-in human could claim an address nobody had signed +# in with yet — a sharee who never signed up, a Workspace-only client. +_BIND_CODE_MINUTES = 10 +_BIND_CODE_MAX_PER_WINDOW = 3 # the sign-in code limit (routers/auth.py) + + +def _bind_purpose(user_id) -> str: + """A bind code completes THIS account's bind only, and is never a sign-in code.""" + return f"email_bind:{user_id}" + + +def _email_can_be_delivered() -> bool: + """False when the provider is `console`: the code only reaches the server log.""" + from services.settings_service import settings_service + return settings_service.get_email_provider() != "console" + + +def _refuse(status_code: int, code: str, message: str): + raise HTTPException(status_code=status_code, detail={"code": code, "message": message}) + + +def _valid_new_email(raw: str, current_user: User) -> str: + email = (raw or "").strip().lower() + if not _EMAIL_RE.match(email): + _refuse(400, "invalid_email", "Invalid email address") + existing = db.get_user_by_email(email) + if existing and existing.get("username") != current_user.username: + _refuse(409, "email_in_use", "That email is already associated with another account") + return email + + +@router.post("/me/email/code") +async def request_email_bind_code( + body: RequestEmailBindCodeRequest, + request: Request, + current_user: User = Depends(require_interactive), +): + """Send a 6-digit code to the address the caller wants to bind (ent#720). + + Interactive sessions only: an identity change is a person's act. The code is + tied to the caller's account (`purpose`), so it completes only their bind and + never signs anyone in. + """ + email = _valid_new_email(body.email, current_user) + if not _email_can_be_delivered() and current_user.role != "admin": + _refuse(409, "email_verification_unavailable", + "Email verification isn't available on this instance yet. Ask an admin to configure email.") + if db.count_recent_code_requests(email, minutes=_BIND_CODE_MINUTES) >= _BIND_CODE_MAX_PER_WINDOW: + _refuse(429, "too_many_codes", "Too many codes requested for that address. Try again in a few minutes.") + + code = db.create_login_code(email, expiry_minutes=_BIND_CODE_MINUTES, + purpose=_bind_purpose(current_user.id)) + from services.email_service import EmailService + sent = await EmailService().send_verification_code( + email, code["code"], context_label="Trinity sign-in email confirmation") + if not sent: + _refuse(502, "email_send_failed", "We couldn't send the code. Try again in a moment.") + await platform_audit_service.log( + event_type=AuditEventType.AUTHENTICATION, event_action="email_bind_code_sent", + source="api", actor_user=current_user, target_type="user", target_id=current_user.username, + endpoint=str(request.url.path), request_id=getattr(request.state, "request_id", None), + ) + return {"sent": True, "expires_in_seconds": code["expires_in_seconds"]} + @router.put("/me/email") async def update_my_email( body: UpdateMyEmailRequest, - current_user: User = Depends(get_current_user), + request: Request, + current_user: User = Depends(require_interactive), ): - """Bind a sign-in email to the current account (#82 Phase 1 transition). + """Bind a sign-in email to the current account (#82 FR-3, ent#720 FR-4). - The migration path for an existing admin created before #82 — whose stored - email is still the placeholder 'admin' — to register a real email and then - sign in with email + password, exactly like a fresh install captures at - first run. No verification mail is sent; binding the identity is independent - of whether an email provider is configured. + Requires the code `POST /me/email/code` sent to the new address. The one + exception is the #82 transition on an install that cannot deliver mail + (provider `console`): an interactive ADMIN may bind without a code, and the + bind is audited as unverified. """ - email = (body.email or "").strip().lower() - if not _EMAIL_RE.match(email): - raise HTTPException(status_code=400, detail="Invalid email address") + email = _valid_new_email(body.email, current_user) + code = (body.code or "").strip() + verified = False + if code: + verified = bool(db.verify_login_code(email, code, purpose=_bind_purpose(current_user.id))) + if not verified: + _refuse(400, "invalid_code", "That code is wrong or has expired. Request a new one.") + elif _email_can_be_delivered(): + _refuse(400, "code_required", "Enter the code we sent to that address.") + elif current_user.role != "admin": + _refuse(409, "email_verification_unavailable", + "Email verification isn't available on this instance yet. Ask an admin to configure email.") - # Don't let one account claim another account's sign-in identity. - existing = db.get_user_by_email(email) - if existing and existing.get("username") != current_user.username: - raise HTTPException( - status_code=409, - detail="That email is already associated with another account", - ) - - updated = db.update_user(current_user.username, {"email": email}) + try: + updated = db.update_user(current_user.username, {"email": email}) + except EmailInUseError: + _refuse(409, "email_in_use", "That email is already associated with another account") if not updated: raise HTTPException(status_code=404, detail="User not found") - return {"success": True, "email": email} + + await platform_audit_service.log( + event_type=AuditEventType.AUTHENTICATION, + event_action="email_bound" if verified else "email_bind_unverified", + source="api", actor_user=current_user, target_type="user", target_id=current_user.username, + endpoint=str(request.url.path), request_id=getattr(request.state, "request_id", None), + details={"verified": verified}, + ) + return {"success": True, "email": email, "verified": verified} # --------------------------------------------------------------------------- diff --git a/src/backend/services/mcp_auth_service.py b/src/backend/services/mcp_auth_service.py index bdb2ee002..85b32b10a 100644 --- a/src/backend/services/mcp_auth_service.py +++ b/src/backend/services/mcp_auth_service.py @@ -227,6 +227,11 @@ def verify_login_code(email: str, code: str) -> Optional[dict]: if not user: logger.error("[#848] verified code but failed to resolve user for %s", email) return None + # ent#720: the same account-state rule as `/verify` + `get_current_user` — + # a suspended account is refused here, not handed an MCP key. + if user.get("suspended_at"): + logger.info("[ent#720] inline redeem refused: account suspended") + return None try: db.update_last_login(user["username"]) diff --git a/src/frontend/src/components/onboarding/steps/StepEmail.vue b/src/frontend/src/components/onboarding/steps/StepEmail.vue index 27230307a..46160208b 100644 --- a/src/frontend/src/components/onboarding/steps/StepEmail.vue +++ b/src/frontend/src/components/onboarding/steps/StepEmail.vue @@ -26,21 +26,45 @@ placeholder="you@company.com" autocomplete="email" :error="fieldError" - :disabled="saving" - help="No verification email is sent." + :disabled="saving || codeSent" + :help="codeSent ? '' : 'We\'ll email a 6-digit code to confirm the address is yours.'" data-testid="first-run-email-input" /> - - Save email - + + +
+ + {{ codeSent ? 'Confirm email' : 'Save email' }} + + + Use a different email + +
@@ -52,6 +76,7 @@ import { useAuthStore } from '../../../stores/auth' import BaseButton from '../../base/BaseButton.vue' import BaseInput from '../../base/BaseInput.vue' import InlineError from '../../InlineError.vue' +import { apiErrorMessage } from '../../../utils/apiError' import FirstRunStepHeader from '../FirstRunStepHeader.vue' const props = defineProps({ ctx: { type: Object, default: () => ({}) } }) @@ -69,10 +94,18 @@ const lead = computed(() => ) const email = ref('') +const code = ref('') +const codeSent = ref(false) const saving = ref(false) const fieldError = ref('') const saveError = ref('') +function reset() { + codeSent.value = false + code.value = '' + saveError.value = '' +} + async function save() { const value = email.value.trim() if (!EMAIL_RE.test(value)) { @@ -83,12 +116,16 @@ async function save() { saveError.value = '' saving.value = true try { - await auth.setOwnEmail(value) + const out = await auth.bindOwnEmail(value, codeSent.value ? code.value.trim() : null) + if (out.needsCode) { + codeSent.value = true + return + } email.value = '' + reset() emit('complete') } catch (e) { - const detail = e?.response?.data?.detail - saveError.value = typeof detail === 'string' ? detail : 'Could not save the email. Try again.' + saveError.value = apiErrorMessage(e, 'Could not save the email. Try again.') } finally { saving.value = false } diff --git a/src/frontend/src/stores/auth.js b/src/frontend/src/stores/auth.js index 8c037030b..f679d6b7b 100644 --- a/src/frontend/src/stores/auth.js +++ b/src/frontend/src/stores/auth.js @@ -291,9 +291,31 @@ export const useAuthStore = defineStore('auth', { // Bind a sign-in email to the current account (#82 / #2381), then re-read // the profile so `userEmail` — and every predicate reading it — updates in // place. Throws on failure; the caller owns the InlineError (ent#581). - async setOwnEmail(email) { - await axios.put('/api/users/me/email', { email }, { headers: this.authHeader }) + // + // trinity-enterprise#720: a bind needs proof of the mailbox. Without a + // `code` this tries the bind once — which succeeds only for the audited + // admin transition on an install that cannot deliver mail — and when the + // server answers `code_required` it has the code sent and returns + // `{ needsCode: true }`; the caller then asks for it and calls again WITH + // the code. `{ bound: true }` once the email is on the account. + async bindOwnEmail(email, code = null) { + try { + await axios.put('/api/users/me/email', code ? { email, code } : { email }, { headers: this.authHeader }) + } catch (e) { + const detail = e?.response?.data?.detail + if (!code && e?.response?.status === 400 && detail?.code === 'code_required') { + await axios.post('/api/users/me/email/code', { email }, { headers: this.authHeader }) + return { needsCode: true } + } + throw e + } await this.fetchUserProfile() + return { bound: true } + }, + + // Re-send the bind code for `email` (ent#720). + async resendEmailBindCode(email) { + await axios.post('/api/users/me/email/code', { email }, { headers: this.authHeader }) }, // Login with username/password (for admin login) diff --git a/src/frontend/src/utils/apiError.js b/src/frontend/src/utils/apiError.js index a9c6d736c..874fdac60 100644 --- a/src/frontend/src/utils/apiError.js +++ b/src/frontend/src/utils/apiError.js @@ -41,6 +41,10 @@ export function apiErrorMessage(err, fallback = 'Something went wrong') { } if (typeof detail === 'string' && detail.trim()) return detail + // Named refusals: `{code, message, ...}` (e.g. ent#720's bind refusals). + if (detail && typeof detail === 'object' && typeof detail.message === 'string' && detail.message.trim()) { + return detail.message + } // Some endpoints return {message: …} rather than {detail: …} const message = err?.response?.data?.message if (typeof message === 'string' && message.trim()) return message diff --git a/src/frontend/src/views/Settings.vue b/src/frontend/src/views/Settings.vue index a1119886f..ec4eaa135 100644 --- a/src/frontend/src/views/Settings.vue +++ b/src/frontend/src/views/Settings.vue @@ -229,7 +229,7 @@

Admin sign-in email

- Sign in with this email and your password instead of the admin username. No verification email is sent. + Sign in with this email and your password instead of the admin username. We email a 6-digit code to confirm the address is yours.

@@ -242,17 +242,33 @@ :placeholder="adminEmailCurrent || 'you@company.com'" :disabled="savingAdminEmail" :class="[SETTINGS_TEXT_INPUT_CLASS, 'flex-1']" + data-testid="admin-email-input" + /> + +
@@ -265,8 +281,8 @@ -