Fix federation user deletion

When IDP is being removed the associated federated_user table entries
are being deleted. However the main user records are staying casing
corrupted data in the database since such records cannot be retrieved
due to the mandatory username (empty for such users).

After deleting the IdP perform additional cleanup of the orphaned
entries.

This issue is causing race failures in the gate where often user_list is
capturing broken records produced by the federation test

Change-Id: Ieb9e0d0bf1c714bf88026ee5c55fcf35c9f494e0
Assisted-by: qwen3
Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>

keystone/federation/backends/sql.py

@@ -14,6 +14,7 @@
 
 from oslo_log import log
 from oslo_serialization import jsonutils
+from sqlalchemy import exists
 from sqlalchemy import orm
 
 from keystone.common import sql
@@ -21,6 +22,7 @@ import keystone.conf
 from keystone import exception
 from keystone.federation.backends import base
 from keystone.i18n import _
+from keystone.identity.backends import sql_model as identity_model
 
 CONF = keystone.conf.CONF
 LOG = log.getLogger(__name__)
@@ -222,11 +224,88 @@ class Federation(base.FederationDriverBase):
         except sql.DBDuplicateEntry as e:
             self._handle_idp_conflict(e)
 
+    @staticmethod
+    def _clean_orphaned_federated_users(session):
+        """Delete federated_user rows whose IDP or protocol no longer exists.
+
+        Some databases (SQLite) do not enforce FK CASCADE, so federated_user
+        records may remain after their parent IDP or protocol is deleted.
+        This method explicitly cleans up those orphaned records.
+        """
+        # Delete federated_user rows whose IDP no longer exists
+        session.query(identity_model.FederatedUser).filter(
+            identity_model.FederatedUser.idp_id.notin_(
+                session.query(IdentityProviderModel.id)
+            )
+        ).delete(synchronize_session=False)
+        # Delete federated_user rows whose protocol no longer exists
+        session.query(identity_model.FederatedUser).filter(
+            identity_model.FederatedUser.protocol_id.notin_(
+                session.query(FederationProtocolModel.id)
+            )
+        ).delete(synchronize_session=False)
+
+    def _delete_orphaned_users(self, session):
+        """Delete users that have no name source after IDP/protocol deletion.
+
+        :param session: SQLAlchemy session
+        """
+        self._clean_orphaned_federated_users(session)
+        # Step 1: collect orphaned user IDs
+        # (SQLAlchemy restricts Query.delete() when joins are used).
+        orphaned_ids = [
+            row[0]
+            for row in (
+                session.query(identity_model.User.id)
+                .outerjoin(
+                    identity_model.LocalUser,
+                    identity_model.User.id == identity_model.LocalUser.user_id,
+                )
+                .outerjoin(
+                    identity_model.NonLocalUser,
+                    identity_model.User.id
+                    == identity_model.NonLocalUser.user_id,
+                )
+                .outerjoin(
+                    identity_model.FederatedUser,
+                    identity_model.User.id
+                    == identity_model.FederatedUser.user_id,
+                )
+                .filter(
+                    identity_model.LocalUser.user_id.is_(None),
+                    identity_model.NonLocalUser.user_id.is_(None),
+                    identity_model.FederatedUser.user_id.is_(None),
+                )
+                .all()
+            )
+        ]
+        if orphaned_ids:
+            # Delete dependent rows with FK on user.id before deleting users.
+            # ExpiringUserGroupMembership.user_id and
+            # UserGroupMembership.user_id have FK on user.id WITHOUT
+            # ondelete='CASCADE', so we must clean up those rows first to
+            # avoid FK constraint violations.
+            session.query(identity_model.ExpiringUserGroupMembership).filter(
+                identity_model.ExpiringUserGroupMembership.user_id.in_(
+                    orphaned_ids
+                )
+            ).delete(synchronize_session=False)
+            session.query(identity_model.UserGroupMembership).filter(
+                identity_model.UserGroupMembership.user_id.in_(orphaned_ids)
+            ).delete(synchronize_session=False)
+
+            # Delete orphaned users
+            session.query(identity_model.User).filter(
+                identity_model.User.id.in_(orphaned_ids)
+            ).delete(synchronize_session=False)
+
     def delete_idp(self, idp_id):
         with sql.session_for_write() as session:
             self._delete_assigned_protocols(session, idp_id)
             idp_ref = self._get_idp(session, idp_id)
             session.delete(idp_ref)
+            session.flush()
+            self._delete_orphaned_users(session)
 
     def _get_idp(self, session, idp_id):
         idp_ref = session.get(IdentityProviderModel, idp_id)
@@ -318,6 +397,8 @@ class Federation(base.FederationDriverBase):
         with sql.session_for_write() as session:
             key_ref = self._get_protocol(session, idp_id, protocol_id)
             session.delete(key_ref)
+            session.flush()
+            self._delete_orphaned_users(session)
 
     def _delete_assigned_protocols(self, session, idp_id):
         query = session.query(FederationProtocolModel)

keystone/tests/unit/test_shadow_users.py

@@ -206,3 +206,371 @@ class TestUserWithFederatedUser(ShadowUsersTests):
         self.assertTrue('federated' in user)
         self.assertEqual(len(user['federated']), 1)
         self.assertEqual(user['federated'][0], new_fed[0])
+
+
+class TestFederatedUserNameInList(ShadowUsersTests):
+    """Test that federated user name is consistent between get_user and list_users.
+
+    Verifies that FederatedUser.display_name is propagated to the User.name
+    hybrid property in the list_users path, which uses explicit outerjoin
+    rather than relying solely on lazy='joined' eager loading.
+    """
+
+    def setUp(self):
+        super().setUp()
+        self.useFixture(database.Database())
+        self.load_backends()
+
+    def test_federated_user_name_in_get_and_list(self):
+        display_name = 'federated_test_user'
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name=display_name,
+        )
+
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+
+        # Also create a local user so list_users query has both local and
+        # federated rows (exercises the outerjoin path).
+        local_user = unit.new_user_ref(domain_id=self.domain_id)
+        PROVIDERS.identity_api.create_user(local_user)
+
+        # Verify get_user returns the federated display_name
+        user_ref = self.identity_api.get_user(user['id'])
+        self.assertEqual(
+            display_name,
+            user_ref['name'],
+            'get_user should return the federated user display_name',
+        )
+
+        # Verify list_users (no filters) returns the same display_name.
+        # This exercises the explicit outerjoin(model.LocalUser) path in
+        # sql.py:list_users and ensures the federated_users eager-loaded
+        # relationship is populated alongside the explicit join.
+        users = PROVIDERS.identity_api.list_users()
+        found = [u for u in users if u['id'] == user['id']]
+        self.assertEqual(
+            1, len(found), 'Federated user should appear in list_users'
+        )
+        self.assertIsNotNone(
+            found[0]['name'],
+            'list_users must not return None for federated user name',
+        )
+        self.assertEqual(
+            display_name,
+            found[0]['name'],
+            'list_users should match get_user for federated display_name',
+        )
+
+    def test_orphaned_users_cleaned_up_after_idp_deletion(self):
+        """Verify users become orphaned and are deleted after IDP deletion.
+
+        When an IDP is deleted, federated_user records are CASCADE deleted.
+        Users that had no other name source (no local_user, no nonlocal_user,
+        no other federated_users) should be cleaned up.
+        """
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        display_name = 'federated_orphan_user'
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name=display_name,
+        )
+
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        with sql.session_for_read() as session:
+            # Verify the user exists before IDP deletion
+            u = session.get(identity_model.User, user_id)
+            self.assertIsNotNone(u)
+
+        # Delete the IDP, which should CASCADE delete federated_user records
+        # and then clean up the orphaned user
+        PROVIDERS.federation_api.delete_idp(self.idp['id'])
+
+        # Clear the cache so get_user doesn't return stale data
+        self.identity_api.get_user.invalidate()
+
+        # The orphaned user should no longer exist
+        self.assertRaises(
+            exception.UserNotFound, self.identity_api.get_user, user_id
+        )
+
+    def test_orphaned_users_cleaned_up_after_protocol_deletion(self):
+        """Verify users become orphaned and are deleted after protocol deletion.
+
+        When a protocol is deleted, federated_user records for that protocol
+        are CASCADE deleted. Users that had no other name source should be
+        cleaned up.
+        """
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        display_name = 'federated_orphan_protocol_user'
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name=display_name,
+        )
+
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        with sql.session_for_read() as session:
+            # Verify the user exists before protocol deletion
+            u = session.get(identity_model.User, user_id)
+            self.assertIsNotNone(u)
+
+        # Delete the protocol, which should CASCADE delete federated_user
+        # records and then clean up the orphaned user
+        PROVIDERS.federation_api.delete_protocol(
+            self.idp['id'], self.protocol['id']
+        )
+
+        # Clear the cache so get_user doesn't return stale data
+        self.identity_api.get_user.invalidate()
+
+        # The orphaned user should no longer exist
+        self.assertRaises(
+            exception.UserNotFound, self.identity_api.get_user, user_id
+        )
+
+    def test_orphaned_federated_users_cleaned_up_after_idp_deletion(self):
+        """Verify federated_user records are cleaned up after IDP deletion.
+
+        Some databases (SQLite) do not enforce FK CASCADE, so federated_user
+        records whose IDP is deleted should be explicitly cleaned up.
+        """
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        display_name = 'federated_cleanup_test'
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name=display_name,
+        )
+
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        # Verify federated_user exists before IDP deletion
+        with sql.session_for_read() as session:
+            fed_count = (
+                session.query(identity_model.FederatedUser)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(1, fed_count)
+
+        # Delete the IDP
+        PROVIDERS.federation_api.delete_idp(self.idp['id'])
+
+        # Verify federated_user record was cleaned up even though
+        # SQLite doesn't enforce FK CASCADE by default
+        with sql.session_for_read() as session:
+            fed_count = (
+                session.query(identity_model.FederatedUser)
+                .filter_by(idp_id=self.idp['id'])
+                .count()
+            )
+            self.assertEqual(
+                0,
+                fed_count,
+                'federated_user records for deleted IDP must be cleaned up',
+            )
+
+    def test_orphaned_federated_users_cleaned_up_after_protocol_deletion(self):
+        """Verify federated_user records are cleaned up after protocol deletion.
+
+        Some databases (SQLite) do not enforce FK CASCADE, so federated_user
+        records whose protocol is deleted should be explicitly cleaned up.
+        """
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        display_name = 'federated_cleanup_protocol_test'
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name=display_name,
+        )
+
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        # Verify federated_user exists before protocol deletion
+        with sql.session_for_read() as session:
+            fed_count = (
+                session.query(identity_model.FederatedUser)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(1, fed_count)
+
+        # Delete the protocol
+        PROVIDERS.federation_api.delete_protocol(
+            self.idp['id'], self.protocol['id']
+        )
+
+        # Verify federated_user record was cleaned up
+        with sql.session_for_read() as session:
+            fed_count = (
+                session.query(identity_model.FederatedUser)
+                .filter_by(protocol_id=self.protocol['id'])
+                .count()
+            )
+            self.assertEqual(
+                0,
+                fed_count,
+                'federated_user records for deleted protocol must be cleaned up',
+            )
+
+    def test_user_group_membership_cleaned_on_orphan_user_deletion(self):
+        """Verify UserGroupMembership rows are deleted before orphaned users.
+
+        UserGroupMembership.user_id has FK on user.id WITHOUT ondelete='CASCADE'.
+        Query.delete() bypasses ORM-level cascades, so we must explicitly clean up
+        dependent rows before deleting orphaned users to avoid FK constraint errors.
+        """
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        # Create a federated user
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name='group_membership_test',
+        )
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        # Create a group and membership
+        group = unit.new_group_ref(domain_id=self.domain_id)
+        group = PROVIDERS.identity_api.create_group(group)
+        group_id = group['id']
+
+        # Add membership
+        PROVIDERS.identity_api.add_user_to_group(user_id, group_id)
+
+        # Verify membership exists
+        with sql.session_for_read() as session:
+            membership_count = (
+                session.query(identity_model.UserGroupMembership)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(1, membership_count)
+
+        # Clear the cache so get_user doesn't return stale data
+        self.identity_api.get_user.invalidate()
+
+        # Delete the IDP, which triggers orphan user cleanup
+        PROVIDERS.federation_api.delete_idp(self.idp['id'])
+
+        # Verify user was deleted
+        self.assertRaises(
+            exception.UserNotFound, self.identity_api.get_user, user_id
+        )
+
+        # Verify UserGroupMembership row was cleaned up alongside the user
+        with sql.session_for_read() as session:
+            membership_count = (
+                session.query(identity_model.UserGroupMembership)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(
+                0,
+                membership_count,
+                'UserGroupMembership rows must be cleaned up before '
+                'orphaned user deletion',
+            )
+
+    def test_expiring_user_group_membership_cleaned_on_orphan_deletion(self):
+        """Verify ExpiringUserGroupMembership rows are deleted before users.
+
+        ExpiringUserGroupMembership.user_id has FK on user.id WITHOUT
+        ondelete='CASCADE'. Query.delete() bypasses ORM-level cascades, so we
+        must explicitly clean up dependent rows before deleting orphaned users.
+        """
+        import datetime
+
+        from keystone.common import sql
+        from keystone.identity.backends import sql_model as identity_model
+
+        # Create a federated user
+        fed_dict = unit.new_federated_user_ref(
+            idp_id=self.idp['id'],
+            protocol_id=self.protocol['id'],
+            display_name='expiring_membership_test',
+        )
+        user = self.shadow_users_api.create_federated_user(
+            self.domain_id, fed_dict
+        )
+        user_id = user['id']
+
+        # Create a group
+        group = unit.new_group_ref(domain_id=self.domain_id)
+        group = PROVIDERS.identity_api.create_group(group)
+        group_id = group['id']
+
+        # Insert an ExpiringUserGroupMembership row directly
+        with sql.session_for_write() as session:
+            membership = identity_model.ExpiringUserGroupMembership(
+                user_id=user_id,
+                group_id=group_id,
+                idp_id=self.idp['id'],
+                last_verified=datetime.datetime.now(datetime.UTC),
+            )
+            session.add(membership)
+
+        # Verify membership exists
+        with sql.session_for_read() as session:
+            membership_count = (
+                session.query(identity_model.ExpiringUserGroupMembership)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(1, membership_count)
+
+        # Clear the cache so get_user doesn't return stale data
+        self.identity_api.get_user.invalidate()
+
+        # Delete the IDP, which triggers orphan user cleanup
+        PROVIDERS.federation_api.delete_idp(self.idp['id'])
+
+        # Verify user was deleted
+        self.assertRaises(
+            exception.UserNotFound, self.identity_api.get_user, user_id
+        )
+
+        # Verify ExpiringUserGroupMembership row was cleaned up
+        with sql.session_for_read() as session:
+            membership_count = (
+                session.query(identity_model.ExpiringUserGroupMembership)
+                .filter_by(user_id=user_id)
+                .count()
+            )
+            self.assertEqual(
+                0,
+                membership_count,
+                'ExpiringUserGroupMembership rows must be cleaned up '
+                'before orphaned user deletion',
+            )

keystone/tests/unit/test_v3_identity.py

@@ -1441,3 +1441,25 @@ class UserFederatedAttributesTests(test_v3.RestfulTestCase):
             body={'user': user},
             expected_status=http.client.BAD_REQUEST,
         )
+
+    def test_list_users_without_filters_returns_federated_name(self):
+        """Call ``GET /users`` without federated attribute filters.
+
+        Verifies that when list_users is called without idp_id, protocol_id,
+        or unique_id filters, the federated user still appears with a non-null
+        name (from FederatedUser.display_name). This exercises the regular
+        SQL list_users query with outerjoin(LocalUser) and relies on eager
+        loading for federated_users.
+        """
+        r = self.get('/users', token=self.get_admin_token())
+        self.assertValidUserListResponse(r)
+        found = [
+            u for u in r.result['users'] if u['id'] == self.fed_user['id']
+        ]
+        self.assertEqual(
+            1, len(found), 'Federated user should appear in list_users'
+        )
+        self.assertIsNotNone(
+            found[0]['name'],
+            'list_users must not return None for federated user name',
+        )