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

@@ -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()
 

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)

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)
 

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})
 

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()
 

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