Skip to content

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

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.

Sign up for GitHub

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

Merged
stitesExpensify merged 2 commits into from
Oct 5, 2020
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f062439
Block requests made without an authToken
marcaaron Oct 1, 2020
ec290ac
add redirectToSignIn
marcaaron Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/lib/API.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ Ion.connect({
// returns from the background. So, if we are returning from the background
// and we are online we should trigger our reconnection callbacks.
Activity.registerOnAppBecameActiveCallback(() => {
if (isOffline) {
// If we know we are offline and we have no authToken then there's
// no reason to trigger the reconnection callbacks as they will fail.
if (isOffline || !authToken) {
return;
}

Expand Down Expand Up @@ -180,6 +182,17 @@ function request(command, parameters, type = 'post') {
});
}

// If we end up here with no authToken it means we are trying to make
// an API request before we are signed in. In this case, we should just

@cead22 cead22 Oct 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcaaron Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cead22 Oct 2, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcaaron Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// cancel this and all other requests and set reauthenticating to false.
if (!authToken) {
console.error('A request was made without an authToken', {command, parameters});
reauthenticating = false;
networkRequestQueue = [];
Comment thread
marcaaron marked this conversation as resolved.
redirectToSignIn();
return Promise.resolve();
}

// Add authToken automatically to all commands
const parametersWithAuthToken = {...parameters, ...{authToken}};

Expand Down