Merge pull request #37 from epochtalk/delete-session

Delete session

Author: akinsey

lib/epochtalk_server/session.ex

@@ -106,99 +106,66 @@ defmodule EpochtalkServer.Session do
   end
 
   @doc """
-  Gets all sessions for a specific `User`
+  Gets all session ids for a specific user_id
   """
-  @spec get_sessions(user :: User.t()) ::
-          {:ok, user :: User.t(), sessions :: [String.t()]}
+  @spec get_sessions(user_id :: non_neg_integer) ::
+          {:ok, sessions_ids :: [String.t()]}
           | {:error, atom() | Redix.Error.t() | Redix.ConnectionError.t()}
-  def get_sessions(%User{} = user) do
+  def get_sessions(user_id) do
     # get session id's from redis under "user:{user_id}:sessions"
-    session_key = generate_key(user.id, "sessions")
-
-    case Redix.command(:redix, ["SMEMBERS", session_key]) do
-      {:ok, sessions} ->
-        session_ids =
-          sessions
-          |> Enum.map(fn session ->
-            [session_id, _expiration] = session |> String.split(":")
-            session_id
-          end)
-
-        {:ok, user, session_ids}
+    session_key = generate_key(user_id, "sessions")
 
-      {:error, error} ->
-        {:error, error}
-    end
+    Redix.command(:redix, ["ZRANGE", session_key, 0, -1])
   end
 
   defp is_active_for_user_id(session_id, user_id) do
-    case get_session_ids_by_user_id(user_id) do
-      {:error, error} ->
-        {:error, error}
-
-      session_ids ->
-        Enum.member?(session_ids, session_id)
-    end
-  end
-
-  defp get_sessions_by_user_id(user_id) do
-    # get session id's from redis under "user:{user_id}:sessions"
     session_key = generate_key(user_id, "sessions")
-
-    case Redix.command(:redix, ["SMEMBERS", session_key]) do
-      {:ok, sessions} -> sessions
-      {:error, error} -> {:error, error}
-    end
-  end
-
-  defp get_session_ids_by_user_id(user_id) do
-    case get_sessions_by_user_id(user_id) do
+    # check ZSCORE for user_id:session_id pair
+    # returns score if it is a member of the set
+    # returns nil if not a member
+    case Redix.command(:redix, ["ZSCORE", session_key, session_id]) do
       {:error, error} -> {:error, error}
-      sessions -> Enum.map(sessions, &(String.split(&1, ":") |> List.first()))
+      {:ok, nil} -> false
+      {:ok, _score} -> true
     end
   end
 
   @doc """
-  Deletes a specific `User` session
+  Deletes the session specified in conn
   """
-  @spec delete_session(
-          user :: User.t(),
-          session_id :: String.t()
-        ) ::
-          {:ok, user :: User.t()} | {:error, atom() | Redix.Error.t() | Redix.ConnectionError.t()}
-  def delete_session(%User{} = user, session_id) do
-    # delete session id from redis under "user:{user_id}:sessions"
-    session_key = generate_key(user.id, "sessions")
-
-    case Redix.command(:redix, ["SREM", session_key, session_id]) do
-      {:ok, _} -> {:ok, user}
-      {:error, error} -> {:error, error}
-    end
-  end
+  @spec delete(conn :: Plug.Conn.t()) ::
+          {:ok, conn :: Plug.Conn.t()}
+          | {:error, atom() | Redix.Error.t() | Redix.ConnectionError.t()}
+  def delete(conn) do
+    # get user from guardian resource
+    %{id: user_id} = Guardian.Plug.current_resource(conn)
+    # get session_id from jti in jwt token
+    %{claims: %{"jti" => session_id}} =
+      Guardian.Plug.current_token(conn)
+      |> Guardian.peek()
 
-  defp delete_session_by_user_id(user_id, session) do
-    # delete session from redis under "user:{user_id}:sessions"
+    # delete session id from redis under "user:{user_id}:sessions"
     session_key = generate_key(user_id, "sessions")
 
-    case Redix.command(:redix, ["SREM", session_key, session]) do
-      {:ok, _} -> {:ok, user_id}
-      {:error, error} -> {:error, error}
+    case Redix.command(:redix, ["ZREM", session_key, session_id]) do
+      {:ok, _} ->
+        # sign user out
+        conn = Guardian.Plug.sign_out(conn)
+        {:ok, conn}
+
+      {:error, error} ->
+        {:error, error}
     end
   end
 
   @doc """
   Deletes every session instance for the specified `User`
   """
