From 52ae7acbed8fd3dda9658690896d62f703646003 Mon Sep 17 00:00:00 2001 From: Kyle Burton Date: Mon, 4 Dec 2023 15:33:00 -0600 Subject: [PATCH 1/6] Fix MFA policy management with unknown users Fixes issue with usersync and MFA policy management in cases where a user has not been persisted by Fence. --- fence/sync/sync_users.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/fence/sync/sync_users.py b/fence/sync/sync_users.py index 37d535832..5b0c96c16 100644 --- a/fence/sync/sync_users.py +++ b/fence/sync/sync_users.py @@ -1868,13 +1868,13 @@ def _update_arborist(self, session, user_yaml): return True - def _revoke_all_policies_preserve_mfa(self, user): + def _revoke_all_policies_preserve_mfa(self, username, idp=None): """ If MFA is enabled for the user's idp, check if they have the /multifactor_auth resource and restore the mfa_policy after revoking all policies. """ - username = user.username - idp = user.identity_provider.name if user.identity_provider else None + # username = user.username + # idp = user.identity_provider.name if user.identity_provider else None is_mfa_enabled = "multifactor_auth_claim_info" in config["OPENID_CONNECT"].get( idp, {} @@ -2000,10 +2000,11 @@ def _update_authz_in_arborist( user = query_for_user(session=session, username=username) if user: username = user.username + idp = user.identity_provider.name if user.identity_provider else None self.arborist_client.create_user_if_not_exist(username) if not single_user_sync: - self._revoke_all_policies_preserve_mfa(user) + self._revoke_all_policies_preserve_mfa(username, idp) # as of 2/11/2022, for single_user_sync, as RAS visa parsing has # previously mapped each project to the same set of privileges From 7acc3961483e37fbce520f8887c6d510fe20028a Mon Sep 17 00:00:00 2001 From: Kyle Burton Date: Mon, 4 Dec 2023 15:44:13 -0600 Subject: [PATCH 2/6] Fixing unit tests --- tests/dbgap_sync/test_user_sync.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/dbgap_sync/test_user_sync.py b/tests/dbgap_sync/test_user_sync.py index 579f1b416..5694265ad 100644 --- a/tests/dbgap_sync/test_user_sync.py +++ b/tests/dbgap_sync/test_user_sync.py @@ -1051,7 +1051,7 @@ def test_revoke_all_policies_preserve_mfa(monkeypatch, db_session, syncer): username="mockuser", identity_provider=IdentityProvider(name="mock_idp") ) syncer.arborist_client.get_user.return_value = {"policies": ["mfa_policy"]} - syncer._revoke_all_policies_preserve_mfa(user) + syncer._revoke_all_policies_preserve_mfa(user.username, user.identity_provider.name) syncer.arborist_client.revoke_all_policies_for_user.assert_called_with( user.username ) @@ -1080,7 +1080,7 @@ def test_revoke_all_policies_preserve_mfa_no_mfa(monkeypatch, db_session, syncer syncer.arborist_client.list_resources_for_user.return_value = [ "/programs/phs0001111" ] - syncer._revoke_all_policies_preserve_mfa(user) + syncer._revoke_all_policies_preserve_mfa(user.username, user.identity_provider.name) syncer.arborist_client.revoke_all_policies_for_user.assert_called_with( user.username ) @@ -1102,7 +1102,7 @@ def test_revoke_all_policies_preserve_mfa_no_idp(monkeypatch, db_session, syncer }, ) user = User(username="mockuser") - syncer._revoke_all_policies_preserve_mfa(user) + syncer._revoke_all_policies_preserve_mfa(user.username) syncer.arborist_client.revoke_all_policies_for_user.assert_called_with( user.username ) @@ -1132,7 +1132,7 @@ def test_revoke_all_policies_preserve_mfa_ensure_revoke_on_error( syncer.arborist_client.list_resources_for_user.side_effect = Exception( "Unknown error" ) - syncer._revoke_all_policies_preserve_mfa(user) + syncer._revoke_all_policies_preserve_mfa(user.username, user.identity_provider.name) syncer.arborist_client.revoke_all_policies_for_user.assert_called_with( user.username ) From 37c1e15d2db58392af123514cbcd8698c043b760 Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Mon, 4 Dec 2023 15:55:52 -0600 Subject: [PATCH 3/6] remove commented out lines --- fence/sync/sync_users.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/fence/sync/sync_users.py b/fence/sync/sync_users.py index 5b0c96c16..b18da414c 100644 --- a/fence/sync/sync_users.py +++ b/fence/sync/sync_users.py @@ -1873,9 +1873,6 @@ def _revoke_all_policies_preserve_mfa(self, username, idp=None): If MFA is enabled for the user's idp, check if they have the /multifactor_auth resource and restore the mfa_policy after revoking all policies. """ - # username = user.username - # idp = user.identity_provider.name if user.identity_provider else None - is_mfa_enabled = "multifactor_auth_claim_info" in config["OPENID_CONNECT"].get( idp, {} ) From effef594d31e99c6d3d1df598c2be79d9bab6746 Mon Sep 17 00:00:00 2001 From: Alex VanTol Date: Mon, 4 Dec 2023 15:59:21 -0600 Subject: [PATCH 4/6] fix(mfa): default value for var used later --- fence/sync/sync_users.py | 1 + 1 file changed, 1 insertion(+) diff --git a/fence/sync/sync_users.py b/fence/sync/sync_users.py index b18da414c..6424742ff 100644 --- a/fence/sync/sync_users.py +++ b/fence/sync/sync_users.py @@ -1995,6 +1995,7 @@ def _update_authz_in_arborist( for username, user_project_info in user_projects.items(): self.logger.info("processing user `{}`".format(username)) user = query_for_user(session=session, username=username) + idp = None if user: username = user.username idp = user.identity_provider.name if user.identity_provider else None From bae737c3c181a877b77030f977830b54c640d76c Mon Sep 17 00:00:00 2001 From: Alex VanTol Date: Mon, 4 Dec 2023 16:21:32 -0600 Subject: [PATCH 5/6] fix(arborist): only revoke when the Arborist user exists --- fence/sync/sync_users.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fence/sync/sync_users.py b/fence/sync/sync_users.py index 6424742ff..2803c4523 100644 --- a/fence/sync/sync_users.py +++ b/fence/sync/sync_users.py @@ -1873,6 +1873,13 @@ def _revoke_all_policies_preserve_mfa(self, username, idp=None): If MFA is enabled for the user's idp, check if they have the /multifactor_auth resource and restore the mfa_policy after revoking all policies. """ + user_data_from_arborist = None + try: + user_data_from_arborist = self.arborist_client.get_user(username) + except ArboristError: + # user doesn't exist in Arborist, nothing to revoke + return + is_mfa_enabled = "multifactor_auth_claim_info" in config["OPENID_CONNECT"].get( idp, {} ) @@ -1883,7 +1890,7 @@ def _revoke_all_policies_preserve_mfa(self, username, idp=None): policies = [] try: - policies = self.arborist_client.get_user()["policies"] + policies = user_data_from_arborist["policies"] except Exception as e: self.logger.error( f"Could not retrieve user's policies, revoking all policies anyway. {e}" From 662c35d6152b1fda1ee68ba0aa738072f53dc500 Mon Sep 17 00:00:00 2001 From: Alex VanTol Date: Mon, 4 Dec 2023 16:55:13 -0600 Subject: [PATCH 6/6] chore(tests): add regression test --- tests/dbgap_sync/test_user_sync.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/dbgap_sync/test_user_sync.py b/tests/dbgap_sync/test_user_sync.py index 5694265ad..0525df99b 100644 --- a/tests/dbgap_sync/test_user_sync.py +++ b/tests/dbgap_sync/test_user_sync.py @@ -1033,6 +1033,20 @@ def test_user_sync_with_visa_sync_job( ) +@pytest.mark.parametrize("syncer", ["cleversafe", "google"], indirect=True) +def test_revoke_all_policies_no_user(db_session, syncer): + """ + Test that function returns even when there's no user + """ + # no arborist user with that username + user_that_doesnt_exist = "foobar" + syncer.arborist_client.get_user.return_value = None + + syncer._revoke_all_policies_preserve_mfa(user_that_doesnt_exist, "mock_idp") + + # we only care that this doesn't error + assert True + @pytest.mark.parametrize("syncer", ["cleversafe", "google"], indirect=True) def test_revoke_all_policies_preserve_mfa(monkeypatch, db_session, syncer): """