TF-4081 Fix mobile user was logged out again

[dab246]

Issue

#4081

Analysis and Solutions

Error handling

• When encountering a network connection error (NoNetworkError), display the message "No internet connection" to the user.
• When encountering a 401 error, perform a token refresh (refreshToken).
  • If refreshToken throws an exception, display a message to the user in the format:
    "Message [CodeError1 - CodeError2]".
  • Otherwise, if refreshToken succeeds but returns an empty token, automatically log in again using the current user's information.

Demo

• Case: Auto re-login with user email on 401 error for mobile

auto-refresh.webm

• Case: User logs out

logout.webm

[github-actions]

This PR has been deployed to https://linagora.github.io/tmail-flutter/4088.

[chibenwa]

Auto re-login with user email on 401 error for mobile

It could be great to display somehow an explanation of what happens to the user.
I think it could reduce the friction

[chibenwa]

TOO LONG, DID NOT READ

1000+ LOC changeset is a lot, even more for a simple bugfic.

I see a lot of duplicated try catch.

This is IMO symptomatic. I would have expected either a generic behaviour of the HTTP library, or an abstraction lying in tmail-flutter or even better in jmap-dart to help us significantly reduce the boiler plate code.

[dab246]

TOO LONG, DID NOT READ

1000+ LOC changeset is a lot, even more for a simple bugfic.

I see a lot of duplicated try catch.

This is IMO symptomatic. I would have expected either a generic behaviour of the HTTP library, or an abstraction lying in tmail-flutter or even better in jmap-dart to help us significantly reduce the boiler plate code.

IMO, this is not a simple bugfix.
Let me explain why there are so many duplicated try-catch blocks.

• I have separated the commits clearly for each specific bugfix.

• The reason for the 1000+ changed lines is due to the last commit, which addresses the following runtime error related to the Future.catchError method:

Invalid argument(s) (onError): The error handler of Future.catchError must return a value of the future's type

To fix this issue properly, we needed to update all related data sources. Since we are following Clean Architecture, with clear separation and abstraction across layers, multiple features depend on different data sources. Therefore, we have to apply try/catch in multiple places. However, the exception handling logic itself is centralized in a single common class, ensuring consistency across the app.

• Moreover, regarding issue [Customer] Mobile User was logged out AGAIN when being in the train #4081, we have implemented two proper solutions according to the requirements described in the issue:

  • Display user-friendly error messages with the format: Message [ErrorCode1 - ErrorCode2]
  • Automatically re-login the user when receiving a 401 Unauthorized error

[dab246]

Auto re-login with user email on 401 error for mobile

It could be great to display somehow an explanation of what happens to the user. I think it could reduce the friction

If you have any suggestions, please share them with us.
It would be even better if you could provide a mockup or a video.

[hoangdat]

• please rebase it with master branch @dab246

[dab246]

• please rebase it with master branch @dab246

Done

[Crash--]

This is not really true, right?

Let's say:

• I make a request
• This request throw a server error (and not a network error because at this time I'm connected)
• During the throw, we check if I've an internet connection but I was on a train, and suddenly I lost my internet connection.
• At the end, we'll throw a "NoNetworkError" instead of a "ServerError"

From dio don't we have a message saying "connection timeout" / "connexion reset" or something like that in order to get the information from the error itself instead of making a guess?

[dab246]

Add your description then it is finally lost internet connection, users should also be aware that they are lost connection. On the other hand we have the fallback of trying to retry some requests a number of times if they fail as in your case. For mobile we also support offline mode.

[Crash--]

users should also be aware that they are lost connection

100% agreed with that. And I imagine you have a watcher somewhere to check the connexion status.

What I find problematic here is the fact that we might end up returning the wrong reason for the error. Can’t we tell that it’s a connection issue directly from the initial error, without making this connectivity check?

And maybe, since this is the only error the user gets, we’ll display “We’re having issues with your internet connection, please try again later”, when in fact, it’s a real server error, so we’d be showing something incorrect to the user.

It’s actually because of this call that you have to change all the catch blocks to async, right?

[Crash--]

I don't understand the flow for iOS:

We set a shouldRefresh boolean to true, and then we call this _refreshToken method. So we know we need to refresh the token because we had an issue with the previous token.

But then, for iOS, we look if we have something within the keychain, and if we've something within the keychain, and this something is not expired then, we return the same token. I don't know if the check we do for setting shouldRefresh to true, is the same than we do in _isTokenExpired but it can be source of bug.

Why not just force the refresh even for iOS in this case since we already know we need to refresh?

And with this forceRefresh, I'm pretty sure, we'll be able to remove this condition:

if (newToken.token == _token?.token) {
      log('AuthorizationInterceptors::onError: Token duplicated');
      return null;
    }

No?

[dab246]

• For iOS, we have to handle notification in native code, due to the limitations of flutter, so we need to handle refresh token in native code to avoid getting data and getting 401 error on native. This token is synchronized with dart code on flutter through keychain

• We can't remove this.

if (newToken.token == _token?.token) {
log('AuthorizationInterceptors::onError: Token duplicated');
return null;
}

Because we added it to fix some issues with running multiple requests on multiple isolates at the same time

[Crash--]

Thanks for the explanation (I'll check where do we do native code)

In _getNewTokenForIOSPlatform why not then use:

_isTokenNotEmpty(tokenOIDC)
      && _isRefreshTokenNotEmpty(tokenOIDC)
      && _isTokenExpired(tokenOIDC);

instead of "just" isTokenExpired? Like that we're doing the same check than the previous one.

But I still don't understand, in that case (where we have multiple request at once from native & dart runtime) why having an old token equal to the new one is an error? To me its say "ok my token has been refreshed during my call, let's continue"

[Crash--]

Oh my bad, it's just a log from "onError", sorry for the last point.

[chibenwa]

Please be hereby formally reminded that this bug till impacts one of our major customer.
Please carry out his work as results are xpected from the team.
(This work is in standby for over a month now)

[hoangdat]

No, not a standby, we built it and monitoring the app through a month. But the problem occurred on last Tuesday, so we are investigating