Refactor and consolidate user session logic #4452
Conversation
zoldar
changed the title
zoldar
force-pushed
the
session-cookie-consolidation
branch
3 times, most recently
from
4302325 to
6207655
Compare
|
| socket = | ||
| socket | ||
| |> assign_new(:site, fn %{current_user: current_user} -> | ||
| Plausible.Sites.get_for_user!(current_user, domain, [:owner, :admin, :super_admin]) |
There was a problem hiding this comment.
Moved site assignment under assign_new here. I think it's fine to reuse existing assign if present but maybe I'm missing something?
| @@ -40,7 +34,7 @@ defmodule PlausibleWeb.Live.Shields.Countries do | |||
| current_user={@current_user} | |||
| country_rules_count={@country_rules_count} | |||
| site={@site} | |||
| id="country-rules-#{@current_user.id}" | |||
| id={"country-rules-#{@current_user.id}"} | |||
There was a problem hiding this comment.
It was a literal instead of interpolated string - fixed it btw (the same goes for other instances in shields LVs).
| @@ -112,10 +110,7 @@ defmodule PlausibleWeb.Live.Sites do | |||
| > | |||
| Total of <span class="font-medium"><%= @sites.total_entries %></span> sites | |||
| </.pagination> | |||
| <.invitation_modal | |||
| :if={Enum.any?(@sites.entries, &(&1.entry_type == "invitation"))} | |||
| user={@user} | |||
There was a problem hiding this comment.
This assign was completely unused in the modal component.
zoldar
force-pushed
the
session-cookie-consolidation
branch
4 times, most recently
from
9f90460 to
5c4445b
Compare
| @@ -622,8 +622,7 @@ defmodule PlausibleWeb.AuthController do | |||
| redirect_to = Map.get(params, "redirect", "/") | |||
|
|
|||
| conn | |||
| |> configure_session(drop: true) | |||
| |> delete_resp_cookie("logged_in") | |||
| |> UserAuth.log_out_user() | |||
There was a problem hiding this comment.
Just to make sure - renewing is just as good as dropping the cookie now?
There was a problem hiding this comment.
Renewing and clearing session is basically identical as dropping it in case of using COOKIE session store. It makes more of a difference in case of other stores (ETS, there's also Redis driver) where session data is stored server-side and session ID is stored in the cookie. Renewing then is indeed necessary to avoid malicious reuse of the same session ID (so-called session fixation attacks). Defaulting to renewal + clear is fine and additionally future-proof in case we ever want to switch the session store to avoid risk of potentially forgetting to update the reset strategy. For reference here's a short discussion clearing that up a bit: phoenixframework/phoenix#5466
| end | ||
| end | ||
|
|
||
| def on_mount(:default, _params, session, socket) do |
zoldar
force-pushed
the
session-cookie-consolidation
branch
from
5c4445b to
fbe1cdb
Compare
Changes
This PR refactors and creates a groundwork for implementing persisted user sessions.
Review can be done commit-by-commit for convenience.
PlausibleWeb.UserAuth. The module takes care of managing cookies (currently the defaults session when and "logged_in" cookie shared with static site). There are also extension points introduced already for supporting persisted sessions and transition from the current (legacy) cookies to the new, token based ones which will be persisted usingPlausible.Auth.UserSession(currently an embedded schema - just enough to support existing cookie auth).live_socket_id. Once the token based authentication is implemented, it will allow disconnecting LV clients on either explicit logout or after revoking the relevant token(s). More details in the docs. NOTE: As Phoenix PubSub is not fully setup, we might still not be able to fully benefit from that mechanism.current_user_idvalue is no longer explicitly passed vialive_render'ssessionoption. All existing session keys are passed through implicitly, to this was redundant.PlausibleWeb.Live.AuthContextis now automatically populatingcurrent_user(andcurrent_user_session) in LV context on mount - provided it's not provided by theAuthPlugalready which is sooner in the overall pipeline.Live.AuthContextandAuthPlugensure the shape of the loaded user is the same, always with an active subscription preloaded if there is one.current_user_idsession value got removed from LV in favor of retrievingcurrent_userimplicitly set by the auth plug or context.current_userfrom the context is used more consistently in a number of spots (mostly around billing where user was refetched and set under a different key, likeuser).Tests
Changelog
Documentation
Dark mode