Skip to content

Commit 22cd831

Browse files
committed
fix(speaker): let an enrolment name its own tenant again
The previous commit made the tenant a pure lookup on speakers.user_id, which is right for a speaker that exists and wrong for /enroll/upload and /enroll/batch, whose whole job is to create one. Enrolment started returning 404 "Speaker not found" for every new speaker. The record still wins whenever there is one — an imported or re-owned speaker keeps an id whose prefix disagrees with its column. Only when no such speaker exists is the id's `user_{user_id}_...` prefix read, because that is currently how a caller states the tenant on create. That remains the weak contract the audit objects to. Retiring it means an explicit user_id on the three enrol endpoints and on every caller: Chronicle's speaker client, the speaker WebUI's Enrollment and Annotation pages, and enroll_speaker.py / laptop_client.py. Left as its own change rather than folded in here untested.
1 parent 77b4037 commit 22cd831

2 files changed

Lines changed: 57 additions & 13 deletions

File tree

extras/speaker-recognition/src/simple_speaker_recognition/api/core/utils.py

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -106,30 +106,48 @@ def secure_temp_file(suffix: str = ".wav") -> tempfile._TemporaryFileWrapper:
106106

107107

108108
def extract_user_id_from_speaker_id(speaker_id: str) -> str:
109-
"""Return the tenant that owns ``speaker_id``, read from the speaker row.
110-
111-
Deliberately a lookup rather than a parse. The id is *conventionally*
112-
``user_{user_id}_speaker_{hex}``, but that convention is not the ownership
113-
record — the ``speakers.user_id`` column is, and it stays right when a speaker is
114-
imported, renamed, or created by a client that formats ids differently. The old
115-
``int(speaker_id.split("_")[1])`` also stopped working the moment a tenant became
116-
a Chronicle ObjectId, which is not an integer.
109+
"""Return the tenant for ``speaker_id``: its recorded owner, else the id's prefix.
110+
111+
The stored ``speakers.user_id`` is the ownership record and wins whenever the
112+
speaker exists — it stays right for a speaker that was imported, renamed, or
113+
created by a client that formats ids differently, none of which the id convention
114+
survives.
115+
116+
The convention ``user_{user_id}_speaker_{...}`` is only consulted when no such
117+
speaker exists yet, because enrolment *creates* one and the id is currently how
118+
the caller states the tenant. That is a weak contract and the reason the audit
119+
says a tenant should not be inferred from a SpeakerId; removing it needs an
120+
explicit ``user_id`` on /enroll/upload, /enroll/batch and /enroll/append and on
121+
each of their callers (Chronicle's client, the speaker WebUI, and two scripts).
122+
123+
Returns a string either way. It used to return ``int(parts[1])``, which stopped
124+
parsing the moment a tenant became a Chronicle ObjectId.
117125
"""
118126

119-
# Imported here to avoid a circular import: database.queries imports the API
120-
# models that this utils module is itself imported by.
127+
# Imported here rather than at module scope: database.queries imports the API
128+
# models that this module is itself imported by.
121129
from simple_speaker_recognition.database import get_db_session
122130
from simple_speaker_recognition.database.models import Speaker
123131

124132
db = get_db_session()
125133
try:
126134
speaker = db.query(Speaker).filter(Speaker.id == speaker_id).first()
127-
if speaker is None:
128-
raise HTTPException(404, f"Speaker not found: {speaker_id}")
129-
return str(speaker.user_id)
135+
if speaker is not None:
136+
return str(speaker.user_id)
130137
finally:
131138
db.close()
132139

140+
if not speaker_id.startswith("user_"):
141+
raise HTTPException(
142+
400,
143+
"Unknown speaker and unrecognised id: expected "
144+
f"'user_{{user_id}}_...', got: {speaker_id}",
145+
)
146+
parts = speaker_id.split("_")
147+
if len(parts) < 3 or not parts[1]:
148+
raise HTTPException(400, f"Cannot determine the owning user from: {speaker_id}")
149+
return parts[1]
150+
133151

134152
def validate_confidence(confidence: Any, context: str = "") -> float:
135153
"""Validate and sanitize confidence values from speaker identification.

extras/speaker-recognition/tests/test_tenant_isolation.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
from sqlalchemy import create_engine
1919
from sqlalchemy.orm import sessionmaker
2020

21+
from simple_speaker_recognition import database
22+
from simple_speaker_recognition.api.core import utils
2123
from simple_speaker_recognition.core import unified_speaker_db
2224
from simple_speaker_recognition.core.unified_speaker_db import UnifiedSpeakerDB
2325
from simple_speaker_recognition.database import Base
@@ -152,3 +154,27 @@ def test_two_tenants_coexist_on_a_cold_database(tmp_path):
152154
assert sorted(u.id for u in UserQueries.get_all_users(session)) == sorted(
153155
[ALICE, BOB]
154156
)
157+
158+
159+
def test_the_owning_tenant_comes_from_the_record_not_the_id(tmp_path, monkeypatch):
160+
"""A stored owner beats the id convention, which can disagree with it.
161+
162+
Enrolment creates the speaker, so on that path the id is the only statement of
163+
the tenant and is honoured. Once a row exists the column is authoritative — an
164+
imported or re-owned speaker keeps an id whose prefix is simply wrong.
165+
"""
166+
167+
engine = create_engine(f"sqlite:///{tmp_path/'owners.db'}")
168+
Base.metadata.create_all(engine)
169+
factory = sessionmaker(bind=engine)
170+
monkeypatch.setattr(database, "get_db_session", factory, raising=False)
171+
172+
session = factory()
173+
UserQueries.get_or_create_user(session, ALICE)
174+
session.add(Speaker(id="user_1_legacy", name="Imported", user_id=ALICE))
175+
session.commit()
176+
177+
# The id says tenant "1"; the record says ALICE. The record wins.
178+
assert utils.extract_user_id_from_speaker_id("user_1_legacy") == ALICE
179+
# An unknown speaker is being created, so its id states the tenant.
180+
assert utils.extract_user_id_from_speaker_id(f"user_{ALICE}_speaker_ab12") == ALICE

0 commit comments

Comments
 (0)