LEGLINK-620: Add-Secret-Key-to-Vendor - #1788
Merged
Merged
Conversation
Captures the decisions behind associating a Key Vault secret with a vendor, so the reasoning survives outside a chat log: why the association is vendor-scoped rather than facility-scoped (Veradigm needs a different key generation algorithm than Epic and Cerner), and why the stored value is the Key Vault secret id rather than a JWKS kid, which LEGLINK-63's title conflates. Also records what this ticket deliberately cannot do. The update endpoint that persists a secret id is owned by neither LEGLINK-620 nor LEGLINK-743, so the UI is built against a single isolated service method. LEGLINK-63's audit-trail acceptance criterion is produced from backend managers onto Kafka and no UI change can satisfy it. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo
Link signs the JWT that Data Acquisition presents to Epic and Cerner during client-credentials auth. LEGLINK-14 moved the PEM into Key Vault; this makes the association explicit and vendor-scoped, because Veradigm needs a different key generation algorithm and so cannot share a key. Add an edit path to Vendor Management. The dashboard grows a Secret ID column that reads "Not set" when empty and a per-row edit action; the form grows a JWT / Authentication panel holding the Key Vault Secret ID, expanded when a vendor already has one so existing configuration is visible without hunting. An emptied box travels as undefined rather than "", so clearing the association reads as absent rather than set-to-empty. No update endpoint exists yet. The Vendor model is moving out of Normalization into Tenant under LEGLINK-743, whose acceptance criteria cover list, add and delete but not update, so this operation is owned by neither ticket. VendorService.updateVendor is the single place that knows the route: when the contract lands, that method is the only edit required. A config-flagged dual path was considered and rejected as permanent complexity bought against a decision expected within days. Two defects in files this already touches: createVendor was typed as IVendorConfigModel while callers pass a name string, which interpolated "[object Object]" into the URL for anything else; and getVendors never cleared its loading flag on success. Deferred: the mocked Playwright spec the design calls for. That harness arrives with PR #1773, which is not yet merged into dev, so there is nowhere on this branch for the spec to live. LEGLINK-63's audit-trail criterion is also outstanding -- audit events are produced from backend managers onto Kafka, so it belongs with the update endpoint rather than here. 16 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo
…itly Two defects found reviewing 4b894e1 against the design. ErrorHandlingService raises its own toastr before rethrowing, so once the save paths began emitting failure to the dialog -- which shows a snackbar and stays open so the admin's input survives -- one failed save reported itself twice, toastr bottom-full-width and snackbar top-right. Saves now route through handleSaveError, which suppresses the toastr and leaves the dialog as the single surface. List and delete keep theirs, having no dialog to carry the news. The rethrown error still carries the sanitized message either way. Clearing a key sent secretId as undefined, which JSON.stringify drops, so the field never reached the wire. An absent field reads as "leave unchanged" to any endpoint with partial-update semantics, which would have made clearing an association succeed visibly and do nothing. It now travels as an explicit null, and the design's open items record that the backend must honour null as "remove the association" when the contract is settled. Adds vendor.service.spec.ts, the service having had no direct coverage: the update route and body, a cleared key surviving serialization, name escaping in the create route, and the toastr suppressed for saves but kept for list and delete. Both new behaviours fail against the previous code -- args[1] was absent rather than false, and secretId was undefined rather than null. 21 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01CX9BzMrPhTzGSakXYDaAVa
A failed createVendor had no test. Assert it emits a single failure with the error message and does not fall through into the update path. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is
VendorController exposes list, add and delete only -- no PUT -- so the edit dialog added on this branch would save into a 404. Put the edit button and onEdit behind a vendorEditEnabled config flag, shipped off, following the existing AppConfig boolean pattern. Flip it once the update contract, including clearing secretId with null, is confirmed. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is
Validation lives on Admin.BFF, which already holds an ISecretManager and is independent of the Vendor model's move to Tenant. Adds ISecretInspector and PemSigningKeyValidator to Shared; the UI warns inline on blur and on save without ever blocking the save. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is
Eight tasks, TDD throughout: characterize EpicAuth's PKCS#8 behavior, then PemSigningKeyValidator and ISecretInspector in Shared, the Admin.BFF endpoint, the Angular service call, and the form's blur/save warnings. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is
Vendor moved into Tenant under LEGLINK-743 carrying only Id and Name, so there
was nowhere to record the Key Vault secret holding a vendor's PEM signing key.
LEGLINK-63 scopes that key to the vendor rather than the facility, because the
key generation algorithm differs by EHR.
Stored as a JSON column rather than a plain one so later vendor-level auth
settings need no migration, following the AuthenticationConfiguration precedent
in DataAcquisitionDbContext. Only the signing key lives here: TokenUrl, Audience
and ClientId are per-EHR-instance and stay on the facility's authentication
configuration, where EpicAuth already reads them.
The value converter carries an explicit ValueComparer. Without one EF snapshots
the property by reference, so mutating a field on the existing instance is never
detected and SaveChanges writes nothing -- a test covers that specifically.
Update treats a missing authentication object as "leave unchanged", matching how
Name already behaves, so a caller that omits it cannot wipe a configured key.
Clearing a key means sending the object with a null inside it.
Vendor versions expose the parent vendor's settings as a read-only projection,
so consumers holding only a vendor version id -- as Data Acquisition will -- can
resolve the key in one call while writes stay on the vendor.
EpicAuth is untouched: it still derives {facilityId}-pem, so nothing changes at
runtime until the fallback rule between vendor and facility keys is settled.
Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
LEGLINK-743 deleted Normalization's VendorController when it moved the model to
Tenant, but left this service calling the old routes. Every vendor operation in
Admin.UI has been hitting a controller that no longer exists.
The API nests the signing key under authentication; the vendor screens work with
a flat secretId. Translating at the gateway keeps that difference out of the
components, so a second vendor-level setting only touches this file.
Create now carries the secret id. The add form has always shown the field, but
the create branch sent the name alone, so anything typed there was silently
discarded behind a success message. Both write paths build the same payload
before branching, which is what stops them drifting apart again.
authentication is always sent, including when the key is being cleared: the
Tenant manager reads an absent object as "leave unchanged", so omitting it would
make a clear no-op. Null inside the object is what removes the association.
The vendorEditEnabled flag is gone with it. It existed only to keep the edit
button hidden while no update endpoint existed, and PUT /api/vendor/{id} now
does, so the gate has nothing left to protect.
Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
The vendor signing-key design and the secret-id validation design and plan were working notes for this change, not reference material the repository needs to carry. They stay recoverable through history. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
The previous commit swept in five files that belong to a local development setup rather than to this change. Both appsettings.Development.json files in particular replaced the committed SQLEXPRESS defaults with a machine-specific SQL Server instance and a plaintext sa password, which would have become the checked-in default for everyone. Restores all five to their dev contents so the branch carries only the vendor signing key work. The AGENTS.md documentation, the check_health.sh compose fix and the Admin.UI Dockerfile npm layer-caching fix are worth landing, but each on its own terms rather than inside this ticket. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesVendor authentication settings
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
DotNet/ServiceTests/IntegrationTests/Tenant/VendorManagerTests.cs (1)
114-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an explicit-clear persistence test.
Set a signing key first. Then update the vendor with
Authentication = new VendorAuthenticationSettings { SigningKeySecretId = null }. Assert that the reloaded vendor has no signing key. This verifies the clear operation that this change introduces.🤖 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 `@DotNet/ServiceTests/IntegrationTests/Tenant/VendorManagerTests.cs` around lines 114 - 131, Extend UpdateVendor_WithoutAuthentication_LeavesTheConfiguredSecretIdIntact with a separate explicit-clear scenario: configure a signing key, update the vendor using VendorAuthenticationSettings with SigningKeySecretId set to null, reload it, and assert that the persisted signing key is absent.Web/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.html (1)
36-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeclare that the edit control is not a submit button.
Add
type="button"to thismat-icon-button. The control callsonEdit(vendor)and does not submit a form. Without the native attribute, HTML defaults the button tosubmitwhen an ancestor form exists.Proposed fix
<button mat-icon-button + type="button" color="primary"Based on learnings, Angular Material buttons do not automatically set
type="button"on the underlying<button>; non-submit actions should declare it explicitly.🤖 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 `@Web/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.html` around lines 36 - 45, Add type="button" to the edit mat-icon-button invoking onEdit(vendor), ensuring it remains a non-submit control when rendered inside an ancestor form.Source: Learnings
🤖 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 `@DotNet/Tenant/Controllers/VendorController.cs`:
- Around line 100-104: Validate every non-null Authentication.SigningKeySecretId
in VendorController before invoking CreateVendorAsync or UpdateVendorAsync,
rejecting empty, whitespace-only, oversized, and unsupported values with a 400
response containing validation details. Preserve null as a valid
explicit-clearing value, and add focused XUnit coverage for invalid
create/update input and null clearing; apply the changes at
DotNet/Tenant/Controllers/VendorController.cs lines 100-104 and 136-140.
---
Nitpick comments:
In `@DotNet/ServiceTests/IntegrationTests/Tenant/VendorManagerTests.cs`:
- Around line 114-131: Extend
UpdateVendor_WithoutAuthentication_LeavesTheConfiguredSecretIdIntact with a
separate explicit-clear scenario: configure a signing key, update the vendor
using VendorAuthenticationSettings with SigningKeySecretId set to null, reload
it, and assert that the persisted signing key is absent.
In
`@Web/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.html`:
- Around line 36-45: Add type="button" to the edit mat-icon-button invoking
onEdit(vendor), ensuring it remains a non-submit control when rendered inside an
ancestor form.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c57dd76-c16d-499a-86d6-cf8cd34dcd39
📒 Files selected for processing (29)
DotNet/ServiceTests/IntegrationTests/Tenant/VendorControllerTests.csDotNet/ServiceTests/IntegrationTests/Tenant/VendorManagerTests.csDotNet/ServiceTests/IntegrationTests/Tenant/VendorQueriesTests.csDotNet/ServiceTests/UnitTests/Tenant/VendorModelBindingTests.csDotNet/Shared/Application/Models/Tenant/CreateVendorModel.csDotNet/Shared/Application/Models/Tenant/UpdateVendorModel.csDotNet/Shared/Application/Models/Tenant/VendorAuthenticationSettings.csDotNet/Shared/Application/Models/Tenant/VendorModel.csDotNet/Shared/Application/Models/Tenant/VendorVersionModel.csDotNet/Tenant/Business/Managers/VendorManager.csDotNet/Tenant/Business/Queries/VendorQueries.csDotNet/Tenant/Controllers/VendorController.csDotNet/Tenant/Data/Entities/Vendor.csDotNet/Tenant/Data/Repository/TenantDbContext.csDotNet/Tenant/Migrations/20260804215404_AddVendorAuthentication.Designer.csDotNet/Tenant/Migrations/20260804215404_AddVendorAuthentication.csDotNet/Tenant/Migrations/TenantDbContextModelSnapshot.csWeb/Admin.UI/src/app/components/vendor/vendor-config-form/vendor-config-form.component.spec.tsWeb/Admin.UI/src/app/components/vendor/vendor-config-form/vendor-config-form.component.tsWeb/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.htmlWeb/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.spec.tsWeb/Admin.UI/src/app/components/vendor/vendor-dashboard/vendor-dashboard.component.tsWeb/Admin.UI/src/app/interfaces/tenant/vendor-interface.tsWeb/Admin.UI/src/app/interfaces/vendor/vendor-config-model.interface.tsWeb/Admin.UI/src/app/services/app-config.service.tsWeb/Admin.UI/src/app/services/gateway/vendor/vendor.service.spec.tsWeb/Admin.UI/src/app/services/gateway/vendor/vendor.service.tsWeb/Admin.UI/src/assets/app.config.jsondocs/superpowers/specs/2026-08-03-vendor-signing-key-secret-id-design.md
💤 Files with no reviewable changes (4)
- Web/Admin.UI/src/assets/app.config.json
- Web/Admin.UI/src/app/interfaces/vendor/vendor-config-model.interface.ts
- docs/superpowers/specs/2026-08-03-vendor-signing-key-secret-id-design.md
- Web/Admin.UI/src/app/services/app-config.service.ts
johnbritton
approved these changes
Aug 5, 2026
This was referenced Aug 5, 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
2 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.
🛠️ Description of Changes
Lets an administrator associate a vendor with the Azure Key Vault secret holding its PEM signing key
🧪 Testing Performed
Tested locally
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
New Features
Bug Fixes