witoldsz / angular-http-auth

MIT License
2.38k stars 418 forks source link

Broadcast auth-loginRequired only once #120

Closed olivierdeckers closed 7 years ago

olivierdeckers commented 8 years ago

I have encountered a bug in my angular application which uses your package. It is related to issue #95. It occurs when using oauth refresh tokens and when multiple calls that all return HTTP 401 are fired in parallel. The loginRequired event is then fired multiple times, which resulted in my case in multiple refresh_token calls to the oauth endpoint. All but the first call fail as a refresh_token can only be used once. When refreshing the token fails, the app falls back to showing a login form, even though the refresh token was valid and the first call obtained a valid access token.

I could have handled this case in my app by checking that I only fire one refresh_token call in the loginRequired event listener, but since I don't see a use case for emitting the loginRequired event again before loginConfirmed function was called, I thought it would be better to solve it in this library.

In issue #95 you suggest to use one probe call to avoid this issue. This solution is not adequate for my app, because waiting for this probe call would slow the app down unnecessarily in the case where the access token is still valid. This PR eliminates the need to use a probe call by only firing the loginRequired event once.

The event is now only fired if the httpBuffer is empty. If the buffer isn't empty, the the loginRequired event has already been fired, and we can safely assume that the app is in the process of logging in and will call the loginConfirmed method, emptying the buffer.

For me, the current behaviour was unexpected and maybe this change can save others from similar hard-to-reproduce bugs.

witoldsz commented 8 years ago

Hi, can you rewrite the commit without code reformat, so we could see the actual change in code of this pull request?

olivierdeckers commented 8 years ago

I rolled back the code reformat. Could you take another look?

simison commented 8 years ago

@olivierdeckers BTW, you might want to squash after the work at this PR is done! :-)

witoldsz commented 7 years ago

It was fixed in 1.3.0.