Replaced CredentialStore Agent with a GenServer

Changes:
- Replaced Agent with GenServer
- We can now change the user data in all sessions and not only the current session.
  We can also change the data from outside a controller.
- Fixed some wrong @specs regarding credentials
- Added a 'credentials' type to enhance readability.

There are lot's of changes, but the commit makes sense as a whole
(except maybe for the tye definitions and fixing the @specs)

Now, in more detail:

*Replaced Agent with a GenServer*
The API remains backward compatible, except for the fact that all
references to Agent are now references to a Server.

A GenServer is a better choice because it abstracts the state further
and allows for more complex queries of the session data.

In this case, a GenServer was necessary to have a fast way of getting
all sessions belonging to a certain user without iterating over all
sessions.

Having a GenServer allows us to keep an (opaque) index, hidden from the
end user (as it should be, given it's an implementation detail).
This extra decoupling between querying the state and the way the state
is in fact stores looks like a good thing.

This GenServer is implemented using ExActor.
ExActor gives an even nicer API on top of the raw GenServer,
and reduces boilerplate by a lot.

It does introduce an extra dependency, but it's compile-time only,
as all macros are expanded at compile-time into a normal GenServer.
The mapping between an ExActor and a GenServer is straightforward,
so I think it doesn't loose much in clarity.
In fact, I think redability is enhanced by not using a raw GenServer.

*Changing the user's data in all sessions*

It requires keeping an additional index in the Credential store,
but it's worth it because it makes it very easy to change the user's
data. Previously, the user had to log out and login so that changes
would be reflected in all sessions.

README.md

@@ -688,20 +688,20 @@ IpAddress authentication is a good solution for server to server rest APIs.
 
 ```elixir
 creds = Coherence.Authentication.Basic.encode_credentials("Admin", "SecretPass")
-Coherence.CredentialStore.Agent.put_credentials(creds, %{role: :admin})
+Coherence.CredentialStore.Server.put_credentials(creds, %{role: :admin})
 ```
 
 ### Add Token Credentials Example
 
 ```elixir
 token = Coherence.Authentication.Token.generate_token
-Coherence.CredentialStore.Agent.put_credentials(token, %{role: :admin})
+Coherence.CredentialStore.Server.put_credentials(token, %{role: :admin})
 ```
 
 ### Add IP Credentials Example
 
 ```elixir
-Coherence.CredentialStore.Agent.put_credentials({127.0.0.1}, %{role: :admin})
+Coherence.CredentialStore.Server.put_credentials({127.0.0.1}, %{role: :admin})
 ```
 
 IpAddress authentication does not require this step. Its optional. If the user_data

lib/coherence.ex

@@ -232,6 +232,31 @@ Run `$ mix help coherence.install` or `$ mix help coherence.install` for more in
   """
   def current_user(conn), do: conn.assigns[Config.assigns_key]
 
+  @doc """
+  Updates the user login data in the current sessions.
+
+  Other sessions belonging to the same user won't be updated.
+  Requires access to the `conn`, which means it can't be called outside of the context of a conn.
+  To update all session belonging to the user see `t:update_user_login/1`.
+  """
+  def update_user_login(conn, user) do
+    apply(Config.auth_module,
+          Config.update_login,
+          [conn, user, [id_key: Config.schema_key]])
+  end
+
+  @doc """
+  Updates the user login data in the all sessions belonging to the user.
+
+  All sessions belonging to the same user will be updated.
+  Doesn't need access to the `conn`, which means it can be called anywhere.
+  To update only the current session see `t:update_user_login/2`
+  """
+  def update_user_logins(user) do
+    # Handle a user's DBStore
+    Coherence.CredentialStore.Server.update_user_logins(user)
+  end
+
   @doc """
   Get the currently logged in user name.
   """

lib/coherence/plugs/authorization/basic.ex

@@ -25,11 +25,12 @@ defmodule Coherence.Authentication.Basic do
   import Coherence.Authentication.Utils
 
   alias Coherence.Messages
+  alias Coherence.CredentialStore.Types, as: T
 
   @doc """
     Returns the encoded form for the given `user` and `password` combination.
   """
-  @spec encode_credentials(atom | String.t, String.t | nil) :: String.t
+  @spec encode_credentials(atom | String.t, String.t | nil) :: T.credentials
   def encode_credentials(user, password), do: Base.encode64("#{user}:#{password}")
 
   @spec create_login(String.t, String.t, t, Keyword.t) :: t
@@ -52,7 +53,7 @@ defmodule Coherence.Authentication.Basic do
     %{
       realm: Keyword.get(opts, :realm, Messages.backend().restricted_area()),
       error: Keyword.get(opts, :error, Messages.backend().http_authentication_required()),
-      store: Keyword.get(opts, :store, Coherence.CredentialStore.Agent),
+      store: Keyword.get(opts, :store, Coherence.CredentialStore.Server),
       assigns_key: Keyword.get(opts, :assigns_key, :current_user),
     }
   end

lib/coherence/plugs/authorization/credential_store/server.ex

@@ -0,0 +1,149 @@
+defmodule Coherence.CredentialStore.Server do
+  # ExActor will help us simplify the definition of our GenServer
+  use ExActor.GenServer, export: Coherence.CredentialStore.Server
+
+  alias Coherence.CredentialStore.Types, as: T
+
+  @behaviour Coherence.CredentialStore
+
+  # Server State
+  # ------------
+  # The state of the server is a record containing a store and an index.
+  require Record
+  # The state is not part of the server's API, so its type and constructor will be private.
+  # We want to have the flexibility of changing this later for efficiency reasons.
+  Record.defrecordp :state, [store: %{}, index: %{}]
+  # The store is a map mapping credentials to user_data
+  @typep store :: %{T.credentials => T.user_data}
+  # The index is a map mapping the user's id to that user's active sessions.
+  @typep index :: %{T.user_id => MapSet.t(T.credentials)}
+  # The index makes it much easier to find out which session credentials belong to a certain user.
+  # Without this index, to get or update all sessions belonging to a user, one has to iterate over all the items in the map
+  # This is linear on the number of active sessions (>= the number of users because there can be >= 1 session per user)
+  # With the index, accessing all the sessions belonging to a user costs a map lookup
+  # and iterating over all sessions it is linear on the number of sessions belonging to that user.
+  # This is a clear improvement if there are many concurrent users.
+  #
+  # The costs of this index are increased memory consumption (we need to keep what's basically a duplicated state) and slower update operations.
+  # For applications with many concurrent sessions, the benefits probably outweight the costs.
+  # For applications without many concurrent sessions these costs aren't great anyway.
+
+  # Public API for our server
+  # -------------------------
+
+  defstart start_link, do:
+    # initially, both the store and the index are empty
+    initial_state(state(store: %{}, index: %{}))
+
+  @spec update_user_logins(T.user_data) :: [T.credentials]
+  defcall update_user_logins(%{id: user_id} = user_data), state: state(store: store, index: index) do
+    # TODO:
+    # Maybe support updating ths user's ID.
+    # Currently it's not obvious what's the best API for this.
+    # -----------------------------------------
+    # Get credentials for all sessions belonging to user
+    # This operations is read-only for the index.
+    sessions_credentials = Map.get(index, user_id, [])
+    # Build the changes to apply to the store.
+    # This is linear on the number of sessions belonging to the user.
+    # It is much better than the naive approach without the index,
+    # which is linear on the total number of active sessions.
+    delta = for credentials <- sessions_credentials, into: %{}, do: {credentials, user_data}
+    set_and_reply(
+      # Update the store with the new user_data model
+      # The index data is not touched, so the index is returned unchanged.
+      state(store: Map.merge(store, delta), index: index),
+      # Return the updated credentials for all sessions belonging to the user
+      Map.keys(delta)
+    )
+  end
+  defcall update_user_logins(_), do:
+    # If the user_data doesn't contain an ID, there are no sessions belonging to the user
+    # There is no need to update anything and we just return an empty list
+    reply([])
+
+  @spec update_user_logins(T.credentials) :: T.user_data
+  defcall get_user_data(credentials), state: state(store: store) do
+    Map.get(store, credentials) |> reply
+  end
+
+  @spec  put_credentials(T.credentials, T.user_data) :: T.user_data
+  defcall put_credentials(credentials, user_data), state: state(store: store, index: index) do
+    # The data has been changed; We must update both the index and the store
+    # Update the index only if it makes sense
+    possibly_updated_index = maybe_add_credentials_to_index(index, user_data, credentials)
+    # Always update the store
+    updated_store = add_credentials_to_store(store, user_data, credentials)
+    # Update the state and reply
+    set_and_reply(
+      state(store: updated_store, index: possibly_updated_index),
+      # Return the updated store
+      user_data
+    )
+  end
+
+  @spec  delete_credentials(T.credentials) :: store
+  defcall delete_credentials(credentials), state: state(store: store, index: index) do
+    user_data = Map.get(store, credentials)
+    # The data has been changed; We must update both the index and the store
+    # Update the index only if it makes sense
+    possibly_updated_index = maybe_delete_credentials_from_index(index, user_data, credentials)
+    # Always update the store
+    updated_store = delete_credentials_from_store(store, credentials)
+    set_and_reply(
+      state(store: updated_store, index: possibly_updated_index),
+      updated_store
+    )
+  end
+
+  defcast stop, do: stop_server(:normal)
+
+  # Helper functions:
+  # -----------------
+  @spec maybe_add_credentials_to_index(index, T.user_data, T.credentials) :: index
+  defp maybe_add_credentials_to_index(index, %{id: user_id}, credentials), do:
+    # If there isn't an entry for user_id in the index, create it.
+    # If there is already an entry for user_id, append the new credentials.
+    Map.update(index,
+               user_id,
+               # If the user has no credentials, create a new entry in the MapSet
+               # with the new credentials
+               MapSet.put(MapSet.new(), credentials),
+               # If the user already has some credentials, put the new credential
+               &(MapSet.put(&1, credentials)))      
+  defp maybe_add_credentials_to_index(index, nil, _), do: index
+  defp maybe_add_credentials_to_index(index, _, _), do: index
+
+  @spec maybe_delete_credentials_from_index(index, T.user_data, T.credentials) :: index
+  defp maybe_delete_credentials_from_index(index, %{id: user_id}, credentials) do
+    # We must handle 3 cases:
+    case index do
+      %{^user_id => sessions_credentials} ->
+        case MapSet.size(sessions_credentials) do
+          # 1. The user has a single session
+          # -----------------------------------
+          # If there is only a single session belonging to that user,
+          # the user is no longer active after deleting the session.
+          # We can delete it from the index.
+          1 -> Map.delete(index, user_id)
+          # 2. The user has more than one session
+          # -------------------------------------
+          # We must delete the credentials from the set of session credentials
+          _ -> %{index | user_id => MapSet.delete(sessions_credentials, credentials)}
+        end
+      # 3. The user has no sessions (return the index unchanged)
+      _ -> index
+    end
+  end
+  defp maybe_delete_credentials_from_index(index, nil, _), do: index
+  defp maybe_delete_credentials_from_index(index, _, _), do: index
+
+
+  @spec add_credentials_to_store(store, T.user_data, T.credentials) :: store
+  defp add_credentials_to_store(store, user_data, credentials), do:
+    Map.put(store, credentials, user_data)
+
+  @spec delete_credentials_from_store(store, T.credentials) :: store
+  defp delete_credentials_from_store(store, credentials), do:
+    Map.delete(store, credentials)
+end

lib/coherence/plugs/authorization/credential_store/session.ex

@@ -2,7 +2,7 @@ defmodule Coherence.CredentialStore.Session do
   @moduledoc """
   Stores current credential information.
 
-  Uses an Agent to save logged in credentials.
+  Uses an Server to save logged in credentials.
 
   Note: If you restart the phoenix server, this information
   is lost, requiring the user to log in again.
@@ -21,17 +21,19 @@ defmodule Coherence.CredentialStore.Session do
 
   require Logger
   alias Coherence.DbStore
-  alias Coherence.CredentialStore.Agent
+  alias Coherence.CredentialStore.Server
+  alias Coherence.CredentialStore.Types, as: T
 
   @spec start_link() :: {:ok, pid} | {:error, atom}
   def start_link do
-    Agent.start_link
+    Server.start_link()
   end
 
   @doc """
   Gets the user data for the given credentials
   """
-  @spec get_user_data({HashDict.t, nil | struct, integer | nil}) :: any
+
+  @spec get_user_data({T.credentials, nil | struct, integer | nil}) :: any
   def get_user_data({credentials, nil, _}) do
     get_data credentials
   end
@@ -41,22 +43,23 @@ defmodule Coherence.CredentialStore.Session do
         case DbStore.get_user_data(db_model.__struct__, credentials, id_key) do
           nil -> nil
           user_data ->
-            Agent.put_credentials(credentials, user_data)
+            Server.put_credentials(credentials, user_data)
             user_data
         end
       other ->
         other
     end
   end
 
-  defp get_data(credentials), do: Agent.get_user_data(credentials)
+  @spec get_data(T.credentials) :: any
+  defp get_data(credentials), do: Server.get_user_data(credentials)
 
   @doc """
   Puts the `user_data` for the given `credentials`.
   """
-  @spec put_credentials({HashDict.t, any, atom}) :: any
+  @spec put_credentials({T.credentials, any, atom}) :: any
   def put_credentials({credentials, user_data, id_key}) do
-    Agent.put_credentials(credentials, user_data)
+    Server.put_credentials(credentials, user_data)
     DbStore.put_credentials(user_data, credentials, id_key)
   end
 
@@ -65,13 +68,13 @@ defmodule Coherence.CredentialStore.Session do
 
   Returns the current value of `credentials`, if `credentials` exists.
   """
-  @spec delete_credentials(HashDict.t) :: any
+  @spec delete_credentials(T.credentials) :: any
   def delete_credentials(credentials) do
     case get_data credentials do
       nil -> nil
       user_data ->
         DbStore.delete_credentials user_data, credentials
-        Agent.delete_credentials(credentials)
+        Server.delete_credentials(credentials)
     end
   end
 end

lib/coherence/plugs/authorization/credential_store/types.ex

@@ -0,0 +1,5 @@
+defmodule Coherence.CredentialStore.Types do
+  @type credentials :: String.t
+  @type user_data :: Ecto.Schema.t | Map.t
+  @type user_id :: any
+end

lib/coherence/plugs/authorization/ip_address.ex

@@ -11,7 +11,7 @@ defmodule Coherence.Authentication.IpAddress do
 
   If you would like access to the current user you must set each authorized IP address like:
 
-      Coherence.CredentialStore.Agent.put_credentials({127.0.0.1}, %{role: :admin})
+      Coherence.CredentialStore.Server.put_credentials({127.0.0.1}, %{role: :admin})
 
   or use a custom store like:
 
@@ -65,15 +65,15 @@ defmodule Coherence.Authentication.IpAddress do
     Add the credentials for a `token`. `user_data` can be any term but must not be `nil`.
   """
   @spec add_credentials(String.t, t, module) :: t
-  def add_credentials(ip, user_data, store \\ Coherence.CredentialStore.Agent) do
+  def add_credentials(ip, user_data, store \\ Coherence.CredentialStore.Server) do
     store.put_credentials(ip, user_data)
   end
 
   @doc """
     Remove the credentials for a `token`.
   """
   @spec remove_credentials(String.t, module) :: t
-  def remove_credentials(ip, store \\ Coherence.CredentialStore.Agent) do
+  def remove_credentials(ip, store \\ Coherence.CredentialStore.Server) do
     store.delete_credentials(ip)
   end
 
@@ -83,7 +83,7 @@ defmodule Coherence.Authentication.IpAddress do
       allow: Keyword.get(opts, :allow, []),
       deny: Keyword.get(opts, :deny, []),
       error: Keyword.get(opts, :error, Messages.backend().unauthorized_ip_address()),
-      store: Keyword.get(opts, :store, Coherence.CredentialStore.Agent),
+      store: Keyword.get(opts, :store, Coherence.CredentialStore.Server),
       assign_key: Keyword.get(opts, :assign_key, :current_user),
     }
   end