Make site param configurable for AuthorizeSiteAccess plug

lib/plausible_web/controllers/google_analytics_controller.ex

@@ -8,7 +8,10 @@ defmodule PlausibleWeb.GoogleAnalyticsController do
 
   plug(PlausibleWeb.RequireAccountPlug)
 
-  plug(PlausibleWeb.Plugs.AuthorizeSiteAccess, [:owner, :admin, :super_admin])
+  plug(PlausibleWeb.Plugs.AuthorizeSiteAccess,
+    site_param: {:path, :website},
+    allowed_roles: [:owner, :admin, :super_admin]
+  )
 
   def property_form(
         conn,

lib/plausible_web/controllers/invitation_controller.ex

@@ -4,7 +4,11 @@ defmodule PlausibleWeb.InvitationController do
   plug PlausibleWeb.RequireAccountPlug
 
   plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
-       [:owner, :admin] when action in [:remove_invitation]
+       [
+         site_param: {:path, :website},
+         allowed_roles: [:owner, :admin]
+       ]
+       when action in [:remove_invitation]
 
   def accept_invitation(conn, %{"invitation_id" => invitation_id}) do
     case Plausible.Site.Memberships.accept_invitation(invitation_id, conn.assigns.current_user) do

lib/plausible_web/controllers/site/membership_controller.ex

@@ -18,10 +18,20 @@ defmodule PlausibleWeb.Site.MembershipController do
   @only_owner_is_allowed_to [:transfer_ownership_form, :transfer_ownership]
 
   plug PlausibleWeb.RequireAccountPlug
-  plug PlausibleWeb.Plugs.AuthorizeSiteAccess, [:owner] when action in @only_owner_is_allowed_to
 
   plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
-       [:owner, :admin] when action not in @only_owner_is_allowed_to
+       [
+         site_param: {:path, :website},
+         allowed_roles: [:owner]
+       ]
+       when action in @only_owner_is_allowed_to
+
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
+       [
+         site_param: {:path, :website},
+         allowed_roles: [:owner, :admin]
+       ]
+       when action not in @only_owner_is_allowed_to
 
   def invite_member_form(conn, _params) do
     site =

lib/plausible_web/controllers/site_controller.ex

@@ -6,12 +6,14 @@ defmodule PlausibleWeb.SiteController do
   alias Plausible.Sites
   alias Plausible.Billing.Quota
 
-  plug(PlausibleWeb.RequireAccountPlug)
+  plug PlausibleWeb.RequireAccountPlug
 
-  plug(
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
-    PlausibleWeb.Plugs.AuthorizeSiteAccess,
+       [
-    [:owner, :admin, :super_admin] when action not in [:new, :create_site]
+         site_param: {:path, :website},
-  )
+         allowed_roles: [:owner, :admin, :super_admin]
+       ]
+       when action not in [:new, :create_site]
 
   def new(conn, params) do
     flow = params["flow"] || PlausibleWeb.Flows.register()

lib/plausible_web/controllers/stats_controller.ex

@@ -48,7 +48,8 @@ defmodule PlausibleWeb.StatsController do
   alias Plausible.Stats.{Filters, Query}
   alias PlausibleWeb.Api
 
-  plug(PlausibleWeb.Plugs.AuthorizeSiteAccess when action in [:stats, :csv_export])
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
+       [site_param: {:path, :domain}] when action in [:stats, :csv_export]
 
   def stats(%{assigns: %{site: site}} = conn, _params) do
     site = Plausible.Repo.preload(site, :owner)

lib/plausible_web/plugs/authorize_site_access.ex

@@ -10,22 +10,27 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccess do
 
   @all_roles [:public, :viewer, :admin, :super_admin, :owner]
 
-  def init([]), do: @all_roles
+  def all_roles(), do: @all_roles
 
-  def init(allowed_roles) do
+  def init(opts) do
+    allowed_roles = Keyword.get(opts, :allowed_roles, @all_roles)
+    site_param = Keyword.fetch!(opts, :site_param)
     unknown_roles = allowed_roles -- @all_roles
 
     if unknown_roles != [] do
       raise ArgumentError, "Unknown allowed roles configured: #{inspect(unknown_roles)}"
     end
 
-    allowed_roles
+    %{site_param: site_param, allowed_roles: allowed_roles}
   end
 
-  def call(conn, allowed_roles) do
+  def call(conn, %{site_param: site_param, allowed_roles: allowed_roles}) do
     current_user = conn.assigns[:current_user]
 
-    with {:ok, %{site: site, role: membership_role}} <- get_site_with_role(conn, current_user),
+    domain = get_domain(conn, site_param)
+
+    with {:ok, %{site: site, role: membership_role}} <-
+           get_site_with_role(conn, current_user, domain),
          {:ok, shared_link} <- maybe_get_shared_link(conn, site) do
       role =
         cond do
@@ -63,13 +68,19 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccess do
     end
   end
 
-  defp get_site_with_role(conn, current_user) do
+  defp get_domain(conn, {:path, param}) when is_atom(param) do
-    # addition is flimsy, do we need an extra argument on plug init to control where we look for the domain?
+    conn.path_params[Atom.to_string(param)]
-    domain =
+  end
-      conn.path_params["domain"] || conn.path_params["website"] ||
-        (conn.method == "POST" && conn.path_info == ["api", "docs", "query"] &&
-           conn.params["site_id"])
 
+  defp get_domain(conn, param) when is_atom(param) do
+    conn.params[Atom.to_string(param)]
+  end
+
+  defp get_domain(_conn, _) do
+    raise ArgumentError, "site_param must be either {:path, param_atom} or param_atom"
+  end
+
+  defp get_site_with_role(conn, current_user, domain) when is_binary(domain) do
     site_query =
       from(
         s in Plausible.Site,
@@ -96,6 +107,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccess do
     end
   end
 
+  defp get_site_with_role(conn, _current_user, _domain) do
+    error_not_found(conn)
+  end
+
   defp maybe_get_shared_link(conn, site) do
     slug = conn.path_params["slug"] || conn.params["auth"]
 

lib/plausible_web/router.ex

@@ -42,7 +42,7 @@ defmodule PlausibleWeb.Router do
     plug :accepts, ["json"]
     plug :fetch_session
     plug PlausibleWeb.AuthPlug
-    plug PlausibleWeb.Plugs.AuthorizeSiteAccess
+    plug PlausibleWeb.Plugs.AuthorizeSiteAccess, site_param: {:path, :domain}
     plug PlausibleWeb.Plugs.NoRobots
   end
 
@@ -50,7 +50,11 @@ defmodule PlausibleWeb.Router do
     plug :accepts, ["json"]
     plug :fetch_session
     plug PlausibleWeb.AuthPlug
-    plug PlausibleWeb.Plugs.AuthorizeSiteAccess, [:admin, :super_admin, :owner]
+
+    plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
+      site_param: :site_id,
+      allowed_roles: [:admin, :super_admin, :owner]
+
     plug PlausibleWeb.Plugs.NoRobots
   end
 

test/plausible_web/plugs/authorize_site_access_test.exs

@@ -6,7 +6,19 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
 
   test "init rejects invalid role names" do
     assert_raise ArgumentError, fn ->
-      AuthorizeSiteAccess.init(_allowed_roles = [:admin, :invalid])
+      AuthorizeSiteAccess.init(site_param: :domain, allowed_roles: [:admin, :invalid])
+    end
+  end
+
+  test "init rejects invalid site_param format" do
+    assert_raise ArgumentError, fn ->
+      AuthorizeSiteAccess.init(site_param: "wrong", allowed_roles: [:admin, :invalid])
+    end
+  end
+
+  test "init crashes on missing site param" do
+    assert_raise KeyError, fn ->
+      AuthorizeSiteAccess.init(allowed_roles: [:admin])
     end
   end
 
@@ -15,7 +27,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/invalid.domain")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :domain},
+        allowed_roles: [:admin, :owner]
+      })
 
     assert conn.halted
     assert html_response(conn, 404)
