From 99b804d56732ad1cd4ea23a84ea31c04c2f88860 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Wed, 18 Sep 2019 13:02:52 +0100 Subject: [PATCH 1/4] Change to separate add and bind to guard 3PID account section This changes to checking for HS support of separate add and bind when guarding the 3PID account section. For older HSes, we in fact always require an IS for add with bind param, so the previous version of this was incorrect. Part of https://github.com/vector-im/riot-web/issues/10839 --- .../settings/tabs/user/GeneralUserSettingsTab.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js index b378db707a..b9c566b22a 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.js @@ -51,7 +51,7 @@ export default class GeneralUserSettingsTab extends React.Component { language: languageHandler.getCurrentLanguage(), theme: SettingsStore.getValueAt(SettingLevel.ACCOUNT, "theme"), haveIdServer: Boolean(MatrixClientPeg.get().getIdentityServerUrl()), - serverRequiresIdServer: null, + serverSupportsSeparateAddAndBind: null, idServerHasUnsignedTerms: false, requiredPolicyInfo: { // This object is passed along to a component for handling hasTerms: false, @@ -69,8 +69,8 @@ export default class GeneralUserSettingsTab extends React.Component { async componentWillMount() { const cli = MatrixClientPeg.get(); - const serverRequiresIdServer = await cli.doesServerRequireIdServerParam(); - this.setState({serverRequiresIdServer}); + const serverSupportsSeparateAddAndBind = await cli.doesServerSupportSeparateAddAndBind(); + this.setState({serverSupportsSeparateAddAndBind}); this._getThreepidState(); } @@ -222,7 +222,12 @@ export default class GeneralUserSettingsTab extends React.Component { let threepidSection = null; - if (this.state.haveIdServer || this.state.serverRequiresIdServer === false) { + // For older homeservers without separate 3PID add and bind methods (MSC2290), + // we use a combo add with bind option API which requires an identity server to + // validate 3PID ownership even if we're just adding to the homeserver only. + // For newer homeservers with separate 3PID add and bind methods (MSC2290), + // there is no such concern, so we can always show the HS account 3PIDs. + if (this.state.haveIdServer || this.state.serverSupportsSeparateAddAndBind === true) { threepidSection =
{_t("Email addresses")}
; - } else if (this.state.serverRequiresIdServer === null) { + } else if (this.state.serverSupportsSeparateAddAndBind === null) { threepidSection = ; } From 9a1305bf4a044c3f489be525419aa9d58ab56f90 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Thu, 19 Sep 2019 15:50:18 +0100 Subject: [PATCH 2/4] Extract separate add / bind methods on AddThreepid --- src/AddThreepid.js | 42 ++++++++++++++----- .../views/dialogs/SetEmailDialog.js | 4 +- .../views/settings/account/EmailAddresses.js | 2 +- .../views/settings/account/PhoneNumbers.js | 2 +- .../settings/discovery/EmailAddresses.js | 6 ++- .../views/settings/discovery/PhoneNumbers.js | 6 ++- 6 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/AddThreepid.js b/src/AddThreepid.js index 8bd3099002..968f381ab0 100644 --- a/src/AddThreepid.js +++ b/src/AddThreepid.js @@ -1,6 +1,7 @@ /* Copyright 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd +Copyright 2019 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,14 +34,12 @@ export default class AddThreepid { } /** - * Attempt to add an email threepid. This will trigger a side-effect of - * sending an email to the provided email address. + * Attempt to add an email threepid to the homeserver. + * This will trigger a side-effect of sending an email to the provided email address. * @param {string} emailAddress The email address to add - * @param {boolean} bind If True, bind this email to this mxid on the Identity Server * @return {Promise} Resolves when the email has been sent. Then call checkEmailLinkClicked(). */ - addEmailAddress(emailAddress, bind) { - this.bind = bind; + addEmailAddress(emailAddress) { return MatrixClientPeg.get().requestAdd3pidEmailToken(emailAddress, this.clientSecret, 1).then((res) => { this.sessionId = res.sid; return res; @@ -55,15 +54,25 @@ export default class AddThreepid { } /** - * Attempt to add a msisdn threepid. This will trigger a side-effect of - * sending a test message to the provided phone number. + * Attempt to bind an email threepid on the identity server via the homeserver. + * This will trigger a side-effect of sending an email to the provided email address. + * @param {string} emailAddress The email address to add + * @return {Promise} Resolves when the email has been sent. Then call checkEmailLinkClicked(). + */ + bindEmailAddress(emailAddress) { + this.bind = true; + // TODO: Actually use a different API here + return this.addEmailAddress(emailAddress); + } + + /** + * Attempt to add a MSISDN threepid to the homeserver. + * This will trigger a side-effect of sending an SMS to the provided phone number. * @param {string} phoneCountry The ISO 2 letter code of the country to resolve phoneNumber in * @param {string} phoneNumber The national or international formatted phone number to add - * @param {boolean} bind If True, bind this phone number to this mxid on the Identity Server * @return {Promise} Resolves when the text message has been sent. Then call haveMsisdnToken(). */ - addMsisdn(phoneCountry, phoneNumber, bind) { - this.bind = bind; + addMsisdn(phoneCountry, phoneNumber) { return MatrixClientPeg.get().requestAdd3pidMsisdnToken( phoneCountry, phoneNumber, this.clientSecret, 1, ).then((res) => { @@ -79,6 +88,19 @@ export default class AddThreepid { }); } + /** + * Attempt to bind a MSISDN threepid on the identity server via the homeserver. + * This will trigger a side-effect of sending an SMS to the provided phone number. + * @param {string} phoneCountry The ISO 2 letter code of the country to resolve phoneNumber in + * @param {string} phoneNumber The national or international formatted phone number to add + * @return {Promise} Resolves when the text message has been sent. Then call haveMsisdnToken(). + */ + bindMsisdn(phoneCountry, phoneNumber) { + this.bind = true; + // TODO: Actually use a different API here + return this.addMsisdn(phoneCountry, phoneNumber); + } + /** * Checks if the email link has been clicked by attempting to add the threepid * @return {Promise} Resolves if the email address was added. Rejects with an object diff --git a/src/components/views/dialogs/SetEmailDialog.js b/src/components/views/dialogs/SetEmailDialog.js index 88baa5fd3e..bedf713c4e 100644 --- a/src/components/views/dialogs/SetEmailDialog.js +++ b/src/components/views/dialogs/SetEmailDialog.js @@ -62,9 +62,7 @@ export default createReactClass({ return; } this._addThreepid = new AddThreepid(); - // we always bind emails when registering, so let's do the - // same here. - this._addThreepid.addEmailAddress(emailAddress, true).done(() => { + this._addThreepid.addEmailAddress(emailAddress).done(() => { Modal.createTrackedDialog('Verification Pending', '', QuestionDialog, { title: _t("Verification Pending"), description: _t( diff --git a/src/components/views/settings/account/EmailAddresses.js b/src/components/views/settings/account/EmailAddresses.js index b7324eb272..a1817b374b 100644 --- a/src/components/views/settings/account/EmailAddresses.js +++ b/src/components/views/settings/account/EmailAddresses.js @@ -161,7 +161,7 @@ export default class EmailAddresses extends React.Component { const task = new AddThreepid(); this.setState({verifying: true, continueDisabled: true, addTask: task}); - task.addEmailAddress(email, false).then(() => { + task.addEmailAddress(email).then(() => { this.setState({continueDisabled: false}); }).catch((err) => { console.error("Unable to add email address " + email + " " + err); diff --git a/src/components/views/settings/account/PhoneNumbers.js b/src/components/views/settings/account/PhoneNumbers.js index f814561c97..e3b1f5d2a7 100644 --- a/src/components/views/settings/account/PhoneNumbers.js +++ b/src/components/views/settings/account/PhoneNumbers.js @@ -158,7 +158,7 @@ export default class PhoneNumbers extends React.Component { const task = new AddThreepid(); this.setState({verifying: true, continueDisabled: true, addTask: task}); - task.addMsisdn(phoneCountry, phoneNumber, false).then((response) => { + task.addMsisdn(phoneCountry, phoneNumber).then((response) => { this.setState({continueDisabled: false, verifyMsisdn: response.msisdn}); }).catch((err) => { console.error("Unable to add phone number " + phoneNumber + " " + err); diff --git a/src/components/views/settings/discovery/EmailAddresses.js b/src/components/views/settings/discovery/EmailAddresses.js index 19e78afb8c..ec641ccff9 100644 --- a/src/components/views/settings/discovery/EmailAddresses.js +++ b/src/components/views/settings/discovery/EmailAddresses.js @@ -82,7 +82,11 @@ export class EmailAddress extends React.Component { // it with IS binding enabled. // See https://github.com/matrix-org/matrix-doc/pull/2140/files#r311462052 await MatrixClientPeg.get().deleteThreePid(medium, address); - await task.addEmailAddress(address, bind); + if (bind) { + await task.bindEmailAddress(address); + } else { + await task.addEmailAddress(address); + } this.setState({ continueDisabled: false, bound: bind, diff --git a/src/components/views/settings/discovery/PhoneNumbers.js b/src/components/views/settings/discovery/PhoneNumbers.js index 99a90f23fb..3462d7a467 100644 --- a/src/components/views/settings/discovery/PhoneNumbers.js +++ b/src/components/views/settings/discovery/PhoneNumbers.js @@ -78,7 +78,11 @@ export class PhoneNumber extends React.Component { // a leading plus sign to a number in E.164 format (which the 3PID // address is), but this goes against the spec. // See https://github.com/matrix-org/matrix-doc/issues/2222 - await task.addMsisdn(null, `+${address}`, bind); + if (bind) { + await task.bindMsisdn(null, `+${address}`); + } else { + await task.addMsisdn(null, `+${address}`); + } this.setState({ continueDisabled: false, bound: bind, From ff69ad02b9c49312259fefabc469f9f4daaad027 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 20 Sep 2019 11:49:32 +0100 Subject: [PATCH 3/4] Use separate email add and bind flow for supporting HSes This changes the paths used for binding emails for discovery to use the new separate bind / unbind APIs on supporting servers. Part of https://github.com/vector-im/riot-web/issues/10839 --- src/AddThreepid.js | 64 ++++++++++++++++--- .../settings/discovery/EmailAddresses.js | 44 +++++++++++-- 2 files changed, 92 insertions(+), 16 deletions(-) diff --git a/src/AddThreepid.js b/src/AddThreepid.js index 968f381ab0..a04e1c2653 100644 --- a/src/AddThreepid.js +++ b/src/AddThreepid.js @@ -27,6 +27,10 @@ import IdentityAuthClient from './IdentityAuthClient'; * This involves getting an email token from the identity server to "prove" that * the client owns the given email address, which is then passed to the * add threepid API on the homeserver. + * + * Diagrams of the intended API flows here are available at: + * + * https://gist.github.com/jryans/839a09bf0c5a70e2f36ed990d50ed928 */ export default class AddThreepid { constructor() { @@ -59,10 +63,30 @@ export default class AddThreepid { * @param {string} emailAddress The email address to add * @return {Promise} Resolves when the email has been sent. Then call checkEmailLinkClicked(). */ - bindEmailAddress(emailAddress) { + async bindEmailAddress(emailAddress) { this.bind = true; - // TODO: Actually use a different API here - return this.addEmailAddress(emailAddress); + if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + // For separate bind, request a token directly from the IS. + const authClient = new IdentityAuthClient(); + const identityAccessToken = await authClient.getAccessToken(); + return MatrixClientPeg.get().requestEmailToken( + emailAddress, this.clientSecret, 1, + undefined, undefined, identityAccessToken, + ).then((res) => { + this.sessionId = res.sid; + return res; + }, function(err) { + if (err.errcode === 'M_THREEPID_IN_USE') { + err.message = _t('This email address is already in use'); + } else if (err.httpStatus) { + err.message = err.message + ` (Status ${err.httpStatus})`; + } + throw err; + }); + } else { + // For tangled bind, request a token via the HS. + return this.addEmailAddress(emailAddress); + } } /** @@ -107,20 +131,40 @@ export default class AddThreepid { * with a "message" property which contains a human-readable message detailing why * the request failed. */ - checkEmailLinkClicked() { + async checkEmailLinkClicked() { const identityServerDomain = MatrixClientPeg.get().idBaseUrl.split("://")[1]; - return MatrixClientPeg.get().addThreePid({ - sid: this.sessionId, - client_secret: this.clientSecret, - id_server: identityServerDomain, - }, this.bind).catch(function(err) { + try { + if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + if (this.bind) { + const authClient = new IdentityAuthClient(); + const identityAccessToken = await authClient.getAccessToken(); + await MatrixClientPeg.get().bindThreePid({ + sid: this.sessionId, + client_secret: this.clientSecret, + id_server: identityServerDomain, + id_access_token: identityAccessToken, + }); + } else { + await MatrixClientPeg.get().addThreePidOnly({ + sid: this.sessionId, + client_secret: this.clientSecret, + }); + } + } else { + await MatrixClientPeg.get().addThreePid({ + sid: this.sessionId, + client_secret: this.clientSecret, + id_server: identityServerDomain, + }, this.bind); + } + } catch (err) { if (err.httpStatus === 401) { err.message = _t('Failed to verify email address: make sure you clicked the link in the email'); } else if (err.httpStatus) { err.message += ` (Status ${err.httpStatus})`; } throw err; - }); + } } /** diff --git a/src/components/views/settings/discovery/EmailAddresses.js b/src/components/views/settings/discovery/EmailAddresses.js index ec641ccff9..a4fcd605fc 100644 --- a/src/components/views/settings/discovery/EmailAddresses.js +++ b/src/components/views/settings/discovery/EmailAddresses.js @@ -64,6 +64,44 @@ export class EmailAddress extends React.Component { } async changeBinding({ bind, label, errorTitle }) { + if (!await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + return this.changeBindingTangledAddBind({ bind, label, errorTitle }); + } + + const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); + const { medium, address } = this.props.email; + + try { + if (bind) { + const task = new AddThreepid(); + this.setState({ + verifying: true, + continueDisabled: true, + addTask: task, + }); + await task.bindEmailAddress(address); + this.setState({ + continueDisabled: false, + }); + } else { + await MatrixClientPeg.get().unbindThreePid(medium, address); + } + this.setState({ bound: bind }); + } catch (err) { + console.error(`Unable to ${label} email address ${address} ${err}`); + this.setState({ + verifying: false, + continueDisabled: false, + addTask: null, + }); + Modal.createTrackedDialog(`Unable to ${label} email address`, '', ErrorDialog, { + title: errorTitle, + description: ((err && err.message) ? err.message : _t("Operation failed")), + }); + } + } + + async changeBindingTangledAddBind({ bind, label, errorTitle }) { const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); const { medium, address } = this.props.email; @@ -75,12 +113,6 @@ export class EmailAddress extends React.Component { }); try { - // XXX: Unfortunately, at the moment we can't just bind via the HS - // in a single operation, at it will error saying the 3PID is in use - // even though it's in use by the current user. For the moment, we - // work around this by removing the 3PID from the HS and re-adding - // it with IS binding enabled. - // See https://github.com/matrix-org/matrix-doc/pull/2140/files#r311462052 await MatrixClientPeg.get().deleteThreePid(medium, address); if (bind) { await task.bindEmailAddress(address); From f9a09d271b2523aaf7c23af6d403e72148f023d5 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 20 Sep 2019 12:45:22 +0100 Subject: [PATCH 4/4] Use separate MSISDN add and bind flow for supporting HSes This changes the paths used for binding MSISDNs for discovery to use the new separate bind / unbind APIs on supporting servers. Fixes https://github.com/vector-im/riot-web/issues/10839 --- src/AddThreepid.js | 54 ++++++++++++++++--- .../views/settings/discovery/PhoneNumbers.js | 48 ++++++++++++++--- 2 files changed, 88 insertions(+), 14 deletions(-) diff --git a/src/AddThreepid.js b/src/AddThreepid.js index a04e1c2653..3b66b1d7ee 100644 --- a/src/AddThreepid.js +++ b/src/AddThreepid.js @@ -119,10 +119,30 @@ export default class AddThreepid { * @param {string} phoneNumber The national or international formatted phone number to add * @return {Promise} Resolves when the text message has been sent. Then call haveMsisdnToken(). */ - bindMsisdn(phoneCountry, phoneNumber) { + async bindMsisdn(phoneCountry, phoneNumber) { this.bind = true; - // TODO: Actually use a different API here - return this.addMsisdn(phoneCountry, phoneNumber); + if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + // For separate bind, request a token directly from the IS. + const authClient = new IdentityAuthClient(); + const identityAccessToken = await authClient.getAccessToken(); + return MatrixClientPeg.get().requestMsisdnToken( + phoneCountry, phoneNumber, this.clientSecret, 1, + undefined, undefined, identityAccessToken, + ).then((res) => { + this.sessionId = res.sid; + return res; + }, function(err) { + if (err.errcode === 'M_THREEPID_IN_USE') { + err.message = _t('This phone number is already in use'); + } else if (err.httpStatus) { + err.message = err.message + ` (Status ${err.httpStatus})`; + } + throw err; + }); + } else { + // For tangled bind, request a token via the HS. + return this.addMsisdn(phoneCountry, phoneNumber); + } } /** @@ -189,10 +209,28 @@ export default class AddThreepid { } const identityServerDomain = MatrixClientPeg.get().idBaseUrl.split("://")[1]; - return MatrixClientPeg.get().addThreePid({ - sid: this.sessionId, - client_secret: this.clientSecret, - id_server: identityServerDomain, - }, this.bind); + if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + if (this.bind) { + const authClient = new IdentityAuthClient(); + const identityAccessToken = await authClient.getAccessToken(); + await MatrixClientPeg.get().bindThreePid({ + sid: this.sessionId, + client_secret: this.clientSecret, + id_server: identityServerDomain, + id_access_token: identityAccessToken, + }); + } else { + await MatrixClientPeg.get().addThreePidOnly({ + sid: this.sessionId, + client_secret: this.clientSecret, + }); + } + } else { + await MatrixClientPeg.get().addThreePid({ + sid: this.sessionId, + client_secret: this.clientSecret, + id_server: identityServerDomain, + }, this.bind); + } } } diff --git a/src/components/views/settings/discovery/PhoneNumbers.js b/src/components/views/settings/discovery/PhoneNumbers.js index 3462d7a467..d6c447938d 100644 --- a/src/components/views/settings/discovery/PhoneNumbers.js +++ b/src/components/views/settings/discovery/PhoneNumbers.js @@ -56,6 +56,48 @@ export class PhoneNumber extends React.Component { } async changeBinding({ bind, label, errorTitle }) { + if (!await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) { + return this.changeBindingTangledAddBind({ bind, label, errorTitle }); + } + + const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); + const { medium, address } = this.props.msisdn; + + try { + if (bind) { + const task = new AddThreepid(); + this.setState({ + verifying: true, + continueDisabled: true, + addTask: task, + }); + // XXX: Sydent will accept a number without country code if you add + // a leading plus sign to a number in E.164 format (which the 3PID + // address is), but this goes against the spec. + // See https://github.com/matrix-org/matrix-doc/issues/2222 + await task.bindMsisdn(null, `+${address}`); + this.setState({ + continueDisabled: false, + }); + } else { + await MatrixClientPeg.get().unbindThreePid(medium, address); + } + this.setState({ bound: bind }); + } catch (err) { + console.error(`Unable to ${label} phone number ${address} ${err}`); + this.setState({ + verifying: false, + continueDisabled: false, + addTask: null, + }); + Modal.createTrackedDialog(`Unable to ${label} phone number`, '', ErrorDialog, { + title: errorTitle, + description: ((err && err.message) ? err.message : _t("Operation failed")), + }); + } + } + + async changeBindingTangledAddBind({ bind, label, errorTitle }) { const ErrorDialog = sdk.getComponent("dialogs.ErrorDialog"); const { medium, address } = this.props.msisdn; @@ -67,12 +109,6 @@ export class PhoneNumber extends React.Component { }); try { - // XXX: Unfortunately, at the moment we can't just bind via the HS - // in a single operation, at it will error saying the 3PID is in use - // even though it's in use by the current user. For the moment, we - // work around this by removing the 3PID from the HS and re-adding - // it with IS binding enabled. - // See https://github.com/matrix-org/matrix-doc/pull/2140/files#r311462052 await MatrixClientPeg.get().deleteThreePid(medium, address); // XXX: Sydent will accept a number without country code if you add // a leading plus sign to a number in E.164 format (which the 3PID