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

async support for ClientSessionDelegate in the FFI #4516

Open
zzorba opened this issue Jan 10, 2025 · 3 comments
Open

async support for ClientSessionDelegate in the FFI #4516

zzorba opened this issue Jan 10, 2025 · 3 comments

Comments

@zzorba
Copy link
Contributor

zzorba commented Jan 10, 2025

At the moment the FFI bindings for interacting with the keychain are both blocking (non-async) functions. Unfortunately the host language I am using this with (react-native) features an async keychain, presenting some challenges, especially with 'reloading' the keychain when dealing with OIDC locks.

I've locally made a change to use an async_trait here and it seems to be working just fine. It required some adjustment of the traits in matrix_sdk::authentication as well since async functions cannot be passed quite as easily, but its not too bad.

#[matrix_sdk_ffi_macros::export(callback_interface)]
#[async_trait::async_trait]
pub trait ClientSessionDelegate: Sync + Send {
    async fn retrieve_session_from_keychain(&self, user_id: String)
        -> Result<Session, ClientError>;
    async fn save_session_in_keychain(&self, session: Session);
}

Would be happy to package this up and merge it if there is appetite for such a change.

@bnjbvr
Copy link
Member

bnjbvr commented Jan 13, 2025

Out of curiosity, what kind of hardships did you run into? Calling a sync function from an async one should work OK in general; it's the other way around that's usually more complicated. Can you explicit what you mean by some challenges, especially with 'reloading' the keychain when dealing with OIDC locks?

@zzorba
Copy link
Contributor Author

zzorba commented Jan 13, 2025

Yeah, the issues I hit were related to the OIDC refresh process, i.e. a NotificationServiceExtension gets a new refresh token, storing it in the system keychain, and now the main process needs to be able to read this back (via the ClientSessionDelegate interface).

Right now the API defines that as a blocking call retrieve_session_from_keychain, which works fine on iOS and Android, but the react-native keychain library only supports async access to the system keychain (historically, most things with the system had to be async, this is changing now but libraries are slow to update). As you said, saving the token is fine in the current setup, but reading it is more complicated.

@bnjbvr
Copy link
Member

bnjbvr commented Jan 15, 2025

Thanks for the detailed explanation. Could you open a PR, please, so that we get an idea of the changes that would be required? If they're not too invasive, we think it'd be fine to take them — but I can't really make any sort of promise until we see what these changes are :)

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

No branches or pull requests

2 participants