See if we can avoid exposing user id

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(

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(

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]

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

lib/plausible_web/templates/site/settings_people.html.heex

@@ -15,7 +15,7 @@
     <div class="flow-root">
       <ul class="divide-y divide-gray-200 dark:divide-gray-400">
         <%= for membership <- @memberships do %>
-          <li class="py-4" id={"membership-#{membership.user_id}"}>
+          <li class="py-4" id={"membership-#{membership.id}"}>
             <div class="flex items-center space-x-4">
               <div class="flex-shrink-0">
                 <img
@@ -124,9 +124,9 @@
                       href={
                         Routes.membership_path(
                           @conn,
-                          :update_role_by_user,
+                          :update_role,
                           @site.domain,
-                          membership.user.id,
+                          membership.id,
                           "admin"
                         )
                       }
@@ -164,9 +164,9 @@
                       href={
                         Routes.membership_path(
                           @conn,
-                          :update_role_by_user,
+                          :update_role,
                           @site.domain,
-                          membership.user.id,
+                          membership.id,
                           "viewer"
                         )
                       }
@@ -205,9 +205,9 @@
                       href={
                         Routes.membership_path(
                           @conn,
-                          :remove_member_by_user,
+                          :remove_member,
                           @site.domain,
-                          membership.user.id
+                          membership.id
                         )
                       }
                       method="delete"

test/plausible/site/sites_test.exs

@@ -215,7 +215,7 @@ defmodule Plausible.SitesTest do
       new_site(owner: user1, domain: "one.example.com")
       site2 = new_site(domain: "two.example.com")
 
-      user1 = site2 |> add_guest(user: user1, role: :viewer)
+      user1 = add_guest(site2, user: user1, role: :viewer).user
 
       {:ok, _} = Sites.toggle_pin(user1, site2)
 

test/plausible_web/controllers/site/membership_controller_test.exs

@@ -289,16 +289,21 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
   end
 
   describe "PUT /sites/memberships/:id/role/:new_role" do
-    test "updates a site member's role by user id", %{conn: conn, user: user} do
+    test "updates a site member's role", %{conn: conn, user: user} do
       site = new_site(owner: user)
       collaborator = add_guest(site, role: :editor)
-      assert_team_membership(collaborator, site.team, :editor)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = collaborator
+      assert_team_membership(collaborator.user, site.team, :editor)
 
-      put(conn, "/sites/#{site.domain}/memberships/u/#{collaborator.id}/role/viewer")
+      if Plausible.Teams.read_team_schemas?(user) do
+        put(conn, "/sites/#{site.domain}/memberships/#{gm_id}/role/viewer")
+      else
+        put(conn, "/sites/#{site.domain}/memberships/#{sm_id}/role/viewer")
+      end
 
-      assert_team_membership(collaborator, site.team, :viewer)
+      assert_team_membership(collaborator.user, site.team, :viewer)
 
-      old_model_membership = Repo.get_by(Plausible.Site.Membership, user_id: collaborator.id)
+      old_model_membership = Repo.get_by(Plausible.Site.Membership, user_id: collaborator.user.id)
       assert old_model_membership.role == :viewer
     end
 
@@ -321,7 +326,14 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       guest_membership =
         insert(:guest_membership, team_membership: team_membership, site: site, role: :editor)
 
-      put(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}/role/viewer")
+      if Plausible.Teams.read_team_schemas?(user) do
+        put(conn, "/sites/#{site.domain}/memberships/#{guest_membership.id}/role/viewer")
+      else
+        put(
+          conn,
+          "/sites/#{site.domain}/memberships/#{Enum.at(site.memberships, 1).id}/role/viewer"
+        )
+      end
 
       assert Repo.reload!(guest_membership).role == :viewer
     end
@@ -330,13 +342,18 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       conn: conn,
       user: user
     } do
-      site = insert(:site, memberships: [build(:site_membership, user: user, role: :admin)])
+      site = new_site()
+      collaborator = add_guest(site, role: :editor, user: user)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = collaborator
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/viewer")
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          put(conn, "/sites/#{site.domain}/memberships/#{gm_id}/role/viewer")
+        else
+          put(conn, "/sites/#{site.domain}/memberships/#{sm_id}/role/viewer")
+        end
 
-      membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id)
-
-      assert membership.role == :viewer
+      assert_team_membership(collaborator.user, site.team, :viewer)
       assert redirected_to(conn) == "/#{URI.encode_www_form(site.domain)}"
     end
 
@@ -346,8 +363,14 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
     } do
       site = new_site()
       admin = add_guest(site, user: user, role: :editor)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = admin
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}/role/owner")
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          put(conn, "/sites/#{site.domain}/memberships/#{gm_id}/role/owner")
+        else
+          put(conn, "/sites/#{site.domain}/memberships/#{sm_id}/role/owner")
+        end
 
       assert_team_membership(user, site.team, :editor)
 
