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

Adding Key Update Before Every New GroupIntent #658

Closed
wants to merge 34 commits into from
Closed

Conversation

lfzkoala
Copy link

@lfzkoala lfzkoala commented Apr 15, 2024

trying to solve this issue - #347

This is what I'm doing

Follow the readme file (xmtp_mls/migrations) to create a migration for SQL database

Install the CLI onto local system (one-time)
cargo install diesel_cli --no-default-features --features sqlite

Create migration SQL
diesel migration generate key_update

Edit the up.sql and down.sql files created
Put in up.sql
ALTER TABLE key_update ADD COLUMN rotated_at_ns BIGINT NOT NULL DEFAULT 0
Leave down.sql blank

Generate application code
cargo run --bin update-schema
This updates the generated schema.rs file. This updates models.rs to reference it and encrypted_store/mod.rs to define queries against the model.

Add a new item called rotated_at_ns in the StoredGroup struct in storage/encrypted_store/group.rs

Write a function called get_rotated_time_checked, which is similar to the function get_installations_time_checked, in order to fetch the rotated_at_ns time stamp.

Then in the create_from_welcome function, set the rotated_at_ns = 0 when creating the StoredGroup.

Write an async function called pre_intent_hook(), which does the following

  • Rotate the key if checking the rotated_at_ns == 0
  • Call maybe_update_installations()
    Before every GroupIntent::New(...), call the pre_intent_hook() function.

Add a boolean flag as an input of the maybe_update_installations function, set the flag to be true to skip the hook when it is called inside from the pre_intent_hook() function. This is intended to avoid infinite loops and make sure it is not called before the key update intent.

@lfzkoala lfzkoala requested a review from a team as a code owner April 15, 2024 14:59
@bwcDvorak bwcDvorak added the inbox-id Support for Inbox ID label Apr 15, 2024
@neekolas neekolas removed the inbox-id Support for Inbox ID label May 3, 2024
lfzkoala and others added 2 commits May 6, 2024 04:32
…- both scripts can run from this directory instead of libxmtp/
@lfzkoala lfzkoala requested a review from humanagent as a code owner May 13, 2024 16:04
@richardhuaaa
Copy link
Contributor

richardhuaaa commented May 20, 2024

Idea for test 1:

  • Client a makes a group with client B
  • Client B creates it from welcome
  • Verify no new payloads on client A
  • Call pre_intent_hook on client B
  • Verify client A receives a key rotation payload
    • Check number of messages == 1
    • Check it is a key rotation - look at consume_message() for example. Need to decrypt the message, get the commit, and check it has an update proposal
  • Call pre_intent_hook on client B again
  • Verify client A receives nothing new
  • Register a new client for the same user as client A
  • Call pre_intent_hook on client A
  • Verify client B receives both a key rotation for client A and a payload to add the new installation

Idea for test 2:
Client a makes a group with client B
Client b creates it from welcome
Verify no new payloads on client A
Client b sends a message to client A
Verify client A receives a key rotation and then the message

xmtp_mls/src/groups/mod.rs Outdated Show resolved Hide resolved
xmtp_mls/src/groups/mod.rs Outdated Show resolved Hide resolved
@richardhuaaa richardhuaaa requested a review from a team as a code owner October 1, 2024 00:11
@richardhuaaa richardhuaaa marked this pull request as draft October 1, 2024 00:12
@richardhuaaa
Copy link
Contributor

Please see #1108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client App inbox-id Support for Inbox ID
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants