"Let’s be clear. What we’re doing here is controversial", testing authentication in a controller, Programming Phoenix. Can someone explain in greater detail the risk here?

Phoenix Forum Questions / Help
, ,
1

In Programming Phoenix a way to simplify tests that require authentication is to add cond in auth.ex such that a test can set the current_user and bypass setting a user_id and then doing a search resulting in setting the current_user there. I understand the purpose of doing this but I’m not sure of the risks involved.

Here is the code in question:


​ 	​def​ call(conn, _opts) ​do​
​ 	  user_id = get_session(conn, ​:user_id​)
​ 	
​ 	  ​cond​ ​do​

        # how risky is having this here? <-----------------------------
​ 	    conn.assigns[​:current_user​] ->
​ 	      conn
​ 	
​ 	    user = user_id && Rumbl.Accounts.get_user(user_id) ->
​ 	      assign(conn, ​:current_user​, user)
​ 	
​ 	    true ->
​ 	      assign(conn, ​:current_user​, nil)
​ 	  ​end​
​ 	​end​

My naive thinking is that somehow a current_user is left in the session. To logout one would configure_session(conn, drop: true). Is there a gap that allows an unauthorized user to access the system?

I’d love to learn what edge cases exist.

Thank you

2

The only reason I can see that being risky is you have no idea what current user is assigned to. It could be a completely incorrect struct, and your app would have bugs.

Using conn.assigns doesn’t actually set anything persistent in the session. In this example, only user ID is present in the session, which is generally considered safe. conn.assigns is a place to store state that can be used throughout the plug pipeline. An example of this is Phoenix template rendering using the assigns to render @variables

2 Likes
3

I haven’t really spent time trying to break things, but the general concern is adding production code that is only really intended for test use. Maybe you are and maybe you aren’t opening an avenue for exploits. I wonder what would happen if someone took a valid path and added ?current_user=1 to it. :thinking:

2 Likes
4

Correct! The code has no security implications nor anything. It is just that some are really against adding test only code to lib/.

3 Likes
5

This function is pretty simple and could be done a few ways, so use imagination for this next question:

Is there more or less pushback to using a Mix.env check around the definition of get_user that checks the assigns only in test mode? That would give the benefit that we’d want for testing, but it would not get compiled into the final code.

Edit: Clarifying that I would not expect to see code like that in a book because it’s more cumbersome than beneficial for learning purposes.

1 Like
6

:thinking: Can you show the code (psuedocode) of what you mean? I’m not sure I follow.

I would think that if there were a general envionrment guard in :test/:dev it would prevent auth from being tested in the :test environment in general. As far as I can tell, adding it in a cond leaves a door open whereas using the env may limit testing. Please correct me if I’m wrong.

7

Sure, here’s an example you can run in iex -S mix session:

defmodule Test do
  def call() do
    %{user: %{fake: true, id: 0}, id: 1}
    |> get_user()
    |> IO.inspect()
  end
  
  if Mix.env() == :test do
    def get_user(%{user: u}), do: u
  end
  
  def get_user(%{id: id}) do
    %{fetched: true, id: id}
  end
end

Test.call()

# If started with MIX_ENV=test, then %{fake: true, id: 0}
# If started with MIX_ENV=dev, then %{fetched: true, id: 1}

In your example, it might look something like:

if Mix.env() == :test do
  def get_user(id, conn = %{assigns: %{user: _}}), do: conn
end

def get_user(id, conn) do
  cond do
    user = id && Accounts.get_user(id) ->
      assign(conn, :user, user)
      
    true ->
      assign(conn, :user, nil)
  end
end

I’m torn on this type of approach, which is why I asked for thoughts here. I see a very pragmatic use for this approach, because sometimes it’s just really convenient to have this for testing. But it’s also important to not abuse it.

2 Likes
8

I think it has the same downsides. The code you will be testing is ultimately not the same as the code in production and you still have lines in lib/ that are not production related. Although it does add a guarantee that the condition cannot be invoked in prod (which is also the case in the original version but there it is an implicit guarantee).

4 Likes
9

I’m constantly catching up… when I first saw your code I thought “oh, I didn’t think of that. nice!”, then reading jose’s thoughts I can see his points.

Learning a lot here. Thank you everyone!

10

I occasionally do this, though it’s not my first choice (more on that below). When I opt for this approach it’s because what you said (not compromising prod with some test-only backdoors), and also to communicate to the reader that this particular backdoor is required only for tests.

However, in this particular case I’d do something along the lines of

conn = get(authenticate(login, pass), path_under_test)
# assert

The authors mention this option, but they also reason that this would
quickly become expensive, because most tests will require a logged-in user.

This statement feels like an early generalization. With rounds set to 1 as suggested in pbkdf2 docs, and Ecto sandbox used, the overhead of this step shouldn’t be too large, at least not in smaller projects. Thus, I’d start with authentication via request, and move to some other version if this approach is proven to introduce significant overhead. The change should boil down to changing the implementation of the authenticate function, so it should be straightforward and localized.

The benefit of actually making a request is that the tests are using the public API, instead of introducing some backdoors. The tests are IMO more straightforward to understand, and they work closer to the way the system is used in production.

A few months ago I started mentoring team in one digital agency, and we introduced this approach to testing. Our general strategy is to use context functions in the arrange phase of the AAA pattern. Developers extract arrange part into private helper function(s) in the same case module. This extraction is first and foremost done to improve the clarity of tests. We aim to keep the arrange phase short and intention revealing without being bothered about mechanical details. If, over time, we need some functions in multiple test cases, we move them into a test/support helper module.

This approach has so far worked well for us, and the team was able to implement a couple of smaller projects without needing to resort to test-only modifications of the lib code. As a result, both the test and the lib are clearer and straight to the point.

If in the future we see that some arrange helpers are causing our tests to be slow, we’ll consider optimizations and we might introduce backdoors where needed, but we’ll do this when there’s actual need, and we’ll do it on a case by case basis, which should keep as much of our tests as possible based on external API.

4 Likes