From 61198cb639945137e84ce4d44e3571c8c70647c5 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 20 Aug 2021 18:48:23 +0200 Subject: [PATCH 1/8] Fix config onboarding after LDAP initial connexion Closes #840 Signed-off-by: Thomas Citharel --- js/src/components/Account/ProfileOnboarding.vue | 1 + js/src/views/Home.vue | 1 + 2 files changed, 2 insertions(+) diff --git a/js/src/components/Account/ProfileOnboarding.vue b/js/src/components/Account/ProfileOnboarding.vue index 5f59e751d..43448fc1b 100644 --- a/js/src/components/Account/ProfileOnboarding.vue +++ b/js/src/components/Account/ProfileOnboarding.vue @@ -21,6 +21,7 @@ }} Date: Fri, 20 Aug 2021 18:48:40 +0200 Subject: [PATCH 2/8] Delete current actor ID as well from local storage when unlogging Signed-off-by: Thomas Citharel --- js/src/utils/auth.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/js/src/utils/auth.ts b/js/src/utils/auth.ts index 4c61a87a5..534456782 100644 --- a/js/src/utils/auth.ts +++ b/js/src/utils/auth.ts @@ -47,6 +47,7 @@ export function deleteUserData(): void { AUTH_USER_EMAIL, AUTH_ACCESS_TOKEN, AUTH_REFRESH_TOKEN, + AUTH_USER_ACTOR_ID, AUTH_USER_ROLE, ].forEach((key) => { localStorage.removeItem(key); From 8eacf29705e82ef443c5135857ce20eef1314df9 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 20 Aug 2021 19:05:17 +0200 Subject: [PATCH 3/8] Fix events pagination on tags page Not a perfect fix because of a blinking issue, but works properly Signed-off-by: Thomas Citharel --- js/src/views/Search.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/views/Search.vue b/js/src/views/Search.vue index 5f2eb36e8..f935df5a5 100644 --- a/js/src/views/Search.vue +++ b/js/src/views/Search.vue @@ -381,7 +381,7 @@ export default class Search extends Vue { set eventPage(page: number) { this.$router.push({ - name: RouteName.SEARCH, + name: this.$route.name || RouteName.SEARCH, query: { ...this.$route.query, eventPage: page.toString() }, }); } From 241710807c4c36df4624732759aa658a21e0e5f7 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Sun, 22 Aug 2021 16:17:20 +0200 Subject: [PATCH 4/8] Fixed deduplicated files from orphan media being deleted as well Happens when a file is uploaded, then orphaned, and a similar file is used somewhere. The CleanMedia job service didn't consider that case Signed-off-by: Thomas Citharel --- .gitignore | 1 + .../fix_unattached_media_in_body.ex | 107 ------------------ lib/mix/tasks/mobilizon/media/clean_orphan.ex | 4 +- lib/mobilizon/medias/medias.ex | 23 ++-- lib/mobilizon/storage/custom_functions.ex | 10 ++ lib/service/clean_orphan_media.ex | 43 ++++++- test/service/clean_orphan_media_test.exs | 6 +- test/support/factory.ex | 2 +- test/tasks/media/clean_orphan_test.exs | 39 ++++++- 9 files changed, 98 insertions(+), 137 deletions(-) delete mode 100644 lib/mix/tasks/mobilizon/maintenance/fix_unattached_media_in_body.ex create mode 100644 lib/mobilizon/storage/custom_functions.ex diff --git a/.gitignore b/.gitignore index b4b15332c..5b7ebafb6 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,7 @@ priv/cert/ cover/ site/ test/fixtures/image_tmp.jpg +test/fixtures/picture_tmp.png test/fixtures/DSCN0010_tmp.jpg test/uploads/ uploads/* diff --git a/lib/mix/tasks/mobilizon/maintenance/fix_unattached_media_in_body.ex b/lib/mix/tasks/mobilizon/maintenance/fix_unattached_media_in_body.ex deleted file mode 100644 index d1934bad7..000000000 --- a/lib/mix/tasks/mobilizon/maintenance/fix_unattached_media_in_body.ex +++ /dev/null @@ -1,107 +0,0 @@ -defmodule Mix.Tasks.Mobilizon.Maintenance.FixUnattachedMediaInBody do - @moduledoc """ - Task to reattach media files that were added in event, post or comment bodies without being attached to their entities. - - This task should only be run once. - """ - use Mix.Task - - import Mix.Tasks.Mobilizon.Common - alias Mobilizon.{Discussions, Events, Medias, Posts} - alias Mobilizon.Discussions.Comment - alias Mobilizon.Events.Event - alias Mobilizon.Posts.Post - alias Mobilizon.Storage.Repo - require Logger - - @preferred_cli_env "prod" - - # TODO: Remove me in Mobilizon 1.2 - - @shortdoc "Reattaches inline media from events and posts" - def run([]) do - start_mobilizon() - - shell_info("Going to extract pictures from events") - extract_inline_pictures_from_bodies(Event) - shell_info("Going to extract pictures from posts") - extract_inline_pictures_from_bodies(Post) - shell_info("Going to extract pictures from comments") - extract_inline_pictures_from_bodies(Comment) - end - - defp extract_inline_pictures_from_bodies(entity) do - Repo.transaction( - fn -> - entity - |> Repo.stream() - |> Stream.map(&extract_pictures(&1)) - |> Stream.map(fn {entity, pics} -> save_entity(entity, pics) end) - |> Stream.run() - end, - timeout: :infinity - ) - end - - defp extract_pictures(entity) do - extracted_pictures = entity |> get_body() |> parse_body() |> get_media_entities_from_urls() - - attached_picture = entity |> get_picture() |> get_media_entity_from_media_id() - attached_pictures = [attached_picture] |> Enum.filter(& &1) - - {entity, extracted_pictures ++ attached_pictures} - end - - defp get_body(%Event{description: description}), do: description - defp get_body(%Post{body: body}), do: body - defp get_body(%Comment{text: text}), do: text - - defp get_picture(%Event{picture_id: picture_id}), do: picture_id - defp get_picture(%Post{picture_id: picture_id}), do: picture_id - defp get_picture(%Comment{}), do: nil - - defp parse_body(nil), do: [] - - defp parse_body(body) do - with res <- Regex.scan(~r/]*?src\s*=\s*['\"]([^'\"]*?)['\"][^>]*?>/, body), - res <- Enum.map(res, fn [_, res] -> res end) do - res - end - end - - defp get_media_entities_from_urls(media_urls) do - media_urls - |> Enum.map(fn media_url -> - # We prefer orphan media, but fallback on already attached media just in case - Medias.get_unattached_media_by_url(media_url) || Medias.get_media_by_url(media_url) - end) - |> Enum.filter(& &1) - end - - defp get_media_entity_from_media_id(nil), do: nil - - defp get_media_entity_from_media_id(media_id) do - Medias.get_media(media_id) - end - - defp save_entity(%Event{} = _event, []), do: :ok - - defp save_entity(%Event{} = event, media) do - event = Repo.preload(event, [:contacts, :media]) - Events.update_event(event, %{media: media}) - end - - defp save_entity(%Post{} = _post, []), do: :ok - - defp save_entity(%Post{} = post, media) do - post = Repo.preload(post, [:media]) - Posts.update_post(post, %{media: media}) - end - - defp save_entity(%Comment{} = _comment, []), do: :ok - - defp save_entity(%Comment{} = comment, media) do - comment = Repo.preload(comment, [:media]) - Discussions.update_comment(comment, %{media: media}) - end -end diff --git a/lib/mix/tasks/mobilizon/media/clean_orphan.ex b/lib/mix/tasks/mobilizon/media/clean_orphan.ex index 7b3ee34e5..09ba6a37f 100644 --- a/lib/mix/tasks/mobilizon/media/clean_orphan.ex +++ b/lib/mix/tasks/mobilizon/media/clean_orphan.ex @@ -64,7 +64,9 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphan do end Enum.each(medias, fn media -> - shell_info("ID: #{media.id}, Actor: #{media.actor_id}, URL: #{media.file.url}") + shell_info( + "ID: #{hd(media).id}, Actor: #{hd(media).actor_id}, Deduplicated #{length(media)} times, URL: #{hd(media).file.url}" + ) end) end diff --git a/lib/mobilizon/medias/medias.ex b/lib/mobilizon/medias/medias.ex index 565b828c7..7b4362621 100644 --- a/lib/mobilizon/medias/medias.ex +++ b/lib/mobilizon/medias/medias.ex @@ -4,6 +4,7 @@ defmodule Mobilizon.Medias do """ import Ecto.Query + import Mobilizon.Storage.CustomFunctions alias Ecto.Multi @@ -40,23 +41,15 @@ defmodule Mobilizon.Medias do end @doc """ - Get an unattached media by it's URL + Get all media by an URL. """ - def get_unattached_media_by_url(url) do + @spec get_all_media_by_url(String.t()) :: Media.t() | nil + def get_all_media_by_url(url) do url + |> String.split("?", parts: 2) + |> hd |> media_by_url_query() - |> join(:left, [m], e in assoc(m, :events)) - |> join(:left, [m], ep in assoc(m, :event_picture)) - |> join(:left, [m], p in assoc(m, :posts)) - |> join(:left, [m], pp in assoc(m, :posts_picture)) - |> join(:left, [m], c in assoc(m, :comments)) - |> where([_m, e], is_nil(e.id)) - |> where([_m, _e, ep], is_nil(ep.id)) - |> where([_m, _e, _ep, p], is_nil(p.id)) - |> where([_m, _e, _ep, _p, pp], is_nil(pp.id)) - |> where([_m, _e, _ep, _p, _pp, c], is_nil(c.id)) - |> limit(1) - |> Repo.one() + |> Repo.all() end @doc """ @@ -163,7 +156,7 @@ defmodule Mobilizon.Medias do defp media_by_url_query(url) do from( p in Media, - where: fragment("? @> ?", p.file, ~s|{"url": "#{url}"}|) + where: split_part(fragment("file->>'url'"), "?", 1) == ^url ) end diff --git a/lib/mobilizon/storage/custom_functions.ex b/lib/mobilizon/storage/custom_functions.ex new file mode 100644 index 000000000..eab327234 --- /dev/null +++ b/lib/mobilizon/storage/custom_functions.ex @@ -0,0 +1,10 @@ +defmodule Mobilizon.Storage.CustomFunctions do + @moduledoc """ + Helper module for custom PostgreSQL functions + """ + defmacro split_part(string, delimiter, position) do + quote do + fragment("split_part(?, ?, ?)", unquote(string), unquote(delimiter), unquote(position)) + end + end +end diff --git a/lib/service/clean_orphan_media.ex b/lib/service/clean_orphan_media.ex index 66703d30c..27c674aa5 100644 --- a/lib/service/clean_orphan_media.ex +++ b/lib/service/clean_orphan_media.ex @@ -5,6 +5,7 @@ defmodule Mobilizon.Service.CleanOrphanMedia do alias Mobilizon.Actors.Actor alias Mobilizon.Medias + alias Mobilizon.Medias.File alias Mobilizon.Medias.Media alias Mobilizon.Storage.Repo import Ecto.Query @@ -25,8 +26,10 @@ defmodule Mobilizon.Service.CleanOrphanMedia do if Keyword.get(opts, :dry_run, false) do {:ok, medias} else - Enum.each(medias, fn media -> - Medias.delete_media(media, ignore_file_not_found: true) + Enum.each(medias, fn media_list -> + Enum.each(media_list, fn media -> + Medias.delete_media(media, ignore_file_not_found: true) + end) end) {:ok, medias} @@ -49,7 +52,7 @@ defmodule Mobilizon.Service.CleanOrphanMedia do |> Enum.join(" UNION ") |> (&"NOT EXISTS(#{&1})").() - @spec find_media(Keyword.t()) :: list(Media.t()) + @spec find_media(Keyword.t()) :: list(list(Media.t())) defp find_media(opts) do default_grace_period = Mobilizon.Config.get([:instance, :orphan_upload_grace_period_hours], 48) @@ -68,6 +71,38 @@ defmodule Mobilizon.Service.CleanOrphanMedia do where: fragment(@union_query) ) - Repo.all(query) + query + |> Repo.all() + |> Enum.filter(fn %Media{file: %File{url: url}} -> + is_all_media_orphan?(url, expiration_date) + end) + |> Enum.chunk_by(fn %Media{file: %File{url: url}} -> + url + |> String.split("?", parts: 2) + |> hd + end) + end + + def is_all_media_orphan?(url, expiration_date) do + url + |> Medias.get_all_media_by_url() + |> Enum.all?(&is_media_orphan?(&1, expiration_date)) + end + + @spec is_media_orphan?(Media.t(), DateTime.t()) :: boolean() + def is_media_orphan?(%Media{id: media_id}, expiration_date) do + query = + from(m in Media, + as: :media, + distinct: true, + join: a in Actor, + on: a.id == m.actor_id, + where: m.id == ^media_id, + where: is_nil(a.domain), + where: m.inserted_at < ^expiration_date, + where: fragment(@union_query) + ) + + Repo.exists?(query) end end diff --git a/test/service/clean_orphan_media_test.exs b/test/service/clean_orphan_media_test.exs index 21c1c2264..f0ca3197d 100644 --- a/test/service/clean_orphan_media_test.exs +++ b/test/service/clean_orphan_media_test.exs @@ -16,7 +16,7 @@ defmodule Mobilizon.Service.CleanOrphanMediaTest do refute is_nil(Medias.get_media(media_id)) refute is_nil(Medias.get_media(media_2_id)) - assert {:ok, [found_media]} = CleanOrphanMedia.clean() + assert {:ok, [[found_media]]} = CleanOrphanMedia.clean() assert found_media.id == media_id assert is_nil(Medias.get_media(media_id)) @@ -31,7 +31,7 @@ defmodule Mobilizon.Service.CleanOrphanMediaTest do refute is_nil(Medias.get_media(media_id)) refute is_nil(Medias.get_media(media_2_id)) - assert {:ok, [found_media]} = CleanOrphanMedia.clean(dry_run: true) + assert {:ok, [[found_media]]} = CleanOrphanMedia.clean(dry_run: true) assert found_media.id == media_id refute is_nil(Medias.get_media(media_id)) @@ -46,7 +46,7 @@ defmodule Mobilizon.Service.CleanOrphanMediaTest do refute is_nil(Medias.get_media(media_id)) refute is_nil(Medias.get_media(media_2_id)) - assert {:ok, [found_media]} = CleanOrphanMedia.clean(grace_period: 12) + assert {:ok, [[found_media]]} = CleanOrphanMedia.clean(grace_period: 12) assert found_media.id == media_id assert is_nil(Medias.get_media(media_id)) diff --git a/test/support/factory.ex b/test/support/factory.ex index db1a40967..953ac8c0b 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -280,7 +280,7 @@ defmodule Mobilizon.Factory do %Mobilizon.Medias.File{ name: "My Media", url: url, - content_type: "image/png", + content_type: "image/jpg", size: 13_120 } end diff --git a/test/tasks/media/clean_orphan_test.exs b/test/tasks/media/clean_orphan_test.exs index 0aeb7e00b..83440dcf2 100644 --- a/test/tasks/media/clean_orphan_test.exs +++ b/test/tasks/media/clean_orphan_test.exs @@ -43,21 +43,22 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphanTest do test "with media returned" do media1 = insert(:media) media2 = insert(:media) + media3 = insert(:media, file: create_file()) with_mock CleanOrphanMedia, - clean: fn [dry_run: true, grace_period: 48] -> {:ok, [media1, media2]} end do + clean: fn [dry_run: true, grace_period: 48] -> {:ok, [[media1, media2], [media3]]} end do CleanOrphan.run(["--dry-run"]) assert_received {:mix_shell, :info, [output_received]} assert output_received == "List of files that would have been deleted" assert_received {:mix_shell, :info, [output_received]} assert output_received == - "ID: #{media1.id}, Actor: #{media1.actor_id}, URL: #{media1.file.url}" + "ID: #{media1.id}, Actor: #{media1.actor_id}, Deduplicated 2 times, URL: #{media1.file.url}" assert_received {:mix_shell, :info, [output_received]} assert output_received == - "ID: #{media2.id}, Actor: #{media2.actor_id}, URL: #{media2.file.url}" + "ID: #{media3.id}, Actor: #{media3.actor_id}, Deduplicated 1 times, URL: #{media3.file.url}" assert_received {:mix_shell, :info, [output_received]} assert output_received == "2 files would have been deleted" @@ -78,21 +79,22 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphanTest do test "with media returned" do media1 = insert(:media) media2 = insert(:media) + media3 = insert(:media, file: create_file()) with_mock CleanOrphanMedia, - clean: fn [dry_run: false, grace_period: 48] -> {:ok, [media1, media2]} end do + clean: fn [dry_run: false, grace_period: 48] -> {:ok, [[media1, media2], [media3]]} end do CleanOrphan.run(["--verbose"]) assert_received {:mix_shell, :info, [output_received]} assert output_received == "List of files that have been deleted" assert_received {:mix_shell, :info, [output_received]} assert output_received == - "ID: #{media1.id}, Actor: #{media1.actor_id}, URL: #{media1.file.url}" + "ID: #{media1.id}, Actor: #{media1.actor_id}, Deduplicated 2 times, URL: #{media1.file.url}" assert_received {:mix_shell, :info, [output_received]} assert output_received == - "ID: #{media2.id}, Actor: #{media2.actor_id}, URL: #{media2.file.url}" + "ID: #{media3.id}, Actor: #{media3.actor_id}, Deduplicated 1 times, URL: #{media3.file.url}" assert_received {:mix_shell, :info, [output_received]} assert output_received == "2 files have been deleted" @@ -121,4 +123,29 @@ defmodule Mix.Tasks.Mobilizon.Media.CleanOrphanTest do end end end + + defp create_file do + File.cp!("test/fixtures/picture.png", "test/fixtures/picture_tmp.png") + + file = %Plug.Upload{ + content_type: "image/png", + path: Path.absname("test/fixtures/picture_tmp.png"), + filename: "image.png" + } + + {:ok, data} = Mobilizon.Web.Upload.store(file) + + %{ + content_type: "image/png", + name: "image.png", + url: url + } = data + + %Mobilizon.Medias.File{ + name: "My Media", + url: url, + content_type: "image/png", + size: 13_120 + } + end end From 426f85884be7ece2a90db571fa5ff8bfe329f4bf Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Mon, 23 Aug 2021 10:20:31 +0200 Subject: [PATCH 5/8] Fix deleting own account Signed-off-by: Thomas Citharel --- js/src/views/Settings/AccountSettings.vue | 33 ++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/js/src/views/Settings/AccountSettings.vue b/js/src/views/Settings/AccountSettings.vue index 8c1f222ca..317b24905 100644 --- a/js/src/views/Settings/AccountSettings.vue +++ b/js/src/views/Settings/AccountSettings.vue @@ -180,14 +180,29 @@ }}

