From 8862763044d1058a542ba784e454f89667b7822a Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Sun, 12 Jan 2020 23:08:42 -0800 Subject: [PATCH 1/2] Don't delete or expire cookies --- CHANGELOG.md | 1 + .../persistent_session/plug/cookie.ex | 52 ++++--------------- .../controllers/controller_callbacks_test.exs | 6 ++- .../persistent_session/plug/cookie_test.exs | 35 ++----------- 4 files changed, 20 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68b65f23..3a71601c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/extensions/persistent_session/plug/cookie.ex b/lib/extensions/persistent_session/plug/cookie.ex index 6b302550..d983d829 100644 --- a/lib/extensions/persistent_session/plug/cookie.ex +++ b/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!() diff --git a/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs b/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs index 63aad2a8..ca07ac94 100644 --- a/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs +++ b/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs @@ -34,10 +34,12 @@ defmodule PowPersistentSession.Phoenix.ControllerCallbacksTest do describe "Pow.Phoenix.SessionController.delete/2" do test "generates 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 diff --git a/test/extensions/persistent_session/plug/cookie_test.exs b/test/extensions/persistent_session/plug/cookie_test.exs index fbee7f5d..12024f77 100644 --- a/test/extensions/persistent_session/plug/cookie_test.exs +++ b/test/extensions/persistent_session/plug/cookie_test.exs @@ -129,7 +129,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 +140,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 +246,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 +259,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 From 095a0f8e4fd6505eeab58e2da436f111f45df859 Mon Sep 17 00:00:00 2001 From: Dan Schultzer Date: Tue, 21 Jan 2020 13:33:35 -0800 Subject: [PATCH 2/2] Add tests for simultaneous requests --- .../controllers/controller_callbacks_test.exs | 2 +- .../persistent_session/plug/cookie_test.exs | 23 ++++++++++++ test/pow/plug/session_test.exs | 37 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs b/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs index ca07ac94..fc1be452 100644 --- a/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs +++ b/test/extensions/persistent_session/phoenix/controllers/controller_callbacks_test.exs @@ -32,7 +32,7 @@ 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}) assert %{value: id} = conn.resp_cookies[@cookie_key] diff --git a/test/extensions/persistent_session/plug/cookie_test.exs b/test/extensions/persistent_session/plug/cookie_test.exs index 12024f77..a045a0c3 100644 --- a/test/extensions/persistent_session/plug/cookie_test.exs +++ b/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" diff --git a/test/pow/plug/session_test.exs b/test/pow/plug/session_test.exs index 2ddd8d43..f5a3e678 100644 --- a/test/pow/plug/session_test.exs +++ b/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)})