From 9e11ddb1cd92c7bf728f95b879be2a040240743b Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Wed, 28 Feb 2024 11:42:07 -0600 Subject: [PATCH] Do not crash on disconnect messages in LV (#3795) This message is sent by the some of the broadcasters and it was resulting in a process crash (on a socket that will be disconnected anyways), but this triggered our logging alerts anyways. So we will simply ignore them globally to suppres the noise. --- .github/workflows/_elixir.yml | 4 +-- elixir/apps/web/lib/web.ex | 13 ++++++++ .../apps/web/test/support/acceptance_case.ex | 20 ++++++++----- .../web/test/web/acceptance/auth_test.exs | 30 +++++++++++++++++++ 4 files changed, 57 insertions(+), 10 deletions(-) diff --git a/.github/workflows/_elixir.yml b/.github/workflows/_elixir.yml index 992a544b6..01ff5a841 100644 --- a/.github/workflows/_elixir.yml +++ b/.github/workflows/_elixir.yml @@ -69,7 +69,7 @@ jobs: mix ecto.migrate - name: Run Tests env: - E2E_MAX_WAIT_SECONDS: 20 + E2E_DEFAULT_WAIT_SECONDS: 20 run: | mix test --warnings-as-errors - name: Test Report @@ -486,7 +486,7 @@ jobs: - name: Run Acceptance Tests env: MIX_TEST_PARTITION: ${{ matrix.MIX_TEST_PARTITION }} - E2E_MAX_WAIT_SECONDS: 5 + E2E_DEFAULT_WAIT_SECONDS: 5 run: | mix test --only acceptance:true \ --partitions=${{ env.MIX_TEST_PARTITIONS }} \ diff --git a/elixir/apps/web/lib/web.ex b/elixir/apps/web/lib/web.ex index 762de2c3c..1a184d0b9 100644 --- a/elixir/apps/web/lib/web.ex +++ b/elixir/apps/web/lib/web.ex @@ -57,6 +57,19 @@ defmodule Web do layout: Keyword.get(unquote(opts), :layout, {Web.Layouts, :app}) unquote(html_helpers()) + + # we ignore Swoosh messages that can crash LV process in dev/test mode + if Mix.env() in [:dev, :test] do + def handle_info({:email, %Swoosh.Email{}}, socket) do + {:noreply, socket} + end + end + + # ignore "disconnect" message that is broadcasted for some pages + # because of subscription for relay/gateway group events + def handle_info("disconnect", socket) do + {:noreply, socket} + end end end diff --git a/elixir/apps/web/test/support/acceptance_case.ex b/elixir/apps/web/test/support/acceptance_case.ex index c317ee74f..cc29b1367 100644 --- a/elixir/apps/web/test/support/acceptance_case.ex +++ b/elixir/apps/web/test/support/acceptance_case.ex @@ -116,7 +116,7 @@ defmodule Web.AcceptanceCase do Wallaby.QueryError ] -> time_spent = now - started_at - max_wait_seconds = fetch_max_wait_seconds!() + max_wait_seconds = fetch_default_wait_seconds!() if time_spent > :timer.seconds(max_wait_seconds) do reraise(e, __STACKTRACE__) @@ -130,33 +130,37 @@ defmodule Web.AcceptanceCase do end end - defp fetch_max_wait_seconds! do - if env = System.get_env("E2E_MAX_WAIT_SECONDS") do + defp fetch_default_wait_seconds! do + if env = System.get_env("E2E_DEFAULT_WAIT_SECONDS") do String.to_integer(env) else 2 end end - def wait_for(assertion_callback, started_at \\ nil) do + @doc """ + Allows to wait for an assertion to be true or to extend the wait timeout for acceptance + assertions when we know the process is going to take a while. + """ + def wait_for(assertion_callback, wait_seconds \\ nil, started_at \\ nil) do now = :erlang.monotonic_time(:milli_seconds) started_at = started_at || now try do assertion_callback.() rescue - e in [ExUnit.AssertionError] -> + e in [ExUnit.AssertionError, Wallaby.ExpectationNotMetError] -> time_spent = now - started_at - max_wait_seconds = fetch_max_wait_seconds!() + wait_seconds = wait_seconds || fetch_default_wait_seconds!() - if time_spent > :timer.seconds(max_wait_seconds) do + if time_spent > :timer.seconds(wait_seconds) do reraise(e, __STACKTRACE__) else floor(time_spent / 10) |> max(100) |> :timer.sleep() - wait_for(assertion_callback, started_at) + wait_for(assertion_callback, wait_seconds, started_at) end end end diff --git a/elixir/apps/web/test/web/acceptance/auth_test.exs b/elixir/apps/web/test/web/acceptance/auth_test.exs index 85e636dea..38abb9f77 100644 --- a/elixir/apps/web/test/web/acceptance/auth_test.exs +++ b/elixir/apps/web/test/web/acceptance/auth_test.exs @@ -69,6 +69,36 @@ defmodule Web.Acceptance.AuthTest do end end + feature "signs out browser session if token is revoked", %{session: session} do + account = Fixtures.Accounts.create_account() + provider = Fixtures.Auth.create_userpass_provider(account: account) + password = "Firezone1234" + + identity = + Fixtures.Auth.create_identity( + account: account, + provider: provider, + actor: [type: :account_admin_user], + provider_virtual_state: %{"password" => password, "password_confirmation" => password} + ) + + session + |> visit(~p"/#{account}") + |> Auth.authenticate(identity) + |> visit(~p"/#{account}/actors") + + {:ok, _tokens} = Domain.Tokens.delete_tokens_for(identity) + + wait_for( + fn -> + assert_el(session, Query.text("You must sign in to access this page.")) + end, + 10_000 + ) + + assert_path(session, ~p"/#{account}") + end + feature "does not allow regular user to navigate to admin routes", %{session: session} do account = Fixtures.Accounts.create_account() provider = Fixtures.Auth.create_userpass_provider(account: account)