From 30aefc267936fbb3e882976d4e17dae1b457398a Mon Sep 17 00:00:00 2001 From: Dariusz Niemczyk Date: Wed, 10 Nov 2021 14:02:12 +0100 Subject: [PATCH 1/5] Refactor MatrixClientPeg to use ReactContext Related: https://github.com/vector-im/element-web/issues/19609 --- .../views/room_settings/AliasSettings.tsx | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/components/views/room_settings/AliasSettings.tsx b/src/components/views/room_settings/AliasSettings.tsx index 4666ca9ef7e..d52e4b97428 100644 --- a/src/components/views/room_settings/AliasSettings.tsx +++ b/src/components/views/room_settings/AliasSettings.tsx @@ -14,11 +14,10 @@ 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 EditableItemList from "../elements/EditableItemList"; -import { MatrixClientPeg } from "../../../MatrixClientPeg"; import { _t } from '../../../languageHandler'; import Field from "../elements/Field"; import Spinner from "../elements/Spinner"; @@ -28,6 +27,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 { logger } from "matrix-js-sdk/src/logger"; @@ -98,13 +98,16 @@ interface IState { @replaceableComponent("views.room_settings.AliasSettings") export default class AliasSettings extends React.Component { + public static contextType = MatrixClientContext; + context: ContextType; + static defaultProps = { canSetAliases: false, canSetCanonicalAlias: false, }; - constructor(props) { - super(props); + constructor(props, context: ContextType) { + super(props, context); const state = { altAliases: [], // [ #alias:domain.tld, ... ] @@ -138,10 +141,10 @@ export default class AliasSettings extends React.Component { 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")) { - const response = await cli.unstableGetLocalAliases(this.props.roomId); + if (await mxClient.doesServerSupportUnstableFeature("org.matrix.msc2432")) { + const response = await mxClient.unstableGetLocalAliases(this.props.roomId); if (Array.isArray(response.aliases)) { localAliases = response.aliases; } @@ -171,7 +174,7 @@ export default class AliasSettings extends React.Component { 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 +195,6 @@ export default class AliasSettings extends React.Component { this.setState({ updatingCanonicalAlias: true, - altAliases, }); const eventContent = {}; @@ -204,7 +206,8 @@ export default class AliasSettings extends React.Component { eventContent["alt_aliases"] = altAliases; } - MatrixClientPeg.get().sendStateEvent(this.props.roomId, "m.room.canonical_alias", + this.context.sendStateEvent(this.props.roomId, "m.room.canonical_alias", + this.context.mxClient.sendStateEvent(this.props.roomId, "m.room.canonical_alias", eventContent, "").catch((err) => { logger.error(err); Modal.createTrackedDialog('Error updating alternative addresses', '', ErrorDialog, { @@ -226,10 +229,10 @@ export default class AliasSettings extends React.Component { 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 { 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 { } render() { - const cli = MatrixClientPeg.get(); - const localDomain = cli.getDomain(); - const isSpaceRoom = cli.getRoom(this.props.roomId)?.isSpaceRoom(); + const mxClient = this.context; + const localDomain = mxClient.getDomain(); + const isSpaceRoom = mxClient.getRoom(this.props.roomId)?.isSpaceRoom(); let found = false; const canonicalValue = this.state.canonicalAlias || ""; From 81b35125a49084017b36d9cff1559821c320f699 Mon Sep 17 00:00:00 2001 From: Dariusz Niemczyk Date: Wed, 10 Nov 2021 14:08:19 +0100 Subject: [PATCH 2/5] Fix invalid validation for room aliases Fixes https://github.com/vector-im/element-web/issues/19609 --- .../views/room_settings/AliasSettings.tsx | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/components/views/room_settings/AliasSettings.tsx b/src/components/views/room_settings/AliasSettings.tsx index d52e4b97428..66c04ed5b03 100644 --- a/src/components/views/room_settings/AliasSettings.tsx +++ b/src/components/views/room_settings/AliasSettings.tsx @@ -206,20 +206,25 @@ export default class AliasSettings extends React.Component { eventContent["alt_aliases"] = altAliases; } - this.context.sendStateEvent(this.props.roomId, "m.room.canonical_alias", - this.context.mxClient.sendStateEvent(this.props.roomId, "m.room.canonical_alias", - eventContent, "").catch((err) => { - 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. " + + this.context.sendStateEvent(this.props.roomId, "m.room.canonical_alias", eventContent, "") + .then(() => { + this.setState({ + altAliases, + }); + }) + .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) => { From 5907d8829fc7586637c50299e573271fe1ff1b2b Mon Sep 17 00:00:00 2001 From: Dariusz Niemczyk Date: Tue, 30 Nov 2021 21:38:50 +0100 Subject: [PATCH 3/5] Add error handling for aliases --- .../views/elements/RoomAliasField.tsx | 92 +++++++++++++++---- .../views/room_settings/AliasSettings.tsx | 5 - 2 files changed, 76 insertions(+), 21 deletions(-) diff --git a/src/components/views/elements/RoomAliasField.tsx b/src/components/views/elements/RoomAliasField.tsx index 9c383989b11..f7b46302d13 100644 --- a/src/components/views/elements/RoomAliasField.tsx +++ b/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 { + static contextType = MatrixClientContext; + public context!: React.ContextType; + private fieldRef = createRef(); constructor(props, context) { @@ -50,25 +53,38 @@ export default class RoomAliasField extends React.PureComponent } 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 = #; + const postfix = domain ? ({ `:${domain}` }) : ; + 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 = (#); - const aliasPostfix = ":" + this.props.domain; - const domain = ({ aliasPostfix }); - const maxlength = 255 - this.props.domain.length - 2; // 2 for # and : + const { prefix, postfix, value, maxlength } = this.domainProps; return ( private validationRules = withValidation({ rules: [ + { key: "hasSingleDomain", + test: async ({ value }) => { + if (!value) { + return true; + } + // Ignore if we have passed domain + if (this.props.domain) { + return true; + } + const split = value.split(':'); + if (split.length < 2) { + return false; + } + return true; + }, + invalid: () => _t("Missing domain separator e.g. (:domain.org)"), + }, + { + key: "hasMultipleDomains", + test: async ({ value }) => { + if (!value) { + return true; + } + // Ignore if we have passed domain + if (this.props.domain) { + return true; + } + const split = value.split(':'); + if (split.length > 2) { + return false; + } + return true; + }, + invalid: () => _t("Multiple domain separators (:) provided, provide an alias with a single domain."), + }, { key: "safeLocalpart", test: async ({ value }) => { if (!value) { return true; } - const fullAlias = this.asFullAlias(value); - // XXX: FIXME https://github.com/matrix-org/matrix-doc/issues/668 - return !value.includes("#") && !value.includes(":") && !value.includes(",") && - encodeURI(fullAlias) === fullAlias; + if (!this.props.domain) { + return true; + } else { + const fullAlias = this.asFullAlias(value); + // XXX: FIXME https://github.com/matrix-org/matrix-doc/issues/668 + return !value.includes("#") && + this.props.domain ? !value.includes(":") : true && + !value.includes(",") && + encodeURI(fullAlias) === fullAlias; + } }, invalid: () => _t("Some characters not allowed"), }, { @@ -114,12 +171,13 @@ export default class RoomAliasField extends React.PureComponent 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 +185,9 @@ export default class RoomAliasField extends React.PureComponent } }, 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"), }, ], }); diff --git a/src/components/views/room_settings/AliasSettings.tsx b/src/components/views/room_settings/AliasSettings.tsx index 66c04ed5b03..febb0fd76b7 100644 --- a/src/components/views/room_settings/AliasSettings.tsx +++ b/src/components/views/room_settings/AliasSettings.tsx @@ -51,11 +51,6 @@ class EditableAliasesList extends EditableItemList { }; 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 (
Date: Thu, 2 Dec 2021 11:16:58 +0100 Subject: [PATCH 4/5] Fix PR comments --- src/components/views/elements/RoomAliasField.tsx | 15 +++++++++------ src/i18n/strings/en_EN.json | 3 +++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/components/views/elements/RoomAliasField.tsx b/src/components/views/elements/RoomAliasField.tsx index f7b46302d13..3a9ec0a8e04 100644 --- a/src/components/views/elements/RoomAliasField.tsx +++ b/src/components/views/elements/RoomAliasField.tsx @@ -107,7 +107,7 @@ export default class RoomAliasField extends React.PureComponent private validationRules = withValidation({ rules: [ - { key: "hasSingleDomain", + { key: "hasDomain", test: async ({ value }) => { if (!value) { return true; @@ -125,7 +125,7 @@ export default class RoomAliasField extends React.PureComponent invalid: () => _t("Missing domain separator e.g. (:domain.org)"), }, { - key: "hasMultipleDomains", + key: "hasRoomnameOrAtLeastASingleSeparator", test: async ({ value }) => { if (!value) { return true; @@ -135,12 +135,13 @@ export default class RoomAliasField extends React.PureComponent return true; } const split = value.split(':'); - if (split.length > 2) { + // Define the value invalid if there's no first part (roomname) or separator's missing + if (split.length < 2 || split[0].length < 1) { return false; } return true; }, - invalid: () => _t("Multiple domain separators (:) provided, provide an alias with a single domain."), + invalid: () => _t("Missing room name or separator e.g. (my-room:domain.org)"), }, { key: "safeLocalpart", @@ -152,10 +153,12 @@ export default class RoomAliasField extends React.PureComponent return true; } else { 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("#") && - this.props.domain ? !value.includes(":") : true && - !value.includes(",") && + hasColon && + !value.includes(",") && encodeURI(fullAlias) === fullAlias; } }, diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index facd3999dda..6b2f5293c57 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -2231,10 +2231,13 @@ "In reply to this message": "In reply to this message", "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", From 55d9e5596082290d13710aa11835405bc16fdb7d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 21 Dec 2021 14:31:17 +0000 Subject: [PATCH 5/5] Address concerns from review --- .../views/elements/RoomAliasField.tsx | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/components/views/elements/RoomAliasField.tsx b/src/components/views/elements/RoomAliasField.tsx index 3a9ec0a8e04..8e82f20a095 100644 --- a/src/components/views/elements/RoomAliasField.tsx +++ b/src/components/views/elements/RoomAliasField.tsx @@ -109,15 +109,12 @@ export default class RoomAliasField extends React.PureComponent rules: [ { key: "hasDomain", test: async ({ value }) => { - if (!value) { - return true; - } // Ignore if we have passed domain - if (this.props.domain) { + if (!value || this.props.domain) { return true; } - const split = value.split(':'); - if (split.length < 2) { + + if (value.split(':').length < 2) { return false; } return true; @@ -125,18 +122,19 @@ export default class RoomAliasField extends React.PureComponent invalid: () => _t("Missing domain separator e.g. (:domain.org)"), }, { - key: "hasRoomnameOrAtLeastASingleSeparator", + key: "hasLocalpart", test: async ({ value }) => { - if (!value) { - return true; - } - // Ignore if we have passed domain - if (this.props.domain) { + if (!value || this.props.domain) { return true; } + const split = value.split(':'); - // Define the value invalid if there's no first part (roomname) or separator's missing - if (split.length < 2 || split[0].length < 1) { + 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;