From 672429b9d1694d0b77623fac08292305e269f725 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Wed, 13 Nov 2024 11:25:44 +0100 Subject: [PATCH] See if we can avoid exposing user id --- lib/plausible/teams/adapter/read/sites.ex | 2 + lib/plausible/teams/memberships.ex | 10 + .../controllers/site/membership_controller.ex | 38 ++- lib/plausible_web/router.ex | 6 +- .../templates/site/settings_people.html.heex | 14 +- test/plausible/site/sites_test.exs | 2 +- .../site/membership_controller_test.exs | 239 +++++++++++------- .../controllers/site_controller_test.exs | 27 +- test/support/teams/test.ex | 19 +- 9 files changed, 234 insertions(+), 123 deletions(-) diff --git a/lib/plausible/teams/adapter/read/sites.ex b/lib/plausible/teams/adapter/read/sites.ex index 0e646598f..3b1eeba6e 100644 --- a/lib/plausible/teams/adapter/read/sites.ex +++ b/lib/plausible/teams/adapter/read/sites.ex @@ -123,6 +123,7 @@ defmodule Plausible.Teams.Adapter.Read.Sites do where: tm.team_id == ^site.team_id, where: tm.role == :owner, select: %Plausible.Site.Membership{ + id: 0, user_id: tm.user_id, role: tm.role } @@ -135,6 +136,7 @@ defmodule Plausible.Teams.Adapter.Read.Sites do inner_join: tm in assoc(gm, :team_membership), where: gm.site_id == ^site.id, select: %Plausible.Site.Membership{ + id: gm.id, user_id: tm.user_id, role: fragment( diff --git a/lib/plausible/teams/memberships.ex b/lib/plausible/teams/memberships.ex index 13927e483..5fc02a23f 100644 --- a/lib/plausible/teams/memberships.ex +++ b/lib/plausible/teams/memberships.ex @@ -114,6 +114,16 @@ defmodule Plausible.Teams.Memberships do :ok end + def get_guest_membership(id) do + query = + from gm in Teams.GuestMembership, + inner_join: tm in assoc(gm, :team_membership), + where: gm.id == ^id, + preload: [team_membership: tm] + + Repo.one(query) + end + defp get_guest_membership(site_id, user_id) do query = from( diff --git a/lib/plausible_web/controllers/site/membership_controller.ex b/lib/plausible_web/controllers/site/membership_controller.ex index d17849f77..319ba310f 100644 --- a/lib/plausible_web/controllers/site/membership_controller.ex +++ b/lib/plausible_web/controllers/site/membership_controller.ex @@ -15,6 +15,7 @@ defmodule PlausibleWeb.Site.MembershipController do use Plausible alias Plausible.Sites alias Plausible.Site.{Membership, Memberships} + alias Plausible.Teams @only_owner_is_allowed_to [:transfer_ownership_form, :transfer_ownership] @@ -151,11 +152,21 @@ defmodule PlausibleWeb.Site.MembershipController do |> Enum.map(fn {k, v} -> {v, k} end) |> Enum.into(%{}) - def update_role_by_user(conn, %{"id" => user_id, "new_role" => new_role_str}) do + def update_role(conn, %{"membership_id" => id, "new_role" => new_role_str}) do %{site: site, current_user: current_user, current_user_role: current_user_role} = conn.assigns membership = - Membership |> Repo.get_by!(user_id: user_id, site_id: site.id) |> Repo.preload(:user) + if Teams.read_team_schemas?(current_user) do + guest_membership = Teams.Memberships.get_guest_membership(id) + + if guest_membership do + Membership + |> Repo.get_by!(user_id: guest_membership.team_membership.user_id, site_id: site.id) + |> Repo.preload(:user) + end + else + Membership |> Repo.get_by!(id: id, site_id: site.id) |> Repo.preload(:user) + end new_role = Map.fetch!(@role_mappings, new_role_str) @@ -204,14 +215,29 @@ defmodule PlausibleWeb.Site.MembershipController do defp can_grant_role_to_other?(:admin, :viewer), do: true defp can_grant_role_to_other?(_, _), do: false - def remove_member_by_user(conn, %{"id" => user_id} = _params) do + def remove_member(conn, %{"membership_id" => id} = _params) do + current_user = conn.assigns.current_user site = conn.assigns.site - site_id = site.id + + membership_id = + if Teams.read_team_schemas?(current_user) do + guest_membership = + Teams.Memberships.get_guest_membership(id) + + if guest_membership do + (Repo.get_by(Membership, + user_id: guest_membership.team_membership.user_id, + site_id: site.id + ) || %{id: 0}).id + end + else + id + end membership_q = from m in Membership, - where: m.user_id == ^user_id, - where: m.site_id == ^site_id, + where: m.id == ^membership_id, + where: m.site_id == ^site.id, inner_join: user in assoc(m, :user), inner_join: site in assoc(m, :site), preload: [user: user, site: site] diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 335fe37ca..e241d5032 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -451,11 +451,11 @@ defmodule PlausibleWeb.Router do get "/sites/:domain/transfer-ownership", Site.MembershipController, :transfer_ownership_form post "/sites/:domain/transfer-ownership", Site.MembershipController, :transfer_ownership - put "/sites/:domain/memberships/u/:id/role/:new_role", + put "/sites/:domain/memberships/:membership_id/role/:new_role", Site.MembershipController, - :update_role_by_user + :update_role - delete "/sites/:domain/memberships/u/:id", Site.MembershipController, :remove_member_by_user + delete "/sites/:domain/memberships/:membership_id", Site.MembershipController, :remove_member get "/sites/:domain/weekly-report/unsubscribe", UnsubscribeController, :weekly_report get "/sites/:domain/monthly-report/unsubscribe", UnsubscribeController, :monthly_report diff --git a/lib/plausible_web/templates/site/settings_people.html.heex b/lib/plausible_web/templates/site/settings_people.html.heex index fdb43ddbb..433b87b12 100644 --- a/lib/plausible_web/templates/site/settings_people.html.heex +++ b/lib/plausible_web/templates/site/settings_people.html.heex @@ -15,7 +15,7 @@