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

Jetty 12 - Simplification of aliases in PathResource #8631

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public class PathResource extends Resource

private final Path path;
private final URI uri;
private boolean targetResolved = false;
private boolean alias = false;
private boolean aliasResolved = false;
private Path targetPath;

/**
Expand Down Expand Up @@ -142,8 +143,8 @@ public static boolean isSameName(Path pathA, Path pathB)

PathResource(URI uri, boolean bypassAllowedSchemeCheck)
{
// normalize to referenced location, Paths.get() doesn't like "/bar/../foo/text.txt" style references
// and will return a Path that will not be found with `Files.exists()` or `Files.isDirectory()`
// Normalize to referenced location, Paths.get() doesn't like "/bar/../foo/text.txt" style references
// and will return a Path that will not be found with `Files.exists()` or `Files.isDirectory()`.
this(Paths.get(uri.normalize()), uri, bypassAllowedSchemeCheck);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
joakime marked this conversation as resolved.
Show resolved Hide resolved
}

Expand All @@ -159,9 +160,12 @@ public static boolean isSameName(Path pathA, Path pathB)
if (!bypassAllowedSchemeCheck && !ALLOWED_SCHEMES.contains(uri.getScheme()))
throw new IllegalArgumentException("not an allowed scheme: " + uri);

String uriString = uri.toASCIIString();
if (Files.isDirectory(path) && !uriString.endsWith(URIUtil.SLASH))
uri = URIUtil.correctFileURI(URI.create(uriString + URIUtil.SLASH));
if (Files.isDirectory(path))
{
String uriString = uri.toASCIIString();
if (!uriString.endsWith(URIUtil.SLASH))
uri = URIUtil.correctFileURI(URI.create(uriString + URIUtil.SLASH));
}

this.path = path;
this.uri = uri;
Expand All @@ -170,36 +174,43 @@ public static boolean isSameName(Path pathA, Path pathB)
@Override
public boolean exists()
{
return Files.exists(targetPath != null ? targetPath : path);
}

@Override
public boolean equals(Object obj)
{
if (this == obj)
if (aliasResolved)
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
return true;
}
if (obj == null)
{
return false;
if (targetPath == null)
return false;
return Files.exists(targetPath);
}
if (getClass() != obj.getClass())
else
{
return false;
// Avoid resolving alias unless necessary to determine existence.
if (Files.exists(path))
return true;
if (isAlias() && targetPath != null)
return Files.exists(targetPath);
}
PathResource other = (PathResource)obj;
return Objects.equals(path, other.path);

return false;
}

/**
* @return the {@link Path} of the resource
*/
@Override
public Path getPath()
{
return path;
}

public Path getTargetPath()
{
checkAlias();
return targetPath;
}

@Override
public URI getTargetURI()
{
Path targetPath = getTargetPath();
return (targetPath == null) ? null : targetPath.toUri();
}

public List<Resource> list()
{
if (!isDirectory())
Expand All @@ -220,6 +231,13 @@ public List<Resource> list()
return List.of(); // empty
}

@Override
public boolean isAlias()
{
checkAlias();
return alias;
}

@Override
public String getName()
{
Expand All @@ -241,90 +259,52 @@ public URI getURI()
return this.uri;
}

@Override
public int hashCode()
{
return Objects.hashCode(path);
}

@Override
public boolean isContainedIn(Resource r)
{
return r.getClass() == PathResource.class && path.startsWith(r.getPath());
}

@Override
public URI getTargetURI()
private void checkAlias()
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to resolveAlias().

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @sbordet, this is a resolveAlias() or resolveTarget() name.

{
if (!targetResolved)
if (!aliasResolved)
{
aliasResolved = true;
targetPath = resolveTargetPath();
targetResolved = true;
}
if (targetPath == null)
return null;
return targetPath.toUri();
}

private Path resolveTargetPath()
{
Path abs = path;

// TODO: is this a valid shortcut?
// If the path doesn't exist, then there's no alias to reference
if (!Files.exists(path))
return null;

/* Catch situation where the Path class has already normalized
* the URI eg. input path "aa./foo.txt"
* from an #resolve(String) is normalized away during
* the creation of a Path object reference.
* If the URI is different from the Path.toUri() then
* we will just use the original URI to construct the
* alias reference Path.
*
* We use the method `toUri(Path)` here, instead of
* Path.toUri() to ensure that the path contains
* a trailing slash if it's a directory, (something
* not all FileSystems seem to support)
*/
if (!URIUtil.equalsIgnoreEncodings(uri, toUri(path)))
{
try
if (targetPath == null)
{
// Use normalized path to get past navigational references like "/bar/../foo/test.txt"
Path ref = Paths.get(uri.normalize());
return ref.toRealPath();
alias = true;
}
catch (IOException ioe)
else
{
// If the toRealPath() call fails, then let
// the alias checking routines continue on
// to other techniques.
LOG.trace("IGNORED", ioe);
/* If the path and targetPath are the same also check
* the Path class has already normalized in the constructor
* from the URI e.g. input path "aa./foo.txt"
* from an #resolve(String) is normalized away during
* the creation of a Path object reference.
* If the URI is different from the Path.toUri() then
* we will just use the original URI to construct the
* alias reference Path.
*
* // On Windows
* PathResource resource = PathResource("C:/temp");
* PathResource child = resource.resolve("aa./foo.txt");
* child.exists() == true
* child.isAlias() == true
* child.toUri() == "file:///C:/temp/aa./foo.txt"
* child.getPath().toUri() == "file:///C:/temp/aa/foo.txt"
* child.getTargetURI() == "file:///C:/temp/aa/foo.txt"
*/
alias = !isSameName(path, targetPath) || !Objects.equals(uri, toUri(targetPath));
}
}
}

if (!abs.isAbsolute())
abs = path.toAbsolutePath();

// Any normalization difference means it's an alias,
// and we don't want to bother further to follow
// symlinks as it's an alias anyway.
Path normal = path.normalize();
if (!isSameName(abs, normal))
return normal;

private Path resolveTargetPath()
{
try
{
if (Files.isSymbolicLink(path))
return path.getParent().resolve(Files.readSymbolicLink(path));
if (Files.exists(path))
{
Path real = abs.toRealPath();
if (!isSameName(abs, real))
return real;
}
return path.normalize().toRealPath();
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
}
catch (IOException e)
{
Expand All @@ -334,6 +314,7 @@ private Path resolveTargetPath()
{
LOG.warn("bad alias ({} {}) for {}", e.getClass().getName(), e.getMessage(), path);
}

return null;
}

Expand Down Expand Up @@ -371,6 +352,25 @@ private static URI toUri(Path path)
return pathUri;
}

@Override
public boolean equals(Object obj)
{
if (this == obj)
return true;
if (obj == null)
return false;
if (getClass() != obj.getClass())
return false;
PathResource other = (PathResource)obj;
return Objects.equals(path, other.path) && Objects.equals(uri, other.uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need equals/hashCode at all?

The comparison on uri is odd, that's a user-space provided field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we need to compare PathResource, I know there are tests for the equals() but not sure if we actually use it anywhere. But to compare these correctly you need to take the uri into account.

You can come up with examples where two different PathResources have the same path field but one is classified as an alias and the other is not. So comparing only on path isn't quite correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will impact the ResourceService cache though.

The same Resource will exist on multiple locations once this new equals/hashcode exists.
Is that an acceptable behavior?

}

@Override
public int hashCode()
{
return Objects.hash(path, uri);
}

@Override
public String toString()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,19 +325,18 @@ public Resource resolve(String subUriPath)
*/
public boolean isAlias()
{
return getTargetURI() != null;
return false;
}

/**
* If this Resource is an alias pointing to a different location,
* return the target location as URI.
* The target URI of the resource. If this Resource is an alias pointing to a different location,
* this will resolve the alias to return the true target URI of the resource.
*
* @return The target URI location of this resource,
* or null if there is no target URI location (eg: not an alias, or a symlink)
* @return The target URI location of this resource, with any aliases resolved.
*/
public URI getTargetURI()
{
return null;
return getURI();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

package org.eclipse.jetty.util.ssl;

import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -48,16 +47,15 @@ public KeyStoreScanner(SslContextFactory sslContextFactory)
{
this.sslContextFactory = sslContextFactory;
Resource keystoreResource = sslContextFactory.getKeyStoreResource();
Path monitoredFile = keystoreResource.getPath();
if (monitoredFile == null || !Files.exists(monitoredFile))
if (!keystoreResource.exists())
throw new IllegalArgumentException("keystore file does not exist");
if (Files.isDirectory(monitoredFile))
if (keystoreResource.isDirectory())
throw new IllegalArgumentException("expected keystore file not directory");

// Use real location of keystore (if different), so that change monitoring can work properly
URI targetURI = keystoreResource.getTargetURI();
if (targetURI != null)
monitoredFile = Paths.get(targetURI);
Path monitoredFile = keystoreResource.getPath();
if (keystoreResource.isAlias())
monitoredFile = Paths.get(keystoreResource.getTargetURI());

keystoreFile = monitoredFile;
if (LOG.isDebugEnabled())
Expand Down
Loading