From 7224d9528c73c02d24afada320901ed5ce122284 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Wed, 16 Jul 2014 15:30:21 +0100 Subject: [PATCH] fix(client): Ensure a down OAuth server does not cause an `undefined` error. Lots of changes under the hood: * oauth-client.js: Convert jQuery promises to rest of the world promises to ensure compatibility. * oauth-client.js: Convert server error responses to OAuthError based errors. * oauth-client.js: Non-reachable OAuth server returns a SERVICE_UNAVAILABLE error * oauth-errors.js: Add all possible oauth error codes, whether they are used or not. * service-mixin.js: Work with OAuthError's instead of raw JSON responses. fixes #1344 --- app/scripts/lib/auth-errors.js | 6 +- app/scripts/lib/oauth-client.js | 19 +++- app/scripts/lib/oauth-errors.js | 22 +++- app/scripts/views/mixins/service-mixin.js | 8 +- app/tests/spec/lib/oauth-client.js | 133 ++++++++++++++++------ 5 files changed, 142 insertions(+), 46 deletions(-) diff --git a/app/scripts/lib/auth-errors.js b/app/scripts/lib/auth-errors.js index fad8d55498..a3026760af 100644 --- a/app/scripts/lib/auth-errors.js +++ b/app/scripts/lib/auth-errors.js @@ -31,13 +31,11 @@ function () { MISSING_CONTENT_LENGTH_HEADER: 112, REQUEST_TOO_LARGE: 113, THROTTLED: 114, - // If the auth server is unavailable, it will not respond. - // Use a client side only code. - SERVICE_UNAVAILABLE: 998, SERVER_BUSY: 201, ENDPOINT_NOT_SUPPORTED: 116, // local only error codes + SERVICE_UNAVAILABLE: 998, USER_CANCELED_LOGIN: 1001, SESSION_EXPIRED: 1002, @@ -72,11 +70,11 @@ function () { 112: t('Missing content-length header'), 113: t('Request body too large'), 114: t('Attempt limit exceeded'), - 998: t('System unavailable, try again soon'), 201: t('Server busy, try again soon'), 116: t('This endpoint is no longer supported'), // local only error messages + 998: t('System unavailable, try again soon'), 1002: t('Session expired. Sign in to continue.'), 1003: t('Cookies are still disabled'), 1004: t('Passwords do not match'), diff --git a/app/scripts/lib/oauth-client.js b/app/scripts/lib/oauth-client.js index 3c420575ce..326fc0c3af 100644 --- a/app/scripts/lib/oauth-client.js +++ b/app/scripts/lib/oauth-client.js @@ -8,9 +8,10 @@ define([ 'jquery', 'lib/promise', 'lib/session', - 'lib/config-loader' + 'lib/config-loader', + 'lib/oauth-errors' ], -function ($, p, Session, ConfigLoader) { +function ($, p, Session, ConfigLoader, OAuthErrors) { var oauthUrl; var GET_CLIENT = '/v1/client/'; @@ -25,6 +26,20 @@ function ($, p, Session, ConfigLoader) { } } + function transformError(xhr) { + if (!xhr || xhr.status === 0) { + return OAuthErrors.toError('SERVICE_UNAVAILABLE'); + } + + var errObj = xhr.responseJSON; + + if (!errObj) { + return OAuthErrors.toError('UNEXPECTED_ERROR'); + } + + return OAuthErrors.toError(errObj.errno); + } + OAuthClient.prototype = { _getOauthUrl: function _getOauthUrl() { var configLoader = new ConfigLoader(); diff --git a/app/scripts/lib/oauth-errors.js b/app/scripts/lib/oauth-errors.js index 7ab34ef82b..2bd3d10d19 100644 --- a/app/scripts/lib/oauth-errors.js +++ b/app/scripts/lib/oauth-errors.js @@ -16,22 +16,36 @@ function (_, AuthErrors) { }; var ERROR_TO_CODE = { - UNEXPECTED_ERROR: 999, UNKNOWN_CLIENT: 101, + INCORRECT_CLIENT_SECRET: 102, INCORRECT_REDIRECT: 103, INVALID_ASSERTION: 104, + UNKNOWN_CODE: 105, + INCORRECT_CODE: 106, + EXPIRED_CODE: 107, INVALID_PARAMETER: 108, - INVALID_REQUEST_SIGNATURE: 109 + INVALID_REQUEST_SIGNATURE: 109, + + // local only errors. + SERVICE_UNAVAILABLE: 998, + UNEXPECTED_ERROR: 999 }; var CODE_TO_MESSAGES = { // errors returned by the oauth server - 999: t('Unexpected error'), 101: t('Unknown client'), + 102: t('Incorrect client secret'), 103: t('Incorrect redirect_uri'), 104: t('Invalid assertion'), + 105: t('Unknown code'), + 106: t('Incorrect code'), + 107: t('Expired code'), 108: t('Invalid parameter in request body: %(param)s'), - 109: t('Invalid request signature') + 109: t('Invalid request signature'), + + // local only errors. + 998: t('System unavailable, try again soon'), + 999: t('Unexpected error') }; return _.extend({}, AuthErrors, { diff --git a/app/scripts/views/mixins/service-mixin.js b/app/scripts/views/mixins/service-mixin.js index 2ac7911a93..32e72d0f1d 100644 --- a/app/scripts/views/mixins/service-mixin.js +++ b/app/scripts/views/mixins/service-mixin.js @@ -61,8 +61,8 @@ define([ self.serviceName = clientInfo.name; self.serviceRedirectURI = clientInfo.redirect_uri; }) - .fail(function(xhr) { - self.displayError(xhr.responseJSON, OAuthErrors); + .fail(function(err) { + self.displayError(err, OAuthErrors); }); }, @@ -86,9 +86,9 @@ define([ self.window.location.href = result.redirect; return { pageNavigation: true }; }) - .fail(function(xhr) { + .fail(function(err) { Session.clear('oauth'); - self.displayError(xhr.responseJSON, OAuthErrors); + self.displayError(err, OAuthErrors); }); }, diff --git a/app/tests/spec/lib/oauth-client.js b/app/tests/spec/lib/oauth-client.js index 493670011c..207a0d85a5 100644 --- a/app/tests/spec/lib/oauth-client.js +++ b/app/tests/spec/lib/oauth-client.js @@ -44,43 +44,112 @@ function (chai, $, testHelpers, sinon, }); describe('oauth-client', function () { - it('calls getCode', function () { - /* jshint camelcase: false */ - var redirect = RP_URL + '?code=code&state=state'; - - server.respondWith('POST', OAUTH_URL + '/v1/authorization', - [200, { 'Content-Type': 'application/json' }, - '{ "redirect": "' + redirect + '" }']); - - var params = { - assertion: 'assertion', - client_id: 'deadbeef', - redirect_uri: 'http://example.com', - scope: 'profile', - state: 'state' - }; - - return client.getCode(params) - .then(function (result) { - assert.ok(result); - assert.equal(result.redirect, redirect); - }); + describe('getCode', function () { + it('normally responds with a redirect', function () { + /* jshint camelcase: false */ + var redirect = RP_URL + '?code=code&state=state'; + + server.respondWith('POST', OAUTH_URL + '/v1/authorization', + [200, { 'Content-Type': 'application/json' }, + '{ "redirect": "' + redirect + '" }']); + + var params = { + assertion: 'assertion', + client_id: 'deadbeef', + redirect_uri: 'http://example.com', + scope: 'profile', + state: 'state' + }; + + return client.getCode(params) + .then(function (result) { + assert.ok(result); + assert.equal(result.redirect, redirect); + }); + }); + + it('responds with a SERVICE_UNAVAILABLE error if the service is unavailable', function () { + var clientId = 'clientId'; + + server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, + [0, {}, '']); + + return client.getClientInfo(clientId) + .then(function (result) { + assert.fail('unexpected success'); + }, function (err) { + assert.isTrue(OAuthErrors.is(err, 'SERVICE_UNAVAILABLE')); + }); + }); + + it('converts returned errors to OAuth error objects', function () { + var clientId = 'clientId'; + + server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, + [400, { 'Content-Type': 'application/json' }, + JSON.stringify({ + errno: OAuthErrors.toCode('INCORRECT_REDIRECT'), + code: 400 + })]); + + + return client.getClientInfo(clientId) + .then(function (result) { + assert.fail('unexpected success'); + }, function (err) { + assert.isTrue(OAuthErrors.is(err, 'INCORRECT_REDIRECT')); + }); + }); }); - it('calls getClientInfo', function () { + describe('getClientInfo', function () { var clientId = 'clientId'; - server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, - [200, { 'Content-Type': 'application/json' }, - '{ "name": "MozRP", "imageUri": "https://mozilla.org/firefox.png" }']); - - return client.getClientInfo(clientId) - .then(function (result) { - assert.ok(result); - assert.equal(result.name, 'MozRP'); - }); + it('normally response with a name and imageUri', function () { + server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, + [200, { 'Content-Type': 'application/json' }, + '{ "name": "MozRP", "imageUri": "https://mozilla.org/firefox.png" }']); + + return client.getClientInfo(clientId) + .then(function (result) { + assert.ok(result); + assert.equal(result.name, 'MozRP'); + }); + }) + + it('responds with a SERVICE_UNAVAILABLE error if the service is unavailable', function () { + var clientId = 'clientId'; + + server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, + [0, {}, '']); + + return client.getClientInfo(clientId) + .then(function (result) { + assert.fail('unexpected success'); + }, function (err) { + assert.isTrue(OAuthErrors.is(err, 'SERVICE_UNAVAILABLE')); + }); + }); + + it('converts returned errors to OAuth error objects', function () { + var clientId = 'clientId'; + + server.respondWith('GET', OAUTH_URL + '/v1/client/' + clientId, + [400, { 'Content-Type': 'application/json' }, + JSON.stringify({ + errno: OAuthErrors.toCode('EXPIRED_CODE'), + code: 400 + })]); + + + return client.getClientInfo(clientId) + .then(function (result) { + assert.fail('unexpected success'); + }, function (err) { + assert.isTrue(OAuthErrors.is(err, 'EXPIRED_CODE')); + }); + }); }); - }); });