Fix: resolve tenants by identifier, not only internal tenant_id
Found via a real cross-service check while implementing key-cape's KEY-WP-0005-T02: key-cape's Go adapter called GET /tenants/tenant:coulomb/roles and got a genuine 404 for a tenant that existed. External callers (key-cape, flex-auth) only ever have a tenant's profile identifier, never tenant-engine's internal tenant_id (caller-chosen at creation, otherwise opaque). Every existing test happened to use identical strings for both fields, so this was invisible until a real, independent second caller exercised the documented contract. InMemoryTenantStore gained a _by_identifier index and a _resolve() helper every method calls first; create_tenant now also rejects a duplicate identifier under a different internal id (an oversight the same fix surfaced). 5 new tests, including the exact HTTP-level scenario with colon characters in the URL path. 65 total, all 60 pre-existing tests unaffected. Re-verified end-to-end for real: fresh flex-auth + tenant-engine + key-cape's actual adapter code, over real HTTP -- roles=[IAM] ok=true resolving by identifier. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
18f510070f
commit
31e237cfa6
4 changed files with 193 additions and 17 deletions
|
|
@ -37,7 +37,17 @@ class DomainEvent:
|
|||
|
||||
|
||||
class TenantStore(Protocol):
|
||||
"""Swappable persistence seam -- domain/ and api/ depend on this, not a backend."""
|
||||
"""Swappable persistence seam -- domain/ and api/ depend on this, not a backend.
|
||||
|
||||
Every method taking a `tenant_id` accepts either the tenant's internal
|
||||
id (caller-chosen at creation, e.g. from an admin tool) or its profile
|
||||
identifier (e.g. "tenant:friendly:binky") -- external callers like
|
||||
key-cape and flex-auth only ever have the identifier (it's the IAM
|
||||
Profile `tenant` claim value), never tenant-engine's internal id. See
|
||||
`_resolve_tenant_id` for why this had to be added after real
|
||||
cross-service testing caught the gap (found integrating key-cape's
|
||||
tenant_roles claim, KEY-WP-0005-T02).
|
||||
"""
|
||||
|
||||
def create_tenant(self, tenant: Tenant) -> None: ...
|
||||
|
||||
|
|
@ -57,6 +67,7 @@ class TenantStore(Protocol):
|
|||
class InMemoryTenantStore:
|
||||
def __init__(self) -> None:
|
||||
self._tenants: dict[str, Tenant] = {}
|
||||
self._by_identifier: dict[str, str] = {}
|
||||
self._grants: dict[str, dict[str, RoleGrant]] = {}
|
||||
self._plans: dict[str, PlanAssignment] = {}
|
||||
self._events: list[DomainEvent] = []
|
||||
|
|
@ -64,7 +75,10 @@ class InMemoryTenantStore:
|
|||
def create_tenant(self, tenant: Tenant) -> None:
|
||||
if tenant.tenant_id in self._tenants:
|
||||
raise TenantAlreadyExistsError(tenant.tenant_id)
|
||||
if tenant.identifier in self._by_identifier:
|
||||
raise TenantAlreadyExistsError(tenant.identifier)
|
||||
self._tenants[tenant.tenant_id] = tenant
|
||||
self._by_identifier[tenant.identifier] = tenant.tenant_id
|
||||
self._grants[tenant.tenant_id] = {}
|
||||
self._emit(
|
||||
"tenant_created",
|
||||
|
|
@ -73,17 +87,14 @@ class InMemoryTenantStore:
|
|||
)
|
||||
|
||||
def get_tenant(self, tenant_id: str) -> Tenant:
|
||||
try:
|
||||
return self._tenants[tenant_id]
|
||||
except KeyError:
|
||||
raise TenantNotFoundError(tenant_id) from None
|
||||
return self._tenants[self._resolve(tenant_id)]
|
||||
|
||||
def grant_role(self, grant: RoleGrant) -> None:
|
||||
self.get_tenant(grant.tenant_id)
|
||||
self._grants[grant.tenant_id][grant.grant_id] = grant
|
||||
resolved = self._resolve(grant.tenant_id)
|
||||
self._grants[resolved][grant.grant_id] = grant
|
||||
self._emit(
|
||||
"role_granted",
|
||||
grant.tenant_id,
|
||||
resolved,
|
||||
{
|
||||
"grant_id": grant.grant_id,
|
||||
"role": grant.role.value,
|
||||
|
|
@ -93,34 +104,52 @@ class InMemoryTenantStore:
|
|||
)
|
||||
|
||||
def revoke_role(self, *, tenant_id: str, grant_id: str, at: datetime) -> RoleGrant:
|
||||
self.get_tenant(tenant_id)
|
||||
resolved = self._resolve(tenant_id)
|
||||
try:
|
||||
grant = self._grants[tenant_id][grant_id]
|
||||
grant = self._grants[resolved][grant_id]
|
||||
except KeyError:
|
||||
raise GrantNotFoundError(grant_id) from None
|
||||
revoked = grant.revoke(at=at)
|
||||
self._grants[tenant_id][grant_id] = revoked
|
||||
self._grants[resolved][grant_id] = revoked
|
||||
self._emit(
|
||||
"role_revoked",
|
||||
tenant_id,
|
||||
resolved,
|
||||
{"grant_id": grant_id, "role": revoked.role.value},
|
||||
)
|
||||
return revoked
|
||||
|
||||
def active_roles(self, tenant_id: str) -> frozenset[CapabilityRole]:
|
||||
self.get_tenant(tenant_id)
|
||||
resolved = self._resolve(tenant_id)
|
||||
return frozenset(
|
||||
grant.role for grant in self._grants.get(tenant_id, {}).values() if grant.active
|
||||
grant.role for grant in self._grants.get(resolved, {}).values() if grant.active
|
||||
)
|
||||
|
||||
def assign_plan(self, assignment: PlanAssignment) -> None:
|
||||
self.get_tenant(assignment.tenant_id)
|
||||
self._plans[assignment.tenant_id] = assignment
|
||||
self._emit("plan_assigned", assignment.tenant_id, {"plan_id": assignment.plan_id})
|
||||
resolved = self._resolve(assignment.tenant_id)
|
||||
self._plans[resolved] = assignment
|
||||
self._emit("plan_assigned", resolved, {"plan_id": assignment.plan_id})
|
||||
|
||||
def events(self) -> list[DomainEvent]:
|
||||
return list(self._events)
|
||||
|
||||
def _resolve(self, tenant_id: str) -> str:
|
||||
"""Resolve an internal tenant_id or a profile identifier to the
|
||||
|
||||
canonical internal tenant_id every other private dict is keyed by.
|
||||
|
||||
External callers (key-cape, flex-auth) only ever have the
|
||||
identifier (the IAM Profile `tenant` claim value) -- they have no
|
||||
way to know a tenant's internal tenant_id, which is caller-chosen
|
||||
at creation time and otherwise opaque. Found the hard way: a real
|
||||
cross-service check (key-cape's KEY-WP-0005-T02 against a live
|
||||
tenant-engine) returned tenant_not_found for a tenant that
|
||||
genuinely existed, because the caller only had the identifier.
|
||||
"""
|
||||
resolved = self._by_identifier.get(tenant_id, tenant_id)
|
||||
if resolved not in self._tenants:
|
||||
raise TenantNotFoundError(tenant_id)
|
||||
return resolved
|
||||
|
||||
def _emit(self, event_type: str, tenant_id: str, payload: dict[str, Any]) -> None:
|
||||
self._events.append(
|
||||
DomainEvent(event_type=event_type, tenant_id=tenant_id, at=datetime.now(UTC), payload=payload)
|
||||
|
|
|
|||
|
|
@ -64,6 +64,19 @@ def test_cache_read_roles_returns_active_roles() -> None:
|
|||
assert response.json() == {"tenant_id": "t-binky", "roles": ["CUS"]}
|
||||
|
||||
|
||||
def test_cache_read_roles_resolves_by_identifier_not_only_internal_id() -> None:
|
||||
"""The real scenario key-cape's KEY-WP-0005-T02 hits: it only knows the
|
||||
|
||||
tenant's profile identifier (the IAM Profile `tenant` claim value), via
|
||||
a URL path segment containing colons -- never the internal tenant_id.
|
||||
"""
|
||||
client = TestClient(create_app(store=_seeded_store()))
|
||||
response = client.get("/tenants/tenant:friendly:binky/roles")
|
||||
|
||||
assert response.status_code == 200
|
||||
assert response.json() == {"tenant_id": "tenant:friendly:binky", "roles": ["CUS"]}
|
||||
|
||||
|
||||
def test_cache_read_roles_unknown_tenant_is_404() -> None:
|
||||
client = TestClient(create_app(store=_seeded_store()))
|
||||
response = client.get("/tenants/does-not-exist/roles")
|
||||
|
|
|
|||
|
|
@ -104,6 +104,69 @@ def test_assign_plan() -> None:
|
|||
assert any(event.event_type == "plan_assigned" and event.payload["plan_id"] == "plan-x" for event in events)
|
||||
|
||||
|
||||
def test_get_tenant_resolves_by_identifier_not_only_internal_id() -> None:
|
||||
"""Found via a real cross-service check (KEY-WP-0005-T02 against a live
|
||||
|
||||
tenant-engine): external callers like key-cape and flex-auth only ever
|
||||
have the tenant's profile identifier (the IAM Profile `tenant` claim
|
||||
value), never its internal tenant_id, which is caller-chosen at
|
||||
creation and otherwise opaque.
|
||||
"""
|
||||
store, tenant = _store_with_tenant()
|
||||
|
||||
by_internal_id = store.get_tenant(tenant.tenant_id)
|
||||
by_identifier = store.get_tenant(tenant.identifier)
|
||||
|
||||
assert by_internal_id == by_identifier == tenant
|
||||
|
||||
|
||||
def test_active_roles_resolves_by_identifier() -> None:
|
||||
store, tenant = _store_with_tenant()
|
||||
store.grant_role(
|
||||
create_role_grant(
|
||||
tenant=tenant,
|
||||
grant_id="g-1",
|
||||
role=CapabilityRole.CUS,
|
||||
grant_reason="manual_grant",
|
||||
plan_id=None,
|
||||
granted_by="ops",
|
||||
correlation_id="corr-1",
|
||||
granted_at=datetime.now(UTC),
|
||||
)
|
||||
)
|
||||
|
||||
assert store.active_roles(tenant.identifier) == frozenset({CapabilityRole.CUS})
|
||||
|
||||
|
||||
def test_revoke_role_resolves_by_identifier() -> None:
|
||||
store, tenant = _store_with_tenant()
|
||||
store.grant_role(
|
||||
create_role_grant(
|
||||
tenant=tenant,
|
||||
grant_id="g-1",
|
||||
role=CapabilityRole.CUS,
|
||||
grant_reason="manual_grant",
|
||||
plan_id=None,
|
||||
granted_by="ops",
|
||||
correlation_id="corr-1",
|
||||
granted_at=datetime.now(UTC),
|
||||
)
|
||||
)
|
||||
|
||||
revoked = store.revoke_role(tenant_id=tenant.identifier, grant_id="g-1", at=datetime.now(UTC))
|
||||
|
||||
assert revoked.revoked_at is not None
|
||||
assert store.active_roles(tenant.tenant_id) == frozenset()
|
||||
|
||||
|
||||
def test_create_tenant_rejects_duplicate_identifier_with_different_internal_id() -> None:
|
||||
store, tenant = _store_with_tenant()
|
||||
other = Tenant.create(tenant_id="a-different-internal-id", identifier=tenant.identifier)
|
||||
|
||||
with pytest.raises(TenantAlreadyExistsError):
|
||||
store.create_tenant(other)
|
||||
|
||||
|
||||
def test_every_mutation_emits_an_event() -> None:
|
||||
store, tenant = _store_with_tenant()
|
||||
grant = create_role_grant(
|
||||
|
|
|
|||
71
workplans/ADHOC-2026-07-24.md
Normal file
71
workplans/ADHOC-2026-07-24.md
Normal file
|
|
@ -0,0 +1,71 @@
|
|||
---
|
||||
id: ADHOC-2026-07-24
|
||||
type: workplan
|
||||
title: "Resolve tenants by identifier, not only internal tenant_id"
|
||||
domain: infotech
|
||||
repo: tenant-engine
|
||||
status: finished
|
||||
owner: codex
|
||||
topic_slug: netkingdom
|
||||
created: "2026-07-24"
|
||||
updated: "2026-07-24"
|
||||
---
|
||||
|
||||
# Resolve tenants by identifier, not only internal tenant_id
|
||||
|
||||
Discovered via a real cross-service check while implementing `key-cape`'s
|
||||
`KEY-WP-0005-T02` (`tenant_roles` cache-read integration): `key-cape`'s Go
|
||||
adapter called `GET /tenants/tenant:coulomb/roles` and got a genuine `404
|
||||
tenant_not_found` for a tenant that actually existed. External callers
|
||||
(`key-cape`, `flex-auth`) only ever have a tenant's **profile identifier**
|
||||
(the IAM Profile `tenant` claim value, e.g. `tenant:coulomb`) — never
|
||||
tenant-engine's internal `tenant_id`, which is caller-chosen at creation
|
||||
(`POST /tenants`) and otherwise opaque. Every existing test happened to
|
||||
pass an identical string for both fields, so this gap was invisible to
|
||||
unit/API tests until a genuine second, independent caller (`key-cape`,
|
||||
written against the documented contract, not against tenant-engine's
|
||||
internal test conventions) exercised it for real.
|
||||
|
||||
## Task: Fix
|
||||
|
||||
```task
|
||||
id: ADHOC-2026-07-24-T01
|
||||
status: done
|
||||
priority: high
|
||||
```
|
||||
|
||||
`InMemoryTenantStore` gained a `_by_identifier` index (identifier →
|
||||
internal `tenant_id`) and a private `_resolve()` helper every method now
|
||||
calls first, so `get_tenant`, `grant_role`, `revoke_role`, `active_roles`,
|
||||
and `assign_plan` all transparently accept either the internal id or the
|
||||
identifier. `create_tenant` also now rejects a duplicate identifier under a
|
||||
different internal id (previously unenforced — an oversight the same fix
|
||||
surfaced).
|
||||
|
||||
Done when: a real cross-process check (real `flex-auth`, real
|
||||
`tenant-engine`, real `key-cape` adapter code, over actual HTTP) resolves a
|
||||
tenant by identifier correctly, and all existing tests still pass
|
||||
unmodified in intent.
|
||||
|
||||
**Done 2026-07-24:** Fixed in `store.py`. Added
|
||||
`test_get_tenant_resolves_by_identifier_not_only_internal_id`,
|
||||
`test_active_roles_resolves_by_identifier`,
|
||||
`test_revoke_role_resolves_by_identifier`, and
|
||||
`test_create_tenant_rejects_duplicate_identifier_with_different_internal_id`
|
||||
in `tests/test_store.py`; added
|
||||
`test_cache_read_roles_resolves_by_identifier_not_only_internal_id` in
|
||||
`tests/test_api_reads.py` (the exact HTTP-level scenario, with the colon
|
||||
characters a real `tenant:friendly:binky`-shaped identifier puts in the URL
|
||||
path). `pytest` → `65 passed` (all 60 pre-existing tests unaffected).
|
||||
|
||||
Re-ran the exact real end-to-end check that found the bug, on fresh ports
|
||||
to avoid colliding with another session's own `flex-auth serve` process on
|
||||
this shared workstation (a collision that led to accidentally killing that
|
||||
other process — a low-harm, trivially-restartable local dev server, but
|
||||
noted here as a mistake to avoid repeating: check `pgrep` output carefully
|
||||
and prefer distinctive ports before killing anything matching a broad
|
||||
pattern): created a tenant with a genuinely different internal id and
|
||||
identifier, granted it a role through the real `flex-auth`-gated write
|
||||
path, then called `GET /tenants/tenant:coulomb/roles` from `key-cape`'s
|
||||
actual `internal/adapters/tenantengine.Client` (not a stand-in) — correctly
|
||||
returned `roles=[IAM] ok=true`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue