Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implements part of MSC3944 by dropping cancelled&duplicated m.room_key_request #15842

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Jun 26, 2023

MSC3944

  • m.room_key_request with action: request, if there is a corresponding m.room_key_request message with action: request_cancellation and the same request_id and requesting_device_id fields, sent by the same user after the request was made. If the request message is dropped, the cancellation message is dropped as well. Room key requests can use the same transaction ID if they are requesting the same room key, so a request could be made, then cancelled, and then re-requested. Thus the request(s) sent before the cancellation should be dropped, but the request (if any) sent after the cancellation should be kept.
  • m.room_key_request with action: request, if there are other identical requests that are currently in the devices inbox, sent before this request. Room key requests can use the same transaction ID if they are requesting the same room key. This can happen, for example, if a key gets requested, and later re-requested. However, if a device was offline during the initial request and has not received it yet, it is redundant for it to receive both requests. The server only needs to keep the most recent copy (unless it has been cancelled - see above - in which case it does not need to keep any copy).

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.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Related to #3599.

@MatMaul MatMaul changed the title Implements bullets 1&2 of MSC 3944 Implements bullets 1&2 of MSC 3944 by dropping cancelled&duplicated m.room_key_request Jun 26, 2023
@MatMaul MatMaul force-pushed the mv/msc3944 branch 4 times, most recently from e635be7 to 2a61c8b Compare June 27, 2023 07:49
@MatMaul MatMaul changed the title Implements bullets 1&2 of MSC 3944 by dropping cancelled&duplicated m.room_key_request Implements part of MSC 3944 by dropping cancelled&duplicated m.room_key_request Jun 27, 2023
@MatMaul MatMaul marked this pull request as ready for review June 27, 2023 08:05
@MatMaul MatMaul requested a review from a team as a code owner June 27, 2023 08:05
@clokep clokep changed the title Implements part of MSC 3944 by dropping cancelled&duplicated m.room_key_request Implements part of MSC3944 by dropping cancelled&duplicated m.room_key_request Jun 27, 2023
if (
self._msc3944_enabled
and message_type == ToDeviceEventTypes.RoomKeyRequest
and user_id == sender_user_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious as to why the check is here for user_id == sender_user_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to drop only your own key requests (not the one from the others) but since MSC3944 is pretty strict in what we drop (unlike my previous PR) I think you are right and it's safe to remove this condition.

if await self.store.delete_device_message(
stream_id
):
previous_request_deleted = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct but the nested ifs and fors on lines ~264-296 are a bit mind-melting to parse, is it possible to refactor or encapsulate some more of the logic so it's easier to keep track of what's happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it's a lot of imbricated if/for. I'll try to do better.

return False
return True

async def get_all_device_messages(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any concern about performance with this function if the device inbox is very large? Presumably we can't just grab the RoomKeyRequests because that information is locked away in the Json blobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the perf I am not sure. get_messages_for_user_devices is kind of unbounded too.

I can narrow down the set of keys by using a LIKE with the key type on the JSON blob. Is it worth it, I am not sure.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants