ffi: expose methods for SSO login

[Johennes]

This exposes methods for logging in via the (legacy) SSO flow in the FFI bindings. I still need to verify that this actually works by calling it from an app. However, I'd first like to get feedback on whether adding this to the bindings would be acceptable in general.

• [ ] Public API changes documented in changelogs (optional)

[Johennes]

Out of curiosity, are you making use of the SSO login method in an app? I thought it was to be deprecated, and as such I would encourage to migrate to the new login methods instead, using OIDC.

Yes, indeed. I'm hoping to use these functions in https://github.com/unomed-dev/react-native-matrix-sdk for an app that connects to a server that doesn't currently use OIDC (which I think is still unstable / at MSC-stage?).

(Moreover, I wouldn't piggyback on the OIDC login methods for new login ways, because the code there is messy; rather, we should have some kind of stateful object that remembers the login steps, or any kind of better, stateful API like that.)

Oh, hm. I might need some guidance on how to properly do that. 🙈

Currently I'm not using the state parameter on the URL because the SSO functions in the SDK didn't seem to support that. So I think other than the redirect URL (which needs to be passed in again anyway) there might not actually be any state to track on the bindings side?

[Hywan]

Oh wait, I didn't see the comments… @bnjbvr has already replied to your PR.

[Johennes]

Do you think it is possible to write tests for these new methods?

I can probably do that, yes. However, @bnjbvr suggested that the architecture should maybe be changed to something akin to a state machine. It would be helpful if we could clarify this before adding the tests.

[bnjbvr]

To clarify, the state machine I was talking about was to return a small object that starts the SSO login, with a single method "finish()" that received the right arguments, instead of adding two loose methods which don't look connected in any way. Just changing the API shape, but not necessarily how the functions work :)

[Johennes]

To clarify, the state machine I was talking about was to return a small object that starts the SSO login, with a single method "finish()" that received the right arguments, instead of adding two loose methods which don't look connected in any way. Just changing the API shape, but not necessarily how the functions work :)

Ah, I see. Ok, let me give that a shot. Will probably be a good stretch of my Rust "skills". 😅

[Johennes]

I'm struggling a little to make this work because I'm not sure how to properly move the things needed to complete the login from the service into the returned object. 😕

I can do something like in the snippet below but this consumes the service which means it cannot be used again if finish fails. I'm not sure if that's a good design?

impl AuthenticationService {
    ...

    pub async fn start_sso_login(
        self,
        redirect_url: String,
        idp_id: Option<String>,
    ) -> Result<SsoHandler, AuthenticationError> {
        let client_guard = self.client.read().await;
        let Some(client) = client_guard.as_ref() else {
            return Err(AuthenticationError::ClientMissing);
        };

        let auth = client.inner.matrix_auth();
        let url = auth.get_sso_login_url(redirect_url.as_str(), idp_id.as_deref()).await.map_err(|e| AuthenticationError::Generic { message: e.to_string() })?;

        drop(client_guard);

        Ok(SsoHandler { service: self, url: url })
    }
}

#[derive(uniffi::Object)]
pub struct SsoHandler {
    /// The wrapped authentication service
    service: AuthenticationService,

    /// The underlying URL for authentication.
    pub url: String
}

#[uniffi::export]
impl SsoHandler {
    /// Completes the SSO login process.
    pub async fn finish(&self, callback_url: String) -> Result<Arc<Client>, AuthenticationError> {
        let client_guard = self.service.client.read().await;
        let Some(client) = client_guard.as_ref() else {
            return Err(AuthenticationError::ClientMissing);
        };

        let auth = client.inner.matrix_auth();

        let url =
            Url::parse(&callback_url).map_err(|_| AuthenticationError::SsoCallbackUrlInvalid)?;

        #[derive(Deserialize)]
        struct QueryParameters {
            #[serde(rename = "loginToken")]
            login_token: Option<String>,
        }

        let query_string = url.query().unwrap_or("");
        let query: QueryParameters = serde_html_form::from_str(query_string)
            .map_err(|_| AuthenticationError::SsoCallbackUrlInvalid)?;
        let token =
            query.login_token.ok_or(AuthenticationError::SsoCallbackUrlMissingLoginToken)?;

        auth.login_token(token.as_str())
            .await
            .map_err(|_| AuthenticationError::SsoLoginWithTokenFailed)?;

        drop(client_guard);

        let Some(client) = self.service.client.write().await.take() else {
            return Err(AuthenticationError::ClientMissing);
        };

        Ok(Arc::new(client))
    }
}

[Johennes]

@bnjbvr do you have any thoughts on how to best proceed here?

[bnjbvr]

I can do something like in the snippet below but this consumes the service which means it cannot be used again if finish fails. I'm not sure if that's a good design?

Since there can be an error during the process, it'd be better to not consume the AuthenticationService (i.e. take its ownership) since we might need to retry, and we'd need to make sure it will properly error out (likely because a client might be absent) next time it's called after a successful login.

[Johennes]

Ok, I think I might have it. Adding tests for this part turned out somewhat tedious. So I only added one to verify the login URL so far.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.88%. Comparing base (74b770f) to head (10d495a). Report is 134 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3558      +/-   ##
==========================================
- Coverage   83.88%   83.88%   -0.01%     
==========================================
  Files         254      254              
  Lines       26306    26311       +5     
==========================================
+ Hits        22068    22071       +3     
- Misses       4238     4240       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Johennes]

I moved everything over to Client due to https://github.com/matrix-org/matrix-rust-sdk/issues/3579#issuecomment-2181435689.

I think the clippy error might only be fixable once AuthenticationService has been removed and Client made public. So this probably still needs to wait for @pixlwave's PR. Feedback on the rest of the code is still appreciated though. 🙂

[pixlwave]

@Johennes The PR removing the service is here. There's still an authentication.rs file which might be a nice place to keep any types you've added. It hasn't been reviewed yet but I don't think there's anything controversial in there.

[Johennes]

@Hywan this is ready for review now.

CC @pixlwave in case you'd like to take a look as well.

[Johennes]

@Hywan is there anything else you'd like me to change before this can be reviewed?