From 430b32324aebf566c2c7f1aa7313f5ec306267fa Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Wed, 15 Jan 2025 12:03:12 -0500 Subject: [PATCH] fix(portal): Update IDP sync error email threshold (#7757) Why: * An IdP sync can fail for different reasons and because of this we previously put a threshold on when to send the first 'IdP sync failed' email, which was set at 10 failed sync attempts. One thing that was accidentally overlooked was that on one specific failure type (i.e. 401 - Unauthorized) the Firezone sync was automatically disabled and not tried from that point forward. Unfortunately, that meant an email did not get sent out because the threshold was not met. This PR resolves that by making sure the 401 error will send out an email immediately, while keeping the 10 failed sync threshold for all other errors. Closes: #7725 --- .../adapter/openid_connect/directory_sync.ex | 39 ++++++++++--------- .../mailer/sync_email/sync_error.html.eex | 2 +- .../mailer/sync_email/sync_error.text.eex | 2 +- elixir/apps/domain/mix.exs | 2 +- .../jobs/sync_directory_test.exs | 16 +++++++- .../jumpcloud/jobs/sync_directory_test.exs | 7 +++- .../jobs/sync_directory_test.exs | 19 ++++++++- .../okta/jobs/sync_directory_test.exs | 7 +++- .../domain/mailer/sync_error_email_test.exs | 2 +- elixir/mix.lock | 2 +- 10 files changed, 69 insertions(+), 29 deletions(-) diff --git a/elixir/apps/domain/lib/domain/auth/adapter/openid_connect/directory_sync.ex b/elixir/apps/domain/lib/domain/auth/adapter/openid_connect/directory_sync.ex index 16edc395f..ea9c2cf07 100644 --- a/elixir/apps/domain/lib/domain/auth/adapter/openid_connect/directory_sync.ex +++ b/elixir/apps/domain/lib/domain/auth/adapter/openid_connect/directory_sync.ex @@ -225,7 +225,7 @@ defmodule Domain.Auth.Adapter.OpenIDConnect.DirectorySync do Auth.Provider.Changeset.sync_failed(provider, user_message) |> Domain.Repo.update!() - |> send_sync_error_email() + |> send_rate_limited_sync_error_email() |> log_sync_error(log_message) :error @@ -384,27 +384,30 @@ defmodule Domain.Auth.Adapter.OpenIDConnect.DirectorySync do end) end + defp send_rate_limited_sync_error_email(provider) do + if notification_criteria_met?(provider) do + send_sync_error_email(provider) + else + Logger.debug("Sync error email already sent today") + provider + end + end + defp send_sync_error_email(provider) do provider = Repo.preload(provider, :account) - if notification_criteria_met?(provider) do - Domain.Actors.all_admins_for_account!(provider.account, preload: :identities) - |> Enum.flat_map(fn actor -> - Enum.map(actor.identities, &Domain.Auth.get_identity_email(&1)) - end) - |> Enum.uniq() - |> Enum.each(fn email -> - Domain.Mailer.SyncEmail.sync_error_email(provider, email) - |> Domain.Mailer.deliver() - end) + Domain.Actors.all_admins_for_account!(provider.account, preload: :identities) + |> Enum.flat_map(fn actor -> + Enum.map(actor.identities, &Domain.Auth.get_identity_email(&1)) + end) + |> Enum.uniq() + |> Enum.each(fn email -> + Domain.Mailer.SyncEmail.sync_error_email(provider, email) + |> Domain.Mailer.deliver() + end) - Auth.Provider.Changeset.sync_error_emailed(provider) - |> Domain.Repo.update!() - else - Logger.debug("Sync error email already sent today") - - provider - end + Auth.Provider.Changeset.sync_error_emailed(provider) + |> Domain.Repo.update!() end defp notification_criteria_met?(provider) do diff --git a/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.html.eex b/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.html.eex index a6085469e..6f281d639 100644 --- a/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.html.eex +++ b/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.html.eex @@ -66,7 +66,7 @@ <%= @provider.name %> Sync Error!

- <%= @provider.name %> has failed to sync <%= @provider.last_syncs_failed %> times. + <%= @provider.name %> has failed to sync <%= @provider.last_syncs_failed %> time(s).

‍

diff --git a/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.text.eex b/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.text.eex index 18d80e66e..ee6cfaaa4 100644 --- a/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.text.eex +++ b/elixir/apps/domain/lib/domain/mailer/sync_email/sync_error.text.eex @@ -1,6 +1,6 @@ Identity Provider Sync Error! -An Identity Provider in your Firezone Account has failed to sync <%= @provider.last_syncs_failed%> times. +An Identity Provider in your Firezone Account has failed to sync <%= @provider.last_syncs_failed%> time(s). The following is the last sync error message: <%= @provider.last_sync_error %> diff --git a/elixir/apps/domain/mix.exs b/elixir/apps/domain/mix.exs index b52edde51..336fae6d6 100644 --- a/elixir/apps/domain/mix.exs +++ b/elixir/apps/domain/mix.exs @@ -59,7 +59,7 @@ defmodule Domain.MixProject do {:openid_connect, github: "firezone/openid_connect", ref: "e4d9dca8ae43c765c00a7d3dfa12d6f24f5b3418"}, {:argon2_elixir, "~> 4.0"}, - {:workos, git: "https://github.com/firezone/workos-elixir.git", branch: "main"}, + {:workos, "~> 1.1"}, # Erlang Clustering {:libcluster, "~> 3.3"}, diff --git a/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs index d03216fdc..d53f62945 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs/sync_directory_test.exs @@ -747,10 +747,12 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.Jobs.SyncDirectoryTest do cancel_bypass_expectations_check(bypass) end - test "disables the sync on 401 response code", %{provider: provider} do + test "disables the sync on 401 response code", %{account: account, provider: provider} do bypass = Bypass.open() GoogleWorkspaceDirectory.override_endpoint_url("http://localhost:#{bypass.port}/") GoogleWorkspaceDirectory.mock_token_endpoint(bypass) + actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account) + _identity = Fixtures.Auth.create_identity(account: account, actor: actor) error_message = "Admin SDK API has not been used in project XXXX before or it is disabled. " <> @@ -813,6 +815,11 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.Jobs.SyncDirectoryTest do assert updated_provider.last_sync_error == error_message assert updated_provider.sync_disabled_at + assert_email_sent(fn email -> + assert email.subject == "Firezone Identity Provider Sync Error" + assert email.text_body =~ "failed to sync 1 time(s)" + end) + cancel_bypass_expectations_check(bypass) end @@ -882,9 +889,14 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.Jobs.SyncDirectoryTest do assert_email_sent(fn email -> assert email.subject == "Firezone Identity Provider Sync Error" - assert email.text_body =~ "failed to sync 10 times" + assert email.text_body =~ "failed to sync 10 time(s)" end) + {:ok, pid} = Task.Supervisor.start_link() + assert execute(%{task_supervisor: pid}) == :ok + + refute_email_sent() + cancel_bypass_expectations_check(bypass) end end diff --git a/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs index d5368021d..08c47ec28 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/jumpcloud/jobs/sync_directory_test.exs @@ -589,9 +589,14 @@ defmodule Domain.Auth.Adapters.JumpCloud.Jobs.SyncDirectoryTest do assert_email_sent(fn email -> assert email.subject == "Firezone Identity Provider Sync Error" - assert email.text_body =~ "failed to sync 10 times" + assert email.text_body =~ "failed to sync 10 time(s)" end) + {:ok, pid} = Task.Supervisor.start_link() + assert execute(%{task_supervisor: pid}) == :ok + + refute_email_sent() + cancel_bypass_expectations_check(bypass) end end diff --git a/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs index 2fa71eedc..2bbb5544a 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/microsoft_entra/jobs/sync_directory_test.exs @@ -407,9 +407,14 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do refute_received {:expire_flow, _flow_id, _client_id, _resource_id} end - test "stops the sync retires on 401 error on the provider", %{provider: provider} do + test "stops the sync retries on 401 error on the provider", %{ + account: account, + provider: provider + } do bypass = Bypass.open() MicrosoftEntraDirectory.override_endpoint_url("http://localhost:#{bypass.port}/") + actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account) + _identity = Fixtures.Auth.create_identity(account: account, actor: actor) error_message = "Lifetime validation failed, the token is expired." @@ -441,6 +446,11 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do refute updated_provider.last_synced_at assert updated_provider.last_syncs_failed == 1 assert updated_provider.last_sync_error == error_message + + assert_email_sent(fn email -> + assert email.subject == "Firezone Identity Provider Sync Error" + assert email.text_body =~ "failed to sync 1 time(s)" + end) end test "persists the sync error on the provider", %{provider: provider} do @@ -507,9 +517,14 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do assert_email_sent(fn email -> assert email.subject == "Firezone Identity Provider Sync Error" - assert email.text_body =~ "failed to sync 10 times" + assert email.text_body =~ "failed to sync 10 time(s)" end) + {:ok, pid} = Task.Supervisor.start_link() + assert execute(%{task_supervisor: pid}) == :ok + + refute_email_sent() + cancel_bypass_expectations_check(bypass) end end diff --git a/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs b/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs index fc771cac7..73123e061 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/okta/jobs/sync_directory_test.exs @@ -819,9 +819,14 @@ defmodule Domain.Auth.Adapters.Okta.Jobs.SyncDirectoryTest do assert_email_sent(fn email -> assert email.subject == "Firezone Identity Provider Sync Error" - assert email.text_body =~ "failed to sync 10 times" + assert email.text_body =~ "failed to sync 10 time(s)" end) + {:ok, pid} = Task.Supervisor.start_link() + assert execute(%{task_supervisor: pid}) == :ok + + refute_email_sent() + cancel_bypass_expectations_check(bypass) end end diff --git a/elixir/apps/domain/test/domain/mailer/sync_error_email_test.exs b/elixir/apps/domain/test/domain/mailer/sync_error_email_test.exs index 0cbad69fe..2ab05f7c9 100644 --- a/elixir/apps/domain/test/domain/mailer/sync_error_email_test.exs +++ b/elixir/apps/domain/test/domain/mailer/sync_error_email_test.exs @@ -25,7 +25,7 @@ defmodule Domain.Mailer.SyncErrorEmailTest do email_body = sync_error_email(provider, admin_email) - assert email_body.text_body =~ "2 times" + assert email_body.text_body =~ "2 time(s)" assert email_body.text_body =~ expected_msg end end diff --git a/elixir/mix.lock b/elixir/mix.lock index 4e211d657..03a038073 100644 --- a/elixir/mix.lock +++ b/elixir/mix.lock @@ -109,7 +109,7 @@ "web_driver_client": {:hex, :web_driver_client, "0.2.0", "63b76cd9eb3b0716ec5467a0f8bead73d3d9612e63f7560d21357f03ad86e31a", [:mix], [{:hackney, "~> 1.6", [hex: :hackney, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:tesla, "~> 1.3", [hex: :tesla, repo: "hexpm", optional: false]}], "hexpm", "83cc6092bc3e74926d1c8455f0ce927d5d1d36707b74d9a65e38c084aab0350f"}, "websock": {:hex, :websock, "0.5.3", "2f69a6ebe810328555b6fe5c831a851f485e303a7c8ce6c5f675abeb20ebdadc", [:mix], [], "hexpm", "6105453d7fac22c712ad66fab1d45abdf049868f253cf719b625151460b8b453"}, "websock_adapter": {:hex, :websock_adapter, "0.5.7", "65fa74042530064ef0570b75b43f5c49bb8b235d6515671b3d250022cb8a1f9e", [:mix], [{:bandit, ">= 0.6.0", [hex: :bandit, repo: "hexpm", optional: true]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 2.6", [hex: :plug_cowboy, repo: "hexpm", optional: true]}, {:websock, "~> 0.5", [hex: :websock, repo: "hexpm", optional: false]}], "hexpm", "d0f478ee64deddfec64b800673fd6e0c8888b079d9f3444dd96d2a98383bdbd1"}, - "workos": {:git, "https://github.com/firezone/workos-elixir.git", "6d6995e2a765656a6834577837433393fac9b35b", [branch: "main"]}, + "workos": {:hex, :workos, "1.1.0", "b64de032a254ebfb86b90b274e5d8865042afda6389e3c4d2d889ee0f4bd7735", [:mix], [{:hackney, "~> 1.9", [hex: :hackney, repo: "hexpm", optional: false]}, {:jason, "~> 1.4.1", [hex: :jason, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:tesla, "~> 1.4", [hex: :tesla, repo: "hexpm", optional: false]}], "hexpm", "88034983748821353cc28660278e0fd1886378bd4888ea77651889c0d126f3d5"}, "yamerl": {:hex, :yamerl, "0.10.0", "4ff81fee2f1f6a46f1700c0d880b24d193ddb74bd14ef42cb0bcf46e81ef2f8e", [:rebar3], [], "hexpm", "346adb2963f1051dc837a2364e4acf6eb7d80097c0f53cbdc3046ec8ec4b4e6e"}, "yaml_elixir": {:hex, :yaml_elixir, "2.11.0", "9e9ccd134e861c66b84825a3542a1c22ba33f338d82c07282f4f1f52d847bd50", [:mix], [{:yamerl, "~> 0.10", [hex: :yamerl, repo: "hexpm", optional: false]}], "hexpm", "53cc28357ee7eb952344995787f4bb8cc3cecbf189652236e9b163e8ce1bc242"}, "ymlr": {:hex, :ymlr, "5.1.3", "a8061add5a378e20272a31905be70209a5680fdbe0ad51f40cb1af4bdd0a010b", [:mix], [], "hexpm", "8663444fa85101a117887c170204d4c5a2182567e5f84767f0071cf15f2efb1e"},