Refactor user-related LDAP logic

lib/recycledcloud/accounts.ex

@@ -23,7 +23,7 @@ defmodule RecycledCloud.Accounts do
 
   """
   def get_user_by_username(username) when is_binary(username) do
-    User.get_by_username(username) |> User.maybe_populate_email()
+    User.get_by_username(username)
   end
 
   @doc """
@@ -58,9 +58,7 @@ defmodule RecycledCloud.Accounts do
       ** (Ecto.NoResultsError)
 
   """
-  def get_user!(id) do
+  def get_user!(id), do: User.get!(id)
-    Repo.get!(User, id) |> User.maybe_populate_email()
-  end
 
   ## User registration
 
@@ -236,7 +234,7 @@ defmodule RecycledCloud.Accounts do
   """
   def get_user_by_session_token(token) do
     {:ok, query} = UserToken.verify_session_token_query(token)
-    Repo.one(query) |> User.maybe_populate_email()
+    Repo.one(query) |> User.maybe_populate_ldap_attributes()
   end
 
   @doc """

lib/recycledcloud/accounts/user.ex

@@ -11,32 +11,46 @@ defmodule RecycledCloud.Accounts.User do
   schema "users" do
     field :username, :string
     field :password, :string, virtual: true
+    field :dn, :string, virtual: true
     field :email, :string, virtual: true
     field :confirmed_at, :naive_datetime
 
     timestamps()
   end
 
-  defp get_dn_for(%User{username: uid}) do
+  # Note: the returned LDAP values usually are lists of charlist (e.g. `%{'uid'
-    # FIXME
+  # => ['myusername']}`).
-    "uid=#{uid},ou=users,dc=recycled,dc=cloud"
+  defp get_ldap_attributes(%User{username: uid}), do: get_ldap_attributes(uid)
+  defp get_ldap_attributes(uid) do
+    query = fn ldap_conn -> Exldap.search_field(ldap_conn, :uid, uid) end
+    case query |> LDAP.execute do
+      {:ok, []} -> {:error, "could not find matching object"}
+      {:ok, [entry]} ->
+        attrs = entry
+                |> Map.get(:attributes)
+                |> Enum.into(%{})
+                |> Map.put('dn', entry.object_name)
+        {:ok, attrs}
+      {:ok, [entry|_]} -> {:error, "found more than one object with uid #{uid}"}
+      {:error, err} -> {:error, inspect(err)}
+    end
   end
 
-  def maybe_populate_email(user) do
+  def maybe_populate_ldap_attributes(user) do
-    query = fn ldap_conn -> Exldap.search_field(ldap_conn, :uid, user.username) end
+    # TODO: could be useful to cache this data somehow, perhaps in an ETS
-    case query |> LDAP.execute do
+    # table? We query the LDAP server every time we try to fetch an user's
-      {:ok, []} ->
+    # data at the moment, which is inefficient.
-        user
-      {:ok, result} ->
-        {:ok, entry} = result |> Enum.fetch(0)
-        attributes = entry |> Map.get(:attributes) |> Enum.into(%{})
-        email = attributes
-                |> Map.get('mail')
-                |> Enum.at(0)
-                |> List.to_string
 
-        user |> Map.put(:email, email)
+    case get_ldap_attributes(user) do
-      {:error, _} ->
+      {:ok, %{'mail' => [raw_email], 'dn' => raw_dn}} ->
+        email = List.to_string(raw_email)
+        dn = List.to_string(raw_dn)
+        user |> Map.put(:email, email) |> Map.put(:dn, dn)
+      {:ok, _} ->
+        Logger.warn("Malformed LDAP user object")
+        user
+      {:error, err} ->
+        Logger.warn("Error querying LDAP backend: #{err}")
         user
     end
   end
@@ -44,34 +58,31 @@ defmodule RecycledCloud.Accounts.User do
   def get_by_username(username) when is_binary(username) do
     local_user = Repo.get_by(User, username: username)
     if local_user do
-      local_user
+      local_user |> User.maybe_populate_ldap_attributes()
     else
-      query = fn ldap_conn -> Exldap.search_field(ldap_conn, :uid, username) end
+      case get_ldap_attributes(username) do
-      case query |> LDAP.execute do
+        {:ok, %{'uid' => [raw_uid], 'mail' => [raw_email], 'dn' => raw_dn}} ->
-        {:ok, []} -> nil
+          uid = List.to_string(raw_uid)
-        {:ok, result} ->
+          email = List.to_string(raw_email)
-          {:ok, entry} = result |> Enum.fetch(0)
+          dn = List.to_string(raw_dn)
-
+          case Accounts.register_user(%{username: uid, email: email, dn: dn}) do
-          Logger.info("Found #{entry.object_name} in directory. Syncing with \
+            {:ok, user} ->
-            local database.")
+              user
-
+            {:error, err} ->
-          attributes = entry |> Map.get(:attributes) |> Enum.into(%{})
+              Logger.warn("Something went wrong importing user from LDAP: #{inspect(err)}")
-          username = attributes
+              nil
-                     |> Map.get('uid')
-                     |> Enum.at(0)
-                     |> List.to_string
-
-          case Accounts.register_user(%{username: username}) do
-            {:ok, user} -> user
-            {:error, _} -> nil
           end
         {:error, err} ->
-          Logger.warn("LDAP error: #{err}")
+          Logger.warn("Error querying LDAP backend: #{err}")
           nil
       end
     end
   end
 
+  def get!(id) do
+    Repo.get!(User, id) |> User.maybe_populate_ldap_attributes()
+  end
+
   @doc """
   A user changeset for registration.
 
