I am still getting used to control structures / conditionals / pattern matching in Elixir, and I have a couple of small functions that work, but don’t follow the elixir style and I think that there is a simpler way to write them. Any advice would be appreciated. These are standard authentication functions that I’m sure many people have written at some point. Here they are:
def find_by_name_or_email(str) do
user1 = Repo.get_by(User, name: name)
if (user1 == nil) do
Repo.get_by(User, email: email)
else
user1
end
end
def user_login(ident, pass) do
user = find_user_by_name_or_email(ident)
cond do
user == nil -> nil
user.hashed_password == nil -> nil
Bcrypt.verify_pass(pass, user.hashed_password) == false -> nil
true -> create_user_token(user)
end
end
I would say that the first one could be made simpler by making use of ectos or:
def find_by_name_or_email(string) do
query = from(user in User, where: user.name == ^string or user.email == ^string)
Repo.one(query)
end
the second one could probably be written like this:
def user_login(identity, password) do
with %User{} = find_by_name_or_email(identity),
hashed_password when not is_nil(hashed_password) <- user.hashed_password,
true <- Bcrypt.verify_pass(password, hashed_password) do
create_user_token(user)
end
end
But I would recommend looking into phx.gen.auth instead as it will generate authentication code for you that is considered safe (it’s easy to make small mistakes when it comes to authentication and those can be very costly)
I would second that. Verifying the password only if the user exists – like the code shown here – for example is prone to timing attacks, where a bad actor can figure out which users exist in your system.
the second one could probably be written like this:
Interesting! I haven’t used “with” and “when” like that, but that’s a good idea.
But I would recommend looking into phx.gen.auth instead
I appreciate what the Pheonix developers have done, and I learned quite a bit from reading it. At the end of the day though, I prefer to write & maintain my own code that is tailored specifically to the needs of my app, and its not something I really need help with.
Others have answered well enough already in terms of structuring, but just to add that if your find_by_name_or_email is a query result, you can use fragments to make it case-insensitive if you want.
def search_users(query) do
Repo.all(
from u in User,
where: (fragment("lower(?) like lower(?)", u.username, ^"%#{query}%")),
)
end
If you have more conditions you can do stuff like this as well:
def existing_request(current_user, user) do
Repo.one(
from r in Request,
where: (r.user_id == ^user.id and r.current_user_id == ^current_user.id)
or (r.user_id == ^current_user.id and r.current_user_id == ^user.id)
)
end
I don’t normally do this, but there are also pipe options. I think the below works…until someone tells me I’m wrong… Pipes can look simpler and more complex at the same time, so I only use them for recursive fragments tbh.
def find_by_name_or_email(query) do
User
|> where([u], u.name == ^query or u.email == ^query)
|> Repo.one()
end
For your user_login I would do this. You can add “else nil” if you want, but your Bcrypt stage is going to fail if the other two fail, so I don’t really get why you would have them as conditions unless your planning to log them being nil?
def user_login(ident, password) do
user = find_user_by_name_or_email(ident)
if Bcrypt.verify_pass(password, user.hashed_password) do
create_user_token(user)
end
end
Once your query becomes longer than 20 lines, the piped version is not only more readable, but more flexible in terms of order. I usually tend now to write all queries in pipe version as it becomes a mess once you have to introduce piped queries at some point.
Like @LostKobrakai said, there is a reason it’s written in a certain way. Your first examples suffer from timing attacks and maybe other issues. Security is not at all trivial, don’t write your own.
As stated, phx.auth doesn’t have the features that I need, I am not comfortable modifying and maintaining authentication code that I didn’t write, and I don’t need help in this area. I understand that you’re trying to help but at a certain point it becomes a little frustrating when people keep taking this discussion off topic and ignoring what I have previously written.
I picked johantell’s response as the solution, because it was timely, combined a query, and used the with statement, but I want to thank everyone who responded. It is definitely more clear to me after this thread that the “when” and “with” statements are designed to improve the flow of the type of functions I was writing, where you have to do some checks on the incoming data before performing some task, and then provide some kind of error or null value if the checks are failed. Just in the couple of days since I posted this, I have been writing cleaner code because of this. Thanks again!
This is a great response. I would actually love to use pipes everywhere and am hoping my understanding of control flow & tuples get me to that point.
For this function:
def user_login(ident, password) do
user = find_user_by_name_or_email(ident)
if Bcrypt.verify_pass(password, user.hashed_password) do
create_user_token(user)
end
end
For this part, the issue is that the find function might return nil, and in that case user.hashed_password would fail (i.e.crash). Although I now see that I could write those bits as a separate two-headed function, like:
def hashed_pw_for_user(nil), do: nil
def hashed_pw_for_user(user), do: user.hashed_password
def user_login(ident, password) do
user = find_user_by_name_or_email(ident)
hash_pw = hashed_pw_for_user(user)
if Bcrypt.verify_pass(password, hash_pw) do
create_user_token(user)
end
end
Maybe there’s a good way to write that in a pipe format using tuples, I don’t know.
Yeah, there is a potentially funny problem here where one account’s name and email address matches another accounts email address and name. Each user would only be able to log in with one or the other but not both! But usernames and email addresses have different formats which both require validation, so in practice, not an issue.
I also personally prefer the cond do version than the with version.
That’s actually what inspired this post - I think cond is great and I was using it for pretty much everything, but I also knew there were other alternatives out there and that every function I write shouldn’t end in “true → (do a bunch of stuff)”.
What I’ve learned here that’s most useful is that it looks like “with” is designed for a specific situation, but one that comes up a lot, which is that your function has some checks it needs to perform before it does its work. So the more tests you have and the more work you have, the more the benefits of using “with” over “case”, “cond”, “if”, etc becomes clear. But sure, for small examples like these, and especially cases where you have to add “true ← check_on_something()” in a bunch of places, the benefits are debatable. Overall though I think it will have a big impact on my code writing.
I normally just use case and if for everything so something like this. I don’t think I’ve ever used cond, but I’m still learning
def user_login(ident, password) do
case find_user_by_name_or_email(ident) do
nil ->
{:error, "User not found"}
user ->
if Bcrypt.verify_pass(password, user.hashed_password) do
create_user_token(user)
else
{:error, "Invalid password"}
end
end
end
