From ead51b308257e50356dd527a7ac8760ceb0f1d75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Floure?= Date: Tue, 22 Dec 2020 14:22:34 +0100 Subject: [PATCH] Refactor user-related LDAP logic --- lib/recycledcloud/accounts.ex | 8 +- lib/recycledcloud/accounts/user.ex | 114 ++++++++---------- .../templates/user_settings/edit.html.eex | 5 +- 3 files changed, 58 insertions(+), 69 deletions(-) diff --git a/lib/recycledcloud/accounts.ex b/lib/recycledcloud/accounts.ex index fd6fd12..cafd9b5 100644 --- a/lib/recycledcloud/accounts.ex +++ b/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 - Repo.get!(User, id) |> User.maybe_populate_email() - end + def get_user!(id), do: User.get!(id) ## 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 """ diff --git a/lib/recycledcloud/accounts/user.ex b/lib/recycledcloud/accounts/user.ex index 3f06e4c..77ee9fd 100644 --- a/lib/recycledcloud/accounts/user.ex +++ b/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 - # FIXME - "uid=#{uid},ou=users,dc=recycled,dc=cloud" + # Note: the returned LDAP values usually are lists of charlist (e.g. `%{'uid' + # => ['myusername']}`). + 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 - query = fn ldap_conn -> Exldap.search_field(ldap_conn, :uid, user.username) end - case query |> LDAP.execute do - {:ok, []} -> - 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 + def maybe_populate_ldap_attributes(user) do + # TODO: could be useful to cache this data somehow, perhaps in an ETS + # table? We query the LDAP server every time we try to fetch an user's + # data at the moment, which is inefficient. - user |> Map.put(:email, email) - {:error, _} -> + case get_ldap_attributes(user) do + {: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 query |> LDAP.execute do - {:ok, []} -> nil - {:ok, result} -> - {:ok, entry} = result |> Enum.fetch(0) - - Logger.info("Found #{entry.object_name} in directory. Syncing with \ - local database.") - - attributes = entry |> Map.get(:attributes) |> Enum.into(%{}) - username = attributes - |> Map.get('uid') - |> Enum.at(0) - |> List.to_string - - case Accounts.register_user(%{username: username}) do - {:ok, user} -> user - {:error, _} -> nil + case get_ldap_attributes(username) do + {:ok, %{'uid' => [raw_uid], 'mail' => [raw_email], 'dn' => raw_dn}} -> + uid = List.to_string(raw_uid) + email = List.to_string(raw_email) + dn = List.to_string(raw_dn) + case Accounts.register_user(%{username: uid, email: email, dn: dn}) do + {:ok, user} -> + user + {:error, err} -> + Logger.warn("Something went wrong importing user from LDAP: #{inspect(err)}") + 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 diff --git a/lib/recycledcloud_web/templates/user_settings/edit.html.eex b/lib/recycledcloud_web/templates/user_settings/edit.html.eex index b04adec..554883a 100644 --- a/lib/recycledcloud_web/templates/user_settings/edit.html.eex +++ b/lib/recycledcloud_web/templates/user_settings/edit.html.eex @@ -1,8 +1,9 @@

Account settings

-You are currently logged in as <%= @current_user.username %>, using <%= -@current_user.email %> as primary contact method. +You are currently logged in as <%= @current_user.username %> (<%= +@current_user.dn %>), using <%= @current_user.email %> as primary contact +method.

Change email