-  @spec delete_sessions(user :: User.t()) :: :ok
+  @spec delete_sessions(user :: User.t()) :: {:ok, non_neg_integer} | Redix.ConnectionError.t()
   def delete_sessions(%User{} = user) do
     # delete session id from redis under "user:{user_id}:sessions"
     session_key = generate_key(user.id, "sessions")
-
-    case Redix.command(:redix, ["SPOP", session_key]) do
-      {:ok, nil} -> :ok
-      # repeat until redix returns nil
-      _ -> delete_sessions(user)
-    end
+    Redix.command(:redix, ["DEL", session_key])
   end
 
   defp save(%User{} = user, session_id, ttl) do
@@ -290,39 +257,39 @@ defmodule EpochtalkServer.Session do
     maybe_extend_ttl(ban_key, ttl, old_ttl)
   end
 
-  # these two rules ensure that sessions will eventually be deleted:
-  # clean expired sessions and add a new one
-  # ttl expiry for this key will delete all sessions in the set
+  # session storage uses "pseudo-expiry" (via redis sorted-set score)
+  # these rules ensure that session storage will not grow indefinitely:
+  #
+  # on every new session add,
+  #   clean (delete) expired sessions by expiry
+  #   add the new session, with expiry
+  #   ttl expiry for this key will delete all sessions in the set
   defp add_session(user_id, session_id, ttl) do
     # save session id to redis under "user:{user_id}:sessions"
     session_key = generate_key(user_id, "sessions")
-    # current unix time (default :seconds)
-    now = DateTime.utc_now() |> DateTime.to_unix()
-    # intended unix expiration of this session
-    unix_expiration = now + ttl
     # delete expired sessions
-    get_sessions_by_user_id(user_id)
-    |> Enum.each(fn session ->
-      [_session_id, expiration] = String.split(session, ":")
-
-      if String.to_integer(expiration) < now do
-        delete_session_by_user_id(user_id, session)
-      end
-    end)
+    delete_expired_sessions(session_key)
+    # intended unix expiration of this session
+    unix_expiration = current_time() + ttl
 
     # add new session, noting unix expiration
-    result =
-      Redix.command(:redix, [
-        "SADD",
-        session_key,
-        session_id <> ":" <> Integer.to_string(unix_expiration)
-      ])
+    result = Redix.command(:redix, ["ZADD", session_key, unix_expiration, session_id])
 
     # set ttl
     maybe_extend_ttl(session_key, ttl)
     result
   end
 
+  # delete expired sessions by expiration (sorted set score)
+  defp delete_expired_sessions(session_key) do
+    Redix.command(:redix, ["ZREMRANGEBYSCORE", session_key, "-inf", current_time()])
+  end
+
+  # current unix time (default :seconds)
+  defp current_time() do
+    DateTime.utc_now() |> DateTime.to_unix()
+  end
+
   defp generate_key(user_id, "user"), do: "user:#{user_id}"
   defp generate_key(user_id, type), do: "user:#{user_id}:#{type}"
 

lib/epochtalk_server_web/controllers/user_controller.ex

@@ -159,17 +159,17 @@ defmodule EpochtalkServerWeb.UserController do
   Logs out the logged in `User`
 
   - TODO(boka): check if user is on page that requires auth
