From 536d255356ff009d64b84dc927816975528725c0 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:28:13 +0200 Subject: [PATCH 01/20] Migration (PR: https://github.com/plausible/analytics/pull/2802) --- .../20230328062644_allow_domain_change.exs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 priv/repo/migrations/20230328062644_allow_domain_change.exs diff --git a/priv/repo/migrations/20230328062644_allow_domain_change.exs b/priv/repo/migrations/20230328062644_allow_domain_change.exs new file mode 100644 index 000000000000..e5c4ff0e3992 --- /dev/null +++ b/priv/repo/migrations/20230328062644_allow_domain_change.exs @@ -0,0 +1,49 @@ +defmodule Plausible.Repo.Migrations.AllowDomainChange do + use Ecto.Migration + + def up do + alter table(:sites) do + add(:domain_changed_from, :string, null: true) + add(:domain_changed_at, :naive_datetime, null: true) + end + + create(unique_index(:sites, :domain_changed_from)) + create(index(:sites, :domain_changed_at)) + + execute(""" + CREATE OR REPLACE FUNCTION check_domain() RETURNS TRIGGER AS $$ + BEGIN + IF EXISTS ( + SELECT 1 FROM sites + WHERE NEW.domain = domain_changed_from + OR (OLD IS NULL AND NEW.domain_changed_from = domain) + ) THEN + RAISE unique_violation USING CONSTRAINT = 'domain_change_disallowed'; + END IF; + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + """) + + execute(""" + CREATE TRIGGER check_domain_trigger + BEFORE INSERT OR UPDATE ON sites + FOR EACH ROW EXECUTE FUNCTION check_domain(); + """) + end + + def down do + execute(""" + DROP TRIGGER check_domain_trigger ON sites + """) + + execute(""" + DROP FUNCTION check_domain() + """) + + alter table(:sites) do + remove(:domain_changed_from) + remove(:domain_changed_at) + end + end +end From cd8d59247524e051ef7ba873f5f2b4c248218d5c Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:29:27 +0200 Subject: [PATCH 02/20] Implement Site.Domain interface allowing change and expiry --- lib/plausible/site.ex | 65 ++++++++++++++++++---- lib/plausible/site/domain.ex | 55 +++++++++++++++++++ test/plausible/site/domain_test.exs | 83 +++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 10 deletions(-) create mode 100644 lib/plausible/site/domain.ex create mode 100644 test/plausible/site/domain_test.exs diff --git a/lib/plausible/site.ex b/lib/plausible/site.ex index aac4c9d5e8e1..c2c6706b8f2b 100644 --- a/lib/plausible/site.ex +++ b/lib/plausible/site.ex @@ -21,6 +21,9 @@ defmodule Plausible.Site do field :ingest_rate_limit_scale_seconds, :integer, default: 60 field :ingest_rate_limit_threshold, :integer + field :domain_changed_from, :string + field :domain_changed_at, :naive_datetime + embeds_one :imported_data, Plausible.Site.ImportedData, on_replace: :update many_to_many :members, User, join_through: Plausible.Site.Membership @@ -40,21 +43,40 @@ defmodule Plausible.Site do timestamps() end + @domain_unique_error """ + This domain cannot be registered. Perhaps one of your colleagues registered it? If that's not the case, please contact support@plausible.io + """ + def changeset(site, attrs \\ %{}) do site |> cast(attrs, [:domain, :timezone]) |> clean_domain() |> validate_required([:domain, :timezone]) - |> validate_format(:domain, ~r/^[-\.\\\/:\p{L}\d]*$/u, - message: "only letters, numbers, slashes and period allowed" - ) + |> validate_domain_format() |> validate_domain_reserved_characters() |> unique_constraint(:domain, - message: - "This domain cannot be registered. Perhaps one of your colleagues registered it? If that's not the case, please contact support@plausible.io" + message: @domain_unique_error ) end + def update_changeset(site, attrs \\ %{}, opts \\ []) do + at = + opts + |> Keyword.get(:at, NaiveDateTime.utc_now()) + |> NaiveDateTime.truncate(:second) + + attrs = + if Plausible.v2?() do + attrs + else + Map.delete(attrs, :domain) + end + + site + |> changeset(attrs) + |> handle_domain_change(at) + end + def crm_changeset(site, attrs) do site |> cast(attrs, [ @@ -173,9 +195,9 @@ defmodule Plausible.Site do |> Timex.to_date() end - defp clean_domain(changeset) do + defp clean_domain(changeset, field \\ :domain) do clean_domain = - (get_field(changeset, :domain) || "") + (get_field(changeset, field) || "") |> String.trim() |> String.replace_leading("http://", "") |> String.replace_leading("https://", "") @@ -183,9 +205,7 @@ defmodule Plausible.Site do |> String.replace_trailing("/", "") |> String.downcase() - change(changeset, %{ - domain: clean_domain - }) + change(changeset, %{field => clean_domain}) end # https://tools.ietf.org/html/rfc3986#section-2.2 @@ -203,4 +223,29 @@ defmodule Plausible.Site do changeset end end + + defp validate_domain_format(changeset) do + validate_format(changeset, :domain, ~r/^[-\.\\\/:\p{L}\d]*$/u, + message: "only letters, numbers, slashes and period allowed" + ) + end + + defp handle_domain_change(changeset, at) do + new_domain = get_change(changeset, :domain) + + if new_domain do + changeset + |> put_change(:domain_changed_from, changeset.data.domain) + |> put_change(:domain_changed_at, at) + |> unique_constraint(:domain, + name: "domain_change_disallowed", + message: @domain_unique_error + ) + |> unique_constraint(:domain_changed_from, + message: @domain_unique_error + ) + else + changeset + end + end end diff --git a/lib/plausible/site/domain.ex b/lib/plausible/site/domain.ex new file mode 100644 index 000000000000..028ea3cdd042 --- /dev/null +++ b/lib/plausible/site/domain.ex @@ -0,0 +1,55 @@ +defmodule Plausible.Site.Domain do + @expire_threshold_hours 72 + + @moduledoc """ + Basic interface for domain changes. + + Once V2 schema migration is ready, domain change operation + will be enabled, accessible to the users. + + We will set a grace period of #{@expire_threshold_hours} hours + during which both old and new domains will redirect events traffic + to the same site. A periodic worker will call the `expire/0` + function to end it where applicable. + """ + + alias Plausible.Site + alias Plausible.Repo + + import Ecto.Query + + @spec expire_change_transitions(integer()) :: {:ok, non_neg_integer()} + def expire_change_transitions(expire_threshold_hours \\ @expire_threshold_hours) do + {updated, _} = + Repo.update_all( + from(s in Site, + where: s.domain_changed_at < ago(^expire_threshold_hours, "hour") + ), + set: [ + domain_changed_from: nil, + domain_changed_at: nil + ] + ) + + {:ok, updated} + end + + @spec change(Site.t(), String.t(), Keyword.t()) :: + {:ok, Site.t()} | {:error, Ecto.Changeset.t()} + def change(site = %Site{}, new_domain, opts \\ []) when is_binary(new_domain) do + changeset = Site.update_changeset(site, %{domain: new_domain}, opts) + + changeset = + if Enum.empty?(changeset.changes) do + Ecto.Changeset.add_error( + changeset, + :domain, + "New domain must be different than your current one." + ) + else + changeset + end + + Repo.update(changeset) + end +end diff --git a/test/plausible/site/domain_test.exs b/test/plausible/site/domain_test.exs new file mode 100644 index 000000000000..6a9d56e4ee43 --- /dev/null +++ b/test/plausible/site/domain_test.exs @@ -0,0 +1,83 @@ +defmodule Plausible.Site.DomainTest do + alias Plausible.Site + alias Plausible.Site.Domain + + use Plausible.DataCase, async: true + + @moduletag :v2_only + + test "successful change" do + site = insert(:site) + assert {:ok, updated} = Domain.change(site, "new-domain.example.com") + assert updated.domain_changed_from == site.domain + assert updated.domain == "new-domain.example.com" + assert updated.domain_changed_at + end + + test "domain_changed_from is kept unique, so no double change is possible" do + site1 = insert(:site) + assert {:ok, _} = Domain.change(site1, "new-domain.example.com") + + site2 = insert(:site) + assert {:error, changeset} = Domain.change(site2, "new-domain.example.com") + assert {error_message, _} = changeset.errors[:domain] + assert error_message =~ "This domain cannot be registered" + end + + test "domain is also guaranteed unique against existing domain_changed_from entries" do + site1 = + insert(:site, domain: "site1.example.com", domain_changed_from: "oldsite1.example.com") + + site2 = insert(:site, domain: "site2.example.com") + + assert {:error, %{errors: [{:domain, {error, _}}]}} = Domain.change(site2, site1.domain) + + assert {:error, %{errors: [{:domain, {^error, _}}]}} = + Domain.change(site2, site1.domain_changed_from) + + assert error =~ "This domain cannot be registered" + end + + test "change info is cleared when the grace period expires" do + site = insert(:site) + + assert {:ok, site} = Domain.change(site, "new-domain.example.com") + assert site.domain_changed_from + assert site.domain_changed_at + + assert {:ok, _} = Domain.expire_change_transitions(-1) + refute Repo.reload!(site).domain_changed_from + refute Repo.reload!(site).domain_changed_at + end + + test "expire changes overdue" do + now = NaiveDateTime.utc_now() + yesterday = now |> NaiveDateTime.add(-60 * 60 * 24, :second) + three_days_ago = now |> NaiveDateTime.add(-60 * 60 * 72, :second) + + {:ok, s1} = insert(:site) |> Domain.change("new-domain1.example.com") + {:ok, s2} = insert(:site) |> Domain.change("new-domain2.example.com", at: yesterday) + + {:ok, s3} = insert(:site) |> Domain.change("new-domain3.example.com", at: three_days_ago) + + assert {:ok, 1} = Domain.expire_change_transitions() + + assert is_nil(Repo.reload!(s3).domain_changed_from) + assert is_nil(Repo.reload!(s3).domain_changed_at) + + assert {:ok, 1} = Domain.expire_change_transitions(24) + assert is_nil(Repo.reload!(s2).domain_changed_at) + + assert {:ok, 0} = Domain.expire_change_transitions() + assert Repo.reload!(s1).domain_changed_at + end + + test "new domain gets validated" do + site = build(:site) + changeset = Site.update_changeset(site, %{domain: " "}) + assert {"can't be blank", _} = changeset.errors[:domain] + + changeset = Site.update_changeset(site, %{domain: "?#[]"}) + assert {"must not contain URI reserved characters" <> _, _} = changeset.errors[:domain] + end +end From 1b352c817b94fd15b8104b3bed8d8a7727363f2f Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:30:24 +0200 Subject: [PATCH 03/20] Fixup seeds so they work with V2_MIGRATION_DONE=1 --- priv/repo/seeds.exs | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 91398f2fe021..4dcb53db2791 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -84,17 +84,31 @@ Enum.flat_map(-720..0, fn day_index -> Enum.map(number_of_events, fn _ -> geolocation = Enum.random(geolocations) - [ - domain: site.domain, - hostname: site.domain, - timestamp: put_random_time.(date, day_index), - referrer_source: Enum.random(["", "Facebook", "Twitter", "DuckDuckGo", "Google"]), - browser: Enum.random(["Edge", "Chrome", "Safari", "Firefox", "Vivaldi"]), - browser_version: to_string(Enum.random(0..50)), - screen_size: Enum.random(["Mobile", "Tablet", "Desktop", "Laptop"]), - operating_system: Enum.random(["Windows", "macOS", "Linux"]), - operating_system_version: to_string(Enum.random(0..15)) - ] + if Plausible.v2?() do + [ + site_id: site.id, + hostname: site.domain, + timestamp: put_random_time.(date, day_index), + referrer_source: Enum.random(["", "Facebook", "Twitter", "DuckDuckGo", "Google"]), + browser: Enum.random(["Edge", "Chrome", "Safari", "Firefox", "Vivaldi"]), + browser_version: to_string(Enum.random(0..50)), + screen_size: Enum.random(["Mobile", "Tablet", "Desktop", "Laptop"]), + operating_system: Enum.random(["Windows", "macOS", "Linux"]), + operating_system_version: to_string(Enum.random(0..15)) + ] + else + [ + domain: site.domain, + hostname: site.domain, + timestamp: put_random_time.(date, day_index), + referrer_source: Enum.random(["", "Facebook", "Twitter", "DuckDuckGo", "Google"]), + browser: Enum.random(["Edge", "Chrome", "Safari", "Firefox", "Vivaldi"]), + browser_version: to_string(Enum.random(0..50)), + screen_size: Enum.random(["Mobile", "Tablet", "Desktop", "Laptop"]), + operating_system: Enum.random(["Windows", "macOS", "Linux"]), + operating_system_version: to_string(Enum.random(0..15)) + ] + end |> Keyword.merge(geolocation) |> then(&Plausible.Factory.build(:pageview, &1)) end) From d6cce74326df60070c8dc75a8472017418ce679e Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:30:53 +0200 Subject: [PATCH 04/20] Update Sites.Cache so it's capable of multi-keyed lookups --- lib/plausible/site/cache.ex | 18 +++++++ test/plausible/site/cache_test.exs | 78 ++++++++++++++++++++++++++---- 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/lib/plausible/site/cache.ex b/lib/plausible/site/cache.ex index 9fe6e2c13d75..0be5e894f2dc 100644 --- a/lib/plausible/site/cache.ex +++ b/lib/plausible/site/cache.ex @@ -9,6 +9,10 @@ defmodule Plausible.Site.Cache do during tests via the `:sites_by_domain_cache_enabled` application env key. This can be overridden on case by case basis, using the child specs options. + NOTE: the cache allows lookups by both `domain` and `domain_changed_from` + fields - this is to allow traffic from sites whose domains changed within a certain + grace period (see: `Plausible.Site.Transfer`). + When Cache is disabled via application env, the `get/1` function falls back to pure database lookups. This should help with introducing cached lookups in existing code, so that no existing tests should break. @@ -49,6 +53,7 @@ defmodule Plausible.Site.Cache do @cached_schema_fields ~w( id domain + domain_changed_from ingest_rate_limit_scale_seconds ingest_rate_limit_threshold )a @@ -91,6 +96,7 @@ defmodule Plausible.Site.Cache do from s in Site, select: { s.domain, + s.domain_changed_from, %{struct(s, ^@cached_schema_fields) | from_cache?: true} } @@ -109,6 +115,7 @@ defmodule Plausible.Site.Cache do where: s.updated_at > ago(^15, "minute"), select: { s.domain, + s.domain_changed_from, %{struct(s, ^@cached_schema_fields) | from_cache?: true} } @@ -124,6 +131,7 @@ defmodule Plausible.Site.Cache do def merge([], _), do: :ok def merge(new_items, opts) do + new_items = unwrap_cache_keys(new_items) cache_name = Keyword.get(opts, :cache_name, @cache_name) true = Cachex.put_many!(cache_name, new_items) @@ -221,4 +229,14 @@ defmodule Plausible.Site.Cache do stop = System.monotonic_time() {stop - start, result} end + + defp unwrap_cache_keys(items) do + Enum.reduce(items, [], fn + {domain, nil, object}, acc -> + [{domain, object} | acc] + + {domain, domain_changed_from, object}, acc -> + [{domain, object}, {domain_changed_from, object} | acc] + end) + end end diff --git a/test/plausible/site/cache_test.exs b/test/plausible/site/cache_test.exs index f7e5ceee704f..5a121418c045 100644 --- a/test/plausible/site/cache_test.exs +++ b/test/plausible/site/cache_test.exs @@ -71,6 +71,15 @@ defmodule Plausible.Site.CacheTest do assert Cache.ready?(test) end + test "cache allows lookups for sites with changed domain", %{test: test} do + {:ok, _} = start_test_cache(test) + insert(:site, domain: "new.example.com", domain_changed_from: "old.example.com") + :ok = Cache.refresh_all(cache_name: test) + + assert Cache.get("old.example.com", force?: true, cache_name: test) + assert Cache.get("new.example.com", force?: true, cache_name: test) + end + test "cache exposes hit rate", %{test: test} do {:ok, _} = start_test_cache(test) @@ -106,6 +115,46 @@ defmodule Plausible.Site.CacheTest do assert %Site{domain: ^domain2} = Cache.get(domain2, cache_opts) end + @tag :v2_only + test "sites with recently changed domains are refreshed", %{test: test} do + {:ok, _} = start_test_cache(test) + cache_opts = [cache_name: test, force?: true] + + domain1 = "first.example.com" + domain2 = "second.example.com" + + site = insert(:site, domain: domain1) + assert :ok = Cache.refresh_updated_recently(cache_opts) + assert item = Cache.get(domain1, cache_opts) + refute item.domain_changed_from + + # change domain1 to domain2 + + {:ok, _site} = Site.Domain.change(site, domain2) + + # small refresh keeps both items in cache + + assert :ok = Cache.refresh_updated_recently(cache_opts) + assert item_by_domain1 = Cache.get(domain1, cache_opts) + assert item_by_domain2 = Cache.get(domain2, cache_opts) + + assert item_by_domain1 == item_by_domain2 + assert item_by_domain1.domain == domain2 + assert item_by_domain1.domain_changed_from == domain1 + + # domain_changed_from gets no longer tracked + + {:ok, _} = Site.Domain.expire_change_transitions(-1) + + # full refresh removes the stale entry + + assert :ok = Cache.refresh_all(cache_opts) + + refute Cache.get(domain1, cache_opts) + assert item = Cache.get(domain2, cache_opts) + refute item.domain_changed_from + end + test "refreshing all sites sends a telemetry event", %{ test: test @@ -205,14 +254,14 @@ defmodule Plausible.Site.CacheTest do test "merging adds new items", %{test: test} do {:ok, _} = start_test_cache(test) - :ok = Cache.merge([{"item1", :item1}], cache_name: test) + :ok = Cache.merge([{"item1", nil, :item1}], cache_name: test) assert :item1 == Cache.get("item1", cache_name: test, force?: true) end test "merging no new items leaves the old cache intact", %{test: test} do {:ok, _} = start_test_cache(test) - :ok = Cache.merge([{"item1", :item1}], cache_name: test) + :ok = Cache.merge([{"item1", nil, :item1}], cache_name: test) :ok = Cache.merge([], cache_name: test) assert :item1 == Cache.get("item1", cache_name: test, force?: true) end @@ -220,8 +269,8 @@ defmodule Plausible.Site.CacheTest do test "merging removes stale items", %{test: test} do {:ok, _} = start_test_cache(test) - :ok = Cache.merge([{"item1", :item1}], cache_name: test) - :ok = Cache.merge([{"item2", :item2}], cache_name: test) + :ok = Cache.merge([{"item1", nil, :item1}], cache_name: test) + :ok = Cache.merge([{"item2", nil, :item2}], cache_name: test) refute Cache.get("item1", cache_name: test, force?: true) assert Cache.get("item2", cache_name: test, force?: true) @@ -230,8 +279,8 @@ defmodule Plausible.Site.CacheTest do test "merging optionally leaves stale items intact", %{test: test} do {:ok, _} = start_test_cache(test) - :ok = Cache.merge([{"item1", :item1}], cache_name: test) - :ok = Cache.merge([{"item2", :item2}], cache_name: test, delete_stale_items?: false) + :ok = Cache.merge([{"item1", nil, :item1}], cache_name: test) + :ok = Cache.merge([{"item2", nil, :item2}], cache_name: test, delete_stale_items?: false) assert Cache.get("item1", cache_name: test, force?: true) assert Cache.get("item2", cache_name: test, force?: true) @@ -240,15 +289,24 @@ defmodule Plausible.Site.CacheTest do test "merging updates changed items", %{test: test} do {:ok, _} = start_test_cache(test) - :ok = Cache.merge([{"item1", :item1}, {"item2", :item2}], cache_name: test) - :ok = Cache.merge([{"item1", :changed}, {"item2", :item2}], cache_name: test) + :ok = Cache.merge([{"item1", nil, :item1}, {"item2", nil, :item2}], cache_name: test) + :ok = Cache.merge([{"item1", nil, :changed}, {"item2", nil, :item2}], cache_name: test) assert :changed == Cache.get("item1", cache_name: test, force?: true) assert :item2 == Cache.get("item2", cache_name: test, force?: true) end - @items1 for i <- 1..200_000, do: {i, :batch1} - @items2 for _ <- 1..200_000, do: {Enum.random(1..400_000), :batch2} + test "merging keeps secondary keys", %{test: test} do + {:ok, _} = start_test_cache(test) + + :ok = Cache.merge([{"item1", nil, :item1}], cache_name: test) + :ok = Cache.merge([{"item2", "item1", :updated}], cache_name: test) + assert :updated == Cache.get("item1", cache_name: test, force?: true) + assert :updated == Cache.get("item2", cache_name: test, force?: true) + end + + @items1 for i <- 1..200_000, do: {i, nil, :batch1} + @items2 for _ <- 1..200_000, do: {Enum.random(1..400_000), nil, :batch2} @max_seconds 2 test "merging large sets is expected to be under #{@max_seconds} seconds", %{test: test} do {:ok, _} = start_test_cache(test) From c488a27b255f6dcb9df61fcd6eb0562edd3027ca Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:31:36 +0200 Subject: [PATCH 05/20] Implement worker handling domain change expiration --- config/runtime.exs | 9 ++-- .../expire_domain_change_transitions.ex | 22 ++++++++++ .../expire_domain_change_transitions_test.exs | 43 +++++++++++++++++++ 3 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 lib/workers/expire_domain_change_transitions.ex create mode 100644 test/workers/expire_domain_change_transitions_test.exs diff --git a/config/runtime.exs b/config/runtime.exs index c937bd1c7d71..75e0a014c02f 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -363,7 +363,7 @@ end base_cron = [ # Daily at midnight {"0 0 * * *", Plausible.Workers.RotateSalts}, - #  hourly + # hourly {"0 * * * *", Plausible.Workers.ScheduleEmailReports}, # hourly {"0 * * * *", Plausible.Workers.SendSiteSetupEmails}, @@ -374,7 +374,9 @@ base_cron = [ # Every day at midnight {"0 0 * * *", Plausible.Workers.CleanEmailVerificationCodes}, # Every day at 1am - {"0 1 * * *", Plausible.Workers.CleanInvitations} + {"0 1 * * *", Plausible.Workers.CleanInvitations}, + # Every 2 hours + {"0 */2 * * *", Plausible.Workers.ExpireDomainChangeTransitions} ] cloud_cron = [ @@ -399,7 +401,8 @@ base_queues = [ site_setup_emails: 1, clean_email_verification_codes: 1, clean_invitations: 1, - google_analytics_imports: 1 + google_analytics_imports: 1, + domain_change_transition: 1 ] cloud_queues = [ diff --git a/lib/workers/expire_domain_change_transitions.ex b/lib/workers/expire_domain_change_transitions.ex new file mode 100644 index 000000000000..b6956feb46fb --- /dev/null +++ b/lib/workers/expire_domain_change_transitions.ex @@ -0,0 +1,22 @@ +defmodule Plausible.Workers.ExpireDomainChangeTransitions do + @moduledoc """ + Periodic worker that expires domain change transition period. + Old domains are frozen for a given time, so users can still access them + before redeploying their scripts and integrations. + """ + use Plausible.Repo + use Oban.Worker, queue: :domain_change_transition + + require Logger + + @impl Oban.Worker + def perform(_job) do + {:ok, n} = Plausible.Site.Domain.expire_change_transitions() + + if n > 0 do + Logger.warning("Expired #{n} from the domain change transition period.") + end + + :ok + end +end diff --git a/test/workers/expire_domain_change_transitions_test.exs b/test/workers/expire_domain_change_transitions_test.exs new file mode 100644 index 000000000000..a2de6fb7f316 --- /dev/null +++ b/test/workers/expire_domain_change_transitions_test.exs @@ -0,0 +1,43 @@ +defmodule Plausible.Workers.ExpireDomainChangeTransitionsTest do + use Plausible.DataCase, async: true + alias Plausible.Workers.ExpireDomainChangeTransitions + alias Plausible.Site + alias Plausible.Sites + + import ExUnit.CaptureLog + + @moduletag :v2_only + + test "doesn't log when there is nothing to do" do + log = + capture_log(fn -> + assert :ok = ExpireDomainChangeTransitions.perform(nil) + end) + + assert log == "" + end + + test "expires domains selectively after change and logs the result" do + now = NaiveDateTime.utc_now() + yesterday = now |> NaiveDateTime.add(-60 * 60 * 24, :second) + three_days_ago = now |> NaiveDateTime.add(-60 * 60 * 72, :second) + long_time_ago = now |> NaiveDateTime.add(-60 * 60 * 24 * 365, :second) + + insert(:site) |> Site.Domain.change("site1.example.com") + insert(:site) |> Site.Domain.change("site2.example.com", at: yesterday) + insert(:site) |> Site.Domain.change("site3.example.com", at: three_days_ago) + insert(:site) |> Site.Domain.change("site4.example.com", at: long_time_ago) + + log = + capture_log(fn -> + assert :ok = ExpireDomainChangeTransitions.perform(nil) + end) + + assert log =~ "Expired 2 from the domain change transition period" + + assert Sites.get_by_domain("site1.example.com").domain_changed_from + assert Sites.get_by_domain("site2.example.com").domain_changed_from + refute Sites.get_by_domain("site3.example.com").domain_changed_from + refute Sites.get_by_domain("site4.example.com").domain_changed_from + end +end From dcd6170eefea7ea9707900908892bd1d4ebdf73c Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:32:26 +0200 Subject: [PATCH 06/20] Implement domain change UI --- .../controllers/site_controller.ex | 64 +++++++++- lib/plausible_web/router.ex | 3 + .../templates/site/change_domain.html.eex | 29 +++++ .../templates/site/settings_general.html.eex | 23 +++- .../site/snippet_after_domain_change.html.eex | 23 ++++ .../controllers/site_controller_test.exs | 114 +++++++++++++++++- 6 files changed, 247 insertions(+), 9 deletions(-) create mode 100644 lib/plausible_web/templates/site/change_domain.html.eex create mode 100644 lib/plausible_web/templates/site/snippet_after_domain_change.html.eex diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 51f293402b96..ea169579c908 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -329,11 +329,10 @@ defmodule PlausibleWeb.SiteController do end def update_settings(conn, %{"site" => site_params}) do - site = conn.assigns[:site] - changeset = site |> Plausible.Site.changeset(site_params) - res = changeset |> Repo.update() + site = conn.assigns[:site] |> Repo.preload(:custom_domain) + changeset = Plausible.Site.update_changeset(site, site_params) - case res do + case Repo.update(changeset) do {:ok, site} -> site_session_key = "authorized_site__" <> site.domain @@ -343,7 +342,13 @@ defmodule PlausibleWeb.SiteController do |> redirect(to: Routes.site_path(conn, :settings_general, site.domain)) {:error, changeset} -> - render(conn, "settings_general.html", site: site, changeset: changeset) + conn + |> put_flash(:error, "Could not update your site settings") + |> render("settings_general.html", + site: site, + changeset: changeset, + layout: {PlausibleWeb.LayoutView, "site_settings.html"} + ) end end @@ -867,4 +872,53 @@ defmodule PlausibleWeb.SiteController do |> redirect(to: Routes.site_path(conn, :settings_general, site.domain)) end end + + def change_domain(conn, _params) do + if Plausible.v2?() do + site = conn.assigns[:site] + + changeset = Plausible.Site.update_changeset(site) + + render(conn, "change_domain.html", + changeset: changeset, + layout: {PlausibleWeb.LayoutView, "focus.html"} + ) + else + render_error(conn, 404) + end + end + + def change_domain_submit(conn, %{"site" => %{"domain" => new_domain}}) do + if Plausible.v2?() do + site = conn.assigns[:site] + + case Plausible.Site.Domain.change(site, new_domain) do + {:ok, updated_site} -> + conn + |> put_flash(:success, "Website domain changed successfully") + |> redirect( + to: Routes.site_path(conn, :add_snippet_after_domain_change, updated_site.domain) + ) + + {:error, changeset} -> + render(conn, "change_domain.html", + changeset: changeset, + layout: {PlausibleWeb.LayoutView, "focus.html"} + ) + end + else + render_error(conn, 404) + end + end + + def add_snippet_after_domain_change(conn, _params) do + site = conn.assigns[:site] |> Repo.preload(:custom_domain) + + conn + |> assign(:skip_plausible_tracking, true) + |> render("snippet_after_domain_change.html", + site: site, + layout: {PlausibleWeb.LayoutView, "focus.html"} + ) + end end diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index cd7e66e77675..53d61183c400 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -175,6 +175,9 @@ defmodule PlausibleWeb.Router do get "/sites", SiteController, :index get "/sites/new", SiteController, :new post "/sites", SiteController, :create_site + get "/sites/:website/change-domain", SiteController, :change_domain + put "/sites/:website/change-domain", SiteController, :change_domain_submit + get "/:website/change-domain-snippet", SiteController, :add_snippet_after_domain_change post "/sites/:website/make-public", SiteController, :make_public post "/sites/:website/make-private", SiteController, :make_private post "/sites/:website/weekly-report/enable", SiteController, :enable_weekly_report diff --git a/lib/plausible_web/templates/site/change_domain.html.eex b/lib/plausible_web/templates/site/change_domain.html.eex new file mode 100644 index 000000000000..17d77e21de2a --- /dev/null +++ b/lib/plausible_web/templates/site/change_domain.html.eex @@ -0,0 +1,29 @@ +
+ <%= form_for @changeset, Routes.site_path(@conn, :change_domain_submit, @site.domain), [class: "max-w-lg w-full mx-auto bg-white dark:bg-gray-800 shadow-lg rounded px-8 pt-6 pb-8 mb-4 mt-8"], fn f -> %> +

Change your website domain

+ +
+ <%= label f, :domain, class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %> +

Just the naked domain or subdomain without 'www'

+
+ + https:// + + <%= text_input f, :domain, class: "focus:ring-indigo-500 focus:border-indigo-500 dark:bg-gray-800 flex-1 block w-full px-3 py-2 rounded-none rounded-r-md sm:text-sm border-gray-300 dark:border-gray-500 dark:bg-gray-900 dark:text-gray-300", placeholder: "example.com" %> +
+ <%= error_tag f, :domain %> +
+ +

+ Once you change your domain, you must update the JavaScript snippet on your site within 72 hours to guarantee continuous tracking. If you're using the API, please also make sure to update your API credentials.

+

+ Visit our documentation for details. +

+ + <%= submit "Change domain and add new snippet →", class: "button mt-4 w-full" %> + +
+ <%= link "Back to site settings", to: Routes.site_path(@conn, :settings_general, @site.domain), class: "text-indigo-500 w-full text-center" %> +
+ <% end %> +
diff --git a/lib/plausible_web/templates/site/settings_general.html.eex b/lib/plausible_web/templates/site/settings_general.html.eex index 58304dde7e6d..61d9ae7eb0f5 100644 --- a/lib/plausible_web/templates/site/settings_general.html.eex +++ b/lib/plausible_web/templates/site/settings_general.html.eex @@ -1,8 +1,27 @@ +<%= if Plausible.v2?() do %> +
+
+
+

Site domain

+

Moving your site to a different domain? We got you!

+ <%= link(to: "https://plausible.io/docs/change-domain-name/", target: "_blank", rel: "noreferrer") do %> + + <% end %> +
+
+
+ + <%= link "Change domain", to: Routes.site_path(@conn, :change_domain, @site.domain), class: "button" %> + +
+
+<% end %> + <%= form_for @changeset, "/#{URI.encode_www_form(@site.domain)}/settings", fn f -> %>
-

General information

+

Site timezone

Update your reporting timezone.

<%= link(to: "https://plausible.io/docs/general/", target: "_blank", rel: "noreferrer") do %> @@ -10,8 +29,6 @@
-
<%= label f, :domain, class: "block text-sm font-medium leading-5 text-gray-700 dark:text-gray-300" %> <%= text_input f, :domain, class: "dark:bg-gray-900 mt-1 block w-full shadow-sm focus:ring-indigo-500 focus:border-indigo-500 sm:max-w-xs sm:text-sm border-gray-300 dark:border-gray-500 rounded-md dark:text-gray-100", disabled: "disabled" %> -
<%= label f, :timezone, "Reporting Timezone", class: "block text-sm font-medium leading-5 text-gray-700 dark:text-gray-300" %> diff --git a/lib/plausible_web/templates/site/snippet_after_domain_change.html.eex b/lib/plausible_web/templates/site/snippet_after_domain_change.html.eex new file mode 100644 index 000000000000..3bfbb3b81ca1 --- /dev/null +++ b/lib/plausible_web/templates/site/snippet_after_domain_change.html.eex @@ -0,0 +1,23 @@ +
+ <%= form_for @conn, "/", [class: "max-w-lg w-full mx-auto bg-white dark:bg-gray-800 shadow-md rounded px-8 pt-6 pb-8 mb-4 mt-8"], fn f -> %> +

Change JavaScript snippet

+
+

Replace your snippet in the <head> of your website.

+ +
+ <%= textarea f, :domain, id: "snippet_code", class: "transition overflow-hidden bg-gray-100 dark:bg-gray-900 appearance-none border border-transparent rounded w-full p-2 pr-6 text-gray-700 dark:text-gray-300 leading-normal appearance-none focus:outline-none focus:bg-white dark:focus:bg-gray-800 focus:border-gray-400 dark:focus:border-gray-500 text-xs mt-4 resize-none", value: snippet(@site), rows: 3, readonly: "readonly" %> + + + +
+
+ +

+ Your domain has been changed. You must update the JavaScript snippet on your site within 72 hours to guarantee continuous tracking. If you're using the API, please also make sure to update your API credentials.

+

+ Visit our documentation for details. +

+ + <%= link("I understand, I'll change my snippet →", class: "button mt-4 w-full", to: "/#{URI.encode_www_form(@site.domain)}") %> + <% end %> +
diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index 7ab6a2057809..9708e6da33a3 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -305,7 +305,7 @@ defmodule PlausibleWeb.SiteControllerTest do conn = get(conn, "/#{site.domain}/settings/general") resp = html_response(conn, 200) - assert resp =~ "General information" + assert resp =~ "Site timezone" assert resp =~ "Data Import from Google Analytics" assert resp =~ "https://accounts.google.com/o/oauth2/v2/auth?" assert resp =~ "analytics.readonly" @@ -1145,4 +1145,116 @@ defmodule PlausibleWeb.SiteControllerTest do assert Repo.reload(job).state == "cancelled" end end + + describe "domain change" do + setup [:create_user, :log_in, :create_site] + + @tag :v2_only + test "shows domain change in the settings form", %{conn: conn, site: site} do + conn = get(conn, Routes.site_path(conn, :settings_general, site.domain)) + resp = html_response(conn, 200) + + assert resp =~ "Site domain" + assert resp =~ "Change domain" + assert resp =~ Routes.site_path(conn, :change_domain, site.domain) + end + + @tag :v2_only + test "domain change form renders", %{conn: conn, site: site} do + conn = get(conn, Routes.site_path(conn, :change_domain, site.domain)) + resp = html_response(conn, 200) + assert resp =~ Routes.site_path(conn, :change_domain_submit, site.domain) + + assert resp =~ + "Once you change your domain, you must update the JavaScript snippet on your site within 72 hours" + end + + @tag :v2_only + test "domain change form submission when no change is made", %{conn: conn, site: site} do + conn = + put(conn, Routes.site_path(conn, :change_domain_submit, site.domain), %{ + "site" => %{"domain" => site.domain} + }) + + resp = html_response(conn, 200) + assert resp =~ "New domain must be different than your current one." + end + + @tag :v2_only + test "domain change form submission to an existing domain", %{conn: conn, site: site} do + another_site = insert(:site) + + conn = + put(conn, Routes.site_path(conn, :change_domain_submit, site.domain), %{ + "site" => %{"domain" => another_site.domain} + }) + + resp = html_response(conn, 200) + assert resp =~ "This domain cannot be registered" + + site = Repo.reload!(site) + assert site.domain != another_site.domain + assert is_nil(site.domain_changed_from) + end + + @tag :v2_only + test "domain change form submission to a domain in transition period", %{ + conn: conn, + site: site + } do + another_site = insert(:site, domain_changed_from: "foo.example.com") + + conn = + put(conn, Routes.site_path(conn, :change_domain_submit, site.domain), %{ + "site" => %{"domain" => "foo.example.com"} + }) + + resp = html_response(conn, 200) + assert resp =~ "This domain cannot be registered" + + site = Repo.reload!(site) + assert site.domain != another_site.domain + assert is_nil(site.domain_changed_from) + end + + @tag :v2_only + test "domain change succcessful form submission redirects to snippet change info", %{ + conn: conn, + site: site + } do + original_domain = site.domain + + conn = + put(conn, Routes.site_path(conn, :change_domain_submit, site.domain), %{ + "site" => %{"domain" => "foo.example.com"} + }) + + assert redirected_to(conn) == + Routes.site_path(conn, :add_snippet_after_domain_change, "foo.example.com") + + site = Repo.reload!(site) + assert site.domain == "foo.example.com" + assert site.domain_changed_from == original_domain + end + + @tag :v2_only + test "snippet info after domain change", %{ + conn: conn, + site: site + } do + put(conn, Routes.site_path(conn, :change_domain_submit, site.domain), %{ + "site" => %{"domain" => "foo.example.com"} + }) + + resp = + conn + |> get(Routes.site_path(conn, :add_snippet_after_domain_change, "foo.example.com")) + |> html_response(200) + |> Floki.parse_document!() + |> Floki.text() + + assert resp =~ + "Your domain has been changed. You must update the JavaScript snippet on your site within 72 hours" + end + end end From 5f0987a5dae70300f88d2274f86e98eb13656a26 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:32:39 +0200 Subject: [PATCH 07/20] Implement transition period for public APIs --- lib/plausible/sites.ex | 2 +- .../plugs/authorize_stats_api.ex | 5 +- .../api/external_sites_controller_test.exs | 94 ++++++++++++++++++- .../external_stats_controller/auth_test.exs | 27 ++++++ 4 files changed, 123 insertions(+), 5 deletions(-) diff --git a/lib/plausible/sites.ex b/lib/plausible/sites.ex index d936f7b74701..c7743ebe6f08 100644 --- a/lib/plausible/sites.ex +++ b/lib/plausible/sites.ex @@ -107,7 +107,7 @@ defmodule Plausible.Sites do on: sm.site_id == s.id, where: sm.user_id == ^user_id, where: sm.role in ^roles, - where: s.domain == ^domain, + where: s.domain == ^domain or s.domain_changed_from == ^domain, select: s ) end diff --git a/lib/plausible_web/plugs/authorize_stats_api.ex b/lib/plausible_web/plugs/authorize_stats_api.ex index 391721b8b393..dbf6e7cf3ea8 100644 --- a/lib/plausible_web/plugs/authorize_stats_api.ex +++ b/lib/plausible_web/plugs/authorize_stats_api.ex @@ -52,7 +52,10 @@ defmodule PlausibleWeb.AuthorizeStatsApiPlug do defp verify_access(_api_key, nil), do: {:error, :missing_site_id} defp verify_access(api_key, site_id) do - case Repo.get_by(Plausible.Site, domain: site_id) do + domain_based_search = + from s in Plausible.Site, where: s.domain == ^site_id or s.domain_changed_from == ^site_id + + case Repo.one(domain_based_search) do %Plausible.Site{} = site -> is_member? = Sites.is_member?(api_key.user_id, site) is_super_admin? = Plausible.Auth.is_super_admin?(api_key.user_id) diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 25e5b15ea09d..3d9dcbbd2f36 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -97,12 +97,24 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "DELETE /api/v1/sites/:site_id" do setup :create_new_site - test "delete a site by it's domain", %{conn: conn, site: site} do + test "delete a site by its domain", %{conn: conn, site: site} do conn = delete(conn, "/api/v1/sites/" <> site.domain) assert json_response(conn, 200) == %{"deleted" => true} end + @tag :v2_only + test "delete a site by its old domain after domain change", %{conn: conn, site: site} do + old_domain = site.domain + new_domain = "new.example.com" + + Plausible.Site.Domain.change(site, new_domain) + + conn = delete(conn, "/api/v1/sites/" <> old_domain) + + assert json_response(conn, 200) == %{"deleted" => true} + end + test "is 404 when site cannot be found", %{conn: conn} do conn = delete(conn, "/api/v1/sites/foobar.baz") @@ -147,6 +159,27 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert String.starts_with?(res["url"], "http://") end + @tag :v2_only + test "can add a shared link to a site using the old site id after domain change", %{ + conn: conn, + site: site + } do + old_domain = site.domain + new_domain = "new.example.com" + + Plausible.Site.Domain.change(site, new_domain) + + conn = + put(conn, "/api/v1/sites/shared-links", %{ + site_id: old_domain, + name: "Wordpress" + }) + + res = json_response(conn, 200) + assert res["name"] == "Wordpress" + assert String.starts_with?(res["url"], "http://") + end + test "is idempotent find or create op", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/shared-links", %{ @@ -238,6 +271,25 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert res["page_path"] == "/signup" end + @tag :v2_only + test "can add a goal using old site_id after domain change", %{conn: conn, site: site} do + old_domain = site.domain + new_domain = "new.example.com" + + Plausible.Site.Domain.change(site, new_domain) + + conn = + put(conn, "/api/v1/sites/goals", %{ + site_id: old_domain, + goal_type: "event", + event_name: "Signup" + }) + + res = json_response(conn, 200) + assert res["goal_type"] == "event" + assert res["event_name"] == "Signup" + end + test "is idempotent find or create op", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/goals", %{ @@ -341,7 +393,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "DELETE /api/v1/sites/goals/:goal_id" do setup :create_new_site - test "delete a goal by it's id", %{conn: conn, site: site} do + test "delete a goal by its id", %{conn: conn, site: site} do conn = put(conn, "/api/v1/sites/goals", %{ site_id: site.domain, @@ -359,6 +411,30 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert json_response(conn, 200) == %{"deleted" => true} end + @tag :v2_only + test "delete a goal using old site_id after domain change", %{conn: conn, site: site} do + old_domain = site.domain + new_domain = "new.example.com" + + Plausible.Site.Domain.change(site, new_domain) + + conn = + put(conn, "/api/v1/sites/goals", %{ + site_id: new_domain, + goal_type: "event", + event_name: "Signup" + }) + + %{"id" => goal_id} = json_response(conn, 200) + + conn = + delete(conn, "/api/v1/sites/goals/#{goal_id}", %{ + site_id: old_domain + }) + + assert json_response(conn, 200) == %{"deleted" => true} + end + test "is 404 when goal cannot be found", %{conn: conn, site: site} do conn = delete(conn, "/api/v1/sites/goals/0", %{ @@ -405,12 +481,24 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "GET /api/v1/sites/:site_id" do setup :create_new_site - test "get a site by it's domain", %{conn: conn, site: site} do + test "get a site by its domain", %{conn: conn, site: site} do conn = get(conn, "/api/v1/sites/" <> site.domain) assert json_response(conn, 200) == %{"domain" => site.domain, "timezone" => site.timezone} end + @tag :v2_only + test "get a site by old site_id after domain change", %{conn: conn, site: site} do + old_domain = site.domain + new_domain = "new.example.com" + + Plausible.Site.Domain.change(site, new_domain) + + conn = get(conn, "/api/v1/sites/" <> old_domain) + + assert json_response(conn, 200) == %{"domain" => new_domain, "timezone" => site.timezone} + end + test "is 404 when site cannot be found", %{conn: conn} do conn = get(conn, "/api/v1/sites/foobar.baz") diff --git a/test/plausible_web/controllers/api/external_stats_controller/auth_test.exs b/test/plausible_web/controllers/api/external_stats_controller/auth_test.exs index 70df17c3fad1..bc88b14f7a65 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/auth_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/auth_test.exs @@ -123,6 +123,33 @@ defmodule PlausibleWeb.Api.ExternalStatsController.AuthTest do ) end + @tag :v2_only + test "can access with either site_id after domain change", %{ + conn: conn, + user: user, + api_key: api_key + } do + old_domain = "old.example.com" + new_domain = "new.example.com" + site = insert(:site, domain: old_domain, members: [user]) + + Plausible.Site.Domain.change(site, new_domain) + + conn + |> with_api_key(api_key) + |> get("/api/v1/stats/aggregate", %{"site_id" => new_domain, "metrics" => "pageviews"}) + |> assert_ok(%{ + "results" => %{"pageviews" => %{"value" => 0}} + }) + + conn + |> with_api_key(api_key) + |> get("/api/v1/stats/aggregate", %{"site_id" => old_domain, "metrics" => "pageviews"}) + |> assert_ok(%{ + "results" => %{"pageviews" => %{"value" => 0}} + }) + end + defp with_api_key(conn, api_key) do Plug.Conn.put_req_header(conn, "authorization", "Bearer #{api_key}") end From c0f51c5cbcb882effc8494b42a5c602a49f2facc Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 14:33:16 +0200 Subject: [PATCH 08/20] Exclude v2 tests in primary test run --- test/test_helper.exs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/test_helper.exs b/test/test_helper.exs index 4d594919b2e5..ec6ec622827d 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1,13 +1,16 @@ {:ok, _} = Application.ensure_all_started(:ex_machina) Mox.defmock(Plausible.HTTPClient.Mock, for: Plausible.HTTPClient.Interface) FunWithFlags.enable(:visits_metric) -ExUnit.start(exclude: :slow) Application.ensure_all_started(:double) Ecto.Adapters.SQL.Sandbox.mode(Plausible.Repo, :manual) if Plausible.v2?() do + ExUnit.configure(exclude: [:slow]) + IO.puts("Running tests against v2 schema") else + ExUnit.configure(exclude: [:v2_only, :slow]) + IO.puts( "Running tests against v1 schema. Use: `V2_MIGRATION_DONE=1 mix test` for secondary run." ) From 95e6b93f5526d223ddcc5e04c0f3b1e036871119 Mon Sep 17 00:00:00 2001 From: hq1 Date: Wed, 29 Mar 2023 17:57:16 +0200 Subject: [PATCH 09/20] Update lib/plausible_web/controllers/site_controller.ex Co-authored-by: Vini Brasil --- lib/plausible_web/controllers/site_controller.ex | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index ea169579c908..31d00d61dff3 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -875,9 +875,7 @@ defmodule PlausibleWeb.SiteController do def change_domain(conn, _params) do if Plausible.v2?() do - site = conn.assigns[:site] - - changeset = Plausible.Site.update_changeset(site) + changeset = Plausible.Site.update_changeset(conn.assigns.site) render(conn, "change_domain.html", changeset: changeset, From cfd3c90194515fe5654c0fb956754b03647aef2e Mon Sep 17 00:00:00 2001 From: hq1 Date: Wed, 29 Mar 2023 17:57:22 +0200 Subject: [PATCH 10/20] Update lib/plausible_web/controllers/site_controller.ex Co-authored-by: Vini Brasil --- lib/plausible_web/controllers/site_controller.ex | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/plausible_web/controllers/site_controller.ex b/lib/plausible_web/controllers/site_controller.ex index 31d00d61dff3..8fa10af8c1e3 100644 --- a/lib/plausible_web/controllers/site_controller.ex +++ b/lib/plausible_web/controllers/site_controller.ex @@ -888,9 +888,7 @@ defmodule PlausibleWeb.SiteController do def change_domain_submit(conn, %{"site" => %{"domain" => new_domain}}) do if Plausible.v2?() do - site = conn.assigns[:site] - - case Plausible.Site.Domain.change(site, new_domain) do + case Plausible.Site.Domain.change(conn.assigns.site, new_domain) do {:ok, updated_site} -> conn |> put_flash(:success, "Website domain changed successfully") From 622769b26196b6b1a579accb3fde4ae86bbfb1d6 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 29 Mar 2023 19:32:27 +0200 Subject: [PATCH 11/20] Update moduledoc --- lib/plausible/site/domain.ex | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/plausible/site/domain.ex b/lib/plausible/site/domain.ex index 028ea3cdd042..6151f066ccb2 100644 --- a/lib/plausible/site/domain.ex +++ b/lib/plausible/site/domain.ex @@ -4,13 +4,20 @@ defmodule Plausible.Site.Domain do @moduledoc """ Basic interface for domain changes. - Once V2 schema migration is ready, domain change operation - will be enabled, accessible to the users. + Once `Plausible.DataMigration.NumericIDs` schema migration is ready, + domain change operation will be enabled, accessible to the users. - We will set a grace period of #{@expire_threshold_hours} hours - during which both old and new domains will redirect events traffic - to the same site. A periodic worker will call the `expire/0` - function to end it where applicable. + We will set a transition period of #{@expire_threshold_hours} hours + during which, both old and new domains, will be accepted as traffic + identifiers to the same site. + + A periodic worker will call the `expire/0` function to end it where applicable. + See: `Plausible.Workers.ExpireDomainChangeTransitions`. + + The underlying changeset for domain change (see: `Plausible.Site`) relies + on database trigger installed via `Plausible.Repo.Migrations.AllowDomainChange` + Postgres migration. The trigger checks if either `domain` or `domain_changed_from` + exist to ensure unicity. """ alias Plausible.Site From e6a56b4f17e5fb0c40ee722033485f82e9a2eb96 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 08:26:08 +0200 Subject: [PATCH 12/20] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfb0177c0f70..2c65478552d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. ### Added - 'Last updated X seconds ago' info to 'current visitors' tooltips - Add support for more Bamboo adapters, i.e. `Bamboo.MailgunAdapter`, `Bamboo.MandrillAdapter`, `Bamboo.SendGridAdapter` plausible/analytics#2649 +- Ability to change domain for existing site (requires numeric IDs data migration, instructions will be provided separately) ### Fixed - Make goal-filtered CSV export return only unique_conversions timeseries in the 'visitors.csv' file From ded696eb8f94be13c882e9b4f929355b0701f318 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 13:56:04 +0200 Subject: [PATCH 13/20] Remove remnant from previous implementation attempt --- lib/plausible/site.ex | 52 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/plausible/site.ex b/lib/plausible/site.ex index c2c6706b8f2b..8d7918fadeea 100644 --- a/lib/plausible/site.ex +++ b/lib/plausible/site.ex @@ -11,34 +11,34 @@ defmodule Plausible.Site do @derive {Jason.Encoder, only: [:domain, :timezone]} schema "sites" do - field :domain, :string - field :timezone, :string, default: "Etc/UTC" - field :public, :boolean - field :locked, :boolean - field :stats_start_date, :date - field :native_stats_start_at, :naive_datetime - - field :ingest_rate_limit_scale_seconds, :integer, default: 60 - field :ingest_rate_limit_threshold, :integer - - field :domain_changed_from, :string - field :domain_changed_at, :naive_datetime - - embeds_one :imported_data, Plausible.Site.ImportedData, on_replace: :update - - many_to_many :members, User, join_through: Plausible.Site.Membership - has_many :memberships, Plausible.Site.Membership - has_many :invitations, Plausible.Auth.Invitation - has_one :google_auth, GoogleAuth - has_one :weekly_report, Plausible.Site.WeeklyReport - has_one :monthly_report, Plausible.Site.MonthlyReport - has_one :custom_domain, Plausible.Site.CustomDomain - has_one :spike_notification, Plausible.Site.SpikeNotification + field(:domain, :string) + field(:timezone, :string, default: "Etc/UTC") + field(:public, :boolean) + field(:locked, :boolean) + field(:stats_start_date, :date) + field(:native_stats_start_at, :naive_datetime) + + field(:ingest_rate_limit_scale_seconds, :integer, default: 60) + field(:ingest_rate_limit_threshold, :integer) + + field(:domain_changed_from, :string) + field(:domain_changed_at, :naive_datetime) + + embeds_one(:imported_data, Plausible.Site.ImportedData, on_replace: :update) + + many_to_many(:members, User, join_through: Plausible.Site.Membership) + has_many(:memberships, Plausible.Site.Membership) + has_many(:invitations, Plausible.Auth.Invitation) + has_one(:google_auth, GoogleAuth) + has_one(:weekly_report, Plausible.Site.WeeklyReport) + has_one(:monthly_report, Plausible.Site.MonthlyReport) + has_one(:custom_domain, Plausible.Site.CustomDomain) + has_one(:spike_notification, Plausible.Site.SpikeNotification) # If `from_cache?` is set, the struct might be incomplete - see `Plausible.Site.Cache`. # Use `Plausible.Repo.reload!(cached_site)` to pre-fill missing fields if # strictly necessary. - field :from_cache?, :boolean, virtual: true, default: false + field(:from_cache?, :boolean, virtual: true, default: false) timestamps() end @@ -195,7 +195,7 @@ defmodule Plausible.Site do |> Timex.to_date() end - defp clean_domain(changeset, field \\ :domain) do + defp clean_domain(changeset) do clean_domain = (get_field(changeset, field) || "") |> String.trim() @@ -205,7 +205,7 @@ defmodule Plausible.Site do |> String.replace_trailing("/", "") |> String.downcase() - change(changeset, %{field => clean_domain}) + change(changeset, %{domain: clean_domain}) end # https://tools.ietf.org/html/rfc3986#section-2.2 From 724100799cea00e6cf81a7cbcb06929266ea179f Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 14:09:24 +0200 Subject: [PATCH 14/20] !fixup --- lib/plausible/site.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible/site.ex b/lib/plausible/site.ex index 8d7918fadeea..ac40bb50a629 100644 --- a/lib/plausible/site.ex +++ b/lib/plausible/site.ex @@ -197,7 +197,7 @@ defmodule Plausible.Site do defp clean_domain(changeset) do clean_domain = - (get_field(changeset, field) || "") + (get_field(changeset, :domain) || "") |> String.trim() |> String.replace_leading("http://", "") |> String.replace_leading("https://", "") From 3078d3b197e5b7f01b128a32e7fe3d8fcb87eefc Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 14:29:19 +0200 Subject: [PATCH 15/20] !fixup --- lib/plausible/site.ex | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/plausible/site.ex b/lib/plausible/site.ex index ac40bb50a629..114214920ddf 100644 --- a/lib/plausible/site.ex +++ b/lib/plausible/site.ex @@ -11,34 +11,34 @@ defmodule Plausible.Site do @derive {Jason.Encoder, only: [:domain, :timezone]} schema "sites" do - field(:domain, :string) - field(:timezone, :string, default: "Etc/UTC") - field(:public, :boolean) - field(:locked, :boolean) - field(:stats_start_date, :date) - field(:native_stats_start_at, :naive_datetime) - - field(:ingest_rate_limit_scale_seconds, :integer, default: 60) - field(:ingest_rate_limit_threshold, :integer) - - field(:domain_changed_from, :string) - field(:domain_changed_at, :naive_datetime) - - embeds_one(:imported_data, Plausible.Site.ImportedData, on_replace: :update) - - many_to_many(:members, User, join_through: Plausible.Site.Membership) - has_many(:memberships, Plausible.Site.Membership) - has_many(:invitations, Plausible.Auth.Invitation) - has_one(:google_auth, GoogleAuth) - has_one(:weekly_report, Plausible.Site.WeeklyReport) - has_one(:monthly_report, Plausible.Site.MonthlyReport) - has_one(:custom_domain, Plausible.Site.CustomDomain) - has_one(:spike_notification, Plausible.Site.SpikeNotification) + field :domain, :string + field :timezone, :string, default: "Etc/UTC" + field :public, :boolean + field :locked, :boolean + field :stats_start_date, :date + field :native_stats_start_at, :naive_datetime + + field :ingest_rate_limit_scale_seconds, :integer, default: 60 + field :ingest_rate_limit_threshold, :integer + + field :domain_changed_from, :string + field :domain_changed_at, :naive_datetime + + embeds_one :imported_data, Plausible.Site.ImportedData, on_replace: :update + + many_to_many :members, User, join_through: Plausible.Site.Membership + has_many :memberships, Plausible.Site.Membership + has_many :invitations, Plausible.Auth.Invitation + has_one :google_auth, GoogleAuth + has_one :weekly_report, Plausible.Site.WeeklyReport + has_one :monthly_report, Plausible.Site.MonthlyReport + has_one :custom_domain, Plausible.Site.CustomDomain + has_one :spike_notification, Plausible.Site.SpikeNotification # If `from_cache?` is set, the struct might be incomplete - see `Plausible.Site.Cache`. # Use `Plausible.Repo.reload!(cached_site)` to pre-fill missing fields if # strictly necessary. - field(:from_cache?, :boolean, virtual: true, default: false) + field :from_cache?, :boolean, virtual: true, default: false timestamps() end From 5ea9b19624a56693c47f24247ebefad0ac32caf3 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 14:35:26 +0200 Subject: [PATCH 16/20] Implement domain change via Sites API cc @ukutaht --- lib/plausible/site/domain.ex | 6 +++--- .../api/external_sites_controller.ex | 19 +++++++++++++++++++ lib/plausible_web/router.ex | 5 +++-- .../api/external_sites_controller_test.exs | 2 ++ .../controllers/site_controller_test.exs | 2 +- 5 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/plausible/site/domain.ex b/lib/plausible/site/domain.ex index 6151f066ccb2..92ad6e872c20 100644 --- a/lib/plausible/site/domain.ex +++ b/lib/plausible/site/domain.ex @@ -43,15 +43,15 @@ defmodule Plausible.Site.Domain do @spec change(Site.t(), String.t(), Keyword.t()) :: {:ok, Site.t()} | {:error, Ecto.Changeset.t()} - def change(site = %Site{}, new_domain, opts \\ []) when is_binary(new_domain) do + def change(site = %Site{}, new_domain, opts \\ []) do changeset = Site.update_changeset(site, %{domain: new_domain}, opts) changeset = - if Enum.empty?(changeset.changes) do + if Enum.empty?(changeset.changes) and is_nil(changeset.errors[:domain]) do Ecto.Changeset.add_error( changeset, :domain, - "New domain must be different than your current one." + "New domain must be different than the current one" ) else changeset diff --git a/lib/plausible_web/controllers/api/external_sites_controller.ex b/lib/plausible_web/controllers/api/external_sites_controller.ex index cbb1eac57dd4..bbf15c0d012b 100644 --- a/lib/plausible_web/controllers/api/external_sites_controller.ex +++ b/lib/plausible_web/controllers/api/external_sites_controller.ex @@ -49,6 +49,25 @@ defmodule PlausibleWeb.Api.ExternalSitesController do end end + def update_site(conn, %{"site_id" => site_id} = params) do + # for now this only allows to change the domain + site = Sites.get_for_user(conn.assigns[:current_user].id, site_id, [:owner, :admin]) + + if site && Plausible.v2?() do + case Plausible.Site.Domain.change(site, params["domain"]) do + {:ok, site} -> + json(conn, site) + + {:error, changeset} -> + conn + |> put_status(400) + |> json(serialize_errors(changeset)) + end + else + H.not_found(conn, "Site could not be found") + end + end + defp expect_param_key(params, key) do case Map.fetch(params, key) do :error -> {:missing, key} diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 53d61183c400..cd93e5fde587 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -98,11 +98,12 @@ defmodule PlausibleWeb.Router do pipe_through [:public_api, PlausibleWeb.AuthorizeSitesApiPlug] post "/", ExternalSitesController, :create_site - get "/:site_id", ExternalSitesController, :get_site - delete "/:site_id", ExternalSitesController, :delete_site put "/shared-links", ExternalSitesController, :find_or_create_shared_link put "/goals", ExternalSitesController, :find_or_create_goal delete "/goals/:goal_id", ExternalSitesController, :delete_goal + get "/:site_id", ExternalSitesController, :get_site + put "/:site_id", ExternalSitesController, :update_site + delete "/:site_id", ExternalSitesController, :delete_site end scope "/api", PlausibleWeb do diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 3d9dcbbd2f36..27176e08bc56 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -481,6 +481,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "GET /api/v1/sites/:site_id" do setup :create_new_site + @tag :v2_only test "get a site by its domain", %{conn: conn, site: site} do conn = get(conn, "/api/v1/sites/" <> site.domain) @@ -499,6 +500,7 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert json_response(conn, 200) == %{"domain" => new_domain, "timezone" => site.timezone} end + @tag :v2_only test "is 404 when site cannot be found", %{conn: conn} do conn = get(conn, "/api/v1/sites/foobar.baz") diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index 9708e6da33a3..312126564c2d 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -1177,7 +1177,7 @@ defmodule PlausibleWeb.SiteControllerTest do }) resp = html_response(conn, 200) - assert resp =~ "New domain must be different than your current one." + assert resp =~ "New domain must be different than the current one" end @tag :v2_only From ca332544f32ba4d26ad05e7092810ebf108fdd20 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 14:37:10 +0200 Subject: [PATCH 17/20] Update CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c65478552d4..0508b84c0188 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Added - 'Last updated X seconds ago' info to 'current visitors' tooltips - Add support for more Bamboo adapters, i.e. `Bamboo.MailgunAdapter`, `Bamboo.MandrillAdapter`, `Bamboo.SendGridAdapter` plausible/analytics#2649 -- Ability to change domain for existing site (requires numeric IDs data migration, instructions will be provided separately) +- Ability to change domain for existing site (requires numeric IDs data migration, instructions will be provided separately) UI + API (`PUT /api/v1/sites`) ### Fixed - Make goal-filtered CSV export return only unique_conversions timeseries in the 'visitors.csv' file From 359b8e2172041e7a9a016968654dd33039636f18 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Thu, 30 Mar 2023 14:40:01 +0200 Subject: [PATCH 18/20] Credo --- lib/plausible/site/domain.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plausible/site/domain.ex b/lib/plausible/site/domain.ex index 92ad6e872c20..3e88aacfdf6b 100644 --- a/lib/plausible/site/domain.ex +++ b/lib/plausible/site/domain.ex @@ -43,7 +43,7 @@ defmodule Plausible.Site.Domain do @spec change(Site.t(), String.t(), Keyword.t()) :: {:ok, Site.t()} | {:error, Ecto.Changeset.t()} - def change(site = %Site{}, new_domain, opts \\ []) do + def change(%Site{} = site, new_domain, opts \\ []) do changeset = Site.update_changeset(site, %{domain: new_domain}, opts) changeset = From b6bbf2bfa7e1765634cbbc206a656e517cbf016f Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Fri, 31 Mar 2023 10:20:51 +0200 Subject: [PATCH 19/20] !fixup commit missing tests --- .../api/external_sites_controller_test.exs | 48 ++++++++++++++++++- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/test/plausible_web/controllers/api/external_sites_controller_test.exs b/test/plausible_web/controllers/api/external_sites_controller_test.exs index 27176e08bc56..03d3f6230e4f 100644 --- a/test/plausible_web/controllers/api/external_sites_controller_test.exs +++ b/test/plausible_web/controllers/api/external_sites_controller_test.exs @@ -481,7 +481,6 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do describe "GET /api/v1/sites/:site_id" do setup :create_new_site - @tag :v2_only test "get a site by its domain", %{conn: conn, site: site} do conn = get(conn, "/api/v1/sites/" <> site.domain) @@ -500,11 +499,56 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do assert json_response(conn, 200) == %{"domain" => new_domain, "timezone" => site.timezone} end - @tag :v2_only test "is 404 when site cannot be found", %{conn: conn} do conn = get(conn, "/api/v1/sites/foobar.baz") assert json_response(conn, 404) == %{"error" => "Site could not be found"} end end + + describe "PUT /api/v1/sites/:site_id" do + setup :create_new_site + + @tag :v2_only + test "can change domain name", %{conn: conn, site: site} do + old_domain = site.domain + assert old_domain != "new.example.com" + + conn = + put(conn, "/api/v1/sites/#{old_domain}", %{ + "domain" => "new.example.com" + }) + + assert json_response(conn, 200) == %{ + "domain" => "new.example.com", + "timezone" => "UTC" + } + + site = Repo.reload!(site) + + assert site.domain == "new.example.com" + assert site.domain_changed_from == old_domain + end + + @tag :v2_only + test "can't make a no-op change", %{conn: conn, site: site} do + conn = + put(conn, "/api/v1/sites/#{site.domain}", %{ + "domain" => site.domain + }) + + assert json_response(conn, 400) == %{ + "error" => "domain: New domain must be different than the current one" + } + end + + @tag :v2_only + test "domain parameter is required", %{conn: conn, site: site} do + conn = put(conn, "/api/v1/sites/#{site.domain}", %{}) + + assert json_response(conn, 400) == %{ + "error" => "domain: can't be blank" + } + end + end end From 67317b21a80249a774877d71c6c40862af51e119 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 3 Apr 2023 15:33:38 +0200 Subject: [PATCH 20/20] Allow continuous domain change within the same site --- .../20230328062644_allow_domain_change.exs | 2 +- test/plausible/site/domain_test.exs | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/priv/repo/migrations/20230328062644_allow_domain_change.exs b/priv/repo/migrations/20230328062644_allow_domain_change.exs index e5c4ff0e3992..6236fa380261 100644 --- a/priv/repo/migrations/20230328062644_allow_domain_change.exs +++ b/priv/repo/migrations/20230328062644_allow_domain_change.exs @@ -15,7 +15,7 @@ defmodule Plausible.Repo.Migrations.AllowDomainChange do BEGIN IF EXISTS ( SELECT 1 FROM sites - WHERE NEW.domain = domain_changed_from + WHERE (NEW.domain = domain_changed_from AND NEW.id != id) OR (OLD IS NULL AND NEW.domain_changed_from = domain) ) THEN RAISE unique_violation USING CONSTRAINT = 'domain_change_disallowed'; diff --git a/test/plausible/site/domain_test.exs b/test/plausible/site/domain_test.exs index 6a9d56e4ee43..51eb37f826f1 100644 --- a/test/plausible/site/domain_test.exs +++ b/test/plausible/site/domain_test.exs @@ -38,6 +38,19 @@ defmodule Plausible.Site.DomainTest do assert error =~ "This domain cannot be registered" end + test "a single site's domain can be changed back and forth" do + site1 = insert(:site, domain: "foo.example.com") + site2 = insert(:site, domain: "baz.example.com") + + assert {:ok, _} = Domain.change(site1, "bar.example.com") + + assert {:error, _} = Domain.change(site2, "bar.example.com") + assert {:error, _} = Domain.change(site2, "foo.example.com") + + assert {:ok, _} = Domain.change(Repo.reload!(site1), "foo.example.com") + assert {:ok, _} = Domain.change(Repo.reload!(site1), "bar.example.com") + end + test "change info is cleared when the grace period expires" do site = insert(:site)