- + + import { IAuthProvider } from "@/types/enums"; +import { GraphQLError } from "graphql/error/GraphQLError"; import { Component, Vue, Ref } from "vue-property-decorator"; import { Route } from "vue-router"; import { @@ -256,6 +272,8 @@ export default class AccountSettings extends Vue { changePasswordErrors: string[] = []; + deletePasswordErrors: string[] = []; + isDeleteAccountModalActive = false; passwordForAccountDeletion = ""; @@ -313,6 +331,8 @@ export default class AccountSettings extends Vue { async deleteAccount(): Promise { try { + this.deletePasswordErrors = []; + console.debug("Asking to delete account..."); await this.$apollo.mutate({ mutation: DELETE_ACCOUNT, variables: { @@ -321,7 +341,8 @@ export default class AccountSettings extends Vue { : null, }, }); - await logout(this.$apollo.provider.defaultClient); + console.debug("Deleted account, logging out client..."); + await logout(this.$apollo.provider.defaultClient, false); this.$buefy.notification.open({ message: this.$t( "Your account has been successfully deleted" @@ -333,7 +354,9 @@ export default class AccountSettings extends Vue { return await this.$router.push({ name: RouteName.HOME }); } catch (err) { - return this.handleErrors("delete", err); + this.deletePasswordErrors = err.graphQLErrors.map( + ({ message }: GraphQLError) => message + ); } } @@ -361,6 +384,10 @@ export default class AccountSettings extends Vue { ); } + get deleteAccountPasswordFieldType(): string | null { + return this.deletePasswordErrors.length > 0 ? "is-danger" : null; + } + private handleErrors(type: string, err: any) { console.error(err); From c5d5758ece1d720cd06c771d0cf5b8ece1dd7d7c Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Mon, 23 Aug 2021 10:20:55 +0200 Subject: [PATCH 6/8] Accessibility improvements in AccountSettings Signed-off-by: Thomas Citharel --- js/src/variables.scss | 2 +- js/src/views/Settings/AccountSettings.vue | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/js/src/variables.scss b/js/src/variables.scss index 02942a8c1..f402b5bf8 100644 --- a/js/src/variables.scss +++ b/js/src/variables.scss @@ -54,7 +54,7 @@ $success-invert: findColorInvert($success); $info: #36bcd4; $info-invert: findColorInvert($info); $danger: #ff2e54; -$danger-invert: findColorInvert($danger); +$danger-invert: #000; $link: $primary; $link-invert: $primary-invert; $text: $violet-1; diff --git a/js/src/views/Settings/AccountSettings.vue b/js/src/views/Settings/AccountSettings.vue index 317b24905..9c5a9e03f 100644 --- a/js/src/views/Settings/AccountSettings.vue +++ b/js/src/views/Settings/AccountSettings.vue @@ -51,20 +51,22 @@ class="form" v-if="canChangeEmail" > - +

{{ $t("You'll receive a confirmation email.") }}

- + - + - + @@ -409,6 +413,12 @@ export default class AccountSettings extends Vue {