🧪 Auto-PR: Merge develop → test - #1028
Merged
Merged
Conversation
The route takes the LOCAL Keycloak user as a path parameter and the REMOTE subject in the body, and both were named `userId`. Any client that flattens path and body into one argument list — the generated MCP tool surface does — collapses them and drops one, so the endpoint could not be called at all. That is not cosmetic. A user created out-of-band has no broker link, and Keycloak's first-broker login then demands proof of ownership a passwordless account cannot give: it offers email verification (needs realm SMTP) or re-authentication with a password that does not exist. Writing the link is the repair, and it was unreachable — so the account had to be deleted and recreated through the IdP instead. The body now says what it means: providerUserId and providerUserName. The UI client is generated and gitignored, so it picks the rename up on the next build; the one hand-written caller is updated here, and typecheck caught it, which is the point of the rename.
The patient picker is a searchable directory of every Patient on the server, and /auth/patient-search proxies to FHIR with NO bearer token — its own description says so. Reaching it required nothing but a launch session key: no role check, no fhirUser check, no scope check. The only thing keeping patients out was `needsPatientPicker`, which goes false once a patient resolves from fhirUser. That is a convenience, not a gate: an identity that did not resolve — a missing claim, an unreachable Keycloak, an app whose patientFacing was never set — fell through to the same directory a patient would have seen. The code even says "they must never see the picker" while implementing it as a best-effort skip. The rule is now positive and fails closed: a practitioner fhirUser, or no picker. The callback gate records `pickerAllowed` on the session, and both /auth/patient-select and /auth/patient-search refuse without it, so the gate is not the only thing standing in front of the data. The integration fixtures that asserted the fall-through now carry a practitioner identity, which is what production actually has — the resolver writes fhirUser onto the session while looking for a patient. NOT fixed here: patient-search still reaches FHIR unauthenticated. Even gated, it should carry the user's own authorisation so the server applies its scopes. That is a larger change and wants testing against beta.
Measured on production today. Publishing three apps left two, and each publish dropped the one before it: publish dicom-viewer -> [dicom-viewer] publish consent-app -> [dicom-viewer, consent-app] publish patient-portal -> [dicom-viewer, patient-portal] <- gone Every mutation was read-modify-write over a whole JSON document: read through a 5-second cache, mutate in memory, `set` the result last-writer-wins. With more than one task there is no coordination, so a task that had never seen `consent-app` rewrote the document without it. Nothing errored. The entry simply ceased to exist, which is the worst shape a config bug can take — the admin UI reports success either way. `admin_config` gains a `version` column (idempotent ALTER, no migration needed) and the Postgres backend gains compare-and-set: an UPDATE that lands only while the revision is unchanged, and a conditional INSERT for first write. `AdminConfigStore.mutate` reads the CURRENT value, applies the caller's update and writes under that revision, retrying when someone else got there first. Backends without compare-and-set — the file fallback, which is single-task by definition — keep a plain read-modify-write. Mutations are also chained per key so two requests on the same task queue instead of interleaving. The app-store publish/unpublish/hide/show now build the next document from the current one rather than from a cached copy, so a concurrent publish is merged instead of dropped. They are async as a result; the four routes await them. The comments claiming this was already "cluster-safe" described the goal, not the behaviour.
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
1 participant
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.
Automated Pull Request 🤖
Commits ahead of test: 6
Commits behind test: 0
TODOs remaining in codebase: 3
Last updated: 2026-08-16 18:17:18 UTC