diff --git a/elixir/apps/domain/lib/domain/actors/group/sync.ex b/elixir/apps/domain/lib/domain/actors/group/sync.ex index 7c59fcbbb..4201dcbc2 100644 --- a/elixir/apps/domain/lib/domain/actors/group/sync.ex +++ b/elixir/apps/domain/lib/domain/actors/group/sync.ex @@ -3,6 +3,7 @@ defmodule Domain.Actors.Group.Sync do alias Domain.Auth alias Domain.Actors alias Domain.Actors.Group + require Logger def sync_provider_groups(%Auth.Provider{} = provider, attrs_list) do attrs_by_provider_identifier = @@ -14,6 +15,7 @@ defmodule Domain.Actors.Group.Sync do with {:ok, groups} <- all_provider_groups(provider), {:ok, {upsert, delete}} <- plan_groups_update(groups, provider_identifiers), + :ok <- deletion_circuit_breaker(groups, delete, provider), {:ok, deleted} <- delete_groups(provider, delete), {:ok, upserted} <- upsert_groups(provider, attrs_by_provider_identifier, upsert) do group_ids_by_provider_identifier = @@ -57,6 +59,47 @@ defmodule Domain.Actors.Group.Sync do {:ok, {upsert, delete}} end + # Used to make sure we don't accidentally mass delete groups + # due to an error in the Identity Provider API (e.g. Google Admin SDK API) + defp deletion_circuit_breaker([], _groups_to_delete, _provider) do + # If there are no groups then there can't be anything to delete + :ok + end + + defp deletion_circuit_breaker(_groups, [], _provider) do + :ok + end + + defp deletion_circuit_breaker(groups, groups_to_delete, provider) do + groups_length = length(groups) + deletion_length = length(groups_to_delete) + + delete_percentage = (deletion_length / groups_length * 100) |> round() + + cond do + groups_length > 40 and delete_percentage >= 25 -> + Logger.error("Refusing to mass delete groups", + groups_length: groups_length, + deletion_length: deletion_length, + provider_id: provider.id + ) + + {:error, "Sync deletion of groups too large"} + + groups_length <= 40 and delete_percentage >= 50 -> + Logger.error("Refusing to mass delete groups", + groups_length: groups_length, + deletion_length: deletion_length, + provider_id: provider.id + ) + + {:error, "Sync deletion of groups too large"} + + true -> + :ok + end + end + defp delete_groups(provider, provider_identifiers_to_delete) do Group.Query.not_deleted() |> Group.Query.by_account_id(provider.account_id) diff --git a/elixir/apps/domain/lib/domain/auth/adapters/google_workspace/api_client.ex b/elixir/apps/domain/lib/domain/auth/adapters/google_workspace/api_client.ex index f1ad777a8..343effd35 100644 --- a/elixir/apps/domain/lib/domain/auth/adapters/google_workspace/api_client.ex +++ b/elixir/apps/domain/lib/domain/auth/adapters/google_workspace/api_client.ex @@ -94,7 +94,17 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.APIClient do }) ) - list_all(uri, api_token, "users") + case list_all(uri, api_token, "users") do + {:ok, []} -> + Logger.warning("Google Workspace API returned 0 users") + {:ok, []} + + {:ok, [_head | _tail] = list} -> + {:ok, list} + + {:error, reason} -> + {:error, reason} + end end def list_groups(api_token) do @@ -111,7 +121,17 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.APIClient do }) ) - list_all(uri, api_token, "groups") + case list_all(uri, api_token, "groups") do + {:ok, []} -> + Logger.warning("Google Workspace API returned 0 groups") + {:ok, []} + + {:ok, [_head | _tail] = list} -> + {:ok, list} + + {:error, reason} -> + {:error, reason} + end end # Note: this functions does not return root (`/`) org unit diff --git a/elixir/apps/domain/lib/domain/auth/identity/sync.ex b/elixir/apps/domain/lib/domain/auth/identity/sync.ex index 67713d560..e33043150 100644 --- a/elixir/apps/domain/lib/domain/auth/identity/sync.ex +++ b/elixir/apps/domain/lib/domain/auth/identity/sync.ex @@ -1,6 +1,7 @@ defmodule Domain.Auth.Identity.Sync do alias Domain.Repo alias Domain.Auth.{Identity, Provider} + require Logger def sync_provider_identities(%Provider{} = provider, attrs_list) do attrs_by_provider_identifier = @@ -13,6 +14,7 @@ defmodule Domain.Auth.Identity.Sync do with {:ok, identities} <- all_provider_identities(provider), {:ok, {insert, update, delete}} <- plan_identities_update(identities, provider_identifiers), + :ok <- deletion_circuit_breaker(identities, delete, provider), {:ok, deleted} <- delete_identities(provider, delete), {:ok, inserted} <- insert_identities(provider, attrs_by_provider_identifier, insert), @@ -72,6 +74,47 @@ defmodule Domain.Auth.Identity.Sync do {:ok, {insert, update, delete}} end + # Used to make sure we don't accidentally mass delete identities + # due to an error in the Identity Provider API (e.g. Google Admin SDK API) + defp deletion_circuit_breaker([], _identities_to_delete, _provider) do + # If there are no identities then there can't be anything to delete + :ok + end + + defp deletion_circuit_breaker(_identities, [], _provider) do + :ok + end + + defp deletion_circuit_breaker(identities, identities_to_delete, provider) do + identities_length = length(identities) + deletion_length = length(identities_to_delete) + + delete_percentage = (deletion_length / identities_length * 100) |> round() + + cond do + identities_length > 40 and delete_percentage >= 25 -> + Logger.error("Refusing to mass delete identities", + identities_length: identities_length, + deletion_length: deletion_length, + provider_id: provider.id + ) + + {:error, "Sync deletion of identities too large"} + + identities_length <= 40 and delete_percentage >= 50 -> + Logger.error("Refusing to mass delete identities", + identities_length: identities_length, + deletion_length: deletion_length, + provider_id: provider.id + ) + + {:error, "Sync deletion of identities too large"} + + true -> + :ok + end + end + defp delete_identities(provider, provider_identifiers_to_delete) do provider_identifiers_to_delete = Enum.uniq(provider_identifiers_to_delete) diff --git a/elixir/apps/domain/test/domain/actors_test.exs b/elixir/apps/domain/test/domain/actors_test.exs index 10ba76c38..565180377 100644 --- a/elixir/apps/domain/test/domain/actors_test.exs +++ b/elixir/apps/domain/test/domain/actors_test.exs @@ -845,30 +845,58 @@ defmodule Domain.ActorsTest do provider_identifier: "OU:OU_ID1" ) + group3 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID2" + ) + + group4 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID3" + ) + + group5 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID4" + ) + :ok = subscribe_to_membership_updates_for_actor(actor) :ok = Domain.Policies.subscribe_to_events_for_actor(actor) :ok = Domain.Policies.subscribe_to_events_for_actor_group(group1) :ok = Domain.Policies.subscribe_to_events_for_actor_group(group2) + :ok = Domain.Policies.subscribe_to_events_for_actor_group(group3) + :ok = Domain.Policies.subscribe_to_events_for_actor_group(group4) + :ok = Domain.Policies.subscribe_to_events_for_actor_group(group5) :ok = Domain.Flows.subscribe_to_flow_expiration_events(group1_flow) - attrs_list = [] + attrs_list = [ + %{"name" => "Group:Infrastructure", "provider_identifier" => "G:GROUP_ID2"}, + %{"name" => "Group:Security", "provider_identifier" => "G:GROUP_ID3"}, + %{"name" => "Group:Finance", "provider_identifier" => "G:GROUP_ID4"} + ] assert {:ok, %{ - groups: [_group1, _group2], - plan: {[], delete}, + groups: [_group1, _group2, _group3, _group4, _group5], + plan: {_upsert, delete}, deleted: [deleted_group1, deleted_group2], - upserted: [], + upserted: [_upserted_group3, _upserted_group4, _upserted_group5], group_ids_by_provider_identifier: group_ids_by_provider_identifier }} = sync_provider_groups(provider, attrs_list) assert Enum.all?(["G:GROUP_ID1", "OU:OU_ID1"], &(&1 in delete)) assert deleted_group1.provider_identifier in ["G:GROUP_ID1", "OU:OU_ID1"] assert deleted_group2.provider_identifier in ["G:GROUP_ID1", "OU:OU_ID1"] - assert Repo.aggregate(Actors.Group, :count) == 2 - assert Repo.aggregate(Actors.Group.Query.not_deleted(), :count) == 0 + assert Repo.aggregate(Actors.Group, :count) == 5 + assert Repo.aggregate(Actors.Group.Query.not_deleted(), :count) == 3 - assert Enum.empty?(group_ids_by_provider_identifier) + assert Map.keys(group_ids_by_provider_identifier) |> length() == 3 actor_id = actor.id @@ -884,6 +912,54 @@ defmodule Domain.ActorsTest do refute_receive {:reject_access, _policy_id, ^group2_id, _resource_id} end + test "circuit breaker prevents mass deletion of groups", %{ + account: account, + provider: provider + } do + _group1 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID1" + ) + + _group2 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "OU:OU_ID1" + ) + + _group3 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID2" + ) + + _group4 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID3" + ) + + _group5 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_ID4" + ) + + attrs_list = [] + + assert {:error, "Sync deletion of groups too large"} == + sync_provider_groups(provider, attrs_list) + + assert Repo.aggregate(Actors.Group, :count) == 5 + assert Repo.aggregate(Actors.Group.Query.not_deleted(), :count) == 5 + end + test "ignores groups that are not synced from the provider", %{ account: account, provider: provider 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 c4c2c26e2..356bbd3c8 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 @@ -267,6 +267,18 @@ defmodule Domain.Auth.Adapters.JumpCloud.Jobs.SyncDirectoryTest do "raw_attributes" => %{} } + group_finance = %{ + "id" => "GROUP_FINANCE_ID", + "object" => "directory_group", + "idp_id" => "finance", + "directory_id" => "dir_123", + "organization_id" => "org_123", + "name" => "Finance", + "created_at" => "2021-10-28 15:21:50.640958", + "updated_at" => "2021-12-14 12:15:45.531847", + "raw_attributes" => %{} + } + users = [ %{ "id" => "workos_user_jdoe_id", @@ -372,6 +384,13 @@ defmodule Domain.Auth.Adapters.JumpCloud.Jobs.SyncDirectoryTest do provider_identifier: "G:GROUP_ALL_ID" ) + _group_finance = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_FINANCE_ID" + ) + deleted_group = Fixtures.Actors.create_group( account: account, @@ -410,7 +429,12 @@ defmodule Domain.Auth.Adapters.JumpCloud.Jobs.SyncDirectoryTest do WorkOSDirectory.override_base_url("http://localhost:#{bypass.port}") WorkOSDirectory.mock_list_directories_endpoint(bypass) WorkOSDirectory.mock_list_users_endpoint(bypass, users) - WorkOSDirectory.mock_list_groups_endpoint(bypass, [group_all, group_engineering]) + + WorkOSDirectory.mock_list_groups_endpoint(bypass, [ + group_all, + group_engineering, + group_finance + ]) {:ok, pid} = Task.Supervisor.start_link() assert execute(%{task_supervisor: pid}) == :ok 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 a34412eea..a8bee3668 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 @@ -356,7 +356,8 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do groups = [ %{"id" => "GROUP_ALL_ID", "displayName" => "All"}, - %{"id" => "GROUP_ENGINEERING_ID", "displayName" => "Engineering"} + %{"id" => "GROUP_ENGINEERING_ID", "displayName" => "Engineering"}, + %{"id" => "GROUP_FINANCE_ID", "displayName" => "Finance"} ] one_member = [ @@ -433,6 +434,13 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do provider_identifier: "G:GROUP_ALL_ID" ) + _finance_group = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_FINANCE_ID" + ) + deleted_group = Fixtures.Actors.create_group( account: account, @@ -494,6 +502,13 @@ defmodule Domain.Auth.Adapters.MicrosoftEntra.Jobs.SyncDirectoryTest do Jason.encode!(%{"value" => one_member}) ) + MicrosoftEntraDirectory.mock_group_members_list_endpoint( + bypass, + "GROUP_FINANCE_ID", + 200, + Jason.encode!(%{"value" => []}) + ) + {:ok, pid} = Task.Supervisor.start_link() assert execute(%{task_supervisor: pid}) == :ok 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 ab74c2f72..ed6a73d61 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 @@ -501,6 +501,40 @@ defmodule Domain.Auth.Adapters.Okta.Jobs.SyncDirectoryTest do "href" => "http://localhost:#{bypass.port}/api/v1/groups/00gezqfqxwa2ohLhp5d7/apps" } } + }, + %{ + "id" => "GROUP_FINANCE_ID", + "created" => "2024-02-07T05:30:49.000Z", + "lastUpdated" => "2024-02-07T05:30:49.000Z", + "lastMembershipUpdated" => "2024-02-07T05:32:23.000Z", + "objectClass" => [ + "okta:user_group" + ], + "type" => "OKTA_GROUP", + "profile" => %{ + "name" => "Finance", + "description" => "Finance Dept" + }, + "_links" => %{ + "logo" => [ + %{ + "name" => "medium", + "href" => "http://localhost/md/image.png", + "type" => "image/png" + }, + %{ + "name" => "large", + "href" => "http://localhost/lg/image.png", + "type" => "image/png" + } + ], + "users" => %{ + "href" => "http://localhost:#{bypass.port}/api/v1/groups/00gezqfqxwa2ohLmp5d7/users" + }, + "apps" => %{ + "href" => "http://localhost:#{bypass.port}/api/v1/groups/00gezqfqxwa2ohLmp5d7/apps" + } + } } ] @@ -642,6 +676,13 @@ defmodule Domain.Auth.Adapters.Okta.Jobs.SyncDirectoryTest do provider_identifier: "G:GROUP_ENGINEERING_ID" ) + _group2 = + Fixtures.Actors.create_group( + account: account, + provider: provider, + provider_identifier: "G:GROUP_FINANCE_ID" + ) + deleted_group = Fixtures.Actors.create_group( account: account, @@ -694,6 +735,13 @@ defmodule Domain.Auth.Adapters.Okta.Jobs.SyncDirectoryTest do Jason.encode!(one_member) ) + OktaDirectory.mock_group_members_list_endpoint( + bypass, + "GROUP_FINANCE_ID", + 200, + Jason.encode!([]) + ) + {:ok, pid} = Task.Supervisor.start_link() assert execute(%{task_supervisor: pid}) == :ok diff --git a/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs b/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs index 1951b3460..2770b1152 100644 --- a/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs +++ b/elixir/apps/domain/test/domain/auth/adapters/openid_connect_test.exs @@ -318,7 +318,8 @@ defmodule Domain.Auth.Adapters.OpenIDConnectTest do Fixtures.Auth.create_identity( account: account, provider: provider, - provider_identifier: sub + provider_identifier: sub, + email: Fixtures.Auth.email() ) identity = diff --git a/elixir/apps/domain/test/domain/auth_test.exs b/elixir/apps/domain/test/domain/auth_test.exs index 2d2443c81..6e528fd33 100644 --- a/elixir/apps/domain/test/domain/auth_test.exs +++ b/elixir/apps/domain/test/domain/auth_test.exs @@ -1839,7 +1839,7 @@ defmodule Domain.AuthTest do end test "deletes removed identities", %{account: account, provider: provider} do - provider_identifiers = ["USER_ID1", "USER_ID2"] + provider_identifiers = ["USER_ID1", "USER_ID2", "USER_ID3", "USER_ID4", "USER_ID5"] deleted_identity_actor = Fixtures.Actors.create_actor(account: account) @@ -1872,33 +1872,67 @@ defmodule Domain.AuthTest do token_id: deleted_identity_token.id ) - Fixtures.Auth.create_identity( - account: account, - provider: provider, - provider_identifier: Enum.at(provider_identifiers, 1) - ) + for n <- 1..4 do + Fixtures.Auth.create_identity( + account: account, + provider: provider, + provider_identifier: Enum.at(provider_identifiers, n) + ) + end :ok = Phoenix.PubSub.subscribe(Domain.PubSub, "sessions:#{deleted_identity_token.id}") :ok = Domain.Flows.subscribe_to_flow_expiration_events(deleted_identity_flow) - attrs_list = [] + attrs_list = [ + %{ + "actor" => %{ + "name" => "Joe Smith", + "type" => "account_user" + }, + "provider_identifier" => "USER_ID3" + }, + %{ + "actor" => %{ + "name" => "Jennie Smith", + "type" => "account_user" + }, + "provider_identifier" => "USER_ID4" + }, + %{ + "actor" => %{ + "name" => "Jane Doe", + "type" => "account_admin_user" + }, + "provider_identifier" => "USER_ID5" + } + ] assert {:ok, %{ - identities: [_identity1, _identity2], - plan: {[], [], delete}, + identities: [_id1, _id2, _id3, _id4, _id5], + plan: {[], ["USER_ID5", "USER_ID4", "USER_ID3"], delete}, deleted: [deleted_identity1, deleted_identity2], inserted: [], actor_ids_by_provider_identifier: actor_ids_by_provider_identifier }} = sync_provider_identities(provider, attrs_list) - assert Enum.all?(provider_identifiers, &(&1 in delete)) + assert Enum.take(provider_identifiers, 2) + |> Enum.all?(&(&1 in delete)) + assert deleted_identity1.provider_identifier in delete assert deleted_identity2.provider_identifier in delete - assert Repo.aggregate(Auth.Identity, :count) == 9 - assert Repo.aggregate(Auth.Identity.Query.not_deleted(), :count) == 7 - assert Enum.empty?(actor_ids_by_provider_identifier) + assert Auth.Identity.Query.all() + |> Auth.Identity.Query.by_provider_id(provider.id) + |> Repo.aggregate(:count) == 5 + + assert Auth.Identity.Query.not_deleted() + |> Auth.Identity.Query.by_provider_id(provider.id) + |> Repo.aggregate(:count) == 3 + + assert actor_ids_by_provider_identifier + |> Map.keys() + |> length() == 3 # Signs out users which identity has been deleted topic = "sessions:#{deleted_identity_token.id}" @@ -1909,6 +1943,34 @@ defmodule Domain.AuthTest do assert_receive {:expire_flow, ^flow_id, _client_id, _resource_id} end + test "circuit breaker prevents mass deletions of identities", %{ + account: account, + provider: provider + } do + provider_identifiers = ["USER_ID1", "USER_ID2", "USER_ID3", "USER_ID4", "USER_ID5"] + + for n <- 0..4 do + Fixtures.Auth.create_identity( + account: account, + provider: provider, + provider_identifier: Enum.at(provider_identifiers, n) + ) + end + + attrs_list = [] + + assert {:error, "Sync deletion of identities too large"} = + sync_provider_identities(provider, attrs_list) + + assert Auth.Identity.Query.all() + |> Auth.Identity.Query.by_provider_id(provider.id) + |> Repo.aggregate(:count) == 5 + + assert Auth.Identity.Query.not_deleted() + |> Auth.Identity.Query.by_provider_id(provider.id) + |> Repo.aggregate(:count) == 5 + end + test "ignores identities that are not synced from the provider", %{ account: account, provider: provider