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

Recreate the group member list if we don't know the parent message #3782

Closed
Hocuri opened this issue Nov 28, 2022 · 3 comments
Closed

Recreate the group member list if we don't know the parent message #3782

Hocuri opened this issue Nov 28, 2022 · 3 comments
Assignees

Comments

@Hocuri
Copy link
Collaborator

Hocuri commented Nov 28, 2022

@adbenitez is constantly having problems with inconsistent group state in his big groups.

Currently, the logic for adding & removing members in apply_group_changes() is:

  • If the message has a Chat-Group-Member-Removed header, then we completely recreate the group member list (i.e. use the member list of the incoming message).
  • If the message has a Chat-Group-Member-Added, then we recreate the group member list, but don't remove anyone (i.e. add recipients of the incoming messages as members).

This empirically doesn't work well.

Instead, we should try this:

  • If the message has a Chat-Group-Member-Removed: member@example.com header, then remove this member
  • If the message has a Chat-Group-Member-Added: member@example.com header, then add this member
  • If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list (i.e. use the member list of the incoming message) (because we assume that we missed some messages & have a wrong group state).
    • Also, we should show an info message to the user when we implicitly add/remove members, currently we don't do this

This will also often help if someone imports an old backup with an old group state, because then they will get messages where they don't know the parent message, and apply the correct group state.

@hpk42
Copy link
Contributor

hpk42 commented Dec 4, 2022

Hasn't @adbenitez given up on using large groups and now uses mailman/mailing-list relayed groups instead of letting everyone know everyone else in the group?
Even if so, it's probably still fine to tackle this issue but noting down why inconsistent groups can exist (lost member-added messages, concurrency around group-adding/group-removal) would be better than blaming adb for it :)

@Hocuri
Copy link
Collaborator Author

Hocuri commented Dec 5, 2022

I actually opened this issue right after adb told about an old group which he can't leave because of this problem. But, yes.

@Hocuri
Copy link
Collaborator Author

Hocuri commented Jun 8, 2023

Fixed by #4185

