Catch trying to create a directory at/under non-directory

coder · code-asher · Apr 24, 2023 · Apr 18, 2023 · Apr 19, 2023 · Apr 18, 2023
commit cdc569a5789444a522f085a451cccc9fe62143f5
diff --git a/src/main/kotlin/com/coder/gateway/CoderSettingsConfigurable.kt b/src/main/kotlin/com/coder/gateway/CoderSettingsConfigurable.kt
@@ -1,7 +1,7 @@
 package com.coder.gateway
 
 import com.coder.gateway.sdk.CoderCLIManager
-import com.coder.gateway.sdk.canWrite
+import com.coder.gateway.sdk.canCreateDirectory
 import com.coder.gateway.services.CoderSettingsState
 import com.intellij.openapi.components.service
 import com.intellij.openapi.options.BoundConfigurable
@@ -45,8 +45,8 @@ class CoderSettingsConfigurable : BoundConfigurable("Coder") {
     }
 
     private fun validateBinaryDestination(): ValidationInfoBuilder.(JBTextField) -> ValidationInfo? = {
-        if (it.text.isNotBlank() && !Path.of(it.text).canWrite()) {
-            error("Cannot write to this path")
+        if (it.text.isNotBlank() && !Path.of(it.text).canCreateDirectory()) {
+            error("Cannot create this directory")
         } else {
             null
         }

diff --git a/src/main/kotlin/com/coder/gateway/sdk/PathExtensions.kt b/src/main/kotlin/com/coder/gateway/sdk/PathExtensions.kt
@@ -4,17 +4,23 @@ import java.nio.file.Files
 import java.nio.file.Path
 
 /**
- * Return true if the path can be created.
+ * Return true if a directory can be created at the specified path or if one
+ * already exists and we can write into it.
  *
- * Unlike File.canWrite() or Files.isWritable() the file does not need to exist;
- * it only needs a writable parent.
+ * Unlike File.canWrite() or Files.isWritable() the directory does not need to
+ * exist; it only needs a writable parent and the target needs to be
+ * non-existent or a directory (not a regular file or nested under one).
+ *
+ * This check is deficient on Windows since directories that have write
+ * permissions but are read-only will still return true.
  */
-fun Path.canWrite(): Boolean {
+fun Path.canCreateDirectory(): Boolean {
     var current: Path? = this.toAbsolutePath()
     while (current != null && !Files.exists(current)) {
         current = current.parent
     }
     // On Windows File.canWrite() only checks read-only while Files.isWritable()
-    // actually checks permissions.
-    return current != null && Files.isWritable(current)
+    // also checks permissions.  For directories neither of them seem to care if
+    // it is read-only.
+    return current != null && Files.isWritable(current) && Files.isDirectory(current)
 }
diff --git a/src/test/groovy/PathExtensionsTest.groovy b/src/test/groovy/PathExtensionsTest.groovy
@@ -12,35 +12,47 @@ class PathExtensionsTest extends Specification {
     @Shared
     private Path tmpdir = Path.of(System.getProperty("java.io.tmpdir"))
     @Shared
-    private Path unwritable = tmpdir.resolve("coder-gateway-test/path-extensions/unwritable")
+    private Path unwritableFile = tmpdir.resolve("coder-gateway-test/path-extensions/unwritable/file")
+    @Shared
+    private Path writableFile = tmpdir.resolve("coder-gateway-test/path-extensions/writable-file")
 
     void setupSpec() {
-        if (unwritable.parent.toFile().exists()) {
-            unwritable.parent.toFile().setWritable(true)
-            unwritable.parent.toFile().deleteDir()
+        // TODO: On Windows setWritable() only sets read-only; how do we set
+        // actual permissions?  Initially I tried an existing dir like WINDIR
+        // which worked locally but in CI that is writable for some reason.
+        if (unwritableFile.parent.toFile().exists()) {
+            unwritableFile.parent.toFile().setWritable(true)
+            unwritableFile.parent.toFile().deleteDir()
         }
-        Files.createDirectories(unwritable.parent)
-        unwritable.toFile().write("text")
-        unwritable.toFile().setWritable(false)
-        unwritable.parent.toFile().setWritable(false)
+        Files.createDirectories(unwritableFile.parent)
+        unwritableFile.toFile().write("text")
+        writableFile.toFile().write("text")
+        unwritableFile.toFile().setWritable(false)
+        unwritableFile.parent.toFile().setWritable(false)
     }
 
-    def "canWrite"() {
+    def "canCreateDirectory"() {
         expect:
         use(PathExtensionsKt) {
-            path.canWrite() == expected
+            path.canCreateDirectory() == expected
         }
 
         where:
-        path                                            | expected
-        unwritable                                      | false
-        unwritable.resolve("probably/nonexistent")      | false
-        Path.of("relative to project")                  | true
-        tmpdir.resolve("./foo/bar/../..")               | true
-        tmpdir                                          | true
-        tmpdir.resolve("some/nested/non-existent/path") | true
-        tmpdir.resolve("with space")                    | true
-        CoderCLIManager.getConfigDir()                  | true
-        CoderCLIManager.getDataDir()                    | true
+        path                                                                 | expected
+        unwritableFile                                                       | false
+        unwritableFile.resolve("probably/nonexistent")                       | false
+        // TODO: Java reports read-only directories on Windows as writable.
+        unwritableFile.parent.resolve("probably/nonexistent")                | System.getProperty("os.name").toLowerCase().contains("windows")
+        writableFile                                                         | false
+        writableFile.parent                                                  | true
+        writableFile.resolve("nested/under/file")                            | false
+        writableFile.parent.resolve("nested/under/dir")                      | true
+        Path.of("relative to project")                                       | true
+        tmpdir.resolve("./foo/bar/../../coder-gateway-test/path-extensions") | true
+        tmpdir                                                               | true
+        tmpdir.resolve("some/nested/non-existent/path")                      | true
+        tmpdir.resolve("with space")                                         | true
+        CoderCLIManager.getConfigDir()                                       | true
+        CoderCLIManager.getDataDir()                                         | true
     }
 }