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

Signature verification #334

Merged
merged 7 commits into from
Nov 22, 2024
Merged

Signature verification #334

merged 7 commits into from
Nov 22, 2024

Conversation

nplasterer
Copy link
Contributor

Adds the ability to verify if a signature was signed or not by a key

@nplasterer nplasterer self-assigned this Nov 21, 2024
@nplasterer nplasterer requested a review from a team as a code owner November 21, 2024 01:35
Comment on lines 373 to 379
assertEquals(
fixtures.boClient.verifySignatureWithInstallationId(
"Testing",
signature,
alixInstallationId
), true
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels weird that it's true? Should Bo really be able to verify if a signature was made by alix's installationId?

Copy link
Contributor

@codabrink codabrink left a comment

Choose a reason for hiding this comment

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

Looks good!

}
}

fun verifySignatureWithInstallationId(message: String, signature: ByteArray, installationId: String): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed this when I approved. Can we rename this function to match with the ffi client?

Copy link
Contributor

@codabrink codabrink Nov 22, 2024

Choose a reason for hiding this comment

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

Or I can rename the fn on the client if it's confusing. I'm fine with going with the nomenclature you went with on ios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's confusing for integrators since it's not clear that the installationId is the public key. I think it's okay for the names between the SDKs and libxmtp to differ. This isn't the first time I've strayed from the ffi naming in benefits of clarity in the sdk.

@nplasterer nplasterer merged commit 3416280 into main Nov 22, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/signature-verification branch November 22, 2024 18:36
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 this pull request may close these issues.

2 participants