isAuthenticated with optional callback

[bas080]

I have this login screen bug.

Aurelia-authentication checks if the token is valid. If that is not the case it does tries to update my token. This is an asynchronous process. Yet isAuthenticated does not return a Promise.

To enable backwards compatibility I think it's best to pass an optional callback. This is called when either no update is required or after the async update is completed.

Example of how I would use it.

authService.isAuthenticated(authenticated => {
  authenticated ? navigateTo('home') : navigateTo('login');
});

@doktordirk @RWOverdijk

[doktordirk]

callback instead of optionallly returning a promise seems to be a valid option here

[bas080]

maybe have it pass an error as first argument. In case the request failed.

[doktordirk]

or we leave if to the user. there are quite a few options with .authenticted, pubsub, and the signaller.in oour case one can manually

if (!auth.isAuthenticated()) auth.updateToken().then(()=>auth.isAuthenticated() ? navigateTo('home') : navigateTo('login'));
else navigateTo('home');

which isn't much longer

[pfurini]

I completely agree with @bas080 here. If a method forks an async operation, either in its main execution flow or in a conditional branch like inside isAuthenticated, that method must either return a Promise or accept a completion callback. When I looked at the method signature, I was confident it was sync.. I do not use refresh tokens (yet) in my app, so everything works fine. But my login flow would have broken if I had enabled them.

Please consider fixing this method, and potentially other methods like that, by adding a callback parameter like @doktordirk suggested, if you really want BC. I'd rather prefer a method returning properly typed Promises (in this case Promise<boolean>), this is such a serious design flaw IMHO that a BC could be beneficial..

Sorry if I'm overreacting, but you definitely shouldn't "leave it to the user", because the method signature is lying to them..

[doktordirk]

maybe we just need a third method. we have isAuthenticated() method, authenticated status variable (doesn t have that issue and imho anyways the best option) and additionally xyz for promises

[pfurini]

@doktordirk not completely sure.. the more I look at isAuthenticated code, the more it doesn't look sound to me.. Take the incriminated if:

    // auto-update token?
    if (!authenticated
      && this.config.autoUpdateToken
      && this.authentication.getAccessToken()
      && this.authentication.getRefreshToken()
    ) {
      this.updateToken().catch(error => LogManager.getLogger('authentication').warn(error.message));
      authenticated = true;
    }

Do you see a code smell here? authenticated is set to true like if updateToken was sync. And at the same time this method is called inside updateToken, with the assumption that that if branch would never be executed because we just get an update token and authenticated was set to true just after updateToken was called... What the heck!! 8-/ When I see such a convoluted callstack I start seeing trouble..

[doktordirk]

authenticated here is the class property, but the return value.

the reason to have it returning true is to prevent unneeded tab reloading.

but as i said, why use this BC method which gets dirty checked when binding anyways

ps: while not much of an issue, we should check for refresh_token expiration

[doktordirk]

PS: I.d like to rewrite the whole plugin with plugable providers anyways

[pfurini]

In my opinion, the refresh token flow has to be refactored, by taking into account an intermediate state that you could expose to lib users.

The state flow should consist in three main states: unauthenticated, refreshing, authenticated. The refreshing state is when you called updateToken but not received a response yet. The user is responsible to deal with this situation, like temp locking the whole interface, or whatever.

Whenever I deal with async processes, I always model them after a state flow diagram and expose the state via properties and event callbacks, or such..

[RWOverdijk]

Closing this due to inactivity. If there's an answer you didn't get, feel free to open up a new issue.