@@ -79,21 +90,11 @@ defmodule RecycledCloud.Accounts.User do
   Otherwise databases may truncate the email without warnings, which
   could lead to unpredictable or insecure behaviour. Long passwords may
   also be very expensive to hash for certain algorithms.
-
-  ## Options
-
-    * `:hash_password` - Hashes the password so it can be stored securely
-      in the database and ensures the password field is cleared to prevent
-      leaks in the logs. If password hashing is not needed and clearing the
-      password field is not desired (like when using this changeset for
-      validations on a LiveView form), this option can be set to `false`.
-      Defaults to `true`.
   """
-  def registration_changeset(user, attrs, opts \\ []) do
+  def registration_changeset(user, attrs) do
     user
-    |> cast(attrs, [:username, :password])
+    |> cast(attrs, [:username, :password, :email, :dn])
     |> validate_email()
-    #|> validate_password(opts)
   end
 
   defp validate_email(changeset) do
@@ -129,15 +130,6 @@ defmodule RecycledCloud.Accounts.User do
 
   @doc """
   A user changeset for changing the password.
-
-  ## Options
-
-    * `:hash_password` - Hashes the password so it can be stored securely
-      in the database and ensures the password field is cleared to prevent
-      leaks in the logs. If password hashing is not needed and clearing the
-      password field is not desired (like when using this changeset for
-      validations on a LiveView form), this option can be set to `false`.
-      Defaults to `true`.
   """
   def password_changeset(user, attrs) do
     user
@@ -159,9 +151,8 @@ defmodule RecycledCloud.Accounts.User do
   """
   def valid_password?(user = %User{username: uid}, password)
   when byte_size(password) > 0 do
-    dn = get_dn_for(user)
     query = fn ldap_conn ->
-      Exldap.verify_credentials(ldap_conn, dn, password)
+      Exldap.verify_credentials(ldap_conn, user.dn, password)
     end
 
     case query |> LDAP.execute_single do
@@ -190,11 +181,10 @@ defmodule RecycledCloud.Accounts.User do
     # fallback on erlang's `:eldap`.
     # See http://erlang.org/doc/man/eldap.html#modify-4
 
-    user_dn = get_dn_for(user)
     query = fn ldap_conn ->
       :eldap.modify_password(
         ldap_conn,
-        String.to_charlist(user_dn),
+        String.to_charlist(user.dn),
         String.to_charlist(new_password)
       )
     end

lib/recycledcloud_web/templates/user_settings/edit.html.eex

@@ -1,8 +1,9 @@
 <h1>Account settings</h1>
 
 <p>
-You are currently logged in as <i><%= @current_user.username %></i>, using <%=
+You are currently logged in as <%= @current_user.username %> (<i><%=
-@current_user.email %> as primary contact method.
+@current_user.dn %></i>), using <%= @current_user.email %> as primary contact
+method.
 </p>
 
 <h3>Change email</h3>