Skip to content
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.

Sign up for GitHub

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

Use CookieStorage when accessing localStorage throws an error #698

Merged
merged 4 commits into from
Mar 8, 2018
Merged

Use CookieStorage when accessing localStorage throws an error #698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6df346
Fixing invalid access to localStorage
luisrudge Feb 22, 2018
9f7d63f
fix .only test
luisrudge Mar 8, 2018
a35a7cb
Merge branch 'master' into feature-new-storage
luisrudge Mar 8, 2018
d0a13ca
add error to console
luisrudge Mar 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions example/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ <h2>Console:</h2>

var webAuth = new auth0.WebAuth({
domain: 'brucke.auth0.com',
redirectUri: 'https://localhost:3000/example',
redirectUri: 'https://localhost:3000/example/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this URL changed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying it in safari and it doesn't accept the #access_token if you don't have a / in the end 🤦‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested in other browsers to make sure this addition doesn't do something silly with this addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chrome/safari/firefox. here's the thing: we already have code that handles when you can't read/write from the storage, the issue was that safari specifically was throwing an exception even before that (just by referencing localSTorage).

clientID: 'k5u3o2fiAA8XweXEEX604KCwCjzjtMU6',
responseType: 'token',
plugins: [
Expand All @@ -211,7 +211,7 @@ <h2>Console:</h2>

var webAuthPasswordless = new auth0.WebAuth({
domain: 'brucke.auth0.com',
redirectUri: 'https://localhost:3000/example',
redirectUri: 'https://localhost:3000/example/',
clientID: 'VZUvmnj3pd9yNgq8BjX4YA8Km14jQ0PN',
responseType: 'token'
});
Expand Down
10 changes: 9 additions & 1 deletion src/helper/storage/handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ var Warn = require('../warn');

function StorageHandler() {
this.warn = new Warn({});
this.storage = windowHandler.getWindow().localStorage || new CookieStorage();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR name is use cookiestorage when local not available. It already did this though? This is a fix to catch errors no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

this.storage = new CookieStorage();
try {
// some browsers throw an error when trying to access localStorage
// when localStorage is disabled.
this.storage = windowHandler.getWindow().localStorage;
} catch (e) {
this.warn.warning(e);
this.warn.warning("Can't use localStorage. Using CookieStorage instead.");
}
}

StorageHandler.prototype.failover = function() {
Expand Down
16 changes: 14 additions & 2 deletions test/helper/storage-handler.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,21 @@ describe('helpers storage handler', function() {
windowHandler.getWindow.restore();
});

it('should use cookie storage is localstorage is not available', function() {
it('should use cookie storage when localstorage is not available', function() {
stub(windowHandler, 'getWindow', function(message) {});

var handler = new StorageHandler();
expect(handler.storage).to.be.a(CookieStorage);

windowHandler.getWindow.restore();
});
it('should use cookie storage when localstorage throws an error', function() {
stub(windowHandler, 'getWindow', function(message) {
return {};
return {
get localStorage() {
throw new Error('asdasd');
}
};
});

var handler = new StorageHandler();
Expand Down