Merge pull request #3462 from matrix-org/jryans/msc2290

Use separate 3PID add and bind flow for supporting HSes

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.
@@ -26,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() {
@@ -33,14 +38,12 @@ export default class AddThreepid {
     }
 
     /**
-     * Attempt to add an email threepid. This will trigger a side-effect of
+     * Attempt to add an email threepid to the homeserver.
-     * sending an email to the provided email address.
+     * 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) {
+    addEmailAddress(emailAddress) {
-        this.bind = bind;
         return MatrixClientPeg.get().requestAdd3pidEmailToken(emailAddress, this.clientSecret, 1).then((res) => {
             this.sessionId = res.sid;
             return res;
@@ -55,15 +58,45 @@ export default class AddThreepid {
     }
 
     /**
-     * Attempt to add a msisdn threepid. This will trigger a side-effect of
+     * Attempt to bind an email threepid on the identity server via the homeserver.
-     * sending a test message to the provided phone number.
+     * 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().
+     */
+    async bindEmailAddress(emailAddress) {
+        this.bind = true;
+        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);
+        }
+    }
+
+    /**
+     * 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) {
+    addMsisdn(phoneCountry, phoneNumber) {
-        this.bind = bind;
         return MatrixClientPeg.get().requestAdd3pidMsisdnToken(
             phoneCountry, phoneNumber, this.clientSecret, 1,
         ).then((res) => {
@@ -79,26 +112,79 @@ 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().
+     */
+    async bindMsisdn(phoneCountry, phoneNumber) {
+        this.bind = true;
+        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);
+        }
+    }
+
     /**
      * 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
      * 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({
+        try {
-            sid: this.sessionId,
+            if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) {
-            client_secret: this.clientSecret,
+                if (this.bind) {
-            id_server: identityServerDomain,
+                    const authClient = new IdentityAuthClient();
-        }, this.bind).catch(function(err) {
+                    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;
-        });
+        }
     }
 
     /**
@@ -123,10 +209,28 @@ export default class AddThreepid {
         }
 
         const identityServerDomain = MatrixClientPeg.get().idBaseUrl.split("://")[1];
-        return MatrixClientPeg.get().addThreePid({
+        if (await MatrixClientPeg.get().doesServerSupportSeparateAddAndBind()) {
-            sid: this.sessionId,
+            if (this.bind) {
-            client_secret: this.clientSecret,
+                const authClient = new IdentityAuthClient();
-            id_server: identityServerDomain,
+                const identityAccessToken = await authClient.getAccessToken();
-        }, this.bind);
+                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);
+        }
     }
 }

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
+        this._addThreepid.addEmailAddress(emailAddress).done(() => {
-        // same here.
-        this._addThreepid.addEmailAddress(emailAddress, true).done(() => {
             Modal.createTrackedDialog('Verification Pending', '', QuestionDialog, {
                 title: _t("Verification Pending"),
                 description: _t(

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);

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);

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,14 +113,12 @@ 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);
-            await task.addEmailAddress(address, bind);
+            if (bind) {
+                await task.bindEmailAddress(address);
+            } else {
+                await task.addEmailAddress(address);
+            }
             this.setState({
                 continueDisabled: false,
                 bound: bind,

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,18 +109,16 @@ 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
             // 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,

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();
+        const serverSupportsSeparateAddAndBind = await cli.doesServerSupportSeparateAddAndBind();
-        this.setState({serverRequiresIdServer});
+        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 = <div>
                 <span className="mx_SettingsTab_subheading">{_t("Email addresses")}</span>
                 <EmailAddresses
@@ -236,7 +241,7 @@ export default class GeneralUserSettingsTab extends React.Component {
                     onMsisdnsChange={this._onMsisdnsChange}
                 />
             </div>;
-        } else if (this.state.serverRequiresIdServer === null) {
+        } else if (this.state.serverSupportsSeparateAddAndBind === null) {
             threepidSection = <Spinner />;
         }