From f7a24c87f6eaaa2a02988fbfb9c8da5a03691e66 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 08:46:47 -0500 Subject: [PATCH 1/8] Bug 1176399 - Multiple requests for master password when GMail OAuth2 is enabled. --- mailnews/base/public/nsIMsgAsyncPrompter.idl | 26 +++++++++--- mailnews/base/src/msgAsyncPrompter.js | 33 +++++++++++---- mailnews/base/src/msgOAuth2Module.js | 42 +++++++++++++++----- mailnews/imap/src/nsImapProtocol.cpp | 7 ++++ mailnews/local/src/nsPop3Protocol.cpp | 7 ++++ mailnews/news/src/nsNNTPProtocol.cpp | 7 ++++ 6 files changed, 101 insertions(+), 21 deletions(-) diff --git a/mailnews/base/public/nsIMsgAsyncPrompter.idl b/mailnews/base/public/nsIMsgAsyncPrompter.idl index 5a59c4f393..4e1f81d121 100644 --- a/mailnews/base/public/nsIMsgAsyncPrompter.idl +++ b/mailnews/base/public/nsIMsgAsyncPrompter.idl @@ -35,20 +35,36 @@ interface nsIMsgAsyncPrompter : nsISupports { in nsIMsgAsyncPromptListener aCaller); }; +[scriptable, function, uuid(acca94c9-378e-46e3-9a91-6655bf9c91a3)] +interface nsIMsgAsyncPromptCallback : nsISupports { + /** + * Called when an auth result is available. Can be passed as a function. + * + * @param aResult True if there is auth information available following the + * prompt, false otherwise. + */ + void onAuthResult(in boolean aResult); +}; + /** * This is used in combination with nsIMsgAsyncPrompter. */ [scriptable, uuid(fb5307a3-39d0-462e-92c8-c5c288a2612f)] interface nsIMsgAsyncPromptListener : nsISupports { /** - * Called when the listener should do its prompt. The listener - * should not return until the prompt is complete. - * - * @return True if there is auth information available following the prompt, - * false otherwise. + * This method has been deprecated, please use onPromptStartAsync instead. */ boolean onPromptStart(); + /** + * Called when the listener should do its prompt. This can happen + * synchronously or asynchronously, but in any case when done the callback + * method should be called. + * + * @param aCallback The callback to execute when auth prompt has completed. + */ + void onPromptStartAsync(in nsIMsgAsyncPromptCallback aCallback); + /** * Called in the case that the queued prompt was combined with another and * there is now authentication information available. diff --git a/mailnews/base/src/msgAsyncPrompter.js b/mailnews/base/src/msgAsyncPrompter.js index 58b5288e97..ae114683ae 100644 --- a/mailnews/base/src/msgAsyncPrompter.js +++ b/mailnews/base/src/msgAsyncPrompter.js @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +Components.utils.import("resource://gre/modules/Deprecated.jsm"); Components.utils.import("resource://gre/modules/Services.jsm"); Components.utils.import("resource://gre/modules/Task.jsm"); Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); @@ -19,28 +20,46 @@ runnablePrompter.prototype = { _asyncPrompter: null, _hashKey: null, + _promiseAuthPrompt: function(listener) { + return new Promise((resolve, reject) => { + try { + listener.onPromptStartAsync({ onAuthResult: resolve }); + } catch (e) { + if (e.result == Components.results.NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED) { + // Fall back to onPromptStart, for add-ons compat + Deprecated.warning("onPromptStart has been replaced by onPromptStartAsync", + "https://bugzilla.mozilla.org/show_bug.cgi?id=1176399"); + let ok = listener.onPromptStart(); + resolve(ok); + } else { + reject(e); + } + } + }); + }, + run: Task.async(function *() { yield Services.logins.initializationPromise; this._asyncPrompter._log.debug("Running prompt for " + this._hashKey); let prompter = this._asyncPrompter._pendingPrompts[this._hashKey]; let ok = false; try { - ok = prompter.first.onPromptStart(); - } - catch (ex) { + ok = yield this._promiseAuthPrompt(prompter.first); + } catch (ex) { Components.utils.reportError("runnablePrompter:run: " + ex + "\n"); + prompter.first.onPromptCanceled(); } delete this._asyncPrompter._pendingPrompts[this._hashKey]; for (var consumer of prompter.consumers) { try { - if (ok) + if (ok) { consumer.onPromptAuthAvailable(); - else + } else { consumer.onPromptCanceled(); - } - catch (ex) { + } + } catch (ex) { // Log the error for extension devs and others to pick up. Components.utils.reportError("runnablePrompter:run: consumer.onPrompt* reported an exception: " + ex + "\n"); } diff --git a/mailnews/base/src/msgOAuth2Module.js b/mailnews/base/src/msgOAuth2Module.js index 407ab05193..22d5dc572c 100644 --- a/mailnews/base/src/msgOAuth2Module.js +++ b/mailnews/base/src/msgOAuth2Module.js @@ -126,19 +126,43 @@ OAuth2Module.prototype = { } } - // Otherwise, we need a new login, so create one and fill it in. - let login = Cc["@mozilla.org/login-manager/loginInfo;1"] - .createInstance(Ci.nsILoginInfo); - login.init(this._loginUrl, null, this._scope, this._username, token, - '', ''); - loginMgr.addLogin(login); + // Unless the token is null, we need to create and fill in a new login + if (token) { + let login = Cc["@mozilla.org/login-manager/loginInfo;1"] + .createInstance(Ci.nsILoginInfo); + login.init(this._loginUrl, null, this._scope, this._username, token, + '', ''); + loginMgr.addLogin(login); + } return token; }, connect(aWithUI, aListener) { - this._oauth.connect(() => aListener.onSuccess(this._oauth.accessToken), - x => aListener.onFailure(x), - aWithUI, false); + let oauth = this._oauth; + let promptlistener = { + onPromptStartAsync: function(callback) { + oauth.connect(() => { + this.onPromptAuthAvailable(); + callback.onAuthResult(true); + }, (err) => { + this.onPromptCanceled(); + callback.onAuthResult(false); + }, aWithUI, false); + }, + + onPromptAuthAvailable: function() { + aListener.onSuccess(oauth.accessToken); + }, + onPromptCanceled: function() { + aListener.onFailure(Components.results.NS_ERROR_ABORT); + }, + onPromptStart: function() {} + }; + + let asyncprompter = Components.classes["@mozilla.org/messenger/msgAsyncPrompter;1"] + .getService(Components.interfaces.nsIMsgAsyncPrompter); + let promptkey = this._loginUrl + "/" + this._username; + asyncprompter.queueAsyncAuthPrompt(promptkey, false, promptlistener); }, buildXOAuth2String() { diff --git a/mailnews/imap/src/nsImapProtocol.cpp b/mailnews/imap/src/nsImapProtocol.cpp index 4cfa9dab29..c8e3ceb67f 100644 --- a/mailnews/imap/src/nsImapProtocol.cpp +++ b/mailnews/imap/src/nsImapProtocol.cpp @@ -8513,6 +8513,13 @@ nsresult nsImapProtocol::GetPassword(nsCString &password, return rv; } +NS_IMETHODIMP nsImapProtocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback) +{ + bool result = false; + OnPromptStart(&result); + return aCallback->OnAuthResult(result); +} + // This is called from the UI thread. NS_IMETHODIMP nsImapProtocol::OnPromptStart(bool *aResult) diff --git a/mailnews/local/src/nsPop3Protocol.cpp b/mailnews/local/src/nsPop3Protocol.cpp index 5d9d9145a0..de129a494e 100644 --- a/mailnews/local/src/nsPop3Protocol.cpp +++ b/mailnews/local/src/nsPop3Protocol.cpp @@ -740,6 +740,13 @@ nsresult nsPop3Protocol::StartGetAsyncPassword(Pop3StatesEnum aNextState) return rv; } +NS_IMETHODIMP nsPop3Protocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback) +{ + bool result = false; + OnPromptStart(&result); + return aCallback->OnAuthResult(result); +} + NS_IMETHODIMP nsPop3Protocol::OnPromptStart(bool *aResult) { MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("OnPromptStart()"))); diff --git a/mailnews/news/src/nsNNTPProtocol.cpp b/mailnews/news/src/nsNNTPProtocol.cpp index 8ce367faac..035dff6e6f 100644 --- a/mailnews/news/src/nsNNTPProtocol.cpp +++ b/mailnews/news/src/nsNNTPProtocol.cpp @@ -2472,6 +2472,13 @@ nsresult nsNNTPProtocol::PasswordResponse() return NS_ERROR_FAILURE; } +NS_IMETHODIMP nsNNTPProtocol::OnPromptStartAsync(nsIMsgAsyncPromptCallback *aCallback) +{ + bool result = false; + OnPromptStart(&result); + return aCallback->OnAuthResult(result); +} + NS_IMETHODIMP nsNNTPProtocol::OnPromptStart(bool *authAvailable) { NS_ENSURE_ARG_POINTER(authAvailable); From 671b973fe510a3d1ad5c4c2a9370c064e0a1a449 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 09:24:51 -0500 Subject: [PATCH 2/8] Bug 1453643 - Enable proper retry on oauth2 authenication failure. This prevents mail applications from attempting to use an unauthenticated connection to mailbox(s) and avoid unexpected deletion of local mbox files and subsequent re-download of mailbox content over imap. --- mailnews/imap/src/nsImapProtocol.cpp | 59 ++++++++++++++++------------ mailnews/imap/src/nsImapProtocol.h | 1 + 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/mailnews/imap/src/nsImapProtocol.cpp b/mailnews/imap/src/nsImapProtocol.cpp index c8e3ceb67f..940d87cbd1 100644 --- a/mailnews/imap/src/nsImapProtocol.cpp +++ b/mailnews/imap/src/nsImapProtocol.cpp @@ -5714,6 +5714,36 @@ void nsImapProtocol::ResetAuthMethods() m_failedAuthMethods = 0; } +nsresult nsImapProtocol::SendDataParseIMAPandCheckForNewMail(const char *aData, const char *aCommand) +{ + nsresult rv; + bool isResend = false; + while (true) + { + // Send authentication string (true: suppress logging the string). + rv = SendData(aData, true); + if (NS_FAILED(rv)) + break; + ParseIMAPandCheckForNewMail(aCommand); + if (!GetServerStateParser().WaitingForMoreClientInput()) + break; + + // The server is asking for the authentication string again. So we send + // the same string again although we know that it might be rejected again. + // We do that to get a firm authentication failure instead of a resend + // request. That keeps things in order before failing authentication and + // trying another method if capable. + if (isResend) + { + rv = NS_ERROR_FAILURE; + break; + } + isResend = true; + } + + return rv; +} + nsresult nsImapProtocol::AuthLogin(const char *userName, const nsCString &password, eIMAPCapabilityFlag flag) { ProgressEventFunctionUsingName("imapStatusSendingAuthLogin"); @@ -5880,29 +5910,7 @@ nsresult nsImapProtocol::AuthLogin(const char *userName, const nsCString &passwo PR_snprintf(m_dataOutputBuf, OUTPUT_BUFFER_SIZE, "%s" CRLF, base64Str); PR_Free(base64Str); - bool isResend = false; - while (true) - { - // Send authentication string (true: suppress logging the string). - rv = SendData(m_dataOutputBuf, true); - if (NS_FAILED(rv)) - break; - ParseIMAPandCheckForNewMail(currentCommand); - if (!GetServerStateParser().WaitingForMoreClientInput()) - break; - - // Server is asking for authentication string again. So we send the - // same string again although we already know that it will be - // rejected again. We do that to get a firm authentication failure - // instead of a resend request. That keeps things in order before - // failing "authenticate PLAIN" and trying another method if capable. - if (isResend) - { - rv = NS_ERROR_FAILURE; - break; - } - isResend = true; - } + rv = SendDataParseIMAPandCheckForNewMail(m_dataOutputBuf, currentCommand); } // if the last command succeeded } // if auth plain capability else if (flag & kHasAuthLoginCapability) @@ -5953,9 +5961,8 @@ nsresult nsImapProtocol::AuthLogin(const char *userName, const nsCString &passwo EscapeUserNamePasswordString(password.get(), &correctedPassword); command.Append(correctedPassword); command.Append("\"" CRLF); - rv = SendData(command.get(), true /* suppress logging */); - NS_ENSURE_SUCCESS(rv, rv); - ParseIMAPandCheckForNewMail(); + + rv = SendDataParseIMAPandCheckForNewMail(command.get(), nullptr); } #ifdef MOZ_MAILNEWS_OAUTH2 else if (flag & kHasXOAuth2Capability) diff --git a/mailnews/imap/src/nsImapProtocol.h b/mailnews/imap/src/nsImapProtocol.h index ba2594c894..8cee4f4fb6 100644 --- a/mailnews/imap/src/nsImapProtocol.h +++ b/mailnews/imap/src/nsImapProtocol.h @@ -485,6 +485,7 @@ private: void Namespace(); void InsecureLogin(const char *userName, const nsCString &password); nsresult AuthLogin(const char *userName, const nsCString &password, eIMAPCapabilityFlag flag); + nsresult SendDataParseIMAPandCheckForNewMail(const char *data, const char *command); void ProcessAuthenticatedStateURL(); void ProcessAfterAuthenticated(); void ProcessSelectedStateURL(); From 9839bd3335d3b02ae9a50e710e66a604c8250970 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 09:33:56 -0500 Subject: [PATCH 3/8] Bug 1597933 - clean up OAuth2 code: remove responseType which is always code. Response type token is part of the OAuth 2.0 Implicit Flow which is not used in Mail Applications, but also discouraged by the OAuth Working Group: https://developer.okta.com/blog/2019/05/01/is-the-oauth-implicit-flow-dead --- mailnews/base/util/OAuth2.jsm | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index 94f850e0be..dcbfb428fe 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -3,7 +3,8 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ /** - * Provides OAuth 2.0 authentication + * Provides OAuth 2.0 authentication. + * @see RFC 6749 */ var EXPORTED_SYMBOLS = ["OAuth2"]; @@ -41,8 +42,6 @@ OAuth2.CODE_AUTHORIZATION = "authorization_code"; OAuth2.CODE_REFRESH = "refresh_token"; OAuth2.prototype = { - - responseType: "code", consumerKey: null, consumerSecret: null, completionURI: "http://localhost", @@ -79,7 +78,7 @@ OAuth2.prototype = { requestAuthorization: function requestAuthorization() { let params = [ - ["response_type", this.responseType], + ["response_type", "code"], ["client_id", this.consumerKey], ["redirect_uri", this.completionURI], ]; @@ -173,13 +172,11 @@ OAuth2.prototype = { onAuthorizationReceived: function(aData) { this.log.info("authorization received" + aData); let results = parseURLData(aData); - if (this.responseType == "code" && results.code) { + if (results.code) { this.requestAccessToken(results.code, OAuth2.CODE_AUTHORIZATION); - } else if (this.responseType == "token") { - this.onAccessTokenReceived(JSON.stringify(results)); - } - else + } else { this.onAuthorizationFailed(null, aData); + } }, onAuthorizationFailed: function(aError, aData) { From b037870a8607ac0d05f9f8b1ca6cee38de979845 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 09:49:29 -0500 Subject: [PATCH 4/8] Bug 1597933 - improve OAuth2 params parsing. --- mailnews/base/util/OAuth2.jsm | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index dcbfb428fe..8feee0e944 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -15,15 +15,6 @@ Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource:///modules/gloda/log4moz.js"); -function parseURLData(aData) { - let result = {}; - aData.split(/[?#]/, 2)[1].split("&").forEach(function (aParam) { - let [key, value] = aParam.split("="); - result[key] = decodeURIComponent(value); - }); - return result; -} - // Only allow one connecting window per endpoint. var gConnecting = {}; @@ -169,13 +160,14 @@ OAuth2.prototype = { delete this._browserRequest; }, - onAuthorizationReceived: function(aData) { - this.log.info("authorization received" + aData); - let results = parseURLData(aData); - if (results.code) { - this.requestAccessToken(results.code, OAuth2.CODE_AUTHORIZATION); + // @see RFC 6749 section 4.1.2: Authorization Response + onAuthorizationReceived(aURL) { + this.log.info("OAuth2 authorization received: url=" + aURL); + let params = new URLSearchParams(aURL.split("?", 2)[1]); + if (params.has("code")) { + this.requestAccessToken(params.get("code"), OAuth2.CODE_AUTHORIZATION); } else { - this.onAuthorizationFailed(null, aData); + this.onAuthorizationFailed(null, aURL); } }, From a8ac34b90b8a0c0cda72c5020f072754db007709 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 10:20:58 -0500 Subject: [PATCH 5/8] Bug 1597933 - don't pass string constants to determine OAuth refresh token or not. --- mailnews/base/util/OAuth2.jsm | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index 8feee0e944..037333abc4 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -29,9 +29,6 @@ function OAuth2(aBaseURI, aScope, aAppKey, aAppSecret) { this.log = Log4Moz.getConfiguredLogger("TBOAuth"); } -OAuth2.CODE_AUTHORIZATION = "authorization_code"; -OAuth2.CODE_REFRESH = "refresh_token"; - OAuth2.prototype = { consumerKey: null, consumerSecret: null, @@ -53,7 +50,7 @@ OAuth2.prototype = { if (!aRefresh && this.accessToken) { aSuccess(); } else if (this.refreshToken) { - this.requestAccessToken(this.refreshToken, OAuth2.CODE_REFRESH); + this.requestAccessToken(this.refreshToken, true); } else { if (!aWithUI) { aFailure('{ "error": "auth_noui" }'); @@ -165,7 +162,7 @@ OAuth2.prototype = { this.log.info("OAuth2 authorization received: url=" + aURL); let params = new URLSearchParams(aURL.split("?", 2)[1]); if (params.has("code")) { - this.requestAccessToken(params.get("code"), OAuth2.CODE_AUTHORIZATION); + this.requestAccessToken(params.get("code"), false); } else { this.onAuthorizationFailed(null, aURL); } @@ -175,18 +172,27 @@ OAuth2.prototype = { this.connectFailureCallback(aData); }, - requestAccessToken: function requestAccessToken(aCode, aType) { + /** + * Request a new access token, or refresh an existing one. + * @param {string} aCode - The token issued to the client. + * @param {boolean} aRefresh - Whether it's a refresh of a token or not. + */ + requestAccessToken(aCode, aRefresh) { + // @see RFC 6749 section 4.1.3. Access Token Request + // @see RFC 6749 section 6. Refreshing an Access Token + let params = [ ["client_id", this.consumerKey], ["client_secret", this.consumerSecret], - ["grant_type", aType], ]; - if (aType == OAuth2.CODE_AUTHORIZATION) { + if (aRefresh) { + params.push(["grant_type", "refresh_token"]); + params.push(["refresh_token", aCode]); + } else { + params.push(["grant_type", "authorization_code"]); params.push(["code", aCode]); params.push(["redirect_uri", this.completionURI]); - } else if (aType == OAuth2.CODE_REFRESH) { - params.push(["refresh_token", aCode]); } let options = { From 30b347303be3b4c933fc1e2e55a9a624560d083a Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 10:26:15 -0500 Subject: [PATCH 6/8] Bug 1597933 - use fetch + URLSearchParms instead of Http.jsm to request OAuth2 access token. --- mailnews/base/util/OAuth2.jsm | 64 +++++++++++++++++------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index 037333abc4..6b1eb84a10 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -10,11 +10,12 @@ var EXPORTED_SYMBOLS = ["OAuth2"]; var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; -Cu.import("resource://gre/modules/Http.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource:///modules/gloda/log4moz.js"); +Cu.importGlobalProperties(["fetch"]); + // Only allow one connecting window per endpoint. var gConnecting = {}; @@ -181,49 +182,48 @@ OAuth2.prototype = { // @see RFC 6749 section 4.1.3. Access Token Request // @see RFC 6749 section 6. Refreshing an Access Token - let params = [ - ["client_id", this.consumerKey], - ["client_secret", this.consumerSecret], - ]; + let data = new URLSearchParams(); + data.append("client_id", this.consumerKey); + data.append("client_secret", this.consumerSecret); if (aRefresh) { - params.push(["grant_type", "refresh_token"]); - params.push(["refresh_token", aCode]); + data.append("grant_type", "refresh_token"); + data.append("refresh_token", aCode); } else { - params.push(["grant_type", "authorization_code"]); - params.push(["code", aCode]); - params.push(["redirect_uri", this.completionURI]); + data.append("grant_type", "authorization_code"); + data.append("code", aCode); + data.append("redirect_uri", this.completionURI); } - let options = { - postData: params, - onLoad: this.onAccessTokenReceived.bind(this), - onError: this.onAccessTokenFailed.bind(this) - } - httpRequest(this.tokenURI, options); - }, - - onAccessTokenFailed: function onAccessTokenFailed(aError, aData) { - if (aError != "offline") { - this.refreshToken = null; - } - this.connectFailureCallback(aData); - }, - - onAccessTokenReceived: function onRequestTokenReceived(aData) { - let result = JSON.parse(aData); - + this.log.info( + `Making access token request to the token endpoint: ${this.tokenURI}` + ); + fetch(this.tokenURI, { + method: "POST", + cache: "no-cache", + body: data, + }) + .then(response => response.json()) + .then(result => { + this.log.info("The authorization server issued an access token."); this.accessToken = result.access_token; if ("refresh_token" in result) { - this.refreshToken = result.refresh_token; + this.refreshToken = result.refresh_token; } if ("expires_in" in result) { - this.tokenExpires = (new Date()).getTime() + (result.expires_in * 1000); + this.tokenExpires = new Date().getTime() + result.expires_in * 1000; } else { - this.tokenExpires = Number.MAX_VALUE; + this.tokenExpires = Number.MAX_VALUE; } this.tokenType = result.token_type; - this.connectSuccessCallback(); + }) + .catch(err => { + // Getting an access token failed. + this.log.info( + `The authorization server returned an error response: ${err}` + ); + this.connectFailureCallback(err); + }); } }; From 1bc5f20c3adf857b9a1ae05d815fd007c5d1a8a2 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 10:31:00 -0500 Subject: [PATCH 7/8] Bug 1597933 - Use URLSearchParams for setting params for OAuth2 authorization request. --- mailnews/base/util/OAuth2.jsm | 57 +++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index 6b1eb84a10..c838660f0b 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -66,25 +66,31 @@ OAuth2.prototype = { }, requestAuthorization: function requestAuthorization() { - let params = [ - ["response_type", "code"], - ["client_id", this.consumerKey], - ["redirect_uri", this.completionURI], - ]; - // The scope can be optional. + let params = new URLSearchParams({ + response_type: "code", + client_id: this.consumerKey, + redirect_uri: this.completionURI, + }); + + // The scope is optional. if (this.scope) { - params.push(["scope", this.scope]); + params.append("scope", this.scope); } - // Add extra parameters - params.push(...this.extraAuthParams); + for (let [name, value] of this.extraAuthParams) { + params.append(name, value); + } - // Now map the parameters to a string - params = params.map(([k,v]) => k + "=" + encodeURIComponent(v)).join("&"); + let authEndpointURI = this.authURI + "?" + params.toString(); + this.log.info( + "Interacting with the resource owner to obtain an authorization grant " + + "from the authorization endpoint: " + + authEndpointURI + ); this._browserRequest = { account: this, - url: this.authURI + "?" + params, + url: authEndpointURI, _active: true, iconURI: "", cancelled: function() { @@ -187,17 +193,20 @@ OAuth2.prototype = { data.append("client_secret", this.consumerSecret); if (aRefresh) { + this.log.info( + `Making a refresh request to the token endpoint: ${this.tokenURI}` + ); data.append("grant_type", "refresh_token"); data.append("refresh_token", aCode); } else { + this.log.info( + `Making access token request to the token endpoint: ${this.tokenURI}` + ); data.append("grant_type", "authorization_code"); data.append("code", aCode); data.append("redirect_uri", this.completionURI); } - this.log.info( - `Making access token request to the token endpoint: ${this.tokenURI}` - ); fetch(this.tokenURI, { method: "POST", cache: "no-cache", @@ -205,6 +214,18 @@ OAuth2.prototype = { }) .then(response => response.json()) .then(result => { + if ("error" in result) { + // RFC 6749 section 5.2. Error Response + this.log.info( + `The authorization server returned an error response: ${JSON.stringify( + result + )}` + ); + this.connectFailureCallback(result); + return; + } + + // RFC 6749 section 5.1. Successful Response this.log.info("The authorization server issued an access token."); this.accessToken = result.access_token; if ("refresh_token" in result) { @@ -215,14 +236,10 @@ OAuth2.prototype = { } else { this.tokenExpires = Number.MAX_VALUE; } - this.tokenType = result.token_type; this.connectSuccessCallback(); }) .catch(err => { - // Getting an access token failed. - this.log.info( - `The authorization server returned an error response: ${err}` - ); + this.log.info(`Connection to authorization server failed: ${err}`); this.connectFailureCallback(err); }); } From f6096004d78a8080195371c4f5aa3efa3577a0aa Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Mon, 30 Dec 2019 10:33:31 -0500 Subject: [PATCH 8/8] Bug 1599054 - allow callers to ommit sending OAuth2 client_secret parameter. --- mailnews/base/util/OAuth2.jsm | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/mailnews/base/util/OAuth2.jsm b/mailnews/base/util/OAuth2.jsm index c838660f0b..8c9282d025 100644 --- a/mailnews/base/util/OAuth2.jsm +++ b/mailnews/base/util/OAuth2.jsm @@ -19,7 +19,21 @@ Cu.importGlobalProperties(["fetch"]); // Only allow one connecting window per endpoint. var gConnecting = {}; -function OAuth2(aBaseURI, aScope, aAppKey, aAppSecret) { +/** + * Constructor for the OAuth2 object. + * + * @constructor + * @param {string} aBaseURI - The base URI for authentication and token + * requests, oauth2/auth or oauth2/token will be added for the actual + * requests. + * @param {?string} aScope - The scope as specified by RFC 6749 Section 3.3. + * Will not be included in the requests if falsy. + * @param {string} aAppKey - The client_id as specified by RFC 6749 Section + * 2.3.1. + * @param {string} [aAppSecret=null] - The client_secret as specified in + * RFC 6749 section 2.3.1. Will not be included in the requests if null. + */ +function OAuth2(aBaseURI, aScope, aAppKey, aAppSecret = null) { this.authURI = aBaseURI + "oauth2/auth"; this.tokenURI = aBaseURI + "oauth2/token"; this.consumerKey = aAppKey; @@ -190,7 +204,12 @@ OAuth2.prototype = { let data = new URLSearchParams(); data.append("client_id", this.consumerKey); - data.append("client_secret", this.consumerSecret); + if (this.consumerSecret !== null) { + // Section 2.3.1. of RFC 6749 states that empty secrets MAY be omitted + // by the client. This OAuth implementation delegates this decission to + // the caller: If the secret is null, it will be omitted. + data.append("client_secret", this.consumerSecret); + } if (aRefresh) { this.log.info(