Reorganise auth routes to make sure that plugs do not block OIDC/SAML when local auth is disabled (#1399)

And fix acceptance test assertion on the wrong broken behavior.

Closes: #1398
Ref:
https://firezone-users.slack.com/archives/C02PV412LGZ/p1674718310558799

apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex

@@ -51,28 +51,15 @@ defmodule FzHttpWeb.AuthController do
     end
   end
 
-  def callback(conn, %{"provider" => "saml"}) do
-    key = {idp, _} = get_session(conn, "samly_assertion_key")
-    assertion = %Samly.Assertion{} = Samly.State.get_assertion(conn, key)
-
-    with {:ok, user} <-
-           UserFromAuth.find_or_create(:saml, idp, %{"email" => assertion.subject.name}) do
-      do_sign_in(conn, user, %{provider: idp})
-    else
-      {:error, %{errors: [email: {"is invalid email address", _metadata}]}} ->
-        conn
-        |> put_flash(
-          :error,
-          "SAML provider did not return a valid email address in `name` assertion"
-        )
-        |> redirect(to: ~p"/")
-
-      other ->
-        other
-    end
+  # This can be called if the user attempts to visit one of the callback redirect URLs
+  # directly.
+  def callback(conn, params) do
+    conn
+    |> put_flash(:error, inspect(params) <> inspect(conn.assigns))
+    |> redirect(to: ~p"/")
   end
 
-  def callback(conn, %{"provider" => provider_id, "state" => state} = params)
+  def oidc_callback(conn, %{"provider" => provider_id, "state" => state} = params)
       when is_binary(provider_id) do
     token_params = Map.merge(params, PKCE.token_params(conn))
 
@@ -116,12 +103,30 @@ defmodule FzHttpWeb.AuthController do
     end
   end
 
-  # This can be called if the user attempts to visit one of the callback redirect URLs
-  # directly.
-  def callback(conn, params) do
-    conn
-    |> put_flash(:error, inspect(params) <> inspect(conn.assigns))
-    |> redirect(to: ~p"/")
+  def saml_callback(conn, _params) do
+    key = {idp, _} = get_session(conn, "samly_assertion_key")
+    assertion = %Samly.Assertion{} = Samly.State.get_assertion(conn, key)
+
+    with {:ok, user} <-
+           UserFromAuth.find_or_create(:saml, idp, %{"email" => assertion.subject.name}) do
+      do_sign_in(conn, user, %{provider: idp})
+    else
+      {:error, %{errors: [email: {"is invalid email address", _metadata}]}} ->
+        conn
+        |> put_flash(
+          :error,
+          "SAML provider did not return a valid email address in `name` assertion"
+        )
+        |> redirect(to: ~p"/")
+
+      {:error, reason} when is_binary(reason) ->
+        conn
+        |> put_flash(:error, reason)
+        |> redirect(to: ~p"/")
+
+      other ->
+        other
+    end
   end
 
   def delete(conn, _params) do

apps/fz_http/lib/fz_http_web/router.ex

@@ -65,27 +65,41 @@ defmodule FzHttpWeb.Router do
     post "/magic_link", AuthController, :magic_link
     get "/magic/:user_id/:token", AuthController, :magic_sign_in
 
-    get "/:provider", AuthController, :request
-    get "/:provider/callback", AuthController, :callback
-    post "/:provider/callback", AuthController, :callback
+    get "/identity", AuthController, :request
+    get "/identity/callback", AuthController, :callback
+    post "/identity/callback", AuthController, :callback
   end
 
-  # XXX: Those routes conflict with the ones above, we must change them in 1.0
   # OIDC auth routes
-  scope "/auth/oidc", FzHttpWeb do
-    pipe_through [
-      :browser,
-      :html_auth,
-      :require_unauthenticated
-    ]
+  scope "/auth", FzHttpWeb do
+    scope "/oidc" do
+      pipe_through [
+        :browser,
+        :require_unauthenticated
+      ]
 
-    get "/:provider/callback", AuthController, :callback, as: :auth_oidc
-    get "/:provider", AuthController, :redirect_oidc_auth_uri, as: :auth_oidc
+      get "/", AuthController, :request
+      get "/:provider/callback", AuthController, :oidc_callback
+      get "/:provider", AuthController, :redirect_oidc_auth_uri
+    end
   end
 
   # SAML auth routes
+  scope "/auth/saml", FzHttpWeb do
+    pipe_through [
+      :browser,
+      :require_unauthenticated
+    ]
+
+    get "/", AuthController, :request
+    get "/callback", AuthController, :saml_callback
+  end
+
   scope "/auth/saml" do
-    pipe_through :samly
+    pipe_through [
+      :samly,
+      :require_unauthenticated
+    ]
 
     forward "/", Samly.Router
   end

apps/fz_http/test/fz_http_web/acceptance/admin_test.exs

@@ -146,7 +146,7 @@ defmodule FzHttpWeb.Acceptance.AdminTest do
       end)
 
       # XXX: We need to show some kind of message when status changed
-      Process.sleep(100)
+      Process.sleep(200)
 
       assert updated_user = Repo.get(FzHttp.Users.User, user.id)
       assert updated_user.disabled_at
@@ -156,7 +156,7 @@ defmodule FzHttpWeb.Acceptance.AdminTest do
         |> toggle("toggle_disabled_at")
       end)
 
-      Process.sleep(100)
+      Process.sleep(200)
 
       assert updated_user = Repo.get(FzHttp.Users.User, user.id)
       refute updated_user.disabled_at
@@ -292,7 +292,7 @@ defmodule FzHttpWeb.Acceptance.AdminTest do
 
       # XXX: We need to show a confirmation dialog on delete,
       # and message once record was saved or deleted.
-      Process.sleep(100)
+      Process.sleep(200)
       assert is_nil(Repo.one(FzHttp.Rules.Rule))
     end
   end

apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs

@@ -127,7 +127,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do
       |> fill_in(Query.fillable_field("username"), with: "user1")
       |> fill_in(Query.fillable_field("password"), with: "user1pass")
       |> click(Query.button("Login"))
-      |> assert_el(Query.text("Local auth disabled"))
+      |> assert_el(Query.text("user not found and auto_create_users disabled"))
     end
   end