From 49cc81458dd13a6871f26f901c0215453a4f1251 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Tue, 12 Oct 2021 12:39:38 +0200
Subject: [PATCH 1/7] Fix setting a user's external_id via the admin API
 returns

500 and deletes users existing external mappings if that external ID is
already mapped
---
 synapse/rest/admin/users.py                   |  20 +-
 .../storage/databases/main/registration.py    |  94 +++++++-
 tests/rest/admin/test_user.py                 | 215 +++++++++++++++++-
 3 files changed, 305 insertions(+), 24 deletions(-)

diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index f20aa6530145..2b7a535fda80 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -282,21 +282,11 @@ async def on_PUT(
                 add_external_ids = new_external_ids - cur_external_ids
                 del_external_ids = cur_external_ids - new_external_ids
 
-                # remove old external_ids
-                for auth_provider, external_id in del_external_ids:
-                    await self.store.remove_user_external_id(
-                        auth_provider,
-                        external_id,
-                        user_id,
-                    )
-
-                # add new external_ids
-                for auth_provider, external_id in add_external_ids:
-                    await self.store.record_user_external_id(
-                        auth_provider,
-                        external_id,
-                        user_id,
-                    )
+                await self.store.record_and_remove_user_external_id(
+                    add_external_ids,
+                    del_external_ids,
+                    user_id,
+                )
 
             if "avatar_url" in body and isinstance(body["avatar_url"], str):
                 await self.profile_handler.set_avatar_url(
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index 7de4ad7f9b3c..89135562a10b 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -16,14 +16,18 @@
 import logging
 import random
 import re
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union
+from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Tuple, Union
 
 import attr
 
 from synapse.api.constants import UserTypes
 from synapse.api.errors import Codes, StoreError, SynapseError, ThreepidValidationError
 from synapse.metrics.background_process_metrics import wrap_as_background_process
-from synapse.storage.database import DatabasePool, LoggingDatabaseConnection
+from synapse.storage.database import (
+    DatabasePool,
+    LoggingDatabaseConnection,
+    LoggingTransaction,
+)
 from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore
 from synapse.storage.databases.main.stats import StatsStore
 from synapse.storage.types import Connection, Cursor
@@ -589,14 +593,30 @@ async def record_user_external_id(
             external_id: id on that system
             user_id: complete mxid that it is mapped to
         """
-        await self.db_pool.simple_insert(
+        await self.db_pool.runInteraction(
+            "record_user_external_id",
+            self._record_user_external_id_txn,
+            auth_provider,
+            external_id,
+            user_id,
+        )
+
+    def _record_user_external_id_txn(
+        self,
+        txn: LoggingTransaction,
+        auth_provider: str,
+        external_id: str,
+        user_id: str,
+    ) -> None:
+
+        self.db_pool.simple_insert_txn(
+            txn,
             table="user_external_ids",
             values={
                 "auth_provider": auth_provider,
                 "external_id": external_id,
                 "user_id": user_id,
             },
-            desc="record_user_external_id",
         )
 
     async def remove_user_external_id(
@@ -611,16 +631,78 @@ async def remove_user_external_id(
             external_id: id on that system
             user_id: complete mxid that it is mapped to
         """
-        await self.db_pool.simple_delete(
+        await self.db_pool.runInteraction(
+            "remove_user_external_id",
+            self._remove_user_external_id_txn,
+            auth_provider,
+            external_id,
+            user_id,
+        )
+
+    def _remove_user_external_id_txn(
+        self,
+        txn: LoggingTransaction,
+        auth_provider: str,
+        external_id: str,
+        user_id: str,
+    ) -> None:
+
+        self.db_pool.simple_delete_txn(
+            txn,
             table="user_external_ids",
             keyvalues={
                 "auth_provider": auth_provider,
                 "external_id": external_id,
                 "user_id": user_id,
             },
-            desc="remove_user_external_id",
         )
 
+    async def record_and_remove_user_external_id(
+        self,
+        record_external_ids: Set[Tuple[str, str]],
+        remove_external_ids: Set[Tuple[str, str]],
+        user_id: str,
+    ) -> None:
+        """Record and remove mappings from external user ids to a mxid
+        in a single transaction.
+
+        Args:
+            record_external_ids:
+                set with tuple of auth_provider and external_id to record
+            remove_external_ids:
+                set with tuple of auth_provider and external_id to remove
+            user_id: complete mxid that it is mapped to
+        Raises:
+            StoreError if the new external_id could not be mapped.
+        """
+
+        def _record_and_remove_user_external_id_txn(
+            txn: LoggingTransaction,
+        ):
+            for auth_provider, external_id in record_external_ids:
+                self._record_user_external_id_txn(
+                    txn,
+                    auth_provider,
+                    external_id,
+                    user_id,
+                )
+
+            for auth_provider, external_id in remove_external_ids:
+                self._remove_user_external_id_txn(
+                    txn,
+                    auth_provider,
+                    external_id,
+                    user_id,
+                )
+
+        try:
+            await self.db_pool.runInteraction(
+                "record_and_remove_user_external_id",
+                _record_and_remove_user_external_id_txn,
+            )
+        except self.database_engine.module.IntegrityError:
+            raise StoreError(409, "External id already in use.")
+
     async def get_user_by_external_id(
         self, auth_provider: str, external_id: str
     ) -> Optional[str]:
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 6ed9e421732b..9eb2d7e435ca 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -1196,9 +1196,8 @@ def prepare(self, reactor, clock, hs):
                 self.other_user, device_id=None, valid_until_ms=None
             )
         )
-        self.url_other_user = "/_synapse/admin/v2/users/%s" % urllib.parse.quote(
-            self.other_user
-        )
+        self.url_prefix = "/_synapse/admin/v2/users/%s"
+        self.url_other_user = self.url_prefix % self.other_user
 
     def test_requester_is_no_admin(self):
         """
@@ -1754,6 +1753,93 @@ def test_set_threepid(self):
         self.assertEqual(0, len(channel.json_body["threepids"]))
         self._check_fields(channel.json_body)
 
+    def test_set_duplicate_threepid(self):
+        """
+        Test setting the same threepid for a second user.
+        First user loses and second user gets mapping of this threepid.
+        """
+
+        # create a user to set a threepid
+        first_user = self.register_user("first_user", "pass")
+        url_first_user = self.url_prefix % first_user
+
+        # Add threepid to first user
+        channel = self.make_request(
+            "PUT",
+            url_first_user,
+            access_token=self.admin_user_tok,
+            content={
+                "threepids": [
+                    {"medium": "email", "address": "bob1@bob.bob"},
+                ],
+            },
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(first_user, channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["threepids"]))
+        self.assertEqual("email", channel.json_body["threepids"][0]["medium"])
+        self.assertEqual("bob1@bob.bob", channel.json_body["threepids"][0]["address"])
+        self._check_fields(channel.json_body)
+
+        # Add threepids to other user
+        channel = self.make_request(
+            "PUT",
+            self.url_other_user,
+            access_token=self.admin_user_tok,
+            content={
+                "threepids": [
+                    {"medium": "email", "address": "bob2@bob.bob"},
+                ],
+            },
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual("@user:test", channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["threepids"]))
+        self.assertEqual("email", channel.json_body["threepids"][0]["medium"])
+        self.assertEqual("bob2@bob.bob", channel.json_body["threepids"][0]["address"])
+        self._check_fields(channel.json_body)
+
+        # Add two new threepids to other user
+        # one is used by first_user
+        channel = self.make_request(
+            "PUT",
+            self.url_other_user,
+            access_token=self.admin_user_tok,
+            content={
+                "threepids": [
+                    {"medium": "email", "address": "bob1@bob.bob"},
+                    {"medium": "email", "address": "bob3@bob.bob"},
+                ],
+            },
+        )
+
+        # other user has this two threepids
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual("@user:test", channel.json_body["name"])
+        self.assertEqual(2, len(channel.json_body["threepids"]))
+        # result does not always have the same sort order, therefore it becomes sorted
+        sorted_result = sorted(
+            channel.json_body["threepids"], key=lambda k: k["address"]
+        )
+        self.assertEqual("email", sorted_result[0]["medium"])
+        self.assertEqual("bob1@bob.bob", sorted_result[0]["address"])
+        self.assertEqual("email", sorted_result[1]["medium"])
+        self.assertEqual("bob3@bob.bob", sorted_result[1]["address"])
+        self._check_fields(channel.json_body)
+
+        # first_user has no threepid anymore
+        channel = self.make_request(
+            "GET",
+            url_first_user,
+            access_token=self.admin_user_tok,
+        )
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(first_user, channel.json_body["name"])
+        self.assertEqual(0, len(channel.json_body["threepids"]))
+        self._check_fields(channel.json_body)
+
     def test_set_external_id(self):
         """
         Test setting external id for an other user.
@@ -1852,6 +1938,129 @@ def test_set_external_id(self):
         self.assertEqual("@user:test", channel.json_body["name"])
         self.assertEqual(0, len(channel.json_body["external_ids"]))
 
+    def test_set_duplicate_external_id(self):
+        """
+        Test that setting the same external id for a second user fails and
+        external id from user must not be changed.
+        """
+
+        # create a user to use an external id
+        first_user = self.register_user("first_user", "pass")
+        url_first_user = self.url_prefix % first_user
+
+        # Add an external id to first user
+        channel = self.make_request(
+            "PUT",
+            url_first_user,
+            access_token=self.admin_user_tok,
+            content={
+                "external_ids": [
+                    {
+                        "external_id": "external_id1",
+                        "auth_provider": "auth_provider",
+                    },
+                ],
+            },
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(first_user, channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["external_ids"]))
+        self.assertEqual(
+            "external_id1", channel.json_body["external_ids"][0]["external_id"]
+        )
+        self.assertEqual(
+            "auth_provider", channel.json_body["external_ids"][0]["auth_provider"]
+        )
+        self._check_fields(channel.json_body)
+
+        # Add an external id to other user
+        channel = self.make_request(
+            "PUT",
+            self.url_other_user,
+            access_token=self.admin_user_tok,
+            content={
+                "external_ids": [
+                    {
+                        "external_id": "external_id2",
+                        "auth_provider": "auth_provider",
+                    },
+                ],
+            },
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual("@user:test", channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["external_ids"]))
+        self.assertEqual(
+            "external_id2", channel.json_body["external_ids"][0]["external_id"]
+        )
+        self.assertEqual(
+            "auth_provider", channel.json_body["external_ids"][0]["auth_provider"]
+        )
+        self._check_fields(channel.json_body)
+
+        # Add two new external_ids to other user
+        # one is used by first
+        channel = self.make_request(
+            "PUT",
+            self.url_other_user,
+            access_token=self.admin_user_tok,
+            content={
+                "external_ids": [
+                    {
+                        "external_id": "external_id1",
+                        "auth_provider": "auth_provider",
+                    },
+                    {
+                        "external_id": "external_id3",
+                        "auth_provider": "auth_provider",
+                    },
+                ],
+            },
+        )
+
+        # must fail
+        self.assertEqual(409, channel.code, msg=channel.json_body)
+        self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
+        self.assertEqual("External id already in use.", channel.json_body["error"])
+
+        # other user must not changed
+        channel = self.make_request(
+            "GET",
+            self.url_other_user,
+            access_token=self.admin_user_tok,
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual("@user:test", channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["external_ids"]))
+        self.assertEqual(
+            "external_id2", channel.json_body["external_ids"][0]["external_id"]
+        )
+        self.assertEqual(
+            "auth_provider", channel.json_body["external_ids"][0]["auth_provider"]
+        )
+        self._check_fields(channel.json_body)
+
+        # first user must not changed
+        channel = self.make_request(
+            "GET",
+            url_first_user,
+            access_token=self.admin_user_tok,
+        )
+
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(first_user, channel.json_body["name"])
+        self.assertEqual(1, len(channel.json_body["external_ids"]))
+        self.assertEqual(
+            "external_id1", channel.json_body["external_ids"][0]["external_id"]
+        )
+        self.assertEqual(
+            "auth_provider", channel.json_body["external_ids"][0]["auth_provider"]
+        )
+        self._check_fields(channel.json_body)
+
     def test_deactivate_user(self):
         """
         Test deactivating another user.

From 7fd4c4970cbdc6396d96881bae59656794591f00 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Tue, 12 Oct 2021 13:29:32 +0200
Subject: [PATCH 2/7] newsfile

---
 changelog.d/11051.bugfix | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/11051.bugfix

diff --git a/changelog.d/11051.bugfix b/changelog.d/11051.bugfix
new file mode 100644
index 000000000000..63126843d27b
--- /dev/null
+++ b/changelog.d/11051.bugfix
@@ -0,0 +1 @@
+Fix a bug where setting a user's external_id via the admin API returns 500 and deletes users existing external mappings if that external ID is already mapped.
\ No newline at end of file

From caedb57b1c36a0861becd91d5656233c6f930fd9 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Wed, 13 Oct 2021 17:11:38 +0200
Subject: [PATCH 3/7] add own exception

---
 synapse/rest/admin/users.py                    | 14 +++++++++-----
 synapse/storage/databases/main/registration.py | 10 ++++++++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 2b7a535fda80..4d3ab27e16c9 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -35,6 +35,7 @@
     assert_user_is_admin,
 )
 from synapse.rest.client._base import client_patterns
+from synapse.storage.databases.main.registration import ExternalIDReuseException
 from synapse.storage.databases.main.stats import UserSortOrder
 from synapse.types import JsonDict, UserID
 
@@ -282,11 +283,14 @@ async def on_PUT(
                 add_external_ids = new_external_ids - cur_external_ids
                 del_external_ids = cur_external_ids - new_external_ids
 
-                await self.store.record_and_remove_user_external_id(
-                    add_external_ids,
-                    del_external_ids,
-                    user_id,
-                )
+                try:
+                    await self.store.record_and_remove_user_external_id(
+                        add_external_ids,
+                        del_external_ids,
+                        user_id,
+                    )
+                except ExternalIDReuseException:
+                    raise SynapseError(409, "External id already in use.")
 
             if "avatar_url" in body and isinstance(body["avatar_url"], str):
                 await self.profile_handler.set_avatar_url(
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index 89135562a10b..7cabb17890c7 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -44,6 +44,12 @@
 logger = logging.getLogger(__name__)
 
 
+class ExternalIDReuseException(Exception):
+    """Exception if writing an external id for a user fails,
+    because this external id is given to an other user."""
+    pass
+
+
 @attr.s(frozen=True, slots=True)
 class TokenLookupResult:
     """Result of looking up an access token.
@@ -673,7 +679,7 @@ async def record_and_remove_user_external_id(
                 set with tuple of auth_provider and external_id to remove
             user_id: complete mxid that it is mapped to
         Raises:
-            StoreError if the new external_id could not be mapped.
+            ExternalIDReuseException if the new external_id could not be mapped.
         """
 
         def _record_and_remove_user_external_id_txn(
@@ -701,7 +707,7 @@ def _record_and_remove_user_external_id_txn(
                 _record_and_remove_user_external_id_txn,
             )
         except self.database_engine.module.IntegrityError:
-            raise StoreError(409, "External id already in use.")
+            raise ExternalIDReuseException()
 
     async def get_user_by_external_id(
         self, auth_provider: str, external_id: str

From 949b1ae7009b9d8d325b84aea95ba01125a80330 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Wed, 13 Oct 2021 18:43:05 +0200
Subject: [PATCH 4/7] rewrite db calls

- rename `record_and_remove_user_external_id` to
`replace_user_external_id`
- modify `replace_user_external_id` to delete all mappings and add given
mapping
- restore `remove_user_external_id` to orign, no `_txn` call is needed
- add `remove_user_external_ids` to delete all mappings with one sql
query
- also add `_remove_user_external_ids_txn`
- change rest handler to use new functions
---
 synapse/rest/admin/users.py                   | 18 ++---
 .../storage/databases/main/registration.py    | 70 ++++++++++---------
 2 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 4d3ab27e16c9..951ad4b03762 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -229,12 +229,12 @@ async def on_PUT(
         if not isinstance(deactivate, bool):
             raise SynapseError(400, "'deactivated' parameter is not of type boolean")
 
-        # convert List[Dict[str, str]] into Set[Tuple[str, str]]
+        # convert List[Dict[str, str]] into List[Tuple[str, str]]
         if external_ids is not None:
-            new_external_ids = {
+            new_external_ids = [
                 (external_id["auth_provider"], external_id["external_id"])
                 for external_id in external_ids
-            }
+            ]
 
         # convert List[Dict[str, str]] into Set[Tuple[str, str]]
         if threepids is not None:
@@ -276,17 +276,9 @@ async def on_PUT(
                     )
 
             if external_ids is not None:
-                # get changed external_ids (added and removed)
-                cur_external_ids = set(
-                    await self.store.get_external_ids_by_user(user_id)
-                )
-                add_external_ids = new_external_ids - cur_external_ids
-                del_external_ids = cur_external_ids - new_external_ids
-
                 try:
-                    await self.store.record_and_remove_user_external_id(
-                        add_external_ids,
-                        del_external_ids,
+                    await self.store.replace_user_external_id(
+                        new_external_ids,
                         user_id,
                     )
                 except ExternalIDReuseException:
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index 7cabb17890c7..3c259c260995 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -16,7 +16,7 @@
 import logging
 import random
 import re
-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Set, Tuple, Union
+from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union
 
 import attr
 
@@ -47,6 +47,7 @@
 class ExternalIDReuseException(Exception):
     """Exception if writing an external id for a user fails,
     because this external id is given to an other user."""
+
     pass
 
 
@@ -629,62 +630,71 @@ async def remove_user_external_id(
         self, auth_provider: str, external_id: str, user_id: str
     ) -> None:
         """Remove a mapping from an external user id to a mxid
-
         If the mapping is not found, this method does nothing.
-
         Args:
             auth_provider: identifier for the remote auth provider
             external_id: id on that system
             user_id: complete mxid that it is mapped to
         """
+        await self.db_pool.simple_delete(
+            table="user_external_ids",
+            keyvalues={
+                "auth_provider": auth_provider,
+                "external_id": external_id,
+                "user_id": user_id,
+            },
+            desc="remove_user_external_id",
+        )
+
+    async def remove_user_external_ids(
+        self, auth_provider: str, external_id: str, user_id: str
+    ) -> None:
+        """Remove all mappings from external user ids to a mxid
+
+        If these mappings are not found, this method does nothing.
+
+        Args:
+            user_id: complete mxid that it is mapped to
+        """
         await self.db_pool.runInteraction(
-            "remove_user_external_id",
-            self._remove_user_external_id_txn,
-            auth_provider,
-            external_id,
+            "remove_user_external_ids",
+            self._remove_user_external_ids_txn,
             user_id,
         )
 
-    def _remove_user_external_id_txn(
+    def _remove_user_external_ids_txn(
         self,
         txn: LoggingTransaction,
-        auth_provider: str,
-        external_id: str,
         user_id: str,
     ) -> None:
 
         self.db_pool.simple_delete_txn(
             txn,
             table="user_external_ids",
-            keyvalues={
-                "auth_provider": auth_provider,
-                "external_id": external_id,
-                "user_id": user_id,
-            },
+            keyvalues={"user_id": user_id},
         )
 
-    async def record_and_remove_user_external_id(
+    async def replace_user_external_id(
         self,
-        record_external_ids: Set[Tuple[str, str]],
-        remove_external_ids: Set[Tuple[str, str]],
+        record_external_ids: List[Tuple[str, str]],
         user_id: str,
     ) -> None:
-        """Record and remove mappings from external user ids to a mxid
-        in a single transaction.
+        """Replace mappings from external user ids to a mxid in a single transaction.
+        All mappings are deleted and the new ones are created.
 
         Args:
             record_external_ids:
-                set with tuple of auth_provider and external_id to record
-            remove_external_ids:
-                set with tuple of auth_provider and external_id to remove
+                List with tuple of auth_provider and external_id to record
             user_id: complete mxid that it is mapped to
         Raises:
             ExternalIDReuseException if the new external_id could not be mapped.
         """
 
-        def _record_and_remove_user_external_id_txn(
+        def _replace_user_external_id_txn(
             txn: LoggingTransaction,
         ):
+            self._remove_user_external_ids_txn(txn, user_id)
+
             for auth_provider, external_id in record_external_ids:
                 self._record_user_external_id_txn(
                     txn,
@@ -693,18 +703,10 @@ def _record_and_remove_user_external_id_txn(
                     user_id,
                 )
 
-            for auth_provider, external_id in remove_external_ids:
-                self._remove_user_external_id_txn(
-                    txn,
-                    auth_provider,
-                    external_id,
-                    user_id,
-                )
-
         try:
             await self.db_pool.runInteraction(
-                "record_and_remove_user_external_id",
-                _record_and_remove_user_external_id_txn,
+                "replace_user_external_id",
+                _replace_user_external_id_txn,
             )
         except self.database_engine.module.IntegrityError:
             raise ExternalIDReuseException()

From f6a2092949a0cda7fc1023e3e3c5c40cdc1e96e3 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Wed, 13 Oct 2021 18:51:13 +0200
Subject: [PATCH 5/7] add exception handling to `record_user_external_id` and

create user in admin API
---
 synapse/rest/admin/users.py                   | 17 +++++++++-------
 .../storage/databases/main/registration.py    | 20 ++++++++++++-------
 tests/rest/admin/test_user.py                 |  2 +-
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 951ad4b03762..c0bebc3cf0f5 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -282,7 +282,7 @@ async def on_PUT(
                         user_id,
                     )
                 except ExternalIDReuseException:
-                    raise SynapseError(409, "External id already in use.")
+                    raise SynapseError(409, "External id is already in use.")
 
             if "avatar_url" in body and isinstance(body["avatar_url"], str):
                 await self.profile_handler.set_avatar_url(
@@ -370,12 +370,15 @@ async def on_PUT(
                         )
 
             if external_ids is not None:
-                for auth_provider, external_id in new_external_ids:
-                    await self.store.record_user_external_id(
-                        auth_provider,
-                        external_id,
-                        user_id,
-                    )
+                try:
+                    for auth_provider, external_id in new_external_ids:
+                        await self.store.record_user_external_id(
+                            auth_provider,
+                            external_id,
+                            user_id,
+                        )
+                except ExternalIDReuseException:
+                    raise SynapseError(409, "External id is already in use.")
 
             if "avatar_url" in body and isinstance(body["avatar_url"], str):
                 await self.profile_handler.set_avatar_url(
diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index 3c259c260995..bd838b557a9b 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -599,14 +599,20 @@ async def record_user_external_id(
             auth_provider: identifier for the remote auth provider
             external_id: id on that system
             user_id: complete mxid that it is mapped to
+        Raises:
+            ExternalIDReuseException if the new external_id could not be mapped.
         """
-        await self.db_pool.runInteraction(
-            "record_user_external_id",
-            self._record_user_external_id_txn,
-            auth_provider,
-            external_id,
-            user_id,
-        )
+
+        try:
+            await self.db_pool.runInteraction(
+                "record_user_external_id",
+                self._record_user_external_id_txn,
+                auth_provider,
+                external_id,
+                user_id,
+            )
+        except self.database_engine.module.IntegrityError:
+            raise ExternalIDReuseException()
 
     def _record_user_external_id_txn(
         self,
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 9eb2d7e435ca..ba49b91081c7 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2023,7 +2023,7 @@ def test_set_duplicate_external_id(self):
         # must fail
         self.assertEqual(409, channel.code, msg=channel.json_body)
         self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
-        self.assertEqual("External id already in use.", channel.json_body["error"])
+        self.assertEqual("External id is already in use.", channel.json_body["error"])
 
         # other user must not changed
         channel = self.make_request(

From e40bcd431b5f7e6b5ee45b47b3946f365f346d83 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 14 Oct 2021 17:37:15 +0200
Subject: [PATCH 6/7] move functions

---
 .../storage/databases/main/registration.py    | 48 ++++++++-----------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index bd838b557a9b..ef96f34b7166 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -652,34 +652,6 @@ async def remove_user_external_id(
             desc="remove_user_external_id",
         )
 
-    async def remove_user_external_ids(
-        self, auth_provider: str, external_id: str, user_id: str
-    ) -> None:
-        """Remove all mappings from external user ids to a mxid
-
-        If these mappings are not found, this method does nothing.
-
-        Args:
-            user_id: complete mxid that it is mapped to
-        """
-        await self.db_pool.runInteraction(
-            "remove_user_external_ids",
-            self._remove_user_external_ids_txn,
-            user_id,
-        )
-
-    def _remove_user_external_ids_txn(
-        self,
-        txn: LoggingTransaction,
-        user_id: str,
-    ) -> None:
-
-        self.db_pool.simple_delete_txn(
-            txn,
-            table="user_external_ids",
-            keyvalues={"user_id": user_id},
-        )
-
     async def replace_user_external_id(
         self,
         record_external_ids: List[Tuple[str, str]],
@@ -696,10 +668,28 @@ async def replace_user_external_id(
             ExternalIDReuseException if the new external_id could not be mapped.
         """
 
+        def _remove_user_external_ids_txn(
+            self,
+            txn: LoggingTransaction,
+            user_id: str,
+        ) -> None:
+            """Remove all mappings from external user ids to a mxid
+            If these mappings are not found, this method does nothing.
+
+            Args:
+                user_id: complete mxid that it is mapped to
+            """
+
+            self.db_pool.simple_delete_txn(
+                txn,
+                table="user_external_ids",
+                keyvalues={"user_id": user_id},
+            )
+
         def _replace_user_external_id_txn(
             txn: LoggingTransaction,
         ):
-            self._remove_user_external_ids_txn(txn, user_id)
+            _remove_user_external_ids_txn(txn, user_id)
 
             for auth_provider, external_id in record_external_ids:
                 self._record_user_external_id_txn(

From fa32735d089190f0788dab553682bca6b9a82730 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 14 Oct 2021 17:48:22 +0200
Subject: [PATCH 7/7] mypy

---
 synapse/storage/databases/main/registration.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py
index ef96f34b7166..28f21f16c050 100644
--- a/synapse/storage/databases/main/registration.py
+++ b/synapse/storage/databases/main/registration.py
@@ -669,7 +669,6 @@ async def replace_user_external_id(
         """
 
         def _remove_user_external_ids_txn(
-            self,
             txn: LoggingTransaction,
             user_id: str,
         ) -> None: