Fix deactivate a user if he does not have a profile

[dklimpel]

Fixes: #10240
A user should also be deactivated and mark as erased if he has no profile.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[anoadragon453]

Looks like this was introduced in #8932.

[anoadragon453]

Thanks for the test! I have a suggestion below for an alternative fix that may prevent other edge cases.

[anoadragon453]

So the reason a StoreError is raised here is due to set_profile_avatar_url performing an update on profiles rather than an upsert:

synapse/synapse/storage/databases/main/profile.py

Lines 83 to 91 in 4b965c8

	async def set_profile_avatar_url(
	self, user_localpart: str, new_avatar_url: Optional[str]
	) -> None:
	await self.db_pool.simple_update_one(
	table="profiles",
	keyvalues={"user_id": user_localpart},
	updatevalues={"avatar_url": new_avatar_url},
	desc="set_profile_avatar_url",
	)

I think a more fundamental fix for this whole problem would actually just be to convert set_profile_avatar_url to use self.db_pool.simple_upsert, like so:

        await self.db_pool.simple_upsert(
            table="profiles",
            keyvalues={"user_id": user_localpart},
            values={},
            insertion_values={"avatar_url": new_avatar_url},
            desc="set_profile_avatar_url",
        )

it'd probably be a good idea to do the same for set_profile_displayname as well, just to avoid the same issue in the future.

[dklimpel]

Ok. I will make an update.

[anoadragon453]

You can actually just pass the Python dict directory to make_request these days.

[dklimpel]

Looks like this was introduced in #8932.

Yes. I didn't realise that there were users without profiles.

[richvdh]

why do we have users without profiles?

[dklimpel]

why do we have users without profiles?

If you create a user without displayname

synapse/synapse/storage/databases/main/registration.py

Lines 1563 to 1575 in f0e02f5

	def _register_user(
	self,
	txn,
	user_id: str,
	password_hash: Optional[str],
	was_guest: bool,
	make_guest: bool,
	appservice_id: Optional[str],
	create_profile_with_displayname: Optional[str],
	admin: bool,
	user_type: Optional[str],
	shadow_banned: bool,
	):

no profile will be created

synapse/synapse/storage/databases/main/registration.py

Lines 1629 to 1638 in f0e02f5

	if create_profile_with_displayname:
	# set a default displayname serverside to avoid ugly race
	# between auto-joins and clients trying to set displaynames
	#
	# *obviously* the 'profiles' table uses localpart for user_id
	# while everything else uses the full mxid.
	txn.execute(
	"INSERT INTO profiles(user_id, displayname) VALUES (?,?)",
	(user_id_obj.localpart, create_profile_with_displayname),
	)

Weird. A default value is normally set.

[dklimpel]

why do we have users without profiles?

#10240 (comment)

[richvdh]

why do we have users without profiles?

If you create a user without displayname ... no profile will be created

ugh.

[richvdh]

lgtm, thanks!