From d135a8b8eb419177bbbc003934daf7cbd49bad5f Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Wed, 21 Feb 2024 16:31:11 -0500 Subject: [PATCH] Add sign-in success page for clients (#3714) Why: * On some clients, the web view that is opened to sign-in to Firezone is left open and ends up getting stuck on the Sign In page with the liveview loader on the top of the page also stuck and appearing as though it is waiting for another response. This commit adds a sign-in success page that is displayed upon successful sign-in and shows a message to the user that lets them know they can close the window if needed. If the client device is able to close the web view that was opened, then the page will either very briefly be shown or will not be visible at all due to how quickly the redirect happens. --- elixir/apps/web/lib/web/auth.ex | 8 +-- .../apps/web/lib/web/live/sign_in/success.ex | 57 +++++++++++++++++++ elixir/apps/web/lib/web/router.ex | 4 ++ elixir/apps/web/lib/web/sandbox.ex | 7 ++- .../web/test/support/acceptance_case/auth.ex | 4 ++ .../test/web/acceptance/auth/email_test.exs | 4 ++ .../acceptance/auth/openid_connect_test.exs | 6 +- .../web/acceptance/auth/userpass_test.exs | 4 ++ elixir/apps/web/test/web/auth_test.exs | 14 ++--- .../web/controllers/auth_controller_test.exs | 9 +-- .../web/test/web/live/sign_in/email_test.exs | 2 +- .../test/web/live/sign_in/success_test.exs | 37 ++++++++++++ 12 files changed, 130 insertions(+), 26 deletions(-) create mode 100644 elixir/apps/web/lib/web/live/sign_in/success.ex create mode 100644 elixir/apps/web/test/web/live/sign_in/success_test.exs diff --git a/elixir/apps/web/lib/web/auth.ex b/elixir/apps/web/lib/web/auth.ex index fb9bb2862..9a38a2eda 100644 --- a/elixir/apps/web/lib/web/auth.ex +++ b/elixir/apps/web/lib/web/auth.ex @@ -118,18 +118,12 @@ defmodule Web.Auth do fragment: encoded_fragment, state: state, actor_name: identity.actor.name, - account_slug: conn.assigns.account.slug, - account_name: conn.assigns.account.name, identity_provider_identifier: identity.provider_identifier } |> Enum.reject(&is_nil(elem(&1, 1))) - |> URI.encode_query() - - client_handler = - Domain.Config.fetch_env!(:web, :client_handler) Phoenix.Controller.redirect(conn, - external: "#{client_handler}handle_client_sign_in_callback?#{query}" + to: ~p"/#{conn.assigns.account.slug}/sign_in/success?#{query}" ) end diff --git a/elixir/apps/web/lib/web/live/sign_in/success.ex b/elixir/apps/web/lib/web/live/sign_in/success.ex new file mode 100644 index 000000000..b7fd4a111 --- /dev/null +++ b/elixir/apps/web/lib/web/live/sign_in/success.ex @@ -0,0 +1,57 @@ +defmodule Web.SignIn.Success do + use Web, {:live_view, layout: {Web.Layouts, :public}} + + def mount(params, _session, socket) do + if connected?(socket) do + Process.send_after(self(), :redirect_client, 100) + end + + query_params = + params + |> Map.take(~w[fragment state actor_name identity_provider_identifier]) + |> Map.put("account_slug", socket.assigns.account.slug) + |> Map.put("account_name", socket.assigns.account.name) + + socket = assign(socket, :params, query_params) + {:ok, socket} + end + + def render(assigns) do + ~H""" +
+
+ <.logo /> + +
+
+

+ + Sign in successful. + +

+

You may close this window.

+
+
+
+
+ """ + end + + def handle_info(:redirect_client, socket) do + {scheme, url} = + Domain.Config.fetch_env!(:web, :client_handler) + |> format_redirect_url() + + query = URI.encode_query(socket.assigns.params) + + {:noreply, redirect(socket, external: {scheme, "#{url}?#{query}"})} + end + + defp format_redirect_url(raw_client_handler) do + uri = URI.parse(raw_client_handler) + + maybe_host = if uri.host == "", do: "", else: "#{uri.host}:#{uri.port}/" + + {uri.scheme, "//#{maybe_host}handle_client_sign_in_callback"} + end +end diff --git a/elixir/apps/web/lib/web/router.ex b/elixir/apps/web/lib/web/router.ex index 44555a132..daf5e8076 100644 --- a/elixir/apps/web/lib/web/router.ex +++ b/elixir/apps/web/lib/web/router.ex @@ -83,6 +83,10 @@ defmodule Web.Router do scope "/:account_id_or_slug", Web do pipe_through [:browser, :account] + live_session :client_redirect, on_mount: [Web.Sandbox, {Web.Auth, :mount_account}] do + live "/sign_in/success", SignIn.Success + end + scope "/sign_in/providers/:provider_id" do # UserPass post "/verify_credentials", AuthController, :verify_credentials diff --git a/elixir/apps/web/lib/web/sandbox.ex b/elixir/apps/web/lib/web/sandbox.ex index f83261ca4..0a6ef9e6f 100644 --- a/elixir/apps/web/lib/web/sandbox.ex +++ b/elixir/apps/web/lib/web/sandbox.ex @@ -31,11 +31,16 @@ defmodule Web.Sandbox do end def allow_live_ecto_sandbox(socket) do + user_agent = Phoenix.LiveView.get_connect_info(socket, :user_agent) + if Phoenix.LiveView.connected?(socket) do - user_agent = Phoenix.LiveView.get_connect_info(socket, :user_agent) Sandbox.allow(Phoenix.Ecto.SQL.Sandbox, user_agent) end + with %{owner: test_pid} <- Phoenix.Ecto.SQL.Sandbox.decode_metadata(user_agent) do + Process.put(:last_caller_pid, test_pid) + end + socket end end diff --git a/elixir/apps/web/test/support/acceptance_case/auth.ex b/elixir/apps/web/test/support/acceptance_case/auth.ex index 267ba4ed0..4de6ba572 100644 --- a/elixir/apps/web/test/support/acceptance_case/auth.ex +++ b/elixir/apps/web/test/support/acceptance_case/auth.ex @@ -112,6 +112,10 @@ defmodule Web.AcceptanceCase.Auth do Plug.Conn.send_resp(conn, 200, "Client redirected") end) + Bypass.stub(bypass, "GET", "/favicon.ico", fn conn -> + Plug.Conn.send_resp(conn, 404, "") + end) + bypass end diff --git a/elixir/apps/web/test/web/acceptance/auth/email_test.exs b/elixir/apps/web/test/web/acceptance/auth/email_test.exs index d72328589..1890fb6c2 100644 --- a/elixir/apps/web/test/web/acceptance/auth/email_test.exs +++ b/elixir/apps/web/test/web/acceptance/auth/email_test.exs @@ -57,6 +57,8 @@ defmodule Web.Acceptance.SignIn.EmailTest do session |> email_login_flow(account, identity.provider_identifier, redirect_params) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") @@ -110,6 +112,8 @@ defmodule Web.Acceptance.SignIn.EmailTest do # And then to a client session |> email_login_flow(account, identity.provider_identifier, redirect_params) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") diff --git a/elixir/apps/web/test/web/acceptance/auth/openid_connect_test.exs b/elixir/apps/web/test/web/acceptance/auth/openid_connect_test.exs index 44c9042c8..b7ba404bf 100644 --- a/elixir/apps/web/test/web/acceptance/auth/openid_connect_test.exs +++ b/elixir/apps/web/test/web/acceptance/auth/openid_connect_test.exs @@ -172,7 +172,7 @@ defmodule Web.Acceptance.Auth.OpenIDConnectTest do provider_identifier: entity_id ) - # Sign In as an portal user + # Sign In as a portal user session |> visit(~p"/#{account}") |> assert_el(Query.text("Sign in to #{account.name}")) @@ -187,6 +187,8 @@ defmodule Web.Acceptance.Auth.OpenIDConnectTest do |> visit(~p"/#{account}?#{redirect_params}") |> assert_el(Query.text("Sign in to #{account.name}")) |> click(Query.link("Sign in with Vault")) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") @@ -240,6 +242,8 @@ defmodule Web.Acceptance.Auth.OpenIDConnectTest do |> assert_el(Query.text("Sign in to #{account.name}")) |> click(Query.link("Sign in with Vault")) |> Vault.userpass_flow(oidc_login, oidc_password) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") diff --git a/elixir/apps/web/test/web/acceptance/auth/userpass_test.exs b/elixir/apps/web/test/web/acceptance/auth/userpass_test.exs index 66a5e083f..019358c1b 100644 --- a/elixir/apps/web/test/web/acceptance/auth/userpass_test.exs +++ b/elixir/apps/web/test/web/acceptance/auth/userpass_test.exs @@ -167,6 +167,8 @@ defmodule Web.Acceptance.Auth.UserPassTest do session |> password_login_flow(account, identity.provider_identifier, password, redirect_params) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") @@ -230,6 +232,8 @@ defmodule Web.Acceptance.Auth.UserPassTest do # And then to a client session |> password_login_flow(account, identity.provider_identifier, password, redirect_params) + |> assert_el(Query.text("Sign in successful")) + |> assert_path(~p"/#{account}/sign_in/success") |> assert_el(Query.text("Client redirected")) |> assert_path(~p"/handle_client_sign_in_callback") diff --git a/elixir/apps/web/test/web/auth_test.exs b/elixir/apps/web/test/web/auth_test.exs index 41382a1d5..7c60c0ec0 100644 --- a/elixir/apps/web/test/web/auth_test.exs +++ b/elixir/apps/web/test/web/auth_test.exs @@ -251,7 +251,7 @@ defmodule Web.AuthTest do assert conn.assigns.flash["error"] == "Please use a client application to access Firezone." end - test "redirects regular users to the deep link for client contexts", %{ + test "redirects regular users to the sign in success page for client contexts", %{ conn: conn, context: context, account: account, @@ -270,11 +270,9 @@ defmodule Web.AuthTest do |> signed_in(provider, identity, context, encoded_fragment, redirect_params) |> redirected_to() - assert redirected_to =~ "firezone-fd0020211111://handle_client_sign_in_callback" + assert redirected_to =~ "#{account.slug}/sign_in/success" assert redirected_to =~ "fragment=#{URI.encode_www_form(encoded_fragment)}" assert redirected_to =~ "state=STATE" - assert redirected_to =~ "account_slug=#{account.slug}" - assert redirected_to =~ "account_name=#{URI.encode_www_form(account.name)}" assert redirected_to =~ "identity_provider_identifier=#{URI.encode_www_form(identity.provider_identifier)}" @@ -299,11 +297,9 @@ defmodule Web.AuthTest do |> signed_in(provider, identity, context, encoded_fragment, redirect_params) |> redirected_to() - assert redirected_to =~ "firezone-fd0020211111://handle_client_sign_in_callback" + assert redirected_to =~ "#{account.slug}/sign_in/success" assert redirected_to =~ "fragment=#{URI.encode_www_form(encoded_fragment)}" assert redirected_to =~ "state=STATE" - assert redirected_to =~ "account_slug=#{account.slug}" - assert redirected_to =~ "account_name=#{URI.encode_www_form(account.name)}" assert redirected_to =~ "identity_provider_identifier=#{URI.encode_www_form(identity.provider_identifier)}" @@ -780,11 +776,9 @@ defmodule Web.AuthTest do assert conn.halted assert redirected_to = redirected_to(conn) - assert redirected_to =~ "firezone-fd0020211111://handle_client_sign_in_callback" + assert redirected_to =~ "#{account.slug}/sign_in/success" assert redirected_to =~ "fragment=#{URI.encode_www_form(encoded_fragment)}" assert redirected_to =~ "state=STATE" - assert redirected_to =~ "account_slug=#{account.slug}" - assert redirected_to =~ "account_name=#{URI.encode_www_form(account.name)}" assert redirected_to =~ "identity_provider_identifier=#{URI.encode_www_form(admin_identity.provider_identifier)}" diff --git a/elixir/apps/web/test/web/controllers/auth_controller_test.exs b/elixir/apps/web/test/web/controllers/auth_controller_test.exs index 04ca53ec2..e7a9a090c 100644 --- a/elixir/apps/web/test/web/controllers/auth_controller_test.exs +++ b/elixir/apps/web/test/web/controllers/auth_controller_test.exs @@ -272,8 +272,7 @@ defmodule Web.AuthControllerTest do assert redirected_to = redirected_to(conn) assert redirected_to_uri = URI.parse(redirected_to) - assert redirected_to_uri.scheme == "firezone-fd0020211111" - assert redirected_to_uri.host == "handle_client_sign_in_callback" + assert redirected_to_uri.path == "/#{account.slug}/sign_in/success" assert %{ "identity_provider_identifier" => identity_provider_identifier, @@ -626,8 +625,7 @@ defmodule Web.AuthControllerTest do refute Map.has_key?(conn.cookies, "fz_auth_state_#{provider.id}") assert redirected_to = conn |> redirected_to() |> URI.parse() - assert redirected_to.scheme == "firezone-fd0020211111" - assert redirected_to.host == "handle_client_sign_in_callback" + assert redirected_to.path == "/#{account.slug}/sign_in/success" assert query_params = URI.decode_query(redirected_to.query) assert not is_nil(query_params["fragment"]) @@ -982,8 +980,7 @@ defmodule Web.AuthControllerTest do }) assert redirected_to = conn |> redirected_to() |> URI.parse() - assert redirected_to.scheme == "firezone-fd0020211111" - assert redirected_to.host == "handle_client_sign_in_callback" + assert redirected_to.path == "/#{account.slug}/sign_in/success" assert query_params = URI.decode_query(redirected_to.query) assert not is_nil(query_params["fragment"]) diff --git a/elixir/apps/web/test/web/live/sign_in/email_test.exs b/elixir/apps/web/test/web/live/sign_in/email_test.exs index 0b8748730..6f7a71ec9 100644 --- a/elixir/apps/web/test/web/live/sign_in/email_test.exs +++ b/elixir/apps/web/test/web/live/sign_in/email_test.exs @@ -104,7 +104,7 @@ defmodule Web.SignIn.EmailTest do }) |> submit_form(conn) - assert redirected_to(conn, 302) =~ "firezone-fd0020211111://handle_client_sign_in_callback" + assert redirected_to(conn, 302) =~ "/#{account.slug}/sign_in/success" refute conn.assigns.flash["error"] end diff --git a/elixir/apps/web/test/web/live/sign_in/success_test.exs b/elixir/apps/web/test/web/live/sign_in/success_test.exs new file mode 100644 index 000000000..2f14d04e4 --- /dev/null +++ b/elixir/apps/web/test/web/live/sign_in/success_test.exs @@ -0,0 +1,37 @@ +defmodule Web.SignIn.SuccessTest do + use Web.ConnCase, async: true + + setup do + account = Fixtures.Accounts.create_account() + + %{account: account} + end + + test "redirects to deep link URL", %{ + account: account, + conn: conn + } do + query_params = %{ + "actor_name" => "actor_name", + "fragment" => "fragment", + "identity_provider_identifier" => "identifier", + "state" => "state" + } + + {:ok, lv, html} = + conn + |> live(~p"/#{account}/sign_in/success?#{query_params}") + + assert html =~ "success" + assert html =~ "close this window" + + expected_query_params = + query_params + |> Map.put("account_name", account.name) + |> Map.put("account_slug", account.slug) + + {path, _flash} = assert_redirect(lv, 500) + uri = URI.parse(path) + assert URI.decode_query(uri.query) == expected_query_params + end +end