fix(identity): login-bootstrap resolves by IdP source_type + external id, not email - #1989
Merged
Merged
Conversation
Contributor
|
📝 WalkthroughWalkthroughThe change adds source-scoped external-ID resolution for authenticator login bootstrap. It separates login resolution from email-based admin overrides, requires identity-resolution deployment and OIDC mapping configuration, updates deployment wiring, and seeds matching login-ID observations. ChangesIdentity-resolution login bootstrap
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OIDCProvider
participant Authenticator
participant IdentityResolution
participant PersonsRepository
OIDCProvider->>Authenticator: validated token with external_id_claim
Authenticator->>IdentityResolution: source_type and external_id lookup
IdentityResolution->>PersonsRepository: query latest id observation
PersonsRepository-->>IdentityResolution: person ID or no match
IdentityResolution-->>Authenticator: resolved person response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cyberantonz
reviewed
Jul 28, 2026
cyberantonz
reviewed
Jul 28, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
charts/insight/values.yaml (1)
491-510: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the runbook’s remaining .NET Identity requirements.
This makes
.NET identityoptional andidentityResolutionmandatory, butdeploy/HELM_DEPLOY.mdstill instructs users to setidentity.deploy: true, expects an Identity pod/config Secret during verification, and repeats that override in its appendix. Following the runbook now deploys an unnecessary legacy service and omits verification of the required resolver.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/insight/values.yaml` around lines 491 - 510, Update deploy/HELM_DEPLOY.md to remove the remaining .NET Identity requirements: stop instructing users to set identity.deploy: true, remove expectations for the legacy Identity pod and config Secret during verification, and delete the repeated override in the appendix. Replace those checks with verification that identityResolution is deployed and available.src/backend/services/authenticator/src/identity.rs (1)
160-190: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid embedding
__overrideparameters in the error message.
resolve_queryinterpolates{query:?}in its failure path, so overriding by email can put that raw query into the anyhow error.internal_problem()logs the full error chain (error = ?err), whileresolve_override()returns that same chain viaperson_resolution, so the override target email can reach logs and the internal-error response.🔒 Proposed fix: avoid raw query params in this error
anyhow::ensure!( resp.status().is_success(), - "Identity returned {} for {path}?{query:?}", + "Identity returned {} for {path}", resp.status() );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/src/identity.rs` around lines 160 - 190, Update the non-success error handling in resolve_query to stop interpolating the raw query parameters into the anyhow error; retain the response status and path context without exposing query values, including __override targets. Leave successful responses and NOT_FOUND handling unchanged.
🧹 Nitpick comments (2)
src/backend/services/authenticator/src/oidc.rs (1)
449-461: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMinor:
subisn't filtered for emptiness like the configured-claim branch.For consistency with the non-default branch (which filters empty values), consider filtering the
subcase too, so a malformed IdP that omits/emptiessubgets the same clear "claim missing" error instead of a generic downstream 400.♻️ Proposed tweak
fn extract_external_id(raw_id_token: &str, external_id_claim: &str, sub: &str) -> Option<String> { if external_id_claim == "sub" { - return Some(sub.to_owned()); + return Some(sub.to_owned()).filter(|v| !v.is_empty()); } payload_string(raw_id_token, external_id_claim).filter(|v| !v.is_empty()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/authenticator/src/oidc.rs` around lines 449 - 461, The sub branch in extract_external_id currently accepts an empty value unlike the configured-claim branch. Filter sub for emptiness as well, returning None when it is empty while preserving the existing Some result for non-empty sub values.src/backend/services/identity-resolution/src/api/handlers.rs (1)
149-153: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMissing vs. empty query params return inconsistent error shapes.
source_type/external_id/Option)Stringfields. When a param is entirely absent, axum'sQueryextractor rejects the request itself with a plain-text400body ("Failed to deserialize query string: missing field ..."), bypassing the handler entirely — differing from the JSONCanonicalErrorbody produced by the explicit empty-string checks below (Lines 182-191, 243-247). Both cases are "required field not usably present" from the caller's perspective but yield different response shapes.Consider making the fields
Option<String>and unifying both the missing- and empty-string cases through the sameProfileError::invalid_argument()path for a consistent internal API error contract.Confirmation of axum's default Query rejection behavior
Per axum's docs and rejection source, a
Queryextractor failure usesFailedToDeserializeQueryString, which is#[status = BAD_REQUEST]with body"Failed to deserialize query string"(plain text, not the service's JSON error format).Also applies to: 217-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/identity-resolution/src/api/handlers.rs` around lines 149 - 153, Update InternalByExternalIdQuery and the corresponding email query type to use Option<String> for required parameters, then validate missing and empty values in the handlers through the existing ProfileError::invalid_argument() path. Preserve the current validation semantics while ensuring absent and blank source_type, external_id, and email parameters return the same CanonicalError response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/HELM_DEPLOY.md`:
- Around line 248-274: Update the later installation and verification
instructions to reflect identity.deploy being optional and
identityResolution.deploy being required. Remove mandatory .NET identity pod and
Secret expectations, add identity-resolution deployment resources to the
expected resources, and ensure all referenced commands and checks use the
identityResolution topology consistently.
In `@src/backend/services/identity-resolution/src/infra/db/persons_repo.rs`:
- Around line 120-175: Update resolve_person_id_by_source_any_tenant so login
resolution cannot arbitrarily select a person when the (source_type,
external_id) pair collides across tenants. Enforce that login-enabled
source_type labels use a globally unique namespace, or require a globally unique
external-id claim such as Entra oid; remove the unconditional cross-tenant LIMIT
1 behavior unless this invariant is validated for every deployed source_type.
---
Outside diff comments:
In `@charts/insight/values.yaml`:
- Around line 491-510: Update deploy/HELM_DEPLOY.md to remove the remaining .NET
Identity requirements: stop instructing users to set identity.deploy: true,
remove expectations for the legacy Identity pod and config Secret during
verification, and delete the repeated override in the appendix. Replace those
checks with verification that identityResolution is deployed and available.
In `@src/backend/services/authenticator/src/identity.rs`:
- Around line 160-190: Update the non-success error handling in resolve_query to
stop interpolating the raw query parameters into the anyhow error; retain the
response status and path context without exposing query values, including
__override targets. Leave successful responses and NOT_FOUND handling unchanged.
---
Nitpick comments:
In `@src/backend/services/authenticator/src/oidc.rs`:
- Around line 449-461: The sub branch in extract_external_id currently accepts
an empty value unlike the configured-claim branch. Filter sub for emptiness as
well, returning None when it is empty while preserving the existing Some result
for non-empty sub values.
In `@src/backend/services/identity-resolution/src/api/handlers.rs`:
- Around line 149-153: Update InternalByExternalIdQuery and the corresponding
email query type to use Option<String> for required parameters, then validate
missing and empty values in the handlers through the existing
ProfileError::invalid_argument() path. Preserve the current validation semantics
while ensuring absent and blank source_type, external_id, and email parameters
return the same CanonicalError response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c45433ca-6321-4cd9-943b-e19cc3d2420c
📥 Commits
Reviewing files that changed from the base of the PR and between f322f40 and e8f254dbce634c178d164733bef96b33642ce4bb.
📒 Files selected for processing (29)
charts/insight/templates/_helpers.tplcharts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/HELM_DEPLOY.mddeploy/gitops/environments/functional-ci/values.yamldeploy/gitops/environments/local/values.yaml.templatedeploy/gitops/scripts/compose-app-secrets.shdeploy/seed/identity.pydeploy/seed/profiles.pydeploy/seed/test_identity.pydev-compose.shdocker-compose.ymlsrc/backend/services/authenticator/config/insight.yamlsrc/backend/services/authenticator/src/api/handlers.rssrc/backend/services/authenticator/src/config.rssrc/backend/services/authenticator/src/gear.rssrc/backend/services/authenticator/src/identity.rssrc/backend/services/authenticator/src/oidc.rssrc/backend/services/authenticator/tests/identity-stub.pysrc/backend/services/authenticator/tests/run-e2e.shsrc/backend/services/identity-resolution/README.mdsrc/backend/services/identity-resolution/src/api/gate.rssrc/backend/services/identity-resolution/src/api/handlers.rssrc/backend/services/identity-resolution/src/api/mod.rssrc/backend/services/identity-resolution/src/infra/db/persons_repo.rssrc/backend/services/identity-resolution/src/main.rssrc/ingestion/tests/e2e/identity/test_internal.pysrc/ingestion/tests/e2e/identity/test_persons_seed.pysrc/ingestion/tests/e2e/lib/identity_seed.py
cyberantonz
reviewed
Aug 3, 2026
| # The identity-resolution source_type this IdP is seeded under (e.g. | ||
| # "ms-entra") — required; drives the login-bootstrap person lookup | ||
| # (GET /internal/persons/by-external-id?source_type=...&external_id=...). | ||
| APP__gears__authenticator__config__idp__source_type: {{ required "authenticator.oidc.sourceType is required" .Values.authenticator.oidc.sourceType | quote }} |
Contributor
There was a problem hiding this comment.
Do not forget to put proper values to the gitlab gitops
cyberantonz
approved these changes
Aug 3, 2026
The login bootstrap resolved the caller by email, which breaks the moment
an IdP account has no email observation and silently ties login to a
mutable attribute. Replace it with two SEPARATE service-only contracts,
so a login that lacks its external id can never fall through to email:
- `GET /internal/persons/by-external-id?source_type=&external_id=` —
login bootstrap only, scoped to the configured IdP's source_type
(e.g. `ms-entra`) and its source-native user id (Entra: the `oid`
claim).
- `GET /internal/persons/by-email-override?email=` — the authenticator's
admin `__override` (view-as) lookup only.
The authenticator's `IdpIdentity` now carries an explicit
`ResolveTarget::ExternalId | Email` instead of inferring the mode from
field emptiness, and fails closed when the configured
`idp.external_id_claim` is absent from the id_token. It is wired to
identity-resolution unconditionally for this lookup, through
docker-compose, the Helm chart (new `insight.validate` check,
`identityResolution.deploy` defaulted to true) and
`compose-app-secrets.sh`.
Keycloak dev/demo seeding is fixed too: `gen-realm.py` pins every realm
user's `sub` to their own roster uuid, so the whole roster can log in
rather than only the dev lead. `deploy/seed/identity.py` makes its
login-id seed idempotent with an explicit existence check — migration
004 put `created_at` in the `persons` unique key, so `INSERT IGNORE` no
longer dedupes on its own.
Rebased onto main after the metrics person_id cutover. Three things the
cutover changed under this branch:
- `resolve_person_ids_by_emails` is gone from main (visible-persons now
filters by person id, not email), so it is dropped here; only the
`__override` doc note on `resolve_person_id_by_email_any_tenant`
survives.
- main's `return_to_prefix` config test builds its fixture from
`valid_config()`, which this branch made stricter by requiring
`idp.source_type` — the helper now sets it, or the fixture builds a
config `validate()` refuses.
- two e2e helpers still resolved a seeded person through the removed
`/internal/persons/by-email/{email}` and answered 404, which is what
the `identity-rust` e2e job was failing on; they now use the override
route.
Verified on the merged tree: `cargo test -p authenticator` 61/61 and
`-p identity-resolution` 88/88 against a live MariaDB, clippy and fmt
clean, `./e2e.sh test identity/` 149 passed / 3 skipped with the runner
image rebuilt from this branch, helm render-contract 14/14, identity
chart contract 17/17, `deploy/seed/test_identity.py` 2/2,
`docker compose config` valid.
Part of #1960.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
mozhaev-dev
force-pushed
the
feat/identity-resolve-by-external-id
branch
from
August 4, 2026 03:47
68124d0 to
9c666a4
Compare
Contributor
Author
|
Rebased onto current main (the branch was 144 commits behind) and force-pushed. CI is green; the branch now needs review only. History was rewritten — the ten commits (six real ones plus three stale "merge main" commits) are collapsed into a single signed commit. Every commit on the branch was missing Three things main changed under this branch, all of them fixed here:
Verified on the rebased tree, which closes the two unchecked boxes in the test plan above:
The rollout notes in the description still stand unchanged: this is a breaking chart upgrade, and logins 403 until |
ktursunov
approved these changes
Aug 4, 2026
mozhaev-dev
enabled auto-merge
August 4, 2026 05:10
…ve-by-external-id # Conflicts: # deploy/seed/identity.py # src/ingestion/tests/e2e/identity/test_internal.py # src/ingestion/tests/e2e/identity/test_persons_seed.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose.yml (1)
767-767: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
AUTH_MODEkey.
seed-sample.environmentdefinesAUTH_MODEtwice indocker-compose.yml: lines 735 and 767. Keep one assignment so the configuration has one unambiguous value.Proposed fix
- AUTH_MODE: "${AUTH_MODE:-fakeidp}" AUTHENTICATOR_OIDC_ISSUER: "${AUTHENTICATOR_OIDC_ISSUER:-}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` at line 767, The AUTH_MODE environment variable is defined twice in the seed-sample.environment configuration section of docker-compose.yml. Remove the duplicate AUTH_MODE assignment at line 767 to ensure a single, unambiguous configuration value. Retain the AUTH_MODE definition that appears earlier in the file to preserve the intended default behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/gitops/environments/functional-ci/values.yaml`:
- Around line 63-74: Remove the duplicate identityResolution mapping in the
values configuration, keeping a single deploy: true declaration. Preserve the
explanatory comment by placing it directly above the remaining
identityResolution block.
---
Outside diff comments:
In `@docker-compose.yml`:
- Line 767: The AUTH_MODE environment variable is defined twice in the
seed-sample.environment configuration section of docker-compose.yml. Remove the
duplicate AUTH_MODE assignment at line 767 to ensure a single, unambiguous
configuration value. Retain the AUTH_MODE definition that appears earlier in the
file to preserve the intended default behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52b59fc0-3abd-49c1-bf2b-077b2c73a72a
📥 Commits
Reviewing files that changed from the base of the PR and between e8f254dbce634c178d164733bef96b33642ce4bb and 6d24615.
📒 Files selected for processing (27)
charts/insight/templates/_helpers.tplcharts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/HELM_DEPLOY.mddeploy/gitops/environments/functional-ci/values.yamldeploy/gitops/environments/local/values.yaml.templatedeploy/gitops/scripts/compose-app-secrets.shdeploy/seed/identity.pydeploy/seed/profiles.pydeploy/seed/test_identity.pydev-compose.shdocker-compose.ymlsrc/backend/services/authenticator/config/insight.yamlsrc/backend/services/authenticator/src/api/handlers.rssrc/backend/services/authenticator/src/config.rssrc/backend/services/authenticator/src/gear.rssrc/backend/services/authenticator/src/identity.rssrc/backend/services/authenticator/src/oidc.rssrc/backend/services/authenticator/tests/identity-stub.pysrc/backend/services/authenticator/tests/run-e2e.shsrc/backend/services/identity-resolution/README.mdsrc/backend/services/identity-resolution/helm/tests/test_seed_cronjob_contract.pysrc/backend/services/identity-resolution/src/api/gate.rssrc/backend/services/identity-resolution/src/api/handlers.rssrc/backend/services/identity-resolution/src/api/mod.rssrc/backend/services/identity-resolution/src/infra/db/persons_repo.rssrc/backend/services/identity-resolution/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- src/backend/services/identity-resolution/src/api/gate.rs
- src/backend/services/authenticator/src/gear.rs
- src/backend/services/identity-resolution/src/api/mod.rs
- charts/insight/values.yaml
- src/backend/services/authenticator/src/api/handlers.rs
- deploy/seed/profiles.py
- src/backend/services/authenticator/src/config.rs
- src/backend/services/identity-resolution/src/main.rs
- deploy/seed/identity.py
- deploy/HELM_DEPLOY.md
- src/backend/services/identity-resolution/README.md
- charts/insight/templates/secrets.yaml
- src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
- deploy/seed/test_identity.py
- src/backend/services/authenticator/src/oidc.rs
- src/backend/services/authenticator/config/insight.yaml
- src/backend/services/authenticator/src/identity.rs
- deploy/gitops/environments/local/values.yaml.template
- src/backend/services/identity-resolution/src/api/handlers.rs
- charts/insight/templates/_helpers.tpl
Comment on lines
63
to
+74
| identityResolution: | ||
| deploy: true | ||
|
|
||
| # The authenticator's login-bootstrap resolve (GET /internal/persons/ | ||
| # by-external-id / by-email-override) is Rust-only — the frozen .NET | ||
| # `identity` twin above never gained it (constructorfabric/insight#1960) — so | ||
| # identity-resolution must ALSO be deployed here, and the authenticator talks | ||
| # to it unconditionally (deploy/gitops/scripts/compose-app-secrets.sh enforces | ||
| # this at apply time regardless of what `.identityUrl` — analytics' own | ||
| # .NET<->Rust switch — is set to). | ||
| identityResolution: | ||
| deploy: true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Remove the duplicated identityResolution mapping.
identityResolution is declared twice: at Line 63 and again at Line 73. The new comment block was inserted before a second copy of the key instead of before the existing one. Strict YAML parsers reject duplicate mapping keys, and yamllint already fails on it. Keep one mapping with the comment above it.
🐛 Proposed fix
-identityResolution:
- deploy: true
-
# The authenticator's login-bootstrap resolve (GET /internal/persons/
# by-external-id / by-email-override) is Rust-only — the frozen .NET
# `identity` twin above never gained it (constructorfabric/insight#1960) — so
# identity-resolution must ALSO be deployed here, and the authenticator talks
# to it unconditionally (deploy/gitops/scripts/compose-app-secrets.sh enforces
# this at apply time regardless of what `.identityUrl` — analytics' own
# .NET<->Rust switch — is set to).
identityResolution:
deploy: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| identityResolution: | |
| deploy: true | |
| # The authenticator's login-bootstrap resolve (GET /internal/persons/ | |
| # by-external-id / by-email-override) is Rust-only — the frozen .NET | |
| # `identity` twin above never gained it (constructorfabric/insight#1960) — so | |
| # identity-resolution must ALSO be deployed here, and the authenticator talks | |
| # to it unconditionally (deploy/gitops/scripts/compose-app-secrets.sh enforces | |
| # this at apply time regardless of what `.identityUrl` — analytics' own | |
| # .NET<->Rust switch — is set to). | |
| identityResolution: | |
| deploy: true | |
| # The authenticator's login-bootstrap resolve (GET /internal/persons/ | |
| # by-external-id / by-email-override) is Rust-only — the frozen .NET | |
| # `identity` twin above never gained it (constructorfabric/insight#1960) — so | |
| # identity-resolution must ALSO be deployed here, and the authenticator talks | |
| # to it unconditionally (deploy/gitops/scripts/compose-app-secrets.sh enforces | |
| # this at apply time regardless of what `.identityUrl` — analytics' own | |
| # .NET<->Rust switch — is set to). | |
| identityResolution: | |
| deploy: true |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 73-73: duplication of key "identityResolution" in mapping
(key-duplicates)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/functional-ci/values.yaml` around lines 63 - 74,
Remove the duplicate identityResolution mapping in the values configuration,
keeping a single deploy: true declaration. Preserve the explanatory comment by
placing it directly above the remaining identityResolution block.
Source: Linters/SAST tools
cyberantonz
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
value_typedispatch, so a login that lacks its external id can never silently fall through to email):GET /internal/persons/by-external-id?source_type=...&external_id=...— login bootstrap ONLY, scoped to the configured IdP'ssource_type(e.g.ms-entra) + its source-native external user id (Entra: theoidclaim).GET /internal/persons/by-email-override?email=...— the authenticator's admin__override(view-as, Admin 'view as another user' override (?__override=<email>) no longer works #1941) lookup ONLY.authenticator'sIdpIdentitynow carries an explicitResolveTarget::ExternalId(String) | Email(String)enum instead of inferring the resolution mode from field emptiness, and fails closed (login refused) if the configuredidp.external_id_claimis absent from the id_token.identity-resolution(Rust) for this lookup — the.NET identitytwin was intentionally NOT given an equivalent endpoint (out of scope by explicit direction;.NET identityservice is untouched by this PR). Wired through docker-compose, the Helm chart (newinsight.validatecheck +identityResolution.deploydefault flipped totrue), and the gitopscompose-app-secrets.shscript (separateAUTHENTICATOR_IDENTITY_URL+ requiredidp.source_type/idp.external_id_claim).gen-realm.pypins every realm user'ssubto their own roster uuid.deploy/seed/identity.py's login-id seed is idempotent via an explicit existence check (notINSERT IGNORE— migration 004 putcreated_atinpersons' unique key, so re-runs no longer dedupe on their own).Part of #1960 (see the issue's "Update (post-implementation)" section for the final-scope writeup, including why the email lookup is intentionally kept for
__override).Test plan
cargo fmt --all -- --check(backend workspace) — cleancargo test -p authenticator -p identity-resolution— 54 + 58 unit tests passcargo clippy -p authenticator -p identity-resolution --all-targets -- -D warnings— cleansrc/backend/services/authenticator/tests/run-e2e.sh(real fakeidp + authenticator + Redis stack) — all scenarios pass, incl. login and__overridepython3 -m unittest deploy/seed/test_identity.py -v— 2/2 pass (idempotency + Keycloak whole-roster seeding)helm templateof the umbrella chart (functional-ci values + local overlay values.yaml.template) — renders cleanly,identity_url/idp.source_type/idp.external_id_claimverified in outputdocker compose config -q— validtest_internal.py,test_persons_seed.py) — not run locally (needs the identity-resolution + MariaDB + ClickHouse compose stack)helm install/ArgoCD sync — onlyhelm templatewas run, no live cluster availableRollout notes (not code changes, just flagging for reviewers)
identityResolution.deploy: true+authenticator.oidc.sourceTypeset, the render/gitops-compose fails closed with a clear error; an authenticator upgraded to the new image but still on an old secret (missingidp.source_type) will crash-loop at boot validation.personshasvalue_type='id'rows under the rightsource_type(for Entra: a fresh persons-seed run after thems-entraconnector sync). Rollout order: seed → deploy.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
sub.Bug Fixes
Documentation