Fix room alias address isn't checked for validity before being shown as added (#7107)

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>

src/components/views/elements/RoomAliasField.tsx

@@ -18,12 +18,12 @@ import React, { createRef, KeyboardEventHandler } from "react";
 
 import { _t } from '../../../languageHandler';
 import withValidation from './Validation';
-import { MatrixClientPeg } from '../../../MatrixClientPeg';
 import { replaceableComponent } from "../../../utils/replaceableComponent";
 import Field, { IValidateOpts } from "./Field";
+import MatrixClientContext from "../../../contexts/MatrixClientContext";
 
 interface IProps {
-    domain: string;
+    domain?: string;
     value: string;
     label?: string;
     placeholder?: string;
@@ -39,6 +39,9 @@ interface IState {
 // Controlled form component wrapping Field for inputting a room alias scoped to a given domain
 @replaceableComponent("views.elements.RoomAliasField")
 export default class RoomAliasField extends React.PureComponent<IProps, IState> {
+    static contextType = MatrixClientContext;
+    public context!: React.ContextType<typeof MatrixClientContext>;
+
     private fieldRef = createRef<Field>();
 
     constructor(props, context) {
@@ -50,25 +53,38 @@ export default class RoomAliasField extends React.PureComponent<IProps, IState>
     }
 
     private asFullAlias(localpart: string): string {
-        return `#${localpart}:${this.props.domain}`;
+        const hashAlias = `#${ localpart }`;
+        if (this.props.domain) {
+            return `${hashAlias}:${this.props.domain}`;
+        }
+        return hashAlias;
+    }
+
+    private get domainProps() {
+        const { domain } = this.props;
+        const prefix = <span>#</span>;
+        const postfix = domain ? (<span title={`:${domain}`}>{ `:${domain}` }</span>) : <span />;
+        const maxlength = domain ? 255 - domain.length - 2 : 255 - 1;   // 2 for # and :
+        const value = domain ?
+            this.props.value.substring(1, this.props.value.length - this.props.domain.length - 1) :
+            this.props.value.substring(1);
+
+        return { prefix, postfix, value, maxlength };
     }
 
     render() {
-        const poundSign = (<span>#</span>);
+        const { prefix, postfix, value, maxlength } = this.domainProps;
-        const aliasPostfix = ":" + this.props.domain;
-        const domain = (<span title={aliasPostfix}>{ aliasPostfix }</span>);
-        const maxlength = 255 - this.props.domain.length - 2;   // 2 for # and :
         return (
             <Field
                 label={this.props.label || _t("Room address")}
                 className="mx_RoomAliasField"
-                prefixComponent={poundSign}
+                prefixComponent={prefix}
-                postfixComponent={domain}
+                postfixComponent={postfix}
                 ref={this.fieldRef}
                 onValidate={this.onValidate}
                 placeholder={this.props.placeholder || _t("e.g. my-room")}
                 onChange={this.onChange}
-                value={this.props.value.substring(1, this.props.value.length - this.props.domain.length - 1)}
+                value={value}
                 maxLength={maxlength}
                 disabled={this.props.disabled}
                 autoComplete="off"
@@ -91,16 +107,58 @@ export default class RoomAliasField extends React.PureComponent<IProps, IState>
 
     private validationRules = withValidation({
         rules: [
+            { key: "hasDomain",
+                test: async ({ value }) => {
+                    // Ignore if we have passed domain
+                    if (!value || this.props.domain) {
+                        return true;
+                    }
+
+                    if (value.split(':').length < 2) {
+                        return false;
+                    }
+                    return true;
+                },
+                invalid: () => _t("Missing domain separator e.g. (:domain.org)"),
+            },
+            {
+                key: "hasLocalpart",
+                test: async ({ value }) => {
+                    if (!value || this.props.domain) {
+                        return true;
+                    }
+
+                    const split = value.split(':');
+                    if (split.length < 2) {
+                        return true; // hasDomain check will fail here instead
+                    }
+
+                    // Define the value invalid if there's no first part (roomname)
+                    if (split[0].length < 1) {
+                        return false;
+                    }
+                    return true;
+                },
+                invalid: () => _t("Missing room name or separator e.g. (my-room:domain.org)"),
+            },
             {
                 key: "safeLocalpart",
                 test: async ({ value }) => {
                     if (!value) {
                         return true;
                     }
-                    const fullAlias = this.asFullAlias(value);
+                    if (!this.props.domain) {
-                    // XXX: FIXME https://github.com/matrix-org/matrix-doc/issues/668
+                        return true;
-                    return !value.includes("#") && !value.includes(":") && !value.includes(",") &&
+                    } else {
-                        encodeURI(fullAlias) === fullAlias;
+                        const fullAlias = this.asFullAlias(value);
+                        const hasColon = this.props.domain ? !value.includes(":") : true;
+                        // XXX: FIXME https://github.com/matrix-org/matrix-doc/issues/668
+                        // NOTE: We could probably use linkifyjs to parse those aliases here?
+                        return !value.includes("#") &&
+                            hasColon &&
+                            !value.includes(",") &&
+                            encodeURI(fullAlias) === fullAlias;
+                    }
                 },
                 invalid: () => _t("Some characters not allowed"),
             }, {
@@ -114,12 +172,13 @@ export default class RoomAliasField extends React.PureComponent<IProps, IState>
                     if (!value) {
                         return true;
                     }
-                    const client = MatrixClientPeg.get();
+                    const client = this.context;
                     try {
                         await client.getRoomIdForAlias(this.asFullAlias(value));
                         // we got a room id, so the alias is taken
                         return false;
                     } catch (err) {
+                        console.log(err);
                         // any server error code will do,
                         // either it M_NOT_FOUND or the alias is invalid somehow,
                         // in which case we don't want to show the invalid message
@@ -127,7 +186,9 @@ export default class RoomAliasField extends React.PureComponent<IProps, IState>
                     }
                 },
                 valid: () => _t("This address is available to use"),
-                invalid: () => _t("This address is already in use"),
+                invalid: () => this.props.domain ?
+                    _t("This address is already in use") :
+                    _t("This address had invalid server or is already in use"),
             },
         ],
     });

src/components/views/room_settings/AliasSettings.tsx

@@ -14,12 +14,11 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { ChangeEvent, createRef } from "react";
+import React, { ChangeEvent, ContextType, createRef } from "react";
 import { MatrixEvent } from "matrix-js-sdk/src/models/event";
 import { logger } from "matrix-js-sdk/src/logger";
 
 import EditableItemList from "../elements/EditableItemList";
-import { MatrixClientPeg } from "../../../MatrixClientPeg";
 import { _t } from '../../../languageHandler';
 import Field from "../elements/Field";
 import Spinner from "../elements/Spinner";
@@ -29,6 +28,7 @@ import Modal from "../../../Modal";
 import RoomPublishSetting from "./RoomPublishSetting";
 import { replaceableComponent } from "../../../utils/replaceableComponent";
 import RoomAliasField from "../elements/RoomAliasField";
+import MatrixClientContext from "../../../contexts/MatrixClientContext";
 import SettingsFieldset from "../settings/SettingsFieldset";
 
 interface IEditableAliasesListProps {
@@ -51,11 +51,6 @@ class EditableAliasesList extends EditableItemList<IEditableAliasesListProps> {
     };
 
     protected renderNewItemField() {
-        // if we don't need the RoomAliasField,
-        // we don't need to overriden version of renderNewItemField
-        if (!this.props.domain) {
-            return super.renderNewItemField();
-        }
         const onChange = (alias) => this.onNewItemChanged({ target: { value: alias } });
         return (
             <form
@@ -98,13 +93,16 @@ interface IState {
 
 @replaceableComponent("views.room_settings.AliasSettings")
 export default class AliasSettings extends React.Component<IProps, IState> {
+    public static contextType = MatrixClientContext;
+    context: ContextType<typeof MatrixClientContext>;
+
     static defaultProps = {
         canSetAliases: false,
         canSetCanonicalAlias: false,
     };
 
-    constructor(props) {
+    constructor(props, context: ContextType<typeof MatrixClientContext>) {
-        super(props);
+        super(props, context);
 
         const state = {
             altAliases: [], // [ #alias:domain.tld, ... ]
@@ -138,10 +136,10 @@ export default class AliasSettings extends React.Component<IProps, IState> {
     private async loadLocalAliases() {
         this.setState({ localAliasesLoading: true });
         try {
-            const cli = MatrixClientPeg.get();
+            const mxClient = this.context;
             let localAliases = [];
-            if (await cli.doesServerSupportUnstableFeature("org.matrix.msc2432")) {
+            if (await mxClient.doesServerSupportUnstableFeature("org.matrix.msc2432")) {
-                const response = await cli.unstableGetLocalAliases(this.props.roomId);
+                const response = await mxClient.unstableGetLocalAliases(this.props.roomId);
                 if (Array.isArray(response.aliases)) {
                     localAliases = response.aliases;
                 }
@@ -171,7 +169,7 @@ export default class AliasSettings extends React.Component<IProps, IState> {
 
         if (alias) eventContent["alias"] = alias;
 
-        MatrixClientPeg.get().sendStateEvent(this.props.roomId, "m.room.canonical_alias",
+        this.context.sendStateEvent(this.props.roomId, "m.room.canonical_alias",
             eventContent, "").catch((err) => {
             logger.error(err);
             Modal.createTrackedDialog('Error updating main address', '', ErrorDialog, {
@@ -192,7 +190,6 @@ export default class AliasSettings extends React.Component<IProps, IState> {
 
         this.setState({
             updatingCanonicalAlias: true,
-            altAliases,
         });
 
         const eventContent = {};
@@ -204,19 +201,25 @@ export default class AliasSettings extends React.Component<IProps, IState> {
             eventContent["alt_aliases"] = altAliases;
         }
 
-        MatrixClientPeg.get().sendStateEvent(this.props.roomId, "m.room.canonical_alias",
+        this.context.sendStateEvent(this.props.roomId, "m.room.canonical_alias", eventContent, "")
-            eventContent, "").catch((err) => {
+            .then(() => {
-            logger.error(err);
+                this.setState({
-            Modal.createTrackedDialog('Error updating alternative addresses', '', ErrorDialog, {
+                    altAliases,
-                title: _t("Error updating main address"),
+                });
-                description: _t(
+            })
-                    "There was an error updating the room's alternative addresses. " +
+            .catch((err) => {
+                // TODO: Add error handling based upon server validation
+                logger.error(err);
+                Modal.createTrackedDialog('Error updating alternative addresses', '', ErrorDialog, {
+                    title: _t("Error updating main address"),
+                    description: _t(
+                        "There was an error updating the room's alternative addresses. " +
                     "It may not be allowed by the server or a temporary failure occurred.",
-                ),
+                    ),
+                });
+            }).finally(() => {
+                this.setState({ updatingCanonicalAlias: false });
             });
-        }).finally(() => {
-            this.setState({ updatingCanonicalAlias: false });
-        });
     }
 
     private onNewAliasChanged = (value: string) => {
@@ -226,10 +229,10 @@ export default class AliasSettings extends React.Component<IProps, IState> {
     private onLocalAliasAdded = (alias: string) => {
         if (!alias || alias.length === 0) return; // ignore attempts to create blank aliases
 
-        const localDomain = MatrixClientPeg.get().getDomain();
+        const localDomain = this.context.getDomain();
         if (!alias.includes(':')) alias += ':' + localDomain;
 
-        MatrixClientPeg.get().createAlias(alias, this.props.roomId).then(() => {
+        this.context.createAlias(alias, this.props.roomId).then(() => {
             this.setState({
                 localAliases: this.state.localAliases.concat(alias),
                 newAlias: null,
@@ -253,7 +256,7 @@ export default class AliasSettings extends React.Component<IProps, IState> {
         const alias = this.state.localAliases[index];
         // TODO: In future, we should probably be making sure that the alias actually belongs
         // to this room. See https://github.com/vector-im/element-web/issues/7353
-        MatrixClientPeg.get().deleteAlias(alias).then(() => {
+        this.context.deleteAlias(alias).then(() => {
             const localAliases = this.state.localAliases.filter(a => a !== alias);
             this.setState({ localAliases });
 
@@ -322,9 +325,9 @@ export default class AliasSettings extends React.Component<IProps, IState> {
     }
 
     render() {
-        const cli = MatrixClientPeg.get();
+        const mxClient = this.context;
-        const localDomain = cli.getDomain();
+        const localDomain = mxClient.getDomain();
-        const isSpaceRoom = cli.getRoom(this.props.roomId)?.isSpaceRoom();
+        const isSpaceRoom = mxClient.getRoom(this.props.roomId)?.isSpaceRoom();
 
         let found = false;
         const canonicalValue = this.state.canonicalAlias || "";

src/i18n/strings/en_EN.json

@@ -2280,10 +2280,13 @@
     "In reply to <a>this message</a>": "In reply to <a>this message</a>",
     "Room address": "Room address",
     "e.g. my-room": "e.g. my-room",
+    "Missing domain separator e.g. (:domain.org)": "Missing domain separator e.g. (:domain.org)",
+    "Missing room name or separator e.g. (my-room:domain.org)": "Missing room name or separator e.g. (my-room:domain.org)",
     "Some characters not allowed": "Some characters not allowed",
     "Please provide an address": "Please provide an address",
     "This address is available to use": "This address is available to use",
     "This address is already in use": "This address is already in use",
+    "This address had invalid server or is already in use": "This address had invalid server or is already in use",
     "Server Options": "Server Options",
     "You can use the custom server options to sign into other Matrix servers by specifying a different homeserver URL. This allows you to use Element with an existing Matrix account on a different homeserver.": "You can use the custom server options to sign into other Matrix servers by specifying a different homeserver URL. This allows you to use Element with an existing Matrix account on a different homeserver.",
     "Join millions for free on the largest public server": "Join millions for free on the largest public server",