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.

Sign up for GitHub

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

Jump to bottom

Add sender HMAC to MessageV2 #164

Merged
merged 31 commits into from
Mar 1, 2024
Merged

Add sender HMAC to MessageV2 #164

merged 31 commits into from
Mar 1, 2024

Conversation

giovasdistillery
Copy link
Contributor

added shouldPush to ContentCodec type
added shouldPush method to TextCodec and CompositeCodec
added senderHmac to MessageV2
added encryption methods for calculating HMAC signature

@giovasdistillery giovasdistillery requested a review from a team as a code owner January 26, 2024 16:40
@giovasdistillery giovasdistillery linked an issue Jan 26, 2024 that may be closed by this pull request
Add sender HMAC to MessageV2 #155
Closed
@giovasdistillery giovasdistillery marked this pull request as draft January 26, 2024 16:41
nplasterer
nplasterer reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

I pushed up a commit to hopefully unblock you. I think adding a test to make sure we can get the shouldPush method like we can with fallbacks would be much appreciated. 🙏

library/src/main/java/org/xmtp/android/library/codecs/ReactionCodec.kt Outdated Show resolved Hide resolved
library/src/main/java/org/xmtp/android/library/messages/MessageV2.kt Outdated Show resolved Hide resolved
library/src/main/java/org/xmtp/android/library/messages/MessageV2.kt Show resolved Hide resolved
@giovasdistillery giovasdistillery marked this pull request as ready for review February 8, 2024 08:19
nplasterer
nplasterer reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@nplasterer nplasterer left a comment

Choose a reason for hiding this comment

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

This is looking really good. Just have some concerns about the cryptography. Also Rich mentioned that this might be missing the method getV2ConversationHmacKeys() https://github.com/xmtp/xmtp-js/pull/519/files#diff-3f8d138711ab8a38267eef7baab8b362cbe3deee7fda5e18689bae9c2db74bb9

library/src/main/java/org/xmtp/android/library/Crypto.kt Show resolved Hide resolved
@nplasterer nplasterer requested a review from Bren2010 February 13, 2024 19:03
@nplasterer
Copy link
Contributor

Merging in @kele-leanes #184 to unblock RN looks to have broken a few places in the code. We can revert this PR if there is a better approach to how to support RN.

rygine
rygine reviewed Feb 20, 2024
View reviewed changes
library/src/main/java/org/xmtp/android/library/Conversations.kt
fun getHmacKeys(
request: Keystore.GetConversationHmacKeysRequest? = null,
): Keystore.GetConversationHmacKeysResponse {
val thirtyDayPeriodsSinceEpoch = (Date().time / 1000 / 60 / 60 / 24 / 30).toInt()
Copy link

Choose a reason for hiding this comment

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

is there a reason this is different than the previously used value?

Instant.now().epochSecond / 60 / 60 / 24 / 30

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just copying how you did it here: https://github.com/xmtp/xmtp-js/pull/519/files#diff-3f8d138711ab8a38267eef7baab8b362cbe3deee7fda5e18689bae9c2db74bb9R306

giovas17 and others added 14 commits February 27, 2024 22:17
@giovas17
Adding the way for adding should push and hmac
25664a0
@giovas17
Adding new approach for codecs and crypto
bda8a7a
@giovas17
Solving test for conversation using new flag
8286a3f
@giovas17
Update ConversationV2.kt
4001857
@nplasterer
do kotlin way of extending proto class
fd117ab
@giovas17
Adding the proper way to manage the codec changes
92a5712
@giovas17
Update GroupMembershipChangeCodec.kt
97ea0c0
@nplasterer @Bren2010
Update library/src/main/java/org/xmtp/android/library/Crypto.kt
696e229 
Co-authored-by: Brendan McMillion <[email protected]>
@nplasterer @Bren2010
Update library/src/main/java/org/xmtp/android/library/Crypto.kt
241992a 
Co-authored-by: Brendan McMillion <[email protected]>
@nplasterer @Bren2010
Update library/src/main/java/org/xmtp/android/library/Crypto.kt
33d50f3 
Co-authored-by: Brendan McMillion <[email protected]>
@nplasterer
add method to get the hmac keys
fd46f81
@nplasterer
fix up the crypto
deec4b0
@nplasterer
add the get keys code for hmacs
c985c6a
@nplasterer
remove unneeded crypto code
2127f77
nplasterer and others added 16 commits February 27, 2024 22:18
@nplasterer
write a test for it
ca3416c
@nplasterer
get the test to pass
dd29da4
feat: integrate `shouldPush' for React Native (#184)
866b5a3 
* Add shouldPush property to SendOptions

* Add shouldPush property to DecryptedMessage class

* Add shouldPush parameter to ConversationV2 and MessageV2Builder
@giovas17
Update GroupMembershipChangeTest.kt
0279d00
@giovas17
Removing issues and failing tests
578e4b9
@giovas17
Update GroupMembershipChangeTest.kt
c5d97df
@giovas17
Updating shouldPush flag and removing issues in instrumental testing
50a413c
@giovas17
Ignoring failing tests
2f67cef
@nplasterer
remove all the ignored tests
2763b7e
feat: add shouldPush property to MessageV2Builder
e113e3b
@nplasterer
fix up the lint issue
df578cf
fix: deriveKey function and improvements
f37d913
fix: getHmacKeys method
7f9394c
fix: CodecText.kt
b04fc1e
Revert "feat: integrate `shouldPush' for React Native (#184)"
6f14b68 
This reverts commit 887f1f4.
Revert "Removing issues and failing tests"
5050c30 
This reverts commit e6cecc7.
@kele-leanes kele-leanes force-pushed the gg/enhancement/add_sender_hmac_to_message_v2 branch from 9d57243 to 5050c30 Compare February 28, 2024 01:22
@nplasterer nplasterer merged commit 037b85b into main Mar 1, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the gg/enhancement/add_sender_hmac_to_message_v2 branch March 1, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nplasterer nplasterer nplasterer approved these changes

@Bren2010 Bren2010 Awaiting requested review from Bren2010

@rygine rygine Awaiting requested review from rygine

@kele-leanes kele-leanes Awaiting requested review from kele-leanes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sender HMAC to MessageV2
6 participants
@giovasdistillery @nplasterer @Bren2010 @rygine @kele-leanes @giovas17