Auth gateway 401 handler doesn't refresh the token multiple times

[spruce-bruce]

Auth gateway 401 handler doesn't refresh the token multiple times

When the oAuth token expires the HttpAuthGateway.handle401() method will attempt to refresh the token EVEN IF the token is already in the middle of being refreshed.

ACs

1. Only one token refresh will be attempted at a given time

   Tasks

2. When handle401() is called it will set a loading flag in local storage and trigger a refresh call
3. If the loading flag is already set then handle401() will simply register a listener to the TOKEN_REFRESH_SUCCESS event. The listener will reattempt the api call that triggered the handle401() call in the first place.
4. When the token refresh attempt is completed successfully the TOKEN_REFRESH_SUCCESS event will be fired
5. If the refresh fails then fire TOKEN_REFRESH_FAILURE

[paulstatezny]

:arrow_down: :snowflake:

[zpchavez]

Task 2 makes it sound like you are registering a listener every time handle401 is called and a refresh is already in progress, which wouldn't make sense right? Seems like the listener should be registered on the first call to handle401 and subsequent calls should do nothing.

[paulstatezny]

I think you misunderstand Task 2, @zpchavez. It's trying to say this:

-. handle401 -- is refresh token exchange currently happening?

• No: Let's get a new access token now!
• Yes: Let's register a listener so that I'm notified when the new access token arrives. When it does, I'll re-try this API call.

Does that make sense? So subsequent calls do register a listener, because if you fire off 4 parallel API calls that all return 401s, you want to go get a new access token in response to the first 401, and then when the other 3 receive 401s you want each of them to listen for the success event and respond by firing off their requests.

[paulstatezny]

(Like that ^^^ Works perfectly, but I want to do some cleanup and testing.)

[paulstatezny]

So this is most the way there. (Tests not complete.)

But there's one problem: it avoids extra token refreshes as long as all of the bulk 401 responses are returned before the token refresh request is finished. So if any of them take extra time to return, another token refresh request is fired. It goes like this:

# This circumstance is already accounted for
1. A bunch of requests fired.
2. First one returns 401. Instantly a refresh token request is fired off.
3. All the other requests return 401s, and they know to wait because there is a refresh going on.
4. Refresh finishes; all remaining requests are re-executed.

# This circumstance is NOT accounted for
1. A bunch of requests fired.
2. First one returns 401. Instantly a refresh token request is fired off.
3. All the other requests EXCEPT ONE return 401s. Most of them know to wait because there is a refresh going on.
4. Refresh finishes, and all original requests that have returned 401s are re-executed.
5. The "laggard" request from the original batch comes back with a 401. There is *not* currently a refresh happening, so it doesn't know that the token is already updated.

I'm pretty sure in practice that circumstance 2 will rarely occur. However, it's kind of a nice to have, to optimize performance to the full extend possible in this area.

It wouldn't be that hard to address. We'd just need to grab the current token in gateway::apiRequest and pass it to gateway.handleError so that whenever 401 is received, we check if the token has changed. If it has, immediately re-execute the request. If it hasn't, we know that there's still an expired token and to go ahead and refresh, provided a refresh isn't already occurring.

[paulstatezny]

:star: Tested on a project. It works!!

In case it's not obvious, I did not unit test every aspect of this feature. I spent like 2 hours debugging why a specific test wasn't working, and realized that due to the nature and architecture of this module, it's really hard to unit test the kind of behavior being added in this issue.

If we want to unit test this stuff, I would recommend splitting up the gateway into multiple units that can be tested more easily.

[zpchavez]

:hand:

[zpchavez]

:+1: