Block requests made without an authToken. Do not fire reconnect callbacks without an authToken.

[marcaaron]

The full context and discussion around this solution is here

Ultimately, we all agree that there is probably a better way to do this and discussed a few options, but this is the simplest solution to fix the problem right now. Which is... we are firing reconnect callbacks without an authToken so the simple solution is to... stop it !

Better solution which we can follow up with would be more of a PubSub or Ion integration where onReconnect callbacks are hooked into Ion and we just let Ion know that it needs to update based on an event like the app returning from background or gaining connectivity after it has been lost.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/142471

Tests (Android specifically is very broken - apparently iOS as well)

1. Delete app and reinstall
2. Background the app
3. Open the app again
4. Attempt to login
5. Verify you are able to do so

Screenshots

❌

[marcaaron]

Updated 🙃

[AndrewGable]

Confirmed this works on native platforms

[alex-mechler]

Tested all platforms 👍

[cead22]

👍 I think we should try to remove the code we added in request asap, and fix the root of the problems (eg, fetchAll being called forever)

[cead22]

Is the case really before the user signs in, I thought it was more when the user was signed in at some point. If it's the former, wouldn't that be a bug, that this isn't really fixing?

Can you explain why reauthenticating is true and needs to be set to false since that isn't obvious? I understand that we should start with it being false on the sign in page, and I think somebody mentioned in the discussion yesterday that this is because fetchAll will continue to be called forever, and will eventually fail with 407, which will trigger a reauthentication loop, is that right? can we put it in the comment?

[marcaaron]

Is the case really before the user signs in, I thought it was more when the user was signed in at some point.

Yes this is the reason. As far as I can tell there is no way to end up with an empty authToken while "signed in". Having an authToken is basically the definition of being "signed in", right?

If it's the former, wouldn't that be a bug, that this isn't really fixing?

I believe we discussed this already and the conclusion we came to is that this isn't the "bug fix" but a protection that we are putting in place to alert us of a bad pattern i.e. trying to make a request when we are not signed in.

Can you explain why reauthenticating is true and needs to be set to false since that isn't obvious?

I can add some more context about why we are doing this. My thinking here is that the app is in a Bad State™️ so we are resetting all the things that could possibly need to be reset and throwing an error. Ideally if this happens to you on dev you go and fix it. If it happens on prod then we'd at least avoid DDoSing ourselves. An alternative here would be to just throw this error and resolve instead of reset these things. The important bit here is that we do not attempt to make the request.

somebody mentioned in the discussion yesterday that this is because fetchAll will continue to be called forever, and will eventually fail with 407, which will trigger a reauthentication loop, is that right?

No, that's not correct. This isn't unique to fetchAll. We'd see the same behavior with any request made from the sign in page without an `authToken. Again, here's what happens:

1. request() is called without an authToken or credentials
2. It fails immediately with a 407
3. We try to reauthenticate and a TypeError is thrown because we have null credentials
4. That error is swallowed and the request is requeued
5. At this point we are stuck with reauthenticating === true and no further requests can be made
6. Users are stuck unable to log in

[cead22]

Thanks for the clarifications 👍. Having an authToken == signed in. Like you said this is a protection, and a good one. And I'm still not super fond of Bad State (tm), and that's usually why I try to really understand, and what I was hoping to get clearer on yesterday.

The Bad State (tm) and the Full Reset (tm) is what I'm still hesitant about, but I think we should merge this PR

[marcaaron]

Yep! I'm gonna create a follow up for this and will put together a problem/solution so we can discuss the root of this issue some more.

[tgolen]

This PR makes sense as a quick protection and in the case of the reconnection callbacks 👍