no route found for GET /users/settings/update_email

[dbi1]

Hi,

If I try to change the email address on the "Settings" page to the same email address as it is now (or use an invalid password), I get an error message ("Oops, something went wrong! Please check the errors below."), which is good.

The page returning the error message has the following URL: http://somedomain/users/settings/update_email

However, there are two errors one can cause while on this URL:

a) When reloading the page while being logged in: "no route found" error (see log below) b) When reloading the page while not being logged in (i.e. by using the back button): Plug.CSRFProtection.InvalidCSRFTokenError

I think in case a) the page should simply reload without errors, and in case b) redirect to the login page?

Have only tested this in dev environment, not production, though.

My setup is a simple default Phoenix app (generated using the generator), with phx_gen_auth injected (following the instructions).

Here is the log extract:

[info] GET /users/settings/update_email
[debug] ** (Phoenix.Router.NoRouteError) no route found for GET /users/settings/update_email (MyAppWeb.Router)
    (my_app 0.1.0) lib/phoenix/router.ex:402: MyAppWeb.Router.call/2
    (my_app 0.1.0) lib/my_app_web/endpoint.ex:1: MyAppWeb.Endpoint.plug_builder_call/2
    (my_app 0.1.0) lib/plug/debugger.ex:132: MyAppWeb.Endpoint."call (overridable 3)"/2
    (my_app 0.1.0) lib/my_app_web/endpoint.ex:1: MyAppWeb.Endpoint.call/2
    (phoenix 1.5.6) lib/phoenix/endpoint/cowboy2_handler.ex:65: Phoenix.Endpoint.Cowboy2Handler.init/4
    (cowboy 2.8.0) /home/abc/my_app/deps/cowboy/src/cowboy_handler.erl:37: :cowboy_handler.execute/2
    (cowboy 2.8.0) /home/abc/my_app/deps/cowboy/src/cowboy_stream_h.erl:300: :cowboy_stream_h.execute/3
    (cowboy 2.8.0) /home/abc/my_app/deps/cowboy/src/cowboy_stream_h.erl:291: :cowboy_stream_h.request_process/3
    (stdlib 3.13.2) proc_lib.erl:226: :proc_lib.init_p_do_apply/3

Thanks!

[dbi1]

Similar behavior for changing the password on the Settings page:

Page reload (while being logged in) brings up "no route found for GET /users/settings/update_password (MyAppWeb.Router)"

Page reload (while being logged out) also brings up the "no route found" error, rather than the Plug.CSRFProtection.InvalidCSRFTokenError we saw above in scenario b).

[josevalim]

Yes, this is unfortunately common for many applications that follow the "REST" URL schema.

I think one solution here is to have both POST to /users/settings and we can use a hidden field to say which operation we want to perform (email or password update), and then we collapse the actions in the controller into a single one with pattern matching.

What do you think @aaronrenner?

[aaronrenner]

That sounds reasonable. I'm happy to work up a PR for that.

[dbi1]

Thank you @josevalim and @aaronrenner!

[aaronrenner]

@josevalim Here's the diff I am thinking of for this one.

https://github.com/aaronrenner/phx_gen_auth_playground/commit/c5e72abd6b38deeaeb315f87b1fbb2e12e37bf9d

A few things to note:

1. There are now two update function heads that pattern match on an "action" parameter to determine which form is being submitted. Unfortunately this makes the function head too long and the formatter requires it be wrapped to multiple lines. Another approach could be only doing the pattern matching in the function head and extracting the param values in the body of the function.

   def update(conn, %{"action" => "update_password"} = params) do
     user = conn.assigns.current_user
     password = Map.get(params, "current_password")
     user_params = Map.get(params, "user", %{})
   
    #...

   I like this second approach, but it breaks the consistency with the rest of the actions in these controllers that are doing destructuring in the function head. WDYT?

2. The confirm_email action had to be moved so the update actions could be grouped together. That's why it's showing up in the diff.
3. I kept separate describe blocks in the controller tests so it's easier to see which tests belong to which form. This is different than the rest of the controller tests, so I wanted to call it out.
4. I added a hidden "action" input to the template for each form. I don't believe the form helper is buying us much over a hard-coded <input hidden="action" value="update_email"> tag, but I went with the form helper for consistency.

Also, let me know if you see anything else you think should be changed. Thanks!

[josevalim]

@aaronrenner I think everything is great! Regarding 1, I think multi-line function head is OK, if you'd rather not to, then I would do this:

def update(conn, %{"action" => "update_password"} = params) do
  %{"current_password" => password, "user" => user_params} = params

I actually prefer that in multi-clauses because it makes it clear which parameter is being used for branching.

[aaronrenner]

I like that approach too! I'll update the generators accordingly.