From e8cabd38d4a1e8ff1831820048bc0bcbd46d1a77 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 5 Jun 2019 18:29:39 +0200 Subject: [PATCH] Delete files when updating parent identities Closes #127 Signed-off-by: Thomas Citharel --- lib/mobilizon/actors/actor.ex | 4 +- lib/mobilizon/actors/actors.ex | 37 ++++++++++ lib/mobilizon/media.ex | 12 +++- lib/mobilizon_web/upload.ex | 10 +++ lib/mobilizon_web/uploaders/local.ex | 48 +++++++++---- lib/mobilizon_web/uploaders/uploader.ex | 7 +- test/mobilizon/actors/actors_test.exs | 94 ++++++++++++++++++++++--- test/mobilizon/media/media_test.exs | 14 ++++ test/mobilizon_web/upload_test.exs | 43 +++++++++++ test/support/factory.ex | 19 ++++- 10 files changed, 263 insertions(+), 25 deletions(-) diff --git a/lib/mobilizon/actors/actor.ex b/lib/mobilizon/actors/actor.ex index 22ca83cb9..8ec368b4d 100644 --- a/lib/mobilizon/actors/actor.ex +++ b/lib/mobilizon/actors/actor.ex @@ -70,8 +70,8 @@ defmodule Mobilizon.Actors.Actor do many_to_many(:memberships, Actor, join_through: Member) belongs_to(:user, User) has_many(:feed_tokens, FeedToken, foreign_key: :actor_id) - embeds_one(:avatar, File) - embeds_one(:banner, File) + embeds_one(:avatar, File, on_replace: :update) + embeds_one(:banner, File, on_replace: :update) timestamps() end diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index e232e5bec..0fe5c6336 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -9,6 +9,8 @@ defmodule Mobilizon.Actors do alias Mobilizon.Repo alias Mobilizon.Actors.{Actor, Bot, Member, Follower} + alias Mobilizon.Media.File + alias Ecto.Multi alias Mobilizon.Service.ActivityPub require Logger @@ -119,9 +121,24 @@ defmodule Mobilizon.Actors do def update_actor(%Actor{} = actor, attrs) do actor |> Actor.changeset(attrs) + |> delete_files_if_media_changed() |> Repo.update() end + defp delete_files_if_media_changed(%Ecto.Changeset{changes: changes} = changeset) do + Enum.each([:avatar, :banner], fn key -> + if Map.has_key?(changes, key) do + with %Ecto.Changeset{changes: %{url: new_url}} <- changes[key], + old_url <- Map.from_struct(changeset.data) |> Map.get(key) |> Map.get(:url), + false <- new_url == old_url do + MobilizonWeb.Upload.remove(old_url) + end + end + end) + + changeset + end + @doc """ Deletes a Actor. @@ -135,6 +152,26 @@ defmodule Mobilizon.Actors do """ @spec delete_actor(Actor.t()) :: {:ok, Actor.t()} | {:error, Ecto.Changeset.t()} + def delete_actor(%Actor{domain: nil} = actor) do + case Multi.new() + |> Multi.delete(:actor, actor) + |> Multi.run(:remove_banner, fn _repo, + %{actor: %Actor{banner: %File{url: url}}} = _picture -> + MobilizonWeb.Upload.remove(url) + end) + |> Multi.run(:remove_avatar, fn _repo, + %{actor: %Actor{avatar: %File{url: url}}} = _picture -> + MobilizonWeb.Upload.remove(url) + end) + |> Repo.transaction() do + {:ok, %{actor: %Actor{} = actor}} -> + {:ok, actor} + + {:error, remove, error, _} when remove in [:remove_banner, :remove_avatar] -> + {:error, error} + end + end + def delete_actor(%Actor{} = actor) do Repo.delete(actor) end diff --git a/lib/mobilizon/media.ex b/lib/mobilizon/media.ex index 4182e0701..850b5274e 100644 --- a/lib/mobilizon/media.ex +++ b/lib/mobilizon/media.ex @@ -7,6 +7,8 @@ defmodule Mobilizon.Media do alias Mobilizon.Repo alias Mobilizon.Media.Picture + alias Mobilizon.Media.File + alias Ecto.Multi @doc false def data() do @@ -97,7 +99,15 @@ defmodule Mobilizon.Media do """ def delete_picture(%Picture{} = picture) do - Repo.delete(picture) + case Multi.new() + |> Multi.delete(:picture, picture) + |> Multi.run(:remove, fn _repo, %{picture: %Picture{file: %File{url: url}}} = _picture -> + MobilizonWeb.Upload.remove(url) + end) + |> Repo.transaction() do + {:ok, %{picture: %Picture{} = picture}} -> {:ok, picture} + {:error, :remove, error, _} -> {:error, error} + end end @doc """ diff --git a/lib/mobilizon_web/upload.ex b/lib/mobilizon_web/upload.ex index 24ad765eb..d98931477 100644 --- a/lib/mobilizon_web/upload.ex +++ b/lib/mobilizon_web/upload.ex @@ -88,6 +88,16 @@ defmodule MobilizonWeb.Upload do end end + def remove(url, opts \\ []) do + with opts <- get_opts(opts), + %URI{path: "/media/" <> path, host: host} <- URI.parse(url), + true <- host == MobilizonWeb.Endpoint.host() do + MobilizonWeb.Uploaders.Uploader.remove_file(opts.uploader, path) + else + %URI{} = _uri -> {:error, "URL doesn't match pattern"} + end + end + def char_unescaped?(char) do URI.char_unreserved?(char) or char == ?/ end diff --git a/lib/mobilizon_web/uploaders/local.ex b/lib/mobilizon_web/uploaders/local.ex index 9308a9ffe..82d87ae56 100644 --- a/lib/mobilizon_web/uploaders/local.ex +++ b/lib/mobilizon_web/uploaders/local.ex @@ -14,18 +14,8 @@ defmodule MobilizonWeb.Uploaders.Local do end def put_file(upload) do - {local_path, file} = - case Enum.reverse(String.split(upload.path, "/", trim: true)) do - [file] -> - {upload_path(), file} - - [file | folders] -> - path = Path.join([upload_path()] ++ Enum.reverse(folders)) - File.mkdir_p!(path) - {path, file} - end - - result_file = Path.join(local_path, file) + {path, file} = local_path(upload.path) + result_file = Path.join(path, file) unless File.exists?(result_file) do File.cp!(upload.tempfile, result_file) @@ -34,6 +24,40 @@ defmodule MobilizonWeb.Uploaders.Local do :ok end + def remove_file(path) do + with {path, file} <- local_path(path), + full_path <- Path.join(path, file), + true <- File.exists?(full_path), + :ok <- File.rm(full_path), + :ok <- remove_folder(path) do + {:ok, path} + else + false -> {:error, "File #{path} doesn't exist"} + end + end + + defp remove_folder(path) do + with {:subfolder, true} <- {:subfolder, path != upload_path()}, + {:empty_folder, {:ok, [] = _files}} <- {:empty_folder, File.ls(path)} do + File.rmdir(path) + else + {:subfolder, _} -> :ok + {:empty_folder, _} -> {:error, "Error: Folder is not empty"} + end + end + + defp local_path(path) do + case Enum.reverse(String.split(path, "/", trim: true)) do + [file] -> + {upload_path(), file} + + [file | folders] -> + path = Path.join([upload_path()] ++ Enum.reverse(folders)) + File.mkdir_p!(path) + {path, file} + end + end + def upload_path do Mobilizon.CommonConfig.get!([__MODULE__, :uploads]) end diff --git a/lib/mobilizon_web/uploaders/uploader.ex b/lib/mobilizon_web/uploaders/uploader.ex index 2154c6d84..1f8d0fda4 100644 --- a/lib/mobilizon_web/uploaders/uploader.ex +++ b/lib/mobilizon_web/uploaders/uploader.ex @@ -35,6 +35,8 @@ defmodule MobilizonWeb.Uploaders.Uploader do @callback put_file(MobilizonWeb.Upload.t()) :: :ok | {:ok, file_spec()} | {:error, String.t()} | :wait_callback + @callback remove_file(file_spec()) :: :ok | {:ok, file_spec()} | {:error, String.t()} + @callback http_callback(Plug.Conn.t(), Map.t()) :: {:ok, Plug.Conn.t()} | {:ok, Plug.Conn.t(), file_spec()} @@ -42,7 +44,6 @@ defmodule MobilizonWeb.Uploaders.Uploader do @optional_callbacks http_callback: 2 @spec put_file(module(), MobilizonWeb.Upload.t()) :: {:ok, file_spec()} | {:error, String.t()} - def put_file(uploader, upload) do case uploader.put_file(upload) do :ok -> {:ok, {:file, upload.path}} @@ -52,6 +53,10 @@ defmodule MobilizonWeb.Uploaders.Uploader do end end + def remove_file(uploader, path) do + uploader.remove_file(path) + end + defp handle_callback(uploader, upload) do :global.register_name({__MODULE__, upload.path}, self()) diff --git a/test/mobilizon/actors/actors_test.exs b/test/mobilizon/actors/actors_test.exs index e1e10dc26..a6d023df0 100644 --- a/test/mobilizon/actors/actors_test.exs +++ b/test/mobilizon/actors/actors_test.exs @@ -4,7 +4,7 @@ defmodule Mobilizon.ActorsTest do alias Mobilizon.Actors alias Mobilizon.Actors.{Actor, Member, Follower, Bot} alias Mobilizon.Users - alias Mobilizon.Media.File + alias Mobilizon.Media.File, as: FileModel import Mobilizon.Factory use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney @@ -97,14 +97,14 @@ defmodule Mobilizon.ActorsTest do id: actor_id, preferred_username: preferred_username, domain: domain, - avatar: %File{name: picture_name} = _picture + avatar: %FileModel{name: picture_name} = _picture } = _actor} = Actors.get_or_fetch_by_url(@remote_account_url) assert picture_name == "avatar" %Actor{ id: actor_found_id, - avatar: %File{name: picture_name} = _picture + avatar: %FileModel{name: picture_name} = _picture } = Actors.get_actor_by_name("#{preferred_username}@#{domain}") assert actor_found_id == actor_id @@ -235,7 +235,8 @@ defmodule Mobilizon.ActorsTest do test "test get_public_key_for_url/1 with remote actor and bad key" do use_cassette "actors/remote_actor_mastodon_tcit_actor_deleted" do - assert Actor.get_public_key_for_url(@remote_account_url) == {:error, :actor_fetch_error} + assert Actor.get_public_key_for_url(@remote_account_url) == + {:error, :actor_fetch_error} end end @@ -257,8 +258,15 @@ defmodule Mobilizon.ActorsTest do assert {:error, %Ecto.Changeset{}} = Actors.create_actor(@invalid_attrs) end - test "update_actor/2 with valid data updates the actor", %{actor: actor} do - assert {:ok, actor} = Actors.update_actor(actor, @update_attrs) + test "update_actor/2 with valid data updates the actor", %{ + actor: %Actor{} = actor + } do + assert {:ok, actor} = + Actors.update_actor( + actor, + @update_attrs + ) + assert %Actor{} = actor assert actor.summary == "some updated description" assert actor.name == "some updated name" @@ -268,15 +276,85 @@ defmodule Mobilizon.ActorsTest do assert actor.preferred_username == "some updated username" end + test "update_actor/2 with valid data updates the actor and it's media files", %{ + actor: %Actor{avatar: %{url: avatar_url}, banner: %{url: banner_url}} = actor + } do + %URI{path: "/media/" <> avatar_path} = URI.parse(avatar_url) + %URI{path: "/media/" <> banner_path} = URI.parse(banner_url) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> avatar_path + ) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> banner_path + ) + + file = %Plug.Upload{ + content_type: "image/jpg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "image.jpg" + } + + {:ok, data} = MobilizonWeb.Upload.store(file) + url = hd(data["url"])["href"] + + assert {:ok, actor} = + Actors.update_actor( + actor, + Map.put(@update_attrs, :avatar, %{name: file.filename, url: url}) + ) + + assert %Actor{} = actor + assert actor.summary == "some updated description" + assert actor.name == "some updated name" + assert actor.domain == "some updated domain" + assert actor.keys == "some updated keys" + refute actor.suspended + assert actor.preferred_username == "some updated username" + + refute File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> avatar_path + ) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> banner_path + ) + end + test "update_actor/2 with invalid data returns error changeset", %{actor: actor} do assert {:error, %Ecto.Changeset{}} = Actors.update_actor(actor, @invalid_attrs) actor_fetched = Actors.get_actor!(actor.id) assert actor = actor_fetched end - test "delete_actor/1 deletes the actor", %{actor: actor} do + test "delete_actor/1 deletes the actor", %{ + actor: %Actor{avatar: %{url: avatar_url}, banner: %{url: banner_url}, id: actor_id} = actor + } do + %URI{path: "/media/" <> avatar_path} = URI.parse(avatar_url) + %URI{path: "/media/" <> banner_path} = URI.parse(banner_url) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> avatar_path + ) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> banner_path + ) + assert {:ok, %Actor{}} = Actors.delete_actor(actor) - assert_raise Ecto.NoResultsError, fn -> Actors.get_actor!(actor.id) end + assert_raise Ecto.NoResultsError, fn -> Actors.get_actor!(actor_id) end + + refute File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> banner_path + ) end test "change_actor/1 returns a actor changeset", %{actor: actor} do diff --git a/test/mobilizon/media/media_test.exs b/test/mobilizon/media/media_test.exs index 04dce976e..6ba645d6a 100644 --- a/test/mobilizon/media/media_test.exs +++ b/test/mobilizon/media/media_test.exs @@ -5,6 +5,7 @@ defmodule Mobilizon.MediaTest do import Mobilizon.Factory describe "media" do + setup [:ensure_local_uploader] alias Mobilizon.Media.Picture @valid_attrs %{ @@ -43,8 +44,21 @@ defmodule Mobilizon.MediaTest do test "delete_picture/1 deletes the picture" do picture = insert(:picture) + + %URI{path: "/media/" <> path} = URI.parse(picture.file.url) + + assert File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> path + ) + assert {:ok, %Picture{}} = Media.delete_picture(picture) assert_raise Ecto.NoResultsError, fn -> Media.get_picture!(picture.id) end + + refute File.exists?( + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/" <> path + ) end test "change_picture/1 returns a picture changeset" do diff --git a/test/mobilizon_web/upload_test.exs b/test/mobilizon_web/upload_test.exs index 39f1bb656..4434d0937 100644 --- a/test/mobilizon_web/upload_test.exs +++ b/test/mobilizon_web/upload_test.exs @@ -173,5 +173,48 @@ defmodule Mobilizon.UploadTest do assert Path.basename(attachment_url["href"]) == "%3A%3F%23%5B%5D%40%21%24%26%5C%27%28%29%2A%2B%2C%3B%3D.jpg" end + + test "upload and delete successfully a file" do + {path, url} = upload() + assert File.exists?(path) + assert {:ok, _} = Upload.remove(url) + refute File.exists?(path) + path = path |> Path.split() |> Enum.reverse() |> tl |> Enum.reverse() |> Path.join() + refute File.exists?(path) + end + + test "delete a not existing file" do + file = + Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> + "/not_existing/definitely.jpg" + + refute File.exists?(file) + + assert {:error, "File not_existing/definitely.jpg doesn't exist"} = + Upload.remove("https://mobilizon.test/media/not_existing/definitely.jpg") + end + end + + defp upload() do + File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") + + file = %Plug.Upload{ + content_type: "image/jpg", + path: Path.absname("test/fixtures/image_tmp.jpg"), + filename: "image.jpg" + } + + {:ok, data} = Upload.store(file) + + assert %{ + "url" => [%{"href" => url, "mediaType" => "image/jpeg"}], + "size" => 13_227, + "type" => "Image" + } = data + + assert String.starts_with?(url, MobilizonWeb.Endpoint.url() <> "/media/") + + %URI{path: "/media/" <> path} = URI.parse(url) + {Mobilizon.CommonConfig.get!([MobilizonWeb.Uploaders.Local, :uploads]) <> "/" <> path, url} end end diff --git a/test/support/factory.ex b/test/support/factory.ex index 5362bbe9c..002566d2f 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -7,6 +7,7 @@ defmodule Mobilizon.Factory do alias Mobilizon.Actors.Actor alias MobilizonWeb.Router.Helpers, as: Routes alias MobilizonWeb.Endpoint + alias MobilizonWeb.Upload def user_factory do %Mobilizon.Users.User{ @@ -171,9 +172,25 @@ defmodule Mobilizon.Factory do end def file_factory do + File.cp!("test/fixtures/image.jpg", "test/fixtures/image_tmp.jpg") + + file = %Plug.Upload{ + content_type: "image/jpg", + path: Path.absname("test/fixtures/image_tmp.jpg"), + filename: "image.jpg" + } + + {:ok, data} = Upload.store(file) + + %{ + "url" => [%{"href" => url, "mediaType" => "image/jpeg"}], + "size" => 13_227, + "type" => "Image" + } = data + %Mobilizon.Media.File{ name: "My Picture", - url: MobilizonWeb.Endpoint.url() <> "/uploads/something", + url: url, content_type: "image/png", size: 13_120 }