Merge pull request #390 from danschultzer/dont-expire-cookies

Don't delete or expire cookies

Author: danschultzer

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)})