Don't delete or expire cookies

CHANGELOG.md

@@ -19,6 +19,7 @@
 * [`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
+* [`PowPersistentSession.Plug.Cookie`] No longer expires invalid cookies
 
 ## Removed
 

lib/extensions/persistent_session/plug/cookie.ex

@@ -40,10 +40,6 @@ defmodule PowPersistentSession.Plug.Cookie do
  `[max_age: max_age, path: "/"]` where `:max_age` is the value defined in
  `:persistent_session_ttl`.
 
- * `:persistent_session_cookie_expiration_timeout` - integer value in
- seconds for how much time should go by before cookie should expire after
- the token is fetched in `authenticate/2`. Defaults to 10.
-
  ## Custom metadata
 
  You can assign a private `:pow_persistent_session_metadata` key in the conn
@@ -74,10 +70,9 @@ defmodule PowPersistentSession.Plug.Cookie do
  alias Pow.{Config, Operations, Plug, UUID}
 
  @cookie_key "persistent_session"
- @cookie_expiration_timeout 10
 
  @doc """
- Sets a persistent session cookie with an auto generated token.
+ Sets a persistent session cookie with a randomly generated unique token.
 
  The token is set as a key in the persistent session cache with the id fetched
  from the struct. Any existing persistent session will be deleted first with
@@ -89,8 +84,8 @@ defmodule PowPersistentSession.Plug.Cookie do
  value will look like:
  `{[id: user_id], session_metadata: [fingerprint: fingerprint]}`
 
- The unique cookie id will be prepended by the `:otp_app` configuration
- value, if present.
+ The unique token will be prepended by the `:otp_app` configuration value, if
+ present.
  """
  @spec create(Conn.t(), map(), Config.t()) :: Conn.t()
  def create(conn, user, config) do
@@ -138,10 +133,11 @@ defmodule PowPersistentSession.Plug.Cookie do
  end
 
  @doc """
- Expires the persistent session cookie.
+ Expires the persistent session.
 
- If a persistent session cookie exists it'll be updated to expire immediately,
- and the token in the persistent session cache will be deleted.
+ If a persistent session cookie exists the token in the persistent session
+ cache will be deleted, and cookie deleted with
+ `Plug.Conn.delete_resp_cookie/3.
  """
  @spec delete(Conn.t(), Config.t()) :: Conn.t()
  def delete(conn, config) do
@@ -165,12 +161,7 @@ defmodule PowPersistentSession.Plug.Cookie do
  end
 
  defp delete_cookie(conn, cookie_key, config) do
- opts =
- config
- |> cookie_opts()
- |> Keyword.put(:max_age, -1)
-
- Conn.put_resp_cookie(conn, cookie_key, "", opts)
+ Conn.delete_resp_cookie(conn, cookie_key, cookie_opts(config))
  end
 
  @doc """
@@ -179,22 +170,13 @@ defmodule PowPersistentSession.Plug.Cookie do
  If a persistent session cookie exists, it'll fetch the credentials from the
  persistent session cache.
 
- After the value is fetched from the cookie, it'll be updated to expire after
- the value of `:persistent_session_cookie_expiration_timeout` so invalid
- cookies will be deleted eventually. This timeout prevents immediate deletion
- of the cookie so in case of multiple simultaneous requests, the cache has
- time to update the value.
-
  If credentials was fetched successfully, the token in the cache is deleted, a
  new session is created, and `create/2` is called to create a new persistent
- session cookie. This will override any expiring cookie.
+ session cookie.
 
  If a `:session_metadata` keyword list is fetched from the persistent session
  metadata, all the values will be merged into the private
  `:pow_session_metadata` key in the conn.
-
- The expiration date for the cookie will be reset on each request where a user
- is assigned to the conn.
  """
  @spec authenticate(Conn.t(), Config.t()) :: Conn.t()
  def authenticate(conn, config) do
@@ -210,16 +192,15 @@ defmodule PowPersistentSession.Plug.Cookie do
 
  case conn.req_cookies[cookie_key] do
  nil -> conn
- key_id -> do_authenticate(conn, cookie_key, key_id, config)
+ key_id -> do_authenticate(conn, key_id, config)
  end
  end
  defp maybe_authenticate(conn, _user, _config), do: conn
 
- defp do_authenticate(conn, cookie_key, key_id, config) do
+ defp do_authenticate(conn, key_id, config) do
  {store, store_config} = store(config)
  res = store.get(store_config, key_id)
  plug = Plug.get_plug(config)
- conn = expire_cookie(conn, cookie_key, key_id, config)
 
  case res do
  :not_found ->
@@ -232,17 +213,6 @@ defmodule PowPersistentSession.Plug.Cookie do
  end
  end
 
- defp expire_cookie(conn, cookie_key, key_id, config) do
- max_age = Config.get(config, :persistent_session_cookie_expiration_timeout, @cookie_expiration_timeout)
- opts =
- config
- |> cookie_opts()
- |> Keyword.put(:max_age, max_age)
-
- Conn.put_resp_cookie(conn, cookie_key, key_id, opts)
- end
-
-
  defp fetch_and_auth_user(conn, {clauses, metadata}, plug, config) do
  clauses
  |> filter_invalid!()

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

@@ -32,12 +32,14 @@ defmodule PowPersistentSession.Phoenix.ControllerCallbacksTest do
  end
 
  describe "Pow.Phoenix.SessionController.delete/2" do
- test "generates cookie", %{conn: conn, ets: ets} do
+ test "expires cookie", %{conn: conn, ets: ets} do
  conn = post conn, Routes.pow_session_path(conn, :create, %{"user" => @valid_params})
- %{value: id} = conn.resp_cookies[@cookie_key]
+
+ assert %{value: id} = conn.resp_cookies[@cookie_key]
+
  conn = delete conn, Routes.pow_session_path(conn, :delete)
 
- assert %{max_age: -1, path: "/", value: ""} = conn.resp_cookies[@cookie_key]
+ assert conn.resp_cookies[@cookie_key] == %{max_age: 0, path: "/", universal_time: {{1970, 1, 1}, {0, 0, 0}}}
  assert PersistentSessionCache.get([backend: ets], id) == :not_found
  end
  end

test/extensions/persistent_session/plug/cookie_test.exs

@@ -60,6 +60,29 @@ defmodule PowPersistentSession.Plug.CookieTest do
  assert PersistentSessionCache.get([backend: ets], new_id) == {[id: 1], []}
  end
 
+ test "call/2 assigns user from cookie and doesn't expire with simultanous request", %{conn: conn, ets: ets} do
+ user = %User{id: 1}
+ id = "test"
+ opts = Cookie.init([])
+ conn = store_persistent(conn, ets, id, {[id: user.id], []})
+
+ first_conn =
+ conn
+ |> Cookie.call(opts)
+ |> Conn.resp(200, "")
+
+ assert Plug.current_user(first_conn) == user
+ assert %{value: _id, max_age: @max_age, path: "/"} = first_conn.resp_cookies[@cookie_key]
+
+ second_conn =
+ conn
+ |> Cookie.call(opts)
+ |> Conn.resp(200, "")
+
+ refute Plug.current_user(second_conn) == user
+ refute second_conn.resp_cookies[@cookie_key]
+ end
+
  test "call/2 assigns user from cookie passing fingerprint to the session metadata", %{conn: conn, ets: ets} do
  user = %User{id: 1}
  id = "test"
@@ -129,7 +152,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
  |> Cookie.call(Cookie.init([]))
 
  refute Plug.current_user(conn)
- assert conn.resp_cookies[@cookie_key] == %{max_age: 10, path: "/", value: "test"}
+ refute conn.resp_cookies[@cookie_key]
  assert PersistentSessionCache.get([backend: ets], id) == :not_found
  end
 
@@ -140,26 +163,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
  |> Cookie.call(Cookie.init([]))
 
  refute Plug.current_user(conn)
- 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(@cookie_key, "test")
- |> Cookie.call(Cookie.init(config))
-
- refute Plug.current_user(conn)
- assert conn.resp_cookies[@cookie_key] == %{
- domain: "domain.com",
- extra: "SameSite=Lax",
- http_only: false,
- max_age: 20,
- path: "/path",
- secure: true,
- value: "test"
- }
+ refute conn.resp_cookies[@cookie_key]
  end
 
  test "call/2 with invalid stored clauses", %{conn: conn, ets: ets} do
@@ -265,7 +269,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
  |> Cookie.delete(config)
 
  refute Plug.current_user(conn)
- assert conn.resp_cookies[@cookie_key] == %{max_age: -1, path: "/", value: ""}
+ assert conn.resp_cookies[@cookie_key] == %{max_age: 0, path: "/", universal_time: {{1970, 1, 1}, {0, 0, 0}}}
  assert PersistentSessionCache.get([backend: ets], id) == :not_found
  end
 
@@ -278,15 +282,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
  |> Cookie.delete(config)
 
  refute Plug.current_user(conn)
- assert conn.resp_cookies[@cookie_key] == %{
- domain: "domain.com",
- extra: "SameSite=Lax",
- http_only: false,
- max_age: -1,
- path: "/path",
- secure: true,
- value: ""
- }
+ assert conn.resp_cookies[@cookie_key] == %{max_age: 0, universal_time: {{1970, 1, 1}, {0, 0, 0}}, path: "/path", domain: "domain.com", extra: "SameSite=Lax", http_only: false, secure: true}
  assert PersistentSessionCache.get([backend: ets], id) == :not_found
  end
 end

test/pow/plug/session_test.exs

@@ -120,6 +120,43 @@ defmodule Pow.Plug.SessionTest do
  assert conn.private[:pow_session_metadata][:fingerprint] == "fingerprint"
  end
 
+ test "call/2 creates new session when :session_renewal_ttl reached and doesn't delete with simultanous request", %{conn: new_conn} do
+ ttl = 100
+ id = "token"
+ config = Keyword.put(@default_opts, :session_ttl_renewal, ttl)
+ stale_timestamp = :os.system_time(:millisecond) - ttl - 1
+ opts = Session.init(config)
+
+ CredentialsCache.put(@store_config, id, {@user, inserted_at: stale_timestamp})
+
+ conn =
+ new_conn
+ |> Conn.fetch_session()
+ |> Conn.put_session(config[:session_key], id)
+ |> Conn.send_resp(200, "")
+
+ conn = Test.recycle_cookies(new_conn, conn)
+
+ first_conn =
+ conn
+ |> Session.call(opts)
+ |> Conn.send_resp(200, "")
+
+ assert Plug.current_user(first_conn) == @user
+ assert %{value: _id} = first_conn.resp_cookies["foobar"]
+ assert new_id = first_conn.private[:plug_session][config[:session_key]]
+ refute new_id == id
+ assert {@user, _metadata} = CredentialsCache.get(@store_config, new_id)
+
+ second_conn =
+ conn
+ |> Session.call(opts)
+ |> Conn.send_resp(200, "")
+
+ refute second_conn.resp_cookies["foobar"]
+ refute second_conn.private[:plug_session]
+ end
+
  test "call/2 with prepended `:otp_app` session key", %{conn: conn} do
  CredentialsCache.put(@store_config, "token", {@user, inserted_at: :os.system_time(:millisecond)})