[9.0] Remove duplicate paths (including exclusive) in FileAccessTree (elastic#123776 and elastic#124023) (elastic#123924)

* remove duplicate paths in FileAccessTree (elastic#123776)

Co-authored-by: Lorenzo Dematté <[email protected]>
Co-authored-by: Patrick Doyle <[email protected]>

* Remove duplicate exclusive paths (elastic#124023)

* Remove duplicate exclusive paths

* Normalize paths in tests to support Windows

* Remove withMode

---------

Co-authored-by: Jack Conradson <[email protected]>
Co-authored-by: Lorenzo Dematté <[email protected]>

Author: 3 people

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

@@ -26,8 +26,10 @@
 import java.util.Objects;
 import java.util.function.BiConsumer;
 
+import static java.util.Comparator.comparing;
 import static org.elasticsearch.core.PathUtils.getDefaultFileSystem;
 import static org.elasticsearch.entitlement.runtime.policy.FileUtils.PATH_ORDER;
+import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ_WRITE;
 
 public final class FileAccessTree {
 
@@ -59,8 +61,7 @@ static List<ExclusivePath> buildExclusivePathList(List<ExclusiveFileEntitlement>
                 }
             }
         }
-        exclusivePaths.sort((ep1, ep2) -> PATH_ORDER.compare(ep1.path(), ep2.path()));
-        return exclusivePaths;
+        return exclusivePaths.stream().sorted(comparing(ExclusivePath::path, PATH_ORDER)).distinct().toList();
     }
 
     static void validateExclusivePaths(List<ExclusivePath> exclusivePaths) {
@@ -103,7 +104,7 @@ private FileAccessTree(
         List<String> writePaths = new ArrayList<>();
         BiConsumer<Path, Mode> addPath = (path, mode) -> {
             var normalized = normalizePath(path);
-            if (mode == Mode.READ_WRITE) {
+            if (mode == READ_WRITE) {
                 writePaths.add(normalized);
             }
             readPaths.add(normalized);
@@ -139,7 +140,7 @@ private FileAccessTree(
         }
 
         // everything has access to the temp dir, config dir and the jdk
-        addPathAndMaybeLink.accept(pathLookup.tempDir(), Mode.READ_WRITE);
+        addPathAndMaybeLink.accept(pathLookup.tempDir(), READ_WRITE);
         // TODO: this grants read access to the config dir for all modules until explicit read entitlements can be added
         addPathAndMaybeLink.accept(pathLookup.configDir(), Mode.READ);
 
@@ -156,14 +157,15 @@ private FileAccessTree(
         this.writePaths = pruneSortedPaths(writePaths).toArray(new String[0]);
     }
 
-    private static List<String> pruneSortedPaths(List<String> paths) {
+    // package private for testing
+    static List<String> pruneSortedPaths(List<String> paths) {
         List<String> prunedReadPaths = new ArrayList<>();
         if (paths.isEmpty() == false) {
             String currentPath = paths.get(0);
             prunedReadPaths.add(currentPath);
             for (int i = 1; i < paths.size(); ++i) {
                 String nextPath = paths.get(i);
-                if (isParent(currentPath, nextPath) == false) {
+                if (currentPath.equals(nextPath) == false && isParent(currentPath, nextPath) == false) {
                     prunedReadPaths.add(nextPath);
                     currentPath = nextPath;
                 }

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java

@@ -11,8 +11,10 @@
 
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.SuppressForbidden;
+import org.elasticsearch.entitlement.runtime.policy.FileAccessTree.ExclusiveFileEntitlement;
 import org.elasticsearch.entitlement.runtime.policy.FileAccessTree.ExclusivePath;
 import org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement;
+import org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.FileData;
 import org.elasticsearch.test.ESTestCase;
 import org.junit.BeforeClass;
 
@@ -26,6 +28,11 @@
 import java.util.Map;
 
 import static org.elasticsearch.core.PathUtils.getDefaultFileSystem;
+import static org.elasticsearch.entitlement.runtime.policy.FileAccessTree.buildExclusivePathList;
+import static org.elasticsearch.entitlement.runtime.policy.FileAccessTree.normalizePath;
+import static org.elasticsearch.entitlement.runtime.policy.Platform.WINDOWS;
+import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ;
+import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ_WRITE;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 
@@ -195,7 +202,7 @@ public void testNormalizePath() {
     }
 
     public void testNormalizeDirectorySeparatorWindows() {
-        assumeTrue("normalization of windows paths", Platform.WINDOWS.isCurrent());
+        assumeTrue("normalization of windows paths", WINDOWS.isCurrent());
 
         assertThat(FileAccessTree.normalizePath(Path.of("C:\\a\\b")), equalTo("C:\\a\\b"));
         assertThat(FileAccessTree.normalizePath(Path.of("C:/a.xml")), equalTo("C:\\a.xml"));
@@ -254,7 +261,7 @@ public void testJdkAccess() {
 
     @SuppressForbidden(reason = "don't care about the directory location in tests")
     public void testFollowLinks() throws IOException {
-        assumeFalse("Windows requires admin right to create symbolic links", Platform.WINDOWS.isCurrent());
+        assumeFalse("Windows requires admin right to create symbolic links", WINDOWS.isCurrent());
 
         Path baseSourceDir = Files.createTempDirectory("fileaccess_source");
         Path source1Dir = baseSourceDir.resolve("source1");
@@ -347,18 +354,102 @@ public void testInvalidExclusiveAccess() {
         assertThat(tree.canWrite(path("a")), is(false));
     }
 
+    public void testDuplicatePrunedPaths() {
+        List<String> inputPaths = List.of("/a", "/a", "/a/b", "/a/b", "/b/c", "b/c/d", "b/c/d", "b/c/d", "e/f", "e/f");
+        List<String> outputPaths = List.of("/a", "/b/c", "b/c/d", "e/f");
+        var actual = FileAccessTree.pruneSortedPaths(inputPaths.stream().map(p -> normalizePath(path(p))).toList());
+        var expected = outputPaths.stream().map(p -> normalizePath(path(p))).toList();
+        assertEquals(expected, actual);
+    }
+
+    public void testDuplicateExclusivePaths() {
+        // Bunch o' handy definitions
+        var originalFileData = FileData.ofPath(path("/a/b"), READ).withExclusive(true);
+        var fileDataWithWriteMode = FileData.ofPath(path("/a/b"), READ_WRITE).withExclusive(true);
+        var original = new ExclusiveFileEntitlement("component1", "module1", new FilesEntitlement(List.of(originalFileData)));
+        var differentComponent = new ExclusiveFileEntitlement("component2", original.moduleName(), original.filesEntitlement());
+        var differentModule = new ExclusiveFileEntitlement(original.componentName(), "module2", original.filesEntitlement());
+        var differentPath = new ExclusiveFileEntitlement(
+            original.componentName(),
+            original.moduleName(),
+            new FilesEntitlement(
+                List.of(FileData.ofPath(path("/c/d"), originalFileData.mode()).withExclusive(originalFileData.exclusive()))
+            )
+        );
+        var differentMode = new ExclusiveFileEntitlement(
+            original.componentName(),
+            original.moduleName(),
+            new FilesEntitlement(List.of(fileDataWithWriteMode))
+        );
+        var differentPlatform = new ExclusiveFileEntitlement(
+            original.componentName(),
+            original.moduleName(),
+            new FilesEntitlement(List.of(originalFileData.withPlatform(WINDOWS)))
+        );
+        var originalExclusivePath = new ExclusivePath("component1", "module1", normalizePath(path("/a/b")));
+
+        // Some basic tests
+
+        assertEquals(
+            "Single element should trivially work",
+            List.of(originalExclusivePath),
+            buildExclusivePathList(List.of(original), TEST_PATH_LOOKUP)
+        );
+        assertEquals(
+            "Two identical elements should be combined",
+            List.of(originalExclusivePath),
+            buildExclusivePathList(List.of(original, original), TEST_PATH_LOOKUP)
+        );
+
+        // Don't merge things we shouldn't
+
+        var distinctEntitlements = List.of(original, differentComponent, differentModule, differentPath);
+        var distinctPaths = List.of(
+            originalExclusivePath,
+            new ExclusivePath("component2", original.moduleName(), originalExclusivePath.path()),
+            new ExclusivePath(original.componentName(), "module2", originalExclusivePath.path()),
+            new ExclusivePath(original.componentName(), original.moduleName(), normalizePath(path("/c/d")))
+        );
+        assertEquals(
+            "Distinct elements should not be combined",
+            distinctPaths,
+            buildExclusivePathList(distinctEntitlements, TEST_PATH_LOOKUP)
+        );
+
+        // Do merge things we should
+
+        List<ExclusiveFileEntitlement> interleavedEntitlements = new ArrayList<>();
+        distinctEntitlements.forEach(e -> {
+            interleavedEntitlements.add(e);
+            interleavedEntitlements.add(original);
+        });
+        assertEquals(
+            "Identical elements should be combined wherever they are in the list",
+            distinctPaths,
+            buildExclusivePathList(interleavedEntitlements, TEST_PATH_LOOKUP)
+        );
+
+        var equivalentEntitlements = List.of(original, differentMode, differentPlatform);
+        var equivalentPaths = List.of(originalExclusivePath);
+        assertEquals(
+            "Exclusive paths should be combined even if the entitlements are different",
+            equivalentPaths,
+            buildExclusivePathList(equivalentEntitlements, TEST_PATH_LOOKUP)
+        );
+    }
+
     public void testWindowsAbsolutPathAccess() {
-        assumeTrue("Specific to windows for paths with a root (DOS or UNC)", Platform.WINDOWS.isCurrent());
+        assumeTrue("Specific to windows for paths with a root (DOS or UNC)", WINDOWS.isCurrent());
 
         var fileAccessTree = FileAccessTree.of(
             "test",
             "test",
             new FilesEntitlement(
                 List.of(
-                    FilesEntitlement.FileData.ofPath(Path.of("\\\\.\\pipe\\"), FilesEntitlement.Mode.READ),
-                    FilesEntitlement.FileData.ofPath(Path.of("D:\\.gradle"), FilesEntitlement.Mode.READ),
-                    FilesEntitlement.FileData.ofPath(Path.of("D:\\foo"), FilesEntitlement.Mode.READ),
-                    FilesEntitlement.FileData.ofPath(Path.of("C:\\foo"), FilesEntitlement.Mode.READ_WRITE)
+                    FileData.ofPath(Path.of("\\\\.\\pipe\\"), READ),
+                    FileData.ofPath(Path.of("D:\\.gradle"), READ),
+                    FileData.ofPath(Path.of("D:\\foo"), READ),
+                    FileData.ofPath(Path.of("C:\\foo"), FilesEntitlement.Mode.READ_WRITE)
                 )
             ),
             TEST_PATH_LOOKUP,
@@ -394,7 +485,7 @@ static FilesEntitlement entitlement(Map<String, String> value) {
     static List<ExclusivePath> exclusivePaths(String componentName, String moduleName, String... paths) {
         List<ExclusivePath> exclusivePaths = new ArrayList<>();
         for (String path : paths) {
-            exclusivePaths.add(new ExclusivePath(componentName, moduleName, path(path).toString()));
+            exclusivePaths.add(new ExclusivePath(componentName, moduleName, normalizePath(path(path))));
         }
         return exclusivePaths;
     }