From 31e237cfa6dad1fb5309c991d5193f8ccfb9c0fe Mon Sep 17 00:00:00 2001 From: tegwick Date: Fri, 24 Jul 2026 00:15:26 +0200 Subject: [PATCH] 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 --- src/tenant_engine/store.py | 63 ++++++++++++++++++++++--------- tests/test_api_reads.py | 13 +++++++ tests/test_store.py | 63 +++++++++++++++++++++++++++++++ workplans/ADHOC-2026-07-24.md | 71 +++++++++++++++++++++++++++++++++++ 4 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 workplans/ADHOC-2026-07-24.md diff --git a/src/tenant_engine/store.py b/src/tenant_engine/store.py index e394485..198c241 100644 --- a/src/tenant_engine/store.py +++ b/src/tenant_engine/store.py @@ -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) diff --git a/tests/test_api_reads.py b/tests/test_api_reads.py index 37d106b..7f7c3b5 100644 --- a/tests/test_api_reads.py +++ b/tests/test_api_reads.py @@ -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") diff --git a/tests/test_store.py b/tests/test_store.py index 54ad8bf..3570e0d 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -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( diff --git a/workplans/ADHOC-2026-07-24.md b/workplans/ADHOC-2026-07-24.md new file mode 100644 index 0000000..4bc4dad --- /dev/null +++ b/workplans/ADHOC-2026-07-24.md @@ -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`.