From 122aa6137f084715f72b93515381b466645e65a7 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sun, 24 Mar 2024 14:43:38 -0700 Subject: [PATCH] Added startup permissions check with more helpful message --- lib/pinchflat/boot/pre_job_startup_tasks.ex | 17 ------ .../filesystem/filesystem_helpers.ex | 21 +++++-- lib/pinchflat/release.ex | 61 +++++++++++++++++++ rel/overlays/bin/check_file_permissions | 3 + rel/overlays/bin/docker_start | 7 +++ .../filesystem/filesystem_helpers_test.exs | 29 ++++++++- 6 files changed, 113 insertions(+), 25 deletions(-) create mode 100755 rel/overlays/bin/check_file_permissions diff --git a/lib/pinchflat/boot/pre_job_startup_tasks.ex b/lib/pinchflat/boot/pre_job_startup_tasks.ex index 8572d8a..27b8e1b 100644 --- a/lib/pinchflat/boot/pre_job_startup_tasks.ex +++ b/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 diff --git a/lib/pinchflat/filesystem/filesystem_helpers.ex b/lib/pinchflat/filesystem/filesystem_helpers.ex index a66bf23..349c4b7 100644 --- a/lib/pinchflat/filesystem/filesystem_helpers.ex +++ b/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 """ diff --git a/lib/pinchflat/release.ex b/lib/pinchflat/release.ex index f98d10d..c47200a 100644 --- a/lib/pinchflat/release.ex +++ b/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 + chmod 755 -R + + 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 + + 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 diff --git a/rel/overlays/bin/check_file_permissions b/rel/overlays/bin/check_file_permissions new file mode 100755 index 0000000..7dc6195 --- /dev/null +++ b/rel/overlays/bin/check_file_permissions @@ -0,0 +1,3 @@ +#!/bin/sh +cd -P -- "$(dirname -- "$0")" +exec ./pinchflat eval Pinchflat.Release.check_file_permissions diff --git a/rel/overlays/bin/docker_start b/rel/overlays/bin/docker_start index 2a0b4f6..4f2d132 100755 --- a/rel/overlays/bin/docker_start +++ b/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")" diff --git a/test/pinchflat/filesystem/filesystem_helpers_test.exs b/test/pinchflat/filesystem/filesystem_helpers_test.exs index 05b7722..4fd18ad 100644 --- a/test/pinchflat/filesystem/filesystem_helpers_test.exs +++ b/test/pinchflat/filesystem/filesystem_helpers_test.exs @@ -34,12 +34,12 @@ defmodule Pinchflat.Filesystem.FilesystemHelpersTest do end end - describe "write_p!/3" do + 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 :ok = FilesystemHelpers.write_p(filepath, content) assert File.read!(filepath) == content File.rm!(filepath) @@ -50,7 +50,30 @@ defmodule Pinchflat.Filesystem.FilesystemHelpersTest do filepath = Path.join([tmpfile_directory, "foo", "bar", "file.json"]) content = "{}" - assert :ok = FilesystemHelpers.write_p!(filepath, 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) + 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)