Skip to content
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.

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

JCL-431: Improve containment validation #653

Merged
merged 2 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
63 changes: 40 additions & 23 deletions solid/src/main/java/com/inrupt/client/solid/SolidContainer.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ public SolidContainer(final URI identifier, final Dataset dataset, final Metadat
* @return the contained resources
*/
public Set<SolidResource> getResources() {
final String container = normalize(getIdentifier());
// As defined by the Solid Protocol, containers always end with a slash.
if (container.endsWith("/")) {
if (getIdentifier().getPath().endsWith("/")) {
final String container = normalize(getIdentifier());
Copy link
Contributor

Choose a reason for hiding this comment

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

Newly, the condition is not normalized. This seems intentional.

But what happens with URIs of the form x/. and x/y/..?
Maybe we should normalize before checking whether it ends in a slash.

Example

Copy link
Contributor

Choose a reason for hiding this comment

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

getIdentifier from RDFSource via SolidRDFSource is a URI and getPath is not normalized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated this to normalize the URI path

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 -> {
Expand All @@ -100,14 +100,14 @@ public Set<SolidResource> getResources() {

@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("/")) {
if (!getIdentifier().getPath().endsWith("/")) {
messages.add("Container URI does not end in a slash");
}

// Get the normalized container URI
final String container = normalize(getIdentifier());
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

Maybe worth extracting something like isContainer for readability and consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in 2b4b916

// Verify that all ldp:contains triples align with Solid expectations
getGraph().stream(null, rdf.createIRI(LDP.contains.toString()), null)
.collect(Collectors.partitioningBy(verifyContainmentTriple(container)))
Expand All @@ -121,10 +121,6 @@ public ValidationResult validate() {
return new ValidationResult(false, messages);
}

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];
}
Expand All @@ -145,22 +141,43 @@ static Predicate<Triple> verifyContainmentTriple(final String container) {
}

static boolean verifyContainmentIri(final String container, final IRI object) {
if (!object.getIRIString().startsWith(container)) {
// Out-of-domain containment triple object

// 1 URI Structure Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these numbers refer to? I looked in several place I imagined would be relevant, but haven't found anything like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was more for my own organization. I've backed that out

final URI normalized = URI.create(object.getIRIString()).normalize();

// 1.A Query strings are not allowed in object URI
if (normalized.getQuery() != null) {
return false;
}

// 1.B URI fragments are not allowed in object URI
if (normalized.getFragment() != null) {
return false;
}

// 2 Relative path tests
final URI base = URI.create(container).normalize();
final URI relative = base.relativize(normalized);

// 2.A Base URI cannot equal the object URI
if (base.equals(normalized)) {
return false;
}

// 2.B Object URI must be relative to (contained in) the base URI
if (relative.isAbsolute()) {
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;
}
}

final String relativePath = relative.getPath();
final String normalizedPath = relativePath.endsWith("/") ?
relativePath.substring(0, relativePath.length() - 1) : relativePath;

// 2.C Containment cannot skip intermediate nodes
if (normalizedPath.contains("/")) {
return false;
}

return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,25 @@ void testEmptyContentType() {
}
}

@Test
void testNonContainer() {
final URI resource = URI.create(config.get("solid_resource_uri") + "/recipe");
final Request request = Request.newBuilder()
.uri(resource)
.header("Accept", "text/turtle")
.GET()
.build();

final Response<SolidContainer> response = client.send(request, SolidResourceHandlers.ofSolidContainer())
.toCompletableFuture().join();

try (final SolidContainer container = response.body()) {
assertEquals(resource, container.getIdentifier());
assertTrue(container.getResources().isEmpty());
assertFalse(container.validate().isValid());
}
}

@Test
void testInvalidRdf() {
final URI resource = URI.create(config.get("solid_resource_uri") + "/nonRDF");
Expand Down
4 changes: 3 additions & 1 deletion solid/src/test/resources/__files/container.ttl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

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