Refactor AuthorizeSiteAccess (plausible#4583)

* Refactor and fix `AuthorizeSiteAccess` plug shared link case handling

* Make plug module name more consistent

* Add moduledoc

* Add regression test for shared link auth case

* Adjust failing case test to account for different response

* Further restrict accepted input and configuration

* Extend plug-specific tests

* Tag EE-only test properly

* Remove one DB roundtrip from AuthorizeSiteAccess

* Improve naming slightly

* Filter shared link by site ID on query level already

---------

Co-authored-by: Uku Taht <[email protected]>

lib/plausible_web/controllers/google_analytics_controller.ex

@@ -8,7 +8,7 @@ defmodule PlausibleWeb.GoogleAnalyticsController do
 
   plug(PlausibleWeb.RequireAccountPlug)
 
-  plug(PlausibleWeb.AuthorizeSiteAccess, [:owner, :admin, :super_admin])
+  plug(PlausibleWeb.Plugs.AuthorizeSiteAccess, [:owner, :admin, :super_admin])
 
   def property_form(
         conn,

lib/plausible_web/controllers/invitation_controller.ex

@@ -3,7 +3,8 @@ defmodule PlausibleWeb.InvitationController do
 
   plug PlausibleWeb.RequireAccountPlug
 
-  plug PlausibleWeb.AuthorizeSiteAccess, [:owner, :admin] when action in [:remove_invitation]
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
+       [: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,9 +18,9 @@ defmodule PlausibleWeb.Site.MembershipController do
   @only_owner_is_allowed_to [:transfer_ownership_form, :transfer_ownership]
 
   plug PlausibleWeb.RequireAccountPlug
-  plug PlausibleWeb.AuthorizeSiteAccess, [:owner] when action in @only_owner_is_allowed_to
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess, [:owner] when action in @only_owner_is_allowed_to
 
-  plug PlausibleWeb.AuthorizeSiteAccess,
+  plug PlausibleWeb.Plugs.AuthorizeSiteAccess,
        [:owner, :admin] when action not in @only_owner_is_allowed_to
 
   def invite_member_form(conn, _params) do

lib/plausible_web/controllers/site_controller.ex

@@ -9,7 +9,7 @@ defmodule PlausibleWeb.SiteController do
   plug(PlausibleWeb.RequireAccountPlug)
 
   plug(
-    PlausibleWeb.AuthorizeSiteAccess,
+    PlausibleWeb.Plugs.AuthorizeSiteAccess,
     [:owner, :admin, :super_admin] when action not in [:new, :create_site]
   )
 

lib/plausible_web/controllers/stats_controller.ex

@@ -48,7 +48,7 @@ defmodule PlausibleWeb.StatsController do
   alias Plausible.Stats.{Filters, Query}
   alias PlausibleWeb.Api
 
-  plug(PlausibleWeb.AuthorizeSiteAccess when action in [:stats, :csv_export])
+  plug(PlausibleWeb.Plugs.AuthorizeSiteAccess 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

@@ -1,55 +1,55 @@
-defmodule PlausibleWeb.AuthorizeSiteAccess do
+defmodule PlausibleWeb.Plugs.AuthorizeSiteAccess do
+  @moduledoc """
+  Plug restricting access to site and shared link, when present.
+  """
+
+  use Plausible.Repo
+
   import Plug.Conn
   import Phoenix.Controller, only: [get_format: 1]
-  use Plausible.Repo
 
-  def init([]), do: [:public, :viewer, :admin, :super_admin, :owner]
-  def init(allowed_roles), do: allowed_roles
+  @all_roles [:public, :viewer, :admin, :super_admin, :owner]
 
-  def call(conn, allowed_roles) do
-    site =
-      Repo.get_by(Plausible.Site,
-        domain:
-          conn.path_params["domain"] || conn.path_params["website"] || conn.params["site_id"]
-      )
+  def init([]), do: @all_roles
 
-    shared_link_auth = conn.params["auth"]
+  def init(allowed_roles) do
+    unknown_roles = allowed_roles -- @all_roles
 
-    shared_link_record =
-      shared_link_auth && Repo.get_by(Plausible.Site.SharedLink, slug: shared_link_auth)
+    if unknown_roles != [] do
+      raise ArgumentError, "Unknown allowed roles configured: #{inspect(unknown_roles)}"
+    end
 
-    if !site do
-      fail(conn)
-    else
-      user_id =
-        case PlausibleWeb.UserAuth.get_user_session(conn) do
-          {:ok, user_session} -> user_session.user_id
-          _ -> nil
-        end
+    allowed_roles
+  end
 
-      membership_role = user_id && Plausible.Sites.role(user_id, site)
+  def call(conn, allowed_roles) do
+    current_user = conn.assigns[:current_user]
 
+    with {:ok, %{site: site, role: membership_role}} <- get_site_with_role(conn, current_user),
+         {:ok, shared_link} <- maybe_get_shared_link(conn, site) do
       role =
         cond do
-          user_id && membership_role ->
+          membership_role ->
             membership_role
 
-          Plausible.Auth.is_super_admin?(user_id) ->
+          Plausible.Auth.is_super_admin?(current_user) ->
             :super_admin
 
           site.public ->
             :public
 
-          shared_link_record && shared_link_record.site_id == site.id ->
+          shared_link ->
             :public
 
           true ->
             nil
         end
 
       if role in allowed_roles do
-        Sentry.Context.set_user_context(%{id: user_id})
-        Plausible.OpenTelemetry.add_user_attributes(user_id)
+        if current_user do
+          Sentry.Context.set_user_context(%{id: current_user.id})
+          Plausible.OpenTelemetry.add_user_attributes(current_user.id)
+        end
 
         Sentry.Context.set_extra_context(%{site_id: site.id, domain: site.domain})
         Plausible.OpenTelemetry.add_site_attributes(site)
@@ -58,21 +58,67 @@ defmodule PlausibleWeb.AuthorizeSiteAccess do
 
         merge_assigns(conn, site: site, current_user_role: role)
       else
-        fail(conn)
+        error_not_found(conn)
+      end
+    end
+  end
+
+  defp get_site_with_role(conn, current_user) do
+    domain = conn.path_params["domain"] || conn.path_params["website"]
+
+    site_query =
+      from(
+        s in Plausible.Site,
+        where: s.domain == ^domain,
+        select: %{site: s}
+      )
+
+    full_query =
+      if current_user do
+        from(s in site_query,
+          left_join: sm in Plausible.Site.Membership,
+          on: sm.site_id == s.id and sm.user_id == ^current_user.id,
+          select_merge: %{role: sm.role}
+        )
+      else
+        from(s in site_query,
+          select_merge: %{role: nil}
+        )
       end
+
+    case Repo.one(full_query) do
+      %{site: _site} = result -> {:ok, result}
+      _ -> error_not_found(conn)
+    end
+  end
+
+  defp maybe_get_shared_link(conn, site) do
+    slug = conn.path_params["slug"] || conn.params["auth"]
+
+    if is_binary(slug) do
+      if shared_link = Repo.get_by(Plausible.Site.SharedLink, slug: slug, site_id: site.id) do
+        {:ok, shared_link}
+      else
+        error_not_found(conn)
+      end
+    else
+      {:ok, nil}
     end
   end
 
-  defp fail(conn) do
+  defp error_not_found(conn) do
     case get_format(conn) do
       "json" ->
-        PlausibleWeb.Api.Helpers.not_found(
-          conn,
+        conn
+        |> PlausibleWeb.Api.Helpers.not_found(
           "Site does not exist or user does not have sufficient access."
         )
+        |> halt()
 
       _ ->
-        PlausibleWeb.ControllerHelpers.render_error(conn, 404) |> halt
+        conn
+        |> PlausibleWeb.ControllerHelpers.render_error(404)
+        |> halt()
     end
   end
 end

lib/plausible_web/router.ex

@@ -42,7 +42,7 @@ defmodule PlausibleWeb.Router do
     plug :accepts, ["json"]
     plug :fetch_session
     plug PlausibleWeb.AuthPlug
-    plug PlausibleWeb.AuthorizeSiteAccess
+    plug PlausibleWeb.Plugs.AuthorizeSiteAccess
     plug PlausibleWeb.Plugs.NoRobots
   end
 
@@ -51,7 +51,7 @@ defmodule PlausibleWeb.Router do
     plug :fetch_session
     plug PlausibleWeb.AuthPlug
 
-    plug PlausibleWeb.AuthorizeSiteAccess, [:admin, :super_admin, :owner]
+    plug PlausibleWeb.Plugs.AuthorizeSiteAccess, [:admin, :super_admin, :owner]
 
     plug PlausibleWeb.Plugs.NoRobots
   end

test/plausible_web/controllers/site_controller_test.exs

@@ -1480,8 +1480,7 @@ defmodule PlausibleWeb.SiteControllerTest do
       conn = delete(conn, "/sites/#{site.domain}/shared-links/#{link.slug}")
 
       assert Repo.one(Plausible.Site.SharedLink)
-      assert redirected_to(conn, 302) =~ "/#{URI.encode_www_form(site.domain)}/settings"
-      assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Could not find Shared Link"
+      assert html_response(conn, 404)
     end
   end