@Hocuri Hocuri closed this as completed Jun 8, 2023
iequidoo added a commit that referenced this issue Aug 31, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove all checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a
  message take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 2, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The current/older versions of DC
  don't always set "In-Reply-To" to the latest message they sent (it depends on timings), so we need
  this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 2, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The current/older versions of DC
  don't always set "In-Reply-To" to the latest message they sent (it depends on timings), so we need
  this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 2, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The current/older versions of DC
  don't always set "In-Reply-To" to the latest message they sent (it depends on timings), so we need
  this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 6, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The older versions of DC don't
  always set "In-Reply-To" to the latest message they sent, but to the latest delivered message (so
  it's a race), so we need this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 6, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The older versions of DC don't
  always set "In-Reply-To" to the latest message they sent, but to the latest delivered message (so
  it's a race), so we need this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 6, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The older versions of DC don't
  always set "In-Reply-To" to the latest message they sent, but to the latest delivered message (so
  it's a race), so we need this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 9, 2023
…3782)(#4624)

- If we don't know the parent (=In-Reply-To) message, then completely recreate the group member list
  (i.e. use the member list of the incoming message) (because we assume that we missed some messages
  & have a wrong group state).
- If the message has a "Chat-Group-Member-Removed: member@example.com" header, then remove this
  member.
- If the message has a "Chat-Group-Member-Added: member@example.com" header, then add this member.

That means:
- Remove checks for the presense of `ContactId::SELF` in the group. Thus all recipients of a message
  take the same decision about group membership changes, no matter if they are in the group
  currently. This fixes a situation when a recipient thinks it's not a member because it missed a
  message about its addition before.
  NOTE: But always recreate membership list if SELF has been added. The older versions of DC don't
  always set "In-Reply-To" to the latest message they sent, but to the latest delivered message (so
  it's a race), so we need this heuristic currently.
- Recreate the group member list if we don't know the parent (=In-Reply-To) message, even if the
  sender isn't in the group as per our view, because we missed some messages and our view may be
  stale.
iequidoo added a commit that referenced this issue Sep 11, 2023
…on-members (#3782)

It can be not good for membership consistency if we missed a message adding a member, but improves
security because nobody can add themselves to a group from now on.
iequidoo added a commit that referenced this issue Sep 11, 2023
…on-members (#3782)

It can be not good for membership consistency if we missed a message adding a member, but improves
security because nobody can add themselves to a group from now on.
iequidoo added a commit that referenced this issue Sep 12, 2023
…on-members (#3782)

It can be not good for membership consistency if we missed a message adding a member, but improves
security because nobody can add themselves to a group from now on.
iequidoo added a commit that referenced this issue Sep 12, 2023
…membership consistency (#3782)

It is better for privacy. It shouldn't be a big problem because if somebody missed a member
addition, they will likely recreate the member list on the next message. The problem occurs only if
that "somebody" managed to reply before. Really, it's a problem for big groups with high message
rate, but let it be for now.

Also:
- Query chat contacts from the db only once.
- Update chat contacts in the only transaction, otherwise we can just break the chat contact list
  halfway.
- Allow classic MUA messages to remove group members if a parent message is missing. Currently it
  doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside
  of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header
  f.e.
iequidoo added a commit that referenced this issue Sep 12, 2023
…membership consistency (#3782)

It is better for privacy. It shouldn't be a big problem because if somebody missed a member
addition, they will likely recreate the member list on the next message. The problem occurs only if
that "somebody" managed to reply before. Really, it's a problem for big groups with high message
rate, but let it be for now.

Also:
- Query chat contacts from the db only once.
- Update chat contacts in the only transaction, otherwise we can just break the chat contact list
  halfway.
- Allow classic MUA messages to remove group members if a parent message is missing. Currently it
  doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside
  of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header
  f.e.
iequidoo added a commit that referenced this issue Sep 14, 2023
…rship consistency (#3782)

9bd7ab7 brings a possibility of group membership inconsistency to the original Hocuri's algo
described and implemented in e12e026 in sake of security so that nobody can add themselves to a
group by forging "InReplyTo" and other headers. This commit fixes the problem by removing group
members locally if we see a discrepancy with the "To" list in the received message as it is better
for privacy than adding absent members locally. But it shouldn't be a big problem if somebody missed
a member addition, because they will likely recreate the member list from the next received
message. The problem occurs only if that "somebody" managed to reply earlier. Really, it's a problem
for big groups with high message rate, but let it be for now.

Also:
- Query chat contacts from the db only once.
- Update chat contacts in the only transaction, otherwise we can just break the chat contact list
  halfway.
- Allow classic MUA messages to remove group members if a parent message is missing. Currently it
  doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside
  of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header
  f.e.
iequidoo added a commit that referenced this issue Sep 27, 2023
…rship consistency (#3782)

9bd7ab7 brings a possibility of group membership inconsistency to the original Hocuri's algo
described and implemented in e12e026 in sake of security so that nobody can add themselves to a
group by forging "InReplyTo" and other headers. This commit fixes the problem by removing group
members locally if we see a discrepancy with the "To" list in the received message as it is better
for privacy than adding absent members locally. But it shouldn't be a big problem if somebody missed
a member addition, because they will likely recreate the member list from the next received
message. The problem occurs only if that "somebody" managed to reply earlier. Really, it's a problem
for big groups with high message rate, but let it be for now.

Also:
- Query chat contacts from the db only once.
- Update chat contacts in the only transaction, otherwise we can just break the chat contact list
  halfway.
- Allow classic MUA messages to remove group members if a parent message is missing. Currently it
  doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside
  of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header
  f.e.
iequidoo added a commit that referenced this issue Sep 30, 2023
…rship consistency (#3782)

9bd7ab7 brings a possibility of group membership inconsistency to the original Hocuri's algo
described and implemented in e12e026 in sake of security so that nobody can add themselves to a
group by forging "InReplyTo" and other headers. This commit fixes the problem by removing group
members locally if we see a discrepancy with the "To" list in the received message as it is better
for privacy than adding absent members locally. But it shouldn't be a big problem if somebody missed
a member addition, because they will likely recreate the member list from the next received
message. The problem occurs only if that "somebody" managed to reply earlier. Really, it's a problem
for big groups with high message rate, but let it be for now.

Also:
- Query chat contacts from the db only once.
- Update chat contacts in the only transaction, otherwise we can just break the chat contact list
  halfway.
- Allow classic MUA messages to remove group members if a parent message is missing. Currently it
  doesn't matter because unrelated messages go to new ad-hoc groups, but let this logic be outside
  of apply_group_changes(). Just in case if there will be a MUA preserving "Chat-Group-ID" header
  f.e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants