feat: Epic FHIR integration with dual authentication#17
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR adds comprehensive FHIR authentication infrastructure including token acquisition strategies for CDS Hooks and JWT-based backend flows, a middleware to extract tokens from request payloads, an authenticated HTTP client provider, and extensive test coverage for all new components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CDS Hooks Client
participant Middleware as CdsHookTokenMiddleware
participant HttpContext as HttpContext.Items
participant Resolver as TokenStrategyResolver
participant Strategy as ITokenAcquisitionStrategy
participant Provider as FhirHttpClientProvider
participant Factory as IHttpClientFactory
participant Token as TokenEndpoint/Cache
Client->>Middleware: POST /cds-hooks/...
Middleware->>Middleware: ExtractAndStoreFhirTokenAsync()
Middleware->>HttpContext: Items["FhirAccessToken"] = token
Middleware->>Resolver: Resolve()
Resolver->>Strategy: Return appropriate strategy
Note over Strategy: CdsHookTokenStrategy or<br/>JwtBackendTokenStrategy
Provider->>Factory: CreateClient("EpicFhir")
Factory-->>Provider: HttpClient instance
Provider->>Resolver: Resolve()
Resolver-->>Provider: Strategy instance
Provider->>Strategy: AcquireTokenAsync()
alt CdsHookTokenStrategy
Strategy->>HttpContext: Get Items["FhirAccessToken"]
HttpContext-->>Strategy: token
else JwtBackendTokenStrategy
Strategy->>Strategy: Check cache for token
alt Token cached
Strategy-->>Provider: Return cached token
else Generate & exchange
Strategy->>Strategy: GenerateClientAssertion()
Strategy->>Token: POST JWT + grant_type
Token-->>Strategy: access_token
Strategy->>Strategy: Cache for 55 min
end
end
Strategy-->>Provider: access_token
Provider->>Provider: Set Authorization: Bearer {token}
Provider-->>Client: Authenticated HttpClient
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
issue: Missing guard clauses in nearly all service methods, e.g., |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API/Middleware/CdsHookTokenMiddleware.cs`:
- Around line 40-72: The ExtractAndStoreFhirTokenAsync method currently assumes
the JSON root is an object and calls doc.RootElement.TryGetProperty which will
throw for non-object roots; update the method to check doc.RootElement.ValueKind
== JsonValueKind.Object (or otherwise guard) before calling TryGetProperty on
the root element, and only attempt to access "fhirAuthorization" and
"access_token" when the root is an object—keep existing JsonException handling
and the context.Items["FhirAccessToken"] assignment/logging behavior.
In `@apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs`:
- Around line 85-101: The ExchangeJwtForTokenAsync method uses an unnamed
HttpClient with default timeout and assumes the response JSON contains
"access_token"; fix by using a named/typed client from _httpClientFactory
configured with an explicit timeout for the token exchange (or create a
CancellationTokenSource with a short timeout around the PostAsync call) and by
defensively parsing the JSON: after reading the JsonElement check
json.TryGetProperty("access_token") and return null or throw a clear,
descriptive exception if missing (include context like _options.TokenEndpoint
and the response body when logging), and wrap the HTTP call/parse in a try/catch
to convert transient/parse failures into meaningful exceptions.
- Around line 112-118: The aud claim is set to an empty string when
_options.TokenEndpoint is null, producing an invalid JWT; update
JwtBackendTokenStrategy to validate _options.TokenEndpoint early (e.g., in the
constructor or at the start of the method that builds the claims) and throw a
clear exception (ArgumentException/InvalidOperationException) if TokenEndpoint
is null or empty, then use the validated TokenEndpoint when creating the claims
array (the Claim with JwtRegisteredClaimNames.Aud) so the aud is never an empty
string.
- Around line 137-148: LoadPrivateKey currently calls RSA.Create() and returns
an RSA that is never disposed; change LoadPrivateKey to avoid leaking by
exporting the key parameters from the PEM-loaded RSA, disposing that temporary
RSA, and returning a RsaSecurityKey constructed from a new RSA instance
initialized with rsa.ExportParameters(true) (i.e., create tempRsa =
RSA.Create(); tempRsa.ImportFromPem(...); var parms =
tempRsa.ExportParameters(true); tempRsa.Dispose(); var rsaForKey = RSA.Create();
rsaForKey.ImportParameters(parms); return new RsaSecurityKey(rsaForKey)). This
keeps the final RSA owned where RsaSecurityKey expects it while ensuring the
initial PEM loader is disposed and prevents the per-cache-miss leak; update any
callers expecting RSA (e.g., code that called LoadPrivateKey and wrapped it in
RsaSecurityKey) to use the new RsaSecurityKey return or adapt accordingly.
🧹 Nitpick comments (16)
apps/gateway/Gateway.API.Tests/Contracts/Http/TokenStrategyResolverTests.cs (1)
6-22: Test doesn’t exercise the real resolver.
Right now it only verifies a mocked return value. Consider instantiating the concreteTokenStrategyResolverwith real strategies and asserting selection behavior (e.g., CDS Hook vs JWT).apps/gateway/Gateway.API/Services/Http/TokenStrategyResolver.cs (1)
10-24: Consider using interfaces instead of concrete strategy types.The constructor accepts
CdsHookTokenStrategyandJwtBackendTokenStrategydirectly rather thanITokenAcquisitionStrategy. This couples the resolver to specific implementations and makes isolated unit testing harder.If extending with additional strategies is anticipated, consider:
♻️ Interface-based injection
- private readonly CdsHookTokenStrategy _cdsStrategy; - private readonly JwtBackendTokenStrategy _jwtStrategy; + private readonly ITokenAcquisitionStrategy _cdsStrategy; + private readonly ITokenAcquisitionStrategy _jwtStrategy; public TokenStrategyResolver( - CdsHookTokenStrategy cdsStrategy, - JwtBackendTokenStrategy jwtStrategy) + [FromKeyedServices("CdsHook")] ITokenAcquisitionStrategy cdsStrategy, + [FromKeyedServices("JwtBackend")] ITokenAcquisitionStrategy jwtStrategy)That said, if these are the only two strategies and the current DI setup works, the pragmatic concrete approach is acceptable.
apps/gateway/Gateway.API/Services/Http/FhirHttpClientProvider.cs (1)
39-49: Method name "GetAuthenticatedClientAsync" is misleading—it may return a client without authentication.The provider intentionally returns an unauthenticated client when token acquisition fails, relying on callers to handle the resulting 401 responses. While error handling exists downstream (FhirHttpClient and EpicFhirContext both check for HttpStatusCode.Unauthorized), the deferred failure point obscures the intent.
Consider either: (1) renaming to clarify the behavior (e.g.,
GetClientAsync), (2) documenting that callers must handle potential 401s, or (3) throwing to fail fast if authentication is mandatory for all FHIR operations.apps/gateway/Gateway.API/Services/FhirDataAggregator.cs (1)
35-39: Consider adding a guard clause forpatientId.Per the coding guidelines favoring guard clauses first, validating
patientIdat the entry point would provide clearer error messages rather than propagating failures to individual FHIR calls.💡 Optional guard clause
public async Task<ClinicalBundle> AggregateClinicalDataAsync( string patientId, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(patientId); + _logger.LogInformation("Aggregating clinical data for patient {PatientId}", patientId);apps/gateway/Gateway.API.Tests/Services/Http/CdsHookTokenStrategyTests.cs (1)
47-65: Consider adding a test for whenFhirAccessTokenexists but is null or non-string.Edge case: what happens if middleware stores a non-string or null value under the key? This would help ensure the
as stringcast in the production code handles edge cases gracefully.💡 Optional edge case test
[Test] public async Task CdsHookTokenStrategy_AcquireTokenAsync_ReturnsNullWhenTokenIsNotString() { // Arrange var httpContext = new DefaultHttpContext(); httpContext.Items["FhirAccessToken"] = 12345; // Non-string value var httpContextAccessor = Substitute.For<IHttpContextAccessor>(); httpContextAccessor.HttpContext.Returns(httpContext); var strategy = new CdsHookTokenStrategy(httpContextAccessor); // Act var token = await strategy.AcquireTokenAsync(); // Assert await Assert.That(token).IsNull(); }apps/gateway/Gateway.API.Tests/Contracts/Http/FhirHttpClientProviderTests.cs (2)
6-42: These tests mock the interface rather than testing the implementation.Both tests substitute
IFhirHttpClientProviderand verify mock behavior. This tests NSubstitute's setup, notFhirHttpClientProvider's actual logic. Consider testing the real implementation with mocked dependencies (likeITokenStrategyResolverandIHttpClientFactory) to validate actual authentication flow.
13-13:HttpClientinstances should be disposed.
HttpClientimplementsIDisposable. While not critical in tests, proper disposal is good hygiene.💡 Dispose HttpClient in tests
[Test] public async Task IFhirHttpClientProvider_GetAuthenticatedClientAsync_ReturnsHttpClient() { // Arrange var provider = Substitute.For<IFhirHttpClientProvider>(); - var expectedClient = new HttpClient(); + using var expectedClient = new HttpClient(); provider.GetAuthenticatedClientAsync(Arg.Any<CancellationToken>()) .Returns(Task.FromResult(expectedClient));Also applies to: 31-31
apps/gateway/Gateway.API/Services/Http/CdsHookTokenStrategy.cs (2)
17-20: Add a guard clause for the constructor parameter.Per coding guidelines requiring guard clauses first, validate
httpContextAccessorto fail fast with a clear error.♻️ Proposed fix
public CdsHookTokenStrategy(IHttpContextAccessor httpContextAccessor) { - _httpContextAccessor = httpContextAccessor; + _httpContextAccessor = httpContextAccessor ?? throw new ArgumentNullException(nameof(httpContextAccessor)); }
23-24: Consider extracting the magic string to a constant.
"FhirAccessToken"is used in bothCanHandleandAcquireTokenAsync. A shared constant would reduce duplication and prevent typo-related bugs.💡 Extract constant
public sealed class CdsHookTokenStrategy : ITokenAcquisitionStrategy { + private const string FhirAccessTokenKey = "FhirAccessToken"; private readonly IHttpContextAccessor _httpContextAccessor; // ... public bool CanHandle => - _httpContextAccessor.HttpContext?.Items.ContainsKey("FhirAccessToken") == true; + _httpContextAccessor.HttpContext?.Items.ContainsKey(FhirAccessTokenKey) == true; public Task<string?> AcquireTokenAsync(CancellationToken ct = default) { - var token = _httpContextAccessor.HttpContext?.Items["FhirAccessToken"] as string; + var token = _httpContextAccessor.HttpContext?.Items[FhirAccessTokenKey] as string; return Task.FromResult(token); } }Also applies to: 29-29
apps/gateway/Gateway.API.Tests/Services/Http/FhirHttpClientProviderTests.cs (1)
27-44: HttpClient instances should be disposed in tests.The
HttpClientinstances created here (and in other tests at lines 50, 70, 90) are not disposed. While this typically works fine in short-lived test processes, it's cleaner to useusingstatements or dispose in teardown.♻️ Optional: Wrap in using statement
[Test] public async Task FhirHttpClientProvider_GetAuthenticatedClientAsync_UsesHttpClientFactory() { // Arrange - var expectedClient = new HttpClient(); + using var expectedClient = new HttpClient(); _httpClientFactory.CreateClient("EpicFhir").Returns(expectedClient);apps/gateway/Gateway.API/DependencyExtensions.cs (2)
68-71:ValidateDataAnnotations()has no effect without data annotation attributes.
EpicFhirOptionsuses C#requiredkeyword, not[Required]attributes.ValidateDataAnnotations()won't validate anything here. The manual check at lines 73-77 partially compensates, but only forFhirBaseUrl.Consider either:
- Adding
[Required]attributes toEpicFhirOptionsproperties, or- Using a custom validation delegate with
.Validate()♻️ Option: Use Validate() with delegate
services.AddOptions<EpicFhirOptions>() .Bind(configuration.GetSection(EpicFhirOptions.SectionName)) - .ValidateDataAnnotations() + .Validate(o => !string.IsNullOrWhiteSpace(o.FhirBaseUrl) && !string.IsNullOrWhiteSpace(o.ClientId), + "EpicFhirOptions requires FhirBaseUrl and ClientId") .ValidateOnStart(); - - var fhirBaseUrl = configuration["Epic:FhirBaseUrl"]; - if (string.IsNullOrWhiteSpace(fhirBaseUrl)) - { - throw new InvalidOperationException("Epic:FhirBaseUrl must be configured."); - }
107-122: Consider documenting the relationship betweenAddFhirClientsandAddEpicFhirServices.
AddFhirClientscallsAddEpicFhirServicesinternally. If both are called separately, some services get registered twice. While this works, a/// <remarks>noting thatAddFhirClientsincludesAddEpicFhirServiceswould prevent confusion.apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs (1)
144-147: Synchronous file I/O in async context.
File.ReadAllTextblocks a thread pool thread. ConsiderFile.ReadAllTextAsyncor loading the key once at startup. Given this only runs on cache miss (~every 55 min), impact is low but avoidable.♻️ Async alternative
- private RSA LoadPrivateKey() + private async Task<RSA> LoadPrivateKeyAsync() { if (string.IsNullOrEmpty(_options.PrivateKeyPath)) { throw new InvalidOperationException("PrivateKeyPath is not configured"); } var rsa = RSA.Create(); - var keyPem = File.ReadAllText(_options.PrivateKeyPath); + var keyPem = await File.ReadAllTextAsync(_options.PrivateKeyPath); rsa.ImportFromPem(keyPem); return rsa; }This would require
GenerateClientAssertionto become async as well.apps/gateway/Gateway.API.Tests/DependencyExtensionsTests.cs (1)
121-142: Consider disposingServiceProviderin tests.The
ServiceProviderbuilt at line 133 (and in other tests) is disposable. While typically fine in tests, addingusingor explicit disposal is cleaner practice.♻️ Optional: Use using declaration
// Act services.AddEpicFhirServices(configuration); - var provider = services.BuildServiceProvider(); + using var provider = services.BuildServiceProvider();apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs (1)
15-31: Class should besealedper coding guidelines.The coding guidelines specify "Sealed by default" for C# classes. Unless inheritance is explicitly needed, consider sealing
EpicFhirContext<TResource>.♻️ Proposed change
-public class EpicFhirContext<TResource> : IFhirContext<TResource> where TResource : class +public sealed class EpicFhirContext<TResource> : IFhirContext<TResource> where TResource : classAs per coding guidelines: "Sealed by default".
apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs (1)
15-15: Consider sealing the test class.The coding guidelines recommend "sealed by default." While this is less critical for test classes, marking it
sealedmaintains consistency with production code patterns.-public class JwtBackendTokenStrategyTests +public sealed class JwtBackendTokenStrategyTests
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/gateway/Gateway.API/Services/DocumentUploader.cs (2)
24-32: Add guard clauses to validate constructor parameters.Per coding guidelines requiring guard clauses first, constructor parameters should be validated. This was also flagged by the PR author.
🛡️ Proposed fix
public DocumentUploader( IFhirHttpClient fhirHttpClient, ILogger<DocumentUploader> logger, IOptions<DocumentOptions> options) { + ArgumentNullException.ThrowIfNull(fhirHttpClient); + ArgumentNullException.ThrowIfNull(logger); + ArgumentNullException.ThrowIfNull(options); _fhirHttpClient = fhirHttpClient; _logger = logger; _options = options.Value; }
35-48: Add guard clauses for method parameters.
pdfBytesandpatientIdare required for document upload to succeed; validate them early.🛡️ Proposed fix
public async Task<Result<string>> UploadDocumentAsync( byte[] pdfBytes, string patientId, string? encounterId, CancellationToken cancellationToken = default) { + ArgumentNullException.ThrowIfNull(pdfBytes); + ArgumentException.ThrowIfNullOrWhiteSpace(patientId); + _logger.LogInformation( "Uploading PA form. Size={Size} bytes", pdfBytes.Length);apps/gateway/Gateway.API/Services/FhirDataAggregator.cs (2)
24-32: Missing guard clauses for constructor parameters.Constructor dependencies should be validated. This aligns with the PR author's own observation about missing guard clauses.
Proposed fix
public FhirDataAggregator( IFhirClient fhirClient, IOptions<ClinicalQueryOptions> options, ILogger<FhirDataAggregator> logger) { + ArgumentNullException.ThrowIfNull(fhirClient); + ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(logger); _fhirClient = fhirClient; _options = options.Value; _logger = logger; }
35-37: Missing guard clause forpatientIdparameter.Public methods should validate input parameters early. An empty or null
patientIdwould propagate to multiple downstream calls before failing.Proposed fix
public async Task<ClinicalBundle> AggregateClinicalDataAsync( string patientId, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(patientId); _logger.LogInformation("Aggregating clinical data for patient {PatientId}", patientId);apps/gateway/Gateway.API/Services/Fhir/BaseFhirRepository.cs (2)
28-32: Missing guard clauses in constructor.As per coding guidelines, guard clauses should come first. The constructor doesn't validate
contextorlogger, which could lead toNullReferenceExceptionlater.🛡️ Proposed fix
protected BaseFhirRepository(IFhirContext<TResource> context, ILogger logger) { + ArgumentNullException.ThrowIfNull(context); + ArgumentNullException.ThrowIfNull(logger); Context = context; Logger = logger; }
80-83: Derived class constructor also missing guard clauses.
BaseFhirRepositoryWithDateRangeconstructor should validate parameters before passing to base.🛡️ Proposed fix
protected BaseFhirRepositoryWithDateRange(IFhirContext<TResource> context, ILogger logger) : base(context, logger) { + // Base constructor validates; nothing additional needed here }Actually, since the base constructor now validates, this is fine. However,
FindByPatientSinceAsyncshould validatepatientId:public virtual Task<Result<IReadOnlyList<TResource>>> FindByPatientSinceAsync( string patientId, DateOnly since, CancellationToken ct = default) { + ArgumentException.ThrowIfNullOrEmpty(patientId); var query = BuildQuery( ("patient", patientId), (DateFieldName, $"ge{since:yyyy-MM-dd}"));apps/gateway/Gateway.API/Services/FhirClient.cs (1)
29-121: Add guard clauses to validate patientId and documentId before building FHIR requests.Empty or whitespace-only IDs will be interpolated directly into URLs and query strings (e.g.,
Patient/orpatient=&clinical-status=active), creating malformed requests that fail at the HTTP layer unnecessarily. Add early validation in each public method to fail fast with clear error messages.🔧 Proposed guard clause pattern
public async Task<PatientInfo?> GetPatientAsync( string patientId, CancellationToken cancellationToken = default) { + if (string.IsNullOrWhiteSpace(patientId)) + { + throw new ArgumentException("Patient ID is required.", nameof(patientId)); + } var result = await _httpClient.ReadAsync("Patient", patientId, cancellationToken);Apply the same pattern to all public methods:
SearchConditionsAsync,SearchObservationsAsync,SearchProceduresAsync,SearchDocumentsAsync, andGetDocumentContentAsync.Per coding guidelines: "Control: Guard clauses first, early return, no arrow code".
🤖 Fix all issues with AI agents
In
`@apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs`:
- Around line 203-204: The test currently asserts
_httpClientFactory.DidNotReceive().CreateClient(Arg.Any<string>()) but
JwtBackendTokenStrategy uses the parameterless overload CreateClient() (see
JwtBackendTokenStrategy's CreateClient() usage), so update the verification to
assert the parameterless CreateClient() was not called (e.g.,
_httpClientFactory.DidNotReceive().CreateClient()) to correctly reflect the
implementation and ensure no HTTP client was created; locate the assertion in
JwtBackendTokenStrategyTests (symbol: _httpClientFactory and the CreateClient
call) and replace the overload check accordingly.
In `@apps/gateway/Gateway.API/Configuration/EpicAuthOptions.cs`:
- Around line 1-42: Delete the unused EpicAuthOptions class (remove the
EpicAuthOptions type declaration) since EpicFhirOptions is the active config
type; then update EpicFhirOptionsTests.cs to remove any tests or references that
reference EpicAuthOptions and adjust those tests to only construct/bind/assert
EpicFhirOptions (ensure binding still targets the "Epic" section and that test
assertions cover the same properties: FhirBaseUrl, ClientId, ClientSecret,
TokenEndpoint, PrivateKeyPath, SigningAlgorithm). Make sure no other files
reference EpicAuthOptions after deletion.
In `@apps/gateway/Gateway.API/Configuration/EpicFhirOptions.cs`:
- Around line 33-41: Add DataAnnotations to EpicFhirOptions: mark the
PrivateKeyPath property with [Required] and, if a FileExists validation
attribute is available in the project, add [FileExists] as well so
missing/invalid files fail startup; mark SigningAlgorithm with
[RegularExpression("^(RS256|RS384|RS512)$")] to allow only those values (remove
the silent fallback). Ensure the class imports the DataAnnotations namespace and
keep the existing default value for SigningAlgorithm if you still want it but
let validation catch invalid overrides.
In `@apps/gateway/Gateway.API/DependencyExtensions.cs`:
- Around line 65-71: Add guard clauses at the top of AddEpicFhirServices to
validate the incoming parameters: throw ArgumentNullException (or use
ArgumentNullException.ThrowIfNull) for both services and configuration before
any processing; update the method signature body (in AddEpicFhirServices) so the
null checks run first and then proceed to the existing options binding and
validation calls.
In `@apps/gateway/Gateway.API/Gateway.API.csproj`:
- Around line 32-34: Update the PackageReference for
System.IdentityModel.Tokens.Jwt in Gateway.API.csproj from Version="8.2.1" to
the latest stable (e.g., "8.15.0"); after changing the Version attribute on the
PackageReference element (System.IdentityModel.Tokens.Jwt) restore/rehydrate
packages and run unit/integration tests, then smoke-test token handling, logging
assertions (log sanitization changes), ConfigurationManager refresh behavior,
and any code using experimental validation APIs to ensure no runtime/behavioral
regressions.
In `@apps/gateway/Gateway.API/Middleware/CdsHookTokenMiddleware.cs`:
- Around line 18-22: Constructor CdsHookTokenMiddleware lacks null checks for
its dependencies; add guard clauses at the start of the
CdsHookTokenMiddleware(RequestDelegate next, ILogger<CdsHookTokenMiddleware>
logger) constructor to validate that next and logger are not null and throw
ArgumentNullException using nameof(next) and nameof(logger) if they are, then
assign to the fields _next and _logger as before.
In `@apps/gateway/Gateway.API/Services/Fhir/BaseFhirRepository.cs`:
- Around line 43-49: The FindByPatientAsync method lacks a guard for the
patientId parameter; add a validation at the top of
BaseFhirRepository.FindByPatientAsync to check for null, empty, or whitespace
(e.g., string.IsNullOrWhiteSpace(patientId)) and throw an
ArgumentException/ArgumentNullException with nameof(patientId) if invalid,
before constructing the query and calling Context.SearchAsync(query, ct).
- Around line 35-40: Add a guard clause at the start of GetByIdAsync to validate
the id parameter (check for null or empty/whitespace) and fail fast instead of
calling Context.ReadAsync with an invalid id; specifically, inside GetByIdAsync
perform a check like string.IsNullOrWhiteSpace(id) and throw an appropriate
exception (ArgumentNullException or ArgumentException with nameof(id)) when
invalid so callers receive a clear error before delegating to Context.ReadAsync.
In `@apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs`:
- Around line 26-31: The constructor EpicFhirContext(IFhirHttpClientProvider
clientProvider, ILogger<EpicFhirContext<TResource>> logger) lacks null-argument
guard clauses; add early-throw checks at the top of the constructor for
clientProvider and logger (throw ArgumentNullException with the parameter name)
before assigning to _clientProvider and _logger, then set _resourceType =
typeof(TResource).Name as before so guards come first and prevent
arrow-style/late-failure initialization.
In `@apps/gateway/Gateway.API/Services/Fhir/FhirHttpClient.cs`:
- Around line 25-29: Add guard clauses to the FhirHttpClient constructor to
validate inputs: check that clientProvider and logger are not null and throw
ArgumentNullException for each (e.g., in the FhirHttpClient constructor before
assigning _clientProvider and _logger) so the class fails fast on
misconfiguration; keep the checks at the top of the constructor and then assign
the fields.
In `@apps/gateway/Gateway.API/Services/FhirClient.cs`:
- Around line 167-224: The mappers currently generate synthetic IDs with
Guid.NewGuid() which hides invalid FHIR resources; update MapToCondition,
MapToObservation, MapToProcedure to return null when the "id" property is
missing or not a valid string (remove Guid.NewGuid() fallback) and change
MapToDocument signature to return DocumentInfo? and similarly return null if
"id" is absent; ensure you locate the null-check logic in
MapToCondition/MapToObservation/MapToProcedure (Id =
resource.TryGetProperty("id", out var id) ? ...) and in MapToDocument (docId =
...), replace those fallbacks with an early null return so downstream
null-handling is used consistently.
- Around line 123-165: Add a defensive check before calling EnumerateArray in
SearchAndMapAsync: verify that the JsonElement entries has ValueKind ==
JsonValueKind.Array and if not, log a warning (include resourceType and perhaps
entries.ValueKind) and return the empty results to avoid
InvalidOperationException; apply the identical guard to the analogous iteration
in EpicFhirContext (the same bundle/entry handling method) so both places
validate entries.ValueKind before EnumerateArray().
In `@apps/gateway/Gateway.API/Services/Http/CdsHookTokenStrategy.cs`:
- Around line 17-20: The CdsHookTokenStrategy constructor is missing a null
guard for the injected IHttpContextAccessor; update the
CdsHookTokenStrategy(IHttpContextAccessor httpContextAccessor) constructor to
validate httpContextAccessor and throw an ArgumentNullException (or use
ArgumentNullException.ThrowIfNull) before assigning to the _httpContextAccessor
field so the dependency is enforced at construction time.
In `@apps/gateway/Gateway.API/Services/Http/FhirHttpClientProvider.cs`:
- Around line 21-29: The constructor of FhirHttpClientProvider lacks guard
clauses for its dependencies; add ArgumentNullException.ThrowIfNull checks for
httpClientFactory, tokenResolver, and logger at the start of the
FhirHttpClientProvider constructor so the parameters IHttpClientFactory
httpClientFactory, ITokenStrategyResolver tokenResolver, and
ILogger<FhirHttpClientProvider> logger are validated before assignment to the
_httpClientFactory, _tokenResolver, and _logger fields.
In `@apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs`:
- Around line 33-43: Add null-check guard clauses to the JwtBackendTokenStrategy
constructor: validate httpClientFactory, options, tokenCache, and logger and
throw ArgumentNullException for any that are null before assigning to the
private fields. Use the parameter names (httpClientFactory, options, tokenCache,
logger) in the exceptions and dereference options.Value only after options is
validated.
In `@apps/gateway/Gateway.API/Services/Http/TokenStrategyResolver.cs`:
- Around line 18-24: The TokenStrategyResolver constructor lacks null checks for
its dependencies; add guard clauses at the top of the constructor to validate
that parameters cdsStrategy and jwtStrategy are not null and throw appropriate
ArgumentNullException (or similar) before assigning to _cdsStrategy and
_jwtStrategy so the constructor fails fast if dependencies are missing.
🧹 Nitpick comments (18)
apps/gateway/Gateway.API/Contracts/Fhir/IFhirRepository.cs (1)
9-45: Two public types in one file—consider splitting if strict adherence to guidelines is required.The coding guidelines specify "one public type per file," but
IFhirRepository<TResource>andIFhirRepositoryWithDateRange<TResource>are tightly coupled (inheritance relationship, same bounded context). Keeping them together is pragmatic and improves discoverability.If you want to align strictly with the guideline, extract
IFhirRepositoryWithDateRange<TResource>to its own file. Otherwise, this is a reasonable exception.As per coding guidelines:
apps/gateway/**/*.cs: SOLID: One public type per file.apps/gateway/Gateway.API/Models/AnalysisResponses.cs (1)
6-127: Consider splitting public records into separate files. This file hosts five public types, which makes navigation and ownership harder as the API surface grows.As per coding guidelines: SOLID: One public type per file.
apps/gateway/Gateway.API.Tests/Services/Http/TokenStrategyResolverTests.cs (1)
42-57: Consider whether async is needed.
TokenStrategyResolver.Resolve()is synchronous (returnsITokenAcquisitionStrategydirectly). The test method is markedasync Taskbut only awaits the TUnit assertion. While TUnit'sAssert.That()is awaitable, usingasyncfor a synchronous SUT operation is slightly misleading.♻️ Optional: Simplify to sync test if TUnit supports it
If TUnit supports synchronous assertions, consider:
[Test] -public async Task TokenStrategyResolver_Resolve_ReturnsCdsStrategyWhenTokenPresent() +public void TokenStrategyResolver_Resolve_ReturnsCdsStrategyWhenTokenPresent() { // ... var result = sut.Resolve(); - await Assert.That(result).IsSameReferenceAs(_cdsStrategy); + Assert.That(result).IsSameReferenceAs(_cdsStrategy); }Otherwise, the current approach works fine with TUnit's async assertions.
apps/gateway/Gateway.API.Tests/Services/Fhir/FhirHttpClientTests.cs (2)
19-28: Remove unused_mockHttpClientfield.The
_mockHttpClientfield is created in the constructor but never used—each test creates its ownHttpClientwith a specificMockHttpMessageHandler.♻️ Proposed fix
public class FhirHttpClientTests { private readonly IFhirHttpClientProvider _clientProvider; private readonly ILogger<FhirHttpClient> _logger; private readonly FhirHttpClient _sut; - private readonly HttpClient _mockHttpClient; public FhirHttpClientTests() { _clientProvider = Substitute.For<IFhirHttpClientProvider>(); _logger = Substitute.For<ILogger<FhirHttpClient>>(); - _mockHttpClient = CreateMockHttpClient(); _sut = new FhirHttpClient(_clientProvider, _logger); }Also remove the unused helper method at lines 202-205.
211-226: SealMockHttpMessageHandlerper coding guidelines.The coding guidelines specify "sealed by default" for types.
♻️ Proposed fix
-internal class MockHttpMessageHandler : HttpMessageHandler +internal sealed class MockHttpMessageHandler : HttpMessageHandlerapps/gateway/Gateway.API/Configuration/EpicAuthOptions.cs (1)
6-42: Consider usingrecordfor configuration DTOs.Per coding guidelines, records are preferred for DTOs. Since this is a pure data container with init-only properties:
♻️ Proposed change
-public sealed class EpicAuthOptions +public sealed record EpicAuthOptionsRecords provide value equality and concise syntax for immutable data.
apps/gateway/Gateway.API.Tests/Contracts/Http/TokenStrategyResolverTests.cs (1)
8-22: Test validates mock behavior, not actual resolver logic.This test mocks
ITokenStrategyResolveritself, so it only confirms that NSubstitute returns configured values. Consider testing the concreteTokenStrategyResolverclass to verify:
- Returns CDS strategy when
_cdsStrategy.CanHandleis true- Falls back to JWT strategy when CDS cannot handle
💡 Suggested approach
[Test] public async Task TokenStrategyResolver_Resolve_ReturnsCdsStrategyWhenCanHandle() { // Arrange var cdsStrategy = Substitute.For<ITokenAcquisitionStrategy>(); cdsStrategy.CanHandle.Returns(true); var jwtStrategy = Substitute.For<ITokenAcquisitionStrategy>(); var resolver = new TokenStrategyResolver(cdsStrategy, jwtStrategy); // Act var result = resolver.Resolve(); // Assert await Assert.That(result).IsSameReferenceAs(cdsStrategy); }apps/gateway/Gateway.API.Tests/Contracts/Http/TokenAcquisitionStrategyTests.cs (1)
6-36: Interface mock tests have limited value.These tests verify that NSubstitute can mock the interface, not the behavior of actual implementations. Given that
CdsHookTokenStrategyTestsandJwtBackendTokenStrategyTestsexist elsewhere in the PR, consider whether these mock-verification tests add meaningful coverage.If the intent is contract testing to ensure the interface shape is mockable, that's valid but worth documenting in the test class.
apps/gateway/Gateway.API/Services/Http/TokenStrategyResolver.cs (1)
10-11: Constructor depends on concrete strategy types instead of interfaces.Injecting
CdsHookTokenStrategyandJwtBackendTokenStrategydirectly couples this resolver to concrete implementations. UsingITokenAcquisitionStrategywould improve testability and align with DIP. However, since you need to distinguish them for priority logic, consider a marker interface or named registration pattern.Also applies to: 18-20
apps/gateway/Gateway.API.Tests/Contracts/Http/FhirHttpClientProviderTests.cs (3)
6-23: Tests verify mock contract, not implementation behavior.These tests mock
IFhirHttpClientProviderand verify the mock returns what it was configured to return. This validates the interface signature but doesn't exerciseFhirHttpClientProvider's actual logic (token acquisition, client configuration).Consider adding tests for the concrete
FhirHttpClientProviderclass that verify:
- Token strategy resolution and application
- HttpClient configuration (base address, headers)
- Error handling when token acquisition fails
13-13:HttpClientis not disposed.
HttpClientimplementsIDisposable. While the impact is minimal in tests, disposing resources is good hygiene.♻️ Suggested fix
[Test] public async Task IFhirHttpClientProvider_GetAuthenticatedClientAsync_ReturnsHttpClient() { // Arrange var provider = Substitute.For<IFhirHttpClientProvider>(); - var expectedClient = new HttpClient(); + using var expectedClient = new HttpClient(); provider.GetAuthenticatedClientAsync(Arg.Any<CancellationToken>()) .Returns(Task.FromResult(expectedClient));
30-31: Disposable resources not disposed.Both
CancellationTokenSourceandHttpClientshould be disposed.♻️ Suggested fix
// Arrange var provider = Substitute.For<IFhirHttpClientProvider>(); - var cts = new CancellationTokenSource(); - var expectedClient = new HttpClient(); + using var cts = new CancellationTokenSource(); + using var expectedClient = new HttpClient();apps/gateway/Gateway.API/Services/Http/CdsHookTokenStrategy.cs (1)
23-24: Magic string"FhirAccessToken"duplicated across files.This key is used here and in
CdsHookTokenMiddleware. Consider extracting to a shared constant to prevent typo-induced bugs.♻️ Example
// In a shared location, e.g., Gateway.API/Constants/HttpContextKeys.cs public static class HttpContextKeys { public const string FhirAccessToken = "FhirAccessToken"; }Also applies to: 29-29
apps/gateway/Gateway.API.Tests/DependencyExtensionsTests.cs (1)
48-48:ServiceProviderinstances not disposed.
BuildServiceProvider()returns anIDisposable. Not disposing can lead to resource leaks, especially with repeated test runs. This pattern repeats across all tests in this file.♻️ Suggested pattern
// Act services.AddEpicFhirServices(configuration); - var provider = services.BuildServiceProvider(); + using var provider = services.BuildServiceProvider(); // AssertApply this
usingpattern to allBuildServiceProvider()calls in this file.apps/gateway/Gateway.API/DependencyExtensions.cs (1)
107-111: Consider idempotency forAddEpicFhirServices.
AddFhirClientsunconditionally callsAddEpicFhirServices. If a consumer calls both methods, or callsAddEpicFhirServicesbeforeAddFhirClients, the strategies and resolver will be registered multiple times. Consider usingTryAdd*variants or a guard pattern to ensure idempotent registration.♻️ One approach using TryAddScoped
- // Token strategies (concrete types needed for TokenStrategyResolver) - services.AddScoped<CdsHookTokenStrategy>(); - services.AddScoped<JwtBackendTokenStrategy>(); - - // Token strategy resolver - services.AddScoped<ITokenStrategyResolver, TokenStrategyResolver>(); - - // FHIR HTTP client provider - services.AddScoped<IFhirHttpClientProvider, FhirHttpClientProvider>(); + // Token strategies (concrete types needed for TokenStrategyResolver) + services.TryAddScoped<CdsHookTokenStrategy>(); + services.TryAddScoped<JwtBackendTokenStrategy>(); + + // Token strategy resolver + services.TryAddScoped<ITokenStrategyResolver, TokenStrategyResolver>(); + + // FHIR HTTP client provider + services.TryAddScoped<IFhirHttpClientProvider, FhirHttpClientProvider>();apps/gateway/Gateway.API/Services/Fhir/JsonElementExtensions.cs (1)
62-65: Empty string handling in given name concatenation.When
GetString()returns null, it's replaced with an empty string, which afterstring.Join(" ", ...)could produce trailing/leading spaces or double spaces (e.g.,"John "if there's a null in the array). Consider filtering nulls/empty values:♻️ Filter empty values
var givenNames = given.EnumerateArray() - .Select(g => g.GetString() ?? "") + .Select(g => g.GetString()) + .Where(g => !string.IsNullOrEmpty(g)) .ToList(); - return string.Join(" ", givenNames); + return givenNames.Count > 0 ? string.Join(" ", givenNames) : null;apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs (1)
97-102: AddJsonExceptionhandling for consistency withFhirHttpClient.
ReadFromJsonAsynccan throwJsonExceptionif the response body is malformed JSON.FhirHttpClientcatches this, butEpicFhirContextmethods do not. Consider adding consistent error handling.♻️ Add JsonException catch block
catch (HttpRequestException ex) { _logger.LogError(ex, "Network error searching {ResourceType}", _resourceType); return Result<IReadOnlyList<TResource>>.Failure(FhirError.Network(ex.Message, ex)); } + catch (JsonException ex) + { + _logger.LogError(ex, "Invalid JSON response searching {ResourceType}", _resourceType); + return Result<IReadOnlyList<TResource>>.Failure( + FhirError.Validation($"Invalid JSON response: {ex.Message}")); + }apps/gateway/Gateway.API.Tests/Middleware/CdsHookTokenMiddlewareTests.cs (1)
14-21: Consider resetting_nextCalledbefore each test.The
_nextCalledfield is set in the constructor, which runs once per test class instance. If the test framework reuses instances, tests could interfere. Adding a setup method or resetting in each test would be safer.♻️ Add explicit reset
+ [Before(Test)] + public void Setup() + { + _nextCalled = false; + }Or reset at the start of
CreateNextDelegate():private RequestDelegate CreateNextDelegate() { + _nextCalled = false; return _ => { _nextCalled = true; return Task.CompletedTask; }; }
apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs
Show resolved
Hide resolved
apps/gateway/Gateway.API/Services/Http/TokenStrategyResolver.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/Gateway.API/Services/FhirClient.cs (1)
29-52: Add guard clauses for string ID parameters before HTTP delegation.Methods
GetPatientAsync(line 29),GetDocumentContentAsync(line 106), and allSearch*methods (lines 56–102) accept string IDs without null/empty validation. A null or whitespacepatientId/documentIdwill produce malformed HTTP requests likePatient/orBinary/. Add guard clauses to validate inputs early and provide clear logging before delegating toIFhirHttpClient.🔧 Suggested fix
public async Task<PatientInfo?> GetPatientAsync( string patientId, CancellationToken cancellationToken = default) { + if (string.IsNullOrWhiteSpace(patientId)) + { + _logger.LogWarning("PatientId is required."); + return null; + } var result = await _httpClient.ReadAsync("Patient", patientId, cancellationToken);Per coding guidelines: Control flow should use guard clauses first with early return.
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs`:
- Around line 24-31: The EpicFhirContext constructor lacks null-guard checks for
its DI parameters; add guard clauses at the start of the
EpicFhirContext(IFhirHttpClientProvider clientProvider,
ILogger<EpicFhirContext<TResource>> logger) constructor to throw
ArgumentNullException when clientProvider or logger is null (before assigning to
_clientProvider, _logger or setting _resourceType), ensuring the class fails
fast on misconfigured dependencies.
- Around line 74-85: Add a guard clause at the start of SearchAsync to validate
the query parameter: if string.IsNullOrWhiteSpace(query) then immediately fail
fast (e.g., throw an ArgumentException or return a failed
Result<IReadOnlyList<TResource>> with a clear message like "query cannot be
empty"), so you do not construct the HttpRequestMessage or call
_clientProvider/GetAuthenticatedClientAsync and do not call the FHIR endpoint
for empty queries; update SearchAsync to reference the query parameter and keep
the rest of the method (request construction using _resourceType) unchanged.
- Around line 34-42: In ReadAsync (EpicFhirContext.ReadAsync) add an early guard
clause that validates the id parameter (e.g., if string.IsNullOrWhiteSpace(id)
throw new ArgumentException("id cannot be null or empty", nameof(id))); do this
before calling _clientProvider.GetAuthenticatedClientAsync or building the
request URL (before using $"{_resourceType}/{id}") so you avoid constructing a
malformed URL and a needless network call.
- Around line 112-123: Add a null-guard at the start of CreateAsync to reject
null TResource inputs before any serialization or HTTP calls: inside CreateAsync
(before calling _clientProvider.GetAuthenticatedClientAsync and before
JsonContent.Create(resource)) check if resource is null and return a failed
Result<TResource> (using the project’s standard failure construction) with an
appropriate error message; this prevents sending null payloads and follows the
guard-clause pattern.
In `@apps/gateway/Gateway.API/Services/FhirClient.cs`:
- Around line 56-65: Add a guard clause to each public search method
(SearchConditionsAsync, SearchObservationsAsync, SearchProceduresAsync,
SearchDocumentsAsync) that checks string.IsNullOrWhiteSpace(patientId) before
composing the query; if the patientId is blank return early with an empty list
wrapped in a completed Task (e.g., Task.FromResult(new List<...>())) matching
the method's return type, otherwise continue to call SearchAndMapAsync as
before.
- Around line 106-119: Add a guard clause at the start of
GetDocumentContentAsync to validate documentId (use string.IsNullOrWhiteSpace)
and avoid calling _httpClient.ReadBinaryAsync with an empty identifier; if
invalid, log a warning with the documentId value and return null (consistent
with the existing null-return pattern and similar methods like GetPatientAsync)
so the HTTP client is never invoked with bad input.
In `@apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs`:
- Around line 33-43: Add guard clauses at the top of the JwtBackendTokenStrategy
constructor to validate incoming dependencies: check that httpClientFactory,
options, options.Value, tokenCache, and logger are not null and throw
ArgumentNullException with the appropriate parameter name if any are null;
perform these checks before assigning to fields (_httpClientFactory, _options,
_tokenCache, _logger) so the constructor fails fast with clear messages.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs`:
- Around line 114-130: In GenerateClientAssertion validate _options.ClientId
before building claims and throw an InvalidOperationException with a clear
message if it's null or empty; specifically check _options.ClientId used for
JwtRegisteredClaimNames.Iss and JwtRegisteredClaimNames.Sub and fail fast
(similar to the existing TokenEndpoint check) so you don't construct an invalid
JWT for downstream consumers.
🧹 Nitpick comments (2)
apps/gateway/Gateway.API.Tests/Middleware/CdsHookTokenMiddlewareTests.cs (2)
32-79: Tests cover the happy path well, though there's some overlap.
ExtractsTokenFromFhirAuthorizationandSetsHttpContextItemtest nearly identical scenarios. Consider consolidating or differentiating them more clearly (e.g., one could test a minimal payload, the other a full CDS Hook structure).
12-21: Consider adding a test for empty or whitespace-only access_token.The middleware checks
!string.IsNullOrEmpty(token)before storing, but there's no test verifying this behavior. A test with"access_token": ""or"access_token": " "would confirm tokens aren't stored when empty/whitespace.🧪 Example test
[Test] public async Task CdsHookTokenMiddleware_InvokeAsync_SkipsEmptyAccessToken() { // Arrange var context = CreateHttpContext("/cds-hooks/test", "POST", """ { "hook": "order-select", "fhirAuthorization": { "access_token": "" } } """); var sut = new CdsHookTokenMiddleware(CreateNextDelegate(), _logger); // Act await sut.InvokeAsync(context); // Assert await Assert.That(context.Items.ContainsKey("FhirAccessToken")).IsFalse(); await Assert.That(_nextCalled).IsTrue(); }
Fixes: - P2 Major (3 issues): - RSA key disposal (already fixed in prior commit) - Missing guard clauses in TokenStrategyResolver - Remove synthetic ID generation in FhirClient mappers - P3 Minor (8 issues): - Guard clauses for JwtBackendTokenStrategy + ClientId validation - Guard clauses for CdsHookTokenMiddleware + JSON object check - Guard clauses for EpicFhirContext (constructor, ReadAsync, SearchAsync, CreateAsync) - Guard clauses for FhirHttpClient and FhirHttpClientProvider - Guard clauses for CdsHookTokenStrategy - Guard clauses for BaseFhirRepository (GetByIdAsync, FindByPatientAsync) - Guard clauses for FhirClient methods (GetPatientAsync, search methods, GetDocumentContentAsync) - Guard clauses for DependencyExtensions methods Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/Gateway.API/Services/FhirClient.cs (1)
22-26:⚠️ Potential issue | 🟡 MinorMissing guard clauses in constructor.
Per coding guidelines, constructor parameters should be validated before assignment.
🛡️ Proposed fix
public FhirClient(IFhirHttpClient httpClient, ILogger<FhirClient> logger) { + ArgumentNullException.ThrowIfNull(httpClient); + ArgumentNullException.ThrowIfNull(logger); + _httpClient = httpClient; _logger = logger; }
🧹 Nitpick comments (3)
apps/gateway/Gateway.API/Services/Http/JwtBackendTokenStrategy.cs (1)
159-170: Consider wrapping file read in try-catch for clearer error messaging.
File.ReadAllTextthrowsFileNotFoundExceptionorIOExceptionif the key file is missing or inaccessible. The current exception propagates up but the error message won't indicate which configuration option is at fault.💡 Optional improvement for better diagnostics
private RSA LoadPrivateKey() { if (string.IsNullOrEmpty(_options.PrivateKeyPath)) { throw new InvalidOperationException("PrivateKeyPath is not configured"); } + if (!File.Exists(_options.PrivateKeyPath)) + { + throw new InvalidOperationException($"Private key file not found: {_options.PrivateKeyPath}"); + } + var rsa = RSA.Create(); - var keyPem = File.ReadAllText(_options.PrivateKeyPath); - rsa.ImportFromPem(keyPem); + try + { + var keyPem = File.ReadAllText(_options.PrivateKeyPath); + rsa.ImportFromPem(keyPem); + } + catch (Exception ex) when (ex is IOException or CryptographicException) + { + rsa.Dispose(); + throw new InvalidOperationException($"Failed to load private key from {_options.PrivateKeyPath}", ex); + } return rsa; }apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs (1)
169-198: Add defensive check forentriesarray type.
EnumerateArray()throwsInvalidOperationExceptionifentriesis not a JSON array. Malformed FHIR responses could haveentryas a non-array value.🛡️ Proposed defensive check
private IReadOnlyList<TResource> ExtractResourcesFromBundle(JsonElement bundle) { var results = new List<TResource>(); - if (!bundle.TryGetProperty("entry", out var entries)) + if (!bundle.TryGetProperty("entry", out var entries) || + entries.ValueKind != JsonValueKind.Array) { return results; }apps/gateway/Gateway.API/Services/FhirClient.cs (1)
163-176: Add defensive check forentriesarray type.Same issue as in
EpicFhirContext:EnumerateArray()throws ifentriesis not a JSON array. Malformed FHIR responses could cause runtime exceptions.🛡️ Proposed fix
var json = result.Value!; - if (!json.TryGetProperty("entry", out var entries)) return results; + if (!json.TryGetProperty("entry", out var entries) || + entries.ValueKind != JsonValueKind.Array) + { + return results; + }
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)
apps/gateway/Gateway.API/Services/Fhir/FhirHttpClient.cs (1)
34-167:⚠️ Potential issue | 🟡 MinorAdd guard clauses for string parameters before building URLs and payloads.
This aligns with the guard-clause-first guideline and follows the validation pattern established in the constructor. Apply toReadAsync,SearchAsync,CreateAsync, andReadBinaryAsync.🔧 Suggested fix (example for ReadAsync)
public async Task<Result<JsonElement>> ReadAsync( string resourceType, string id, CancellationToken ct = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(resourceType); + ArgumentException.ThrowIfNullOrWhiteSpace(id); + try { var httpClient = await _clientProvider.GetAuthenticatedClientAsync(ct);
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API/Services/Fhir/EpicFhirContext.cs`:
- Around line 111-115: The POST request is sending JSON with the default
Content-Type application/json, which violates FHIR R4; update the request
creation in EpicFhirContext (the block that gets the HttpClient via
_clientProvider.GetAuthenticatedClientAsync and calls ConfigureRequest) to set
the request.Content's Content-Type to "application/fhir+json" after assigning
JsonContent.Create(resource) (e.g., request.Content.Headers.ContentType = new
MediaTypeHeaderValue("application/fhir+json")); ensure this change is applied
where request.Content = JsonContent.Create(resource) is done and that
ConfigureRequest remains responsible only for Accept headers.
Fixes: - Major: Set FHIR Content-Type header for POST requests in EpicFhirContext - Major: Update System.IdentityModel.Tokens.Jwt to 8.15.0 - Minor: Add JsonException handling in EpicFhirContext - Minor: Add guard clauses to FhirClient constructor - Minor: Delete unused EpicAuthOptions class - Minor: Fix CreateClient test verification overload Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs`:
- Around line 126-154: The test assigns null to a non-nullable
EpicFhirOptions.ClientId causing a static-analysis warning; to keep the intended
null-behavior for the
JwtBackendTokenStrategy_GenerateClientAssertion_ThrowsWhenClientIdIsNull test,
update the test to use the null-forgiving operator when setting ClientId (e.g.,
assign null! to ClientId) so the compile-time warning is suppressed, then run
the test flow through CreateSut and call GenerateClientAssertion as before.
🧹 Nitpick comments (2)
apps/gateway/Gateway.API.Tests/Configuration/EpicFhirOptionsTests.cs (1)
44-70: Consider using a non-defaultSigningAlgorithmvalue to strengthen test coverage.The test sets
SigningAlgorithmto"RS384", which is already the default value. To actually verify that explicit configuration values bind correctly (rather than just confirming the default), consider using a different valid algorithm like"RS256"or"RS512".💡 Suggested improvement
["Epic:FhirBaseUrl"] = "https://fhir.epic.com/api/FHIR/R4", ["Epic:ClientId"] = "test-client-id", ["Epic:PrivateKeyPath"] = "/path/to/private.key", - ["Epic:SigningAlgorithm"] = "RS384" + ["Epic:SigningAlgorithm"] = "RS512" }) .Build();And update the assertion:
- await Assert.That(options.SigningAlgorithm).IsEqualTo("RS384"); + await Assert.That(options.SigningAlgorithm).IsEqualTo("RS512");apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs (1)
329-443: Reduce duplication across token exchange tests.The four
AcquireTokenAsynctests (lines 329-485) share nearly identical setup: creatingMockHttpMessageHandler,HttpClient, mockIHttpClientFactory,MemoryCache,EpicFhirOptions, and the SUT. Consider extracting a helper method:private (JwtBackendTokenStrategy Sut, MemoryCache Cache, MockHttpMessageHandler Handler) CreateSutWithHttpMock(string responseContent, HttpStatusCode statusCode) { var mockHandler = new MockHttpMessageHandler(responseContent, statusCode); var httpClient = new HttpClient(mockHandler); var httpClientFactory = Substitute.For<IHttpClientFactory>(); httpClientFactory.CreateClient(Arg.Any<string>()).Returns(httpClient); var cache = new MemoryCache(new MemoryCacheOptions()); var options = new EpicFhirOptions { FhirBaseUrl = "https://fhir.epic.com/api/FHIR/R4", ClientId = "test-client-id", TokenEndpoint = "https://fhir.epic.com/oauth2/token", PrivateKeyPath = _testKeyPath, SigningAlgorithm = "RS384" }; var sut = new JwtBackendTokenStrategy( httpClientFactory, Options.Create(options), cache, _logger); return (sut, cache, mockHandler); }This would reduce each test to ~10 lines and improve maintainability.
apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs
Outdated
Show resolved
Hide resolved
|
@coderabbitai If there are no remaining unaddressed critical/major issues, approve this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5850 🏁 Script executed: Length of output: 3601 🏁 Script executed: Length of output: 2686 🏁 Script executed: Length of output: 2343 🏁 Script executed: Length of output: 2756 🏁 Script executed: Length of output: 3694
🔴 Critical Issue: Missing Middleware RegistrationThe Required fix in app.UseCors();
app.UseMiddleware<CdsHookTokenMiddleware>(); // Add this line
app.UseHealthChecks("/health");🟠 Major Issue: Incomplete Guard Clause CoverageWhile new HTTP services have proper
public DocumentUploader(
IFhirHttpClient fhirHttpClient,
ILogger<DocumentUploader> logger,
IOptions<DocumentOptions> options)
{
ArgumentNullException.ThrowIfNull(fhirHttpClient);
ArgumentNullException.ThrowIfNull(logger);
ArgumentNullException.ThrowIfNull(options);
_fhirHttpClient = fhirHttpClient;
_logger = logger;
_options = options.Value;
}
public FhirDataAggregator(
IFhirClient fhirClient,
IOptions<ClinicalQueryOptions> options,
ILogger<FhirDataAggregator> logger)
{
ArgumentNullException.ThrowIfNull(fhirClient);
ArgumentNullException.ThrowIfNull(options);
ArgumentNullException.ThrowIfNull(logger);
_fhirClient = fhirClient;
_options = options.Value;
_logger = logger;
}✅ What's Working Well
I cannot approve until these issues are resolved, as they directly address your original concern about guard clauses and would cause the CDS Hook authentication flow to fail. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/Gateway.API/Services/DocumentUploader.cs (1)
39-52:⚠️ Potential issue | 🟡 MinorAdd method-level guard clauses for inputs.
pdfBytesandpatientIdare used immediately; null/empty values can cause runtime errors or invalid FHIR references. Place guards at the top of the method.✅ Proposed fix
public async Task<Result<string>> UploadDocumentAsync( byte[] pdfBytes, string patientId, string? encounterId, CancellationToken cancellationToken = default) { + ArgumentNullException.ThrowIfNull(pdfBytes); + if (string.IsNullOrWhiteSpace(patientId)) + { + throw new ArgumentException("Patient ID is required.", nameof(patientId)); + } + _logger.LogInformation( "Uploading PA form. Size={Size} bytes", pdfBytes.Length);As per coding guidelines: Guard clauses first, early return, no arrow code.
🧹 Nitpick comments (2)
apps/gateway/Gateway.API/Services/FhirDataAggregator.cs (1)
39-55: Add guard clause forpatientIdparameter.The method logs and makes multiple FHIR API calls using
patientIdwithout validating it first. While the downstreamIFhirClientmethods now have their own guards, defense-in-depth and consistent error reporting at the aggregation layer would be beneficial.🛡️ Proposed fix
public async Task<ClinicalBundle> AggregateClinicalDataAsync( string patientId, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(patientId); + _logger.LogInformation("Aggregating clinical data for patient {PatientId}", patientId);As per coding guidelines: "Guard clauses first, early return, no arrow code"
apps/gateway/Gateway.API.Tests/Services/Http/JwtBackendTokenStrategyTests.cs (1)
67-94: Use TUnit's fluent exception assertion pattern to simplify repetitive try-catch blocks.The manual try-catch is verbose and repeats identically across four tests. TUnit's
Assert.That(() => …).Throws<T>()pattern is cleaner and more idiomatic.♻️ Cleaner exception assertion pattern
await Assert.That(() => sut.GenerateClientAssertion()) .Throws<InvalidOperationException>() .WithMessage("TokenEndpoint is not configured for JWT generation");Consider consolidating the four exception tests (TokenEndpointIsNull, TokenEndpointIsEmpty, ClientIdIsNull, ClientIdIsEmpty) into a single parameterized test to further reduce duplication.
|
@coderabbitai If there are no remaining unaddressed critical/major issues, approve this PR. |
🧩 Analysis chain🏁 Script executed: Length of output: 3058
✅ Issues Resolved1. Critical: Middleware Registration ✅ 2. Major: Guard Clauses ✅ 🎯 PR Quality Summary
No remaining critical or major issues found. The PR implements a robust Epic FHIR integration with proper error handling, security practices, and test coverage. ✅ Actions performedComments resolved and changes approved. |
- Add JwtBackendTokenStrategy for backend JWT authentication - Add CdsHookTokenStrategy for extracting tokens from CDS Hook requests - Add FhirHttpClientProvider for authenticated FHIR API access - Add CdsHookTokenMiddleware for request token extraction - Add ITokenStrategyResolver interface for strategy selection - Add AddEpicFhirServices DI registration method - Add comprehensive tests for all new components Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
rsalus
force-pushed
the
feature/epic-integration
branch
from
27e0c49 to
ce27023
Compare
1 participant
Summary
Implements dual OAuth authentication for Epic FHIR R4 API with strategy pattern. CDS Hook requests use the provided
fhirAuthorization.access_token, while backend services use JWT-based client credentials flow with token caching.Changes
CdsHookTokenStrategyextracts tokens from CDS Hook context;JwtBackendTokenStrategyhandles JWT generation with RS384 signing and 55-minute token cachingTokenStrategyResolverselects strategy based on request context (CDS Hook priority, JWT fallback)FhirHttpClientProviderattaches Bearer tokens to authenticated HttpClient instancesaccessTokenparameter from all IFhirHttpClient methods; auth handled internallyCdsHookTokenMiddlewareextracts FHIR tokens from CDS Hook request payloadsTest Plan
Unit tests cover all new components with 46 new tests. Integration verified with full test suite.
Results: Tests 102 ✓ · Build 0 errors
Design: docs/designs/2026-01-28-epic-fhir-integration.md