Merge pull request #410 from UW-GAC/bugfix/404-audit-deactivated-acco…

…unts

Fix unexpected errors with deactivated accounts and managed group membership

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 * Add filtering in list views.
 * Bugfix: Print the correct number of "ok" instances in audit emails. 0.18 introduced a bug where the email included "0 instance(s) verified even if there was more than one verified instance.
+* Bugfix: ManagedGroupMembershipAudit does not unexpectedly show errors for deactivated accounts that were in the group before they were deactivated.
+* Bugfix: ManagedGroupMembershipAudit now raises the correct exception when instantiated with a ManagedGroup that is not managed by the app.
 
 ## 0.18 (2023-10-03)
 

anvil_consortium_manager/__init__.py

@@ -1 +1 @@
-__version__ = "0.19dev2"
+__version__ = "0.19dev3"

anvil_consortium_manager/audit/audit.py

@@ -267,6 +267,16 @@ class ManagedGroupMembershipAudit(AnVILAudit):
     ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL = "Account not a member in AnVIL"
     """Error when an Account is a member of a ManagedGroup on the app, but not in AnVIL."""
 
+    ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL = (
+        "Account is deactivated but is an admin in AnVIL."
+    )
+    """Error when a deactivated Account is an admin of a ManagedGroup in AnVIL."""
+
+    ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL = (
+        "Account is deactivated but is a member in AnVIL."
+    )
+    """Error when a deactivated Account is a member of a ManagedGroup in AnVIL."""
+
     ERROR_GROUP_ADMIN_NOT_IN_ANVIL = "Group not an admin in AnVIL"
     """Error when a ManagedGroup is an admin of another ManagedGroup on the app, but not in AnVIL."""
 
@@ -277,7 +287,7 @@ def __init__(self, managed_group, *args, **kwargs):
         super().__init__(*args, **kwargs)
         if not managed_group.is_managed_by_app:
             raise exceptions.AnVILNotGroupAdminError(
-                "group {} is not managed by app".format(self.name)
+                "group {} is not managed by app".format(managed_group.name)
             )
         self.managed_group = managed_group
 
@@ -317,19 +327,29 @@ def run_audit(self):
             if membership.role == models.GroupAccountMembership.ADMIN:
                 try:
                     admins_in_anvil.remove(membership.account.email.lower())
+                    if membership.account.status == models.Account.INACTIVE_STATUS:
+                        model_instance_result.add_error(
+                            self.ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL
+                        )
                 except ValueError:
-                    # This email is not in the list of members.
-                    model_instance_result.add_error(
-                        self.ERROR_ACCOUNT_ADMIN_NOT_IN_ANVIL
-                    )
+                    # For active accounts, this is an error - the email is not in the list of members.
+                    if membership.account.status == models.Account.ACTIVE_STATUS:
+                        model_instance_result.add_error(
+                            self.ERROR_ACCOUNT_ADMIN_NOT_IN_ANVIL
+                        )
             elif membership.role == models.GroupAccountMembership.MEMBER:
                 try:
                     members_in_anvil.remove(membership.account.email.lower())
+                    if membership.account.status == models.Account.INACTIVE_STATUS:
+                        model_instance_result.add_error(
+                            self.ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL
+                        )
                 except ValueError:
-                    # This email is not in the list of members.
-                    model_instance_result.add_error(
-                        self.ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL
-                    )
+                    # For active accounts, this is an error - the email is not in the list of members.
+                    if membership.account.status == models.Account.ACTIVE_STATUS:
+                        model_instance_result.add_error(
+                            self.ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL
+                        )
             self.add_result(model_instance_result)
 
         # Check group-group membership.

anvil_consortium_manager/tests/test_audit.py

@@ -2,7 +2,7 @@
 from django.test import TestCase
 from faker import Faker
 
-from .. import models
+from .. import exceptions, models
 from ..audit import audit
 from . import api_factories, factories
 from .utils import AnVILAPIMockTestMixin
@@ -1476,6 +1476,11 @@ def get_api_url_admins(self, group_name):
             self.api_client.sam_entry_point + "/api/groups/v1/" + group_name + "/admin"
         )
 
+    def test_group_not_managed_by_app(self):
+        group = factories.ManagedGroupFactory.create(is_managed_by_app=False)
+        with self.assertRaises(exceptions.AnVILNotGroupAdminError):
+            audit.ManagedGroupMembershipAudit(group)
+
     def test_no_members(self):
         """audit works correctly if this group has no members."""
         group = factories.ManagedGroupFactory.create()
@@ -2634,6 +2639,140 @@ def test_group_is_both_admin_and_member(self):
         record_result = audit_results.get_result_for_model_instance(membership)
         self.assertTrue(record_result.ok())
 
+    def test_deactivated_account_not_member_in_anvil(self):
+        """Audit is ok if a deactivated account is not in the group on AnVIL."""
+        group = factories.ManagedGroupFactory.create()
+        # Create an inactive account that is a member of this group.
+        factories.GroupAccountMembershipFactory.create(
+            group=group, account__status=models.Account.INACTIVE_STATUS
+        )
+        # The Account is not a member in AnVIL
+        api_url_members = self.get_api_url_members(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_members,
+            status=200,
+            json=api_factories.GetGroupMembershipResponseFactory().response,
+        )
+        api_url_admins = self.get_api_url_admins(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_admins,
+            status=200,
+            json=api_factories.GetGroupMembershipAdminResponseFactory().response,
+        )
+        audit_results = audit.ManagedGroupMembershipAudit(group)
+        audit_results.run_audit()
+        self.assertTrue(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 1)
+        self.assertEqual(len(audit_results.get_error_results()), 0)
+        self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+
+    def test_deactivated_account_member_in_anvil(self):
+        """Audit is not ok if a deactivated account is in the group on AnVIL."""
+        group = factories.ManagedGroupFactory.create()
+        # Create an inactive account that is a member of this group.
+        membership = factories.GroupAccountMembershipFactory.create(
+            group=group, account__status=models.Account.INACTIVE_STATUS
+        )
+        # The Account is not a member in AnVIL
+        api_url_members = self.get_api_url_members(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_members,
+            status=200,
+            json=api_factories.GetGroupMembershipResponseFactory(
+                response=[membership.account.email]
+            ).response,
+        )
+        api_url_admins = self.get_api_url_admins(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_admins,
+            status=200,
+            json=api_factories.GetGroupMembershipAdminResponseFactory().response,
+        )
+        audit_results = audit.ManagedGroupMembershipAudit(group)
+        audit_results.run_audit()
+        self.assertFalse(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 0)
+        self.assertEqual(len(audit_results.get_error_results()), 1)
+        self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+        record_result = audit_results.get_result_for_model_instance(membership)
+        self.assertEqual(
+            record_result.errors,
+            set([audit_results.ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL]),
+        )
+
+    def test_deactivated_account_not_admin_in_anvil(self):
+        """Audit is ok if a deactivated account is not in the group on AnVIL."""
+        group = factories.ManagedGroupFactory.create()
+        # Create an inactive account that is a member of this group.
+        factories.GroupAccountMembershipFactory.create(
+            group=group,
+            account__status=models.Account.INACTIVE_STATUS,
+            role=models.GroupAccountMembership.ADMIN,
+        )
+        # The Account is not a member in AnVIL
+        api_url_members = self.get_api_url_members(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_members,
+            status=200,
+            json=api_factories.GetGroupMembershipResponseFactory().response,
+        )
+        api_url_admins = self.get_api_url_admins(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_admins,
+            status=200,
+            json=api_factories.GetGroupMembershipAdminResponseFactory().response,
+        )
+        audit_results = audit.ManagedGroupMembershipAudit(group)
+        audit_results.run_audit()
+        self.assertTrue(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 1)
+        self.assertEqual(len(audit_results.get_error_results()), 0)
+        self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+
+    def test_deactivated_account_admin_in_anvil(self):
+        """Audit is not ok if a deactivated account is in the group on AnVIL."""
+        group = factories.ManagedGroupFactory.create()
+        # Create an inactive account that is a member of this group.
+        membership = factories.GroupAccountMembershipFactory.create(
+            group=group,
+            account__status=models.Account.INACTIVE_STATUS,
+            role=models.GroupAccountMembership.ADMIN,
+        )
+        # The Account is not a member in AnVIL
+        api_url_members = self.get_api_url_members(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_members,
+            status=200,
+            json=api_factories.GetGroupMembershipResponseFactory().response,
+        )
+        api_url_admins = self.get_api_url_admins(group.name)
+        self.anvil_response_mock.add(
+            responses.GET,
+            api_url_admins,
+            status=200,
+            json=api_factories.GetGroupMembershipAdminResponseFactory(
+                response=[membership.account.email]
+            ).response,
+        )
+        audit_results = audit.ManagedGroupMembershipAudit(group)
+        audit_results.run_audit()
+        self.assertFalse(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 0)
+        self.assertEqual(len(audit_results.get_error_results()), 1)
+        self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+        record_result = audit_results.get_result_for_model_instance(membership)
+        self.assertEqual(
+            record_result.errors,
+            set([audit_results.ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL]),
+        )
+
 
 class WorkspaceAuditTest(AnVILAPIMockTestMixin, TestCase):
     """Tests for the Workspace.anvil_audit method."""