Merge pull request #385 from danschultzer/refactor-session

Refactor session and persistent session store handling

Author: danschultzer

CHANGELOG.md

@@ -17,6 +17,8 @@
 * [`Pow.Phoenix.RegistrationView`] Now renders `:password_confirmation` field instead of `:confirm_password`
 * [`PowEmailConfirmation.Ecto.Schema`] No longer validates if `:email` has been taken before setting `:unconfirmed_email`
 * [`PowEmailConfirmation.Phoenix.ControllerCallbacks`] Now prevents user enumeration attack for `PowInvitation.Phoenix.InvitationController.create/2`
+* [`PowPersistentSession.Plug.Cookie`] Changed default cookie name to `persistent_session`
+* [`PowPersistentSession.Plug.Cookie`] Removed renewal of cookie as the token will always expire
 
 ## Removed
 

lib/extensions/persistent_session/README.md

@@ -1,6 +1,6 @@
 # PowPersistentSession
 
-This extension will permit reissuing sessions, by setting a cookie with a 30-day expiration (renewed on every request). The token in this cookie can be used exactly once to create the session. When the session has been created, the cookie will be updated with a new id.
+This extension can reissue sessions by setting a cookie with a token that may be used exactly once to issue the session. The cookie and token will expire after 30 days. Once the session has been issued, a new cookie and token will be set that expires after another 30 days.
 
 ## Installation
 

lib/extensions/persistent_session/plug/cookie.ex

@@ -2,9 +2,8 @@ defmodule PowPersistentSession.Plug.Cookie do
   @moduledoc """
   This plug will handle persistent user sessions with cookies.
 
-  By default, the cookie will expire after 30 days. The cookie expiration will
-  be renewed on every request where a user is assigned to the conn. The token
-  in the cookie can only be used once to create a session.
+  The cookie and token will expire after 30 days. The token in the cookie can
+  only be used once to create a session.
 
   If an assigned private `:pow_session_metadata` key exists in the conn with a
   keyword list containing a `:fingerprint` key, that fingerprint value will be
@@ -30,8 +29,8 @@ defmodule PowPersistentSession.Plug.Cookie do
     * `:cache_store_backend` - see `PowPersistentSession.Plug.Base`
 
     * `:persistent_session_cookie_key` - session key name. This defaults to
-      "persistent_session_cookie". If `:otp_app` is used it'll automatically
-      prepend the key with the `:otp_app` value.
+      "persistent_session". If `:otp_app` is used it'll automatically prepend
+      the key with the `:otp_app` value.
 
     * `:persistent_session_ttl` - used for both backend store and max age for
       cookie. See `PowPersistentSession.Plug.Base` for more.
@@ -74,7 +73,7 @@ defmodule PowPersistentSession.Plug.Cookie do
   alias Plug.Conn
   alias Pow.{Config, Operations, Plug, UUID}
 
-  @cookie_key "persistent_session_cookie"
+  @cookie_key "persistent_session"
   @cookie_expiration_timeout 10
 
   @doc """
