Skip to content

JCL-417: Validate LDP containment data in SolidContainer::getResources #571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acoburn merged 3 commits into from
Jul 6, 2023
Merged

JCL-417: Validate LDP containment data in SolidContainer::getResources #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
403e066
JCL-417: Validate LDP containment data in SolidContainer::getResource…
acoburn Jul 3, 2023
d3fd371
Use verify mechanism
acoburn Jul 5, 2023
c94833f
Merge branch 'main' into JCL-417-ldp-contains
acoburn Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion api/src/main/java/com/inrupt/client/ValidationResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,18 @@ public class ValidationResult {
* @param messages the messages from validation method
*/
public ValidationResult(final boolean valid, final String... messages) {
this(valid, Arrays.asList(messages));
}

/**
* Create a ValidationResult object.
*
* @param valid the result from validation
* @param messages the messages from validation method
*/
public ValidationResult(final boolean valid, final List<String> messages) {
this.valid = valid;
this.result = Arrays.asList(messages);
this.result = messages;
}

/**
Expand Down
92 changes: 84 additions & 8 deletions solid/src/main/java/com/inrupt/client/solid/SolidContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,26 @@
*/
package com.inrupt.client.solid;

import com.inrupt.client.ValidationResult;
import com.inrupt.client.vocabulary.LDP;
import com.inrupt.client.vocabulary.RDF;
import com.inrupt.rdf.wrapping.commons.ValueMappings;
import com.inrupt.rdf.wrapping.commons.WrapperIRI;

import java.net.URI;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.rdf.api.Dataset;
import org.apache.commons.rdf.api.Graph;
import org.apache.commons.rdf.api.IRI;
import org.apache.commons.rdf.api.RDFTerm;
import org.apache.commons.rdf.api.Triple;

/**
* A Solid Container Object.
Expand Down Expand Up @@ -77,15 +82,43 @@ public SolidContainer(final URI identifier, final Dataset dataset, final Metadat
* @return the contained resources
*/
public Set<SolidResource> getResources() {
final Node node = new Node(rdf.createIRI(getIdentifier().toString()), getGraph());
try (final Stream<Node.TypedNode> stream = node.getResources()) {
return stream.map(child -> {
final Metadata.Builder builder = Metadata.newBuilder();
getMetadata().getStorage().ifPresent(builder::storage);
child.getTypes().forEach(builder::type);
return new SolidResourceReference(URI.create(child.getIRIString()), builder.build());
}).collect(Collectors.collectingAndThen(Collectors.toSet(), Collections::unmodifiableSet));
final String container = normalize(getIdentifier());
Copy link
Contributor

@timea-solid timea-solid Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe now you can call validate() here.

Copy link
Collaborator Author

@acoburn acoburn Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not validate() directly, as that performs a number of checks that are irrelevant here. You will see that in https://github.com/inrupt/solid-client-java/pull/571/files#diff-2c9b192110a924fa828ba14eaa2e7359afa8b3b811c8ad12fd6563e9161d7ba0R90, we re-use some of the same code as validate in this pipeline

// As defined by the Solid Protocol, containers always end with a slash.
if (container.endsWith("/")) {
Copy link
Contributor

@timea-solid timea-solid Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could have a dedicated error thrown here and upon creation too, if the URI does not end in a slash.
And while this is a precondition, I find it in general strange to be checked here.

Copy link
Collaborator Author

@acoburn acoburn Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not be inclined to throw an exception here. Generally, I'd prefer the client libraries to be generous in the data they accept. In this particular case, if the resource is a SolidContainer, then the getResources() method must follow the rules of Solid, but if the resource isn't strictly a SolidContainer, it shouldn't cause the consuming application to throw an exception (and thus the getResources method would just return an empty collection).

This condition is specifically to prevent against such cases as this:

A resource is https://example.com/resource and it includes the triple:

<> ldp:contains <https://example.com/resourcechild> .

That triple should not cause the getResources method to include the sibling resource from being included in that list (since it doesn't conform to the Solid definition of containment)

Copy link
Collaborator Author

@acoburn acoburn Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is that these checks are put in place to prevent a malicious or misbehaving server from causing an application to perform unexpected operations on non-contained resources. For well-behaved servers, this will never be an issue and there is no need to add these constraints, but we can't expect all servers to be well-behaved

Copy link
Contributor

@NSeydoux NSeydoux Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, your last comment was the reason for which we decided to throw an error in the JS implementation of this feature: it may be desirable to let the consuming app know that there is something wrong with the target resource so that it takes action and removes the offending triple, so that other clients who don't implement this check don't perform the unintended operations we are preventing against here. I would prefer the DX to be convergent between JS and Java, so I'm happy to filter out the resources that do not follow slash semantics from the children list, but isn't there value in communicating this inconsistency to the app/user, especially when they can act on it?

Copy link
Collaborator Author

@acoburn acoburn Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale is basically the "robustness principle" https://en.wikipedia.org/wiki/Robustness_principle

Be conservative in what you do and liberal in what you accept from others.

There may be cases where an RDFSource is used to model an example Container's RDF or where an app is intending to write certain triples or where a server is simply doing things incorrectly. There will be a lot of "slightly off" data in the wild, and I think it is important that client libraries don't just blow up in those cases. As long as the data is syntactically correct, the semantics may vary from domain to domain, and it would be unfortunate if we imposed constraints that prevented certain interesting use cases.

Copy link
Collaborator Author

@acoburn acoburn Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a compromise (actually, I think it's a better solution overall)

  • Leave the current get resources() method as-is

  • Also implement the validate() method and have it return an invalid result if there are illegal containment triples or if it doesn't end in a slash, etc

Copy link
Contributor

@timea-solid timea-solid Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see both sides of the coin but I like the validate() option a lot! Validate should by default already have this check in then.
And we need to highlight it in the docs then I would be fine with it.

Copy link
Contributor

@NSeydoux NSeydoux Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a distinction between a read method that just ignores triples we deem invalid and a validation method where the intent is to be aware of any issues sounds like a good pattern. I'll think about other cases when it would be applicable.

Copy link
Collaborator Author

@acoburn acoburn Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timea-solid and @NSeydoux I'll wait on merging this until you have a chance to see my latest additions to this in d3fd371

final Node node = new Node(rdf.createIRI(getIdentifier().toString()), getGraph());
try (final Stream<Node.TypedNode> stream = node.getResources()) {
return stream.filter(child -> verifyContainmentIri(container, child)).map(child -> {
final Metadata.Builder builder = Metadata.newBuilder();
getMetadata().getStorage().ifPresent(builder::storage);
child.getTypes().forEach(builder::type);
return new SolidResourceReference(URI.create(child.getIRIString()), builder.build());
}).collect(Collectors.collectingAndThen(Collectors.toSet(), Collections::unmodifiableSet));
}
}
return Collections.emptySet();
}

@Override
public ValidationResult validate() {
// Get the normalized container URI
final String container = normalize(getIdentifier());
final List<String> messages = new ArrayList<>();
// Verify that the container URI path ends with a slash
if (!container.endsWith("/")) {
messages.add("Container URI does not end in a slash");
}

// Verify that all ldp:contains triples align with Solid expectations
getGraph().stream(null, rdf.createIRI(LDP.contains.toString()), null)
.collect(Collectors.partitioningBy(verifyContainmentTriple(container)))
.get(false) // we are only concerned with the invalid triples
.forEach(triple -> messages.add("Invalid containment triple: " + triple.getSubject().ntriplesString() +
" ldp:contains " + triple.getObject().ntriplesString() + " ."));

if (messages.isEmpty()) {
return new ValidationResult(true);
}
return new ValidationResult(false, messages);
}

/**
Expand All @@ -99,6 +132,49 @@ public Stream<SolidResource> getContainedResources() {
return getResources().stream();
}

static String normalize(final IRI iri) {
return normalize(URI.create(iri.getIRIString()));
}

static String normalize(final URI uri) {
return uri.normalize().toString().split("#")[0].split("\\?")[0];
}

static Predicate<Triple> verifyContainmentTriple(final String container) {
final IRI subject = rdf.createIRI(container);
return triple -> {
if (!triple.getSubject().equals(subject)) {
// Out-of-domain containment triple subject
return false;
}
if (triple.getObject() instanceof IRI) {
return verifyContainmentIri(container, (IRI) triple.getObject());
}
// Non-URI containment triple object
return false;
};
}

static boolean verifyContainmentIri(final String container, final IRI object) {
if (!object.getIRIString().startsWith(container)) {
// Out-of-domain containment triple object
return false;
} else {
final String relativePath = object.getIRIString().substring(container.length());
final String normalizedPath = relativePath.endsWith("/") ?
relativePath.substring(0, relativePath.length() - 1) : relativePath;
if (normalizedPath.isEmpty()) {
// Containment triple subject and object cannot be the same
return false;
}
if (normalizedPath.contains("/")) {
// Containment cannot skip intermediate nodes
return false;
}
}
return true;
}

@SuppressWarnings("java:S2160") // Wrapper equality is correctly delegated to underlying node
static final class Node extends WrapperIRI {
private final IRI ldpContains = rdf.createIRI(LDP.contains.toString());
Expand Down
30 changes: 29 additions & 1 deletion solid/src/test/java/com/inrupt/client/solid/SolidClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@

import com.inrupt.client.ClientProvider;
import com.inrupt.client.Headers;
import com.inrupt.client.Request;
import com.inrupt.client.Response;
import com.inrupt.client.auth.Session;
import com.inrupt.client.spi.RDFFactory;
import com.inrupt.client.util.URIBuilder;

import java.io.ByteArrayInputStream;
import java.io.IOException;
Expand All @@ -36,6 +38,7 @@
import java.util.*;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.rdf.api.RDF;
Expand Down Expand Up @@ -161,7 +164,7 @@ void testGetResource() throws IOException, InterruptedException {

@Test
void testGetContainer() throws IOException, InterruptedException {
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlist");
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlists/");

client.read(uri, SolidContainer.class).thenAccept(container -> {
try (final SolidContainer c = container) {
Expand Down Expand Up @@ -216,6 +219,31 @@ void testGetBinaryCreate() {
}).toCompletableFuture().join();
}

@Test
void testSolidContainerWithInvalidData() {
final URI uri = URI.create(config.get("solid_resource_uri") + "/container/");
final CompletionException err = assertThrows(CompletionException.class,
client.read(uri, SolidContainer.class).toCompletableFuture()::join);
assertInstanceOf(DataMappingException.class, err.getCause());
}

@Test
void testLowLevelSolidContainer() {
final URI uri = URI.create(config.get("solid_resource_uri") + "/container/");

final Set<URI> expected = new HashSet<>();
expected.add(URIBuilder.newBuilder(uri).path("newContainer/").build());
expected.add(URIBuilder.newBuilder(uri).path("test.txt").build());
expected.add(URIBuilder.newBuilder(uri).path("test2.txt").build());

client.send(Request.newBuilder(uri).build(), SolidResourceHandlers.ofSolidContainer())
.thenAccept(response -> {
final SolidContainer container = response.body();
assertEquals(expected, container.getResources().stream()
.map(SolidResource::getIdentifier).collect(Collectors.toSet()));
}).toCompletableFuture().join();
}

@Test
void testBinaryCreate() throws IOException {
final URI uri = URI.create(config.get("solid_resource_uri") + "/binary");
Expand Down
55 changes: 54 additions & 1 deletion solid/src/test/java/com/inrupt/client/solid/SolidMockHttpService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,25 @@ private void setupMocks() {
)
);

wireMockServer.stubFor(get(urlEqualTo("/container/"))
.withHeader("User-Agent", equalTo(USER_AGENT))
.willReturn(aResponse()
.withStatus(200)
.withHeader("Content-Type", "text/turtle")
.withHeader("Link", Link.of(LDP.BasicContainer, "type").toString())
.withHeader("Link", Link.of(URI.create("http://acl.example/solid/"), "acl").toString())
.withHeader("Link", Link.of(URI.create("http://storage.example/"),
PIM.storage).toString())
.withHeader("Link", Link.of(URI.create("https://history.test/"), "timegate").toString())
.withHeader("WAC-Allow", "user=\"read write\",public=\"read\"")
.withHeader("Allow", "POST, PUT, PATCH")
.withHeader("Accept-Post", "application/ld+json, text/turtle")
.withHeader("Accept-Put", "application/ld+json, text/turtle")
.withHeader("Accept-Patch", "application/sparql-update, text/n3")
.withBodyFile("container.ttl")
)
);

wireMockServer.stubFor(get(urlEqualTo("/recipe"))
.withHeader("User-Agent", equalTo(USER_AGENT))
.willReturn(aResponse()
Expand Down Expand Up @@ -130,6 +149,40 @@ private void setupMocks() {
.willReturn(aResponse()
.withStatus(204)));

wireMockServer.stubFor(get(urlEqualTo("/playlists/"))
.withHeader("User-Agent", equalTo(USER_AGENT))
.willReturn(aResponse()
.withStatus(200)
.withHeader("Content-Type", "text/turtle")
.withHeader("Link", Link.of(LDP.BasicContainer, "type").toString())
.withHeader("Link", Link.of(URI.create("http://storage.example/"),
PIM.storage).toString())
.withHeader("Link", Link.of(URI.create("https://history.test/"), "timegate").toString())
.withHeader("Link", Link.of(URI.create("http://acl.example/playlists"), "acl").toString())
.withHeader("WAC-Allow", "user=\"read write\",public=\"read\"")
.withHeader("Allow", "POST, PUT, PATCH")
.withHeader("Accept-Post", "application/ld+json, text/turtle")
.withHeader("Accept-Put", "application/ld+json, text/turtle")
.withHeader("Accept-Patch", "application/sparql-update, text/n3")
.withBodyFile("playlist.ttl")
)
);

wireMockServer.stubFor(put(urlEqualTo("/playlists/"))
.withHeader("User-Agent", equalTo(USER_AGENT))
.withHeader("Content-Type", containing("text/turtle"))
.withRequestBody(containing(
"<https://library.test/12345/song1.mp3>"))
.willReturn(aResponse()
.withStatus(204)));

wireMockServer.stubFor(delete(urlEqualTo("/playlists/"))
.withHeader("User-Agent", equalTo(USER_AGENT))
.willReturn(aResponse()
.withStatus(204)));




wireMockServer.stubFor(get(urlEqualTo("/playlist"))
.withHeader("User-Agent", equalTo(USER_AGENT))
Expand All @@ -140,7 +193,7 @@ private void setupMocks() {
.withHeader("Link", Link.of(URI.create("http://storage.example/"),
PIM.storage).toString())
.withHeader("Link", Link.of(URI.create("https://history.test/"), "timegate").toString())
.withHeader("Link", Link.of(URI.create("http://acl.example/playlist"), "acl").toString())
.withHeader("Link", Link.of(URI.create("http://acl.example/playlists"), "acl").toString())
.withHeader("WAC-Allow", "user=\"read write\",public=\"read\"")
.withHeader("Allow", "POST, PUT, PATCH")
.withHeader("Accept-Post", "application/ld+json, text/turtle")
Expand Down
8 changes: 7 additions & 1 deletion solid/src/test/java/com/inrupt/client/solid/SolidSyncClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ void testGetResource() {

@Test
void testGetContainer() {
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlist");
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlists/");

try (final SolidContainer container = client.read(uri, SolidContainer.class)) {
assertEquals(uri, container.getIdentifier());
Expand All @@ -156,6 +156,12 @@ void testGetContainer() {
}
}

@Test
void testGetContainerDataMappingError() {
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlist");
assertThrows(DataMappingException.class, () -> client.read(uri, SolidContainer.class));
}

@Test
void testGetInvalidType() {
final URI uri = URI.create(config.get("solid_resource_uri") + "/playlist");
Expand Down
25 changes: 25 additions & 0 deletions solid/src/test/resources/__files/container.ttl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
@prefix dct: <http://purl.org/dc/terms/>.
@prefix ldp: <http://www.w3.org/ns/ldp#>.
@prefix xsd: <http://www.w3.org/2001/XMLSchema#>.
@prefix pl: <http://www.w3.org/ns/iana/media-types/text/plain#>.

<>
a ldp:BasicContainer ;
dct:modified "2022-11-25T10:36:36Z"^^xsd:dateTime;
ldp:contains <newContainer/>, <test.txt>, <test2.txt> .
<newContainer/>
a ldp:BasicContainer ;
dct:modified "2022-11-25T10:36:36Z"^^xsd:dateTime .
<test.txt>
a pl:Resource, ldp:NonRDFSource;
dct:modified "2022-11-25T10:34:14Z"^^xsd:dateTime .
<test2.txt>
a pl:Resource, ldp:NonRDFSource;
dct:modified "2022-11-25T10:37:06Z"^^xsd:dateTime .

# These containment triples should not be included in a getResources response
<>
ldp:contains <https://example.com/other> , <newContainer/child> , <> , <./> .
<https://example.test/container/>
a ldp:BasicContainer ;
ldp:contains <https://example.test/container/external> .