Files
odysseus/tests/test_email_ownerless_account_owner_scope.py
T
Ashvin c8169ad7a9 fix(security): scope owner-less email accounts to a mailbox match in route guards (#5238)
The HTTP email route guard `_assert_owns_account` and the explicit-account_id
path in `_get_email_config` gated cross-tenant access with
`if row.owner and row.owner != owner` -- which skips the check entirely when the
account row is owner-less (owner NULL or ""). `email_accounts` is the one
owner-scoped table left out of the legacy-owner migration backfill
(core/database.py), so such rows persist on multi-user deploys: an account
configured while auth was disabled, or an imported legacy row. Any authenticated
user could then pass that account's id to read/send/update-credentials/delete
another tenant's mailbox and read its decrypted IMAP/SMTP creds.

Both sibling paths already enforce the intended contract -- the same-file
`_owner_or_matching_legacy_account` fallback and the MCP `_account_visible_to_owner`
gate (whose comment says it mirrors "the HTTP email route fallback") only expose
an owner-less account when its own mailbox (imap_user / from_address) is the
caller's. Factor that row-level predicate into `_account_visible_to_owner` and
use it in both guards, so owner-less accounts are visible only on a mailbox
match. Owned accounts, the legacy-claim path, and single-user mode (owner == "")
are unchanged.

Complements #5234 (which fixes the same class on the MCP tool layer); this is
the HTTP route layer it does not touch.
2026-07-05 12:50:32 +01:00

118 lines
4.9 KiB
Python

"""Cross-tenant access control for legacy owner-less email accounts.
`email_accounts` is the one owner-scoped table left out of the legacy-owner
migration backfill (core/database.py), so rows with owner NULL/"" persist on a
multi-user deploy — e.g. an account configured while auth was disabled, or an
imported legacy row. The HTTP route guards (`_assert_owns_account` and the
explicit-account_id path in `_get_email_config`) must scope such rows to a
mailbox match, exactly like the `_owner_or_matching_legacy_account` fallback and
the MCP `_account_visible_to_owner` gate. Otherwise any authenticated user can
read/send/update-credentials/delete another tenant's imported mailbox.
"""
from unittest import mock
import pytest
from fastapi import HTTPException
def _make_db():
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker
from core.database import Base
engine = create_engine("sqlite:///:memory:", connect_args={"check_same_thread": False})
Base.metadata.create_all(engine)
Factory = sessionmaker(bind=engine)
return Factory
def _make_account(Factory, account_id, owner, imap_user, from_address="", is_default=False):
from core.database import EmailAccount
db = Factory()
row = EmailAccount(
id=account_id,
owner=owner,
name="Test",
enabled=True,
is_default=is_default,
imap_host="imap.example.com",
imap_port=993,
imap_user=imap_user,
smtp_host="smtp.example.com",
smtp_port=587,
smtp_user=imap_user,
from_address=from_address or imap_user,
)
db.add(row)
db.commit()
db.close()
def test_assert_owns_account_rejects_ownerless_account_for_other_tenant():
"""The core regression: a legacy owner-less mailbox is NOT accessible to an
authenticated caller whose own mailbox does not match it."""
from routes.email_helpers import _assert_owns_account
Factory = _make_db()
# owner="" (created while auth was disabled); mailbox belongs to victim.
_make_account(Factory, "acct-legacy", owner="", imap_user="victim@corp.com")
with mock.patch("core.database.SessionLocal", Factory):
with pytest.raises(HTTPException) as exc:
_assert_owns_account("acct-legacy", "attacker")
assert exc.value.status_code == 404
def test_assert_owns_account_allows_owned_account():
from routes.email_helpers import _assert_owns_account
Factory = _make_db()
_make_account(Factory, "acct-bob", owner="bob", imap_user="bob@corp.com")
with mock.patch("core.database.SessionLocal", Factory):
_assert_owns_account("acct-bob", "bob") # no raise
def test_assert_owns_account_allows_ownerless_account_on_mailbox_match():
"""Legacy-claim path stays intact: the user whose mailbox matches an
owner-less account may still act on it (imap_user or from_address)."""
from routes.email_helpers import _assert_owns_account
Factory = _make_db()
_make_account(Factory, "acct-legacy", owner="", imap_user="alice@corp.com")
with mock.patch("core.database.SessionLocal", Factory):
_assert_owns_account("acct-legacy", "alice@corp.com") # no raise
def test_assert_owns_account_noop_for_single_user_mode():
"""owner == "" (unconfigured / single-user) accepts any account, unchanged."""
from routes.email_helpers import _assert_owns_account
Factory = _make_db()
_make_account(Factory, "acct-legacy", owner="", imap_user="whoever@corp.com")
with mock.patch("core.database.SessionLocal", Factory):
_assert_owns_account("acct-legacy", "") # no raise
def test_get_email_config_does_not_resolve_ownerless_account_for_other_tenant(monkeypatch):
"""`_get_email_config(account_id=..., owner=...)` must not serve an
owner-less account (and its decrypted creds) to a non-matching tenant."""
import routes.email_helpers as eh
Factory = _make_db()
_make_account(Factory, "acct-legacy", owner="", imap_user="victim@corp.com", is_default=True)
# Make the settings.json / env fallback empty and deterministic.
monkeypatch.setattr(eh, "_load_settings", lambda: {}, raising=False)
for var in ("IMAP_HOST", "SMTP_HOST", "IMAP_USER", "SMTP_USER"):
monkeypatch.delenv(var, raising=False)
with mock.patch("core.database.SessionLocal", Factory):
cfg = eh._get_email_config(account_id="acct-legacy", owner="attacker")
assert cfg.get("account_id") != "acct-legacy"
def test_get_email_config_resolves_ownerless_account_on_mailbox_match():
"""The mailbox owner still resolves their claimable legacy account by id."""
import routes.email_helpers as eh
Factory = _make_db()
_make_account(Factory, "acct-legacy", owner="", imap_user="alice@corp.com")
with mock.patch("core.database.SessionLocal", Factory):
cfg = eh._get_email_config(account_id="acct-legacy", owner="alice@corp.com")
assert cfg.get("account_id") == "acct-legacy"