From 752e6676e1c18eb269f4e2b19415c0f1f6c7c7cc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 22 Mar 2016 16:54:01 +0100 Subject: [PATCH 1/4] Detect user navigating away, don't interpret as ajax error Whenever a user navigates away, all ajax calls will fail with the same result like a cross-domain redirect (SSO). To distinguish these cases, we need to detect whether the error is a result of the user navigating away. For this, we introduce a new flag that will be set in "beforeunload". Additional handling was required for false positives in case "beforeunload" is used (ex: cancelled upload) and the user cancelled the navigation. --- core/js/js.js | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/core/js/js.js b/core/js/js.js index e90ceaf4e1..584bf1c1c8 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -741,7 +741,9 @@ var OC={ */ _processAjaxError: function(xhr) { // purposefully aborted request ? - if (xhr.status === 0 && (xhr.statusText === 'abort' || xhr.statusText === 'timeout')) { + // this._userIsNavigatingAway needed to distinguish ajax calls cancelled by navigating away + // from calls cancelled by failed cross-domain ajax due to SSO redirect + if (xhr.status === 0 && (xhr.statusText === 'abort' || xhr.statusText === 'timeout' || this._userIsNavigatingAway)) { return; } @@ -1438,6 +1440,29 @@ function initCore() { $('html').addClass('edge'); } + $(window).on('unload', function() { + OC._unloadCalled = true; + }); + $(window).on('beforeunload', function() { + // super-trick thanks to http://stackoverflow.com/a/4651049 + // in case another handler displays a confirmation dialog (ex: navigating away + // during an upload), there are two possible outcomes: user clicked "ok" or + // "cancel" + + // first timeout handler is called after unload dialog is closed + setTimeout(function() { + OC._userIsNavigatingAway = true; + + // second timeout event is only called if user cancelled (Chrome), + // but in other browsers it might still be triggered, so need to + // set a higher delay... + setTimeout(function() { + if (!OC._unloadCalled) { + OC._userIsNavigatingAway = false; + } + }, 10000); + },1); + }); $(document).on('ajaxError.main', function( event, request, settings ) { if (settings && settings.allowAuthErrors) { return; From d8261d41cc7777c460a0a240b2b6291b7962b1b9 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 22 Mar 2016 16:55:43 +0100 Subject: [PATCH 2/4] Firefox returns 303 on cross-domain redirect Added 303 to catch SSO cross-domain redirect in Firefox. --- core/js/js.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/js/js.js b/core/js/js.js index 584bf1c1c8..1a3b532ed4 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -747,7 +747,7 @@ var OC={ return; } - if (_.contains([0, 302, 307, 401], xhr.status)) { + if (_.contains([0, 302, 303, 307, 401], xhr.status)) { OC.reload(); } }, From 51ff3e7443592461dba9ba3730e80ca775f2c04b Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 22 Mar 2016 18:28:54 +0100 Subject: [PATCH 3/4] Stronger fix for navigate away detection --- core/js/js.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/js/js.js b/core/js/js.js index 1a3b532ed4..f4e0fa464d 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -740,15 +740,23 @@ var OC={ * if an error/auth error status was returned. */ _processAjaxError: function(xhr) { + var self = this; // purposefully aborted request ? // this._userIsNavigatingAway needed to distinguish ajax calls cancelled by navigating away // from calls cancelled by failed cross-domain ajax due to SSO redirect - if (xhr.status === 0 && (xhr.statusText === 'abort' || xhr.statusText === 'timeout' || this._userIsNavigatingAway)) { + if (xhr.status === 0 && (xhr.statusText === 'abort' || xhr.statusText === 'timeout' || self._reloadCalled)) { return; } if (_.contains([0, 302, 303, 307, 401], xhr.status)) { - OC.reload(); + // sometimes "beforeunload" happens later, so need to defer the reload a bit + setTimeout(function() { + if (!self._userIsNavigatingAway && !self._reloadCalled) { + OC.reload(); + // only call reload once + self._reloadCalled = true; + } + }, 100); } }, From c58e96e6395dff5cf914d66df099922c0b1b6981 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 23 Mar 2016 10:53:40 +0100 Subject: [PATCH 4/4] Adjust core unit tests for unload/reload cases --- core/js/js.js | 4 +-- core/js/tests/specs/coreSpec.js | 45 ++++++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/core/js/js.js b/core/js/js.js index f4e0fa464d..b74775a935 100644 --- a/core/js/js.js +++ b/core/js/js.js @@ -1448,10 +1448,10 @@ function initCore() { $('html').addClass('edge'); } - $(window).on('unload', function() { + $(window).on('unload.main', function() { OC._unloadCalled = true; }); - $(window).on('beforeunload', function() { + $(window).on('beforeunload.main', function() { // super-trick thanks to http://stackoverflow.com/a/4651049 // in case another handler displays a confirmation dialog (ex: navigating away // during an upload), there are two possible outcomes: user clicked "ok" or diff --git a/core/js/tests/specs/coreSpec.js b/core/js/tests/specs/coreSpec.js index 774c2fdc72..f18ecbc1a4 100644 --- a/core/js/tests/specs/coreSpec.js +++ b/core/js/tests/specs/coreSpec.js @@ -20,6 +20,15 @@ */ describe('Core base tests', function() { + afterEach(function() { + // many tests call window.initCore so need to unregister global events + // ideally in the future we'll need a window.unloadCore() function + $(document).off('ajaxError.main'); + $(document).off('unload.main'); + $(document).off('beforeunload.main'); + OC._userIsNavigatingAway = false; + OC._reloadCalled = false; + }); describe('Base values', function() { it('Sets webroots', function() { expect(OC.webroot).toBeDefined(); @@ -925,9 +934,10 @@ describe('Core base tests', function() { }); }); describe('global ajax errors', function() { - var reloadStub, ajaxErrorStub; + var reloadStub, ajaxErrorStub, clock; beforeEach(function() { + clock = sinon.useFakeTimers(); reloadStub = sinon.stub(OC, 'reload'); // unstub the error processing method ajaxErrorStub = OC._processAjaxError; @@ -936,15 +946,17 @@ describe('Core base tests', function() { }); afterEach(function() { reloadStub.restore(); - $(document).off('ajaxError'); + clock.restore(); }); - it('reloads current page in case of auth error', function () { + it('reloads current page in case of auth error', function() { var dataProvider = [ [200, false], [400, false], + [0, true], [401, true], [302, true], + [303, true], [307, true] ]; @@ -953,9 +965,13 @@ describe('Core base tests', function() { var expectedCall = dataProvider[i][1]; reloadStub.reset(); + OC._reloadCalled = false; $(document).trigger(new $.Event('ajaxError'), xhr); + // trigger timers + clock.tick(1000); + if (expectedCall) { expect(reloadStub.calledOnce).toEqual(true); } else { @@ -963,6 +979,27 @@ describe('Core base tests', function() { } } }); - }) + it('reload only called once in case of auth error', function() { + var xhr = { status: 401 }; + + $(document).trigger(new $.Event('ajaxError'), xhr); + $(document).trigger(new $.Event('ajaxError'), xhr); + + // trigger timers + clock.tick(1000); + + expect(reloadStub.calledOnce).toEqual(true); + }); + it('does not reload the page if the user was navigating away', function() { + var xhr = { status: 0 }; + OC._userIsNavigatingAway = true; + clock.tick(100); + + $(document).trigger(new $.Event('ajaxError'), xhr); + + clock.tick(1000); + expect(reloadStub.notCalled).toEqual(true); + }); + }); });