From aa7a43dc7030156f4e19bba487a152af4aca3895 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Wed, 1 Feb 2023 17:12:46 -0600 Subject: [PATCH] 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 --- .../controllers/auth_controller.ex | 57 ++++++++++--------- apps/fz_http/lib/fz_http_web/router.ex | 40 ++++++++----- .../fz_http_web/acceptance/admin_test.exs | 6 +- .../acceptance/authentication_test.exs | 2 +- 4 files changed, 62 insertions(+), 43 deletions(-) diff --git a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex index 249653433..7320d7296 100644 --- a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/router.ex b/apps/fz_http/lib/fz_http_web/router.ex index 519d105e8..e04ccbbff 100644 --- a/apps/fz_http/lib/fz_http_web/router.ex +++ b/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 diff --git a/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs b/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs index 50a2e55af..fb03607b2 100644 --- a/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs +++ b/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 diff --git a/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs b/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs index 0ccb2055b..054d7b8ba 100644 --- a/apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs +++ b/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