fix(fetchClientConfig): reject when updateToken fails

[MaximBalaganskiy]

Let's say I've got current and refresh tokens in the storage. Current token is already expired and refresh one is not. But refresh one has been invalidated on a server.

In this scenario an original request will fail with 401 and http interceptor will try to update the token. This update request will also fail because the refresh token has been invalidated. As a result, this uncaught rejection will break the execution flow completely.

There is no way to catch this rejection in the code which made the original call to an authorised endpoint.

[RWOverdijk]

Good eye on spotting the issue. However, I think we need a different solution for this. If it fails, it fails, and the user should be "logged out" with a reason (tokens invalidated).

[RWOverdijk]

@doktordirk Am I making sense or am I talking nonsense? :D

[MaximBalaganskiy]

Sounds good.

I'm not quite sure though why this reject cannot be caught on the original http promise...

On Wed., 13 Jun. 2018, 6:32 pm Roberto Wesley Overdijk, < notifications@github.com> wrote:

@doktordirk https://github.com/doktordirk Am I making sense or am I talking nonsense? :D

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SpoonX/aurelia-authentication/pull/395#issuecomment-396858710, or mute the thread https://github.com/notifications/unsubscribe-auth/ADimvGkV32NF0pEiOXl6-ZVWRNEOKKeLks5t8M4NgaJpZM4UlqdV .

[MaximBalaganskiy]

Oh, and another thing is that updateToken already redirects to the expired URL

On Wed., 13 Jun. 2018, 6:32 pm Roberto Wesley Overdijk, < notifications@github.com> wrote:

@doktordirk https://github.com/doktordirk Am I making sense or am I talking nonsense? :D

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SpoonX/aurelia-authentication/pull/395#issuecomment-396858710, or mute the thread https://github.com/notifications/unsubscribe-auth/ADimvGkV32NF0pEiOXl6-ZVWRNEOKKeLks5t8M4NgaJpZM4UlqdV .

[doktordirk]

the request is already a promise and hence rejects on errors. the additional catch won't do anything.

i'm not sure what currently is happening with expired refresh-tokens on your end (in my setting it just clears all tokens and redirects to the expiredRedirect url), resp what you want to happen

[MaximBalaganskiy]

The issue is that rejects in the interceptor cannot be caught for some reason, so if updateToken fails it's gonna stop the flow completely without a chance to recover in the code doing the fetch

On Wed., 13 Jun. 2018, 9:17 pm doktordirk, notifications@github.com wrote:

the request is already a promise and hence rejects on errors. the additional catch won't do anything.

i'm not sure what currently is happening with expired refresh-tokens on your end (in my setting it just clears all tokens and redirects to the expiredRedirect url), resp what you want to happen

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SpoonX/aurelia-authentication/pull/395#issuecomment-396902487, or mute the thread https://github.com/notifications/unsubscribe-auth/ADimvI8XvZEOL2YWvFple2u9t0Tubhsgks5t8PTdgaJpZM4UlqdV .

[MaximBalaganskiy]

In tried adding a catch or putting the call in try/catch... Nothing worked

On Wed., 13 Jun. 2018, 9:17 pm doktordirk, notifications@github.com wrote:

the request is already a promise and hence rejects on errors. the additional catch won't do anything.

i'm not sure what currently is happening with expired refresh-tokens on your end (in my setting it just clears all tokens and redirects to the expiredRedirect url), resp what you want to happen

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SpoonX/aurelia-authentication/pull/395#issuecomment-396902487, or mute the thread https://github.com/notifications/unsubscribe-auth/ADimvI8XvZEOL2YWvFple2u9t0Tubhsgks5t8PTdgaJpZM4UlqdV .

[doktordirk]

it's a rejected promise. you can't try.. catch it. indeed the .catch() would be the way to go, but just catching and rethrowing won't get you anywhere as that's just what happens atm. generally for refresh-tokens, that should be handled internally and redirects to expiredRedirect- if set- and for other places eg login it would be eg authService.login(...).catch(...)

so, the question remains, what you want to happen on a rejected refresh-token request and what happens atm for you (if it is not the usually desired redirection to the expiredRedirect url)

[MaximBalaganskiy]

Redirection is fine. All I need is a catch and explicit reject in the interceptor, like in the PR, because neither try/except nor promise catch work

On Thu., 14 Jun. 2018, 8:05 am doktordirk, notifications@github.com wrote:

it's a rejected promise. you can't try.. catch it. indeed the .catch() would be the way to go, but just catching and rethrowing won't get you anywhere as that's just what happens atm. generally for refresh-tokens, that should be handled internally and redirects to expiredRedirect- if set- and for other places eg login it would be eg authService.login(...).catch(...)

so, the question remains, what you want to happen on a rejected refresh-token request and what happens atm for you (if it is not the usually desired redirection to the expiredRedirect url)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SpoonX/aurelia-authentication/pull/395#issuecomment-397102762, or mute the thread https://github.com/notifications/unsubscribe-auth/ADimvHH5yTf7x840jGi0iHuGCq40b2Nmks5t8YyQgaJpZM4UlqdV .

[doktordirk]

i still don't get you problem, but anyways, have you tried you fix? as i said, it can't do anything, errors are rejected already. that's how promises are implemented. also returning promises pass down their rejections and throws.

so following are equivalent

let p = new Promise( (resolve, reject) => {
  throw Error('text');
});
p.catch(e=>console(e)); // -> 'text'

let p = new Promise( (resolve, reject) => {
  reject('text');
});
p.catch(e=>console(e)); // -> 'text'

let p = new Promise( (res, rej) => {
  return Promise.reject('text');
// edit: or maybe it should be resolve(Promise.reject('text')) and not return? i better have a quick look
});
p.catch(e=>console(e)); // -> 'text'

let p = new Promise( (resolve, reject) => {
  return new Promise( (res, rej) => thrown new Error('text'))
});
p.catch(e=>console(e)); // -> 'text'

hence, unless i'm mistaken, the proposed catch will not add anything. any possible thrown error/rejection should be passed down already.

anyways, i suspect you want to handle assumed unhandled 'throws' with the addition. while those are passed down already anyways, i'd still like to know what that error is, so we can fix that. there are not that many function calls in there and i didn't spot anything which might throw an Error

[MaximBalaganskiy]

That's what I was thinking too, it should just bubble to the closest catch or try/catch in typescript... except it does not... I've tried the explicit catch in your interceptor and surprisingly it worked. I'll try to investigate a little bit more.

[doktordirk]

the error seems to be indeed the return this.authService.updateToken(), should be resolve(this.authService.updateToken()........).

see if that works and if yes, PR that i guess

edit: did a quick search and that seem to be the only wrong return(typeof Promise) instead of the correct (return if needed) resolve(typeof Promise) in this plugin

[MaximBalaganskiy]

Aha, how did I not see that... not resolving breaks the chain. I've updated the PR

[doktordirk]

@RWOverdijk merged, build and tagged (patch). please npm publish (is v3.8.1) as convenient

[RWOverdijk]

@doktordirk Done!