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

Announce deleted devices explicitly over federation. #3520

Merged
merged 7 commits into from
Jul 23, 2018

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Jul 12, 2018

Adds a new deleted flag on the m.device_list_update EDU contents to notify when a device has been deleted. This should be nicely backwards compatible with existing servers.

Specced at the bottom of https://docs.google.com/document/d/1fNBZUeMlp0fn0en5bCji5fn6mSvj48UylWfGKrk8ZIw/edit#

Fixes element-hq/element-web#4527
Sytest: matrix-org/sytest#463

ara4n added 2 commits July 12, 2018 01:32
Previously we queued up the poke correctly when the device was deleted,
but then the actual EDU wouldn't get sent, as the device was no longer known.
Instead, we now send EDUs for deleted devices too if there's a poke for them.
@ara4n ara4n requested a review from a team July 12, 2018 10:44
@ara4n
Copy link
Member Author

ara4n commented Jul 12, 2018

@matrixbot retest this please

},
)

# Do we need this?
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not have to go and figure this out myself...

it seems odd that it would be needed here and not in the upsert case.

Copy link
Member Author

Choose a reason for hiding this comment

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

i believe it is needed, as that cache is used to track device existence and we're deleting a device here. whereas in the upsert case we're not deleting a device.

Copy link
Member

Choose a reason for hiding this comment

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

ok, the critical piece of information here was that device_id_exists_cache only caches the presence of devices, not the absence of them, which is why we don't need to update it when we insert a new device.

"""Fetch a list of device keys.
Args:
query_list(list): List of pairs of user_ids and device_ids.
include_all_devices (bool): whether to include entries for devices
that don't have device keys
include_deleted_devices (bool): whether to include null entries for
devices which no longer exist (but were in the query_list)
Copy link
Member

Choose a reason for hiding this comment

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

how does this interact with include_all_devices? can haz doc pls

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@richvdh
Copy link
Member

richvdh commented Jul 19, 2018

(this is also blocked on getting some agreement on exactly what the format of the EDUs on the wire is)

@ara4n
Copy link
Member Author

ara4n commented Jul 19, 2018

looks like i completely misread the code of how the EDU payloads were structured; my bad. Thanks for digging in and fixing it.

ptal?

@ara4n ara4n assigned richvdh and unassigned ara4n Jul 19, 2018
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

def get_e2e_device_keys(self, query_list, include_all_devices=False):
def get_e2e_device_keys(
self, query_list, include_all_devices=False,
include_deleted_devices=False
Copy link
Member

Choose a reason for hiding this comment

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

trailing comma would be nice here

@richvdh richvdh assigned ara4n and unassigned richvdh Jul 19, 2018
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