From ab35a5ea762e0c5a9862e3657abcf73b1c73aada Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Wed, 27 Mar 2024 15:20:13 -0400 Subject: [PATCH] fix(portal): Fix bug in actor edit page preventing updates (#4347) Why: * A bug was present in the actor edit page that prevented updating an actor due to managed and synced groups being sent as part of the form submission. Along with that, if a user manually removed the managed group(s) from the form submission, the actor being edited would be removed from the managed group, which should not be allowed. * There was also another small bug which prevent an admin actor from being updated at all if they were the only admin in the account. --- elixir/apps/domain/lib/domain/actors.ex | 17 +++++++++-- .../lib/domain/actors/membership/query.ex | 12 ++++---- elixir/apps/web/lib/web/live/actors/edit.ex | 5 +--- .../web/live/actors/service_accounts/new.ex | 5 +--- .../apps/web/lib/web/live/actors/users/new.ex | 5 +--- .../web/test/web/live/actors/edit_test.exs | 28 +++++++++++++++++-- 6 files changed, 48 insertions(+), 24 deletions(-) diff --git a/elixir/apps/domain/lib/domain/actors.ex b/elixir/apps/domain/lib/domain/actors.ex index bd632bf99..d8cfe5273 100644 --- a/elixir/apps/domain/lib/domain/actors.ex +++ b/elixir/apps/domain/lib/domain/actors.ex @@ -55,6 +55,16 @@ defmodule Domain.Actors do |> Repo.preload(preload) end + def all_editable_groups!(%Auth.Subject{} = subject, opts \\ []) do + {preload, _opts} = Keyword.pop(opts, :preload, []) + + Group.Query.not_deleted() + |> Group.Query.editable() + |> Authorizer.for_subject(subject) + |> Repo.all() + |> Repo.preload(preload) + end + def peek_group_actors(groups, limit, %Auth.Subject{} = subject) do with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_actors_permission()) do ids = groups |> Enum.map(& &1.id) |> Enum.uniq() @@ -416,7 +426,7 @@ defmodule Domain.Actors do |> Authorizer.for_subject(subject) |> Repo.fetch_and_update(Actor.Query, with: fn actor -> - actor = maybe_preload_not_synced_memberships(actor, attrs) + actor = maybe_preload_editable_memberships(actor, attrs) synced_groups = list_readonly_groups(attrs) changeset = Actor.Changeset.update(actor, attrs, synced_groups, subject) @@ -424,6 +434,7 @@ defmodule Domain.Actors do changeset.data.type != :account_admin_user -> changeset Map.get(changeset.changes, :type) == :account_admin_user -> changeset other_enabled_admins_exist?(actor) -> changeset + is_nil(Map.get(changeset.changes, :type)) -> changeset true -> :cant_remove_admin_type end end, @@ -432,11 +443,11 @@ defmodule Domain.Actors do end end - defp maybe_preload_not_synced_memberships(%Actor{} = actor, attrs) do + defp maybe_preload_editable_memberships(%Actor{} = actor, attrs) do if Map.has_key?(attrs, "memberships") || Map.has_key?(attrs, :memberships) do memberships = Membership.Query.by_actor_id(actor.id) - |> Membership.Query.by_not_synced_group() + |> Membership.Query.only_editable_groups() |> Membership.Query.lock() |> Repo.all() diff --git a/elixir/apps/domain/lib/domain/actors/membership/query.ex b/elixir/apps/domain/lib/domain/actors/membership/query.ex index 0ce37c067..47a117985 100644 --- a/elixir/apps/domain/lib/domain/actors/membership/query.ex +++ b/elixir/apps/domain/lib/domain/actors/membership/query.ex @@ -6,6 +6,12 @@ defmodule Domain.Actors.Membership.Query do from(memberships in Membership, as: :memberships) end + def only_editable_groups(queryable \\ all()) do + queryable + |> with_joined_groups() + |> where([groups: groups], is_nil(groups.provider_id) and groups.type == :static) + end + def by_actor_id(queryable \\ all(), actor_id) do where(queryable, [memberships: memberships], memberships.actor_id == ^actor_id) end @@ -42,12 +48,6 @@ defmodule Domain.Actors.Membership.Query do |> where([groups: groups], groups.provider_id == ^provider_id) end - def by_not_synced_group(queryable \\ all()) do - queryable - |> with_joined_groups() - |> where([groups: groups], is_nil(groups.provider_id)) - end - def count_actors_by_group_id(queryable \\ all()) do queryable |> group_by([memberships: memberships], memberships.group_id) diff --git a/elixir/apps/web/lib/web/live/actors/edit.ex b/elixir/apps/web/lib/web/live/actors/edit.ex index cd60b7a80..bab5fad44 100644 --- a/elixir/apps/web/lib/web/live/actors/edit.ex +++ b/elixir/apps/web/lib/web/live/actors/edit.ex @@ -13,10 +13,7 @@ defmodule Web.Actors.Edit do ) do # TODO: unify this and dropdowns for filters groups = - Actors.all_groups!(socket.assigns.subject, - preload: [:provider], - filter: [editable?: true] - ) + Actors.all_editable_groups!(socket.assigns.subject, preload: [:provider]) changeset = Actors.change_actor(actor) diff --git a/elixir/apps/web/lib/web/live/actors/service_accounts/new.ex b/elixir/apps/web/lib/web/live/actors/service_accounts/new.ex index 1948a469d..810517f72 100644 --- a/elixir/apps/web/lib/web/live/actors/service_accounts/new.ex +++ b/elixir/apps/web/lib/web/live/actors/service_accounts/new.ex @@ -5,10 +5,7 @@ defmodule Web.Actors.ServiceAccounts.New do def mount(_params, _session, socket) do groups = - Actors.all_groups!(socket.assigns.subject, - preload: :provider, - filter: [editable?: true] - ) + Actors.all_editable_groups!(socket.assigns.subject, preload: :provider) changeset = Actors.new_actor(%{type: :service_account}) diff --git a/elixir/apps/web/lib/web/live/actors/users/new.ex b/elixir/apps/web/lib/web/live/actors/users/new.ex index 87c3a9e35..727f732fb 100644 --- a/elixir/apps/web/lib/web/live/actors/users/new.ex +++ b/elixir/apps/web/lib/web/live/actors/users/new.ex @@ -5,10 +5,7 @@ defmodule Web.Actors.Users.New do def mount(_params, _session, socket) do groups = - Actors.all_groups!(socket.assigns.subject, - preload: :provider, - filter: [editable?: true] - ) + Actors.all_editable_groups!(socket.assigns.subject, preload: :provider) changeset = Actors.new_actor() diff --git a/elixir/apps/web/test/web/live/actors/edit_test.exs b/elixir/apps/web/test/web/live/actors/edit_test.exs index 472fdc6e3..07d5c9e8b 100644 --- a/elixir/apps/web/test/web/live/actors/edit_test.exs +++ b/elixir/apps/web/test/web/live/actors/edit_test.exs @@ -166,7 +166,7 @@ defmodule Web.Live.Actors.EditTest do actor: actor, conn: conn } do - attrs = %{name: String.duplicate("X", 555)} + attrs = %{name: String.duplicate("X", 555), type: :account_user} {:ok, lv, _html} = conn @@ -184,6 +184,23 @@ defmodule Web.Live.Actors.EditTest do actor: actor, conn: conn } do + managed_group = Fixtures.Actors.create_managed_group(account: account) + + {provider, _bypass} = + Fixtures.Auth.start_and_create_google_workspace_provider(account: account) + + synced_group = Fixtures.Actors.create_group(account: account) + + synced_group + |> Ecto.Changeset.change( + created_by: :provider, + provider_id: provider.id, + provider_identifier: Ecto.UUID.generate() + ) + |> Repo.update!() + + Fixtures.Actors.create_membership(actor: actor, group: synced_group) + group1 = Fixtures.Actors.create_group(account: account) Fixtures.Actors.create_membership(actor: actor, group: group1) @@ -205,10 +222,15 @@ defmodule Web.Live.Actors.EditTest do assert_redirected(lv, ~p"/#{account}/actors/#{actor}") + expected_group_ids = [managed_group.id, synced_group.id, group2.id] + assert actor = Repo.get_by(Domain.Actors.Actor, id: actor.id) |> Repo.preload(:memberships) assert actor.name == attrs.name - assert [%{group_id: group_id}] = actor.memberships - assert group_id == group2.id + assert length(actor.memberships) == length(expected_group_ids) + + Enum.each(actor.memberships, fn membership -> + assert membership.group_id in expected_group_ids + end) end end