-
-
Notifications
You must be signed in to change notification settings - Fork 750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document using localStorage shim in certain private browsers #1344
Comments
The crappy part in Safari is that you actually have to call |
Yup, that's what meteor does. It's not elegant, but it's certainly an accepted solution to resolving the corner case. |
I'd like to help get this implemented. Instead of using the memory provider, why don't we use cookies? They work in Private Browsing mode, and as long as we're not consuming them on the server, they're basically the same as localStorage. The additional bonus of using them would be that they persist on refreshes, where memory-based storage won't. Maybe we should make this part configurable: |
I actually think in 95% of use cases it already works just fine. That's just me throwing out a random number and pretending to be definitive.... but how many people actually use private browsing mode only on Safari? It sort of defeats the purpose of storing any token anyway in private browsing mode... I think the shim is a totally valid option. I'm also wondering if it is something that should just be left up to the developer to implement?? You have to think about React Native as well, which doesn't support cookies unless you are inside a webview because RN isn't a browser. |
The number of users you encounter using Safari in Private Browsing mode increases a lot when you are talking about a financial application, or any app where users naturally want to do more to protect potentially-sensitive data. In this case, though, the app can simply pass a cookie storage engine and not worry about localStorage at all. I can always check if localStorage is available and switch the storageEngine based on that. function getTokenLocation(){
let localStorageAvailable = true;
// Only run this in the browser. (localStorage isn't available on the SSR server)
if (window.localStorage) {
// Try localStorage
try{
localStorage.setItem('2398sdvlkn4v', 'test');
localStorage.removeItem('2398sdvlkn4v');
}
catch(err){
localStorageAvailable = false;
}
// If localStorage isn't available, try reading from a cookie.
if (localStorageAvailable) {
return 'localStorage';
} else {
// document.cookie = 'feathers-jwt=hi';
return 'cookie';
}
}
}
getTokenLocation(); |
Looks like somebody already made a web storage compatible syntax around document.cookie: |
Why don't we get opinionated about the storage engine used by default? We can make the user experience better by providing a
|
If we want to keep it more modular and not have the cookie-storage package as part of this one, then maybe we just allow the |
You can easily provide your own store by passing an object that implements an interface like: {
getItem(key) {},
setItem(key, value) {}
} We can add a |
@ekryski what works for React Native? |
I believe this issue should be closed and a new issue should be created under feathers-authentication-client. |
I think this can be solved by directing people to use localForage. We can focus on solutions that others aren't already solving. Anybody object to closing this? |
Here's my current solution for safari (private browsing mode): /* eslint no-undefined: 0 */
export default function getStorage() {
const storageKey = '_localstorage_test_';
let retrieved;
try {
if (window.localStorage) {
window.localStorage.setItem(storageKey, storageKey);
retrieved = window.localStorage.getItem(storageKey);
window.localStorage.removeItem(storageKey);
}
} catch (e) {
// ... ignore
}
if (storageKey === retrieved) {
return window.localStorage;
}
return {
data: {},
setItem: function (key, val) {
this.data[key] = val;
},
removeItem: function (key) {
delete this.data[key];
},
getItem: function (key) {
const value = this.data[key];
return value === undefined ? null : value;
}
};
} init like this: import auth from 'feathers-authentication-client';
import feathers from 'feathers/client';
import hooks from 'feathers-hooks';
import io from 'socket.io-client';
import socketio from 'feathers-socketio/client';
const socket = io('http://api/url');
const app = feathers()
.configure(hooks())
.configure(socketio(socket))
.configure(auth({
storage: getStorage()
})); |
Yeah @marshallswain I agree that for web the use case, we should probably just be a documentation update to discuss edge cases when using localStorage (in normal web environments), an inform people to use one of the polyfills in environments where I believe for React Native you'd probably just use RN's AsyncStorage, however on RN you're given a Promise-based API for import auth from 'feathers-authentication-client';
import feathers from 'feathers/client';
import hooks from 'feathers-hooks';
import io from 'socket.io-client';
import socketio from 'feathers-socketio/client';
import {AsyncStorage} from 'react-native';
const socket = io('http://api/url');
const app = feathers()
.configure(hooks())
.configure(socketio(socket))
.configure(auth({
storage: AsyncStorage
})); In light of that, you may want to ensure that the auth client works with a storage API that returns promises instead of synchronous values. I'm not sure if it does this currently. But it wouldn't be hard to add support for it. |
This works with AsyncStorage as @ekryski and I are using it in a few RN apps |
The getStorage implementation needs to be augmented to supported private browsers on Safari OS X, Safari iOS, and Android browsers.
Users typically supply their own storage implementation:
window.localStorage
is defined on these browsers, so it will be returned, rather than the shim. Consequently, any calls tosetItem
(e.g., https://github.com/feathersjs/feathers-authentication/blob/fca25f2b5b03b773975595ae9ac034199df5127b/src/client/index.js#L40-L42) will throw exceptions in these browsers.More info in this comment on #132.
The text was updated successfully, but these errors were encountered: