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

Make /sync attempt to return device updates for both joined and invited users #3484

Merged
merged 8 commits into from
May 16, 2019

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Jul 4, 2018

This means that we can encrypt for invited users' devices before that user joins the room if the client wants.

[was intended for element-hq/element-web#2713, but does not actually fix it]

Fwiw the clientside patches required for matrix-js-sdk and matrix-react-sdk are: matrix-org/matrix-js-sdk#666 and matrix-org/matrix-react-sdk#2042.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Looks plausible to me - I can't think of any reason it would be any more complex than this

ara4n added a commit that referenced this pull request Jul 6, 2018
Previously we only queried the device list when the user joined the room; now we
do it when they are invited too.  This means that new messages can be encrypted
for the devices of the invited user as of the point they were invited.

WARNING: This commit has two major problems however:
 1. If the invited user adds devices after being invited but before joining, the
    device-list will not be updated to the other servers in the room (as we don't
    know who those servers are).
 2. This introduces a regression, as previously the device-list would be correctly
    updated when when user joined the room.  However, this resync doesn't happen
    now, so devices which joined after the invite and before the join may never
    be added to the device-list.

This is being merged for DINSIC given the edge case of adding devices between
invite & join is pretty rare in their use case, but before it can be merged to
synapse in general we need to at least re-sync the devicelist when the user joins
or to implement some kind of pubsub mechanism to let interested servers subscribe
to devicelist updates on other servers irrespective of user join/invite membership.

This was originally #3484
@ara4n
Copy link
Member Author

ara4n commented Jul 6, 2018

As per matrix-org/matrix-js-sdk#666 this doesn't work, and in fact introduces a slight regression, as invited users who add devices between being invited and joining will no longer ever tell the room participants about these new devices (whereas previously they would be synced when the user joined the room).

This has been merged for dinsic (via the matthew/dinsic-encrypt-for-invited-users branch) but before we merge this to develop, we need to either work out how to trigger a resync of the devicelist when the user transitions from invite->join, or have some other way of synchronising devicelists implemented.

@ara4n
Copy link
Member Author

ara4n commented Jul 10, 2018

Turns out this doesn't introduce a regression, as clients resync the devicelist by querying /unstable/keys/query every time the membership list changes. However, it also doesn't help anything either due to #3503.

There's a sytest that asserts that /keys/query works for invited users (which it did already, apparently), and then a broken sytest for #3503 committed at matrix-org/sytest#462.

@richvdh
Copy link
Member

richvdh commented Jul 26, 2018

I'm a bit confused about what's going on here. I think it needs more work?

@ara4n
Copy link
Member Author

ara4n commented Aug 12, 2018

not sure what to do with this.

my hunch is probably to merge it to avoid repetition of work in future, even though in practice for it to actually improve things a bit it would need #3503 to be fixed and a solution for #3504 to be found.

@richvdh richvdh requested review from dbkr and richvdh and removed request for dbkr May 10, 2019 08:19
@richvdh
Copy link
Member

richvdh commented May 15, 2019

ok, for clarity: this does not fix element-hq/element-web#2713. Will update the description accordingly so that it hopefully doesn't get auto-resolved.

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.

seems plausible I think.

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #3484 into develop will not change coverage.
The diff coverage is 62.5%.

@@           Coverage Diff            @@
##           develop    #3484   +/-   ##
========================================
  Coverage    62.16%   62.16%           
========================================
  Files          336      336           
  Lines        34686    34686           
  Branches      5679     5679           
========================================
  Hits         21562    21562           
  Misses       11590    11590           
  Partials      1534     1534

@richvdh richvdh changed the title maintain device lists for both joined and invited users so we can encrypt for invited users. (attempt to) maintain device lists for both joined and invited users May 16, 2019
@richvdh richvdh changed the title (attempt to) maintain device lists for both joined and invited users Make /sync attempt to return device updates for both joined and invited users May 16, 2019
@richvdh richvdh merged commit 4a6d5de into develop May 16, 2019
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.

3 participants