@@ -146,6 +145,7 @@ defmodule PowPersistentSession.Plug.Cookie do
   """
   @spec delete(Conn.t(), Config.t()) :: Conn.t()
   def delete(conn, config) do
+    conn       = Conn.fetch_cookies(conn)
     cookie_key = cookie_key(config)
 
     case conn.req_cookies[cookie_key] do
@@ -203,7 +203,6 @@ defmodule PowPersistentSession.Plug.Cookie do
     conn
     |> Conn.fetch_cookies()
     |> maybe_authenticate(user, config)
-    |> maybe_renew(config)
   end
 
   defp maybe_authenticate(conn, nil, config) do
@@ -315,26 +314,6 @@ defmodule PowPersistentSession.Plug.Cookie do
     end
   end
 
-  defp maybe_renew(conn, config) do
-    cookie_key = cookie_key(config)
-
-    with user when not is_nil(user) <- Plug.current_user(conn, config),
-         nil <- conn.resp_cookies[cookie_key] do
-      renew(conn, cookie_key, config)
-    else
-      _ -> conn
-    end
-  end
-
-  defp renew(conn, cookie_key, config) do
-    opts = cookie_opts(config)
-
-    case conn.req_cookies[cookie_key] do
-      nil   -> conn
-      value -> Conn.put_resp_cookie(conn, cookie_key, value, opts)
-    end
-  end
-
   defp cookie_id(config) do
     uuid = UUID.generate()
 

lib/pow/plug/session.ex

@@ -130,8 +130,7 @@ defmodule Pow.Plug.Session do
   @spec fetch(Conn.t(), Config.t()) :: {Conn.t(), map() | nil}
   def fetch(conn, config) do
     {store, store_config} = store(config)
-    conn                  = Conn.fetch_session(conn)
-    key                   = Conn.get_session(conn, session_key(config))
+    key                   = client_store_fetch(conn, config)
 
     {key, store.get(store_config, key)}
     |> convert_old_session_value()
@@ -159,21 +158,18 @@ defmodule Pow.Plug.Session do
   @impl true
   @spec create(Conn.t(), map(), Config.t()) :: {Conn.t(), map()}
   def create(conn, user, config) do
-    conn                  = Conn.fetch_session(conn)
     {store, store_config} = store(config)
     metadata              = Map.get(conn.private, :pow_session_metadata, [])
     {user, metadata}      = session_value(user, metadata)
     key                   = session_id(config)
-    session_key           = session_key(config)
 
     store.put(store_config, key, {user, metadata})
 
     conn =
       conn
       |> delete(config)
       |> Conn.put_private(:pow_session_metadata, metadata)
-      |> Conn.put_session(session_key, key)
-      |> Conn.configure_session(renew: true)
+      |> client_store_put(key, config)
 
     {conn, user}
   end
@@ -199,14 +195,12 @@ defmodule Pow.Plug.Session do
   @impl true
   @spec delete(Conn.t(), Config.t()) :: Conn.t()
   def delete(conn, config) do
-    conn                  = Conn.fetch_session(conn)
-    key                   = Conn.get_session(conn, session_key(config))
+    key                   = client_store_fetch(conn, config)
     {store, store_config} = store(config)
-    session_key           = session_key(config)
 
     store.delete(store_config, key)
 
-    Conn.delete_session(conn, session_key)
+    client_store_delete(conn, config)
   end
 
   # TODO: Remove by 1.1.0
@@ -267,4 +261,23 @@ defmodule Pow.Plug.Session do
   end
 
   defp timestamp, do: :os.system_time(:millisecond)
+
+  defp client_store_fetch(conn, config) do
+    conn
+    |> Conn.fetch_session()
+    |> Conn.get_session(session_key(config))
+  end
+
+  defp client_store_put(conn, value, config) do
+    conn
+    |> Conn.fetch_session()
+    |> Conn.put_session(session_key(config), value)
+    |> Conn.configure_session(renew: true)
+  end
+
+  defp client_store_delete(conn, config) do
+    conn
+    |> Conn.fetch_session()
+    |> Conn.delete_session(session_key(config))
+  end
 end

test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs

@@ -5,7 +5,7 @@ defmodule PowPersistentSession.Phoenix.ControllerCallbacksTest do
 
   @valid_params %{"email" => "[email protected]", "password" => "secret1234"}
   @max_age Integer.floor_div(:timer.hours(30) * 24, 1000)
-  @cookie_key "persistent_session_cookie"
+  @cookie_key "persistent_session"
 
   describe "Pow.Phoenix.SessionController.create/2" do
     test "generates cookie", %{conn: conn, ets: ets} do

test/extensions/persistent_session/plug/cookie_test.exs

@@ -8,6 +8,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
   alias PowPersistentSession.{Plug.Cookie, Store.PersistentSessionCache}
   alias PowPersistentSession.Test.Users.User
 
+  @cookie_key "persistent_session"
   @max_age Integer.floor_div(:timer.hours(24) * 30, 1000)
   @custom_cookie_opts [domain: "domain.com", max_age: 1, path: "/path", http_only: false, secure: true, extra: "SameSite=Lax"]
 
@@ -26,7 +27,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
     {:ok, %{conn: conn, config: config, ets: ets}}
   end
 
-  defp store_persistent(conn, ets, id, value, cookie_key \\ "persistent_session_cookie") do
+  defp store_persistent(conn, ets, id, value, cookie_key \\ @cookie_key) do
     PersistentSessionCache.put([backend: ets], id, value)
     persistent_cookie(conn, cookie_key, id)
   end
@@ -41,7 +42,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
     expected_config = [mod: Session, plug: Session] ++ config
 
     assert {Cookie, ^expected_config} = conn.private[:pow_persistent_session]
-    refute conn.resp_cookies["persistent_session_cookie"]
+    refute conn.resp_cookies[@cookie_key]
   end
 
   test "call/2 assigns user from cookie", %{conn: conn, ets: ets} do
@@ -53,7 +54,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     assert Plug.current_user(conn) == user
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies[@cookie_key]
     refute new_id == id
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
     assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], []}
@@ -68,7 +69,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     assert Plug.current_user(conn) == user
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies[@cookie_key]
     refute new_id == id
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
     assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], session_metadata: [fingerprint: "fingerprint"]}
@@ -85,7 +86,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     assert Plug.current_user(conn) == user
-    assert %{value: id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{value: id, max_age: @max_age, path: "/"} = conn.resp_cookies[@cookie_key]
     assert PersistentSessionCache.get([backend: ets], id) == {[id: 1], session_metadata: [fingerprint: "new_fingerprint", b: 2, a: 2]}
     assert [inserted_at: _, b: 2, a: 3, fingerprint: "new_fingerprint"] = conn.private[:pow_session_metadata]
   end
@@ -97,11 +98,11 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> ConnHelpers.conn("/")
       |> ConnHelpers.init_session()
       |> Session.call(config ++ [otp_app: :test_app])
-      |> store_persistent(ets, "test_app_test", {[id: user.id], []}, "test_app_persistent_session_cookie")
+      |> store_persistent(ets, "test_app_test", {[id: user.id], []}, "test_app_" <> @cookie_key)
       |> Cookie.call(Cookie.init(config))
 
     assert Plug.current_user(conn) == user
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["test_app_persistent_session_cookie"]
+    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["test_app_" <> @cookie_key]
     assert String.starts_with?(new_id, "test_app")
     assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], []}
   end
@@ -115,8 +116,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Plug.assign_current_user(:user, [])
       |> Cookie.call(Cookie.init([]))
 
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
-    assert new_id == id
+    refute conn.resp_cookies[@cookie_key]
     assert PersistentSessionCache.get([backend: ets], id) == {[id: 1], []}
   end
 
@@ -129,29 +129,29 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: 10, path: "/", value: "test"}
+    assert conn.resp_cookies[@cookie_key] == %{max_age: 10, path: "/", value: "test"}
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
   end
 
   test "call/2 when persistent session cache doesn't have credentials", %{conn: conn} do
     conn =
       conn
-      |> persistent_cookie("persistent_session_cookie", "test")
+      |> persistent_cookie(@cookie_key, "test")
       |> Cookie.call(Cookie.init([]))
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: 10, path: "/", value: "test"}
+    assert conn.resp_cookies[@cookie_key] == %{max_age: 10, path: "/", value: "test"}
   end
 
   test "call/2 when persistent session cache doesn't have credentials with custom cookie options", %{conn: conn, config: config} do
     config = Keyword.merge(config, persistent_session_cookie_opts: @custom_cookie_opts, persistent_session_cookie_expiration_timeout: 20)
     conn   =
       conn
-      |> persistent_cookie("persistent_session_cookie", "test")
+      |> persistent_cookie(@cookie_key, "test")
       |> Cookie.call(Cookie.init(config))
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{
+    assert conn.resp_cookies[@cookie_key] == %{
       domain: "domain.com",
       extra: "SameSite=Lax",
       http_only: false,
@@ -183,7 +183,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     assert Plug.current_user(conn) == user
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies[@cookie_key]
     refute new_id == id
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
     assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], []}
@@ -199,7 +199,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     assert Plug.current_user(conn) == user
-    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{value: new_id, max_age: @max_age, path: "/"} = conn.resp_cookies[@cookie_key]
     refute new_id == id
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
     assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], session_metadata: [fingerprint: "fingerprint"]}
@@ -213,7 +213,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
     assert_received {:ets, :put, [{_key, _value} | _rest], config}
     assert config[:ttl] == 1000
 
-    assert %{max_age: 1, path: "/"} = conn.resp_cookies["persistent_session_cookie"]
+    assert %{max_age: 1, path: "/"} = conn.resp_cookies[@cookie_key]
   end
 
   test "create/3 with custom cookie options", %{conn: conn, config: config} do
@@ -227,7 +227,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       max_age: 1,
       path: "/path",
       secure: true
-    } = conn.resp_cookies["persistent_session_cookie"]
+    } = conn.resp_cookies[@cookie_key]
   end
 
   test "create/3 deletes previous persistent session", %{conn: conn, config: config, ets: ets} do
@@ -265,7 +265,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.delete(config)
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: -1, path: "/", value: ""}
+    assert conn.resp_cookies[@cookie_key] == %{max_age: -1, path: "/", value: ""}
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
   end
 
@@ -278,7 +278,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.delete(config)
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{
+    assert conn.resp_cookies[@cookie_key] == %{
       domain: "domain.com",
       extra: "SameSite=Lax",
       http_only: false,