Switch from localStorage to Silent auth #1
Conversation
| render() { | ||
| const { auth } = this.state; | ||
| // Supress rendering until the token renewal check is completed. | ||
| if (!this.state.tokenRenewalComplete) return "Loading..."; |
There was a problem hiding this comment.
In a scenario a little bit more complex, I have created a SplashScreen HOC that triggers checkSession (and after that that signs in on Firebase with a custom token, but that is not important) and that keeps showing a loading screen.
https://github.com/Digituz/krebseng-panel/blob/master/src/components/withSplashScreen.js
Feel free to ping me if you are interested on more info.
There was a problem hiding this comment.
Nice. Makes sense. Thanks for sharing!
|
|
||
| logout = () => { | ||
| localStorage.removeItem("access_token"); | ||
| localStorage.removeItem("id_token"); |
There was a problem hiding this comment.
expires_at should be removed from localStorage as well. This information is related to the tokens generated and, as such, will be useless when you reload the page. The checkSession method will get you a new one.
There was a problem hiding this comment.
I was using expires_at to avoid the overhead of calling renewToken when the user's session has expired or doesn't exist. Note that I'm checking in App.js to suppress the call to renewToken when the expiration has passed or is null.
So it's a perf enhancement. Make sense?
There was a problem hiding this comment.
I see. Well, the problem here is that expires_at doesn't represent when the users' session at Auth0 will expire, just when the token you got will expire. Users' sessions at Auth0 last for 24h+ (by default, if I'm not mistaken), tokens usually last for a couple of hours.
The guidance on how to approach this was defined a few days ago to be honest, so we don't have any public content mentioning it (I think). But, basically, fixing this is just as simple as adding a flag (e.g., apparentlySignedIn < terrible var name 😄) that, when your app loads, it checks if this flag is true and then tries to checkSession. If this flag is falsy, the app avoids the overhead.
There was a problem hiding this comment.
What is apparentlySignedIn set to? I assume the value stored in localStorage?
There was a problem hiding this comment.
yes, you can persist this in localStorage. Initially, when your users load your app for the first time, there will be nothing there (falsy). Then, when they sign in and get redirected back from Auth0, your app will changed to true.
With that in place, if your users refresh (or close & reopen) your app, your app will see true there and will check if the session at Auth0 is still up. If it is, you will get back tokens, as you already know.
Now, when your users hit logout, your app will set this flag to false so when the app refreshes, no checkSession call is made.
To give more context, another alternative would be making your app's signOut kill the session at Auth0. But then you would kill a session that might be used for SSO from a particular app, which is not ideal.
There was a problem hiding this comment.
Thanks Bruno! I've updated my approach to store "checkSession" in memory as you suggested. Look good?
I've also moved expires_at to an instance var.
Assuming this all looks good to you, my only reservation is how to handle the API calls. I think I'll move them back outside Auth.js to the React components, and accept that they'll make a call to this.props.auth.getAccessToken. I'm not sure moving them inside Auth.js is buying me anything.
There was a problem hiding this comment.
@brunokrebs - I just added a short 4 min clip explaining approaches for storing tokens. I'd love your feedback: https://www.dropbox.com/s/5ntaf4acpnpc4ec/react-auth0-authentication-security-m4-05.mp4?dl=0
There was a problem hiding this comment.
Yeah, I agree that moving them inside Auth.js doesn't really add any value. Oh, and I just checked the clip and it looks just fine for me. I liked the comparison table you provided and that you briefly explained some trade offs.
There was a problem hiding this comment.
Excellent. Thanks so much for all your valuable input Bruno! 🔥
| // Delay in milliseconds before requesting renewal. | ||
| // Will be 7200000 (120 minutes) immediately after login | ||
| // since Auth0's default token expiration is 2 hours. | ||
| if (delay > 0) setTimeout(() => this.renewToken(), delay); |
There was a problem hiding this comment.
good stuff, I should add this too in my article 😄
src/Auth/Auth.js
Outdated
| // This protects from XSS since the access token isn't accessible outside this file | ||
|
|
||
| getPrivate() { | ||
| return fetch("/private", { |
There was a problem hiding this comment.
An attacker could just overwrite window.fetch and access the token that way instead?
There was a problem hiding this comment.
Perhaps. Shoot, if I was using Axios instead, I'd be referencing an import and thus overriding it wouldn't be possible. That said, I'm not sure if what I'm trying here is helpful/necessary. Unfortunately, I can find no docs on people doing this beyond @brunokrebs post. I don't see how storing in memory protects from XSS if the values in memory are just as easily accessible via JS.
There was a problem hiding this comment.
Accessing in-memory variables like _idToken and _accessToken is as easy as parsing content available at localStorage?
An yeah, overwriting window.fetch would give you access indeed, @jrwebdev But, importing and using a module like axios would fix that I believe.
There was a problem hiding this comment.
You could do the same thing with XMLHttpRequest (which Axios uses behind the scenes in place of fetch).
I was drawn to this because I researched it recently and I feel your pain with this - there is a severe lack of best practice articles on the web on the matter, especially for single page applications. I'm by no means a security expert, but my take is that if you're using third-party scripts, including node_modules (even ones that are part of your build), then that's where the real problem lies, so it's just then a matter of taking steps to mitigate the risks involved.
There was a problem hiding this comment.
That's a great point @jrwebdev. I'm going to revert moving the API calls here and put them back in their original React components.
No description provided.