From 11af7f79b571ce92390e44cc39e69aa83544a794 Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Sun, 29 Dec 2019 20:14:32 -0800 Subject: [PATCH 1/3] Prevent eager deletion of invalid persistent session cookie --- CHANGELOG.md | 1 + .../persistent_session/plug/cookie.ex | 58 +++++++++++++----- .../persistent_session/plug/cookie_test.exs | 59 ++++++++++++++++++- 3 files changed, 100 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c85aa22..dae75bda 100644 --- a/CHANGELOG.md +++ b/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 after 10 seconds after the last request when authenticating to prevent multiple simultaneous requests deletes the cookie immediately ## v1.0.15 (2019-11-20) diff --git a/lib/extensions/persistent_session/plug/cookie.ex b/lib/extensions/persistent_session/plug/cookie.ex index c20136f8..bc183d5c 100644 --- a/lib/extensions/persistent_session/plug/cookie.ex +++ b/lib/extensions/persistent_session/plug/cookie.ex @@ -71,6 +71,7 @@ defmodule PowPersistentSession.Plug.Cookie do alias Pow.{Config, Plug, UUID} @cookie_key "persistent_session_cookie" + @cookie_delete_time_drift 10 @doc """ Sets a persistent session cookie with an auto generated token. @@ -144,18 +145,28 @@ defmodule PowPersistentSession.Plug.Cookie 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 """ @@ -187,23 +198,38 @@ 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 + opts = + config + |> cookie_opts() + |> Keyword.put(:max_age, @cookie_delete_time_drift) + + 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 +302,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 diff --git a/test/extensions/persistent_session/plug/cookie_test.exs b/test/extensions/persistent_session/plug/cookie_test.exs index ddeeacd0..f6da127e 100644 --- a/test/extensions/persistent_session/plug/cookie_test.exs +++ b/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.put(config, :persistent_session_cookie_opts, @custom_cookie_opts) + 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: 10, + 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 From c90e6d14701ccf9fb7b8b2f9f78d609c3cc40eae Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Mon, 30 Dec 2019 06:02:00 -0800 Subject: [PATCH 2/3] Add config option and update docs --- CHANGELOG.md | 2 +- .../persistent_session/plug/cookie.ex | 20 ++++++++++++------- .../persistent_session/plug/cookie_test.exs | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dae75bda..79550f8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +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 after 10 seconds after the last request when authenticating to prevent multiple simultaneous requests deletes the cookie immediately +* [`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) diff --git a/lib/extensions/persistent_session/plug/cookie.ex b/lib/extensions/persistent_session/plug/cookie.ex index bc183d5c..e97f72b4 100644 --- a/lib/extensions/persistent_session/plug/cookie.ex +++ b/lib/extensions/persistent_session/plug/cookie.ex @@ -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_drift` - integer value in seconds + for how much time till the cookie should expire after the token has been + fetched in `authenticate/2`. Defaults to 10. + ## Custom metadata You can assign a private `:pow_persistent_session_metadata` key in the conn @@ -71,7 +75,7 @@ defmodule PowPersistentSession.Plug.Cookie do alias Pow.{Config, Plug, UUID} @cookie_key "persistent_session_cookie" - @cookie_delete_time_drift 10 + @cookie_expiration_drift 10 @doc """ Sets a persistent session cookie with an auto generated token. @@ -174,14 +178,15 @@ defmodule PowPersistentSession.Plug.Cookie do 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. + cookie. The max age of the old cookie will always be updated to the value of + `:persistent_session_cookie_expiration_drift` to prevent eager expiration in + case of multiple simultaneous requests. 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. + If there is a user assigned in the conn, the cookie expiration will be renewed. """ @spec authenticate(Conn.t(), Config.t()) :: Conn.t() def authenticate(conn, config) do @@ -221,10 +226,11 @@ defmodule PowPersistentSession.Plug.Cookie do end defp expire_cookie(conn, cookie_key, key_id, config) do - opts = + max_age = Config.get(config, :persistent_session_cookie_expiration_drift, @cookie_expiration_drift) + opts = config |> cookie_opts() - |> Keyword.put(:max_age, @cookie_delete_time_drift) + |> Keyword.put(:max_age, max_age) Conn.put_resp_cookie(conn, cookie_key, key_id, opts) end diff --git a/test/extensions/persistent_session/plug/cookie_test.exs b/test/extensions/persistent_session/plug/cookie_test.exs index f6da127e..7093ee50 100644 --- a/test/extensions/persistent_session/plug/cookie_test.exs +++ b/test/extensions/persistent_session/plug/cookie_test.exs @@ -144,7 +144,7 @@ defmodule PowPersistentSession.Plug.CookieTest do end test "call/2 when persistent session cache doesn't have credentials with custom cookie options", %{conn: conn, config: config} do - config = Keyword.put(config, :persistent_session_cookie_opts, @custom_cookie_opts) + config = Keyword.merge(config, persistent_session_cookie_opts: @custom_cookie_opts, persistent_session_cookie_expiration_drift: 20) conn = conn |> persistent_cookie("persistent_session_cookie", "test") @@ -155,7 +155,7 @@ defmodule PowPersistentSession.Plug.CookieTest do domain: "domain.com", extra: "SameSite=Lax", http_only: false, - max_age: 10, + max_age: 20, path: "/path", secure: true, value: "test" From 2f584bed31fc47d05408a621703e39a667a1ada9 Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Mon, 30 Dec 2019 10:45:59 -0800 Subject: [PATCH 3/3] Clean docs and change var name --- .../persistent_session/plug/cookie.ex | 36 +++++++++++-------- .../persistent_session/plug/cookie_test.exs | 2 +- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/lib/extensions/persistent_session/plug/cookie.ex b/lib/extensions/persistent_session/plug/cookie.ex index e97f72b4..6a81ae5c 100644 --- a/lib/extensions/persistent_session/plug/cookie.ex +++ b/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 @@ -41,9 +41,9 @@ 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_drift` - integer value in seconds - for how much time till the cookie should expire after the token has been - fetched in `authenticate/2`. Defaults to 10. + * `: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 @@ -75,7 +75,7 @@ defmodule PowPersistentSession.Plug.Cookie do alias Pow.{Config, Plug, UUID} @cookie_key "persistent_session_cookie" - @cookie_expiration_drift 10 + @cookie_expiration_timeout 10 @doc """ Sets a persistent session cookie with an auto generated token. @@ -141,8 +141,8 @@ 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 @@ -177,16 +177,24 @@ defmodule PowPersistentSession.Plug.Cookie do 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 max age of the old cookie will always be updated to the value of - `:persistent_session_cookie_expiration_drift` to prevent eager expiration in - case of multiple simultaneous requests. + 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. - If there is a user assigned in the conn, the cookie expiration will be renewed. + 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 @@ -226,7 +234,7 @@ defmodule PowPersistentSession.Plug.Cookie do end defp expire_cookie(conn, cookie_key, key_id, config) do - max_age = Config.get(config, :persistent_session_cookie_expiration_drift, @cookie_expiration_drift) + max_age = Config.get(config, :persistent_session_cookie_expiration_timeout, @cookie_expiration_timeout) opts = config |> cookie_opts() diff --git a/test/extensions/persistent_session/plug/cookie_test.exs b/test/extensions/persistent_session/plug/cookie_test.exs index 7093ee50..17df571f 100644 --- a/test/extensions/persistent_session/plug/cookie_test.exs +++ b/test/extensions/persistent_session/plug/cookie_test.exs @@ -144,7 +144,7 @@ defmodule PowPersistentSession.Plug.CookieTest do 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_drift: 20) + 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")