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

Make http requests soft logout aware #2027

Open
1 of 2 tasks
TheOneWithTheBraid opened this issue Feb 11, 2025 · 3 comments · May be fixed by #2033
Open
1 of 2 tasks

Make http requests soft logout aware #2027

TheOneWithTheBraid opened this issue Feb 11, 2025 · 3 comments · May be fixed by #2033
Labels
bug Something isn't working

Comments

@TheOneWithTheBraid
Copy link
Contributor

Checklist

  • I could not find a solution in the documentation, the existing issues or discussions.
  • I already asked for help in the chat

In which Project did the bug appear?

Other

If you selected "Other" as Project, please enter in which project the bug occurred.

matrix-dart-sdk

On which platform did the bug appear?

Firefox

SDK Version

v0.38.0

Describe the problem caused by this bug

(Note : the bug report form is really cursed.)

The Client._innerSync method currently seems to be the only place honoring MatrixError.M_UNKNOWN_TOKEN with soft_logout. As a developer using the SDK, I'd expect all called endpoints to handle a soft log out. The SDK already ships the FixedTimeoutHttpClient. I'd propose to implement an an additional layer similar to the RetryClient on SDK side filtering for soft logout responses and retrying after calling Client.onSoftLogout.

As a current workaround, I ship a RetryClient on my application side.

Steps To Reproduce

  1. Start SSSS bootstrap with a really short lived token (SSSS performs many individual requests)
  2. Be slow with user input
  3. See how it will fail due to an expired access token

Please note this might take a couple of times to retry since usually the Client.sync will refresh the access token fast enough.

Screenshots or Logs

No response

Security related

No response

@TheOneWithTheBraid TheOneWithTheBraid added the bug Something isn't working label Feb 11, 2025
@TheOneWithTheBraid
Copy link
Contributor Author

TheOneWithTheBraid commented Feb 11, 2025

My workaround :

import 'package:http/http.dart' hide Client;
import 'package:matrix/matrix.dart';

class MatrixRefreshTokenClient extends BaseClient {
  MatrixRefreshTokenClient({
    required this.inner,
    required this.client,
  });

  final Client client;
  final BaseClient inner;

  @override
  Future<StreamedResponse> send(BaseRequest request) async {
    Request? req;
    if ( // only refresh if
        // we are actually initialized
        client.onSync.value != null &&
            // the request is to the homeserver rather than e.g. IDP
            request.url.host == client.homeserver?.host &&
            // the request is authenticated
            request.headers
                .map((k, v) => MapEntry(k.toLowerCase(), v))
                .containsKey('authorization') &&
            // and last but not least we're logged in
            client.isLogged()) {
      try {
        await client.ensureNotSoftLoggedOut();
      } catch (_) {
      }
      // in every case ensure we run with the latest bearer token to avoid
      // race conditions
      finally {
        final headers = request.headers;
        // hours wasted : unknown :facepalm:
        headers.removeWhere((k, _) => k.toLowerCase() == 'authorization');
        headers['Authorization'] = 'Bearer ${client.bearerToken!}';
        req = Request(request.method, request.url);
        req.headers.addAll(headers);
        if (request is Request) {
          req.bodyBytes = request.bodyBytes;
        }
      }
    }
    return inner.send(req ?? request);
  }
}

Edit: Way cleaner implementation.

@krille-chan
Copy link
Contributor

Suggestion to make it even smaller:

We implement this as part of the ´unexpectedResponse` method with maybe a start like this:

diff --git a/lib/matrix_api_lite/matrix_api.dart b/lib/matrix_api_lite/matrix_api.dart
index 81023e13..a87e65cd 100644
--- a/lib/matrix_api_lite/matrix_api.dart
+++ b/lib/matrix_api_lite/matrix_api.dart
@@ -45,12 +45,21 @@ class MatrixApi extends Api {
 
   set accessToken(String? token) => bearerToken = token;
 
+  final StreamController<MatrixException> _onUnknownTokenResponse =
+      StreamController.broadcast();
+  Stream<MatrixException> get onUnknownTokenResponse =>
+      _onUnknownTokenResponse.stream;
+
   @override
   Never unexpectedResponse(http.BaseResponse response, Uint8List body) {
     if (response.statusCode >= 400 && response.statusCode < 500) {
       final resp = json.decode(utf8.decode(body));
       if (resp is Map<String, Object?>) {
-        throw MatrixException.fromJson(resp);
+        final exception = MatrixException.fromJson(resp);
+        if (exception.error == MatrixError.M_UNKNOWN_TOKEN) {
+          _onUnknownTokenResponse.add(exception);
+        }
+        throw exception;
       }
     }
     super.unexpectedResponse(response, body);

@TheOneWithTheBraid
Copy link
Contributor Author

But that lacks a functional retry mechanism. I'd highly prefer a high-level implementation which does not even throw an error first. I tried a lot about possible retry mechanisms, ensuring we have a valid token before sending the request was by far the most reliable which never got me logged out by the SDK.

TheOneWithTheBraid added a commit to TheOneWithTheBraid/matrix-dart-sdk that referenced this issue Feb 13, 2025
- implement a `MatrixRefreshTokenClient` to await token refresh before
  dispatching http requests
- improve documentation about soft logout logic
- fix dartdoc locations about soft logout

Fixes: famedly#2027

Signed-off-by: The one with the braid <[email protected]>
TheOneWithTheBraid added a commit to TheOneWithTheBraid/matrix-dart-sdk that referenced this issue Feb 15, 2025
- implement a `MatrixRefreshTokenClient` to await token refresh before
  dispatching http requests
- improve documentation about soft logout logic
- fix dartdoc locations about soft logout

Fixes: famedly#2027

Signed-off-by: The one with the braid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants