From 2e856a7eee1e73b10b741dac1a58c2da281516a7 Mon Sep 17 00:00:00 2001 From: Adrian Gruntkowski Date: Mon, 2 Sep 2024 12:34:08 +0200 Subject: [PATCH] Only touch session once an hour and keep `user.last_seen` in sync --- lib/plausible/users.ex | 13 +++++ lib/plausible_web/plugs/last_seen_plug.ex | 37 ------------ lib/plausible_web/router.ex | 1 - lib/plausible_web/user_auth.ex | 12 +++- .../controllers/site_controller_test.exs | 57 +++++++++++++------ test/plausible_web/user_auth_test.exs | 37 ++++++++++++ 6 files changed, 99 insertions(+), 58 deletions(-) delete mode 100644 lib/plausible_web/plugs/last_seen_plug.ex diff --git a/lib/plausible/users.ex b/lib/plausible/users.ex index 8b8c521385c0a..39a32d78d3a1b 100644 --- a/lib/plausible/users.ex +++ b/lib/plausible/users.ex @@ -35,6 +35,19 @@ defmodule Plausible.Users do |> Repo.update!() end + @spec bump_last_seen(Auth.User.t() | pos_integer(), NaiveDateTime.t()) :: :ok + def bump_last_seen(%Auth.User{id: user_id}, now) do + bump_last_seen(user_id, now) + end + + def bump_last_seen(user_id, now) do + q = from(u in Auth.User, where: u.id == ^user_id) + + Repo.update_all(q, set: [last_seen: now]) + + :ok + end + @spec accept_traffic_until(Auth.User.t()) :: Date.t() on_ee do def accept_traffic_until(user) do diff --git a/lib/plausible_web/plugs/last_seen_plug.ex b/lib/plausible_web/plugs/last_seen_plug.ex deleted file mode 100644 index 5a935d62f0c71..0000000000000 --- a/lib/plausible_web/plugs/last_seen_plug.ex +++ /dev/null @@ -1,37 +0,0 @@ -defmodule PlausibleWeb.LastSeenPlug do - import Plug.Conn - use Plausible.Repo - - @one_hour 60 * 60 - - def init(opts) do - opts - end - - def call(conn, _opts) do - last_seen = get_session(conn, :last_seen) - user = conn.assigns[:current_user] - - cond do - user && last_seen && last_seen < unix_now() - @one_hour -> - persist_last_seen(user) - put_session(conn, :last_seen, unix_now()) - - user && !last_seen -> - put_session(conn, :last_seen, unix_now()) - - true -> - conn - end - end - - defp persist_last_seen(user) do - q = from(u in Plausible.Auth.User, where: u.id == ^user.id) - - Repo.update_all(q, set: [last_seen: DateTime.utc_now()]) - end - - defp unix_now do - DateTime.utc_now() |> DateTime.to_unix() - end -end diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index 258d01f58b0ce..8cf7bc92d2bcf 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -14,7 +14,6 @@ defmodule PlausibleWeb.Router do plug PlausibleWeb.SessionTimeoutPlug, timeout_after_seconds: @two_weeks_in_seconds plug PlausibleWeb.AuthPlug plug PlausibleWeb.Plugs.UserSessionTouch - plug PlausibleWeb.LastSeenPlug end pipeline :shared_link do diff --git a/lib/plausible_web/user_auth.ex b/lib/plausible_web/user_auth.ex index 77fed8f523ff0..45012e650bcb0 100644 --- a/lib/plausible_web/user_auth.ex +++ b/lib/plausible_web/user_auth.ex @@ -68,9 +68,15 @@ defmodule PlausibleWeb.UserAuth do end def touch_user_session(user_session, now \\ NaiveDateTime.utc_now(:second)) do - user_session - |> Auth.UserSession.touch_session(now) - |> Repo.update!(allow_stale: true) + if NaiveDateTime.diff(now, user_session.last_used_at, :hour) >= 1 do + Plausible.Users.bump_last_seen(user_session.user_id, now) + + user_session + |> Auth.UserSession.touch_session(now) + |> Repo.update!(allow_stale: true) + else + user_session + end end @doc """ diff --git a/test/plausible_web/controllers/site_controller_test.exs b/test/plausible_web/controllers/site_controller_test.exs index f2805d378d862..a5908f79b51a2 100644 --- a/test/plausible_web/controllers/site_controller_test.exs +++ b/test/plausible_web/controllers/site_controller_test.exs @@ -954,7 +954,7 @@ defmodule PlausibleWeb.SiteControllerTest do describe "PUT /:website/settings/features/visibility/:setting" do def query_conn_with_some_url(context) do - {:ok, Map.put(context, :conn, get(context.conn, "/some_parent_path"))} + {:ok, Map.put(context, :conn_with_url, get(context.conn, "/some_parent_path"))} end setup [:create_user, :log_in, :query_conn_with_some_url] @@ -966,7 +966,8 @@ defmodule PlausibleWeb.SiteControllerTest do } do test "can toggle #{title} with admin access", %{ user: user, - conn: conn0 + conn: conn0, + conn_with_url: conn_with_url } do site = insert(:site, @@ -979,7 +980,12 @@ defmodule PlausibleWeb.SiteControllerTest do conn = put( conn0, - PlausibleWeb.Components.Site.Feature.target(site, unquote(setting), conn0, false) + PlausibleWeb.Components.Site.Feature.target( + site, + unquote(setting), + conn_with_url, + false + ) ) assert Phoenix.Flash.get(conn.assigns.flash, :success) == @@ -992,7 +998,12 @@ defmodule PlausibleWeb.SiteControllerTest do conn = put( conn0, - PlausibleWeb.Components.Site.Feature.target(site, unquote(setting), conn0, true) + PlausibleWeb.Components.Site.Feature.target( + site, + unquote(setting), + conn_with_url, + true + ) ) assert Phoenix.Flash.get(conn.assigns.flash, :success) == @@ -1011,7 +1022,8 @@ defmodule PlausibleWeb.SiteControllerTest do } do test "cannot toggle #{title} with viewer access", %{ user: user, - conn: conn0 + conn: conn0, + conn_with_url: conn_with_url } do site = insert(:site) insert(:site_membership, user: user, site: site, role: :viewer) @@ -1019,7 +1031,12 @@ defmodule PlausibleWeb.SiteControllerTest do conn = put( conn0, - PlausibleWeb.Components.Site.Feature.target(site, unquote(setting), conn0, false) + PlausibleWeb.Components.Site.Feature.target( + site, + unquote(setting), + conn_with_url, + false + ) ) assert conn.status == 404 @@ -1027,7 +1044,11 @@ defmodule PlausibleWeb.SiteControllerTest do end end - test "setting feature visibility is idempotent", %{user: user, conn: conn0} do + test "setting feature visibility is idempotent", %{ + user: user, + conn: conn0, + conn_with_url: conn_with_url + } do site = insert(:site) insert(:site_membership, user: user, site: site, role: :admin) @@ -1036,7 +1057,7 @@ defmodule PlausibleWeb.SiteControllerTest do conn = put( conn0, - PlausibleWeb.Components.Site.Feature.target(site, setting, conn0, false) + PlausibleWeb.Components.Site.Feature.target(site, setting, conn_with_url, false) ) assert %{^setting => false} = Plausible.Sites.get_by_domain(site.domain) @@ -1045,7 +1066,7 @@ defmodule PlausibleWeb.SiteControllerTest do conn = put( conn0, - PlausibleWeb.Components.Site.Feature.target(site, setting, conn0, false) + PlausibleWeb.Components.Site.Feature.target(site, setting, conn_with_url, false) ) assert %{^setting => false} = Plausible.Sites.get_by_domain(site.domain) @@ -1132,11 +1153,11 @@ defmodule PlausibleWeb.SiteControllerTest do site = insert(:site) insert(:weekly_report, site: site, recipients: ["recipient@email.com"]) - conn = delete(conn, "/sites/#{site.domain}/weekly-report/recipients/recipient@email.com") - assert conn.status == 404 + conn1 = delete(conn, "/sites/#{site.domain}/weekly-report/recipients/recipient@email.com") + assert conn1.status == 404 - conn = delete(conn, "/sites/#{site.domain}/weekly-report/recipients/recipient%40email.com") - assert conn.status == 404 + conn2 = delete(conn, "/sites/#{site.domain}/weekly-report/recipients/recipient%40email.com") + assert conn2.status == 404 report = Repo.get_by(Plausible.Site.WeeklyReport, site_id: site.id) assert [_] = report.recipients @@ -1215,11 +1236,13 @@ defmodule PlausibleWeb.SiteControllerTest do site = insert(:site) insert(:monthly_report, site: site, recipients: ["recipient@email.com"]) - conn = delete(conn, "/sites/#{site.domain}/monthly-report/recipients/recipient@email.com") - assert conn.status == 404 + conn1 = delete(conn, "/sites/#{site.domain}/monthly-report/recipients/recipient@email.com") + assert conn1.status == 404 - conn = delete(conn, "/sites/#{site.domain}/monthly-report/recipients/recipient%40email.com") - assert conn.status == 404 + conn2 = + delete(conn, "/sites/#{site.domain}/monthly-report/recipients/recipient%40email.com") + + assert conn2.status == 404 report = Repo.get_by(Plausible.Site.MonthlyReport, site_id: site.id) assert [_] = report.recipients diff --git a/test/plausible_web/user_auth_test.exs b/test/plausible_web/user_auth_test.exs index 05d7cb2c38413..71311519e15d4 100644 --- a/test/plausible_web/user_auth_test.exs +++ b/test/plausible_web/user_auth_test.exs @@ -159,9 +159,46 @@ defmodule PlausibleWeb.UserAuthTest do assert refreshed_session.id == user_session.id assert NaiveDateTime.compare(refreshed_session.last_used_at, two_days_later) == :eq + assert NaiveDateTime.compare(Repo.reload(user).last_seen, two_days_later) == :eq assert NaiveDateTime.compare(refreshed_session.timeout_at, user_session.timeout_at) == :gt end + test "does not refresh if timestamps were updated less than hour before", %{user: user} do + %{sessions: [user_session]} = Repo.preload(user, :sessions) + user_session = Repo.reload(user_session) + last_seen = Repo.reload(user).last_seen + + fifty_minutes_later = + NaiveDateTime.utc_now(:second) + |> NaiveDateTime.shift(minute: 50) + + assert refreshed_session1 = + %Auth.UserSession{} = + UserAuth.touch_user_session(user_session, fifty_minutes_later) + + assert NaiveDateTime.compare( + refreshed_session1.last_used_at, + user_session.last_used_at + ) == :eq + + assert NaiveDateTime.compare(Repo.reload(user).last_seen, last_seen) == :eq + + sixty_five_minutes_later = + NaiveDateTime.utc_now(:second) + |> NaiveDateTime.shift(minute: 65) + + assert refreshed_session2 = + %Auth.UserSession{} = + UserAuth.touch_user_session(user_session, sixty_five_minutes_later) + + assert NaiveDateTime.compare( + refreshed_session2.last_used_at, + sixty_five_minutes_later + ) == :eq + + assert NaiveDateTime.compare(Repo.reload(user).last_seen, sixty_five_minutes_later) == :eq + end + test "handles concurrent refresh gracefully", %{user: user} do %{sessions: [user_session]} = Repo.preload(user, :sessions)