@@ -359,9 +382,16 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       conn: conn,
       user: user
     } do
+      # In normal circumstances, there is no guest_membership for the owner,
+      # so the user won't be able to downgrade themselves
+      if not Plausible.Teams.read_team_schemas?(user) do
         site = insert(:site, memberships: [build(:site_membership, user: user, role: :owner)])
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/admin")
+        conn =
+          put(
+            conn,
+            "/sites/#{site.domain}/memberships/#{List.first(site.memberships).id}/role/admin"
+          )
 
         membership = Repo.get_by(Plausible.Site.Membership, user_id: user.id)
 
@@ -370,6 +400,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
         assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
                  "You are not allowed to grant the admin role"
       end
+    end
 
     test "admin can make another user admin", %{
       conn: conn,
@@ -379,18 +410,30 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
 
       add_guest(site, user: user, role: :editor)
       viewer = add_guest(site, user: new_user(), role: :viewer)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = viewer
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{viewer.id}/role/admin")
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          put(conn, "/sites/#{site.domain}/memberships/#{gm_id}/role/admin")
+        else
+          put(conn, "/sites/#{site.domain}/memberships/#{sm_id}/role/admin")
+        end
 
-      assert_team_membership(viewer, site.team, :editor)
+      assert_team_membership(viewer.user, site.team, :editor)
       assert redirected_to(conn) == "/#{URI.encode_www_form(site.domain)}/settings/people"
     end
 
     test "admin can't make themselves an owner", %{conn: conn, user: user} do
       site = new_site()
-      add_guest(site, user: user, role: :editor)
+      admin = add_guest(site, user: user, role: :editor)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = admin
 
-      conn = put(conn, "/sites/#{site.domain}/memberships/u/#{user.id}/role/owner")
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          put(conn, "/sites/#{site.domain}/memberships/#{gm_id}/role/owner")
+        else
+          put(conn, "/sites/#{site.domain}/memberships/#{sm_id}/role/owner")
+        end
 
       assert_team_membership(user, site.team, :editor)
 
@@ -400,40 +443,41 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
   end
 
   describe "DELETE /sites/:domain/memberships/:id" do
-    test "removes a member from a site by user id", %{conn: conn, user: user} do
+    test "removes a member from a site", %{conn: conn, user: user} do
       site = new_site(owner: user)
       admin = add_guest(site, role: :editor)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = admin
+
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          delete(conn, "/sites/#{site.domain}/memberships/#{gm_id}")
+        else
+          delete(conn, "/sites/#{site.domain}/memberships/#{sm_id}")
+        end
 
-      conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
       assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed"
 
-      refute Repo.exists?(from sm in Plausible.Site.Membership, where: sm.user_id == ^admin.id)
+      refute Repo.reload(admin.site_membership)
+      refute Repo.reload(admin.guest_membership)
+      refute Repo.reload(admin.team_membership)
     end
 
     @tag :teams
     test "syncs member removal to team", %{conn: conn, user: user} do
-      admin = insert(:user)
+      site = new_site(owner: user)
+      admin = add_guest(site, role: :editor)
 
-      site =
-        insert(:site,
-          memberships: [
-            build(:site_membership, user: user, role: :owner),
-            build(:site_membership, user: admin, role: :admin)
-          ]
-        )
-        |> Plausible.Teams.load_for_site()
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          delete(conn, "/sites/#{site.domain}/memberships/#{admin.guest_membership.id}")
+        else
+          delete(conn, "/sites/#{site.domain}/memberships/#{admin.site_membership.id}")
+        end
 
-      team_membership =
-        insert(:team_membership, user: admin, team: site.team, role: :guest)
-
-      guest_membership =
-        insert(:guest_membership, team_membership: team_membership, site: site, role: :editor)
-
-      conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
       assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed"
 
-      refute Repo.reload(guest_membership)
-      refute Repo.reload(team_membership)
+      refute Repo.reload(admin.guest_membership)
+      refute Repo.reload(admin.team_membership)
     end
 
     @tag :teams
@@ -441,6 +485,7 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
       conn: conn,
       user: user
     } do
+      if not Plausible.Teams.read_team_schemas?(user) do
         admin = insert(:user)
 
         site =
@@ -475,50 +520,50 @@ defmodule PlausibleWeb.Site.MembershipControllerTest do
             role: :editor
           )
 
-      conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
+        conn =
+          delete(conn, "/sites/#{site.domain}/memberships/#{Enum.at(site.memberships, 1).id}")
+
         assert Phoenix.Flash.get(conn.assigns.flash, :success) =~ "has been removed"
 
         refute Repo.reload(guest_membership)
         assert Repo.reload(another_guest_membership)
         assert Repo.reload(team_membership)
       end
