Attempting to delete an in-use group fails and leaves group in inconsistent state

[mbookman]

Reproducible test case:

In Terra:

1. Create a group: deleteable-test-group
2. Create a workspace: deleteable-workspace
3. Share deleteable-workspace with deleteable-test-group as Readers
4. Try to delete the group deleteable-test-group

You will get an error message here:

Error deleting group
Error 409: group deleteable-test-group cannot be deleted because it is a member of
at least 1 other group
Source: sam

and in Terra, it appears that the group has not been deleted.

In the Google Cloud Console:

1. Create a Cloud Storage bucket deleteable-bucket in a Cloud project you own
2. In the Permissions tab for the bucket, try to Add Member deleteable-test-group@firecloud.org

You will get an error message:

Email addresses and domains must be associated with an active Google Account
or Google Apps account.

The underlying meaning of this message is that deleteable-test-group@firecloud.org does not exist.

Looking into the SAM code, this appears to be implemented here:

  def deleteManagedGroup(groupId: ResourceId): Future[Unit] =
    for {
      // order is important here, we want to make sure we do all the cloudExtensions calls before we touch ldap
      // so failures there do not leave ldap in a bad state
      // resourceService.deleteResource also does cloudExtensions.onGroupDelete first thing
      _ <- cloudExtensions.onGroupDelete(WorkbenchEmail(constructEmail(groupId.value)))
      managedGroupResourceId = FullyQualifiedResourceId(managedGroupType.name, groupId)
      _ <- resourceService.cloudDeletePolicies(managedGroupResourceId)
      _ <- directoryDAO.deleteGroup(WorkbenchGroupName(groupId.value)).unsafeToFuture()
      _ <- resourceService.deleteResource(managedGroupResourceId)
    } yield ()

Looking elsewhere I see in src/main/scala/org/broadinstitute/dsde/workbench/sam/directory/LdapDirectoryDAO.scala:

  override def deleteGroup(groupName: WorkbenchGroupName): IO[Unit] =
    for {
      ancestors <- listAncestorGroups(groupName)
      res <- if (ancestors.nonEmpty) {
        IO.raiseError(
          new WorkbenchExceptionWithErrorReport(
            ErrorReport(StatusCodes.Conflict, s"group ${groupName.value} cannot be deleted because it is a member of at least 1 other group")))
      } else {
        executeLdap(IO(ldapConnectionPool.delete(groupDn(groupName))).void)
      }
    } yield res```

So it looks to me like these steps complete successfully:

      _ <- cloudExtensions.onGroupDelete(WorkbenchEmail(constructEmail(groupId.value)))
      managedGroupResourceId = FullyQualifiedResourceId(managedGroupType.name, groupId)
      _ <- resourceService.cloudDeletePolicies(managedGroupResourceId)

and this fails:

      _ <- directoryDAO.deleteGroup(WorkbenchGroupName(groupId.value)).unsafeToFuture()

Leaving the group in an inconsistent state. Not sure the right solution here, since this is attempting to execute something atomic across multiple systems. A change that would help here would be to check:

      ancestors <- listAncestorGroups(groupName)
      res <- if (ancestors.nonEmpty) {

before attempting the deletion.