From dd9fe348d9c82c51c31055d6dfbf8ee7f33a1357 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 13:37:04 +0100 Subject: [PATCH 1/4] Implement team deletion and refactor user deletion --- lib/plausible/auth/auth.ex | 84 ++++++++++++------- lib/plausible/teams.ex | 24 ++++++ .../controllers/auth_controller.ex | 8 ++ .../controllers/settings_controller.ex | 23 +++++ lib/plausible_web/router.ex | 2 + .../settings/team_danger_zone.html.heex | 25 ++++++ lib/plausible_web/views/layout_view.ex | 3 + .../controllers/auth_controller_test.exs | 17 +++- 8 files changed, 153 insertions(+), 33 deletions(-) create mode 100644 lib/plausible_web/templates/settings/team_danger_zone.html.heex diff --git a/lib/plausible/auth/auth.ex b/lib/plausible/auth/auth.ex index d146c9d62d09..44d72c0566a5 100644 --- a/lib/plausible/auth/auth.ex +++ b/lib/plausible/auth/auth.ex @@ -70,52 +70,72 @@ defmodule Plausible.Auth do end end + @spec delete_user(Auth.User.t()) :: + {:ok, :deleted} | {:error, :is_only_team_owner | :active_subscription} def delete_user(user) do - Repo.transaction(fn -> - case Teams.get_by_owner(user) do - {:ok, %{setup_complete: false} = team} -> - for site <- Teams.owned_sites(team) do - Plausible.Site.Removal.run(site) - end + case Teams.get_by_owner(user) do + {:ok, %{setup_complete: false} = team} -> + delete_team_and_user(team, user) - Repo.delete_all(from s in Plausible.Billing.Subscription, where: s.team_id == ^team.id) + {:ok, team} -> + with :ok <- check_can_leave_team(team) do + delete_user!(user) + {:ok, :deleted} + end - Repo.delete_all( - from ep in Plausible.Billing.EnterprisePlan, where: ep.team_id == ^team.id - ) + {:error, :multiple_teams} -> + teams = Teams.Users.owned_teams(user) - Plausible.Segments.user_removed(user) - Repo.delete!(team) - Repo.delete!(user) + with :ok <- check_can_leave_teams(teams) do + personal_team = Enum.find(teams, & &1.setup_complete) + delete_team_and_user(personal_team, user) + end - {:ok, team} -> - check_can_leave_team!(team) - Repo.delete!(user) + {:error, :no_team} -> + delete_user!(user) + {:ok, :deleted} + end + end - {:error, :multiple_teams} -> - check_can_leave_teams!(user) - Repo.delete!(user) + defp delete_team_and_user(nil, user) do + delete_user!(user) + {:ok, :deleted} + end - {:error, :no_team} -> - Repo.delete!(user) - end + defp delete_team_and_user(team, user) do + Repo.transaction(fn -> + case Teams.delete(team) do + {:ok, :deleted} -> + delete_user!(user) + :deleted - :deleted + {:error, error} -> + Repo.rollback(error) + end end) end - defp check_can_leave_teams!(user) do - user - |> Teams.Users.owned_teams() - |> Enum.reject(&(&1.setup_complete == false)) - |> Enum.map(fn team -> - check_can_leave_team!(team) + defp delete_user!(user) do + Plausible.Segments.user_removed(user) + Repo.delete!(user) + end + + defp check_can_leave_teams(teams) do + teams + |> Enum.filter(& &1.setup_complete) + |> Enum.reduce_while(:ok, fn team, :ok -> + case check_can_leave_team(team) do + :ok -> {:cont, :ok} + {:error, error} -> {:halt, {:error, error}} + end end) end - defp check_can_leave_team!(team) do - if Teams.Memberships.owners_count(team) <= 1 do - Repo.rollback(:is_only_team_owner) + defp check_can_leave_team(team) do + if Teams.Memberships.owners_count(team) > 1 do + :ok + else + {:error, :is_only_team_owner} end end diff --git a/lib/plausible/teams.ex b/lib/plausible/teams.ex index df6adaf8298c..ed94c16c620b 100644 --- a/lib/plausible/teams.ex +++ b/lib/plausible/teams.ex @@ -7,6 +7,7 @@ defmodule Plausible.Teams do alias __MODULE__ alias Plausible.Auth + alias Plausible.Billing alias Plausible.Repo use Plausible @@ -185,6 +186,29 @@ defmodule Plausible.Teams do end end + @spec delete(Teams.Team.t()) :: {:ok, :deleted} | {:error, :active_subscription} + def delete(team) do + team = Teams.with_subscription(team) + + if Billing.Subscription.Status.active?(team.subscription) do + {:error, :active_subscription} + else + Repo.transaction(fn -> + for site <- Teams.owned_sites(team) do + Plausible.Site.Removal.run(site) + end + + Repo.delete_all(from s in Billing.Subscription, where: s.team_id == ^team.id) + + Repo.delete_all(from ep in Billing.EnterprisePlan, where: ep.team_id == ^team.id) + + Repo.delete!(team) + + :deleted + end) + end + end + @spec get_by_owner(Auth.User.t()) :: {:ok, Teams.Team.t()} | {:error, :no_team | :multiple_teams} def get_by_owner(user) do diff --git a/lib/plausible_web/controllers/auth_controller.ex b/lib/plausible_web/controllers/auth_controller.ex index 9731e32c83de..bfc72398ad69 100644 --- a/lib/plausible_web/controllers/auth_controller.ex +++ b/lib/plausible_web/controllers/auth_controller.ex @@ -476,6 +476,14 @@ defmodule PlausibleWeb.AuthController do {:ok, :deleted} -> logout(conn, params) + {:error, :active_subscription} -> + conn + |> put_flash( + :error, + "You have an active subscription which must be canceled first." + ) + |> redirect(to: Routes.settings_path(conn, :danger_zone)) + {:error, :is_only_team_owner} -> conn |> put_flash( diff --git a/lib/plausible_web/controllers/settings_controller.ex b/lib/plausible_web/controllers/settings_controller.ex index 6eda6e2927c6..c3993c58de29 100644 --- a/lib/plausible_web/controllers/settings_controller.ex +++ b/lib/plausible_web/controllers/settings_controller.ex @@ -132,6 +132,29 @@ defmodule PlausibleWeb.SettingsController do render(conn, :danger_zone, layout: {PlausibleWeb.LayoutView, :settings}) end + def team_danger_zone(conn, _params) do + render(conn, :team_danger_zone, layout: {PlausibleWeb.LayoutView, :settings}) + end + + def delete_team(conn, _params) do + team = conn.assigns.current_team + + case Plausible.Teams.delete(team) do + {:ok, :deleted} -> + conn + |> put_flash(:success, ~s|Team "#{Plausible.Teams.name(team)}" deleted|) + |> redirect(to: Routes.site_path(conn, :index, __team: "none")) + + {:error, :active_subscription} -> + conn + |> put_flash( + :error, + "Team has an active subscription. You must cancel it first." + ) + |> redirect(to: Routes.settings_path(conn, :team_danger_zone)) + end + end + # Preferences actions def update_name(conn, %{"user" => params}) do diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 44acf6843a34..cbfec6a71eff 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -395,6 +395,8 @@ defmodule PlausibleWeb.Router do post "/team/invitations/:invitation_id/accept", InvitationController, :accept_invitation post "/team/invitations/:invitation_id/reject", InvitationController, :reject_invitation delete "/team/invitations/:invitation_id", InvitationController, :remove_team_invitation + get "/team/delete", SettingsController, :team_danger_zone + delete "/team/delete", SettingsController, :delete_team end scope "/", PlausibleWeb do diff --git a/lib/plausible_web/templates/settings/team_danger_zone.html.heex b/lib/plausible_web/templates/settings/team_danger_zone.html.heex new file mode 100644 index 000000000000..acc6f3b2fd6c --- /dev/null +++ b/lib/plausible_web/templates/settings/team_danger_zone.html.heex @@ -0,0 +1,25 @@ +<.notice title="Danger Zone" theme={:red}> + Destructive actions below can result in irrecoverable data loss. Be careful. + + +<.settings_tiles> + <.tile docs="delete-team"> + <:title>Delete Team + <:subtitle>Deleting the team removes all associated sites and collected stats + + <%= if Plausible.Billing.Subscription.Status.active?(@current_team && @current_team.subscription) do %> + <.notice theme={:gray} title="Cannot delete the team at this time"> + The team cannot be deleted because it has an active subscription. Please cancel the subscription first. + + <% else %> + <.button_link + data-confirm="Deleting the team will also delete all the associated sites and data. This action cannot be reversed. Are you sure?" + href={Routes.settings_path(@conn, :delete_team)} + method="delete" + theme="danger" + > + Delete "{Plausible.Teams.name(@current_team)}" + + <% end %> + + diff --git a/lib/plausible_web/views/layout_view.ex b/lib/plausible_web/views/layout_view.ex index 6f1009e4f017..a6bc0905b2fb 100644 --- a/lib/plausible_web/views/layout_view.ex +++ b/lib/plausible_web/views/layout_view.ex @@ -125,6 +125,9 @@ defmodule PlausibleWeb.LayoutView do %{key: "Subscription", value: "billing/subscription", icon: :circle_stack}, if(current_team_role in [:owner, :admin, :billing], do: %{key: "Invoices", value: "billing/invoices", icon: :banknotes} + ), + if(current_team_role == :owner, + do: %{key: "Danger Zone", value: "team/delete", icon: :exclamation_triangle} ) ] |> Enum.reject(&is_nil/1) diff --git a/test/plausible_web/controllers/auth_controller_test.exs b/test/plausible_web/controllers/auth_controller_test.exs index 81835163c1a1..a9fa01353ba7 100644 --- a/test/plausible_web/controllers/auth_controller_test.exs +++ b/test/plausible_web/controllers/auth_controller_test.exs @@ -587,7 +587,6 @@ defmodule PlausibleWeb.AuthControllerTest do insert(:google_auth, site: site, user: user) subscribe_to_growth_plan(user, status: Subscription.Status.deleted()) - subscribe_to_growth_plan(user, status: Subscription.Status.active()) subscribe_to_enterprise_plan(user, site_limit: 1, subscription?: false) {:ok, team} = Plausible.Teams.get_or_create(user) @@ -601,6 +600,22 @@ defmodule PlausibleWeb.AuthControllerTest do refute Repo.get(Plausible.Teams.Team, team.id) end + test "refuses to delete when a personal team has an active subscription", %{ + conn: conn, + user: user + } do + subscribe_to_growth_plan(user, status: Subscription.Status.active()) + + conn = delete(conn, "/me") + + assert redirected_to(conn, 302) == Routes.settings_path(conn, :danger_zone) + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ + "You have an active subscription which must be canceled first" + + assert Repo.reload(user) + end + test "deletes sites that the user owns", %{conn: conn, user: user, site: owner_site} do viewer_site = new_site() add_guest(viewer_site, user: user, role: :viewer) From e87ec8a8a91844f5739932f1889c39b504030f8d Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 14:56:37 +0100 Subject: [PATCH 2/4] Secure team deletion endpoints with team access plug --- lib/plausible_web/controllers/settings_controller.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/plausible_web/controllers/settings_controller.ex b/lib/plausible_web/controllers/settings_controller.ex index c3993c58de29..51b136781280 100644 --- a/lib/plausible_web/controllers/settings_controller.ex +++ b/lib/plausible_web/controllers/settings_controller.ex @@ -14,6 +14,9 @@ defmodule PlausibleWeb.SettingsController do plug Plausible.Plugs.AuthorizeTeamAccess, [:owner, :admin, :billing] when action in [:invoices] + plug Plausible.Plugs.AuthorizeTeamAccess, + [:owner] when action in [:team_danger_zone, :delete_team] + def index(conn, _params) do redirect(conn, to: Routes.settings_path(conn, :preferences)) end From faa9b7b937f0f9e06d63d4dab51acdee725bc51b Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 15:21:53 +0100 Subject: [PATCH 3/4] Add dedicated tests for `Teams.delete/1` --- test/plausible/teams_test.exs | 70 +++++++++++++++++++++++++++++++++++ test/support/teams/test.ex | 7 ++++ 2 files changed, 77 insertions(+) diff --git a/test/plausible/teams_test.exs b/test/plausible/teams_test.exs index 4025dfe66cc0..31801f4653ae 100644 --- a/test/plausible/teams_test.exs +++ b/test/plausible/teams_test.exs @@ -3,9 +3,12 @@ defmodule Plausible.TeamsTest do use Plausible use Plausible.Teams.Test + alias Plausible.Billing.Subscription alias Plausible.Teams alias Plausible.Repo + require Plausible.Billing.Subscription.Status + describe "name/1" do test "returns default name when there's no team" do assert Teams.name(nil) == "My Personal Sites" @@ -323,4 +326,71 @@ defmodule Plausible.TeamsTest do assert Teams.accept_traffic_until(team_of(user)) == ~D[2135-01-01] end end + + describe "delete/1" do + test "deletes a team" do + user = new_user() + subscribe_to_growth_plan(user, status: Subscription.Status.deleted()) + subscribe_to_enterprise_plan(user, site_limit: 1, subscription?: false) + team = team_of(user) + team = Teams.complete_setup(team) + + another_user = new_user() + another_site = new_site(owner: another_user) + another_team = team_of(another_user) + add_member(another_team, user: user, role: :owner) + + site1 = new_site(team: team) + site2 = new_site(team: team) + + viewer_member = new_user() + add_member(team, user: viewer_member, role: :viewer) + owner_member = new_user() + add_member(team, user: owner_member, role: :owner) + + guest_member = new_user() + add_guest(site1, user: guest_member, role: :editor) + + team_invitee = new_user() + invite_member(team, team_invitee, inviter: user, role: :admin) + guest_invitee = new_user() + invite_guest(site2, guest_invitee, inviter: user, role: :viewer) + + assert {:ok, :deleted} = Teams.delete(team) + + refute Repo.reload(team) + + assert Repo.reload(another_user) + assert Repo.reload(another_team) + assert Repo.reload(another_site) + + refute Repo.reload(site1) + refute Repo.reload(site2) + + assert Repo.reload(viewer_member) + refute_team_member(viewer_member, team) + + assert Repo.reload(owner_member) + refute_team_member(owner_member, team) + + assert Repo.reload(guest_member) + refute_team_member(guest_member, team) + + assert Repo.reload(team_invitee) + refute_team_invitation(team, team_invitee.email) + + assert Repo.reload(guest_invitee) + refute_team_invitation(team, guest_invitee.email) + end + + test "does not delete a team with active subscription" do + user = new_user() + subscribe_to_growth_plan(user, status: Subscription.Status.active()) + team = team_of(user) + + assert {:error, :active_subscription} = Teams.delete(team) + + assert Repo.reload(team) + end + end end diff --git a/test/support/teams/test.ex b/test/support/teams/test.ex index 69fe85c87d80..445a721bbddb 100644 --- a/test/support/teams/test.ex +++ b/test/support/teams/test.ex @@ -340,6 +340,13 @@ defmodule Plausible.Teams.Test do ) end + def refute_team_invitation(team, email) do + refute Repo.get_by(Plausible.Teams.Invitation, + email: email, + team_id: team.id + ) + end + def assert_site_transfer(site, %Plausible.Auth.User{} = user) do assert_site_transfer(site, user.email) end From b63f404c0cd0fab94e37f8b811532d0b500c5a3f Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Thu, 13 Mar 2025 16:38:06 +0100 Subject: [PATCH 4/4] Test new controller actions and views --- .../controllers/settings_controller_test.exs | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/test/plausible_web/controllers/settings_controller_test.exs b/test/plausible_web/controllers/settings_controller_test.exs index 309fa043b74b..1b4c8f9402b1 100644 --- a/test/plausible_web/controllers/settings_controller_test.exs +++ b/test/plausible_web/controllers/settings_controller_test.exs @@ -1107,6 +1107,29 @@ defmodule PlausibleWeb.SettingsControllerTest do end end + describe "GET /settings/danger-zone" do + setup [:create_user, :log_in, :create_team] + + test "without active subscription", %{conn: conn} do + conn = get(conn, Routes.settings_path(conn, :danger_zone)) + + assert html = html_response(conn, 200) + + refute html =~ "Your account cannot be deleted because you have an active subscription" + assert html =~ "Delete my account" + end + + test "with active subscription", %{conn: conn, user: user} do + subscribe_to_growth_plan(user) + conn = get(conn, Routes.settings_path(conn, :danger_zone)) + + assert html = html_response(conn, 200) + + assert html =~ "Your account cannot be deleted because you have an active subscription" + refute html =~ "Delete my account" + end + end + describe "Team Settings" do setup [:create_user, :log_in] @@ -1172,6 +1195,101 @@ defmodule PlausibleWeb.SettingsControllerTest do assert text(html_response(conn, 200)) =~ "can't be blank" end + + test "GET /settings/team/delete - without active subscription", %{conn: conn, user: user} do + {:ok, team} = Plausible.Teams.get_or_create(user) + + team = + team + |> Plausible.Teams.complete_setup() + |> Ecto.Changeset.change(name: "Foo Crew") + |> Repo.update!() + + conn = set_current_team(conn, team) + conn = get(conn, Routes.settings_path(conn, :team_danger_zone)) + + assert html = html_response(conn, 200) + + refute html =~ "The team cannot be deleted because it has an active subscription" + assert html =~ "Delete \"Foo Crew\"" + end + + test "GET /settings/team/delete - with active subscription", %{conn: conn, user: user} do + user = subscribe_to_growth_plan(user) + team = team_of(user) + + team = + team + |> Plausible.Teams.complete_setup() + |> Ecto.Changeset.change(name: "Foo Crew") + |> Repo.update!() + + conn = set_current_team(conn, team) + conn = get(conn, Routes.settings_path(conn, :team_danger_zone)) + + assert html = html_response(conn, 200) + + assert html =~ "The team cannot be deleted because it has an active subscription" + refute html =~ "Delete \"Foo Crew\"" + end + + test "GET /settings/team/delete - permission denied", %{conn: conn, user: user} do + another_user = new_user() |> subscribe_to_growth_plan() + team = team_of(another_user) + add_member(team, user: user, role: :admin) + conn = set_current_team(conn, team) + conn = get(conn, Routes.settings_path(conn, :team_danger_zone)) + + assert redirected_to(conn, 302) == Routes.site_path(conn, :index) + end + + test "DELETE /settings/team/delete - deletes a team", %{conn: conn, user: user} do + {:ok, team} = Plausible.Teams.get_or_create(user) + + team = + team + |> Plausible.Teams.complete_setup() + |> Ecto.Changeset.change(name: "Foo Crew") + |> Repo.update!() + + conn = set_current_team(conn, team) + conn = delete(conn, Routes.settings_path(conn, :delete_team)) + + assert redirected_to(conn, 302) == Routes.site_path(conn, :index, __team: "none") + + assert Phoenix.Flash.get(conn.assigns.flash, :success) == "Team \"Foo Crew\" deleted" + end + + test "DELETE /settings/team/delete - fails when there's an active subscription", %{ + conn: conn, + user: user + } do + subscribe_to_growth_plan(user) + team = team_of(user) + + team = + team + |> Plausible.Teams.complete_setup() + |> Ecto.Changeset.change(name: "Foo Crew") + |> Repo.update!() + + conn = set_current_team(conn, team) + conn = delete(conn, Routes.settings_path(conn, :delete_team)) + + assert redirected_to(conn, 302) == Routes.settings_path(conn, :team_danger_zone) + + assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ "Team has an active subscription" + end + + test "DELETE /settings/team/delete - permission denied", %{conn: conn, user: user} do + another_user = new_user() |> subscribe_to_growth_plan() + team = team_of(another_user) + add_member(team, user: user, role: :admin) + conn = set_current_team(conn, team) + conn = delete(conn, Routes.settings_path(conn, :delete_team)) + + assert redirected_to(conn, 302) == Routes.site_path(conn, :index) + end end defp configure_enterprise_plan(user, attrs \\ []) do