From 694bbc86aa76c2246633677a5f6c7000dc4ec8b4 Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Fri, 14 Feb 2025 13:29:52 -0800 Subject: [PATCH 1/4] Add inactive user checks on CDSA UploaderAudit Inactive users are expected to not be a part of the access group. Update the audits to check user status and handle appropriately. --- primed/cdsa/audit/uploader_audit.py | 52 +++++++++++++++++++++-------- primed/cdsa/tests/test_audit.py | 48 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/primed/cdsa/audit/uploader_audit.py b/primed/cdsa/audit/uploader_audit.py index 7b379fa6..3d1c95a9 100644 --- a/primed/cdsa/audit/uploader_audit.py +++ b/primed/cdsa/audit/uploader_audit.py @@ -128,6 +128,9 @@ class UploaderAudit(PRIMEDAudit): NOT_UPLOADER = "Not an uploader." GROUP_WITHOUT_ACCESS = "Groups do not have access." + # message when an account is inactive. + INACTIVE_ACCOUNT = "Account is inactive." + # # Unexpected. # ERROR_HAS_ACCESS = "Has access for an unknown reason." UNEXPECTED_GROUP_ACCESS = "Group should not have access." @@ -254,17 +257,28 @@ def _audit_agreement_and_account(self, data_affiliate_agreement, account): return is_uploader = user in data_affiliate_agreement.uploaders.all() + is_active = account.status == account.ACTIVE_STATUS if is_in_access_group: if is_uploader: - self.verified.append( - VerifiedAccess( - data_affiliate_agreement=data_affiliate_agreement, - user=user, - member=account, - note=self.UPLOADER_IN_ACCESS_GROUP, + if is_active: + self.verified.append( + VerifiedAccess( + data_affiliate_agreement=data_affiliate_agreement, + user=user, + member=account, + note=self.UPLOADER_IN_ACCESS_GROUP, + ) + ) + else: + self.needs_action.append( + RemoveAccess( + data_affiliate_agreement=data_affiliate_agreement, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: self.needs_action.append( RemoveAccess( @@ -276,14 +290,24 @@ def _audit_agreement_and_account(self, data_affiliate_agreement, account): ) else: if is_uploader: - self.needs_action.append( - GrantAccess( - data_affiliate_agreement=data_affiliate_agreement, - user=user, - member=account, - note=self.UPLOADER_LINKED_ACCOUNT, + if is_active: + self.needs_action.append( + GrantAccess( + data_affiliate_agreement=data_affiliate_agreement, + user=user, + member=account, + note=self.UPLOADER_LINKED_ACCOUNT, + ) + ) + else: + self.verified.append( + VerifiedNoAccess( + data_affiliate_agreement=data_affiliate_agreement, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: self.verified.append( VerifiedNoAccess( diff --git a/primed/cdsa/tests/test_audit.py b/primed/cdsa/tests/test_audit.py index 4b7b884f..67a3ae93 100644 --- a/primed/cdsa/tests/test_audit.py +++ b/primed/cdsa/tests/test_audit.py @@ -3122,6 +3122,54 @@ def test_uploader_no_account(self): self.assertEqual(record.member, None) self.assertEqual(record.note, uploader_audit.UploaderAudit.UPLOADER_NO_ACCOUNT) + def test_uploader_inactive_account_not_in_access_group(self): + # Create agreement. + data_affiliate_agreement = factories.DataAffiliateAgreementFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up uploaders. + data_affiliate_agreement.uploaders.add(account.user) + # Deactivate account. + account.deactivate() + # Set up audit + audit = uploader_audit.UploaderAudit() + # Run audit + audit.audit_agreement(data_affiliate_agreement) + self.assertEqual(len(audit.verified), 1) + self.assertEqual(len(audit.needs_action), 0) + self.assertEqual(len(audit.errors), 0) + record = audit.verified[0] + self.assertIsInstance(record, uploader_audit.VerifiedNoAccess) + self.assertEqual(record.data_affiliate_agreement, data_affiliate_agreement) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, uploader_audit.UploaderAudit.INACTIVE_ACCOUNT) + + def test_uploader_inactive_account_in_access_group(self): + # Create agreement. + data_affiliate_agreement = factories.DataAffiliateAgreementFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up uploaders. + data_affiliate_agreement.uploaders.add(account.user) + # Deactivate account. + account.deactivate() + # Access group membership. + GroupAccountMembershipFactory.create(group=data_affiliate_agreement.anvil_upload_group, account=account) + # Set up audit + audit = uploader_audit.UploaderAudit() + # Run audit + audit.audit_agreement(data_affiliate_agreement) + self.assertEqual(len(audit.verified), 0) + self.assertEqual(len(audit.needs_action), 1) + self.assertEqual(len(audit.errors), 0) + record = audit.needs_action[0] + self.assertIsInstance(record, uploader_audit.RemoveAccess) + self.assertEqual(record.data_affiliate_agreement, data_affiliate_agreement) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, uploader_audit.UploaderAudit.INACTIVE_ACCOUNT) + def test_user_in_group_not_uploader(self): # Create applications. data_affiliate_agreement = factories.DataAffiliateAgreementFactory.create() From 34c17786bd1dc61714e27e370d813c704d9a52bc Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Fri, 14 Feb 2025 14:05:15 -0800 Subject: [PATCH 2/4] Add active/inactive account checks for CDSA AccessorAudit --- primed/cdsa/audit/accessor_audit.py | 76 ++++++++++++++++++++--------- primed/cdsa/tests/test_audit.py | 48 ++++++++++++++++++ 2 files changed, 102 insertions(+), 22 deletions(-) diff --git a/primed/cdsa/audit/accessor_audit.py b/primed/cdsa/audit/accessor_audit.py index 7c50c00c..3c5dc883 100644 --- a/primed/cdsa/audit/accessor_audit.py +++ b/primed/cdsa/audit/accessor_audit.py @@ -122,6 +122,8 @@ class AccessorAudit(PRIMEDAudit): NOT_ACCESSOR = "Not an accessor." GROUP_WITHOUT_ACCESS = "Groups do not have access." + # Active status + INACTIVE_ACCOUNT = "Account is inactive." # # Unexpected. # ERROR_HAS_ACCESS = "Has access for an unknown reason." UNEXPECTED_GROUP_ACCESS = "Group should not have access." @@ -222,6 +224,7 @@ def _audit_agreement_and_account(self, signed_agreement, account): is_in_access_group = GroupAccountMembership.objects.filter( account=account, group=signed_agreement.anvil_access_group ).exists() + is_active = account.status == account.ACTIVE_STATUS # Get the user. if hasattr(account, "user") and account.user: @@ -248,36 +251,65 @@ def _audit_agreement_and_account(self, signed_agreement, account): return is_accessor = user in signed_agreement.accessors.all() - if is_in_access_group: if is_accessor: - self.verified.append( - VerifiedAccess( - signed_agreement=signed_agreement, - user=user, - member=account, - note=self.ACCESSOR_IN_ACCESS_GROUP, + if is_active: + self.verified.append( + VerifiedAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.ACCESSOR_IN_ACCESS_GROUP, + ) + ) + else: + self.needs_action.append( + RemoveAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: - self.needs_action.append( - RemoveAccess( - signed_agreement=signed_agreement, - user=user, - member=account, - note=self.NOT_ACCESSOR, + if is_active: + self.needs_action.append( + RemoveAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.NOT_ACCESSOR, + ) + ) + else: + self.needs_action.append( + RemoveAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: if is_accessor: - self.needs_action.append( - GrantAccess( - signed_agreement=signed_agreement, - user=user, - member=account, - note=self.ACCESSOR_LINKED_ACCOUNT, + if is_active: + self.needs_action.append( + GrantAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.ACCESSOR_LINKED_ACCOUNT, + ) + ) + else: + self.verified.append( + VerifiedNoAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: self.verified.append( VerifiedNoAccess( diff --git a/primed/cdsa/tests/test_audit.py b/primed/cdsa/tests/test_audit.py index 67a3ae93..c5caa9ed 100644 --- a/primed/cdsa/tests/test_audit.py +++ b/primed/cdsa/tests/test_audit.py @@ -2680,6 +2680,54 @@ def test_not_accessor_and_account_has_no_user(self): self.assertEqual(record.member, account) self.assertEqual(record.note, accessor_audit.AccessorAudit.ACCOUNT_NOT_LINKED_TO_USER) + def test_accessor_inactive_account_not_in_access_group(self): + # Create applications. + signed_agreement = factories.SignedAgreementFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up accessors. + signed_agreement.accessors.add(account.user) + # Deactivate account. + account.deactivate() + # Set up audit + audit = accessor_audit.AccessorAudit() + # Run audit + audit.audit_agreement(signed_agreement) + self.assertEqual(len(audit.verified), 1) + self.assertEqual(len(audit.needs_action), 0) + self.assertEqual(len(audit.errors), 0) + record = audit.verified[0] + self.assertIsInstance(record, accessor_audit.VerifiedNoAccess) + self.assertEqual(record.signed_agreement, signed_agreement) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, accessor_audit.AccessorAudit.INACTIVE_ACCOUNT) + + def test_accessor_inactive_account_in_access_group(self): + # Create applications. + signed_agreement = factories.SignedAgreementFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up accessors. + signed_agreement.accessors.add(account.user) + # Deactivate account. + account.deactivate() + # Access group membership. + GroupAccountMembershipFactory.create(group=signed_agreement.anvil_access_group, account=account) + # Set up audit + audit = accessor_audit.AccessorAudit() + # Run audit + audit.audit_agreement(signed_agreement) + self.assertEqual(len(audit.verified), 0) + self.assertEqual(len(audit.needs_action), 1) + self.assertEqual(len(audit.errors), 0) + record = audit.needs_action[0] + self.assertIsInstance(record, accessor_audit.RemoveAccess) + self.assertEqual(record.signed_agreement, signed_agreement) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, accessor_audit.AccessorAudit.INACTIVE_ACCOUNT) + def test_two_accessors(self): """Audit works when there are two accessors.""" # Create applications. From a8a43d7d162866a684f3554ed09b3130405d2eaf Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Fri, 14 Feb 2025 14:21:56 -0800 Subject: [PATCH 3/4] Add active/inactive accoutn check for dbGaP collaborators Check collaborators' active or inactive status when auditing. Note that we are not checking PI status, because if a PI leaves we'll know about it through other channels, and need to decide what to do. --- primed/dbgap/audit/collaborator_audit.py | 52 +++++++++++++++++------- primed/dbgap/tests/test_audit.py | 48 ++++++++++++++++++++++ 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/primed/dbgap/audit/collaborator_audit.py b/primed/dbgap/audit/collaborator_audit.py index 572fb3d0..a4a84341 100644 --- a/primed/dbgap/audit/collaborator_audit.py +++ b/primed/dbgap/audit/collaborator_audit.py @@ -131,6 +131,9 @@ class dbGaPCollaboratorAudit(PRIMEDAudit): NOT_COLLABORATOR = "Not a collaborator." GROUP_WITHOUT_ACCESS = "Groups do not have access." + # Inactive account. + INACTIVE_ACCOUNT = "Account is inactive." + # # Unexpected. # ERROR_HAS_ACCESS = "Has access for an unknown reason." UNEXPECTED_GROUP_ACCESS = "Group should not have access." @@ -267,6 +270,7 @@ def _audit_application_and_account(self, dbgap_application, account): is_pi = user == dbgap_application.principal_investigator is_collaborator = user in dbgap_application.collaborators.all() + is_active = account.status == account.ACTIVE_STATUS if is_in_access_group: if is_pi: @@ -279,14 +283,24 @@ def _audit_application_and_account(self, dbgap_application, account): ) ) elif is_collaborator: - self.verified.append( - VerifiedAccess( - dbgap_application=dbgap_application, - user=user, - member=account, - note=self.COLLABORATOR_IN_ACCESS_GROUP, + if is_active: + self.verified.append( + VerifiedAccess( + dbgap_application=dbgap_application, + user=user, + member=account, + note=self.COLLABORATOR_IN_ACCESS_GROUP, + ) + ) + else: + self.needs_action.append( + RemoveAccess( + dbgap_application=dbgap_application, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: self.needs_action.append( RemoveAccess( @@ -307,14 +321,24 @@ def _audit_application_and_account(self, dbgap_application, account): ) ) elif is_collaborator: - self.needs_action.append( - GrantAccess( - dbgap_application=dbgap_application, - user=user, - member=account, - note=self.COLLABORATOR_LINKED_ACCOUNT, + if is_active: + self.needs_action.append( + GrantAccess( + dbgap_application=dbgap_application, + user=user, + member=account, + note=self.COLLABORATOR_LINKED_ACCOUNT, + ) + ) + else: + self.verified.append( + VerifiedNoAccess( + dbgap_application=dbgap_application, + user=user, + member=account, + note=self.INACTIVE_ACCOUNT, + ) ) - ) else: self.verified.append( VerifiedNoAccess( diff --git a/primed/dbgap/tests/test_audit.py b/primed/dbgap/tests/test_audit.py index 9bb8c6a0..18405493 100644 --- a/primed/dbgap/tests/test_audit.py +++ b/primed/dbgap/tests/test_audit.py @@ -1190,6 +1190,54 @@ def test_collaborator_linked_account_not_in_access_group(self): self.assertEqual(record.member, account) self.assertEqual(record.note, collaborator_audit.dbGaPCollaboratorAudit.COLLABORATOR_LINKED_ACCOUNT) + def test_collaborator_linked_account_inactive_in_access_group(self): + # Create applications. + dbgap_application = factories.dbGaPApplicationFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up collaborators. + dbgap_application.collaborators.add(account.user) + # Deactivate account. + account.deactivate() + # Access group membership. + GroupAccountMembershipFactory.create(group=dbgap_application.anvil_access_group, account=account) + # Set up audit + collab_audit = collaborator_audit.dbGaPCollaboratorAudit() + # Run audit + collab_audit.audit_application_and_object(dbgap_application, account.user) + self.assertEqual(len(collab_audit.verified), 0) + self.assertEqual(len(collab_audit.needs_action), 1) + self.assertEqual(len(collab_audit.errors), 0) + record = collab_audit.needs_action[0] + self.assertIsInstance(record, collaborator_audit.RemoveAccess) + self.assertEqual(record.dbgap_application, dbgap_application) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, collaborator_audit.dbGaPCollaboratorAudit.INACTIVE_ACCOUNT) + + def test_collaborator_linked_account_inactive_not_in_access_group(self): + # Create applications. + dbgap_application = factories.dbGaPApplicationFactory.create() + # Create accounts. + account = AccountFactory.create(verified=True) + # Set up collaborators. + dbgap_application.collaborators.add(account.user) + # Deactivate account. + account.deactivate() + # Set up audit + collab_audit = collaborator_audit.dbGaPCollaboratorAudit() + # Run audit + collab_audit.audit_application_and_object(dbgap_application, account.user) + self.assertEqual(len(collab_audit.verified), 1) + self.assertEqual(len(collab_audit.needs_action), 0) + self.assertEqual(len(collab_audit.errors), 0) + record = collab_audit.verified[0] + self.assertIsInstance(record, collaborator_audit.VerifiedNoAccess) + self.assertEqual(record.dbgap_application, dbgap_application) + self.assertEqual(record.user, account.user) + self.assertEqual(record.member, account) + self.assertEqual(record.note, collaborator_audit.dbGaPCollaboratorAudit.INACTIVE_ACCOUNT) + def test_collaborator_no_account(self): # Create applications. dbgap_application = factories.dbGaPApplicationFactory.create() From d540c83bb6557cf891fd91d3fba489a041e454f7 Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Fri, 14 Feb 2025 14:53:04 -0800 Subject: [PATCH 4/4] Remove accidentally added, unneeded code block This block was checking account active/inactive status for someone in the access group was not an accessor. At that point it doesn't matter if their account was inactive; they need to be removed. --- primed/cdsa/audit/accessor_audit.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/primed/cdsa/audit/accessor_audit.py b/primed/cdsa/audit/accessor_audit.py index 3c5dc883..ade63e47 100644 --- a/primed/cdsa/audit/accessor_audit.py +++ b/primed/cdsa/audit/accessor_audit.py @@ -272,24 +272,14 @@ def _audit_agreement_and_account(self, signed_agreement, account): ) ) else: - if is_active: - self.needs_action.append( - RemoveAccess( - signed_agreement=signed_agreement, - user=user, - member=account, - note=self.NOT_ACCESSOR, - ) - ) - else: - self.needs_action.append( - RemoveAccess( - signed_agreement=signed_agreement, - user=user, - member=account, - note=self.INACTIVE_ACCOUNT, - ) + self.needs_action.append( + RemoveAccess( + signed_agreement=signed_agreement, + user=user, + member=account, + note=self.NOT_ACCESSOR, ) + ) else: if is_accessor: if is_active: