From aced0a97c4b0d097da003b5d792c1358c7f1cc0d Mon Sep 17 00:00:00 2001 From: Nick Desaulniers Date: Thu, 5 Jul 2012 09:32:07 -0700 Subject: [PATCH] Bug 760910: Handle 401 responses correctly in AITC; r=gps --- services/aitc/modules/client.js | 4 +- services/aitc/modules/manager.js | 80 +++++++++++++++---- services/aitc/tests/unit/test_aitc_manager.js | 63 +++++++++++++-- services/common/tokenserverclient.js | 5 +- 4 files changed, 127 insertions(+), 25 deletions(-) diff --git a/services/aitc/modules/client.js b/services/aitc/modules/client.js index 31b5ba7d499..2dd8a96b309 100644 --- a/services/aitc/modules/client.js +++ b/services/aitc/modules/client.js @@ -376,7 +376,7 @@ AitcClient.prototype = { // Set values from X-Backoff and Retry-After headers, if present. _setBackoff: function _setBackoff(req) { let backoff = 0; - let successfulStatusCodes = [200, 201, 204, 304]; + let successfulStatusCodes = [200, 201, 204, 304, 401]; let val; if (req.response.headers["Retry-After"]) { @@ -387,7 +387,7 @@ AitcClient.prototype = { val = req.response.headers["X-Backoff"]; backoff = parseInt(val, 10); this._log.warn("X-Backoff header was seen: " + val); - } else if (successfulStatusCodes.indexOf(req.response.status) === -1) { + } else if (statusCodesWithoutBackoff.indexOf(req.response.status) === -1) { // Bad status code. this._consecutiveFailures++; if (this._consecutiveFailures === this._maxFailures) { diff --git a/services/aitc/modules/manager.js b/services/aitc/modules/manager.js index cee48a3548b..75b51213279 100644 --- a/services/aitc/modules/manager.js +++ b/services/aitc/modules/manager.js @@ -21,7 +21,7 @@ Cu.import("resource://services-common/tokenserverclient.js"); Cu.import("resource://services-common/utils.js"); const PREFS = new Preferences("services.aitc."); -const TOKEN_TIMEOUT = 240000; // 4 minutes +const INITIAL_TOKEN_DURATION = 240000; // 4 minutes const DASHBOARD_URL = PREFS.get("dashboard.url"); const MARKETPLACE_URL = PREFS.get("marketplace.url"); @@ -29,16 +29,20 @@ const MARKETPLACE_URL = PREFS.get("marketplace.url"); * The constructor for the manager takes a callback, which will be invoked when * the manager is ready (construction is asynchronous). *DO NOT* call any * methods on this object until the callback has been invoked, doing so will - * lead to undefined behaviour. The premadeClient is used - * to bypass BrowserID for xpcshell tests, since the window object is not + * lead to undefined behaviour. The premadeClient and premadeToken are used + * to bypass BrowserID for xpcshell tests, since the window object in not * available. */ -function AitcManager(cb, premadeClient) { +function AitcManager(cb, premadeClient, premadeToken) { this._client = null; this._getTimer = null; this._putTimer = null; - this._lastToken = 0; + this._lastTokenTime = 0; + this._tokenDuration = INITIAL_TOKEN_DURATION; + this._premadeToken = premadeToken || null; + this._invalidTokenFlag = false; + this._lastEmail = null; this._dashboardWindow = null; @@ -225,11 +229,14 @@ AitcManager.prototype = { * not be called and an error will be logged. */ _validateToken: function _validateToken(func) { - if (Date.now() - this._lastToken < TOKEN_TIMEOUT) { + let timeSinceLastToken = Date.now() - this._lastTokenTime; + if (!this._invalidTokenFlag && timeSinceLastToken < this._tokenDuration) { + this._log.info("Current token is valid"); func(); return; } + this._log.info("Current token is invalid"); let win; if (this._state == this.ACTIVE) { win = this._dashboardWindow; @@ -238,10 +245,10 @@ AitcManager.prototype = { let self = this; this._refreshToken(function(err, done) { if (!done) { - this._log.warn("_checkServer could not refresh token, aborting"); + self._log.warn("_checkServer could not refresh token, aborting"); return; } - func(); + func(err); }, win); }, @@ -260,7 +267,14 @@ AitcManager.prototype = { return; } - this._validateToken(this._getApps.bind(this)); + let self = this; + this._validateToken(function validation(err) { + if (err) { + self._log.error(err); + } else { + self._getApps(); + } + }); }, _getApps: function _getApps() { @@ -271,7 +285,16 @@ AitcManager.prototype = { this._client.getApps(function gotApps(err, apps) { if (err) { // Error was logged in client. - return; + if (err.authfailure) { + self._invalidTokenFlag = true; + self._validateToken(function revalidated(err) { + if (!err) { + self._getApps(); + } + }); + } else { + return; + } } if (!apps) { // No changes, got 304. @@ -310,7 +333,14 @@ AitcManager.prototype = { return; } - this._validateToken(this._putApps.bind(this)); + let self = this; + this._validateToken(function validation(err) { + if (err) { + self._log.error(err); + } else { + self._putApps(); + } + }); }, _putApps: function _putApps() { @@ -324,6 +354,17 @@ AitcManager.prototype = { // Send to end of queue if unsuccessful or err.removeFromQueue is false. if (err && !err.removeFromQueue) { self._log.info("PUT failed, re-adding to queue"); + // Error was logged in client. + if (err.authfailure) { + self._invalidTokenFlag = true; + self._validateToken(function validation(err) { + if (err) { + self._log.error("Failed to obtain an updated token"); + } + _reschedule(); + }); + return; + } // Update retries and time record.retries += 1; @@ -427,8 +468,9 @@ AitcManager.prototype = { cb(err, null); return; } - self._lastToken = Date.now(); + self._lastTokenTime = Date.now(); self._client.updateToken(token); + self._invalidTokenFlag = false; cb(null, true); }); return; @@ -449,6 +491,7 @@ AitcManager.prototype = { // makeClient sets an updated token. self._client = client; + self._invalidTokenFlag = false; cb(null, true); }, win); } @@ -459,7 +502,15 @@ AitcManager.prototype = { } else { options.sameEmailAs = MARKETPLACE_URL; } - BrowserID.getAssertion(refreshedAssertion, options); + if (this._premadeToken) { + this._client.updateToken(this._premadeToken); + this._tokenDuration = parseInt(this._premadeToken.duration, 10); + this._lastTokenTime = Date.now(); + this._invalidTokenFlag = false; + cb(null, true); + } else { + BrowserID.getAssertion(refreshedAssertion, options); + } }, /* Obtain a token from Sagrada token server, given a BrowserID assertion @@ -485,6 +536,7 @@ AitcManager.prototype = { _gotToken: function _gotToken(err, tok, cb) { if (!err) { this._log.info("Got token from server: " + JSON.stringify(tok)); + this._tokenDuration = parseInt(tok.duration, 10); cb(null, tok); return; } @@ -539,7 +591,7 @@ AitcManager.prototype = { } // Store when we got the token so we can refresh it as needed. - self._lastToken = Date.now(); + self._lastTokenTime = Date.now(); // We only create one client instance, store values in a pref tree cb(null, new AitcClient( diff --git a/services/aitc/tests/unit/test_aitc_manager.js b/services/aitc/tests/unit/test_aitc_manager.js index 895f30c0583..d3e1b528c87 100644 --- a/services/aitc/tests/unit/test_aitc_manager.js +++ b/services/aitc/tests/unit/test_aitc_manager.js @@ -72,15 +72,62 @@ function get_client_for_server(username, server) { return new AitcClient(token, new Preferences("services.aitc.client.")); } -// Check that a is less than b +// Check that a is less than b. function do_check_lt(a, b) { do_check_true(a < b); } +add_test(function test_401_responses() { + PREFS.set("client.backoff", "50"); + PREFS.set("manager.putFreq", 50); + const app = get_mock_app(); + const username = "123"; + const premadeToken = { + id: "testtest", + key: "testtest", + endpoint: "http://localhost:8080/1.0/123", + uid: "uid", + duration: "5000" + }; + let server = get_server_with_user(username); + server.mockStatus = { + code: 401, + method: "Unauthorized" + } + let client = get_client_for_server(username, server); + let manager = new AitcManager(function () {}, client, premadeToken); + // Assume first token is not out dated. + manager._lastTokenTime = Date.now(); + let mockRequestCount = 0; + let clientFirstToken = null; + + server.onRequest = function mockstatus () { + mockRequestCount++; + switch (mockRequestCount) { + case 1: + clientFirstToken = client.token; + // Switch to using mock 201s. + this.mockStatus = { + code: 201, + method: "Created" + }; + break; + case 2: + // Check that the client obtained a different token. + do_check_neq(client.token.id, clientFirstToken.id); + do_check_neq(client.token.key, clientFirstToken.key); + server.stop(run_next_test); + break; + } + } + + manager.appEvent("install", get_mock_app()); +}); + add_test(function test_client_exponential_backoff() { _("Test that the client is properly setting the backoff"); - // Use prefs to speed up tests + // Use prefs to speed up tests. const putFreq = 50; const initialBackoff = 50; const username = "123"; @@ -90,7 +137,7 @@ add_test(function test_client_exponential_backoff() { let mockRequestCount = 0; let lastRequestTime = Date.now(); - // Create server that returns failure codes + // Create server that returns failure codes. let server = get_server_with_user(username); server.mockStatus = { code: 399, @@ -106,26 +153,26 @@ add_test(function test_client_exponential_backoff() { lastRequestTime = timeNow; // The time between the 3rd and 4th request should be atleast the - // initial backoff + // initial backoff. if (mockRequestCount === 4) { do_check_lt(initialBackoff, timeDiff); // The time beween the 4th and 5th request should be atleast double - // the intial backoff + // the intial backoff. } else if (mockRequestCount === 5) { do_check_lt(initialBackoff * 2, timeDiff); server.stop(run_next_test); } } - // Create dummy client and manager + // Create dummy client and manager. let client = get_client_for_server(username, server); let manager = new AitcManager(function() { CommonUtils.nextTick(gotManager); }, client); function gotManager() { - manager._lastToken = new Date(); - // Create a bunch of dummy apps for the queue to cycle through + manager._lastTokenTime = Date.now(); + // Create a bunch of dummy apps for the queue to cycle through. manager._pending._queue = [ get_mock_queue_element(), get_mock_queue_element(), diff --git a/services/common/tokenserverclient.js b/services/common/tokenserverclient.js index 7a2d116c7bf..8a340302701 100644 --- a/services/common/tokenserverclient.js +++ b/services/common/tokenserverclient.js @@ -114,6 +114,7 @@ TokenServerClient.prototype = { * key (string) HTTP MAC shared symmetric key. * endpoint (string) URL where service can be connected to. * uid (string) user ID for requested service. + * duration (string) the validity duration of the issued token. * * e.g. * @@ -128,7 +129,9 @@ TokenServerClient.prototype = { * return; * } * - * let {id: id, key: key, uid: uid, endpoint: endpoint} = result; + * let { + * id: id, key: key, uid: uid, endpoint: endpoint, duration: duration + * } = result; * // Do stuff with data and carry on. * }); *