Add support for pushing secrets and receiving secret pushes#6164
Add support for pushing secrets and receiving secret pushes#6164uhoreg wants to merge 3 commits intomatrix-org:mainfrom
Conversation
uhoreg
requested review from
poljar and
stefanceriu
and removed request for
a team
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6164 +/- ##
=======================================
Coverage 89.81% 89.82%
=======================================
Files 365 365
Lines 100480 100493 +13
Branches 100480 100493 +13
=======================================
+ Hits 90243 90264 +21
+ Misses 6702 6694 -8
Partials 3535 3535 ☔ View full report in Codecov by Sentry. |
richvdh
left a comment
richvdh
left a comment
There was a problem hiding this comment.
A couple of initial thoughts.
| /// Get all the pushed secrets with the given [`SecretName`] we have | ||
| /// currently stored. | ||
| #[cfg(feature = "experimental-push-secrets")] | ||
| async fn get_pushed_secrets_from_inbox( | ||
| &self, | ||
| secret_name: &SecretName, | ||
| ) -> Result<Vec<SecretPushContent>, Self::Error>; |
There was a problem hiding this comment.
do we not need to call this in matrix-sdk somewhere? Is that planned for a future PR?
There was a problem hiding this comment.
Yes, that is a future PR. I wrote this PR first, then worked on the js-sdk parts, and then worked on the matrix-sdk parts. I could add the matrix-sdk parts to this PR, but I kind of feel like it's cleaner keeping them separate. But I'm not entirely sure, and am willing to be convinced otherwise.
There was a problem hiding this comment.
ah, no worries. Could I ask you to try and make that sort of thing clear (e.g. in the description) for future PRs?
| /// Get all the pushed secrets with the given [`SecretName`] we have | ||
| /// currently stored. | ||
| #[cfg(feature = "experimental-push-secrets")] | ||
| async fn get_pushed_secrets_from_inbox( | ||
| &self, | ||
| secret_name: &SecretName, | ||
| ) -> Result<Vec<SecretPushContent>, Self::Error>; | ||
|
|
||
| /// Delete all the pushed secrets with the given [`SecretName`] we have | ||
| /// currently stored. | ||
| #[cfg(feature = "experimental-push-secrets")] | ||
| async fn delete_pushed_secrets_from_inbox( | ||
| &self, | ||
| secret_name: &SecretName, | ||
| ) -> Result<(), Self::Error>; |
There was a problem hiding this comment.
Is there a reason to give pushed secrets and requested secrets separate APIs? Could we not expose a single API that returns both, thus simplifying application logic? (and maybe even use the same backing store in sqlite/indexeddb, though that might involve some irritating backwards support)?
I notice that, although the current API returns a GossipedSecret which is specific to m.secret.send, the callers of said API don't actually make any use of any fields other than secret and name.
There was a problem hiding this comment.
Yes, I considered trying to combine them. It would be a breaking change. I don't know if anything else uses matrix-sdk-crypto directly, and these functions in particular, but it seems unlikely that anything would require anything other than secret or name.
There was a problem hiding this comment.
We've not been shy with breaking changes of matrix-sdk-crypto in the past.
richvdh
left a comment
richvdh
left a comment
There was a problem hiding this comment.
also, needs more CHANGELOG.md
2 participants
see MSC4385
Pushing secrets allow devices to send secrets to other devices without waiting for a secret request.
closes #6040
Signed-off-by: