Add per-key Odoo connection scoping, fix named keys never authenticating
API keys can now be locked to a single Odoo connection at creation time. A scoped key's tool calls silently default to that connection when none is specified, and are rejected outright if the caller explicitly requests a different one. Unscoped keys (including the legacy key) keep working across all connections as before. New instance_name column on api_keys, added via a runtime migration so it applies safely to already-deployed databases, not just fresh installs. Also fixes a significant pre-existing bug found while testing this: get_user_by_api_key (used for both REST and MCP auth) only ever checked the legacy single-key column — it never checked the api_keys table at all, meaning every named key created via POST /api/keys was completely unusable for actual authentication. You could create/list/revoke them, but never log in with one. Named keys now authenticate correctly everywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
0439811613
commit
b56435e258
@ -56,6 +56,7 @@ class ResetPasswordRequest(BaseModel):
|
|||||||
|
|
||||||
class CreateApiKeyRequest(BaseModel):
|
class CreateApiKeyRequest(BaseModel):
|
||||||
name: str
|
name: str
|
||||||
|
instance_name: Optional[str] = None
|
||||||
|
|
||||||
|
|
||||||
# ─── Auth dependency ──────────────────────────────────────────────────────────
|
# ─── Auth dependency ──────────────────────────────────────────────────────────
|
||||||
@ -195,7 +196,8 @@ def api_list_keys(authorization: Optional[str] = Header(None)):
|
|||||||
"keys": [
|
"keys": [
|
||||||
{"id": k.id, "name": k.name, "key_prefix": k.key_prefix,
|
{"id": k.id, "name": k.name, "key_prefix": k.key_prefix,
|
||||||
"created_at": k.created_at, "last_used_at": k.last_used_at,
|
"created_at": k.created_at, "last_used_at": k.last_used_at,
|
||||||
"revoked_at": k.revoked_at, "is_active": k.is_active}
|
"revoked_at": k.revoked_at, "is_active": k.is_active,
|
||||||
|
"instance_name": k.instance_name}
|
||||||
for k in keys
|
for k in keys
|
||||||
],
|
],
|
||||||
"total": len(keys),
|
"total": len(keys),
|
||||||
@ -204,12 +206,23 @@ def api_list_keys(authorization: Optional[str] = Header(None)):
|
|||||||
|
|
||||||
@router.post("/keys")
|
@router.post("/keys")
|
||||||
def api_create_key(body: CreateApiKeyRequest, authorization: Optional[str] = Header(None)):
|
def api_create_key(body: CreateApiKeyRequest, authorization: Optional[str] = Header(None)):
|
||||||
"""Create a new named API key. Shown once."""
|
"""Create a new named API key. Shown once. If instance_name is set, the
|
||||||
|
key is locked to that one Odoo connection — must be one the user
|
||||||
|
actually owns, checked here to avoid scoping a key to a typo'd name
|
||||||
|
that would make it permanently unusable."""
|
||||||
user = _require_auth(authorization)
|
user = _require_auth(authorization)
|
||||||
key_id, full_key = create_api_key(user.id, body.name)
|
if body.instance_name:
|
||||||
|
owned_names = {c.instance_name for c in list_odoo_credentials(user.id)}
|
||||||
|
if body.instance_name not in owned_names:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=400,
|
||||||
|
detail=f"No Odoo connection named '{body.instance_name}'. Add it first via POST /api/credentials.",
|
||||||
|
)
|
||||||
|
key_id, full_key = create_api_key(user.id, body.name, body.instance_name)
|
||||||
return {
|
return {
|
||||||
"id": key_id,
|
"id": key_id,
|
||||||
"name": body.name,
|
"name": body.name,
|
||||||
|
"instance_name": body.instance_name,
|
||||||
"api_key": full_key,
|
"api_key": full_key,
|
||||||
"warning": "Save this key — it will not be shown again.",
|
"warning": "Save this key — it will not be shown again.",
|
||||||
"mcp_connection": {
|
"mcp_connection": {
|
||||||
|
|||||||
@ -34,3 +34,4 @@ class ApiKeyInfo:
|
|||||||
last_used_at: Optional[str]
|
last_used_at: Optional[str]
|
||||||
revoked_at: Optional[str]
|
revoked_at: Optional[str]
|
||||||
is_active: bool
|
is_active: bool
|
||||||
|
instance_name: Optional[str]
|
||||||
|
|||||||
@ -74,15 +74,19 @@ def login(email: str, password: str) -> User:
|
|||||||
def get_user_by_api_key(api_key: str) -> Optional[User]:
|
def get_user_by_api_key(api_key: str) -> Optional[User]:
|
||||||
"""Look up a user by their API key. Returns None if not found/invalid.
|
"""Look up a user by their API key. Returns None if not found/invalid.
|
||||||
|
|
||||||
The legacy users.api_key_hash path is checked first for backward
|
Checks two places: the legacy users.api_key_hash column (kept for
|
||||||
compatibility, but if that same key has since been backfilled into
|
backward compatibility), and the api_keys table for named/scoped keys
|
||||||
api_keys (via list_api_keys) and explicitly revoked there, it must be
|
created via POST /api/keys — a named key is a real credential and must
|
||||||
rejected here too — otherwise "revoking" the legacy key in the UI would
|
authenticate here too, not just show up in the Tokens list.
|
||||||
silently do nothing since this function never looked at api_keys before.
|
|
||||||
|
If the legacy key matches but has since been backfilled into api_keys
|
||||||
|
(via list_api_keys) and explicitly revoked there, it's rejected —
|
||||||
|
otherwise "revoking" the legacy key in the UI would silently do nothing.
|
||||||
"""
|
"""
|
||||||
if not api_key or not api_key.startswith("mtom_"):
|
if not api_key or not api_key.startswith("mtom_"):
|
||||||
return None
|
return None
|
||||||
prefix = api_key[:12]
|
prefix = api_key[:12]
|
||||||
|
|
||||||
rows = db_fetchall("SELECT * FROM users WHERE api_key_prefix = ? AND is_active = 1", (prefix,))
|
rows = db_fetchall("SELECT * FROM users WHERE api_key_prefix = ? AND is_active = 1", (prefix,))
|
||||||
for row in rows:
|
for row in rows:
|
||||||
if verify_api_key(api_key, row["api_key_hash"]):
|
if verify_api_key(api_key, row["api_key_hash"]):
|
||||||
@ -93,6 +97,16 @@ def get_user_by_api_key(api_key: str) -> Optional[User]:
|
|||||||
if revoked:
|
if revoked:
|
||||||
return None
|
return None
|
||||||
return _row_to_user(row)
|
return _row_to_user(row)
|
||||||
|
|
||||||
|
key_rows = db_fetchall(
|
||||||
|
"SELECT * FROM api_keys WHERE key_prefix = ? AND is_active = 1 AND revoked_at IS NULL",
|
||||||
|
(prefix,),
|
||||||
|
)
|
||||||
|
for key_row in key_rows:
|
||||||
|
if verify_api_key(api_key, key_row["key_hash"]):
|
||||||
|
user_row = db_fetchone("SELECT * FROM users WHERE id = ? AND is_active = 1", (key_row["user_id"],))
|
||||||
|
if user_row:
|
||||||
|
return _row_to_user(user_row)
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
@ -148,12 +162,14 @@ def delete_user(user_id: int) -> bool:
|
|||||||
|
|
||||||
# ─── Multiple API Keys ──────────────────────────────────────────────────────
|
# ─── Multiple API Keys ──────────────────────────────────────────────────────
|
||||||
|
|
||||||
def create_api_key(user_id: int, name: str) -> tuple[int, str]:
|
def create_api_key(user_id: int, name: str, instance_name: Optional[str] = None) -> tuple[int, str]:
|
||||||
"""Create a new named API key. Returns (key_id, full_key) — full_key shown once."""
|
"""Create a new named API key. If instance_name is set, this key is
|
||||||
|
scoped to that one Odoo connection only — see server.py's call_tool
|
||||||
|
wrapper for enforcement. Returns (key_id, full_key) — full_key shown once."""
|
||||||
full_key, key_hash, key_prefix = generate_api_key()
|
full_key, key_hash, key_prefix = generate_api_key()
|
||||||
cur = db_execute(
|
cur = db_execute(
|
||||||
"INSERT INTO api_keys (user_id, name, key_hash, key_prefix) VALUES (?, ?, ?, ?)",
|
"INSERT INTO api_keys (user_id, name, key_hash, key_prefix, instance_name) VALUES (?, ?, ?, ?, ?)",
|
||||||
(user_id, name, key_hash, key_prefix),
|
(user_id, name, key_hash, key_prefix, instance_name),
|
||||||
)
|
)
|
||||||
db_commit()
|
db_commit()
|
||||||
return cur.lastrowid, full_key
|
return cur.lastrowid, full_key
|
||||||
@ -191,28 +207,29 @@ def revoke_api_key(user_id: int, key_id: int) -> bool:
|
|||||||
return cur.rowcount > 0
|
return cur.rowcount > 0
|
||||||
|
|
||||||
|
|
||||||
def get_api_key_id_for_mcp(api_key: str) -> Optional[int]:
|
def get_api_key_scope_for_mcp(api_key: str) -> tuple[Optional[int], Optional[str]]:
|
||||||
"""Resolve which api_keys row (if any) this raw key belongs to, for
|
"""Resolve which api_keys row (if any) this raw key belongs to, plus its
|
||||||
analytics attribution at MCP-session-start. Returns the row id whether
|
instance scope (None = unscoped, can access any of the user's Odoo
|
||||||
it's a named key or a backfilled 'Default (legacy)' row (see
|
connections). Used at MCP-session-start for analytics attribution
|
||||||
list_api_keys). Returns None only if the key has never been backfilled
|
(api_key_id) and per-call instance enforcement (instance_name) — see
|
||||||
into api_keys at all — harmless, just means analytics rows for that
|
server.py's call_tool wrapper. Returns (None, None) only if the key has
|
||||||
session carry api_key_id=NULL until the user's first /api/keys view
|
never been backfilled into api_keys at all (harmless — analytics rows
|
||||||
triggers the backfill. The caller already validated the key via
|
for that session carry api_key_id=NULL until the user's first /api/keys
|
||||||
|
view triggers the backfill). The caller already validated the key via
|
||||||
get_user_by_api_key before calling this."""
|
get_user_by_api_key before calling this."""
|
||||||
if not api_key or not api_key.startswith("mtom_"):
|
if not api_key or not api_key.startswith("mtom_"):
|
||||||
return None
|
return None, None
|
||||||
prefix = api_key[:12]
|
prefix = api_key[:12]
|
||||||
rows = db_fetchall(
|
rows = db_fetchall(
|
||||||
"SELECT id, key_hash FROM api_keys WHERE key_prefix = ? AND is_active = 1 AND revoked_at IS NULL",
|
"SELECT id, key_hash, instance_name FROM api_keys WHERE key_prefix = ? AND is_active = 1 AND revoked_at IS NULL",
|
||||||
(prefix,),
|
(prefix,),
|
||||||
)
|
)
|
||||||
for row in rows:
|
for row in rows:
|
||||||
if verify_api_key(api_key, row["key_hash"]):
|
if verify_api_key(api_key, row["key_hash"]):
|
||||||
db_execute("UPDATE api_keys SET last_used_at = datetime('now') WHERE id = ?", (row["id"],))
|
db_execute("UPDATE api_keys SET last_used_at = datetime('now') WHERE id = ?", (row["id"],))
|
||||||
db_commit()
|
db_commit()
|
||||||
return row["id"]
|
return row["id"], row["instance_name"]
|
||||||
return None
|
return None, None
|
||||||
|
|
||||||
|
|
||||||
def _row_to_api_key(row) -> ApiKeyInfo:
|
def _row_to_api_key(row) -> ApiKeyInfo:
|
||||||
@ -224,6 +241,7 @@ def _row_to_api_key(row) -> ApiKeyInfo:
|
|||||||
last_used_at=row["last_used_at"],
|
last_used_at=row["last_used_at"],
|
||||||
revoked_at=row["revoked_at"],
|
revoked_at=row["revoked_at"],
|
||||||
is_active=bool(row["is_active"]),
|
is_active=bool(row["is_active"]),
|
||||||
|
instance_name=row["instance_name"],
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -12,6 +12,7 @@ _user_email: ContextVar[str] = ContextVar("user_email")
|
|||||||
_conn_manager: ContextVar["ConnectionManager"] = ContextVar("conn_manager")
|
_conn_manager: ContextVar["ConnectionManager"] = ContextVar("conn_manager")
|
||||||
_confirm_manager: ContextVar["ConfirmationManager"] = ContextVar("confirm_manager")
|
_confirm_manager: ContextVar["ConfirmationManager"] = ContextVar("confirm_manager")
|
||||||
_api_key_id: ContextVar[Optional[int]] = ContextVar("api_key_id", default=None)
|
_api_key_id: ContextVar[Optional[int]] = ContextVar("api_key_id", default=None)
|
||||||
|
_key_instance_scope: ContextVar[Optional[str]] = ContextVar("key_instance_scope", default=None)
|
||||||
|
|
||||||
|
|
||||||
def get_user_id() -> int:
|
def get_user_id() -> int:
|
||||||
@ -34,14 +35,25 @@ def get_api_key_id() -> Optional[int]:
|
|||||||
return _api_key_id.get()
|
return _api_key_id.get()
|
||||||
|
|
||||||
|
|
||||||
def set_user_context(user_id: int, user_email: str, conn_mgr, confirm_mgr, api_key_id: Optional[int] = None):
|
def get_key_instance_scope() -> Optional[str]:
|
||||||
|
"""None means the current session's API key can access any of the
|
||||||
|
user's Odoo connections. If set, tool calls are locked to that one
|
||||||
|
connection — see server.py's call_tool wrapper for enforcement."""
|
||||||
|
return _key_instance_scope.get()
|
||||||
|
|
||||||
|
|
||||||
|
def set_user_context(
|
||||||
|
user_id: int, user_email: str, conn_mgr, confirm_mgr,
|
||||||
|
api_key_id: Optional[int] = None, key_instance_scope: Optional[str] = None,
|
||||||
|
):
|
||||||
"""Set all context vars for the current MCP session. Returns tokens for reset."""
|
"""Set all context vars for the current MCP session. Returns tokens for reset."""
|
||||||
t1 = _user_id.set(user_id)
|
t1 = _user_id.set(user_id)
|
||||||
t2 = _user_email.set(user_email)
|
t2 = _user_email.set(user_email)
|
||||||
t3 = _conn_manager.set(conn_mgr)
|
t3 = _conn_manager.set(conn_mgr)
|
||||||
t4 = _confirm_manager.set(confirm_mgr)
|
t4 = _confirm_manager.set(confirm_mgr)
|
||||||
t5 = _api_key_id.set(api_key_id)
|
t5 = _api_key_id.set(api_key_id)
|
||||||
return (t1, t2, t3, t4, t5)
|
t6 = _key_instance_scope.set(key_instance_scope)
|
||||||
|
return (t1, t2, t3, t4, t5, t6)
|
||||||
|
|
||||||
|
|
||||||
def reset_user_context(tokens):
|
def reset_user_context(tokens):
|
||||||
@ -50,3 +62,4 @@ def reset_user_context(tokens):
|
|||||||
_conn_manager.reset(tokens[2])
|
_conn_manager.reset(tokens[2])
|
||||||
_confirm_manager.reset(tokens[3])
|
_confirm_manager.reset(tokens[3])
|
||||||
_api_key_id.reset(tokens[4])
|
_api_key_id.reset(tokens[4])
|
||||||
|
_key_instance_scope.reset(tokens[5])
|
||||||
|
|||||||
@ -50,12 +50,16 @@ def init_db():
|
|||||||
|
|
||||||
-- Multiple named, revocable API keys. users.api_key_hash/api_key_prefix
|
-- Multiple named, revocable API keys. users.api_key_hash/api_key_prefix
|
||||||
-- stay untouched — that's the legacy key and keeps working as-is.
|
-- stay untouched — that's the legacy key and keeps working as-is.
|
||||||
|
-- instance_name (nullable): if set, this key can only be used
|
||||||
|
-- against that one Odoo connection (see migration below for
|
||||||
|
-- adding this column to already-deployed databases).
|
||||||
CREATE TABLE IF NOT EXISTS api_keys (
|
CREATE TABLE IF NOT EXISTS api_keys (
|
||||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||||
user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
|
user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
|
||||||
name TEXT NOT NULL,
|
name TEXT NOT NULL,
|
||||||
key_hash TEXT NOT NULL,
|
key_hash TEXT NOT NULL,
|
||||||
key_prefix TEXT NOT NULL,
|
key_prefix TEXT NOT NULL,
|
||||||
|
instance_name TEXT,
|
||||||
created_at TEXT NOT NULL DEFAULT (datetime('now')),
|
created_at TEXT NOT NULL DEFAULT (datetime('now')),
|
||||||
last_used_at TEXT,
|
last_used_at TEXT,
|
||||||
revoked_at TEXT,
|
revoked_at TEXT,
|
||||||
@ -92,6 +96,17 @@ def init_db():
|
|||||||
CREATE INDEX IF NOT EXISTS idx_tce_user_tool ON tool_call_events(user_id, tool_name);
|
CREATE INDEX IF NOT EXISTS idx_tce_user_tool ON tool_call_events(user_id, tool_name);
|
||||||
""")
|
""")
|
||||||
db.commit()
|
db.commit()
|
||||||
|
_migrate(db)
|
||||||
|
|
||||||
|
|
||||||
|
def _migrate(db: sqlite3.Connection) -> None:
|
||||||
|
"""CREATE TABLE IF NOT EXISTS above doesn't alter already-existing tables
|
||||||
|
(e.g. a production api_keys table created before a new column existed),
|
||||||
|
so new columns on existing tables are added here, idempotently."""
|
||||||
|
cols = {row["name"] for row in db.execute("PRAGMA table_info(api_keys)")}
|
||||||
|
if "instance_name" not in cols:
|
||||||
|
db.execute("ALTER TABLE api_keys ADD COLUMN instance_name TEXT")
|
||||||
|
db.commit()
|
||||||
|
|
||||||
|
|
||||||
def db_execute(sql: str, params: tuple = ()) -> sqlite3.Cursor:
|
def db_execute(sql: str, params: tuple = ()) -> sqlite3.Cursor:
|
||||||
|
|||||||
@ -19,9 +19,11 @@ from mcp.types import Tool, TextContent
|
|||||||
|
|
||||||
from .config import settings
|
from .config import settings
|
||||||
from .database import init_db
|
from .database import init_db
|
||||||
from .auth.service import get_user_by_api_key, get_api_key_id_for_mcp
|
from .auth.service import get_user_by_api_key, get_api_key_scope_for_mcp
|
||||||
from .registry import get_conn_manager, get_confirm_manager
|
from .registry import get_conn_manager, get_confirm_manager
|
||||||
from .context import set_user_context, reset_user_context, get_user_id, get_api_key_id
|
from .context import (
|
||||||
|
set_user_context, reset_user_context, get_user_id, get_api_key_id, get_key_instance_scope,
|
||||||
|
)
|
||||||
from .api.routes import router as api_router
|
from .api.routes import router as api_router
|
||||||
from .admin.routes import router as admin_router
|
from .admin.routes import router as admin_router
|
||||||
|
|
||||||
@ -592,11 +594,29 @@ async def call_tool(name: str, arguments: dict) -> list[TextContent]:
|
|||||||
if not entry:
|
if not entry:
|
||||||
return [TextContent(type="text", text=json.dumps({"error": f"Unknown tool: '{name}'"}))]
|
return [TextContent(type="text", text=json.dumps({"error": f"Unknown tool: '{name}'"}))]
|
||||||
|
|
||||||
_, _, fn, _ = entry
|
_, _, fn, schema = entry
|
||||||
instance = arguments.get("instance", "default")
|
instance = arguments.get("instance", "default")
|
||||||
started = time.monotonic()
|
started = time.monotonic()
|
||||||
started_at_iso = datetime.now(timezone.utc).isoformat()
|
started_at_iso = datetime.now(timezone.utc).isoformat()
|
||||||
|
|
||||||
|
# If this session's API key is scoped to one Odoo instance, enforce it
|
||||||
|
# for every tool that takes an `instance` argument: silently default to
|
||||||
|
# the scoped instance when the caller didn't specify one (or specified
|
||||||
|
# the schema's literal "default"), or reject outright if they explicitly
|
||||||
|
# asked for a different instance. Tools with no `instance` param (e.g.
|
||||||
|
# list_instances) are untouched — nothing to scope there.
|
||||||
|
key_scope = get_key_instance_scope()
|
||||||
|
if key_scope and "instance" in (schema.get("properties") or {}):
|
||||||
|
if "instance" not in arguments or arguments["instance"] == "default":
|
||||||
|
arguments = {**arguments, "instance": key_scope}
|
||||||
|
instance = key_scope
|
||||||
|
elif arguments["instance"] != key_scope:
|
||||||
|
error_text = (
|
||||||
|
f"This API key is scoped to Odoo instance '{key_scope}' and cannot access '{arguments['instance']}'."
|
||||||
|
)
|
||||||
|
_record_tool_call(name, arguments["instance"], started, started_at_iso, False, error_text)
|
||||||
|
return [TextContent(type="text", text=json.dumps({"error": error_text}))]
|
||||||
|
|
||||||
# Auto-connect instance (skip for tools that explicitly handle connection errors)
|
# Auto-connect instance (skip for tools that explicitly handle connection errors)
|
||||||
from .context import get_conn_manager as _gcm
|
from .context import get_conn_manager as _gcm
|
||||||
try:
|
try:
|
||||||
@ -759,7 +779,7 @@ class _MCPEndpoint:
|
|||||||
resp = _JSON({"error": "Invalid API key."}, status_code=401)
|
resp = _JSON({"error": "Invalid API key."}, status_code=401)
|
||||||
await resp(scope, receive, send)
|
await resp(scope, receive, send)
|
||||||
return
|
return
|
||||||
api_key_id = get_api_key_id_for_mcp(api_key)
|
api_key_id, key_instance_scope = get_api_key_scope_for_mcp(api_key)
|
||||||
|
|
||||||
# Session ID must be visible ASCII chars; hex satisfies that
|
# Session ID must be visible ASCII chars; hex satisfies that
|
||||||
new_session_id = _secrets.token_hex(32)
|
new_session_id = _secrets.token_hex(32)
|
||||||
@ -774,9 +794,9 @@ class _MCPEndpoint:
|
|||||||
|
|
||||||
async def _run_session(
|
async def _run_session(
|
||||||
_t=transport, _u=user, _cm=conn_mgr, _fm=confirm_mgr,
|
_t=transport, _u=user, _cm=conn_mgr, _fm=confirm_mgr,
|
||||||
_ev=ready, _sid=new_session_id, _kid=api_key_id,
|
_ev=ready, _sid=new_session_id, _kid=api_key_id, _scope=key_instance_scope,
|
||||||
):
|
):
|
||||||
tokens = set_user_context(_u.id, _u.email, _cm, _fm, _kid)
|
tokens = set_user_context(_u.id, _u.email, _cm, _fm, _kid, _scope)
|
||||||
try:
|
try:
|
||||||
async with _t.connect() as (read_stream, write_stream):
|
async with _t.connect() as (read_stream, write_stream):
|
||||||
_ev.set()
|
_ev.set()
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user