feat(ffi): Allow passing in a SecretsBundle after logging in#6212
feat(ffi): Allow passing in a SecretsBundle after logging in#6212
Conversation
Merging this PR will improve performance by 67.96%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | Restore session [memory store] |
282 ms | 167.9 ms | +67.96% |
Comparing poljar/login/secret-bundle-import (6849bad) with main (acda2e8)
|
Hello, thanks, I am testing the updated API using EXA. |
Ah, thanks for testing. |
| } | ||
|
|
||
| impl SecretsBundle { | ||
| pub fn bundle_from_str(bundle: &str) -> Result<Arc<Self>, ClientError> { |
There was a problem hiding this comment.
it looks like the method bundle_from_str is not exposed in the FFI layer, can you double check @poljar ? Thanks! An alternative would be that the secrets parameter added to login_with_oidc_callback and login would be of type optional String.
There was a problem hiding this comment.
Whoops, that's true.
An alternative would be that the secrets parameter added to login_with_oidc_callback and login would be of type optional String.
That would complicate things for the codepath where we want to get the secrets bundle from a database.
I exported the methods now.
0f34947 to
f75470e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6212 +/- ##
==========================================
- Coverage 89.91% 89.89% -0.03%
==========================================
Files 372 372
Lines 102538 102565 +27
Branches 102538 102565 +27
==========================================
+ Hits 92199 92200 +1
- Misses 6785 6809 +24
- Partials 3554 3556 +2 ☔ View full report in Codecov by Sentry. |
|
I am providing the secrets to the function with oidc, and at the end the session is not verified. I believe that this is supposed to be the case? Happy to provide some logs if it helps. |
Hmm, I read through the code again, it should give you a verified device in the end. If it's not too much trouble logs could help, though I think I'll need to make an integration test for this and move the logic into the SDK. |
|
Oh wait, I did another mistake. Did you perhaps test this with an OIDC login? If so I forgot to connect things for the OIDC login. |
Yes |
Ok, my bad: 44a92c3 fixes this for the OIDC login. |
|
Thanks @poljar, I confirm that the session is now verified when using MAS! |
| Err(ClientError::Generic { | ||
| msg: "Secrets bundle does not belong to the user which was logged in".to_owned(), | ||
| details: None, | ||
| }) |
There was a problem hiding this comment.
IMO the SDK should not throw anything but just ignore the provided secrets if the userId does not match, so the application can just continue with the "manual" verification step. Do you agree?
CC @pixlwave
There was a problem hiding this comment.
Personally I think I'd rather split this API into 2 calls if it doesn't throw.
client.login()
client.importSecrets()
That way a client can tell whether or not that operation was successful or not and decide to act accordingly.
There was a problem hiding this comment.
Or specifically make it throw some kind of ClientError::Secrets { msg } so that this specific error can be handled differently. It would be nice to start moving away from ClientError::Generic everywhere anyway 😇
There was a problem hiding this comment.
That way a client can tell whether or not that operation was successful or not and decide to act accordingly.
We can do this, but you have to promise me to call this before you start a sync.
Or specifically make it throw some kind of ClientError::Secrets { msg } so that this specific error can be handled differently. It would be nice to start moving away from ClientError::Generic everywhere anyway 😇
Yeah, I skipped adding more specific errors for now, but we can obviously add them.
Let me know which way you prefer.
There was a problem hiding this comment.
We can do this, but you have to promise me to call this before you start a sync.
I don't think it's even possible for us to start a sync at this stage in the flow anyway but, just in case…
Let me know which way you prefer.
Personally I think 2 calls makes more sense but I'm happy either way. Any preference @bmarty?
There was a problem hiding this comment.
Alright, let's go with two separate calls then.
There was a problem hiding this comment.
Maybe the login function (and the other of the same vein) can return an object which has an importSecrets method?
There was a problem hiding this comment.
Maybe the
loginfunction (and the other of the same vein) can return an object which has animportSecretsmethod?
Hmm, that would again result in a breaking change and complicate the interface compared to the added error variant and argument.
There was a problem hiding this comment.
(maybe just ignore my last comment)
With the double API approach, I guess the application can also check the userId, after the login call, and before providing the secret, using client.session().userId, so it's good.
There was a problem hiding this comment.
I would leave the user ID check on the Rust side, so people don't forget about it.
5150bda to
bef186d
Compare
|
The complement-crypto failure is due to mozilla/uniffi-rs#2775. Other than that, this should now work for iOS as well. It should also have the two step interface people were requesting. Still needs some tests before this is ready for review. |
|
Thanks, I can test the new API. Can I ask you to update the branch regarding develop, so that I will not have too many API breaks to manage? |
Ah, you mean rebase on top of main? Sure, can do that. |
36750c9 to
b7e7412
Compare
Co-authored-by: Benoit Marty <[email protected]> Signed-off-by: Damir Jelić <[email protected]>
|
Rebased now. |
This allows us to double check if the right user got logged in.
|
I have no conflict on building the Android sdk, probably because EDIT: OK, my bad, I need to call |
|
Tested on Android using b7e7412 and it's working as expected 🎉 |
Since we already export a type with the same name and technically this type contains more than just the bundle.
Annoying, but ok, I renamed one of the types. |
Co-authored-by: Benoit Marty <[email protected]> Signed-off-by: Damir Jelić <[email protected]>


This PR allows people to get a secrets bundle out of band and import it after logging in a new client.
Mainly targeted to support the Element Classic -> Element X migration.
This closes #6087.
CHANGELOG.mdfiles.CONTRIBUTING.mdfile, notably the sections about Pull requests, Commit message format, and AI policy.