+    end
 
     test "fails to remove a member from a foreign site", %{conn: conn, user: user} do
-      foreign_site =
-        insert(:site,
-          memberships: [
-            build(:site_membership, user: build(:user), role: :admin)
-          ]
-        )
+      foreign_site = new_site()
+      foreign_admin = add_guest(foreign_site, role: :editor)
 
-      [foreign_membership] = foreign_site.memberships
+      site = new_site(owner: user)
 
-      site =
-        insert(:site,
-          memberships: [
-            build(:site_membership, user: user, role: :owner)
-          ]
-        )
-
-      conn = delete(conn, "/sites/#{site.domain}/memberships/u/#{foreign_membership.user_id}")
+      conn =
+        if Plausible.Teams.read_team_schemas?(user) do
+          delete(conn, "/sites/#{site.domain}/memberships/#{foreign_admin.guest_membership.id}")
+        else
+          delete(conn, "/sites/#{site.domain}/memberships/#{foreign_admin.site_membership.id}")
+        end
 
       assert Phoenix.Flash.get(conn.assigns.flash, :error) ==
                "Failed to find membership to remove"
 
-      assert Repo.exists?(
-               from sm in Plausible.Site.Membership,
-                 where: sm.user_id == ^foreign_membership.user.id
-             )
+      assert Repo.reload(foreign_admin.site_membership)
+      assert Repo.reload(foreign_admin.guest_membership)
     end
 
     test "notifies the user who has been removed via email", %{conn: conn, user: user} do
       site = new_site()
       admin = add_guest(site, user: user, role: :editor)
+      %{site_membership: %{id: sm_id}, guest_membership: %{id: gm_id}} = admin
 
-      delete(conn, "/sites/#{site.domain}/memberships/u/#{admin.id}")
+      if Plausible.Teams.read_team_schemas?(user) do
+        delete(conn, "/sites/#{site.domain}/memberships/#{gm_id}")
+      else
+        delete(conn, "/sites/#{site.domain}/memberships/#{sm_id}")
+      end
 
       assert_email_delivered_with(
-        to: [nil: admin.email],
+        to: [nil: admin.user.email],
         subject: @subject_prefix <> "Your access to #{site.domain} has been revoked"
       )
     end

test/plausible_web/controllers/site_controller_test.exs

@@ -526,19 +526,34 @@ defmodule PlausibleWeb.SiteControllerTest do
       conn = get(conn, "/#{site.domain}/settings/people")
       resp = html_response(conn, 200)
 
-      owner_row =
-        text_of_element(resp, "#membership-#{user.id}")
+      {owner_row, editor_row, viewer_row} =
+        if Plausible.Teams.read_team_schemas?(user) do
+          {
+            text_of_element(resp, "#membership-0"),
+            text_of_element(resp, "#membership-#{editor.guest_membership.id}"),
+            text_of_element(resp, "#membership-#{viewer.guest_membership.id}")
+          }
+        else
+          owner_membership =
+            user
+            |> Repo.preload(:site_memberships)
+            |> Map.fetch!(:site_memberships)
+            |> Enum.at(1)
 
-      editor_row = text_of_element(resp, "#membership-#{editor.id}")
-      viewer_row = text_of_element(resp, "#membership-#{viewer.id}")
+          {
+            text_of_element(resp, "#membership-#{owner_membership.id}"),
+            text_of_element(resp, "#membership-#{editor.site_membership.id}"),
+            text_of_element(resp, "#membership-#{viewer.site_membership.id}")
+          }
+        end
 
       assert owner_row =~ user.email
       assert owner_row =~ "Owner"
 
-      assert editor_row =~ editor.email
+      assert editor_row =~ editor.user.email
       refute editor_row =~ "Owner"
 
-      assert viewer_row =~ viewer.email
+      assert viewer_row =~ viewer.user.email
       refute viewer_row =~ "Owner"
     end
 

test/support/teams/test.ex

@@ -60,12 +60,25 @@ defmodule Plausible.Teams.Test do
     role = Keyword.fetch!(args, :role)
     team = Repo.preload(site, :team).team
 
+    site_membership =
       insert(:site_membership, user: user, role: translate_role_to_old_model(role), site: site)
 
     team_membership = insert(:team_membership, team: team, user: user, role: :guest)
+
+    guest_membership =
       insert(:guest_membership, team_membership: team_membership, site: site, role: role)
 
-    user |> Repo.preload([:site_memberships, :team_memberships])
+    user =
+      user
+      |> Repo.reload!()
+      |> Repo.preload([:site_memberships, team_memberships: :guest_memberships])
+
+    %{
+      user: user,
+      site_membership: site_membership,
+      guest_membership: guest_membership,
+      team_membership: team_membership
+    }
   end
 
   def invite_guest(site, invitee_or_email, args \\ []) when not is_nil(invitee_or_email) do