From bbdf31697f301153ebf0f5433d75d5694da5591d Mon Sep 17 00:00:00 2001 From: Andrew Sutherland Date: Fri, 14 Dec 2012 00:59:13 -0500 Subject: [PATCH] checkpoint; still writing unit tests, but largely there --- data/lib/imap.js | 10 +- data/lib/mailapi/accountcommon.js | 174 ++++++--- data/lib/mailapi/imap/account.js | 63 +--- data/lib/mailapi/imap/probe.js | 124 +++++-- data/lib/mailapi/mailapi.js | 21 +- data/lib/mailapi/mailbridge.js | 3 +- data/lib/mailapi/mailuniverse.js | 3 +- data/lib/mailapi/smtp/probe.js | 85 +++-- node-deps/simplesmtp | 2 +- test/unit/resources/fake_xhr.js | 44 +++ test/unit/resources/fault_injecting_socket.js | 15 + test/unit/resources/window_shims.js | 1 + test/unit/test_autoconfig.js | 338 ++++++++++++++++++ test/unit/test_imap_prober.js | 34 +- test/unit/test_smtp_prober.js | 155 ++++++++ test/unit/xpcshell.ini | 3 + 16 files changed, 920 insertions(+), 155 deletions(-) create mode 100644 test/unit/resources/fake_xhr.js create mode 100644 test/unit/test_autoconfig.js create mode 100644 test/unit/test_smtp_prober.js diff --git a/data/lib/imap.js b/data/lib/imap.js index 54f85f42..318512cc 100644 --- a/data/lib/imap.js +++ b/data/lib/imap.js @@ -749,17 +749,19 @@ ImapConnection.prototype.connect = function(loginCb) { }; this._state.conn.onerror = function(evt) { try { - var err = evt.data; + var err = evt.data, errType = 'unknown'; // (only do error probing on things we can safely use 'in' on) if (err && typeof(err) === 'object') { // detect an nsISSLStatus instance by an unusual property. - if ('isNotValidAtThisTime' in err) - err = 'bad-security'; + if ('isNotValidAtThisTime' in err) { + err = new Error('SSL error'); + errType = err.type = 'bad-security'; + } } clearTimeoutFunc(self._state.tmrConn); if (self._state.status === STATES.NOCONNECT) { var connErr = new Error('Unable to connect. Reason: ' + err); - connErr.type = 'unknown'; + connErr.type = errType; connErr.serverResponse = ''; loginCb(connErr); } diff --git a/data/lib/mailapi/accountcommon.js b/data/lib/mailapi/accountcommon.js index dd6a2891..9f79604b 100644 --- a/data/lib/mailapi/accountcommon.js +++ b/data/lib/mailapi/accountcommon.js @@ -221,7 +221,7 @@ CompositeAccount.prototype = { this.universe.appendMessages(sentFolder.id, [message]); } - callback(err, errDetails); + callback(err, errDetails, null); }.bind(this)); }, @@ -362,18 +362,18 @@ Configurators['imap+smtp'] = { ['imap', 'smtp'], function probesDone(results) { // -- both good? - if (!results.imap[0] && results.smtp) { + if (results.imap[0] === null && results.smtp === null) { var account = self._defineImapAccount( universe, userDetails, credentials, imapConnInfo, smtpConnInfo, results.imap[1], results.imap[2]); - callback(null, account); + callback(null, account, null); } // -- either/both bad else { // clean up the imap connection if it was okay but smtp failed - if (!results.imap[0]) { + if (results.imap[0] === null) { results.imap[1].die(); // Failure was caused by SMTP, but who knows why callback('smtp-unknown', null); @@ -434,7 +434,7 @@ Configurators['imap+smtp'] = { var account = this._loadAccount(universe, accountDef, oldAccountInfo.folderInfo); - callback(null, account); + callback(null, account, null); }, /** @@ -538,7 +538,7 @@ Configurators['fake'] = { }; var account = this._loadAccount(universe, accountDef); - callback(null, account); + callback(null, account, null); }, recreateAccount: function cfg_fake_ra(universe, oldVersion, oldAccountInfo, @@ -568,7 +568,7 @@ Configurators['fake'] = { }; var account = this._loadAccount(universe, accountDef); - callback(null, account); + callback(null, account, null); }, /** @@ -603,18 +603,34 @@ Configurators['activesync'] = { conn.timeout = $asacct.DEFAULT_TIMEOUT_MS; conn.connect(function(error, options) { - // XXX: Think about what to do with this error handling, since it's - // replicated in the autoconfig code. if (error) { - var failureType = 'unknown'; + // This error is basically an indication of whether we were able to + // call getOptions or not. If the XHR request completed, we get an + // HttpError. If we timed out or an XHR error occurred, we get a + // general Error. + var failureType, + failureDetails = { server: domainInfo.incoming.server }; if (error instanceof $asproto.HttpError) { - if (error.status === 401) + if (error.status === 401) { failureType = 'bad-user-or-pass'; - else if (error.status === 403) + } + else if (error.status === 403) { failureType = 'not-authorized'; + } + // Treat any other errors where we talked to the server as a problem + // with the server. + else { + failureType = 'server-problem'; + failureDetails.status = error.status; + } + } + else { + // We didn't talk to the server, so let's call it an unresponsive + // server. + failureType = 'unresponsive-server'; } - callback(failureType, null); + callback(failureType, null, failureDetails); return; } @@ -644,7 +660,7 @@ Configurators['activesync'] = { }; var account = self._loadAccount(universe, accountDef, conn); - callback(null, account); + callback(null, account, null); }); }, @@ -673,7 +689,7 @@ Configurators['activesync'] = { }; var account = this._loadAccount(universe, accountDef, null); - callback(null, account); + callback(null, account, null); }, /** @@ -760,6 +776,20 @@ function Autoconfigurator(_LOG) { } exports.Autoconfigurator = Autoconfigurator; Autoconfigurator.prototype = { + /** + * The list of fatal error codes. + * + * What's fatal and why: + * - bad-user-or-pass: We found a server, it told us the credentials were + * bogus. There is no point going on. + * - not-authorized: We found a server, it told us the credentials are fine + * but the access rights are insufficient. There is no point going on. + * + * Non-fatal and why: + * - unknown: If something failed we should keep checking other info sources. + * - no-config-info: The specific source had no details; we should keep + * checking other sources. + */ _fatalErrors: ['bad-user-or-pass', 'not-authorized'], /** @@ -791,7 +821,9 @@ Autoconfigurator.prototype = { xhr.onload = function() { if (xhr.status < 200 || xhr.status >= 300) { - callback('unknown'); + // Non-fatal failure to get the config info. While a 404 is the + // expected case, this is the appropriate error for weirder cases too. + callback('no-config-info', null, { status: xhr.status }); return; } // XXX: For reasons which are currently unclear (possibly a platform @@ -823,19 +855,36 @@ Autoconfigurator.prototype = { config.type = 'imap+smtp'; for (let [,child] in Iterator(outgoing.children)) config.outgoing[child.tagName] = child.textContent; + + // We do not support unencrypted connections outside of unit tests. + if (config.incoming.socketType !== 'SSL' || + config.outgoing.socketType !== 'SSL') { + callback('no-config-info', null, { status: 'unsafe' }); + return; + } } else { - callback('unknown'); + callback('no-config-info', null, { status: 'no-outgoing' }); + return; } - callback(null, config); + callback(null, config, null); } else { - callback('unknown'); + callback('no-config-info', null, { status: 'no-incoming' }); } }; - xhr.ontimeout = xhr.onerror = function() { callback('unknown'); }; + xhr.ontimeout = xhr.onerror = function() { + // The effective result is a failure to get configuration info, but make + // sure the status conveys that a timeout occurred. + callback('no-config-info', null, { status: 'timeout' }); + }; + xhr.onerror = function() { + // The effective result is a failure to get configuration info, but make + // sure the status conveys that a timeout occurred. + callback('no-config-info', null, { status: 'error' }); + }; xhr.send(); }, @@ -864,15 +913,18 @@ Autoconfigurator.prototype = { $asproto.autodiscover(userDetails.emailAddress, userDetails.password, this.timeout, function(error, config) { if (error) { - var failureType = 'unknown'; + var failureType = 'no-config-info', + failureDetails = {}; if (error instanceof $asproto.HttpError) { if (error.status === 401) failureType = 'bad-user-or-pass'; else if (error.status === 403) failureType = 'not-authorized'; + else + failureDetails.status = error.status; } - callback(failureType); + callback(failureType, null, failureDetails); return; } @@ -884,7 +936,7 @@ Autoconfigurator.prototype = { username: config.user.email }, }; - callback(null, autoconfig); + callback(null, autoconfig, null); }); }, @@ -905,15 +957,19 @@ Autoconfigurator.prototype = { let url = 'http://autoconfig.' + domain + suffix; let self = this; - this._getXmlConfig(url, function(error, config) { - if (self._isSuccessOrFatal(error)) - return callback(error, config); + this._getXmlConfig(url, function(error, config, errorDetails) { + if (self._isSuccessOrFatal(error)) { + callback(error, config, errorDetails); + return; + } // See . let url = 'http://' + domain + '/.well-known/autoconfig' + suffix; - self._getXmlConfig(url, function(error, config) { - if (self._isSuccessOrFatal(error)) - return callback(error, config); + self._getXmlConfig(url, function(error, config, errorDetails) { + if (self._isSuccessOrFatal(error)) { + callback(error, config, errorDetails); + return; + } console.log(' Trying domain autodiscover'); self._getConfigFromAutodiscover(userDetails, callback); @@ -949,12 +1005,17 @@ Autoconfigurator.prototype = { xhr.onload = function() { if (xhr.status === 200) - callback(null, xhr.responseText.split('\n')[0]); + callback(null, xhr.responseText.split('\n')[0], null); else - callback('unknown'); + callback('no-config-info', null, { status: 'mx' + xhr.status }); }; - xhr.ontimeout = xhr.onerror = function() { callback('unknown'); }; + xhr.ontimeout = function() { + callback('no-config-info', null, { status: 'mxtimeout' }); + }; + xhr.onerror = function() { + callback('no-config-info', null, { status: 'mxerror' }); + }; xhr.send(); }, @@ -969,9 +1030,9 @@ Autoconfigurator.prototype = { */ _getConfigFromMX: function getConfigFromMX(domain, callback) { let self = this; - this._getMX(domain, function(error, mxDomain) { + this._getMX(domain, function(error, mxDomain, errorDetails) { if (error) - return callback(error); + return callback(error, null, errorDetails); // XXX: We need to normalize the domain here to get the base domain, but // that's complicated because people like putting dots in TLDs. For now, @@ -980,15 +1041,19 @@ Autoconfigurator.prototype = { console.log(' Found MX for', mxDomain); if (domain === mxDomain) - return callback('unknown'); + return callback('no-config-info', null, { status: 'mxsame' }); // If we found a different domain after MX lookup, we should look in our // local file store (mostly to support Google Apps domains) and, if that // doesn't work, the Mozilla ISPDB. console.log(' Looking in local file store'); - self._getConfigFromLocalFile(mxDomain, function(error, config) { - if (!error) - return callback(error, config); + self._getConfigFromLocalFile(mxDomain, function(error, config, + errorDetails) { + // (Local XML lookup should not have any fatal errors) + if (!error) { + callback(error, config, errorDetails); + return; + } console.log(' Looking in the Mozilla ISPDB'); self._getConfigFromDB(mxDomain, callback); @@ -1022,7 +1087,7 @@ Autoconfigurator.prototype = { .replace('%REALNAME%', userDetails.displayName); } - function onComplete(error, config) { + function onComplete(error, config, errorDetails) { console.log(error ? 'FAILURE' : 'SUCCESS'); // Fill any placeholder strings in the configuration object we retrieved. @@ -1039,7 +1104,7 @@ Autoconfigurator.prototype = { } } - callback(error, config); + callback(error, config, errorDetails); } console.log(' Looking in GELAM'); @@ -1050,19 +1115,26 @@ Autoconfigurator.prototype = { let self = this; console.log(' Looking in local file store'); - this._getConfigFromLocalFile(domain, function(error, config) { - if (self._isSuccessOrFatal(error)) - return onComplete(error, config); + this._getConfigFromLocalFile(domain, function(error, config, errorDetails) { + if (self._isSuccessOrFatal(error)) { + onComplete(error, config, errorDetails); + return; + } console.log(' Looking at domain'); - self._getConfigFromDomain(userDetails, domain, function(error, config) { - if (self._isSuccessOrFatal(error)) - return onComplete(error, config); + self._getConfigFromDomain(userDetails, domain, function(error, config, + errorDetails) { + if (self._isSuccessOrFatal(error)) { + onComplete(error, config, errorDetails); + return; + } console.log(' Looking in the Mozilla ISPDB'); - self._getConfigFromDB(domain, function(error, config) { - if (self._isSuccessOrFatal(error)) - return onComplete(error, config); + self._getConfigFromDB(domain, function(error, config, errorDetails) { + if (self._isSuccessOrFatal(error)) { + onComplete(error, config, errorDetails); + return; + } console.log(' Looking up MX'); self._getConfigFromMX(domain, onComplete); @@ -1084,9 +1156,9 @@ Autoconfigurator.prototype = { */ tryToCreateAccount: function(universe, userDetails, callback) { let self = this; - this.getConfig(userDetails, function(error, config) { + this.getConfig(userDetails, function(error, config, errorDetails) { if (error) - return callback(error); + return callback(error, null, errorDetails); var configurator = Configurators[config.type]; configurator.tryToCreateAccount(universe, userDetails, config, diff --git a/data/lib/mailapi/imap/account.js b/data/lib/mailapi/imap/account.js index e8e551ac..d7691294 100644 --- a/data/lib/mailapi/imap/account.js +++ b/data/lib/mailapi/imap/account.js @@ -13,6 +13,7 @@ define( '../mailslice', '../searchfilter', '../util', + './probe', './folder', './jobs', 'module', @@ -28,6 +29,7 @@ define( $mailslice, $searchfilter, $util, + $imapprobe, $imapfolder, $imapjobs, $module, @@ -579,61 +581,22 @@ ImapAccount.prototype = { connectCallbackTriggered = true; this._pendingConn = null; if (err) { - var errName, reachable = false, maybeRetry = true; - // We want to produce error-codes as defined in `MailApi.js` for - // tryToCreateAccount. We have also tried to make imap.js produce - // error codes of the right type already, but for various generic paths - // (like saying 'NO'), there isn't currently a good spot for that. - switch (err.type) { - // dovecot says after a delay and does not terminate the connection: - // NO [AUTHENTICATIONFAILED] Authentication failed. - // zimbra 7.2.x says after a delay and DOES terminate the connection: - // NO LOGIN failed - // * BYE Zimbra IMAP server terminating connection - // yahoo says after a delay and does not terminate the connection: - // NO [AUTHENTICATIONFAILED] Incorrect username or password. - case 'NO': - case 'no': - // XXX: Should we check if it's GMail first? - if (!err.serverResponse) - errName = 'unknown'; - else if (err.serverResponse.indexOf( - '[ALERT] Application-specific password required') !== -1) - errName = 'needs-app-pass'; - else if (err.serverResponse.indexOf( - '[ALERT] Your account is not enabled for IMAP use.') !== -1) - errName = 'imap-disabled'; - else - errName = 'bad-user-or-pass'; - reachable = true; - // go directly to the broken state; no retries - maybeRetry = false; - // tell the higher level to disable our account until we fix our - // credentials problem and ideally generate a UI prompt. - this.universe.__reportAccountProblem(this.compositeAccount, - errName); - break; - // errors we can pass through directly: - case 'server-maintenance': - errName = err.type; - reachable = true; - break; - case 'timeout': - errName = 'unresponsive-server'; - break; - default: - errName = 'unknown'; - break; - } - console.error('Connect error:', errName, 'formal:', err, 'on', + var normErr = $imapprobe.normalizeError(err); + console.error('Connect error:', normErr.name, 'formal:', err, 'on', this._connInfo.hostname, this._connInfo.port); + if (normErr.reportProblem) + this.universe.__reportAccountProblem(this.compositeAccount, + normErr.name); + + if (listener) - listener(errName); + listener(normErr.name); conn.die(); // track this failure for backoff purposes - if (maybeRetry) { - if (this._backoffEndpoint.noteConnectFailureMaybeRetry(reachable)) + if (normErr.retry) { + if (this._backoffEndpoint.noteConnectFailureMaybeRetry( + normErr.reachable)) this._makeConnectionIfPossible(); else this._killDieOnConnectFailureDemands(); diff --git a/data/lib/mailapi/imap/probe.js b/data/lib/mailapi/imap/probe.js index d3e13b47..09fd13a0 100644 --- a/data/lib/mailapi/imap/probe.js +++ b/data/lib/mailapi/imap/probe.js @@ -92,30 +92,9 @@ ImapProber.prototype = { return; console.warn('PROBE:IMAP sad', err); - switch (err.type) { - case 'NO': - case 'no': - if (!err.serverResponse) - this.error = 'unknown'; - else if (err.serverResponse.indexOf( - '[ALERT] Application-specific password required') != -1) - this.error = 'needs-app-pass'; - else if (err.serverResponse.indexOf( - '[ALERT] Your account is not enabled for IMAP use.') != -1) - this.error = 'imap-disabled'; - else - this.error = 'bad-user-or-pass'; - break; - case 'timeout': - this.error = 'timeout'; - break; - // XXX we currently don't have a string for server maintenance, so go - // with unknown. But it's also a very unlikely thing. - case 'server-maintenance': - default: - this.error = 'unknown'; - break; - } + var normErr = normalizeError(err); + this.error = normErr.name; + // we really want to make sure we clean up after this dude. try { this._conn.die(); @@ -130,6 +109,103 @@ ImapProber.prototype = { }, }; +/** + * Convert error objects from the IMAP connection to our internal error codes + * as defined in `MailApi.js` for tryToCreateAccount. This is used by the + * probe during account creation and by `ImapAccount` during general connection + * establishment. + * + * @return[@dict[ + * @key[name String] + * @key[reachable Boolean]{ + * Does this error indicate the server was reachable? This is to be + * reported to the `BackoffEndpoint`. + * } + * @key[retry Boolean]{ + * Should we retry the connection? The answer is no for persistent problems + * or transient problems that are expected to be longer lived than the scale + * of our automatic retries. + * } + * @key[reportProblem Boolean]{ + * Should we report this as a problem on the account? We should do this + * if we expect this to be a persistent problem that requires user action + * to resolve and we expect `MailUniverse.__reportAccountProblem` to + * generate a specific user notification for the error. If we're not going + * to bother the user with a popup, then we probably want to return false + * for this and leave it for the connection failure to cause the + * `BackoffEndpoint` to cause a problem to be logged via the listener + * mechanism. + * } + * ]] + */ +var normalizeError = exports.normalizeError = function normalizeError(err) { + var errName, reachable = false, retry = true, reportProblem = false; + // We want to produce error-codes as defined in `MailApi.js` for + // tryToCreateAccount. We have also tried to make imap.js produce + // error codes of the right type already, but for various generic paths + // (like saying 'NO'), there isn't currently a good spot for that. + switch (err.type) { + // dovecot says after a delay and does not terminate the connection: + // NO [AUTHENTICATIONFAILED] Authentication failed. + // zimbra 7.2.x says after a delay and DOES terminate the connection: + // NO LOGIN failed + // * BYE Zimbra IMAP server terminating connection + // yahoo says after a delay and does not terminate the connection: + // NO [AUTHENTICATIONFAILED] Incorrect username or password. + case 'NO': + case 'no': + reachable = true; + if (!err.serverResponse) { + errName = 'unknown'; + reportProblem = false; + } + else { + // All of these require user action to resolve. + reportProblem = true; + retry = false; + if (err.serverResponse.indexOf( + '[ALERT] Application-specific password required') !== -1) + errName = 'needs-app-pass'; + else if (err.serverResponse.indexOf( + '[ALERT] Your account is not enabled for IMAP use.') !== -1 || + err.serverResponse.indexOf( + '[ALERT] IMAP access is disabled for your domain.') !== -1) + errName = 'imap-disabled'; + else + errName = 'bad-user-or-pass'; + } + break; + case 'server-maintenance': + errName = err.type; + reachable = true; + // do retry + break; + // An SSL error is either something we just want to report (probe), or + // something that is currently probably best treated as a network failure. We + // could tell the user they may be experiencing a MITM attack, but that's not + // really something they can do anything about and we have protected them from + // it currently. + case 'bad-security': + errName = err.type; + reachable = true; + retry = false; + break; + case 'timeout': + errName = 'unresponsive-server'; + break; + default: + errName = 'unknown'; + break; + } + + return { + name: errName, + reachable: reachable, + retry: retry, + reportProblem: reportProblem, + }; +}; + /** * If a folder has no messages, then we need to default the timezone, and diff --git a/data/lib/mailapi/mailapi.js b/data/lib/mailapi/mailapi.js index b8fe0101..74be1715 100644 --- a/data/lib/mailapi/mailapi.js +++ b/data/lib/mailapi/mailapi.js @@ -1502,6 +1502,9 @@ MailAPI.prototype = { * @case['no-dns-entry']{ * We couldn't find the domain name in question, full stop. * } + * @case['no-config-info']{ + * We were unable to locate configuration information for the domain. + * } * @case['unresponsive-server']{ * Requests to the server timed out. AKA we sent packets into a black * hole. @@ -1537,6 +1540,11 @@ MailAPI.prototype = { * The username and password are correct, but the user isn't allowed to * access the mail server. * } + * @case['server-problem']{ + * We were able to talk to the "server" named in the details object, but + * we encountered some type of problem. The details object will also + * include a "status" value. + * } * @case['server-maintenance']{ * The server appears to be undergoing maintenance, at least for this * account. We infer this if the server is telling us that login is @@ -1564,6 +1572,17 @@ MailAPI.prototype = { * @param[callback @func[ * @args[ * @param[err AccountCreationError] + * @param[errDetails @dict[ + * @key[server #:optional String]{ + * The server we had trouble talking to. + * } + * @key[status #:optional @oneof[Number String]]{ + * The HTTP status code number, or "timeout", or something otherwise + * providing detailed additional information about the error. This + * is usually too technical to be presented to the user, but is + * worth encoding with the error name proper if possible. + * } + * ]] * ] * ] * ] @@ -1594,7 +1613,7 @@ MailAPI.prototype = { } delete this._pendingRequests[msg.handle]; - req.callback.call(null, msg.error); + req.callback.call(null, msg.error, msg.errorDetails); }, _clearAccountProblems: function ma__clearAccountProblems(account) { diff --git a/data/lib/mailapi/mailbridge.js b/data/lib/mailapi/mailbridge.js index 6f9015a5..ff7ba0b3 100644 --- a/data/lib/mailapi/mailbridge.js +++ b/data/lib/mailapi/mailbridge.js @@ -158,11 +158,12 @@ MailBridge.prototype = { _cmd_tryToCreateAccount: function mb__cmd_tryToCreateAccount(msg) { var self = this; this.universe.tryToCreateAccount(msg.details, msg.domainInfo, - function(error, account) { + function(error, account, errorDetails) { self.__sendMessage({ type: 'tryToCreateAccountResults', handle: msg.handle, error: error, + errorDetails: errorDetails, }); }); }, diff --git a/data/lib/mailapi/mailuniverse.js b/data/lib/mailapi/mailuniverse.js index 0725b75f..8898133e 100644 --- a/data/lib/mailapi/mailuniverse.js +++ b/data/lib/mailapi/mailuniverse.js @@ -763,7 +763,8 @@ MailUniverse.prototype = { * Self-reporting by an account that it is experiencing difficulties. * * We mutate its state for it, and generate a notification if this is a new - * problem. + * problem. For problems that require user action, we additionally generate + * a bad login notification. */ __reportAccountProblem: function(account, problem) { // nothing to do if the problem is already known diff --git a/data/lib/mailapi/smtp/probe.js b/data/lib/mailapi/smtp/probe.js index e8be6957..597918b6 100644 --- a/data/lib/mailapi/smtp/probe.js +++ b/data/lib/mailapi/smtp/probe.js @@ -1,5 +1,5 @@ /** - * + * SMTP probe logic. **/ define( @@ -12,6 +12,31 @@ define( exports ) { +var setTimeoutFunc = window.setTimeout.bind(window), + clearTimeoutFunc = window.clearTimeout.bind(window); + +exports.TEST_useTimeoutFuncs = function(setFunc, clearFunc) { + setTimeoutFunc = setFunc; + clearTimeoutFunc = clearFunc; +}; + +/** + * How many milliseconds should we wait before giving up on the connection? + * + * I have a whole essay on the rationale for this in the IMAP prober. Us, we + * just want to use the same value as the IMAP prober. This is a candidate for + * centralization. + */ +exports.CONNECT_TIMEOUT_MS = 30000; + +/** + * Validate that we find an SMTP server using the connection info and that it + * seems to like our credentials. + * + * Because the SMTP client has no connection timeout support, use our own timer + * to decide when to give up on the SMTP connection. We use the timer for the + * whole process, including even after the connection is established. + */ function SmtpProber(credentials, connInfo) { console.log("PROBE:SMTP attempting to connect to", connInfo.hostname); this._conn = $simplesmtp( @@ -22,33 +47,53 @@ function SmtpProber(credentials, connInfo) { auth: { user: credentials.username, pass: credentials.password }, debug: false, }); - this._conn.on('idle', this.onIdle.bind(this)); - this._conn.on('error', this.onBadness.bind(this)); - this._conn.on('end', this.onBadness.bind(this)); + // onIdle happens after successful login, and so is what our probing uses. + this._conn.on('idle', this.onResult.bind(this, null)); + this._conn.on('error', this.onResult.bind(this)); + this._conn.on('end', this.onResult.bind(this, 'unknown')); + + this.timeoutId = setTimeoutFunc( + this.onResult.bind(this, 'unresponsive-server'), + exports.CONNECT_TIMEOUT_MS); this.onresult = null; + this.error = null; } exports.SmtpProber = SmtpProber; SmtpProber.prototype = { - /** - * onIdle happens after successful login, and so is what our probing uses. - */ - onIdle: function() { - console.log('onIdle!'); - if (this.onresult) { - console.log('PROBE:SMTP happy'); - this.onresult(true); - this.onresult = null; + onResult: function(err) { + if (!this.onresult) + return; + if (err && typeof(err) === 'object') { + // detect an nsISSLStatus instance by an unusual property. + if ('isNotValidAtThisTime' in err) { + err = 'bad-security'; + } + else { + switch (err.name) { + case 'AuthError': + err = 'bad-user-or-pass'; + break; + case 'UnknownAuthError': + default: + err = 'server-problem'; + break; + } + } } - this._conn.close(); - }, - onBadness: function(err) { - if (this.onresult) { + this.error = err; + if (err) console.warn('PROBE:SMTP sad. error: |' + err + '|'); - this.onresult(false); - this.onresult = null; - } + else + console.log('PROBE:SMTP happy'); + + clearTimeoutFunc(this.timeoutId); + + this.onresult(this.error); + this.onresult = null; + + this._conn.close(); }, }; diff --git a/node-deps/simplesmtp b/node-deps/simplesmtp index 73ee2aa4..938fcb8a 160000 --- a/node-deps/simplesmtp +++ b/node-deps/simplesmtp @@ -1 +1 @@ -Subproject commit 73ee2aa4f84e80ab704dc4d41c10fa4c96843a4f +Subproject commit 938fcb8a1d26a7de2f147e6d6f57b46ab26f973b diff --git a/test/unit/resources/fake_xhr.js b/test/unit/resources/fake_xhr.js new file mode 100644 index 00000000..0fa06b42 --- /dev/null +++ b/test/unit/resources/fake_xhr.js @@ -0,0 +1,44 @@ +var gFakeXHRListener = null; + +function FakeXHR() { + this._args = { + method: null, + url: null, + async: null, + timeout: null + }; + + this.onload = null; + this.onerror = null; + this.ontimeout = null; + + this.timeout = null; + + this.upload = { + onprogress: null, + onload: null, + }; + + this.status = null; + this.statusText = 'Meh'; +} +FakeXHR.prototype = { + open: function(method, url, async) { + this._args.method = method; + this._args.url = url; + this._args.async = async; + }, + + send: function() { + this._args.timeout = this.timeout; + if (gFakeXHRListener) { + gFakeXHRListener(this, this._args); + } + }, + + setRequestHeader: function() { + // ActiveSync uses this to set various headers that we don't care about. + }, +}; + +window.XMLHttpRequest = FakeXHR; diff --git a/test/unit/resources/fault_injecting_socket.js b/test/unit/resources/fault_injecting_socket.js index 9b24c523..c960cbd1 100644 --- a/test/unit/resources/fault_injecting_socket.js +++ b/test/unit/resources/fault_injecting_socket.js @@ -35,6 +35,21 @@ function FawltySocket(host, port, options, cmdDict) { // or at least allow changing the defaults right now. return; + case 'bad-security': + // Fake an nsISSLStatus object, which is what bad crypto engenders + var fakeSslError = { + serverCert: {}, + cipherName: 'zob', + keyLength: 2048, + secretKeyLength: 2048, + isDomainMismatch: false, + isNotValidAtThisTime: false, + isUntrusted: false, + isExtendedValidation: false + }; + this._queueEvent('onerror', fakeSslError); + return; + case 'fake': // We are only going to send fake data, so don't bother establishing // a connection. diff --git a/test/unit/resources/window_shims.js b/test/unit/resources/window_shims.js index 0d54656d..48be7306 100644 --- a/test/unit/resources/window_shims.js +++ b/test/unit/resources/window_shims.js @@ -42,6 +42,7 @@ var __blobLogFunc = function() { var __deviceStorageLogFunc = function() { }; + var _window_mixin = { // - indexed db indexedDB: indexedDB, diff --git a/test/unit/test_autoconfig.js b/test/unit/test_autoconfig.js new file mode 100644 index 00000000..b03e3bb3 --- /dev/null +++ b/test/unit/test_autoconfig.js @@ -0,0 +1,338 @@ +/** + * Fairly simple test cases covering the various successful autoconfig + * permutations and a few failures. + */ + +load('resources/loggest_test_framework.js'); +load('resources/fake_xhr.js'); + +var TD = $tc.defineTestsFor( + { id: 'test_autoconfig' }, null, [$th_imap.TESTHELPER], ['app']); + +const goodImapXML = + '\n' + + '' + + '' + + 'imap.xampl.tld' + + '993' + + 'SSL' + + '%EMAILADDRESS%' + + 'password-cleartext' + + '' + + '' + + 'smtp.xampl.tld' + + '465' + + 'SSL' + + '%EMAILADDRESS%' + + 'password-cleartext' + + '' + + ''; + +const goodImapConfig = { + type: 'imap+smtp', + incoming: { + hostname: 'imap.xampl.tld', + port: '993', + socketType: 'SSL', + username: 'user@xampl.tld', + authentication: 'password-cleartext', + }, + outgoing: { + hostname: 'smtp.xampl.tld', + port: '465', + socketType: 'SSL', + username: 'user@xampl.tld', + authentication: 'password-cleartext', + }, +}; + +const unsafeImapXML = goodImapXML.replace('SSL', 'plain', 'g'); + +const goodActivesyncXML = + '\n' + + '' + + '' + + 'https://m.xampl.tld/' + + '%EMAILADDRESS%' + + '' + + ''; + +const goodActivesyncConfig = { + type: 'activesync', + incoming: { + server: 'https://m.xampl.tld/', + username: 'user@xampl.tld', + }, + outgoing: { + }, +}; + +const goodActivesyncAutodiscoverConfig = { + type: 'activesync', + displayName: 'DISPLAYNAME', + incoming: { + server: 'https://m.xampl.tld/', + username: 'EMAILADDRESS', + }, +}; + + +const goodActivesyncAutodiscoverXML = + '\n' + + '' + + '' + + '' + + 'DISPLAYNAME' + + 'EMAILADDRESS' + + '' + + 'CULTURE' + + '' + + 'MobileSync' + + 'https://m.xampl.tld/' + + 'SERVERNAME' + + 'SERVERDATA' + + '' + + '' + + ''; + +const gibberishXML = 'I NOT GOOD XML'; + +function expectXHRs(lazy, xhrs) { + var iServiced = 0; + gFakeXHRListener = function(req, args) { + lazy.namedValue('xhr', args); + if (iServiced >= xhrs.length) + return; + var def = xhrs[iServiced++]; + window.setZeroTimeout(function() { + if (def.data) { + req.status = 200; + req.responseText = def.data; + req.onload(); + } + else if (typeof(def.status) === 'number') { + req.status = def.status; + req.onload(); + } + else if (typeof(def.status) === 'string') { + if (def.status === 'timeout') + req.ontimeout(); + else if (def.status === 'error') + req.onerror(); + } + }); + }; + for (var i = 0; i < xhrs.length; i++) { + var def = xhrs[i]; + + lazy.expect_namedValue( + 'xhr', + { + method: def.method || 'GET', + url: def.url, + async: true, + timeout: 30000 + }); + } +} + +function cannedTest(T, RT, xhrs, results) { + var lazyConsole = T.lazyLogger('console'); + gConsoleLogFunc = function(msg) { + lazyConsole.value(msg); + }; + var eCheck = T.lazyLogger('check'); + T.action(eCheck, 'autoconfig', function() { + expectXHRs(eCheck, xhrs); + var configurator = new $_accountcommon.Autoconfigurator(); + var userDetails = { + emailAddress: 'user@xampl.tld', + password: 'PASSWORD', + }; + eCheck.expect_namedValue('error', results.error); + eCheck.expect_namedValue('config', results.config); + eCheck.expect_namedValue('errorDetails', results.errorDetails); + configurator.getConfig(userDetails, function(error, config, errorDetails) { + eCheck.namedValue('error', error); + eCheck.namedValue('config', config); + eCheck.namedValue('errorDetails', errorDetails); + }); + }); +}; + +/** + * local XML config file tells us activesync. + */ +TD.commonCase('successful local activesync', function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + data: goodActivesyncXML }, + ], + { + error: null, + config: goodActivesyncConfig, + errorDetails: null, + }); +}); + +/** + * local XML config file tells us IMAP. + */ +TD.commonCase('successful local IMAP', function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + data: goodImapXML }, + ], + { + error: null, + config: goodImapConfig, + errorDetails: null, + }); +}); + +/** + * The domain self-hosts an XML config at autoconfig.domain. + */ +TD.commonCase('successful IMAP autoconfig.domain', function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + status: 404 }, + { url: 'http://autoconfig.xampl.tld/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + data: goodImapXML }, + ], + { + error: null, + config: goodImapConfig, + errorDetails: null, + }); +}); + +/** + * The domain self-hosts an XML config at domain/.well-known/ + */ +TD.commonCase('successful IMAP domain/.well-known/', function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + status: 404 }, + { url: 'http://autoconfig.xampl.tld/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'http://xampl.tld/.well-known/autoconfig/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + data: goodImapXML }, + ], + { + error: null, + config: goodImapConfig, + errorDetails: null, + }); +}); + +/** + * ActiveSync autodiscovery worked for the domain via /autodiscover/. + */ +TD.commonCase('successful activesync domain/autodiscover/ autodiscovery', + function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + status: 404 }, + { url: 'http://autoconfig.xampl.tld/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'http://xampl.tld/.well-known/autoconfig/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'https://xampl.tld/autodiscover/autodiscover.xml', + method: 'POST', data: goodActivesyncAutodiscoverXML }, + ], + { + error: null, + config: goodActivesyncAutodiscoverConfig, + errorDetails: null, + }); +}); + +/** + * ActiveSync autodiscovery worked for the domain via autodiscover.domain. + */ +TD.commonCase('successful activesync autodiscover.domain autodiscovery', + function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + status: 404 }, + { url: 'http://autoconfig.xampl.tld/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'http://xampl.tld/.well-known/autoconfig/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + // if we return a 404, we won't try the second autodiscover location, so + // return some XML. + { url: 'https://xampl.tld/autodiscover/autodiscover.xml', + method: 'POST', data: gibberishXML }, + { url: 'https://autodiscover.xampl.tld/autodiscover/autodiscover.xml', + method: 'POST', data: goodActivesyncAutodiscoverXML }, + ], + { + error: null, + config: goodActivesyncAutodiscoverConfig, + errorDetails: null, + }); +}); + + +/** + * ISPDB lookup found the domain and told us IMAP. + */ +TD.commonCase('successful ISPDB IMAP', function(T, RT) { + cannedTest(T, RT, + [ + { url: '/autoconfig/xampl.tld', + status: 404 }, + { url: 'http://autoconfig.xampl.tld/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'http://xampl.tld/.well-known/autoconfig/mail/config-v1.1.xml?emailaddress=user%40xampl.tld', + status: 404 }, + { url: 'https://xampl.tld/autodiscover/autodiscover.xml', + method: 'POST', status: 404 }, + { url: 'https://live.mozillamessaging.com/autoconfig/v1.1/xampl.tld', + data: goodImapXML }, + ], + { + error: null, + config: goodImapConfig, + errorDetails: null, + }); +}); + +/** + * local XML config file tells us activesync after checking MX. + */ +TD.commonCase('successful MX local activesync', function(T, RT) { +}); + +/** + * local XML config file tells us IMAP after checking MX. + */ +TD.commonCase('successful MX local IMAP', function(T, RT) { +}); + +/** + * ISPDB lookup found the MX-resolved domain + */ +TD.commonCase('successful MX ISPDB IMAP', function(T, RT) { +}); + +TD.commonCase('everything fails, get no-config-info', function(T, RT) { +}); + +TD.commonCase('non-SSL ISPDB turns into no-config-info', function(T, RT) { +}); + + +function run_test() { + runMyTests(5); +} diff --git a/test/unit/test_imap_prober.js b/test/unit/test_imap_prober.js index 60c65828..42365d20 100644 --- a/test/unit/test_imap_prober.js +++ b/test/unit/test_imap_prober.js @@ -72,11 +72,34 @@ TD.commonCase('timeout failure', function(T, RT) { }); T.action(eCheck, 'trigger timeout', function() { eCheck.expect_event('imap:clearTimeout'); - eCheck.expect_namedValue('probe result', 'timeout'); + eCheck.expect_namedValue('probe result', 'unresponsive-server'); fireTimeout(0); }); }); +TD.commonCase('SSL failure', function(T, RT) { + thunkConsole(T); + var eCheck = T.lazyLogger('check'), + prober = null; + + var fireTimeout = thunkImapTimeouts(eCheck); + var cci = makeCredsAndConnInfo(); + + T.action(eCheck, 'create prober, see SSL error', function() { + FawltySocketFactory.precommand(HOST, PORT, 'bad-security'); + eCheck.expect_namedValue('imap:setTimeout', $_probe.CONNECT_TIMEOUT_MS); + prober = new $_probe.ImapProber(cci.credentials, cci.connInfo, + eCheck._logger); + prober.onresult = function(err) { + eCheck.namedValue('probe result', err); + }; + eCheck.expect_event('imap:clearTimeout'); + eCheck.expect_event('imap:clearTimeout'); + eCheck.expect_namedValue('probe result', 'bad-security'); + }); +}); + + const OPEN_RESPONSE = '* OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE STARTTLS AUTH=PLAIN] Dovecot ready.\r\n'; const CAPABILITY_RESPONSE = [ @@ -128,13 +151,20 @@ TD.commonCase('gmail 2-factor auth error', function(T, RT) { }); }); -TD.commonCase('gmail IMAP disabled error', function(T, RT) { +TD.commonCase('gmail IMAP user disabled error', function(T, RT) { cannedLoginTest(T, RT, { loginErrorString: 'NO [ALERT] Your account is not enabled for IMAP use.', expectResult: 'imap-disabled', }); }); +TD.commonCase('gmail IMAP domain disabled error', function(T, RT) { + cannedLoginTest(T, RT, { + loginErrorString: 'NO [ALERT] IMAP access is disabled for your domain.', + expectResult: 'imap-disabled', + }); +}); + function run_test() { runMyTests(15); } diff --git a/test/unit/test_smtp_prober.js b/test/unit/test_smtp_prober.js new file mode 100644 index 00000000..b94568a0 --- /dev/null +++ b/test/unit/test_smtp_prober.js @@ -0,0 +1,155 @@ +/** + * Test the SMTP prober in isolation. + * + * Right now we cover: + * - Timeout trying to talk to the server. + * - SSL error trying to talk to the server. + * - Auth failure. + */ + +load('resources/loggest_test_framework.js'); +// Use the faulty socket implementation. +load('resources/fault_injecting_socket.js'); + +var $_smtpprobe = require('mailapi/smtp/probe'); + +var TD = $tc.defineTestsFor( + { id: 'test_smtp_prober' }, null, [$th_imap.TESTHELPER], ['app']); + +function thunkConsole(T) { + var lazyConsole = T.lazyLogger('console'); + + gConsoleLogFunc = function(msg) { + lazyConsole.value(msg); + }; +} + +function thunkSmtpTimeouts(lazyLogger) { + var timeouts = []; + $_smtpprobe.TEST_useTimeoutFuncs( + function thunkedSetTimeout(func, delay) { + lazyLogger.namedValue('smtp:setTimeout', delay); + return timeouts.push(func); + }, + function thunkedClearTimeout() { + lazyLogger.event('smtp:clearTimeout'); + }); + return function fireThunkedTimeout(index) { + timeouts[index](); + timeouts[index] = null; + }; +} + +// Currently all the tests in here are completely fake; we never connect. +const HOST = 'localhost', PORT = 465; + +function makeCredsAndConnInfo() { + return { + credentials: { + username: 'USERNAME', + password: 'PASSWORD', + }, + connInfo: { + hostname: HOST, + port: PORT, + crypto: true, + }, + }; +} + +TD.commonCase('timeout failure', function(T, RT) { + thunkConsole(T); + var eCheck = T.lazyLogger('check'), + prober = null; + + var fireTimeout = thunkSmtpTimeouts(eCheck); + var cci = makeCredsAndConnInfo(); + + T.action(eCheck, 'create prober', function() { + FawltySocketFactory.precommand(HOST, PORT, 'unresponsive-server'); + eCheck.expect_namedValue('smtp:setTimeout', $_smtpprobe.CONNECT_TIMEOUT_MS); + prober = new $_smtpprobe.SmtpProber(cci.credentials, cci.connInfo); + prober.onresult = function(err) { + eCheck.namedValue('probe result', err); + }; + }); + T.action(eCheck, 'trigger timeout', function() { + eCheck.expect_event('smtp:clearTimeout'); + eCheck.expect_namedValue('probe result', 'unresponsive-server'); + fireTimeout(0); + }); +}); + +TD.commonCase('SSL failure', function(T, RT) { + thunkConsole(T); + var eCheck = T.lazyLogger('check'), + prober = null; + + var fireTimeout = thunkSmtpTimeouts(eCheck); + var cci = makeCredsAndConnInfo(); + + T.action(eCheck, 'create prober, see SSL error', function() { + FawltySocketFactory.precommand(HOST, PORT, 'bad-security'); + eCheck.expect_namedValue('smtp:setTimeout', $_smtpprobe.CONNECT_TIMEOUT_MS); + prober = new $_smtpprobe.SmtpProber(cci.credentials, cci.connInfo); + prober.onresult = function(err) { + eCheck.namedValue('probe result', err); + }; + eCheck.expect_event('smtp:clearTimeout'); + eCheck.expect_namedValue('probe result', 'bad-security'); + }); +}); + +const SMTP_GREETING = '220 localhsot ESMTP Fake'; +const SMTP_EHLO_RESPONSE = '250 AUTH PLAIN'; + + +function cannedLoginTest(T, RT, opts) { + thunkConsole(T); + var eCheck = T.lazyLogger('check'); + + var fireTimeout = thunkSmtpTimeouts(eCheck), + cci = makeCredsAndConnInfo(), + prober; + + T.action('connect, get error, return', eCheck, function() { + eCheck.expect_namedValue('smtp:setTimeout', $_smtpprobe.CONNECT_TIMEOUT_MS); + eCheck.expect_event('smtp:clearTimeout'); + eCheck.expect_namedValue('probe result', opts.expectResult); + FawltySocketFactory.precommand( + HOST, PORT, + { + cmd: 'fake', + data: SMTP_GREETING, + }, + [ + opts.ehloResponse || SMTP_EHLO_RESPONSE, + opts.loginErrorString + ]); + prober = new $_smtpprobe.SmtpProber(cci.credentials, cci.connInfo); + prober.onresult = function(err) { + eCheck.namedValue('probe result', err); + }; + }); +}; + +TD.commonCase('bad username or password', function(T, RT) { + cannedLoginTest(T, RT, { + loginErrorString: '535 Authentication DENIED', + expectResult: 'bad-user-or-pass', + }); +}); + +TD.commonCase('angry server', function(T, RT) { + cannedLoginTest(T, RT, { + ehloResponse: '500 go away!', + // it will then say HELO, which we also hate, because we are angry. + loginErrorString: '500 I said go away!', + expectResult: 'server-problem', + }); +}); + + +function run_test() { + runMyTests(5); +} diff --git a/test/unit/xpcshell.ini b/test/unit/xpcshell.ini index c6eddd81..fe1ceafe 100644 --- a/test/unit/xpcshell.ini +++ b/test/unit/xpcshell.ini @@ -28,6 +28,8 @@ run-if = account == 'imap' run-if = account == 'imap' [test_imap_prober.js] run-if = account == 'imap' +[test_smtp_prober.js] +run-if = account == 'imap' [test_activesync_general.js] @@ -43,6 +45,7 @@ run-if = torture == 'true' # works on everything: +[test_autoconfig.js] [test_account_logic.js] [test_folder_storage.js] [test_linkify.js]