From e32c325863b2ef3091549927c616d850b8e3bc54 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 11 Aug 2016 13:50:38 +0100 Subject: [PATCH 1/2] Don't use MatrixClientPeg for temporary clients Get rid of MatrixClientPeg.replaceUsingUrls, and instead create local, temporary MatrixClients for the unauthed steps; we therefore only use MatrixClientPeg for logged-in clients. --- src/Lifecycle.js | 21 ++++-- src/MatrixClientPeg.js | 32 ++------- src/Signup.js | 87 +++++++++++-------------- src/components/structures/MatrixChat.js | 18 +---- 4 files changed, 61 insertions(+), 97 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index bc7d24c707..0a2a503ce6 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -142,8 +142,12 @@ function _loginWithToken(queryParams) { function _registerAsGuest(hsUrl, isUrl) { console.log("Doing guest login on %s", hsUrl); - MatrixClientPeg.replaceUsingUrls(hsUrl, isUrl); - return MatrixClientPeg.get().registerGuest().then((creds) => { + // create a temporary MatrixClient to do the login + var client = Matrix.createClient({ + baseUrl: hsUrl, + }); + + return client.registerGuest().then((creds) => { console.log("Registered as guest: %s", creds.user_id); setLoggedIn({ userId: creds.user_id, @@ -286,7 +290,7 @@ export function onLoggedOut() { if (hsUrl) window.localStorage.setItem("mx_hs_url", hsUrl); if (isUrl) window.localStorage.setItem("mx_is_url", isUrl); } - _stopMatrixClient(); + stopMatrixClient(); dis.dispatch({action: 'on_logged_out'}); } @@ -294,11 +298,14 @@ export function onLoggedOut() { /** * Stop all the background processes related to the current client */ -function _stopMatrixClient() { +export function stopMatrixClient() { Notifier.stop(); UserActivity.stop(); Presence.stop(); - MatrixClientPeg.get().stopClient(); - MatrixClientPeg.get().removeAllListeners(); - MatrixClientPeg.unset(); + var cli = MatrixClientPeg.get(); + if (cli) { + cli.stopClient(); + cli.removeAllListeners(); + MatrixClientPeg.unset(); + } } diff --git a/src/MatrixClientPeg.js b/src/MatrixClientPeg.js index 4fefc885af..e22990a850 100644 --- a/src/MatrixClientPeg.js +++ b/src/MatrixClientPeg.js @@ -67,26 +67,12 @@ class MatrixClientPeg { this.matrixClient = null; } - /** - * Replace this MatrixClientPeg's client with a client instance that has - * Home Server / Identity Server URLs but no credentials - */ - replaceUsingUrls(hs_url, is_url) { - this._replaceClient(hs_url, is_url); - } - /** * Replace this MatrixClientPeg's client with a client instance that has * Home Server / Identity Server URLs and active credentials */ replaceUsingCreds(creds: MatrixClientCreds) { - this._replaceClient( - creds.homeserverUrl, - creds.identityServerUrl, - creds.userId, - creds.accessToken, - creds.guest, - ); + this._createClient(creds); } start() { @@ -96,10 +82,6 @@ class MatrixClientPeg { this.get().startClient(opts); } - _replaceClient(hs_url, is_url, user_id, access_token, isGuest) { - this._createClient(hs_url, is_url, user_id, access_token, isGuest); - } - getCredentials(): MatrixClientCreds { return { homeserverUrl: this.matrixClient.baseUrl, @@ -110,12 +92,12 @@ class MatrixClientPeg { }; } - _createClient(hs_url, is_url, user_id, access_token, isGuest) { + _createClient(creds: MatrixClientCreds) { var opts = { - baseUrl: hs_url, - idBaseUrl: is_url, - accessToken: access_token, - userId: user_id, + baseUrl: creds.homeserverUrl, + idBaseUrl: creds.identityServerUrl, + accessToken: creds.accessToken, + userId: creds.userId, timelineSupport: true, }; @@ -130,7 +112,7 @@ class MatrixClientPeg { // potential number of event listeners is quite high. this.matrixClient.setMaxListeners(500); - this.matrixClient.setGuest(Boolean(isGuest)); + this.matrixClient.setGuest(Boolean(creds.guest)); } } diff --git a/src/Signup.js b/src/Signup.js index 7a46005d11..aa0aee597c 100644 --- a/src/Signup.js +++ b/src/Signup.js @@ -1,4 +1,7 @@ "use strict"; + +import Matrix from "matrix-js-sdk"; + var MatrixClientPeg = require("./MatrixClientPeg"); var SignupStages = require("./SignupStages"); var dis = require("./dispatcher"); @@ -31,6 +34,17 @@ class Signup { setIdentityServerUrl(isUrl) { this._isUrl = isUrl; } + + /** + * Get a temporary MatrixClient, which can be used for login or register + * requests. + */ + _createTemporaryClient() { + return Matrix.createClient({ + baseUrl: this._hsUrl, + idBaseUrl: this._isUrl, + }); + } } /** @@ -106,19 +120,11 @@ class Register extends Signup { this.email = email; this.username = username; this.password = password; - - // feels a bit wrong to be clobbering the global client for something we - // don't even know if it'll work, but we'll leave this here for now to - // not complicate matters further. It would be nicer to isolate this - // logic entirely from the rest of the app though. - MatrixClientPeg.replaceUsingUrls( - this._hsUrl, - this._isUrl - ); - return this._tryRegister(); + const client = this._createTemporaryClient(); + return this._tryRegister(client); } - _tryRegister(authDict, poll_for_success) { + _tryRegister(client, authDict, poll_for_success) { var self = this; var bindEmail; @@ -129,7 +135,7 @@ class Register extends Signup { bindEmail = true; } - return MatrixClientPeg.get().register( + return client.register( this.username, this.password, this.params.sessionId, authDict, bindEmail, this.guestAccessToken ).then(function(result) { @@ -152,7 +158,7 @@ class Register extends Signup { console.log("Active flow => %s", JSON.stringify(flow)); var flowStage = self.firstUncompletedStage(flow); if (flowStage != self.activeStage) { - return self.startStage(flowStage).catch(function(err) { + return self._startStage(client, flowStage).catch(function(err) { self.setStep('START'); throw err; }); @@ -161,7 +167,7 @@ class Register extends Signup { } if (poll_for_success) { return q.delay(5000).then(function() { - return self._tryRegister(authDict, poll_for_success); + return self._tryRegister(client, authDict, poll_for_success); }); } else { throw new Error("Authorisation failed!"); @@ -201,7 +207,7 @@ class Register extends Signup { return completed.indexOf(stageType) !== -1; } - startStage(stageName) { + _startStage(client, stageName) { var self = this; this.setStep(`STEP_${stageName}`); var StageClass = SignupStages[stageName]; @@ -210,12 +216,12 @@ class Register extends Signup { throw new Error("Unknown stage: " + stageName); } - var stage = new StageClass(MatrixClientPeg.get(), this); + var stage = new StageClass(client, this); this.activeStage = stage; return stage.complete().then(function(request) { if (request.auth) { console.log("Stage %s is returning an auth dict", stageName); - return self._tryRegister(request.auth, request.poll_for_success); + return self._tryRegister(client, request.auth, request.poll_for_success); } else { // never resolve the promise chain. This is for things like email auth @@ -263,14 +269,6 @@ class Register extends Signup { } recheckState() { - // feels a bit wrong to be clobbering the global client for something we - // don't even know if it'll work, but we'll leave this here for now to - // not complicate matters further. It would be nicer to isolate this - // logic entirely from the rest of the app though. - MatrixClientPeg.replaceUsingUrls( - this._hsUrl, - this._isUrl - ); // We've been given a bunch of data from a previous register step, // this only happens for email auth currently. It's kinda ming we need // to know this though. A better solution would be to ask the stages if @@ -281,7 +279,8 @@ class Register extends Signup { ); if (this.params.hasEmailInfo) { - this.registrationPromise = this.startStage(EMAIL_STAGE_TYPE); + const client = this._createTemporaryClient(); + this.registrationPromise = this._startStage(client, EMAIL_STAGE_TYPE); } return this.registrationPromise; } @@ -305,15 +304,8 @@ class Login extends Signup { getFlows() { var self = this; - // feels a bit wrong to be clobbering the global client for something we - // don't even know if it'll work, but we'll leave this here for now to - // not complicate matters further. It would be nicer to isolate this - // logic entirely from the rest of the app though. - MatrixClientPeg.replaceUsingUrls( - this._hsUrl, - this._isUrl - ); - return MatrixClientPeg.get().loginFlows().then(function(result) { + var client = this._createTemporaryClient(); + return client.loginFlows().then(function(result) { self._flows = result.flows; self._currentFlowIndex = 0; // technically the UI should display options for all flows for the @@ -334,8 +326,8 @@ class Login extends Signup { } loginAsGuest() { - MatrixClientPeg.replaceUsingUrls(this._hsUrl, this._isUrl); - return MatrixClientPeg.get().registerGuest().then((creds) => { + var client = this._createTemporaryClient(); + return client.registerGuest().then((creds) => { return { userId: creds.user_id, accessToken: creds.access_token, @@ -366,7 +358,8 @@ class Login extends Signup { loginParams.user = username; } - return MatrixClientPeg.get().login('m.login.password', loginParams).then(function(data) { + var client = this._createTemporaryClient(); + return client.login('m.login.password', loginParams).then(function(data) { return q({ homeserverUrl: self._hsUrl, identityServerUrl: self._isUrl, @@ -384,13 +377,12 @@ class Login extends Signup { 'Incorrect username and/or password.' ); if (self._fallbackHsUrl) { - // as per elsewhere, it would be much nicer to not replace the global - // client just to try an alternate HS - MatrixClientPeg.replaceUsingUrls( - self._fallbackHsUrl, - self._isUrl - ); - return MatrixClientPeg.get().login('m.login.password', loginParams).then(function(data) { + var fbClient = Matrix.createClient({ + baseUrl: self._fallbackHsUrl, + idBaseUrl: this._isUrl, + }); + + return fbClient.login('m.login.password', loginParams).then(function(data) { return q({ homeserverUrl: self._fallbackHsUrl, identityServerUrl: self._isUrl, @@ -398,11 +390,6 @@ class Login extends Signup { accessToken: data.access_token }); }, function(fallback_error) { - // We also have to put the default back again if it fails... - MatrixClientPeg.replaceUsingUrls( - this._hsUrl, - this._isUrl - ); // throw the original error throw error; }); diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index ffd4ad0a70..25a7e6a81a 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -193,7 +193,7 @@ module.exports = React.createClass({ }, componentWillUnmount: function() { - this._stopMatrixClient(); + Lifecycle.stopMatrixClient(); dis.unregister(this.dispatcherRef); document.removeEventListener("keydown", this.onKeyDown); window.removeEventListener("focus", this.onFocus); @@ -601,16 +601,6 @@ module.exports = React.createClass({ }); }, - // stop all the background processes related to the current client - _stopMatrixClient: function() { - Notifier.stop(); - UserActivity.stop(); - Presence.stop(); - MatrixClientPeg.get().stopClient(); - MatrixClientPeg.get().removeAllListeners(); - MatrixClientPeg.unset(); - }, - onKeyDown: function(ev) { /* // Remove this for now as ctrl+alt = alt-gr so this breaks keyboards which rely on alt-gr for numbers @@ -935,10 +925,8 @@ module.exports = React.createClass({ var NewVersionBar = sdk.getComponent('globals.NewVersionBar'); var ForgotPassword = sdk.getComponent('structures.login.ForgotPassword'); - // work out the HS URL prompts we should show for - - console.log("rendering; loading="+this.state.loading+"; screen="+this.state.screen + - "; logged_in="+this.state.logged_in+"; ready="+this.state.ready); + // console.log("rendering; loading="+this.state.loading+"; screen="+this.state.screen + + // "; logged_in="+this.state.logged_in+"; ready="+this.state.ready); if (this.state.loading) { var Spinner = sdk.getComponent('elements.Spinner'); From 5440fd750477cb18f4ebf98d4b6691422c2d74d9 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 12 Aug 2016 07:27:53 +0100 Subject: [PATCH 2/2] Fix tests MatrixClientPeg no longer has a replaceUsingUrls method, so don't try to stub it out. --- test/test-utils.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/test-utils.js b/test/test-utils.js index e2ff5e8c10..799f04ce54 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -50,8 +50,7 @@ module.exports.stubClient = function() { // // 'sandbox.restore()' doesn't work correctly on inherited methods, // so we do this for each method - var methods = ['get', 'unset', 'replaceUsingUrls', - 'replaceUsingCreds']; + var methods = ['get', 'unset', 'replaceUsingCreds']; for (var i = 0; i < methods.length; i++) { sandbox.stub(peg, methods[i]); } @@ -184,4 +183,3 @@ module.exports.mkStubRoom = function() { }, }; }; -