forked from MirrorHub/synapse
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 (#11051)
Fixes #10846
This commit is contained in:
parent
57501d9194
commit
ef7fe09778
1
changelog.d/11051.bugfix
Normal file
1
changelog.d/11051.bugfix
Normal file
|
@ -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.
|
|
@ -35,6 +35,7 @@ from synapse.rest.admin._base import (
|
||||||
assert_user_is_admin,
|
assert_user_is_admin,
|
||||||
)
|
)
|
||||||
from synapse.rest.client._base import client_patterns
|
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.storage.databases.main.stats import UserSortOrder
|
||||||
from synapse.types import JsonDict, UserID
|
from synapse.types import JsonDict, UserID
|
||||||
|
|
||||||
|
@ -228,12 +229,12 @@ class UserRestServletV2(RestServlet):
|
||||||
if not isinstance(deactivate, bool):
|
if not isinstance(deactivate, bool):
|
||||||
raise SynapseError(400, "'deactivated' parameter is not of type boolean")
|
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:
|
if external_ids is not None:
|
||||||
new_external_ids = {
|
new_external_ids = [
|
||||||
(external_id["auth_provider"], external_id["external_id"])
|
(external_id["auth_provider"], external_id["external_id"])
|
||||||
for external_id in external_ids
|
for external_id in external_ids
|
||||||
}
|
]
|
||||||
|
|
||||||
# convert List[Dict[str, str]] into Set[Tuple[str, str]]
|
# convert List[Dict[str, str]] into Set[Tuple[str, str]]
|
||||||
if threepids is not None:
|
if threepids is not None:
|
||||||
|
@ -275,28 +276,13 @@ class UserRestServletV2(RestServlet):
|
||||||
)
|
)
|
||||||
|
|
||||||
if external_ids is not None:
|
if external_ids is not None:
|
||||||
# get changed external_ids (added and removed)
|
try:
|
||||||
cur_external_ids = set(
|
await self.store.replace_user_external_id(
|
||||||
await self.store.get_external_ids_by_user(user_id)
|
new_external_ids,
|
||||||
)
|
|
||||||
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,
|
user_id,
|
||||||
)
|
)
|
||||||
|
except ExternalIDReuseException:
|
||||||
|
raise SynapseError(409, "External id is already in use.")
|
||||||
|
|
||||||
if "avatar_url" in body and isinstance(body["avatar_url"], str):
|
if "avatar_url" in body and isinstance(body["avatar_url"], str):
|
||||||
await self.profile_handler.set_avatar_url(
|
await self.profile_handler.set_avatar_url(
|
||||||
|
@ -384,12 +370,15 @@ class UserRestServletV2(RestServlet):
|
||||||
)
|
)
|
||||||
|
|
||||||
if external_ids is not None:
|
if external_ids is not None:
|
||||||
for auth_provider, external_id in new_external_ids:
|
try:
|
||||||
await self.store.record_user_external_id(
|
for auth_provider, external_id in new_external_ids:
|
||||||
auth_provider,
|
await self.store.record_user_external_id(
|
||||||
external_id,
|
auth_provider,
|
||||||
user_id,
|
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):
|
if "avatar_url" in body and isinstance(body["avatar_url"], str):
|
||||||
await self.profile_handler.set_avatar_url(
|
await self.profile_handler.set_avatar_url(
|
||||||
|
|
|
@ -23,7 +23,11 @@ import attr
|
||||||
from synapse.api.constants import UserTypes
|
from synapse.api.constants import UserTypes
|
||||||
from synapse.api.errors import Codes, StoreError, SynapseError, ThreepidValidationError
|
from synapse.api.errors import Codes, StoreError, SynapseError, ThreepidValidationError
|
||||||
from synapse.metrics.background_process_metrics import wrap_as_background_process
|
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.cache import CacheInvalidationWorkerStore
|
||||||
from synapse.storage.databases.main.stats import StatsStore
|
from synapse.storage.databases.main.stats import StatsStore
|
||||||
from synapse.storage.types import Cursor
|
from synapse.storage.types import Cursor
|
||||||
|
@ -40,6 +44,13 @@ THIRTY_MINUTES_IN_MS = 30 * 60 * 1000
|
||||||
logger = logging.getLogger(__name__)
|
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)
|
@attr.s(frozen=True, slots=True)
|
||||||
class TokenLookupResult:
|
class TokenLookupResult:
|
||||||
"""Result of looking up an access token.
|
"""Result of looking up an access token.
|
||||||
|
@ -588,24 +599,44 @@ class RegistrationWorkerStore(CacheInvalidationWorkerStore):
|
||||||
auth_provider: identifier for the remote auth provider
|
auth_provider: identifier for the remote auth provider
|
||||||
external_id: id on that system
|
external_id: id on that system
|
||||||
user_id: complete mxid that it is mapped to
|
user_id: complete mxid that it is mapped to
|
||||||
|
Raises:
|
||||||
|
ExternalIDReuseException if the new external_id could not be mapped.
|
||||||
"""
|
"""
|
||||||
await self.db_pool.simple_insert(
|
|
||||||
|
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,
|
||||||
|
txn: LoggingTransaction,
|
||||||
|
auth_provider: str,
|
||||||
|
external_id: str,
|
||||||
|
user_id: str,
|
||||||
|
) -> None:
|
||||||
|
|
||||||
|
self.db_pool.simple_insert_txn(
|
||||||
|
txn,
|
||||||
table="user_external_ids",
|
table="user_external_ids",
|
||||||
values={
|
values={
|
||||||
"auth_provider": auth_provider,
|
"auth_provider": auth_provider,
|
||||||
"external_id": external_id,
|
"external_id": external_id,
|
||||||
"user_id": user_id,
|
"user_id": user_id,
|
||||||
},
|
},
|
||||||
desc="record_user_external_id",
|
|
||||||
)
|
)
|
||||||
|
|
||||||
async def remove_user_external_id(
|
async def remove_user_external_id(
|
||||||
self, auth_provider: str, external_id: str, user_id: str
|
self, auth_provider: str, external_id: str, user_id: str
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Remove a mapping from an external user id to a mxid
|
"""Remove a mapping from an external user id to a mxid
|
||||||
|
|
||||||
If the mapping is not found, this method does nothing.
|
If the mapping is not found, this method does nothing.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
auth_provider: identifier for the remote auth provider
|
auth_provider: identifier for the remote auth provider
|
||||||
external_id: id on that system
|
external_id: id on that system
|
||||||
|
@ -621,6 +652,60 @@ class RegistrationWorkerStore(CacheInvalidationWorkerStore):
|
||||||
desc="remove_user_external_id",
|
desc="remove_user_external_id",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
async def replace_user_external_id(
|
||||||
|
self,
|
||||||
|
record_external_ids: List[Tuple[str, str]],
|
||||||
|
user_id: str,
|
||||||
|
) -> None:
|
||||||
|
"""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:
|
||||||
|
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 _remove_user_external_ids_txn(
|
||||||
|
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,
|
||||||
|
):
|
||||||
|
_remove_user_external_ids_txn(txn, user_id)
|
||||||
|
|
||||||
|
for auth_provider, external_id in record_external_ids:
|
||||||
|
self._record_user_external_id_txn(
|
||||||
|
txn,
|
||||||
|
auth_provider,
|
||||||
|
external_id,
|
||||||
|
user_id,
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
|
await self.db_pool.runInteraction(
|
||||||
|
"replace_user_external_id",
|
||||||
|
_replace_user_external_id_txn,
|
||||||
|
)
|
||||||
|
except self.database_engine.module.IntegrityError:
|
||||||
|
raise ExternalIDReuseException()
|
||||||
|
|
||||||
async def get_user_by_external_id(
|
async def get_user_by_external_id(
|
||||||
self, auth_provider: str, external_id: str
|
self, auth_provider: str, external_id: str
|
||||||
) -> Optional[str]:
|
) -> Optional[str]:
|
||||||
|
|
|
@ -1180,9 +1180,8 @@ class UserRestTestCase(unittest.HomeserverTestCase):
|
||||||
self.other_user, device_id=None, valid_until_ms=None
|
self.other_user, device_id=None, valid_until_ms=None
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
self.url_other_user = "/_synapse/admin/v2/users/%s" % urllib.parse.quote(
|
self.url_prefix = "/_synapse/admin/v2/users/%s"
|
||||||
self.other_user
|
self.url_other_user = self.url_prefix % self.other_user
|
||||||
)
|
|
||||||
|
|
||||||
def test_requester_is_no_admin(self):
|
def test_requester_is_no_admin(self):
|
||||||
"""
|
"""
|
||||||
|
@ -1738,6 +1737,93 @@ class UserRestTestCase(unittest.HomeserverTestCase):
|
||||||
self.assertEqual(0, len(channel.json_body["threepids"]))
|
self.assertEqual(0, len(channel.json_body["threepids"]))
|
||||||
self._check_fields(channel.json_body)
|
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):
|
def test_set_external_id(self):
|
||||||
"""
|
"""
|
||||||
Test setting external id for an other user.
|
Test setting external id for an other user.
|
||||||
|
@ -1836,6 +1922,129 @@ class UserRestTestCase(unittest.HomeserverTestCase):
|
||||||
self.assertEqual("@user:test", channel.json_body["name"])
|
self.assertEqual("@user:test", channel.json_body["name"])
|
||||||
self.assertEqual(0, len(channel.json_body["external_ids"]))
|
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 is 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):
|
def test_deactivate_user(self):
|
||||||
"""
|
"""
|
||||||
Test deactivating another user.
|
Test deactivating another user.
|
||||||
|
|
Loading…
Reference in a new issue