From 32ece817e1ebd4b748f0ebbf41036ba5a06dacd2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 14 Apr 2023 21:30:22 -0500 Subject: [PATCH 01/10] Properly translate errors in `ChangePassword.tsx` So they show up translated to the user but not in our logs. Part of https://github.com/vector-im/element-web/issues/9597 and also fixes it since it's the last piece mentioned (there could be other cases we log translated strings) Fix https://github.com/vector-im/element-web/issues/9597 --- .../views/settings/ChangePassword.tsx | 41 ++++++++++++------- .../tabs/user/GeneralUserSettingsTab.tsx | 33 +++++++++------ 2 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 9978985ad87..3b38ce9cf23 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -24,7 +24,7 @@ import { MatrixClientPeg } from "../../../MatrixClientPeg"; import AccessibleButton from "../elements/AccessibleButton"; import Spinner from "../elements/Spinner"; import withValidation, { IFieldState, IValidationResult } from "../elements/Validation"; -import { _t, _td } from "../../../languageHandler"; +import { UserFriendlyError, _t, _td } from "../../../languageHandler"; import Modal from "../../../Modal"; import PassphraseField from "../auth/PassphraseField"; import { PASSWORD_MIN_SCORE } from "../auth/RegistrationForm"; @@ -48,7 +48,7 @@ interface IProps { /** Was one or more other devices logged out whilst changing the password */ didLogoutOutOtherDevices: boolean; }) => void; - onError: (error: { error: string }) => void; + onError: (error: Error) => void; rowClassName?: string; buttonClassName?: string; buttonKind?: string; @@ -183,7 +183,16 @@ export default class ChangePassword extends React.Component { } }, (err) => { - this.props.onError(err); + if (err instanceof Error) { + this.props.onError(err); + } else { + this.props.onError( + new UserFriendlyError("Error while setting password: %(error)s", { + error: String(err), + cause: undefined, + }), + ); + } }, ) .finally(() => { @@ -196,15 +205,19 @@ export default class ChangePassword extends React.Component { }); } - private checkPassword(oldPass: string, newPass: string, confirmPass: string): { error: string } | undefined { + /** + * Checks the `newPass` and throws an error if it is unacceptable. + * @param oldPass The old password + * @param newPass The new password that the user is trying to be set + * @param confirmPass The confirmation password where the user types the `newPass` + * again for confirmation and should match the `newPass` before we accept their new + * password. + */ + private checkPassword(oldPass: string, newPass: string, confirmPass: string): void { if (newPass !== confirmPass) { - return { - error: _t("New passwords don't match"), - }; + throw new UserFriendlyError("New passwords don't match"); } else if (!newPass || newPass.length === 0) { - return { - error: _t("Passwords can't be empty"), - }; + throw new UserFriendlyError("Passwords can't be empty"); } } @@ -307,11 +320,11 @@ export default class ChangePassword extends React.Component { const oldPassword = this.state.oldPassword; const newPassword = this.state.newPassword; const confirmPassword = this.state.newPasswordConfirm; - const err = this.checkPassword(oldPassword, newPassword, confirmPassword); - if (err) { - this.props.onError(err); - } else { + try { + this.checkPassword(oldPassword, newPassword, confirmPassword); return this.onChangePassword(oldPassword, newPassword); + } catch (err) { + this.props.onError(err); } }; diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index dc3bf9f408b..3a1e80986ce 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -21,7 +21,7 @@ import { SERVICE_TYPES } from "matrix-js-sdk/src/service-types"; import { IThreepid } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { IDelegatedAuthConfig, M_AUTHENTICATION } from "matrix-js-sdk/src/matrix"; -import { MatrixError } from "matrix-js-sdk/src/matrix"; +import { HTTPError } from "matrix-js-sdk/src/matrix"; import { _t } from "../../../../../languageHandler"; import ProfileSettings from "../../ProfileSettings"; @@ -43,7 +43,7 @@ import Spinner from "../../../elements/Spinner"; import { SettingLevel } from "../../../../../settings/SettingLevel"; import { UIFeature } from "../../../../../settings/UIFeature"; import { ActionPayload } from "../../../../../dispatcher/payloads"; -import ErrorDialog from "../../../dialogs/ErrorDialog"; +import ErrorDialog, { extractErrorMessageFromError } from "../../../dialogs/ErrorDialog"; import AccountPhoneNumbers from "../../account/PhoneNumbers"; import AccountEmailAddresses from "../../account/EmailAddresses"; import DiscoveryEmailAddresses from "../../discovery/EmailAddresses"; @@ -253,18 +253,27 @@ export default class GeneralUserSettingsTab extends React.Component { - // TODO: Figure out a design that doesn't involve replacing the current dialog - let errMsg = err.error || err.message || ""; - if (err.httpStatus === 403) { - errMsg = _t("Failed to change password. Is your password correct?"); - } else if (!errMsg) { - errMsg += ` (HTTP status ${err.httpStatus})`; + private onPasswordChangeError = (err: Error): void => { + logger.error("Failed to change password: " + err); + + const errorMessage = extractErrorMessageFromError(err, _t("Unknown password change error")); + + let errorMessageToDisplay; + if (err instanceof HTTPError && err.httpStatus === 403) { + errorMessageToDisplay = _t("Failed to change password. Is your password correct?"); + } else if (err instanceof HTTPError) { + errorMessageToDisplay = _t("%(errorMessage)s (HTTP status %(httpStatus)s)", { + errorMessage, + httpStatus: err.httpStatus, + }); + } else { + errorMessageToDisplay = errorMessage; } - logger.error("Failed to change password: " + errMsg); + + // TODO: Figure out a design that doesn't involve replacing the current dialog Modal.createDialog(ErrorDialog, { - title: _t("Error"), - description: errMsg, + title: _t("Error changing password"), + description: errorMessageToDisplay, }); }; From 07a0a74c6f7fe1b292e55813d6fd86b5166bd100 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 14 Apr 2023 21:41:10 -0500 Subject: [PATCH 02/10] Make more useful --- .../tabs/user/GeneralUserSettingsTab.tsx | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index 3a1e80986ce..da9cd92f004 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -23,7 +23,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { IDelegatedAuthConfig, M_AUTHENTICATION } from "matrix-js-sdk/src/matrix"; import { HTTPError } from "matrix-js-sdk/src/matrix"; -import { _t } from "../../../../../languageHandler"; +import { UserFriendlyError, _t } from "../../../../../languageHandler"; import ProfileSettings from "../../ProfileSettings"; import * as languageHandler from "../../../../../languageHandler"; import SettingsStore from "../../../../../settings/SettingsStore"; @@ -256,15 +256,25 @@ export default class GeneralUserSettingsTab extends React.Component { logger.error("Failed to change password: " + err); - const errorMessage = extractErrorMessageFromError(err, _t("Unknown password change error")); + let underlyingError = err; + if (err instanceof UserFriendlyError && err.cause instanceof Error) { + underlyingError = err.cause; + } + + const errorMessage = extractErrorMessageFromError( + err, + _t("Unknown password change error (%(stringifiedError)s)", { + stringifiedError: String(err), + }), + ); let errorMessageToDisplay; - if (err instanceof HTTPError && err.httpStatus === 403) { + if (underlyingError instanceof HTTPError && underlyingError.httpStatus === 403) { errorMessageToDisplay = _t("Failed to change password. Is your password correct?"); - } else if (err instanceof HTTPError) { + } else if (underlyingError instanceof HTTPError) { errorMessageToDisplay = _t("%(errorMessage)s (HTTP status %(httpStatus)s)", { errorMessage, - httpStatus: err.httpStatus, + httpStatus: underlyingError.httpStatus, }); } else { errorMessageToDisplay = errorMessage; From c62c6c990bf07c3eefc4526c8a40f613721a6ac2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 14 Apr 2023 21:46:55 -0500 Subject: [PATCH 03/10] Update i18n strings --- src/i18n/strings/en_EN.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 5f6ced12911..2928f9fac31 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1343,6 +1343,7 @@ "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.": "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.", "You can also ask your homeserver admin to upgrade the server to change this behaviour.": "You can also ask your homeserver admin to upgrade the server to change this behaviour.", "Export E2E room keys": "Export E2E room keys", + "Error while setting password: %(error)s": "Error while setting password: %(error)s", "New passwords don't match": "New passwords don't match", "Passwords can't be empty": "Passwords can't be empty", "Do you want to set an email address?": "Do you want to set an email address?", @@ -1545,7 +1546,10 @@ "Set the name of a font installed on your system & %(brand)s will attempt to use it.": "Set the name of a font installed on your system & %(brand)s will attempt to use it.", "Customise your appearance": "Customise your appearance", "Appearance Settings only affect this %(brand)s session.": "Appearance Settings only affect this %(brand)s session.", + "Unknown password change error (%(stringifiedError)s)": "Unknown password change error (%(stringifiedError)s)", "Failed to change password. Is your password correct?": "Failed to change password. Is your password correct?", + "%(errorMessage)s (HTTP status %(httpStatus)s)": "%(errorMessage)s (HTTP status %(httpStatus)s)", + "Error changing password": "Error changing password", "Your password was successfully changed.": "Your password was successfully changed.", "You will not receive push notifications on other devices until you sign back in to them.": "You will not receive push notifications on other devices until you sign back in to them.", "Success": "Success", From 7786dd151028e6fbf04d1a38a9c2cd47a3fbfc4b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 14 Apr 2023 22:06:01 -0500 Subject: [PATCH 04/10] No need to checkPassword since field validation already covers this See https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1167363765 Both of the error cases are covered by the logic in `verifyFieldsBeforeSubmit()` just above and there is no way `checkPassword` would ever throw one of these errors since they are already valid by the time it reaches here. --- .../views/settings/ChangePassword.tsx | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 3b38ce9cf23..65bee87e679 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -205,22 +205,6 @@ export default class ChangePassword extends React.Component { }); } - /** - * Checks the `newPass` and throws an error if it is unacceptable. - * @param oldPass The old password - * @param newPass The new password that the user is trying to be set - * @param confirmPass The confirmation password where the user types the `newPass` - * again for confirmation and should match the `newPass` before we accept their new - * password. - */ - private checkPassword(oldPass: string, newPass: string, confirmPass: string): void { - if (newPass !== confirmPass) { - throw new UserFriendlyError("New passwords don't match"); - } else if (!newPass || newPass.length === 0) { - throw new UserFriendlyError("Passwords can't be empty"); - } - } - private optionallySetEmail(): Promise { // Ask for an email otherwise the user has no way to reset their password const modal = Modal.createDialog(SetEmailDialog, { @@ -319,13 +303,7 @@ export default class ChangePassword extends React.Component { const oldPassword = this.state.oldPassword; const newPassword = this.state.newPassword; - const confirmPassword = this.state.newPasswordConfirm; - try { - this.checkPassword(oldPassword, newPassword, confirmPassword); - return this.onChangePassword(oldPassword, newPassword); - } catch (err) { - this.props.onError(err); - } + return this.onChangePassword(oldPassword, newPassword); }; private async verifyFieldsBeforeSubmit(): Promise { From a7c2fe47bcf99c987193aa944a951ac7485ec83b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 14 Apr 2023 22:10:25 -0500 Subject: [PATCH 05/10] Update i18n strings --- src/i18n/strings/en_EN.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 2928f9fac31..ee56afd58f9 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1344,9 +1344,8 @@ "You can also ask your homeserver admin to upgrade the server to change this behaviour.": "You can also ask your homeserver admin to upgrade the server to change this behaviour.", "Export E2E room keys": "Export E2E room keys", "Error while setting password: %(error)s": "Error while setting password: %(error)s", - "New passwords don't match": "New passwords don't match", - "Passwords can't be empty": "Passwords can't be empty", "Do you want to set an email address?": "Do you want to set an email address?", + "Passwords can't be empty": "Passwords can't be empty", "Confirm password": "Confirm password", "Passwords don't match": "Passwords don't match", "Current password": "Current password", From 329b8683d90c8eed6c54be87f79b70f4b5bbbcbf Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 20 Apr 2023 14:18:24 -0500 Subject: [PATCH 06/10] Revert "No need to checkPassword since field validation already covers this" This reverts commit 7786dd151028e6fbf04d1a38a9c2cd47a3fbfc4b. --- .../views/settings/ChangePassword.tsx | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 65bee87e679..3b38ce9cf23 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -205,6 +205,22 @@ export default class ChangePassword extends React.Component { }); } + /** + * Checks the `newPass` and throws an error if it is unacceptable. + * @param oldPass The old password + * @param newPass The new password that the user is trying to be set + * @param confirmPass The confirmation password where the user types the `newPass` + * again for confirmation and should match the `newPass` before we accept their new + * password. + */ + private checkPassword(oldPass: string, newPass: string, confirmPass: string): void { + if (newPass !== confirmPass) { + throw new UserFriendlyError("New passwords don't match"); + } else if (!newPass || newPass.length === 0) { + throw new UserFriendlyError("Passwords can't be empty"); + } + } + private optionallySetEmail(): Promise { // Ask for an email otherwise the user has no way to reset their password const modal = Modal.createDialog(SetEmailDialog, { @@ -303,7 +319,13 @@ export default class ChangePassword extends React.Component { const oldPassword = this.state.oldPassword; const newPassword = this.state.newPassword; - return this.onChangePassword(oldPassword, newPassword); + const confirmPassword = this.state.newPasswordConfirm; + try { + this.checkPassword(oldPassword, newPassword, confirmPassword); + return this.onChangePassword(oldPassword, newPassword); + } catch (err) { + this.props.onError(err); + } }; private async verifyFieldsBeforeSubmit(): Promise { From 93f238b9d65b5dc00c32ac2bd7a9ec0dc78ad712 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 20 Apr 2023 14:18:50 -0500 Subject: [PATCH 07/10] Update i18n strings --- src/i18n/strings/en_EN.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index ee56afd58f9..2928f9fac31 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1344,8 +1344,9 @@ "You can also ask your homeserver admin to upgrade the server to change this behaviour.": "You can also ask your homeserver admin to upgrade the server to change this behaviour.", "Export E2E room keys": "Export E2E room keys", "Error while setting password: %(error)s": "Error while setting password: %(error)s", - "Do you want to set an email address?": "Do you want to set an email address?", + "New passwords don't match": "New passwords don't match", "Passwords can't be empty": "Passwords can't be empty", + "Do you want to set an email address?": "Do you want to set an email address?", "Confirm password": "Confirm password", "Passwords don't match": "Passwords don't match", "Current password": "Current password", From e0fbd73270b09acaac9f68924ad03023e277cfd6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 20 Apr 2023 14:23:22 -0500 Subject: [PATCH 08/10] Add todo context to note that we can remove this logic in the future --- src/components/views/settings/ChangePassword.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 3b38ce9cf23..734f8a4b0ba 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -321,6 +321,10 @@ export default class ChangePassword extends React.Component { const newPassword = this.state.newPassword; const confirmPassword = this.state.newPasswordConfirm; try { + // TODO: We can remove this check (but should add some Cypress tests to + // sanity check this flow). This logic is redundant with the input field + // validation we do and `verifyFieldsBeforeSubmit()` above. See + // https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1167364214 this.checkPassword(oldPassword, newPassword, confirmPassword); return this.onChangePassword(oldPassword, newPassword); } catch (err) { From 1f8bd91d0207cdb586a3f846d9bb0fe8f1861ed8 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 20 Apr 2023 14:33:34 -0500 Subject: [PATCH 09/10] Ensure is an error --- src/components/views/settings/ChangePassword.tsx | 13 +++++++++++-- src/i18n/strings/en_EN.json | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 734f8a4b0ba..1e2e96b33db 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -187,7 +187,7 @@ export default class ChangePassword extends React.Component { this.props.onError(err); } else { this.props.onError( - new UserFriendlyError("Error while setting password: %(error)s", { + new UserFriendlyError("Error while changing password: %(error)s", { error: String(err), cause: undefined, }), @@ -328,7 +328,16 @@ export default class ChangePassword extends React.Component { this.checkPassword(oldPassword, newPassword, confirmPassword); return this.onChangePassword(oldPassword, newPassword); } catch (err) { - this.props.onError(err); + if (err instanceof Error) { + this.props.onError(err); + } else { + this.props.onError( + new UserFriendlyError("Error while changing password: %(error)s", { + error: String(err), + cause: undefined, + }), + ); + } } }; diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 2928f9fac31..056d272471e 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1343,7 +1343,7 @@ "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.": "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.", "You can also ask your homeserver admin to upgrade the server to change this behaviour.": "You can also ask your homeserver admin to upgrade the server to change this behaviour.", "Export E2E room keys": "Export E2E room keys", - "Error while setting password: %(error)s": "Error while setting password: %(error)s", + "Error while changing password: %(error)s": "Error while changing password: %(error)s", "New passwords don't match": "New passwords don't match", "Passwords can't be empty": "Passwords can't be empty", "Do you want to set an email address?": "Do you want to set an email address?", From 798bb1928e8bdeaaa2b04af51d12fdf9a36f34d5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 21 Apr 2023 11:41:03 -0500 Subject: [PATCH 10/10] Remove else See https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1173477053 --- .../views/settings/tabs/user/GeneralUserSettingsTab.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index 9244688abf7..8c643ff0fb5 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -268,7 +268,7 @@ export default class GeneralUserSettingsTab extends React.Component