-  - TODO(boka): Delete users session
   """
   def logout(conn, _attrs) do
-    with {:auth, true} <- {:auth, Guardian.Plug.authenticated?(conn)} do
-      user = Guardian.Plug.current_resource(conn)
-      token = Guardian.Plug.current_token(conn)
+    with {:auth, true} <- {:auth, Guardian.Plug.authenticated?(conn)},
+         user <- Guardian.Plug.current_resource(conn),
+         token <- Guardian.Plug.current_token(conn),
+         {:ok, conn} <- Session.delete(conn) do
       EpochtalkServerWeb.Endpoint.broadcast("user:#{user.id}", "logout", %{token: token})
-      Guardian.Plug.sign_out(conn)
       render(conn, "logout.json", data: %{success: true})
     else
       {:auth, false} -> ErrorHelpers.render_json_error(conn, 400, "Not logged in")
+      {:error, error} -> ErrorHelpers.render_json_error(conn, 500, error)
       _ -> ErrorHelpers.render_json_error(conn, 500, "There was an issue signing out")
     end
   end

test/epochtalk_server/session_test.exs

@@ -33,6 +33,105 @@ defmodule EpochtalkServerWeb.SessionTest do
     end
   end
 
+  describe "delete/1" do
+    @tag :authenticated
+    test "deletes authenticated user's session", %{conn: conn, authed_user: authed_user} do
+      session_id = conn.private.guardian_default_claims["jti"]
+      {:ok, resource} = Session.get_resource(authed_user.id, session_id)
+      %{id: session_user_id, username: session_user_username} = resource
+      assert session_user_id == authed_user.id
+      assert session_user_username == authed_user.username
+
+      # delete user session indirectly via logout route
+      #   this is done rather than calling Session.delete
+      #   because guardian does not load the resource
+      #   when calling Session.delete directly
+      unauthed_conn = delete(conn, Routes.user_path(conn, :logout))
+      assert Guardian.Plug.authenticated?(unauthed_conn) == false
+      unauthed_resource = Session.get_resource(authed_user.id, session_id)
+
+      assert unauthed_resource ==
+               {:error, "No session for user_id #{authed_user.id} with id #{session_id}"}
+    end
+  end
+
+  describe "create/3 session expiration" do
+    test "deletes an expired user session when logging in", %{conn: conn, user: user} do
+      remember_me = false
+      # create session that should be deleted
+      {:ok, authed_user_to_delete, _token, authed_conn_to_delete} =
+        Session.create(user, remember_me, conn)
+
+      session_id_to_delete = authed_conn_to_delete.private.guardian_default_claims["jti"]
+      # create session that shouldn't be deleted
+      {:ok, authed_user_to_persist, _token, authed_conn_to_persist} =
+        Session.create(user, remember_me, conn)
+
+      session_id_to_persist = authed_conn_to_persist.private.guardian_default_claims["jti"]
+      # check that all sessions are active
+      {:ok, resource_to_delete} = Session.get_resource(user.id, session_id_to_delete)
+
+      %{id: session_user_id_to_delete, username: session_user_username_to_delete} =
+        resource_to_delete
+
+      assert session_user_id_to_delete == user.id
+      assert session_user_username_to_delete == user.username
+      {:ok, resource_to_persist} = Session.get_resource(user.id, session_id_to_persist)
+
+      %{id: session_user_id_to_persist, username: session_user_username_to_persist} =
+        resource_to_persist
+
+      assert session_user_id_to_persist == user.id
+      assert session_user_username_to_persist == user.username
+      # change expiration of session to delete to UTC 0
+      expiration_utc = 0
+
+      Redix.command(:redix, [
+        "ZADD",
+        "user:#{user.id}:sessions",
+        expiration_utc,
+        session_id_to_delete
+      ])
+
+      # create a new session (should delete expired sessions)
+      {:ok, new_authed_user, _token, new_authed_conn} = Session.create(user, remember_me, conn)
+      new_session_id = new_authed_conn.private.guardian_default_claims["jti"]
+      # check that active sessions are still active
+      {:ok, resource_to_persist} = Session.get_resource(user.id, session_id_to_persist)
+
+      %{id: session_user_id_to_persist, username: session_user_username_to_persist} =
+        resource_to_persist
+
+      assert session_user_id_to_persist == user.id
+      assert session_user_username_to_persist == user.username
+
+      authenticate_persisted_conn =
+        get(authed_conn_to_persist, Routes.user_path(authed_conn_to_persist, :authenticate))
+
+      assert user.id == json_response(authenticate_persisted_conn, 200)["id"]
+      {:ok, new_resource} = Session.get_resource(user.id, new_session_id)
+      %{id: new_session_user_id, username: new_session_user_username} = new_resource
+      assert new_session_user_id == user.id
+      assert new_session_user_username == user.username
+
+      new_authenticate_conn =
+        get(new_authed_conn, Routes.user_path(new_authed_conn, :authenticate))
+
+      assert user.id == json_response(new_authenticate_conn, 200)["id"]
+      # check that expired session is not active
+      unauthed_resource = Session.get_resource(user.id, session_id_to_delete)
+
+      assert unauthed_resource ==
+               {:error, "No session for user_id #{user.id} with id #{session_id_to_delete}"}
+
+      authenticate_deleted_conn =
+        get(authed_conn_to_delete, Routes.user_path(authed_conn_to_delete, :authenticate))
+
+      assert %{"error" => "Unauthorized", "message" => "No resource found"} =
+               json_response(authenticate_deleted_conn, 401)
+    end
+  end
+
   describe "create/3 expiration/ttl" do
     setup [:flush_redis]