Improve preflight file permissions check (#114)

* Added startup permissions check with more helpful message

* Updated README

* Fixes faulty test

README.md

@@ -61,6 +61,22 @@ If it doesn't work for your use case, please make a feature request! You can als
 
 Simply search for Pinchflat in the Community Apps store!
 
+### Portainer
+
+Docker Compose file:
+
+```yaml
+version: '3'
+services:
+  pinchflat:
+    image: keglin/pinchflat:latest
+    ports:
+      - '8945:8945'
+    volumes:
+      - /host/path/to/config:/config
+      - /host/path/to/downloads:/downloads
+```
+
 ### Docker
 
 1. Create two directories on your host machine: one for storing config and one for storing downloaded media. Make sure they're both writable by the user running the Docker container.
@@ -79,7 +95,11 @@ docker run \
   keglin/pinchflat:latest
 ```
 
-NOTE: it's recommended to not run the container as root. Doing so can create permission issues if other apps need to work with the downloaded media. If you need to run any command as root, you can run `su` from the container's shell as there is no password set for the root user.
+### IMPORTANT: File permissions
+
+You _must_ ensure the host directories you've mounted are writable by the user running the Docker container. If you get a permission error follow the steps it suggests. See [#106](https://github.com/kieraneglin/pinchflat/issues/106) for more.
+
+It's recommended to not run the container as root. Doing so can create permission issues if other apps need to work with the downloaded media. If you need to run any command as root, you can run `su` from the container's shell as there is no password set for the root user.
 
 ## Authentication
 

lib/pinchflat/boot/pre_job_startup_tasks.ex

@@ -14,7 +14,6 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
 
   alias Pinchflat.Repo
   alias Pinchflat.Settings
-  alias Pinchflat.Filesystem.FilesystemHelpers
 
   def start_link(opts \\ []) do
     GenServer.start_link(__MODULE__, %{}, opts)
@@ -33,7 +32,6 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
   def init(state) do
     reset_executing_jobs()
     apply_default_settings()
-    ensure_directories_are_writeable()
     rename_old_job_workers()
 
     {:ok, state}
@@ -56,21 +54,6 @@ defmodule Pinchflat.Boot.PreJobStartupTasks do
     Settings.fetch!(:pro_enabled, false)
   end
 
-  defp ensure_directories_are_writeable do
-    directories = [
-      Application.get_env(:pinchflat, :media_directory),
-      Application.get_env(:pinchflat, :tmpfile_directory),
-      Application.get_env(:pinchflat, :metadata_directory)
-    ]
-
-    Enum.each(directories, fn dir ->
-      file = Path.join([dir, ".keep"])
-
-      # This will fail if the directory is not writeable, stopping boot
-      FilesystemHelpers.write_p!(file, "")
-    end)
-  end
-
   # As part of a large refactor, I ended up moving a bunch of workers around. This
   # is a problem because the workers are stored in the database and the runner
   # will try to run the OLD jobs. This is also why these tasks run before the job

lib/pinchflat/filesystem/filesystem_helpers.ex

@@ -20,6 +20,21 @@ defmodule Pinchflat.Filesystem.FilesystemHelpers do
     filepath
   end
 
+  @doc """
+  Writes content to a file, creating directories as needed.
+  Takes the same args as File.write/3.
+
+  Returns :ok | {:error, any()}
+  """
+  def write_p(file, content, modes \\ []) do
+    dirname = Path.dirname(file)
+
+    case File.mkdir_p(dirname) do
+      :ok -> File.write(file, content, modes)
+      err -> err
+    end
+  end
+
   @doc """
   Writes content to a file, creating directories as needed.
   Takes the same args as File.write!/3.
@@ -27,11 +42,7 @@ defmodule Pinchflat.Filesystem.FilesystemHelpers do
   Returns :ok | raises on error
   """
   def write_p!(filepath, content, modes \\ []) do
-    filepath
-    |> Path.dirname()
-    |> File.mkdir_p!()
-
-    File.write!(filepath, content, modes)
+    :ok = write_p(filepath, content, modes)
   end
 
   @doc """

lib/pinchflat/release.ex

@@ -5,6 +5,10 @@ defmodule Pinchflat.Release do
   """
   @app :pinchflat
 
+  require Logger
+
+  alias Pinchflat.Filesystem.FilesystemHelpers
+
   def migrate do
     load_app()
 
@@ -18,6 +22,36 @@ defmodule Pinchflat.Release do
     {:ok, _, _} = Ecto.Migrator.with_repo(repo, &Ecto.Migrator.run(&1, :down, to: version))
   end
 
+  def check_file_permissions do
+    load_app()
+
+    directories = [
+      "/config",
+      "/downloads",
+      Application.get_env(:pinchflat, :media_directory),
+      Application.get_env(:pinchflat, :tmpfile_directory),
+      Application.get_env(:pinchflat, :metadata_directory)
+    ]
+
+    Enum.each(directories, fn dir ->
+      Logger.info("Checking permissions for #{dir}")
+      filepath = Path.join([dir, ".keep"])
+
+      case FilesystemHelpers.write_p(filepath, "") do
+        :ok ->
+          Logger.info("Permissions OK")
+
+        {:error, :eacces} ->
+          Logger.error(permission_denied_screed(dir))
+          raise "Permission denied"
+
+        err ->
+          Logger.error("Permissions check failed: #{inspect(err)}")
+          raise "Unknown error"
+      end
+    end)
+  end
+
   defp repos do
     Application.fetch_env!(@app, :ecto_repos)
   end
@@ -25,4 +59,31 @@ defmodule Pinchflat.Release do
   defp load_app do
     Application.load(@app)
   end
+
+  defp permission_denied_screed(dir) do
+    """
+    The directory "#{dir}" is not writeable by the Docker container.
+
+    Please ensure that the directory exists and is writeable by the Docker
+    container. All setups are different, but you may be able to run something
+    like this on the *host*:
+
+      chown nobody -R <host path that maps to #{dir}>
+      chmod 755 -R <host path that maps to #{dir}>
+
+    Swapping in your real host path. Then, you should set the user running
+    this container by editing your `docker run` command like so:
+
+        docker run --user 99:100 <rest of the command>
+
+    Or adding `user: '99:100'` to the Pinchflat service of your Docker Compose
+    file. Again, there are many ways to do this depending on your setup and
+    this is just one example. See issue #106 in the Pinchflat Github for more.
+
+    No matter the case, this _is_ a permissions error and allowing the container
+    to write to the directory is the only way to fix it. It is not recommended
+    to run the container as `root` because files created by Pinchflat may not
+    be accessible to other apps that want to modify them.
+    """
+  end
 end

rel/overlays/bin/check_file_permissions

@@ -0,0 +1,3 @@
+#!/bin/sh
+cd -P -- "$(dirname -- "$0")"
+exec ./pinchflat eval Pinchflat.Release.check_file_permissions

rel/overlays/bin/docker_start

@@ -1,4 +1,11 @@
 #!/bin/sh
+/app/bin/check_file_permissions
+
+if [ $? -ne 0 ]; then
+  echo "Filesystem error. Exiting."
+  exit 1
+fi
+
 /app/bin/migrate
 
 cd -P -- "$(dirname -- "$0")"

test/pinchflat/filesystem/filesystem_helpers_test.exs

@@ -34,6 +34,29 @@ defmodule Pinchflat.Filesystem.FilesystemHelpersTest do
     end
   end
 
+  describe "write_p/3" do
+    test "writes content to a file" do
+      filepath = FilesystemHelpers.generate_metadata_tmpfile(:json)
+      content = "{}"
+
+      assert :ok = FilesystemHelpers.write_p(filepath, content)
+      assert File.read!(filepath) == content
+
+      File.rm!(filepath)
+    end
+
+    test "creates directories as needed" do
+      tmpfile_directory = Application.get_env(:pinchflat, :tmpfile_directory)
+      filepath = Path.join([tmpfile_directory, "foo", "bar", "file.json"])
+      content = "{}"
+
+      assert :ok = FilesystemHelpers.write_p(filepath, content)
+      assert File.read!(filepath) == content
+
+      File.rm!(filepath)
+    end
+  end
+
   describe "write_p!/3" do
     test "writes content to a file" do
       filepath = FilesystemHelpers.generate_metadata_tmpfile(:json)