Merge pull request #366 from danschultzer/eager-delete-bug

Prevent eager deletion of invalid persistent session cookie

Author: danschultzer

CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Bug fixes
 
 * [`Pow.Ecto.Schema.Changeset`] Fixed bug where `Pow.Ecto.Schema.Changeset.user_id_field_changeset/3` update with `nil` value caused an exception to be raised
+* [`PowPersistentSession.Plug.Cookie`] Now expires the cookie 10 seconds after the last request when authenticating to prevent multiple simultaneous requests deletes the cookie immediately
 
 ## v1.0.15 (2019-11-20)
 

lib/extensions/persistent_session/plug/cookie.ex

@@ -3,8 +3,8 @@ defmodule PowPersistentSession.Plug.Cookie do
   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. The token in the cookie can only be used once to
-  create a session.
+  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.
 
   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
@@ -36,11 +36,15 @@ defmodule PowPersistentSession.Plug.Cookie do
     * `:persistent_session_ttl` - used for both backend store and max age for
       cookie. See `PowPersistentSession.Plug.Base` for more.
 
-    * `:persistent_session_cookie_opts` - a keyword list of cookie options, see
+    * `:persistent_session_cookie_opts` - keyword list of cookie options, see
       `Plug.Conn.put_resp_cookie/4` for options. The default options are
       `[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
@@ -71,6 +75,7 @@ defmodule PowPersistentSession.Plug.Cookie do
   alias Pow.{Config, Plug, UUID}
 
   @cookie_key "persistent_session_cookie"
+  @cookie_expiration_timeout 10
 
   @doc """
   Sets a persistent session cookie with an auto generated token.
@@ -136,41 +141,60 @@ defmodule PowPersistentSession.Plug.Cookie do
   @doc """
   Expires the persistent session cookie.
 
-  If a persistent session cookie exists it'll be expired, and the token in
-  the persistent session cache will be deleted.
+  If a persistent session cookie exists it'll be updated to expire immediately,
+  and the token in the persistent session cache will be deleted.
   """
   @spec delete(Conn.t(), Config.t()) :: Conn.t()
   def delete(conn, config) do
     cookie_key = cookie_key(config)
 
     case conn.req_cookies[cookie_key] do
-      nil    -> conn
-      key_id -> do_delete(conn, cookie_key, key_id, config)
+      nil ->
+        conn
+
+      key_id ->
+        expire_token_in_store(key_id, config)
+        delete_cookie(conn, cookie_key, config)
     end
   end
 
-  defp do_delete(conn, cookie_key, key_id, config) do
+  defp expire_token_in_store(key_id, config) do
     {store, store_config} = store(config)
-    value                 = ""
-    opts                  = [max_age: -1, path: "/"]
 
     store.delete(store_config, key_id)
-    Conn.put_resp_cookie(conn, cookie_key, value, opts)
+  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)
   end
 
   @doc """
   Authenticates a user with the persistent session cookie.
 
   If a persistent session cookie exists, it'll fetch the credentials from the
-  persistent session cache, and create a new session and persistent session
-  cookie. The old persistent session cookie and session cache credentials will
-  be removed.
+  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.
 
   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 cookie expiration will automatically be renewed on every request.
+  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
@@ -187,23 +211,39 @@ defmodule PowPersistentSession.Plug.Cookie do
 
     case conn.req_cookies[cookie_key] do
       nil    -> conn
-      key_id -> do_authenticate(conn, key_id, config)
+      key_id -> do_authenticate(conn, cookie_key, key_id, config)
     end
   end
   defp maybe_authenticate(conn, _user, _config), do: conn
 
-  defp do_authenticate(conn, key_id, config) do
+  defp do_authenticate(conn, cookie_key, key_id, config) do
     {store, store_config} = store(config)
     res                   = store.get(store_config, key_id)
     plug                  = Plug.get_plug(config)
-    conn                  = delete(conn, config)
+    conn                  = expire_cookie(conn, cookie_key, key_id, config)
 
     case res do
-      :not_found -> conn
-      res        -> fetch_and_auth_user(conn, res, plug, config)
+      :not_found ->
+        conn
+
+      res ->
+        expire_token_in_store(key_id, config)
+
+        fetch_and_auth_user(conn, res, plug, config)
     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!()
@@ -276,11 +316,13 @@ defmodule PowPersistentSession.Plug.Cookie do
   end
 
   defp maybe_renew(conn, config) do
-    cookie_key  = cookie_key(config)
+    cookie_key = cookie_key(config)
 
-    case conn.resp_cookies[cookie_key] do
-      nil  -> renew(conn, cookie_key, config)
-      _any -> conn
+    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
 

test/extensions/persistent_session/plug/cookie_test.exs

@@ -9,6 +9,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
   alias PowPersistentSession.Test.Users.User
 
   @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"]
 
   setup do
     config = PowPersistentSession.Test.pow_config()
@@ -128,7 +129,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: -1, path: "/", value: ""}
+    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: 10, path: "/", value: "test"}
     assert PersistentSessionCache.get([backend: ets], id) == :not_found
   end
 
@@ -139,7 +140,26 @@ defmodule PowPersistentSession.Plug.CookieTest do
       |> Cookie.call(Cookie.init([]))
 
     refute Plug.current_user(conn)
-    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: -1, path: "/", value: ""}
+    assert conn.resp_cookies["persistent_session_cookie"] == %{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")
+      |> Cookie.call(Cookie.init(config))
+
+    refute Plug.current_user(conn)
+    assert conn.resp_cookies["persistent_session_cookie"] == %{
+      domain: "domain.com",
+      extra: "SameSite=Lax",
+      http_only: false,
+      max_age: 20,
+      path: "/path",
+      secure: true,
+      value: "test"
+    }
   end
 
   test "call/2 with invalid stored clauses", %{conn: conn, ets: ets} do
@@ -197,7 +217,7 @@ defmodule PowPersistentSession.Plug.CookieTest do
   end
 
   test "create/3 with custom cookie options", %{conn: conn, config: config} do
-    config = Keyword.put(config, :persistent_session_cookie_opts, [domain: "domain.com", max_age: 1, path: "/path", http_only: false, secure: true, extra: "SameSite=Lax"])
+    config = Keyword.put(config, :persistent_session_cookie_opts, @custom_cookie_opts)
     conn   = Cookie.create(conn, %User{id: 1}, config)
 
     assert %{
@@ -236,4 +256,37 @@ defmodule PowPersistentSession.Plug.CookieTest do
 
     assert_received {:ets, :put, [{_key, {[id: 1], session_metadata: [a: 1]}}], _config}
   end
+
+  test "delete/3", %{conn: conn, ets: ets, config: config} do
+    id   = "test"
+    conn =
+      conn
+      |> store_persistent(ets, id, {[id: 1], []})
+      |> Cookie.delete(config)
+
+    refute Plug.current_user(conn)
+    assert conn.resp_cookies["persistent_session_cookie"] == %{max_age: -1, path: "/", value: ""}
+    assert PersistentSessionCache.get([backend: ets], id) == :not_found
+  end
+
+  test "delete/3 with custom cookie options", %{conn: conn, ets: ets, config: config} do
+    id   = "test"
+    config = Keyword.put(config, :persistent_session_cookie_opts, @custom_cookie_opts)
+    conn =
+      conn
+      |> store_persistent(ets, id, {[id: 1], []})
+      |> Cookie.delete(config)
+
+    refute Plug.current_user(conn)
+    assert conn.resp_cookies["persistent_session_cookie"] == %{
+      domain: "domain.com",
+      extra: "SameSite=Lax",
+      http_only: false,
+      max_age: -1,
+      path: "/path",
+      secure: true,
+      value: ""
+    }
+    assert PersistentSessionCache.get([backend: ets], id) == :not_found
+  end
 end