From 1b3c1f708394d6f1756254ab6b10bb7d81e0ea9c Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 10:29:05 +0100 Subject: [PATCH 1/3] Allow site transfer between different teams of the same user --- lib/plausible/site/admin.ex | 5 +-- .../site/memberships/accept_invitation.ex | 8 ++-- .../site/memberships/create_invitation.ex | 7 --- lib/plausible/teams/invitations.ex | 9 ++-- .../controllers/invitation_controller.ex | 5 +++ .../controllers/site/membership_controller.ex | 7 --- test/plausible/site/admin_test.exs | 38 +++++++++++----- .../memberships/accept_invitation_test.exs | 32 ++++++++++++++ .../memberships/create_invitation_test.exs | 8 ---- .../invitation_controller_test.exs | 43 +++++++++++++++++++ 10 files changed, 118 insertions(+), 44 deletions(-) diff --git a/lib/plausible/site/admin.ex b/lib/plausible/site/admin.ex index 5b18ff6c377a..4f22c1ffe354 100644 --- a/lib/plausible/site/admin.ex +++ b/lib/plausible/site/admin.ex @@ -144,9 +144,6 @@ defmodule Plausible.SiteAdmin do else {:error, :user_not_found} -> {:error, "User could not be found"} - - {:error, :transfer_to_self} -> - {:error, "User is already an owner of one of the sites"} end end @@ -169,7 +166,7 @@ defmodule Plausible.SiteAdmin do {:error, "User could not be found"} {:error, :transfer_to_self} -> - {:error, "User is already an owner of one of the sites"} + {:error, "The site is already in the picked team"} {:error, :no_plan} -> {:error, "The new owner does not have a subscription"} diff --git a/lib/plausible/site/memberships/accept_invitation.ex b/lib/plausible/site/memberships/accept_invitation.ex index 6d64aaf9064a..9c73d8e24f70 100644 --- a/lib/plausible/site/memberships/accept_invitation.ex +++ b/lib/plausible/site/memberships/accept_invitation.ex @@ -75,8 +75,8 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do defp transfer_ownership(site, new_owner, team) do site = Repo.preload(site, :team) - with :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_owner, :owner), - {:ok, new_team} <- maybe_get_team(new_owner, team), + with {:ok, new_team} <- maybe_get_team(new_owner, team), + :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_team, :owner), :ok <- check_can_transfer_site(new_team, new_owner), :ok <- Teams.Invitations.ensure_can_take_ownership(site, new_team), :ok <- Teams.Invitations.transfer_site(site, new_team) do @@ -89,8 +89,8 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do defp do_accept_ownership_transfer(site_transfer, new_owner, team) do site = Repo.preload(site_transfer.site, :team) - with :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_owner, :owner), - {:ok, new_team} <- maybe_get_team(new_owner, team), + with {:ok, new_team} <- maybe_get_team(new_owner, team), + :ok <- Teams.Invitations.ensure_transfer_valid(site.team, new_team, :owner), :ok <- check_can_transfer_site(new_team, new_owner), :ok <- Teams.Invitations.ensure_can_take_ownership(site, new_team), :ok <- Teams.Invitations.accept_site_transfer(site_transfer, new_team) do diff --git a/lib/plausible/site/memberships/create_invitation.ex b/lib/plausible/site/memberships/create_invitation.ex index 4d31af79de6b..2310d9c79595 100644 --- a/lib/plausible/site/memberships/create_invitation.ex +++ b/lib/plausible/site/memberships/create_invitation.ex @@ -13,7 +13,6 @@ defmodule Plausible.Site.Memberships.CreateInvitation do @type invite_error() :: Ecto.Changeset.t() | :already_a_member - | :transfer_to_self | :no_plan | {:over_limit, non_neg_integer()} | :forbidden @@ -65,12 +64,6 @@ defmodule Plausible.Site.Memberships.CreateInvitation do invitee_email ), invitee = Plausible.Auth.find_user_by(email: invitee_email), - :ok <- - Teams.Invitations.ensure_transfer_valid( - site.team, - invitee, - role - ), :ok <- Teams.Invitations.ensure_new_membership( site, diff --git a/lib/plausible/teams/invitations.ex b/lib/plausible/teams/invitations.ex index e8b1ece88fc3..2ef147a0ec36 100644 --- a/lib/plausible/teams/invitations.ex +++ b/lib/plausible/teams/invitations.ex @@ -233,10 +233,11 @@ defmodule Plausible.Teams.Invitations do @doc false def ensure_transfer_valid(_team, nil, :owner), do: :ok - def ensure_transfer_valid(team, new_owner, :owner) do - case Teams.Memberships.team_role(team, new_owner) do - {:ok, :owner} -> {:error, :transfer_to_self} - _ -> :ok + def ensure_transfer_valid(team, new_team, :owner) do + if team.id == new_team.id do + {:error, :transfer_to_self} + else + :ok end end diff --git a/lib/plausible_web/controllers/invitation_controller.ex b/lib/plausible_web/controllers/invitation_controller.ex index eee17245694e..ec8bb6075f23 100644 --- a/lib/plausible_web/controllers/invitation_controller.ex +++ b/lib/plausible_web/controllers/invitation_controller.ex @@ -41,6 +41,11 @@ defmodule PlausibleWeb.InvitationController do |> put_flash(:error, "Invitation missing or already accepted") |> redirect(to: "/sites") + {:error, :transfer_to_self} -> + conn + |> put_flash(:error, "The site is already in the current team") + |> redirect(to: "/sites") + {:error, :permission_denied} -> conn |> put_flash(:error, "You can't add sites in the current team") diff --git a/lib/plausible_web/controllers/site/membership_controller.ex b/lib/plausible_web/controllers/site/membership_controller.ex index 18dde77f6de7..1652fea7c737 100644 --- a/lib/plausible_web/controllers/site/membership_controller.ex +++ b/lib/plausible_web/controllers/site/membership_controller.ex @@ -118,13 +118,6 @@ defmodule PlausibleWeb.Site.MembershipController do |> put_flash(:success, "Site transfer request has been sent to #{email}") |> redirect(external: Routes.site_path(conn, :settings_people, site.domain)) - {:error, :transfer_to_self} -> - conn - |> put_flash(:ttl, :timer.seconds(5)) - |> put_flash(:error_title, "Transfer error") - |> put_flash(:error, "Can't transfer ownership to existing owner") - |> redirect(external: Routes.site_path(conn, :settings_people, site.domain)) - {:error, changeset} -> errors = Plausible.ChangesetHelpers.traverse_errors(changeset) diff --git a/test/plausible/site/admin_test.exs b/test/plausible/site/admin_test.exs index 0281826b80b8..76d58f0235fd 100644 --- a/test/plausible/site/admin_test.exs +++ b/test/plausible/site/admin_test.exs @@ -38,14 +38,6 @@ defmodule Plausible.Site.AdminTest do {:error, "User could not be found"} end - test "new owner can't be the same as old owner", %{conn: conn, transfer_action: action} do - current_owner = new_user() - site = new_site(owner: current_owner) - - assert {:error, "User is already an owner of one of the sites"} = - action.(conn, [site], %{"email" => current_owner.email}) - end - test "initiates ownership transfer for multiple sites in one action", %{ conn: conn, transfer_action: action @@ -81,11 +73,14 @@ defmodule Plausible.Site.AdminTest do {:error, "User could not be found"} end - test "new owner can't be the same as old owner", %{conn: conn, transfer_direct_action: action} do + test "new team (via owner) can't be the same as old team", %{ + conn: conn, + transfer_direct_action: action + } do current_owner = new_user() site = new_site(owner: current_owner) - assert {:error, "User is already an owner of one of the sites"} = + assert {:error, "The site is already in the picked team"} = action.(conn, [site], %{"email" => current_owner.email}) end @@ -147,6 +142,29 @@ defmodule Plausible.Site.AdminTest do }) end + test "the new owner can be the same as old owner given selected team is different", %{ + conn: conn, + transfer_direct_action: action + } do + today = Date.utc_today() + current_owner = new_user() + site = new_site(owner: current_owner) + + another_owner = + new_user() + |> subscribe_to_growth_plan(last_bill_date: Date.shift(today, day: -5)) + + another_site = new_site(owner: another_owner) + new_team = another_site.team + add_member(new_team, user: current_owner, role: :owner) + + assert :ok = + action.(conn, [site], %{ + "email" => current_owner.email, + "team_id" => new_team.identifier + }) + end + @tag :ee_only test "new owner's plan must accommodate the transferred site", %{ conn: conn, diff --git a/test/plausible/site/memberships/accept_invitation_test.exs b/test/plausible/site/memberships/accept_invitation_test.exs index 908a793ba068..957b9a134130 100644 --- a/test/plausible/site/memberships/accept_invitation_test.exs +++ b/test/plausible/site/memberships/accept_invitation_test.exs @@ -65,6 +65,24 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do ) end + test "allows transferring between teams of the same owner" do + current_owner = new_user() |> subscribe_to_growth_plan() + another_owner = new_user() |> subscribe_to_growth_plan() + + site1 = new_site(owner: current_owner) + site2 = new_site(owner: current_owner) + + new_team = team_of(another_owner) + add_member(new_team, user: current_owner, role: :owner) + + assert {:ok, _} = + AcceptInvitation.bulk_transfer_ownership_direct( + [site1, site2], + current_owner, + new_team + ) + end + test "does not allow transferring ownership to a team where user has no permission" do other_owner = new_user() |> subscribe_to_growth_plan() other_team = team_of(other_owner) @@ -664,6 +682,20 @@ defmodule Plausible.Site.Memberships.AcceptInvitationTest do AcceptInvitation.accept_invitation(transfer.transfer_id, current_owner) end + test "allows transferring between different teams of the same owner" do + current_owner = new_user() |> subscribe_to_growth_plan() + site = new_site(owner: current_owner) + + another_owner = new_user() |> subscribe_to_growth_plan() + new_team = team_of(another_owner) + add_member(new_team, user: current_owner, role: :owner) + + transfer = invite_transfer(site, current_owner, inviter: current_owner) + + assert {:ok, _} = + AcceptInvitation.accept_invitation(transfer.transfer_id, current_owner, new_team) + end + @tag :ee_only test "does not allow transferring to and account without suitable plan" do current_owner = new_user() diff --git a/test/plausible/site/memberships/create_invitation_test.exs b/test/plausible/site/memberships/create_invitation_test.exs index 50e1f619b56a..bb0e16f37f80 100644 --- a/test/plausible/site/memberships/create_invitation_test.exs +++ b/test/plausible/site/memberships/create_invitation_test.exs @@ -150,14 +150,6 @@ defmodule Plausible.Site.Memberships.CreateInvitationTest do CreateInvitation.create_invitation(site, inviter, invitee.email, :owner) end - test "does not allow transferring ownership to existing owner" do - inviter = new_user(email: "vini@plausible.test") - site = new_site(owner: inviter) - - assert {:error, :transfer_to_self} = - CreateInvitation.create_invitation(site, inviter, "vini@plausible.test", :owner) - end - test "allows creating an ownership transfer even when at team member limit" do inviter = new_user() site = new_site(owner: inviter) diff --git a/test/plausible_web/controllers/invitation_controller_test.exs b/test/plausible_web/controllers/invitation_controller_test.exs index 6107dc736689..cf0ab81edd6f 100644 --- a/test/plausible_web/controllers/invitation_controller_test.exs +++ b/test/plausible_web/controllers/invitation_controller_test.exs @@ -110,6 +110,49 @@ defmodule PlausibleWeb.Site.InvitationControllerTest do "You can't add sites in the current team" end + test "fails when transferring to the same team", %{conn: conn, user: user} do + current_owner = user |> subscribe_to_growth_plan() + site = new_site(owner: current_owner) + + transfer = invite_transfer(site, current_owner, inviter: current_owner) + + conn = post(conn, "/sites/invitations/#{transfer.transfer_id}/accept") + + assert redirected_to(conn, 302) == "/sites" + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ + "The site is already in the current team" + end + + test "allows transferring between different teams of the same owner", %{ + conn: conn, + user: user + } do + current_owner = user |> subscribe_to_growth_plan() + site = new_site(owner: current_owner) + + another_owner = new_user() |> subscribe_to_growth_plan() + new_team = team_of(another_owner) + add_member(new_team, user: current_owner, role: :owner) + + transfer = invite_transfer(site, current_owner, inviter: current_owner) + + conn = set_current_team(conn, new_team) + + conn = post(conn, "/sites/invitations/#{transfer.transfer_id}/accept") + + assert redirected_to(conn, 302) == "/#{URI.encode_www_form(site.domain)}" + + assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ + "You now have access to" + + refute Repo.reload(transfer) + + assert_team_membership(current_owner, new_team, :owner) + + assert_team_attached(site, new_team.id) + end + @tag :ee_only test "fails when new owner has no plan", %{conn: conn, user: user} do old_owner = new_user() From 51657e5d81ae00fc017bfe7473500888af969fc2 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 10:38:08 +0100 Subject: [PATCH 2/3] Fix typespec --- lib/plausible/site/memberships/accept_invitation.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/plausible/site/memberships/accept_invitation.ex b/lib/plausible/site/memberships/accept_invitation.ex index 9c73d8e24f70..f41c42dbfd7c 100644 --- a/lib/plausible/site/memberships/accept_invitation.ex +++ b/lib/plausible/site/memberships/accept_invitation.ex @@ -33,6 +33,7 @@ defmodule Plausible.Site.Memberships.AcceptInvitation do | Billing.Quota.Limits.over_limits_error() | Ecto.Changeset.t() | :no_plan + | :transfer_to_self | :multiple_teams | :permission_denied From 2dd79d22b42f7c6ad7c21060461dd46b522150f5 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 11:23:45 +0100 Subject: [PATCH 3/3] More type and branching fixes to satisfy dialyzer --- lib/plausible/site/admin.ex | 3 +++ lib/plausible/site/memberships/create_invitation.ex | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/plausible/site/admin.ex b/lib/plausible/site/admin.ex index 4f22c1ffe354..f8b162175ead 100644 --- a/lib/plausible/site/admin.ex +++ b/lib/plausible/site/admin.ex @@ -144,6 +144,9 @@ defmodule Plausible.SiteAdmin do else {:error, :user_not_found} -> {:error, "User could not be found"} + + {:error, %Ecto.Changeset{}} -> + {:error, "Site transfer request has failed for one of the sites"} end end diff --git a/lib/plausible/site/memberships/create_invitation.ex b/lib/plausible/site/memberships/create_invitation.ex index 2310d9c79595..0e011383aef0 100644 --- a/lib/plausible/site/memberships/create_invitation.ex +++ b/lib/plausible/site/memberships/create_invitation.ex @@ -13,7 +13,6 @@ defmodule Plausible.Site.Memberships.CreateInvitation do @type invite_error() :: Ecto.Changeset.t() | :already_a_member - | :no_plan | {:over_limit, non_neg_integer()} | :forbidden