From 941879327f4c5eb99ef487d92c86979431ef00c8 Mon Sep 17 00:00:00 2001 From: William Yochum Date: Thu, 24 Jun 2021 09:17:50 -0700 Subject: [PATCH 1/4] validate patient id is a relative reference --- ...R4DeviceAndPatientLookupIdentityService.cs | 12 ++++++++- ...iceAndPatientLookupIdentityServiceTests.cs | 26 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs index b15a7b13..d780d734 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs @@ -3,6 +3,8 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- +using System; +using System.Text.RegularExpressions; using System.Threading.Tasks; using EnsureThat; using Hl7.Fhir.Rest; @@ -41,7 +43,15 @@ protected static string GetPatientIdFromDevice(Model.Device device) { EnsureArg.IsNotNull(device, nameof(device)); - return device.Patient?.GetId() ?? throw new FhirResourceNotFoundException(ResourceType.Patient); + var patientId = device.Patient?.GetId() ?? throw new FhirResourceNotFoundException(ResourceType.Patient); + + // only allow unreserved URI characters + if (!Regex.IsMatch(patientId, @"^[A-Za-z0-9_.\-~]+$")) + { + throw new NotSupportedException("Unsupported characters found in patient id"); + } + + return patientId; } protected async override Task<(string DeviceId, string PatientId)> LookUpDeviceAndPatientIdAsync(string value, string system = null) diff --git a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs index db64abff..6752576f 100644 --- a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs +++ b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs @@ -3,6 +3,7 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- +using System; using System.Threading.Tasks; using Hl7.Fhir.Rest; using Microsoft.Health.Extensions.Fhir.Service; @@ -100,5 +101,30 @@ public async void GivenDeviceWithNotPatientReference_WhenResolveResourceIdentiti await resourceService.Received(1).GetResourceByIdentityAsync(fhirClient, "deviceId", null); } + + [Fact] + public async void GivenDeviceWithPatientReferenceUnsupportedCharacters_WhenResolveResourceIdentitiesAsync_ThenNotSupportedExceptionThrown_Test() + { + var fhirClient = Utilities.CreateMockFhirClient(); + var resourceService = Substitute.For(); + var device = new Model.Device + { + Id = "1", + Patient = new Model.ResourceReference("Patient/&containsURIrestrictedchars"), + }; + + var mg = Substitute.For(); + mg.DeviceId.Returns("deviceId"); + + resourceService.GetResourceByIdentityAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(device)); + + using (var idSrv = new R4DeviceAndPatientLookupIdentityService(fhirClient, resourceService)) + { + var ex = await Assert.ThrowsAsync(async () => await idSrv.ResolveResourceIdentitiesAsync(mg)); + } + + await resourceService.Received(1).GetResourceByIdentityAsync(fhirClient, "deviceId", null); + } } } From 0073b6712336c65c4ab919df95f1d1a9a7d54df1 Mon Sep 17 00:00:00 2001 From: William Yochum Date: Tue, 29 Jun 2021 14:34:38 -0700 Subject: [PATCH 2/4] address pr feedback --- .../ModelExtensions.cs | 30 ++++++++++++++++--- ...R4DeviceAndPatientLookupIdentityService.cs | 10 +------ ...iceAndPatientLookupIdentityServiceTests.cs | 6 ++-- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs index dd7c5e2f..1a5ac62d 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs @@ -3,12 +3,15 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- -using System; +using System.Collections.Concurrent; +using System.Text.RegularExpressions; namespace Microsoft.Health.Extensions.Fhir { public static class ModelExtensions { + private static ConcurrentDictionary _idMatcherRegexCache = new ConcurrentDictionary(); + public static Hl7.Fhir.Model.ResourceReference ToReference(this TResource resource) where TResource : Hl7.Fhir.Model.Resource { @@ -39,9 +42,28 @@ public static Hl7.Fhir.Model.ResourceReference ToReference(this strin /// The id for the specified resource type if it exists, null otherwise. public static string GetId(this Hl7.Fhir.Model.ResourceReference reference) { - var referenceType = $"{typeof(TResource).Name}/"; - return reference?.Reference == null || !reference.Reference.StartsWith(referenceType, StringComparison.InvariantCultureIgnoreCase) - ? null : reference.Reference.Substring(referenceType.Length); + string id; + var referenceType = typeof(TResource).Name; + + if (reference?.Reference == null) + { + return null; + } + + var idMatcherRegex = _idMatcherRegexCache.GetOrAdd(referenceType, new Regex(referenceType + @"\/([A-Za-z0-9_.\-~#]{1,64})", RegexOptions.Compiled)); + + // Reference should be in the form: ResourceType/Identifier + // If there is a match, the 2nd group will contain the identifier. + var matches = idMatcherRegex.Match(reference.Reference); + if (matches?.Groups?.Count != 2) + { + return null; + } + else + { + id = matches?.Groups?[1].Value; + return id; + } } } } diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs index d780d734..d0da82c8 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs @@ -43,15 +43,7 @@ protected static string GetPatientIdFromDevice(Model.Device device) { EnsureArg.IsNotNull(device, nameof(device)); - var patientId = device.Patient?.GetId() ?? throw new FhirResourceNotFoundException(ResourceType.Patient); - - // only allow unreserved URI characters - if (!Regex.IsMatch(patientId, @"^[A-Za-z0-9_.\-~]+$")) - { - throw new NotSupportedException("Unsupported characters found in patient id"); - } - - return patientId; + return device.Patient?.GetId() ?? throw new FhirResourceNotFoundException(ResourceType.Patient); } protected async override Task<(string DeviceId, string PatientId)> LookUpDeviceAndPatientIdAsync(string value, string system = null) diff --git a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs index 6752576f..05a48e5b 100644 --- a/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs +++ b/test/Microsoft.Health.Fhir.R4.Ingest.UnitTests/Service/R4DeviceAndPatientLookupIdentityServiceTests.cs @@ -3,7 +3,6 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- -using System; using System.Threading.Tasks; using Hl7.Fhir.Rest; using Microsoft.Health.Extensions.Fhir.Service; @@ -110,7 +109,7 @@ public async void GivenDeviceWithPatientReferenceUnsupportedCharacters_WhenResol var device = new Model.Device { Id = "1", - Patient = new Model.ResourceReference("Patient/&containsURIrestrictedchars"), + Patient = new Model.ResourceReference("Not a reference in the form of: /ResourceName/Identifier"), }; var mg = Substitute.For(); @@ -121,7 +120,8 @@ public async void GivenDeviceWithPatientReferenceUnsupportedCharacters_WhenResol using (var idSrv = new R4DeviceAndPatientLookupIdentityService(fhirClient, resourceService)) { - var ex = await Assert.ThrowsAsync(async () => await idSrv.ResolveResourceIdentitiesAsync(mg)); + var ex = await Assert.ThrowsAsync(async () => await idSrv.ResolveResourceIdentitiesAsync(mg)); + Assert.Equal(ResourceType.Patient, ex.FhirResourceType); } await resourceService.Received(1).GetResourceByIdentityAsync(fhirClient, "deviceId", null); From 18a6eba6b53891cbc53f0f4484621de74838f970 Mon Sep 17 00:00:00 2001 From: William Yochum Date: Tue, 29 Jun 2021 14:47:23 -0700 Subject: [PATCH 3/4] removed unused imports --- .../Service/R4DeviceAndPatientLookupIdentityService.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs index d0da82c8..b15a7b13 100644 --- a/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs +++ b/src/lib/Microsoft.Health.Fhir.R4.Ingest/Service/R4DeviceAndPatientLookupIdentityService.cs @@ -3,8 +3,6 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- -using System; -using System.Text.RegularExpressions; using System.Threading.Tasks; using EnsureThat; using Hl7.Fhir.Rest; From 82fee16445baf726f58245b879077c97dd46a4e5 Mon Sep 17 00:00:00 2001 From: William Yochum Date: Tue, 6 Jul 2021 11:57:27 -0700 Subject: [PATCH 4/4] address pr comments --- .../ModelExtensions.cs | 18 ++++----- .../ModelExtensionsTests.cs | 39 +++++++++++++++++++ 2 files changed, 47 insertions(+), 10 deletions(-) create mode 100644 test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/ModelExtensionsTests.cs diff --git a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs index 1a5ac62d..52b9c175 100644 --- a/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs +++ b/src/lib/Microsoft.Health.Extensions.Fhir.R4/ModelExtensions.cs @@ -3,14 +3,14 @@ // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. // ------------------------------------------------------------------------------------------------- -using System.Collections.Concurrent; +using System; using System.Text.RegularExpressions; namespace Microsoft.Health.Extensions.Fhir { public static class ModelExtensions { - private static ConcurrentDictionary _idMatcherRegexCache = new ConcurrentDictionary(); + private static Regex _idMatcherRegex = new Regex(@"^[A-Za-z0-9_.\-~#]+$", RegexOptions.Compiled); public static Hl7.Fhir.Model.ResourceReference ToReference(this TResource resource) where TResource : Hl7.Fhir.Model.Resource @@ -43,25 +43,23 @@ public static Hl7.Fhir.Model.ResourceReference ToReference(this strin public static string GetId(this Hl7.Fhir.Model.ResourceReference reference) { string id; - var referenceType = typeof(TResource).Name; + var referenceType = $"{typeof(TResource).Name}/"; - if (reference?.Reference == null) + if (reference?.Reference == null || !reference.Reference.StartsWith(referenceType, StringComparison.InvariantCultureIgnoreCase)) { return null; } - var idMatcherRegex = _idMatcherRegexCache.GetOrAdd(referenceType, new Regex(referenceType + @"\/([A-Za-z0-9_.\-~#]{1,64})", RegexOptions.Compiled)); - // Reference should be in the form: ResourceType/Identifier - // If there is a match, the 2nd group will contain the identifier. - var matches = idMatcherRegex.Match(reference.Reference); - if (matches?.Groups?.Count != 2) + var relativeReferenceIdentifier = reference.Reference.Substring(referenceType.Length); + var matches = _idMatcherRegex.Match(relativeReferenceIdentifier); + if (matches?.Groups?.Count != 1) { return null; } else { - id = matches?.Groups?[1].Value; + id = matches?.Groups?[0].Value; return id; } } diff --git a/test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/ModelExtensionsTests.cs b/test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/ModelExtensionsTests.cs new file mode 100644 index 00000000..c7ff1887 --- /dev/null +++ b/test/Microsoft.Health.Extensions.Fhir.R4.UnitTests/ModelExtensionsTests.cs @@ -0,0 +1,39 @@ +// ------------------------------------------------------------------------------------------------- +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information. +// ------------------------------------------------------------------------------------------------- + +using Xunit; +using Model = Hl7.Fhir.Model; + +namespace Microsoft.Health.Extensions.Fhir.R4.UnitTests +{ + public class ModelExtensionsTests + { + [Fact] + public void GivenDeviceWithValidPatientReference_GetId_ThenReferenceIdentifierIsReturned_Test() + { + var device = new Model.Device + { + Id = "1", + Patient = new Model.ResourceReference("Patient/123"), + }; + + var patientIdentifier = device.Patient?.GetId(); + Assert.Equal("123", patientIdentifier); + } + + [Fact] + public void GivenDeviceWithInvalidPatientReference_GetId_ThenNullIsReturned_Test() + { + var device = new Model.Device + { + Id = "1", + Patient = new Model.ResourceReference("Not a reference in the form of: ResourceName/Identifier"), + }; + + var patientReference = device.Patient?.GetId(); + Assert.Null(patientReference); + } + } +}