@@ -28,7 +43,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :domain},
+        allowed_roles: [:admin, :owner]
+      })
 
     assert conn.halted
     assert html_response(conn, 404)
@@ -47,7 +65,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
         "domain" => site.domain,
         "website" => site.domain
       })
-      |> AuthorizeSiteAccess.call(_allowed_roles = [:admin, :owner])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :domain},
+        allowed_roles: [:admin, :owner]
+      })
 
     assert conn.halted
     assert html_response(conn, 404)
@@ -67,7 +88,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
         "domain" => site.domain,
         "website" => site.domain
       })
-      |> AuthorizeSiteAccess.call(_allowed_roles = [:admin, :owner])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :website},
+        allowed_roles: [:admin, :owner]
+      })
 
     assert conn.halted
     assert conn.status == 404
@@ -81,7 +105,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/api/stats/#{site.domain}/main-graph")
-      |> AuthorizeSiteAccess.call([:admin, :owner, :super_admin])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :domain},
+        allowed_roles: [:admin, :owner, :super_admin]
+      })
 
     assert conn.halted
 
@@ -102,7 +129,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> put("/sites/#{site.domain}/shared-links/#{shared_link_other_site.slug}", params)
-      |> AuthorizeSiteAccess.call(_allowed_roles = [:super_admin, :admin, :owner])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :website},
+        allowed_roles: [:super_admin, :admin, :owner]
+      })
 
     assert conn.halted
     assert conn.status == 404
