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

[anoadragon453]

Attempting to use the create or modify user account admin api to add an external ID to a user that is already mapped to another user fails with a unique constraint db error. It should really return a HTTP 400 error instead.

In addition, any existing external IDs for the user that we were trying to modify will be erased (all entries for that user are removed from the user_external_ids table. This is due to the fact that the removal and addition of old and new IDs are separate actions:

synapse/synapse/rest/admin/users.py

Lines 285 to 299 in 220f901

	# 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,
	)

They should really be bundled into a single database transaction in a new storage function so that it's not possible for the user to be left without any mappings.

[anoadragon453]

Link to sentry issue for those with access: https://sentry.matrix.org/sentry/synapse-modular/issues/231064/

Full traceback

  File "synapse/http/server.py", line 258, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "synapse/http/server.py", line 446, in _async_render
    callback_return = await raw_callback_return
  File "synapse/rest/admin/users.py", line 295, in on_PUT
    await self.store.record_user_external_id(
  File "synapse/storage/databases/main/registration.py", line 592, in record_user_external_id
    await self.db_pool.simple_insert(
  File "synapse/storage/database.py", line 865, in simple_insert
    await self.runInteraction(desc, self.simple_insert_txn, table, values)
  File "synapse/storage/database.py", line 686, in runInteraction
    result = await self.runWithConnection(
  File "synapse/storage/database.py", line 791, in runWithConnection
    return await make_deferred_yieldable(
  File "twisted/python/threadpool.py", line 238, in inContext
    result = inContext.theWork()  # type: ignore[attr-defined]
  File "twisted/python/threadpool.py", line 254, in <lambda>
    inContext.theWork = lambda: context.call(  # type: ignore[attr-defined]
  File "twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "twisted/python/context.py", line 83, in callWithContext
    return func(*args, **kw)
  File "twisted/enterprise/adbapi.py", line 293, in _runWithConnection
    compat.reraise(excValue, excTraceback)
  File "twisted/python/deprecate.py", line 298, in deprecatedFunction
    return function(*args, **kwargs)
  File "twisted/python/compat.py", line 404, in reraise
    raise exception.with_traceback(traceback)
  File "twisted/enterprise/adbapi.py", line 284, in _runWithConnection
    result = func(conn, *args, **kw)
  File "synapse/storage/database.py", line 786, in inner_func
    return func(db_conn, *args, **kwargs)
  File "synapse/storage/database.py", line 554, in new_transaction
    r = func(cursor, *args, **kwargs)
  File "synapse/storage/database.py", line 879, in simple_insert_txn
    txn.execute(sql, vals)
  File "synapse/storage/database.py", line 298, in execute
    self._do_execute(self.txn.execute, sql, *args)
  File "synapse/storage/database.py", line 331, in _do_execute
    return func(sql, *args)
UniqueViolation: duplicate key value violates unique constraint "user_external_ids_auth_provider_external_id_key"
DETAIL:  Key (auth_provider, external_id)=(saml, user@example.com) already exists.

[Twi1ightSparkle]

The Matrix user you tried to modify also gets the current external_id silently deleted

[anoadragon453]

@Twi1ightSparkle thanks, added.