From a8f717996b96b826f0e3c711f52fdf150db000d3 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Tue, 9 Nov 2021 18:15:01 -0500 Subject: [PATCH] issue #2965 - use the first instance of `/resourceType` and not the last when inferring the baseUrl from the request URL. This should be relatively safe because the match is case-sensitive. However, the logic would break if the intended baseUrl of the server actually contains a path segment that overlaps with a resource name (e.g. https://example.com/PatientAPI ). Alternatives would be to either A. rely solely on a configured baseUrl; or B. do more processing of the URL to ensure we're stripping a path and not a hostname Signed-off-by: Lee Surprenant --- .../com/ibm/fhir/model/util/FHIRUtil.java | 8 +-- .../fhir/server/test/FHIRServerTestBase.java | 68 ++++++++----------- .../ibm/fhir/server/test/ServerSpecTest.java | 10 +-- .../com/ibm/fhir/server/test/UpdateTest.java | 58 ++++++++++++---- .../fhir/server/resources/FHIRResource.java | 6 +- .../com/ibm/fhir/server/resources/Update.java | 12 ++-- .../ibm/fhir/server/util/FHIRRestHelper.java | 5 +- 7 files changed, 94 insertions(+), 73 deletions(-) diff --git a/fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java b/fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java index aca3165dde4..051910d87d1 100644 --- a/fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java +++ b/fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java @@ -259,18 +259,14 @@ public static OperationOutcome buildOperationOutcome(String message, IssueType i /** * Builds a relative "Location" header value for the specified resource. This will be a string of the form - * "//_history/". Note that the server will turn this into an absolute URL prior to + * "[resource-type]/[id]/_history/[version]". Note that the server will turn this into an absolute URL prior to * returning it to the client. * * @param resource * the resource for which the location header value should be returned */ public static URI buildLocationURI(String type, Resource resource) { - String resourceTypeName = resource.getClass().getSimpleName(); - if (!resourceTypeName.equals(type)) { - resourceTypeName = type; - } - return URI.create(resourceTypeName + "/" + resource.getId() + "/_history/" + resource.getMeta().getVersionId().getValue()); + return URI.create(type + "/" + resource.getId() + "/_history/" + resource.getMeta().getVersionId().getValue()); } /** diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/FHIRServerTestBase.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/FHIRServerTestBase.java index 75c4502aba9..6659b5cd55d 100644 --- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/FHIRServerTestBase.java +++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/FHIRServerTestBase.java @@ -580,7 +580,7 @@ public void assertResponse(Response response, int expectedStatusCode) { assertNotNull(response); assertEquals(expectedStatusCode, response.getStatus()); } - + /** * Verify that the status code in the Response is of the expected status * family. @@ -594,7 +594,7 @@ protected void assertResponse(Response response, Response.Status.Family family) if( ! response.getStatusInfo().getFamily().equals(family) ) { message = response.readEntity(String.class); } - assertEquals(message, response.getStatusInfo().getFamily(), family); + assertEquals(message, family, response.getStatusInfo().getFamily()); } /** @@ -667,10 +667,24 @@ protected void assertValidationOperationOutcome(OperationOutcome oo, String msgP // Misc. common functions used by testcases. // + /** + * Assert that the locationURI is formatted appropriately with the expected baseURL, resourceType, resourceId, and + * and versionId. + * + * @throws Exception + * @implNote Uses {@link #getRestBaseURL()} to construct the base URL for the expected location URI + */ + public void validateLocationURI(String locationURI, String resourceType, + String expectedResourceId, String expectedVersionId) throws Exception { + String expectedLocation = getRestBaseURL() + "/" + resourceType + "/" + + expectedResourceId + "/_history/" + expectedVersionId; + assertEquals(expectedLocation, locationURI.toString()); + } + /** * For the specified response, this function will extract the logical id value * from the response's Location header. The format of a location header value - * should be: [base]///_history/ + * should be: [base]/[resource-type]/[id]/_history/[version] * * @param response the response object for a REST API invocation * @return the logical id value @@ -734,20 +748,6 @@ private String getAbsoluteFilename(String filename) { return null; } - protected String[] getLocationURITokens(String locationURI) { - String[] temp = locationURI.split("/"); - String[] tokens; - if (temp.length > 4) { - tokens = new String[4]; - for (int i = 0; i < 4; i++) { - tokens[i] = temp[temp.length - 4 + i]; - } - } else { - tokens = temp; - } - return tokens; - } - protected Patient setUniqueFamilyName(Patient patient, String uniqueName) { List nameList = new ArrayList(); for(HumanName humanName: patient.getName()) { @@ -787,10 +787,10 @@ public void checkForIssuesWithValidation(Resource resource, boolean failOnValida } System.out.println("count = [" + issues.size() + "]"); - assertEquals(nonWarning,0); + assertEquals(0, nonWarning); if(failOnWarning) { - assertEquals(allOtherIssues,0); + assertEquals(0, allOtherIssues); } } else { @@ -799,9 +799,9 @@ public void checkForIssuesWithValidation(Resource resource, boolean failOnValida } /** - * Parses a location URI into the resourceType, resourceId, and (optionally) the version id. + * Parses a location URI into the resourceType, resource id, and version id. * @param location - * @return + * @return A string array with three parts: resourceType, resourceId, and versionId (in that order) */ public static String[] parseLocationURI(String location) { String[] result = null; @@ -810,25 +810,15 @@ public static String[] parseLocationURI(String location) { } String[] tokens = location.split("/"); + // 1 2 3 4 5 6 7 8 9 10 + // https: // localhost:9443 fhir-server api v4 Patient id _history version + assertEquals(10, tokens.length); + // Check if we should expect 4 tokens or only 2. - if (location.contains("_history")) { - if (tokens.length >= 4) { - result = new String[3]; - result[0] = tokens[tokens.length - 4]; - result[1] = tokens[tokens.length - 3]; - result[2] = tokens[tokens.length - 1]; - } else { - throw new IllegalArgumentException("Incorrect location value specified: " + location); - } - } else { - if (tokens.length >= 2) { - result = new String[2]; - result[0] = tokens[tokens.length - 2]; - result[1] = tokens[tokens.length - 1]; - } else { - throw new IllegalArgumentException("Incorrect location value specified: " + location); - } - } + result = new String[3]; + result[0] = tokens[tokens.length - 4]; + result[1] = tokens[tokens.length - 3]; + result[2] = tokens[tokens.length - 1]; return result; } diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/ServerSpecTest.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/ServerSpecTest.java index ef22908fe3d..30dfd5303a1 100644 --- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/ServerSpecTest.java +++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/ServerSpecTest.java @@ -661,7 +661,7 @@ public void testConditionalUpdateObservation2() throws Exception { String obsId = UUID.randomUUID().toString(); Observation obs = TestUtil.readLocalResource("Observation1.json"); obs = obs.toBuilder() - .subject(Reference.builder().reference(string(fakePatientRef)).build()) + .subject(Reference.builder().reference(fakePatientRef).build()) .build(); // First conditional update should find no matches, so we should get back a 201 @@ -673,11 +673,11 @@ public void testConditionalUpdateObservation2() throws Exception { String locationURI = response.getLocation(); assertNotNull(locationURI); - String[] tokens = parseLocationURI(locationURI); - String resourceId = tokens[1]; + // get the server-assigned resource id from the location header + String resourceId = getLocationLogicalId(response.getResponse()); - // Second conditional update should find 1 match, but because there is a un-matching - // resourceId in the input resource, so we should get back a 400 error. + // Second conditional update should find 1 match, but because there is an un-matching + // resourceId in the input resource, we should get back a 400 error. query = new FHIRParameters().searchParam("_id", resourceId); obs = obs.toBuilder().id(obsId).build(); response = client.conditionalUpdate(obs, query); diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/UpdateTest.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/UpdateTest.java index 78893a96119..0f098c6e66f 100644 --- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/UpdateTest.java +++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/UpdateTest.java @@ -68,6 +68,8 @@ public void testUpdateCreate1() throws Exception { assertNotNull(response); assertResponse(response.getResponse(), Response.Status.CREATED.getStatusCode()); + validateLocationURI(response.getLocation(), "Patient", newId, "1"); + // Now read the resource to verify it's there. response = client.read("Patient", newId); assertNotNull(response); @@ -97,9 +99,9 @@ public void testUpdateCreate2() throws Exception { assertNotNull(response); assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); String locationURI = response.getLocation(); + + validateLocationURI(locationURI, "Patient", patient.getId(), "2"); String[] locationTokens = parseLocationURI(locationURI); - assertEquals(3, locationTokens.length); - assertEquals("2", locationTokens[2]); // Now read the resource to verify it's there. response = client.vread(locationTokens[0], locationTokens[1], locationTokens[2]); @@ -129,9 +131,9 @@ public void testUpdateIfModified() throws Exception { assertNotNull(response); assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); String locationURI = response.getLocation(); + + validateLocationURI(locationURI, "Patient", patient.getId(), "2"); String[] locationTokens = parseLocationURI(locationURI); - assertEquals(3, locationTokens.length); - assertEquals("2", locationTokens[2]); // Now read the resource to verify it's there. response = client.vread(locationTokens[0], locationTokens[1], locationTokens[2]); @@ -153,9 +155,9 @@ public void testUpdateIfModified() throws Exception { assertNotNull(response); assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); String locationURI = response.getLocation(); + + validateLocationURI(locationURI, "Patient", patient.getId(), "3"); String[] locationTokens = parseLocationURI(locationURI); - assertEquals(3, locationTokens.length); - assertEquals("3", locationTokens[2]); // Now read the resource to verify it's there. response = client.vread(locationTokens[0], locationTokens[1], locationTokens[2]); @@ -298,16 +300,15 @@ public void testUpdateCreate3() throws Exception { assertResponse(response.getResponse(), Response.Status.CREATED.getStatusCode()); String locationURI = response.getLocation(); - String[] locationTokens = response.parseLocation(response.getLocation()); - String deletedId = locationTokens[1]; + String resourceId = getLocationLogicalId(response.getResponse()); // Read the new patient. - response = client.read(locationTokens[0], locationTokens[1]); + response = client.read("Patient", resourceId); assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); Patient createdPatient = response.getResource(Patient.class); assertNotNull(createdPatient); - response = client.delete("Patient", deletedId); + response = client.delete("Patient", resourceId); assertNotNull(response); if (isDeleteSupported()) { assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); @@ -318,7 +319,7 @@ public void testUpdateCreate3() throws Exception { } // Read the new patient. - response = client.read("Patient", deletedId); + response = client.read("Patient", resourceId); assertResponse(response.getResponse(), Response.Status.GONE.getStatusCode()); // Update the patient. @@ -327,9 +328,38 @@ public void testUpdateCreate3() throws Exception { response = client.update(createdPatient); assertResponse(response.getResponse(), Response.Status.CREATED.getStatusCode()); locationURI = response.getLocation(); - locationTokens = parseLocationURI(locationURI); - assertEquals(3, locationTokens.length); - assertEquals("3", locationTokens[2]); + validateLocationURI(locationURI, "Patient", resourceId, "3"); + } + } + + /** + * Test the "update/create" behavior with an purposefully nasty id. + */ + @Test(dependsOnMethods = {"retrieveConfig"}) + public void testUpdateCreate4() throws Exception { + assertNotNull(updateCreateEnabled); + + // If the "Update/Create" feature is enabled, then test the normal update behavior. + if (updateCreateEnabled.booleanValue()) { + + String nastyId = "Patient" + UUID.randomUUID(); + Patient patient = TestUtil.getMinimalResource(Patient.class).toBuilder() + .id(nastyId) + .build(); + + // Create the new resource via the update operation. + FHIRClient client = getFHIRClient(); + FHIRResponse response = client.update(patient); + assertNotNull(response); + assertResponse(response.getResponse(), Response.Status.CREATED.getStatusCode()); + validateLocationURI(response.getLocationURI().toString(), "Patient", nastyId, "1"); + + // Now read the resource to verify it's there. + response = client.read("Patient", getLocationLogicalId(response.getResponse())); + assertNotNull(response); + assertResponse(response.getResponse(), Response.Status.OK.getStatusCode()); + Patient responsePatient = response.getResource(Patient.class); + assertNotNull(responsePatient); } } diff --git a/fhir-server/src/main/java/com/ibm/fhir/server/resources/FHIRResource.java b/fhir-server/src/main/java/com/ibm/fhir/server/resources/FHIRResource.java index 905dd0d2ae6..8a46b9e5405 100644 --- a/fhir-server/src/main/java/com/ibm/fhir/server/resources/FHIRResource.java +++ b/fhir-server/src/main/java/com/ibm/fhir/server/resources/FHIRResource.java @@ -399,6 +399,9 @@ protected String getRequestUri() throws Exception { * https://myhost:9443/fhir-server/api/v4/Patient to create a Patient resource, this method would return * "https://myhost:9443/fhir-server/api/v4". * + * @param type + * The resource type associated with the request URI (e.g. "Patient" in the case of + * https://myhost:9443/fhir-server/api/v4/Patient), or null if there is no such resource type * @return The base endpoint URI associated with the current request. * @throws Exception if an error occurs while reading the config * @implNote This method uses {@link #getRequestUri()} to get the original request URI and then strips it to the @@ -419,10 +422,11 @@ protected String getRequestBaseUri(String type) throws Exception { // Strip off any path elements after the base if (type != null && !type.isEmpty()) { - int resourceNamePathLocation = baseUri.lastIndexOf("/" + type); + int resourceNamePathLocation = baseUri.indexOf("/" + type); if (resourceNamePathLocation != -1) { baseUri = requestUri.substring(0, resourceNamePathLocation); } else { + // Assume the request was a batch/transaction and just use the requestUri as the base baseUri = requestUri; } diff --git a/fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java b/fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java index 14788d81810..b98754a4957 100644 --- a/fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java +++ b/fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java @@ -57,8 +57,8 @@ public Update() throws Exception { @PUT @Path("{type}/{id}") public Response update(@PathParam("type") String type, @PathParam("id") String id, Resource resource, - @HeaderParam(HttpHeaders.IF_MATCH) String ifMatch, - @HeaderParam(FHIRConstants.UPDATE_IF_MODIFIED_HEADER) boolean onlyIfModified, + @HeaderParam(HttpHeaders.IF_MATCH) String ifMatch, + @HeaderParam(FHIRConstants.UPDATE_IF_MODIFIED_HEADER) boolean onlyIfModified, @HeaderParam(HttpHeaders.IF_NONE_MATCH) String ifNoneMatchHdr) { log.entering(this.getClass().getName(), "update(String,String,Resource)"); Date startTime = new Date(); @@ -73,8 +73,8 @@ public Response update(@PathParam("type") String type, @PathParam("id") String i FHIRRestHelper helper = new FHIRRestHelper(getPersistenceImpl()); ior = helper.doUpdate(type, id, resource, ifMatch, null, onlyIfModified, ifNoneMatch); - ResponseBuilder response = - Response.ok().location(toUri(getAbsoluteUri(getRequestBaseUri(type), ior.getLocationURI().toString()))); + ResponseBuilder response = Response.ok() + .location(toUri(getAbsoluteUri(getRequestBaseUri(type), ior.getLocationURI().toString()))); status = ior.getStatus(); response.status(status); @@ -181,13 +181,13 @@ private Integer encodeIfNoneMatch(String value) throws FHIROperationException { if (value == null || value.isEmpty()) { return null; } - + if ("*".equals(value)) { // FHIR resource version numbers start at 1, so we use 0 to represent // all values "*" return IF_NONE_MATCH_ZERO; } - + // Values of the form W/"1" where 1 is the version number would result // in less-than-intuitive behavior and so are not supported at this time if (value.length() > 4 && value.startsWith("W/\"") && value.endsWith("\"")) { diff --git a/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java b/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java index 1a9bc843f27..800cffcac1a 100644 --- a/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java +++ b/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java @@ -445,7 +445,8 @@ private FHIRRestOperationResponse doPatchOrUpdate(String type, String id, FHIRPa } // Persist the resource - FHIRRestOperationResponse ior = doPatchOrUpdatePersist(event, type, id, patch != null, metaResponse.getResource(), metaResponse.getPrevResource(), warnings, metaResponse.isDeleted(), + FHIRRestOperationResponse ior = doPatchOrUpdatePersist(event, type, id, patch != null, + metaResponse.getResource(), metaResponse.getPrevResource(), warnings, metaResponse.isDeleted(), ifNoneMatch); txn.commit(); @@ -1606,7 +1607,7 @@ private Map validateBundle(Bundle bundle) throws Exception { throw buildRestException(msg, IssueType.VALUE); } String fullUrlPlusVersion = fullUrl; - if (resource != null && resource.getMeta() != null + if (resource != null && resource.getMeta() != null && resource.getMeta().getVersionId() != null && resource.getMeta().getVersionId().hasValue()) { fullUrlPlusVersion = fullUrl + resource.getMeta().getVersionId().getValue(); } else {