Which strategy do you prefer for dealing with using current_user in your controllers?

After reading Programming Phoenix 1.4, I learned a 2nd potential way to deal with current_user in controllers and I was curious which strategy do you prefer?

1. Using an action function in each controller

The basic idea is every single controller that needs to reference current_user has the below action function defined in the controller.

  def action(conn, _) do
    apply(__MODULE__, action_name(conn), [conn, conn.params, conn.assigns.current_user])
  end

And then in every single controller action, instead of def new(conn, _params) you would do def new(conn, _params, _current_user). Basically you need to reference the current_user in the call signature of every action in that controller.

This is the strategy they use in the book, but the book’s app only has 1 controller that uses it, so it doesn’t really go into what you would do if you have many controllers wanting to do this.

2. Situationally extracting the current_user from assigns on demand

Another strategy I’ve seen out in the wild, specifically the changelog code base, has been to extract (is this called deconstruct?) the current_user when you need it.

def foo(conn = %{assigns: %{current_user: current_user}}, params) do
 # ...
end

It ends up looking like that, and doesn’t require any additional functions or configuration. In the cases where you don’t care about the current_user you just go about business as usual with def foo(conn, params).

I think I prefer this method only because it avoids having to define the same action function in every controller where I want to use current_user, and also avoids having to explicitly use _current_user in every action that doesn’t care about the current_user.

Am I missing any benefits on what makes the first strategy better in the long run, even in an application that has multiple (but not all) controllers wanting to potentially access current_user?

Also, is there another strategy that you prefer? If so, what it is and why did you choose it?

1 Like
  1. For this you could do something in web.ex so that you wouldn’t have to define it every time.

  2. deconstruct = pattern matching. 2 seems more idiomatic to me, but I could be wrong.

1 Like

I personally wouldn’t do 1, because it would cause all the controller functions to look different than the docs. So, if someone new started working on it, they’d get confused.

I wouldn’t pattern match the user in the function header either. I try to limit pattern matching in function headers to determining which clause of a function should be called. Otherwise, I do matching in the body. Though, I’d probably prefer conn.assigns.user in this case.

2 Likes

I usually have a Plug that puts the current_user id into the conn within an authenticated router pipeline. Then in an accounts context, have a Accounts.get_user like function that accepts a Plug.Conn struct to pull it out of the database (then you can chain it with preloads as you need it for your page). Call this inside a controller action, or before all the actions in the controller with another plug function. The plug function would only load the user and preload associations, so inside the action you’d still have to pluck it out of the conn for ease if you wanted to.

I’ve used the action function to redefine it for the controller to add a 3rd argument (the user), and it’s fine, but ya, takes new developers by surprise to see a 3rd argument. As long as your team knows about it I don’t see an issue with it, but I think I’d rather have a line of setup in the controller action itself. The controller shouldn’t be so large that this is too tedious anyway.

1 Like

:wave:

I usually create a module consisting of helpers and current_user(conn | socket) with put_user(conn | socket) is one of them

def current_user(%Plug.Conn{assigns: assigns}) do
  case assigns do
    %{current_user: user} -> user
    _other -> nil
  end
end

def current_user(%Phoenix.Socket{assigns: assigns}) do
  case assigns do
    %{current_user: user} -> user
    _other -> nil
  end
end

then I import it in the web.ex macros. Then in the app I just do current_user(conn) to get the current user and put_user(conn) to save it.

def update(conn, %{"id" => resource_id, "resource" => resource_params}) do
  resource = ResourceContext.get_resource!(resource_id, current_user(conn)) # <-- almost every function out of controllers get the user passed in to for authorization checks
  case ResourceContext.update_resource(resource, resource_params, current_user(conn)) do
    {:ok, _resource} -> # ...
    {:error, changeset} -> # ...
  end
end

I avoid using conn.assigns[anything] since it’s hard (for me) to refactor.

2 Likes

I create an Auth plug that puts a current_user into the assigns. It’s assigned either a %User{} struct if someone is logged in or nil if not. That way, every template in the app can access @current_user and any controller that needs to can use that assigns.

FWIW, I got this technique from the Pragmatic Bookshelf Programming Phoenix book as well.

Yeah I do that too. That is phase 1 and accounted for in all cases.

Phase 2 (this post) is more around how to reach into assigns and get the current_user in a controller action so you can do something with it in the controller.

1 Like

Gotcha. I definitely prefer option #2. I just don’t see the benefit of getting current_user as a separate function parameter vs getting it out of the conn.

In the relatively large Phoenix app I wrote for my old startup, I did something somewhat similar, where I was adding a parameter to all routes for locale. E.g. instead of /articles/3 it would be /en/articles/3 or /jp/articles/3, etc.

While the solution worked, I found it very annoying to need to add another parameter to each path helper created by any generator. I still use this pattern when I need first-class support for locales and different URLs by language, but it’s made me hesitate before adding parameters globally like this unless there’s an overwhelmingly clear benefit.

I do a mixture of #1 and #2

def action(conn, _) do
    apply(__MODULE__, action_name(conn), [conn, conn.params, conn.assigns])
  end

and

def index(conn, _, %{current_user: user, tenant: tenant}) do
1 Like

Nice, thanks for sharing. This is a neat approach. It’s almost like you get the best of both worlds (you don’t pollute your function pattern match and when you don’t need the current_user you just avoid calling current_user(conn).

3 questions for you:

  1. In your example I see you referenced current_user(conn) twice. Do you ever find yourself doing something like user = current_user(conn) to avoid having to pattern match the current_user function multiple times?

  2. In that helpers module, where do you usually that in your project’s file system? Something like lib/hello_web/helpers/current_user.ex?

  3. Do you think in the case of extracting out the current_user in the function’s parameters, it may help the “glance factor” so that if you do a skim of the controller, it’s easier to see which actions depend on the current user? It seems like it would be hard to see which actions use the current_user with your approach, unless you look carefully.

Third question is interesting to me because there is a trade off. As a few people mentioned in this thread already, complicating the action’s arguments has its downsides (doesn’t match the docs, looks kind of noisy especially if it results in having to mix format the function head across 2 lines).

I used to, but then stopped since it’s probably a negligible performance hit. I do extract relevant info before loops like in templates

<% %User{id: me_id} = current_user(@conn) %>
<%= for user <- users do %>
  <%= user.name %>
  <%= if user.id == me_id do %>(me)<% end %>
<% end %>

In that helpers module, where do you usually that in your project’s file system? Something like lib/hello_web/helpers/current_user.ex ?

Right now I have lib/hello_web/helpers/{conn,socket,changeset}.ex, and I have current_user/1 and current_user!/1 both in conn.ex and socket.ex.

Do you think in the case of extracting out the current_user in the function’s parameters, it may help the “glance factor” so that if you do a skim of the controller, it’s easier to see which actions depend on the current user? It seems like it would be hard to see which actions use the current_user with your approach, unless you look carefully.

I agree. Haven’t had much problems with not knowing from the start if a function uses current user or not … I might be misunderstanding, but I guess I tend to solve common “current user” problems with plugs like

defmodule SomeController do
  # ...
  plug EnsureRole, ["admin"] when action in [...] # checks if the page can be shown to a user
  # ...

Can you maybe provide an example where knowing that an action uses current user is important?

Ah, I didn’t anticipate you doing this for templates too. I know you said you’re not a fan of assigns in general, which I guess means in your auth plug you don’t even |> assign(:current_user, user) to make it available as @current_user in your templates?

I guess it doesn’t really matter too much. In a settings controller I have, I do the same thing as you with a controller level plug at the top which explains what’s happening, ie. plug :authenticate_user.

That really is enough to say “hey, every controller action deals with a user”.

Although I do have one example in a checkout controller that doesn’t require the user to be signed in, but it will use information from the current_user. But in theory, if your most of your controller functions are very short, it shouldn’t be a problem to figure out what’s using current_user and where.