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

Self-removal/muting support MVP #441

Open
Tracked by #693
neekolas opened this issue Jan 22, 2024 · 5 comments
Open
Tracked by #693

Self-removal/muting support MVP #441

neekolas opened this issue Jan 22, 2024 · 5 comments

Comments

@neekolas
Copy link
Contributor

neekolas commented Jan 22, 2024

Added by Rich:
MLS allows you to publish proposals and commits. In some systems, the proposals are sent separately, and then a later commit is published by someone else that commits all of the previous proposals. In our current implementation, all proposals are immediately committed by the proposer, so we don't use separate proposal messages, just commits that include the proposals.

The problem with removing yourself is that MLS prohibits commiting a Remove proposal that removes the committer. I think the reason is that generating commits requires deriving secrets to use for the next epoch of the group, and therefore anyone being removed from the group should not have access to those secrets.

It seems there are two ways to solve this problem:

  1. Add support for separating out proposals and commits. So to remove yourself, you publish a proposal, and then somebody else commits it. This requires a bunch of changes to how we do proposals/commits today. We will need to chat about how to design this.

  2. You remove yourself from the group, but don't update the group encryption state (in essence, you've effectively 'muted' the group without changing the group membership). I don't like this so much, because you still need a way to notify your other devices you've done this, otherwise you're only muting for your current device.

@richardhuaaa
Copy link
Contributor

richardhuaaa commented Apr 23, 2024

Here's how this task could be done using floating proposals (no longer recommended):

There are two parties here, the ‘proposer’ and the ‘committer’.

The proposer needs to create an intent, and make sure that the proposal makes it onto the group successfully, before deleting the group on their end. Logic to add for the proposer:

  1. Add another membership state like LEFT here: https://github.com/xmtp/libxmtp/blob/main/xmtp_mls/src/storage/encrypted_store/group.rs#L192
  2. Create a IntentKind::LeaveGroup intent type here.
  3. Cmd+F for IntentKind::RemoveMembers, and follow the same lifecycle:
    1. Just like remove_members(), add a method called leave_group() that constructs the LeaveGroup intent and then syncs.
    2. In get_publish_intent_data(), add another match arm for LeaveGroup. There is no IntentData to fetch - you know that you are removing all installations with your own wallet address. Use the same logic to gather the list of leaf nodes to remove, then build a list of proposals (not a commit) to remove them. Return this from get_publish_intent_data and it will be sent on the group.
      1. When saving the payload hash on the intent, we can just hash the first proposal, so that it's easier to match against later.
    3. In process_own_message(), add another case for IntentKind::LeaveGroup. Extract the PrivateMessageIn from the message: ProtocolMessage param, then decrypt it via openmls_group.process_message(). If the decryption fails, call conn.set_group_intent_to_publish() to reset the intent. If the decryption is successful, our proposal has been successfully synced, and we can go ahead and update the group membership state to LEFT. (conn.set_group_intent_committed() will automatically be called at the end of the function)
      1. We can update the group here because we don't care if the proposal is committed or not. We've done our end by making sure the proposal is correctly published, it's up to the rest of the group to commit it.
  4. We need to somehow expose that the group is 'left' to the bindings. Perhaps we can add an optional filter to whatever method lists groups if it doesn't already exist, and make sure the membership state is accessible to the bindings.

Once the committer receives the proposal, they can 'lazy commit' it the next time they interact with the group. Logic to add for the committer:

  1. In process_external_message(), fill in the match arm for proposal messages:
    1. Validate that the proposal is an attempt by a member to leave, and that it's someone who has permission to leave, discarding the proposal if not. You could consider using this.
    2. Store the pending proposal as described here. The next commit we send will automatically include the proposal, as described here.
  2. The very next thing we send could be a message, not a commit. This is what OpenMLS wants us to do in that case. We actually have a TODO to implement that right here. We should expect that this won't create any Welcome messages, because the only pending proposal type we support is for leaving a group.
  3. Somebody might 'beat us to the punch' and commit the proposal before us. That will come to us as a commit, eventually reaching here. We need to make sure that this commit validates successfully, so we need to modify this validation logic. It is okay for a commit to have multiple actors, if everything with a different actor is a 'leave group' proposal. You could consider using this.
  4. If we were the one to commit the proposal, we also need to make sure we process our own commit correctly, via process_own_message(). For both process_own_message() and process_external_message(), we need to make sure a transcript message is generated and written.

Ideally the member list is updated immediately once the proposal is received. It is also possible to commit immediately, rather than just lazily, but it is additional work. Stretch:

  1. You could consider making a CommitPendingProposals intent whenever a LeaveGroup proposal is received. You'd have to implement the intent lifecycle like you did for IntentKind::LeaveGroup. Or, you could publish pending proposals whenever Group.members() is called.

@richardhuaaa
Copy link
Contributor

richardhuaaa commented Apr 23, 2024

I wrote all of this up, but with XIP-46 coming, I just realized it may actually be easier to wait for that, and have the proposer just propose an update to the members list on the group context, and have the committer handle resolving the list of installations to remove. That might simplify the above significantly. In fact, it may be worth considering rolling this into the identity work.

@richardhuaaa
Copy link
Contributor

richardhuaaa commented Jun 5, 2024

Simpler metadata-based approach without floating proposals:

  1. Commit a metadata change to remove your inbox ID - use a new field called something like 'pending_removals'
  2. When any other installation processes metadata changes:
    1. If they remove yourself, go ahead and update the group state to LEFT_GROUP
    2. If they remove someone else, publish a commit to remove all of their installations from the group

Reason for this is there are some complexities with doing it through floating proposals:

  • Do you queue proposals for both the metadata and the removal, or just the removal? Or just propose the removal, and metadata update is up to the committer?
  • What if the removed installations in the proposal doesn’t match the full list?
  • What if someone adds installations after the proposal is added?
  • What if the proposal is valid at the time it was created, but not when it was committed?
  • How do your other installations know that they’ve been removed?
    • With floating proposals: When processing proposals, if they propose removing you, mark yourself as having left the group
    • With committed metadata: Use metadata as source of truth

@nplasterer
Copy link
Contributor

Related to ephemeraHQ/converse-app#931

@nplasterer
Copy link
Contributor

PR started here: #1090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Status: Todo
Development

No branches or pull requests

4 participants