Closed schuettecarsten closed 6 years ago
Hey @schuettecarsten, good catch! Thanks for reporting.
My suggestion to resolve this one would be as following:
1) We don't fail the token when parsing it. So that if the token has incorrect JSON or any other cause for parsing to fail, we just leave the payload as empty. Empty payload will make the isValid
method to return false (with an exception for NbAuthSimpleToken
).
2) Upon token creation, during the authentication (login/register/refreshToken) phase we check the token, and if it is not valid - NbAuthResult
will be marked as success=false
.
3) For the cases, when we don't expect the token to be presented or valid (for example registration which requires user to login separately) we introduce a new option called requireToken
which is true
by default. When set to false
it won't fail the authentication request and returns the NbAuthResult
as it is (possible with an invalid token inside).
@schuettecarsten @alain-charles does this sound as a solution to you?
/cc @Tibing
I think your made an excellent analysis. I would only give the name requireValidToken in the last item instead of requireToken . Do you want me to send a PR ?
@alain-charles thanks! Agree, requireValidToken
makes more sense. Let's see if this resolves @schuettecarsten's issue and then we can schedule it.
This sounds absolutely great! 👍
I have begun to work on that. The behavior of the framework is now not exactly the same as presented here, because we introduced the tokens created_at attribute. So, the payload is parsed at token creation, so that the app now "crashes" at login if the token is malformed.
So we have to decide what to do. If the jwt token is malformed, i don't think we shoud accept it and store it, because it is really useless. we won't be able to use it for authenticating against backend.
If the token is malformed, maybe it is a misconfiguration of the strategy, maybe a backend problem. I propose that instead of crashing, we send NbAuthResult with success=false, and the error description "invalid jwt token".
What do you think ?
@alain-charles yes, I completely agree.
Just thinking what is the best way to do this.
One of the options would be to re-do the createToken
method so that it throws an exception if the token is not valid, so that in the following example:
return this.http.post(url, this.buildCodeRequestData(code),
this.buildAuthHeader())
.pipe(
map((res) => {
return new NbAuthResult(
true,
res,
this.getOption('redirect.success'),
[],
this.getOption('defaultMessages'),
this.createToken(res)); // here we call, it creates a token, checks if it's invalid, and if it is and `requireValidToken === true` - throws an exception new Error('invalid token ... reason ...')
}),
catchError((res) => this.handleResponseError(res)),
);
}
if the createToken
method throws an exception, it will be catched by handleResponseError
so that the amout of changes we need to do through all strateges is quite small. The only thing we have to make sure is the all createToken
calls are handled by some catchError
in case of an exception.
I verified. For the moment, it is almost always the case, but not in :
NbDummyAuthStrategy.createDummyResult()
NbOAuth2AuthStrategy.redirectResultHandlers
@nnixaa I am getting near a satisfying solution for this issue.
If you configure the token as jwt and if backend sends malformed token, you get an error in any case with a detailed message as we decided above:
If the token is malformed, maybe it is a misconfiguration of the strategy, maybe a backend problem. I propose that instead of crashing, we send NbAuthResult with success=false, and the error description "invalid jwt token".
if the token is well formed but invalid, and if requireValidToken
is set to true, you get an error 'invalid token'
However we are running into concurrent issue between this new requireValidToken
flag positionned at the strategy level and the existing failWhenNoToken
flag positionned at the login/register/refresh levels in NbAuthPasswordStrategy
.
requireValidToken
takes precedence so that if set to true, positionning failWhenNoToken
to false is useless.
=> possible breaking change if we set by default requireValidToken
to true
What do you think ? Do we maintain this requireValidToken flag since we decided to reject malformed token and since there is this existing failWhenNoToken
flag ?
@alain-charles since we never released the failWhenNoToken
flag feature we can just remove it, as requireValidToken
replaces this functionality.
Issue type
I'm submitting a ... (check one with "x")
Issue description
Current behavior: When login response of a NbPasswordAuthStrategy returns an broken JWT token, it is stored in the local storage without validation. On the next request, the app will fail because NbAuthJWTToken cannot parse the token.
Expected behavior: A broken token should be rejected during login.
Related code: See the exception that occurs when token from login response is invalid:
This exception is shown in Console log when the App tries to use the broken Token, the app crashes:
Other information:
npm, node, OS, Browser
Angular, Nebular