aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
772 stars 55 forks source link

User_return_to test breaks on phoenix master #96

Closed aaronrenner closed 3 years ago

aaronrenner commented 3 years ago

I'm getting the following test failure in user_auth_test.exs when running on phoenix master.

  1) test require_authenticated_user/2 stores the path to redirect to on GET (DefaultAppWeb.UserAuthTest)
     test/default_app_web/controllers/user_auth_test.exs:141
     Assertion with == failed
     code:  assert get_session(halted_conn, :user_return_to) == "/foo"
     left:  "/"
     right: "/foo"
     stacktrace:
       test/default_app_web/controllers/user_auth_test.exs:148: (test)

Here is the test in question:

    test "stores the path to redirect to on GET", %{conn: conn} do
      halted_conn =
        %{conn | request_path: "/foo", query_string: ""}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      assert get_session(halted_conn, :user_return_to) == "/foo"

      halted_conn =
        %{conn | request_path: "/foo", query_string: "bar=baz"}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      assert get_session(halted_conn, :user_return_to) == "/foo?bar=baz"

      halted_conn =
        %{conn | request_path: "/foo?bar", method: "POST"}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      refute get_session(halted_conn, :user_return_to)
    end

It appears this issue is caused by this recent commit to phoenix: https://github.com/phoenixframework/phoenix/commit/44a8d90ced173d6083579266e528cdb3281677c6. Phoenix's current_url/1 is now looking at script_name + path_info instead of request_path, which is causing the failure.

One option would be to update the test like this:

    test "stores the path to redirect to on GET", %{conn: conn} do
      halted_conn =
-       %{conn | request_path: "/foo", query_string: ""}
+       %{conn | script_name: ["foo"], query_string: ""}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      assert get_session(halted_conn, :user_return_to) == "/foo"

      halted_conn =
-         %{conn | request_path: "/foo", query_string: "bar=baz"}
+         %{conn | script_name: ["foo"], query_string: "bar=baz"}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      assert get_session(halted_conn, :user_return_to) == "/foo?bar=baz"

      halted_conn =
+       # I'm not sure how this should be updated
        %{conn | request_path: "/foo?bar", method: "POST"}
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

      assert halted_conn.halted
      refute get_session(halted_conn, :user_return_to)
    end

However, the problem with this approach is the test is now coupled to the underlying implementation of current_url/1 and isn't compatible across phoenix versions. I tried using Phoenix.ConnTest.get/3, but a Phoenix.Router.NoRouteError is raised because "/foo" isn't a valid path. @josevalim What approach would you suggest we take here? Is there a test helper I'm overlooking that will make this easier?

josevalim commented 3 years ago

Ah, good call. You can use build_conn(:get, "/this/is/a/path") which ships with Phoenix.ConnTest. WDYT?

aaronrenner commented 3 years ago

I actually tried that, but then I get the following error:

     ** (ArgumentError) session not fetched, call fetch_session/2

If I add fetch_session/1 to the pipeline,

      halted_conn =
        build_conn(:get, "/foo")
        |> fetch_session()
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

it then says:

     ** (ArgumentError) cannot fetch session without a configured session plug

I also tried to use bypass_through/3 since the endpoint has a session configuration in it and the :browser pipeline has a :fetch_session plug in it, .

      halted_conn =
        build_conn(:get, "/foo")
        |> bypass_through(DemoAppWeb.Router, [:browser])
        |> fetch_flash()
        |> UserAuth.require_authenticated_user([])

but I still get the same error

     ** (ArgumentError) cannot fetch session without a configured session plug

I'm sure there's some combination of these functions I need to call, but just haven't figured it out yet. :sweat_smile:

josevalim commented 3 years ago

You need to call the same functions in setup (set secret_key_base and plug_init_session): https://github.com/aaronrenner/phx_gen_auth/blob/master/priv/templates/phx.gen.auth/auth_test.exs#L13-L14 but then i am not sure if it is worth it. I think it is easier to just set path_info (prefer path_info instead of script_name).

aaronrenner commented 3 years ago

Fixed on phoenix master in https://github.com/phoenixframework/phoenix/pull/4077