Make sure to save the tokens the Client might return when its session is restored

[jmartinesp]

Content

When we're restoring a Rust Client object with some session data, the client might automatically refresh the tokens it uses. Usually, this would get reported by the ClientDelegate methods, but since that's only added after the session is restored, the update doesn't trigger the delegate callback and the new tokens aren't stored locally. This means next time we restore the session we'd be using discarded credentials and the server will log us out.

I'm creating a new RustClientSessionDelegate that implements both:

• ClientSessionDelegate: used by iOS mainly up until now, it's passed to the ClientBuilder so it will be able to handle token refreshes while the Client is restoring the session, solving the problem above.
• ClientDelegate: what we were using for handling token refreshes and refresh errors that should end in a logout. Since the previous delegate is handling token refreshes now, we only need the logout part.

Motivation and context

Found some promising explanation in a rageshake.

This might fix #3274.

Tests

This is quite difficult to test since it depends on the access token expiration time, which is different in each homeserver and also you need some luck (or bad luck) to make it fail, so even if it doesn't fail in some hours or days the fix might not do anything.

I think the best we can do is include it, then check if there haven't been any recent reports of spontaneous logouts.

When I tried to test it, I did the next:

• Set up an OIDC account, log in, verify, etc., and have a room in common that will trigger notifications.
• Wait until a token refresh happens, then kill the app by swiping them from the recents menu (this will kill the app but not prevent notifications from being received).
• Wait until the access token expiry date happens and don't touch the app in the meantime.
• Send a message that will trigger a notification in the OIDC session.
• Check the logs, the restoration of the client and the refresh of the tokens should happen in that order and there should be no issues.

However, I'm not 100% certain this is the right way to reproduce the previous error.

Tested devices

• Physical
• Emulator
• OS version(s): 14

Checklist

• Changes have been tested on an Android device or Android emulator with API 23
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request title will be used in the release note, it clearly define what will change for the user
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.59%. Comparing base (ae3fd89) to head (8be90e2).
Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
...droid/features/logout/impl/DefaultLogoutUseCase.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3378   +/-   ##
========================================
  Coverage    82.59%   82.59%           
========================================
  Files         1693     1693           
  Lines        39646    39646           
  Branches      4822     4822           
========================================
  Hits         32745    32745           
  Misses        5216     5216           
  Partials      1685     1685           

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

[github-actions]

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/RHSSjq

[bmarty]

LGTM, but maybe a better fix is to invoke client.restoreSession(sessionData.toSession()) after creating the RustMatrixClient instance? So move the line 67 into this also block? So in this case, the callback didRefreshTokens() will be invoked, since the clientDelegate will be set?

[jmartinesp]

I don't think we can because RustMatrixClient needs a SyncService parameter that we can only get once the session is restored. We'd have to maybe lazily initialize it, and make sure that doesn't break any existing behaviours.

In any case, I think we'll implement a fix in the Rust SDK instead. I'm leaving the PR as a draft until either that solution is ready or we decide a client side one might be better.

[jmartinesp]

I hate that this is needed, but I don't see any better way to centralise handling the token operations in a single point without being able to access the client for a logout.

[jmartinesp]

In any case, I think we'll implement a fix in the Rust SDK instead. I'm leaving the PR as a draft until either that solution is ready or we decide a client side one might be better.

Well, this is no longer true. We saw we could reuse the same flow as iOS with ClientSessionDelegate. This part probably needs a refactor in the SDK, but it's probably not the time to tackle it.

[bmarty]

There is something missing, see the comment: #3378 (comment)

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[bmarty]

Thanks for the update, all my tests are passing now!

Hopefully no more wild sign out!

[bmarty]

There is a swap in the parameters, this is not a big deal, but it can be confusing. Also when userInitiated is false, ignoreSdkError does not have sense.

Maybe we should create a small sealed interface to handle this:

    sealed interface LogoutParameter {
        data class UserInitiated(val ignoreSdkError: Boolean) : LogoutParameter
        data object ServerInitiated : LogoutParameter
    }

Not a blocker for this PR though, I can do it later, just sharing the idea here :)

[jmartinesp]

Can we add this in a separate PR then? So I can merge this and add it to the current release.