The DELETE method for the unstable/org.matrix.msc3814.v1/dehydrated_device endpoint doesn't delete the device

[poljar]

Seems that the patch I provided in #15929 doesn't do what it should be doing, at least not for the DELETE method.

After trying to delete the dehydrated device a subsequent /keys/query will still return the dehydrated device.

Uploading a new dehydrated device using the PUT method might also not delete the old dehydrated device.

Steps to reproduce:

• Create a new dehydrated device using the PUT method on the unstable/org.matrix.msc3814.v1/dehydrated_device endpoint.
• Observe the device over the /keys/query endpoint.
• Delete the dehydrated device using the DELETE method on the endpoint from step 1.
• Continue observing that the device exists using the /keys/query endpoint, calling DELETE from the previous step claims that the device is deleted.

@H-Shay any chance you could take a look at this?

[H-Shay]

@H-Shay any chance you could take a look at this?

Absolutely

[H-Shay]

I think the problem is somewhere in here:

• The current DELETE endpoint calls rehydrate_device which sets the access token to the current device (current device being the device we most recently uploaded via PUT and presumably the one we are trying to delete), and then deletes everything for the previous device, as determined by whatever device the access token was set for previously:
  

  synapse/synapse/handlers/device.py

  Lines 705 to 715 in 68b2611

  	old_device_id = await self.store.set_device_for_access_token(
  	access_token, device_id
  	)
  	old_device = await self.store.get_device(user_id, old_device_id)
  	if old_device is None:
  	raise errors.NotFoundError()
  	await self.store.update_device(user_id, device_id, old_device["display_name"])
  	# can't call self.delete_device because that will clobber the
  	# access token so call the storage layer directly
  	await self.store.delete_devices(user_id, [old_device_id])
  	await self.store.delete_e2e_keys_by_device(

• If this is the first device added, this deletes everything for the current device, otherwise it deletes everything for the previous device.
• Thus if you PUT a new device and then try to delete it via the endpoint it will to fail (unless this is the very first dehydrated device for the account) to deleted the current device

Could this be solved by separating out the rehydration and deletion steps? This also raised a question for me as to what the difference between rehydrating and deleting the device is - this wasn't very clear in either MSC and it appears that in MSC3814 it is stated that "Rather than changing its device ID when it rehydrates the
device, it will keep its device ID and upload its own device keys" - which leads me to believe rehydrating and deleting are separate actions?

[poljar]

Oh I see. We (or rather I) reused the code the previous MSC added and that has incompatible semantics.

Could this be solved by separating out the rehydration and deletion steps? This also raised a question for me as to what the difference between rehydrating and deleting the device is

I think part of the problem is that "rehydration" isn't the same thing between the two MSCs. Yes we should separate those concerns. There are three steps where deletion of an existing dehydrated device is OK to happen:

1. When we consume all the to-device events from the /events endpoint (as opposed to when we start consuming, like the MSC suggested).
2. When the user PUTs a new dehydrated device.
3. When the user explicitly DELETEs the dehydrated device.

I think no. 1 is too implicit and magical. If we only support no. 2 and 3, then the mechanism becomes obvious and simple. The server has a single slot for a dehydrated device and you can insert, delete, and get a value from this slot, the getter should not remove the value.

In pseudo-rust code it becomes a simple optional: Option<DehydratedDevice>.

[H-Shay]

I've opened a PR here to hopefully fix this: #16046