Move the AuthenticationService from the FFI into the SDK crate. #3049

pixlwave · 2024-01-23T12:53:12Z

This PR moves the AuthenticationService from the FFI into the SDK crate and transforms the FFI layer into a light wrapping around it. Closes #3029.

It would be best to review this PR commit by commit.

Todo

Add tests for existing functionality
Consider what we should do with finalise_client in the FFI (given the FFI builder is somewhat different to the SDK builder).
Expose some of the common configuration errors we've seen in the community to improve the error messages in Element X - possibly this is a separate PR, I'll see based on writing tests.

pixlwave · 2024-01-30T12:25:31Z

I've marked this Ready for review, mainly because I would like a review, rather thank thinking this is in a good enough state to be merged. I expect this to change a lot, will add comments on the areas I was unsure about.

pixlwave · 2024-01-30T12:27:02Z

crates/matrix-sdk/src/oidc/mod.rs

The changes in this file and backend/mod.rs were very much "just get it working". It generates some warnings and I would love some direction on what the correct solution would be here.

pixlwave · 2024-01-30T12:29:05Z

crates/matrix-sdk/src/authentication/service.rs

@@ -811,20 +842,148 @@ pub(crate) mod tests {
        assert!(matches!(error, AuthenticationError::Generic { .. }));
    }

+    #[async_test]
+    #[cfg(feature = "experimental-oidc")]


Honestly, I'm not 100% sure if these new tests will run or not on CI given the feature flag? I've been running all of the tests in this file using the below which is why I'm asking.

cargo test -p matrix-sdk --features experimental-sliding-sync,experimental-oidc authentication::service

The "all crates" CI steps will run all the tests with the necessary features as required by the combination of crates in the workspace; because we have the bindings and the UI crate, that means experimental-sliding-sync and experimental-oidc are enabled by default. And you can check that by seeing that some of the oidc tests are run in those CI steps. So this feature guard should work.

You can also double-check if the following command line runs the test:

cargo nextest run --workspace --exclude matrix-sdk-integration-testing --features testing -- test_name

codecov · 2024-01-30T12:35:41Z

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (f5f8f47) 83.72% compared to head (8cf7550) 83.89%.
Report is 13 commits behind head on main.

❗ Current head 8cf7550 differs from pull request most recent head 5cdc032. Consider uploading reports for the commit 5cdc032 to get more accurate results

Files	Patch %	Lines
crates/matrix-sdk/src/authentication/service.rs	75.40%	61 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3049      +/-   ##
==========================================
+ Coverage   83.72%   83.89%   +0.17%     
==========================================
  Files         223      223              
  Lines       23367    23548     +181     
==========================================
+ Hits        19564    19756     +192     
+ Misses       3803     3792      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pixlwave · 2024-01-30T12:38:45Z

bindings/matrix-sdk-ffi/src/authentication_service.rs

+            .map_err(|error| AuthenticationError::Generic { message: error.to_string() })?;
+        let session = client.matrix_auth().session().context(
+            "Login was successful but is missing a valid Session to configure the file store.",
+        )?;

        self.finalize_client(client, session, whoami.user_id)


The main point of this refactor was to get to a stage where everything was tested so we could fix the requirement of creating a second client after login. However! This first step seems to make more than enough changes before trying to rationalise what to do with the 2 different types of client builder, so if it still a requirement for this part to move into the SDK, I suggest we tackle that in a follow-up PR.

As it stands right now, I've attempted to implement the AuthenticationService with the idea of using it from Rust more than the FFI use case.

pixlwave · 2024-01-30T12:40:40Z

bindings/matrix-sdk-ffi/src/authentication_service.rs

-            client.inner.oidc().full_session().ok_or(AuthenticationError::SessionMissing)?;
+        // Login and get the user ID that was resolved by the authentication server.
+        let client = RUNTIME.block_on(self.inner.login_with_oidc_callback(
+            OidcAuthorizationData {


Looking at it, the OidcAuthenticationData struct can likely be removed now, in favour of exposing this one through uniffi.

pixlwave · 2024-01-30T12:43:09Z

crates/matrix-sdk/src/authentication/service.rs

+
+/// Errors related to authentication through the `AuthenticationService`.
+#[derive(Debug, thiserror::Error)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Error), uniffi(flat_error))]


I have a feeling the right option here would (sadly) be to duplicate this enum in the FFI, and box the underlying messages instead of using to_string() on them. Didn't want to do that until this had been reviewed, maybe its ok, I'm not sure?

pixlwave · 2024-01-30T12:45:12Z

crates/matrix-sdk/src/authentication/service.rs

+        // Now we've verified that it's a valid homeserver, make sure
+        // there's a sliding sync proxy available one way or another.
+        #[cfg(feature = "experimental-sliding-sync")]
+        if self.custom_sliding_sync_proxy.read().unwrap().is_none()
+            && client.sliding_sync_proxy().is_none()
+        {
+            return Err(AuthenticationError::SlidingSyncNotAvailable);
+        }


I don't know if this is a rational approach to take here given anyone might want to use this service now, but its the behaviour we have been using in the FFI.

Indeed, we can't mandate usage of sliding sync for users of the authentication API, so this would probably need to go. This was specific to the FFI layer which only uses Sliding Sync, so there it would make sense to have such a check IMO.

pixlwave · 2024-01-30T12:46:21Z

crates/matrix-sdk/src/authentication/service.rs

+// TODO: Is it normal to split up the implementation into public and private
+// like this?


Just highlighting this here to find out the answer :)

fix: Remove useless result.

- Configuration and password login for now.

pixlwave mentioned this pull request Jan 24, 2024

Element X fails to validate empty m.identity_server element-hq/element-x-ios#2310

Closed

pixlwave force-pushed the doug/authentication-service-for-real branch 3 times, most recently from f50c4f6 to 8cf7550 Compare January 30, 2024 12:22

pixlwave marked this pull request as ready for review January 30, 2024 12:24

pixlwave requested a review from a team as a code owner January 30, 2024 12:24

pixlwave requested review from andybalaam and removed request for a team January 30, 2024 12:24

pixlwave commented Jan 30, 2024

View reviewed changes

pixlwave added 4 commits January 31, 2024 18:45

sdk: Adopt the AuthenticationService from the FFI ready for testing.

daa3664

fix: Remove useless result.

sdk: Add AuthenticationService tests.

38fc295

- Configuration and password login for now.

sdk: Add OIDC AuthenticationService tests.

c9d7246

chore: Break up large method.

5cdc032

pixlwave force-pushed the doug/authentication-service-for-real branch from 8cf7550 to 5cdc032 Compare January 31, 2024 18:46

pixlwave mentioned this pull request Feb 8, 2024

SDK: Add the beginnings of an authentication service. #3108

Closed

pixlwave closed this Feb 8, 2024

pixlwave deleted the doug/authentication-service-for-real branch June 20, 2024 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the AuthenticationService from the FFI into the SDK crate. #3049

Move the AuthenticationService from the FFI into the SDK crate. #3049

pixlwave commented Jan 23, 2024 •

edited

Loading

pixlwave commented Jan 30, 2024

pixlwave Jan 30, 2024 •

edited

Loading

pixlwave Jan 30, 2024 •

edited

Loading

bnjbvr Feb 8, 2024

codecov bot commented Jan 30, 2024 •

edited

Loading

pixlwave Jan 30, 2024

pixlwave Jan 30, 2024

pixlwave Jan 30, 2024

pixlwave Jan 30, 2024

bnjbvr Feb 8, 2024

pixlwave Jan 30, 2024

		// TODO: Is it normal to split up the implementation into public and private
		// like this?

Move the AuthenticationService from the FFI into the SDK crate. #3049

Move the AuthenticationService from the FFI into the SDK crate. #3049

Conversation

pixlwave commented Jan 23, 2024 • edited Loading

pixlwave commented Jan 30, 2024

pixlwave Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

pixlwave Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

bnjbvr Feb 8, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

pixlwave Jan 30, 2024

Choose a reason for hiding this comment

pixlwave Jan 30, 2024

Choose a reason for hiding this comment

pixlwave Jan 30, 2024

Choose a reason for hiding this comment

pixlwave Jan 30, 2024

Choose a reason for hiding this comment

bnjbvr Feb 8, 2024

Choose a reason for hiding this comment

pixlwave Jan 30, 2024

Choose a reason for hiding this comment

pixlwave commented Jan 23, 2024 •

edited

Loading

pixlwave Jan 30, 2024 •

edited

Loading

pixlwave Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading