Skip to content

fix(auth): resolve service-principal created_by via shared identity store - #95

Merged
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/issue-501-service-identity-attribution
Sep 1, 2026
Merged

fix(auth): resolve service-principal created_by via shared identity store#95
Salil Das (sadlilas) merged 2 commits into
mainfrom
fix/issue-501-service-identity-attribution

Conversation

@colombod

@colombod Diego Colombo (colombod) commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Problem

In Entra auth, the service/app (managed-identity) token path derived created_by from a fallback chain — service_identities[oid] → appid → azp → oid. When the token's oid was not in the service identity map, the chain fell through to a raw GUID, so ingested data was silently attributed to a machine identifier instead of a contributor handle. That produces a phantom "contributor" in the graph, and nothing errors or logs.

The user-token path already 403s on an unmapped oid. Only the service path failed soft.

Fix — align the service path with the user path

auth.pycreated_by for a service token now comes solely from the identity map. An unmapped oid is a 403 naming the principal; there is no appid / azp / oid / app_displayname fallback.

main.py — one shared IdentityStore serves both user and service oids. Entra object IDs are disjoint across users and service principals, so a single oid → contributor map covers both. The resolver receives the store's live flat_dict as both maps, so a mapping added via PUT /admin/identities/{oid} takes effect immediately, with no restart.

config.pyservice_identities may be empty; an empty service map is the normal bootstrap state, not a startup error.

Startup warningservice_identities seeds the shared store on first boot only; once the store file exists, config is never re-read into it. Setting it on an already-deployed server therefore has no effect, and the only symptom would be a 403 whose message points at the administrator rather than at the ignored config. Boot now logs a WARNING naming any ignored oids and pointing at PUT /admin/identities/{oid}.

No new admin route and no new record fields — service identities are managed through the existing PUT / DELETE / GET /admin/identities endpoints with the existing record shape.

Documentation

AGENTS.md, README.md, docs/entra-auth-setup.md, docs/identity-management.md, docs/azure-deployment.md, docs/architecture/README.md, docs/m2-auth-acceptance.md, plus the three auth architecture diagrams re-rendered.

docs/m2-auth-acceptance.md needed particular care: the live-token acceptance gate asserted created_by == appid, which targeted the fallback chain this PR removes. Those assertions are marked superseded, and re-running that gate now requires the caller's oid to be mapped first.

Testing

  • pytest -m "not neo4j and not integration" (the CI selection): 1819 passed, 2 skipped
  • pyright on context_intelligence_server/: 0 errors
  • ruff format --check on all changed Python files: clean

New coverage: fail-loud 403 and no ingest on an unmapped service token; service-oid resolution through the single shared store; the resolver's user map, service map and the store's flat_dict being the same live object; empty service_identities booting cleanly; and the ignored-config startup warning (verified failing without the warning and passing with it).

Migration

⚠️ Behavior change: an unmapped service principal now receives a 403 instead of silently ingesting under a GUID.

Before, or immediately after, deploying — map the service principal's oid. Key on the service principal's object ID, not its appId:

PUT /admin/identities/{oid}
{"id": "<contributor>"}

This is the same call used to onboard a user, and it takes effect without a restart. Config service_identities will not work for this on a server whose identity store file already exists — the startup warning added here will say so explicitly.

Historical data: existing mis-attributed nodes are write-once (ON CREATE SET) and will not self-heal. They can be remapped with a one-off data fix once the mapping is in place — no code change required.

@colombod
Diego Colombo (colombod) force-pushed the fix/issue-501-service-identity-attribution branch from 9929b99 to 40fb7c1 Compare September 1, 2026 09:01
Salil Das (sadlilas) added a commit that referenced this pull request Sep 1, 2026
Refine PR #95 (service-principal identity resolution) by removing spec-
incompatible fields and addressing silent config misses.

PR #95 fixed a critical bug: when a service principal's Entra oid was not in
the identity map, created_by was silently stamped with a raw GUID instead of
a contributor handle. The fix (403 on unmapped service oid + resolve service
oids from the shared IdentityStore) is correct and retained untouched.

This commit simplifies the change by removing the type: "user" | "service"
record field introduced in PR #95. The field was never read by the auth path
— the resolver performs only oid -> contributor lookup — so it added durably
to the record contract while providing no value today. It also caused two
concrete problems: (1) boot rewrote the identity store file on every load
since the migration stamped type: "user" onto every record, and worse, this
was aliased to the api-keys.json IdentityStore in static mode, corrupting the
API-key file; (2) the admin DELETE ?type= filter did not gate the deletion,
recording caller assertion rather than ground truth.

The commit also adds a WARNING on startup when service_identities config is
being ignored. The config seeds the store only on first boot; once the store
file exists, config is never re-read. Silent identity-resolution misses are
the exact failure this PR was designed to prevent, so administrators now see
which oids are ignored and are pointed at PUT /admin/identities/{oid}.

identity_store.py and routers/admin.py are now byte-identical to main.

Verification (same scope as CI):
- pytest -m "not neo4j and not integration": 1819 passed, 2 skipped
- pyright on context_intelligence_server/: 0 errors
- ruff format --check: clean on all changed Python files
- startup-warning tests confirmed RED without warning, GREEN with it

Net effect: 785 insertions across 24 files → 557 insertions across 20 files.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@sadlilas
Salil Das (sadlilas) force-pushed the fix/issue-501-service-identity-attribution branch 2 times, most recently from 594aff6 to e3b5c09 Compare September 1, 2026 13:51
…tore

Service identity-map miss now raises 403 (fail-loud), mirroring user path
instead of silently stamping a raw GUID as created_by. One shared IdentityStore
serves user+service oids with disjoint keyspaces; type field (user|service)
normalized on load. Service identities managed via /admin/identities endpoint.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Follow-up on the service-principal attribution fix in this PR. The fix itself
is correct and is retained untouched: an unmapped service oid now gets a 403
instead of a raw-GUID `created_by`, and service oids resolve from the same
shared IdentityStore the user path already uses, so an operator can onboard
one at runtime with no redeploy.

Two changes here.

1. Remove the `type: "user" | "service"` record field.

   Nothing in the auth path ever read it — the resolver looks up
   `oid -> contributor` and nothing else — so it delivered no capability today
   while adding a field to the durable record contract, a load-time migration,
   and a `?type=` filter/param on the admin API. Removability is asymmetric:
   a record-contract field in a file that ships to production is hard to take
   back once it accrues consumers.

   Two concrete problems it introduced:

   - The same IdentityStore class backs `api-keys.json`. In static mode the
     load-time migration rewrote that file to stamp `type: "user"` onto every
     API-key record — a boot-time write to a file holding auth material, on a
     path unrelated to this bug, adding a field meaningless for a key digest.
   - `DELETE /admin/identities/{oid}?type=` did not gate the deletion (its own
     docstring said so), so the audit line recorded an unvalidated caller
     assertion instead of what was actually deleted.

   `identity_store.py`, `routers/admin.py` and their tests are byte-identical
   to main again.

2. Warn at startup when `service_identities` config is being ignored.

   `service_identities` seeds the shared store on first boot only; once the
   store file exists, config is never re-read into it. Setting it on an
   already-deployed server therefore has no effect, and the only symptom was a
   403 whose message points at the administrator rather than at the ignored
   config. Boot now logs a WARNING naming the ignored oids and pointing at
   `PUT /admin/identities/{oid}`. A silent identity-resolution miss is the
   exact failure this PR exists to fix; leaving a second one a few lines away
   was not acceptable.

Docs and the three auth architecture diagrams were updated to match, and the
diagram PNGs re-rendered with the repo's documented `dot -Tpng` command.

Verification (same scope as CI):
- pytest -m "not neo4j and not integration": 1819 passed, 2 skipped
- pyright on context_intelligence_server/: 0 errors
- ruff format --check: clean on all changed Python files
- the new startup-warning tests were confirmed RED with the warning disabled
  and GREEN with it enabled

Net effect on the PR: 785 insertions across 24 files -> 557 across 20.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@sadlilas
Salil Das (sadlilas) force-pushed the fix/issue-501-service-identity-attribution branch from e3b5c09 to 65708b9 Compare September 1, 2026 14:25
@sadlilas
Salil Das (sadlilas) merged commit 80e289f into main Sep 1, 2026
3 checks passed
@sadlilas
Salil Das (sadlilas) deleted the fix/issue-501-service-identity-attribution branch September 1, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants