Skip to content

Commit

Permalink
issue #2965 - use the first instance of /resourceType and not the last
Browse files Browse the repository at this point in the history 
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 <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Nov 10, 2021
1 parent 6503979 commit a8f7179
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 73 deletions.
8 changes: 2 additions & 6 deletions fhir-model/src/main/java/com/ibm/fhir/model/util/FHIRUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
* <code>"<resource-type>/<id>/_history/<version>"</code>. Note that the server will turn this into an absolute URL prior to
* <code>"[resource-type]/[id]/_history/[version]"</code>. 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());
}

/**
Expand Down
68 changes: 29 additions & 39 deletions fhir-server-test/src/test/java/com/ibm/fhir/server/test/FHIRServerTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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());
}

/**
Expand Down Expand Up @@ -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: <code>[base]/<resource-type>/<id>/_history/<version></code>
* should be: <code>[base]/[resource-type]/[id]/_history/[version]</code>
*
* @param response the response object for a REST API invocation
* @return the logical id value
Expand Down Expand Up @@ -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 <HumanName> nameList = new ArrayList<HumanName>();
for(HumanName humanName: patient.getName()) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down
10 changes: 5 additions & 5 deletions fhir-server-test/src/test/java/com/ibm/fhir/server/test/ServerSpecTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down
58 changes: 44 additions & 14 deletions fhir-server-test/src/test/java/com/ibm/fhir/server/test/UpdateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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]);
Expand All @@ -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]);
Expand Down Expand Up @@ -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());
Expand All @@ -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.
Expand All @@ -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);
}
}

Expand Down
6 changes: 5 additions & 1 deletion fhir-server/src/main/java/com/ibm/fhir/server/resources/FHIRResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down
12 changes: 6 additions & 6 deletions fhir-server/src/main/java/com/ibm/fhir/server/resources/Update.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);

Expand Down Expand Up @@ -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("\"")) {
Expand Down
5 changes: 3 additions & 2 deletions fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -1606,7 +1607,7 @@ private Map<Integer, Entry> 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 {
Expand Down

0 comments on commit a8f7179

Please sign in to comment.