@@ -121,7 +151,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{other_site.domain}", %{"auth" => shared_link.slug})
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :domain},
+        allowed_roles: AuthorizeSiteAccess.all_roles()
+      })
 
     assert conn.halted
     assert conn.status == 404
@@ -144,7 +177,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> put("/sites/#{site.domain}/shared-links/#{shared_link_other_site.slug}", params)
-      |> AuthorizeSiteAccess.call(_allowed_roles = [:super_admin, :admin, :owner])
+      |> AuthorizeSiteAccess.call(%{
+        site_param: {:path, :website},
+        allowed_roles: [:super_admin, :admin, :owner]
+      })
 
     assert conn.halted
     assert conn.status == 404
@@ -158,7 +194,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:owner])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:owner]})
 
     assert conn.halted
     assert html_response(conn, 404)
@@ -173,7 +209,10 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
         conn
         |> bypass_through(PlausibleWeb.Router)
         |> get("/#{site.domain}")
-        |> AuthorizeSiteAccess.call(_all_allowed_roles = [unquote(role)])
+        |> AuthorizeSiteAccess.call(%{
+          site_param: {:path, :domain},
+          allowed_roles: [unquote(role)]
+        })
 
       refute conn.halted
       assert conn.assigns.site.id == site.id
@@ -191,7 +230,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:super_admin])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:super_admin]})
 
     refute conn.halted
     assert conn.assigns.site.id == site.id
@@ -205,7 +244,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:public])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:public]})
 
     refute conn.halted
     assert conn.assigns.site.id == site.id
@@ -218,7 +257,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       build_conn()
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}")
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:public])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:public]})
 
     refute conn.halted
     assert conn.assigns.site.id == site.id
@@ -232,7 +271,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       conn
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}", %{"auth" => shared_link.slug})
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:public])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:public]})
 
     refute conn.halted
     assert conn.assigns.site.id == site.id
@@ -246,7 +285,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccessTest do
       build_conn()
       |> bypass_through(PlausibleWeb.Router)
       |> get("/#{site.domain}", %{"auth" => shared_link.slug})
-      |> AuthorizeSiteAccess.call(_all_allowed_roles = [:public])
+      |> AuthorizeSiteAccess.call(%{site_param: {:path, :domain}, allowed_roles: [:public]})
 
     refute conn.halted
     assert conn.assigns.site.id == site.id