Closed derdeka closed 3 years ago
Adding token refresh would add a bit of polish. 👍
@sformisano I would prefer option 3 too.
To make this working i would like to extend the TokenService
:
https://github.com/strongloop/loopback-next/blob/6eea5e428b145cafb84a998bd53979da8c8fba07/packages/authentication/src/services/token.service.ts#L11-L20
E.g. by adding a refreshToken
method:
/**
* Renews, refreshes or extends ttl a given token string.
*
* @param token A current valid token.
* @param secret A secret, e.g. refreshtoken to secure the operation.
* @returns A promise which resolves to a new or refreshed token
*/
refreshToken(token: string, secret?: string): Promise<string>;
The TokenService
needs to:
Anything i missed?
@derdeka, since you have a PoC, let me assign this to you. Thanks.
While we are at it, I would also suggest that the tokens are HttpOnly
cookies instead of storing them in localStorage
, see here for why this is important. Another advantage of cookies is that the server can set and remove the tokens and clients do not need to know anything about the authentication flow / implementation.
The problem with cookies is that there needs to be some sort of csrf protection which could be achieved by introduction an additional csrf-token header that needs to be send by the client or even setting the cookies to sameSite=strict
could do the job in modern browsers.
Another important point that was not mentioned above is that if the access token is long lived it is impossible to change the access rights of the user which means in a real world application this is not suitable at all.
@derdeka I already developed the authentication flow you described above but in a microservice context. I also think that JWT in general makes more sense in that context, if your application is a monolith I would rather go for session cookies
@nflaig 👎 for cookies. LocalStorage is better suited to the task at hand. Cookies break RESTful best practices as it requires the server to store session information.
@dougal83 I guess you didn't understand my proposal correctly. We are not storing any session information on the server, the only thing that changes is how the JWT is stored on the client which is localStorage vs HttpOnly cookie.
@dougal83 I guess you didn't understand my proposal correctly. We are not storing any session information on the server, the only thing that changes is how the JWT is stored on the client which is localStorage vs HttpOnly cookie.
You could be right but I'm just not convinced atm. I'll read more about it and come back if I change my mind.
Another important point that was not mentioned above is that if the access token is long lived it is impossible to change the access rights of the user which means in a real world application this is not suitable at all.
Furthermore, I don't really see the above statement being a concern with regard to inclusion of the refresh token that this PR proposes. The short lived access token contains the permissions/scopes. The longer lived refresh token simply requests/refreshes the access token and at the point of reissue we can check if access has been revoked or altered.
You could be right but I'm just not convinced atm. I'll read more about it and come back if I change my mind.
There are a lot of great resources with information on web security best practices for example OWASP cheat sheets. Also if you take a look how websites store sensitive information you will not find a single one that uses local storage.
Furthermore, I don't really see the above statement being a concern with regard to inclusion of the refresh token that this PR proposes. The short lived access token contains the permissions/scopes. The longer lived refresh token simply requests/refreshes the access token and at the point of reissue we can check if access has been revoked or altered.
The PR listed 3 different options. This was just another argument against option 1 and why it is not a suitable approach.
You're right that LocalStorage shouldn't be used to store sensitive information. However the OWASP Docs don't highlight a particular issue. The JWT token, is fine in LocalStorage as it is really just a verifiable claim and not the actual credentials for instance.
This was just another argument against option 1 and why it is not a suitable approach.
Fair enough, I boiled down the post in my head to option 3 in agreement with @derdeka.
They are highlighting some issues like XSS in the chapter about local storage. I think the highest risk might be a malicious npm module you are using in your UI code which just gets all items in local storage and sends those to the attacker. I can't really tell how likely it is that such an attack happens but I guess when it comes to security one should try to follow best practices to avoid potential exploits.
@nflaig I don't make any asumptions how the tokens are stored (in cookies or in localstorage). I think this is in the responsibility of the frontend and is out of scope of my POC. But i'm open to discussions about best practices and feedback how other developers handle this. In my case i have a angular frontend which should periodically refresh the accessToken
to avoid interuption of user interaction.
@derdeka yes it probably makes sense to discuss this topic in another issue.
I think this is in the responsibility of the frontend and is out of scope of my POC
This can not be decided by the frontend since the backend sets the cookies in the response. In case the cookies are Http only it is even impossible for the UI javascript code to interact with the cookies at all. But I guess the first step is anyways to do the PoC implementation with refresh tokens and the next step would be security considerations. In my opinion even though this is just a demo application it should still showcase best practices.
This can not be decided by the frontend since the backend sets the cookies in the response.
@nflaig I don't see any cookies in the login process set by loopback-next
or loopback4-example-shopping
. Which application are you referencing?
@derdeka loopback4-example-shopping
does not use cookies but if it would it has to be decided in the backend. Of course if the token is sent back in the request body for example you can decide in the frontend where you want to store it. Usually the backend would use the set-cookie
header in case the tokens are intended to be stored as cookies.
I agree that tokens should be stored as a HTTPOnly
cookie to prevent XSS.
However if I'm not mistaken, verifyToken()
and generateToken()
do not assume where the token is stored (and neither will refreshToken()
). Furthermore, the authentication package only stores the interfaces and do not contain any implementation of the aforementioned functions.
This seems to be more of an issue on how loopback4-example-shopping
implements these interfaces thereby out-of-scope of this issue.
This seems to be more of an issue on how
loopback4-example-shopping
implements these interfaces thereby out-of-scope of this issue.
Agreed. Client side implementation is out of scope of PR.
Just to clarify; are we simply adding a refreshToken
function to the interface?
If so, we should land this before the loopback4-shopping-example
PoC.
Is there any documentation about how to make this work with the example?
Closing as done.
Suggestion
TTL of AccessToken (
JWT
,LB3 legacy token
or any other token system) should be extendable or token should be replaced after some time of beeing used. AFAIK the@loopback/authentication
component does not support this yet.Use Cases
Cross-Posting @sformisano 's comment from https://github.com/strongloop/loopback-next/issues/3673#issuecomment-560954366 as discussion point:
Acceptance criteria
TBD - will be filled by the team.