Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update membership state of client user based on state events #507

Closed
agraven opened this issue Feb 28, 2022 · 3 comments
Closed

Update membership state of client user based on state events #507

agraven opened this issue Feb 28, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@agraven
Copy link
Contributor

agraven commented Feb 28, 2022

Is your feature request related to a problem? Please describe.
Currently, the sdk updates what the client user's membership state of a given room is based on what timeline segment (invited, joined and left) in the sync response events for the given room appear in. However, this leads to edge cases where the sdk interprets the membership state of the client incorrectly. This is particularly an issue for appservices.

Describe the solution you'd like
Changes to the client's membership of a room should be based on which state events appear in a sync response, not which segment of the sync response events for a room appear in.

Describe alternatives you've considered
Library consumers could presumably do some of this handling based on state events themselves, however, the private state of the sdk cannot be affected by consumers, so this would be an incomplete approach.

Additional context
The specification's section on syncing: https://spec.matrix.org/latest/client-server-api/#syncing. As far as I can tell, it does not specify how clients should keep track of their own membership.

Some comments in #505 touch on the issue as well.

@gnunicorn gnunicorn added the bug Something isn't working label Apr 21, 2022
@agraven
Copy link
Contributor Author

agraven commented May 12, 2022

After discussing the matter in #matrix-dev:matrix.org, it seems that the only scenario where it can occur that there's a membership event in a sync response that indicates that a client should have a different membership that what segment a room appears in is when there's a DAG merge issue, or a server is ill-behaved. Link to the conversation: https://matrix.to/#/!jxlRxnrZCsjpjDubDX:matrix.org/$1ulwWgB3Zc9HAXl8ZRPWHWC_tKtVdf3zzfJtWu_Xg78?via=matrix.org&via=libera.chat&via=privacytools.io

On that note, I'm a little confused by the final state event in the mock sync response in matrix-sdk-test/src/test_json/sync.rs, as it includes a room_id field when events should be what the spec calls a ClientEventWithoutRoomID. I think the intent was to have the membership apply to a different room, but I'm relatively certain serde ignores the room_id field and just deserializes it as a AnySyncStateEvent, c.f. matrix-sdk-base/src/client.rs, line 466

Relevant part of the spec https://spec.matrix.org/v1.2/client-server-api/#get_matrixclientv3sync

@jplatte
Copy link
Collaborator

jplatte commented May 17, 2022

Given the tests are fixed and the conclusion overall seems to be that the current behavior is fine assuming the server is well-behaved (which you always have to assume, right??), can this be closed?

@agraven
Copy link
Contributor Author

agraven commented May 17, 2022

The membership handling for appservices still needs to be improved, but I guess that's best to have in a separate issue, rather than editing this one

@jplatte jplatte closed this as completed May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants