Fix MFA policy management with unknown users (#1125)

fence/sync/sync_users.py

@@ -1868,13 +1868,17 @@ 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
+        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, {}
@@ -1886,7 +1890,7 @@ def _revoke_all_policies_preserve_mfa(self, user):
 
         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}"
@@ -1998,12 +2002,14 @@ 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
 
             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

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):
     """
@@ -1051,7 +1065,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 +1094,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 +1116,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 +1146,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
     )