Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Properly translate errors in AddThreepid.ts so they show up translated to the user but not in our logs #10432

Merged
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
72c08a1
Properly translate errors in AddThreepid.ts
MadLittleMods Mar 22, 2023
777a4fc
Use translated message
MadLittleMods Mar 22, 2023
167150d
Avoid returning undefined ever
MadLittleMods Mar 22, 2023
4492ecc
More usage
MadLittleMods Mar 22, 2023
8c24afb
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Mar 23, 2023
e57a11c
Introduce UserFriendlyError
MadLittleMods Mar 24, 2023
ebbb455
Use UserFriendlyError
MadLittleMods Mar 24, 2023
b3d37fb
Add more usage instead of normal error
MadLittleMods Mar 24, 2023
cf0ba20
Use types and translatedMessage
MadLittleMods Mar 24, 2023
e557019
Fix lints
MadLittleMods Mar 24, 2023
e6f053a
Update i18n although it's wrong
MadLittleMods Mar 24, 2023
8152c6b
Use unknown for easier creation from try/catch
MadLittleMods Mar 24, 2023
d88d5f2
Use types
MadLittleMods Mar 24, 2023
6e21c16
Use error types
MadLittleMods Mar 24, 2023
078e24b
Use types
MadLittleMods Mar 24, 2023
924416c
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Mar 31, 2023
1cd8ec0
Update i18n strings
MadLittleMods Mar 31, 2023
306a1dc
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 3, 2023
ac02179
Remove generic re-label of HTTPError
MadLittleMods Apr 3, 2023
8675464
Make error message extraction generic
MadLittleMods Apr 3, 2023
529212c
Update i18n strings
MadLittleMods Apr 3, 2023
edac565
Add tests for email addresses
MadLittleMods Apr 4, 2023
477db79
More consistent error logging to actually see error in logs
MadLittleMods Apr 4, 2023
629f429
Consistent error handling
MadLittleMods Apr 4, 2023
c2080ce
Any is okay because we have a fallback
MadLittleMods Apr 4, 2023
8ffeb6f
Check error type
MadLittleMods Apr 4, 2023
1b54ba6
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 5, 2023
96ba896
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 5, 2023
f8164d3
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 6, 2023
016341a
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 7, 2023
5d74ef9
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 10, 2023
ac67f1f
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 11, 2023
251df4d
Use dedicated mockResolvedValue function
MadLittleMods Apr 11, 2023
ae8bdaf
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 12, 2023
817190c
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 13, 2023
ed00c59
Merge branch 'develop' into madlittlemods/9597-properly-translated-er…
MadLittleMods Apr 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 83 additions & 82 deletions src/AddThreepid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,21 @@ limitations under the License.
*/
Copy link
Contributor Author

@MadLittleMods MadLittleMods Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


import { IAuthData, IRequestMsisdnTokenResponse, IRequestTokenResponse } from "matrix-js-sdk/src/matrix";
import { MatrixError, HTTPError } from "matrix-js-sdk/src/matrix";

import { MatrixClientPeg } from "./MatrixClientPeg";
import Modal from "./Modal";
import { _t } from "./languageHandler";
import { _t, UserFriendlyError } from "./languageHandler";
import IdentityAuthClient from "./IdentityAuthClient";
import { SSOAuthEntry } from "./components/views/auth/InteractiveAuthEntryComponents";
import InteractiveAuthDialog from "./components/views/dialogs/InteractiveAuthDialog";

function getIdServerDomain(): string {
const idBaseUrl = MatrixClientPeg.get().idBaseUrl;
const idBaseUrl = MatrixClientPeg.get().getIdentityServerUrl(true);
if (!idBaseUrl) {
throw new Error("Identity server not set");
throw new UserFriendlyError("Identity server not set");
}
return idBaseUrl.split("://")[1];
return idBaseUrl;
}

export type Binding = {
Expand Down Expand Up @@ -67,23 +68,18 @@ export default class AddThreepid {
* @param {string} emailAddress The email address to add
* @return {Promise} Resolves when the email has been sent. Then call checkEmailLinkClicked().
*/
public addEmailAddress(emailAddress: string): Promise<IRequestTokenResponse> {
return MatrixClientPeg.get()
.requestAdd3pidEmailToken(emailAddress, this.clientSecret, 1)
.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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit of logging seems to get lost, is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see #10432 (comment)

I don't think we need to re-label a HTTPError as they already have a good message that looks like Server returned ${response.status} error: ${body} and the main part of the error isn't translated regardless (only the word "Status" 😄)

Probably best to just remove in favor of thinking about a translations in general from the matrix-js-sdk, see matrix-org/matrix-js-sdk#1309

err.message = err.message + ` (Status ${err.httpStatus})`;
}
throw err;
},
);
public async addEmailAddress(emailAddress: string): Promise<IRequestTokenResponse> {
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just getting some async/await treatment with some UserFriendlyError

try {
const res = await MatrixClientPeg.get().requestAdd3pidEmailToken(emailAddress, this.clientSecret, 1);
this.sessionId = res.sid;
return res;
} catch (err) {
if (err instanceof MatrixError && err.errcode === "M_THREEPID_IN_USE") {
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the nature of unifying our error handling, we're going to have "duplicate lines" that SonarCloud is calling out. Need an exception here.

I've already de-duplicated some logic with extractErrorMessageFromError(...). Anything else we want to generalize? Perhaps the underlyingError pattern? I'm inclined to just keep things simple though.

throw new UserFriendlyError("This email address is already in use", { cause: err });
}
// Otherwise, just blurt out the same error
throw err;
}
}

/**
Expand All @@ -98,22 +94,23 @@ export default class AddThreepid {
// For separate bind, request a token directly from the IS.
const authClient = new IdentityAuthClient();
const identityAccessToken = (await authClient.getAccessToken()) ?? undefined;
return MatrixClientPeg.get()
.requestEmailToken(emailAddress, this.clientSecret, 1, 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;
},
try {
const res = await MatrixClientPeg.get().requestEmailToken(
emailAddress,
this.clientSecret,
1,
undefined,
identityAccessToken,
);
this.sessionId = res.sid;
return res;
} catch (err) {
if (err instanceof MatrixError && err.errcode === "M_THREEPID_IN_USE") {
throw new UserFriendlyError("This email address is already in use", { cause: err });
}
// Otherwise, just blurt out the same error
throw err;
}
} else {
// For tangled bind, request a token via the HS.
return this.addEmailAddress(emailAddress);
Expand All @@ -127,24 +124,24 @@ 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().
*/
public addMsisdn(phoneCountry: string, phoneNumber: string): Promise<IRequestMsisdnTokenResponse> {
return MatrixClientPeg.get()
.requestAdd3pidMsisdnToken(phoneCountry, phoneNumber, this.clientSecret, 1)
.then(
(res) => {
this.sessionId = res.sid;
this.submitUrl = res.submit_url;
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;
},
public async addMsisdn(phoneCountry: string, phoneNumber: string): Promise<IRequestMsisdnTokenResponse> {
try {
const res = await MatrixClientPeg.get().requestAdd3pidMsisdnToken(
phoneCountry,
phoneNumber,
this.clientSecret,
1,
);
this.sessionId = res.sid;
this.submitUrl = res.submit_url;
return res;
} catch (err) {
if (err instanceof MatrixError && err.errcode === "M_THREEPID_IN_USE") {
throw new UserFriendlyError("This phone number is already in use", { cause: err });
}
// Otherwise, just blurt out the same error
throw err;
}
}

/**
Expand All @@ -160,22 +157,24 @@ export default class AddThreepid {
// For separate bind, request a token directly from the IS.
const authClient = new IdentityAuthClient();
const identityAccessToken = (await authClient.getAccessToken()) ?? undefined;
return MatrixClientPeg.get()
.requestMsisdnToken(phoneCountry, phoneNumber, this.clientSecret, 1, 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;
},
try {
const res = await MatrixClientPeg.get().requestMsisdnToken(
phoneCountry,
phoneNumber,
this.clientSecret,
1,
undefined,
identityAccessToken,
);
this.sessionId = res.sid;
return res;
} catch (err) {
if (err instanceof MatrixError && err.errcode === "M_THREEPID_IN_USE") {
throw new UserFriendlyError("This phone number is already in use", { cause: err });
}
// Otherwise, just blurt out the same error
throw err;
}
} else {
// For tangled bind, request a token via the HS.
return this.addMsisdn(phoneCountry, phoneNumber);
Expand All @@ -195,7 +194,7 @@ export default class AddThreepid {
const authClient = new IdentityAuthClient();
const identityAccessToken = await authClient.getAccessToken();
if (!identityAccessToken) {
throw new Error("No identity access token found");
throw new UserFriendlyError("No identity access token found");
}
await MatrixClientPeg.get().bindThreePid({
sid: this.sessionId,
Expand All @@ -210,10 +209,10 @@ export default class AddThreepid {
// The spec has always required this to use UI auth but synapse briefly
// implemented it without, so this may just succeed and that's OK.
return [true];
} catch (e) {
if (e.httpStatus !== 401 || !e.data || !e.data.flows) {
} catch (err) {
if (!(err instanceof MatrixError) || err.httpStatus !== 401 || !err.data || !err.data.flows) {
// doesn't look like an interactive-auth failure
throw e;
throw err;
}

const dialogAesthetics = {
Expand All @@ -235,7 +234,7 @@ export default class AddThreepid {
const { finished } = Modal.createDialog(InteractiveAuthDialog, {
title: _t("Add Email Address"),
matrixClient: MatrixClientPeg.get(),
authData: e.data,
authData: err.data,
makeRequest: this.makeAddThreepidOnlyRequest,
aestheticsForStagePhases: {
[SSOAuthEntry.LOGIN_TYPE]: dialogAesthetics,
Expand All @@ -256,11 +255,13 @@ export default class AddThreepid {
);
}
} 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})`;
if (err instanceof HTTPError && err.httpStatus === 401) {
throw new UserFriendlyError(
"Failed to verify email address: make sure you clicked the link in the email",
{ cause: err },
);
}
// Otherwise, just blurt out the same error
throw err;
}
return [];
Expand Down Expand Up @@ -308,7 +309,7 @@ export default class AddThreepid {
await authClient.getAccessToken(),
);
} else {
throw new Error("The add / bind with MSISDN flow is misconfigured");
throw new UserFriendlyError("The add / bind with MSISDN flow is misconfigured");
}
if (result.errcode) {
throw result;
Expand All @@ -329,10 +330,10 @@ export default class AddThreepid {
// The spec has always required this to use UI auth but synapse briefly
// implemented it without, so this may just succeed and that's OK.
return;
} catch (e) {
if (e.httpStatus !== 401 || !e.data || !e.data.flows) {
} catch (err) {
if (!(err instanceof MatrixError) || err.httpStatus !== 401 || !err.data || !err.data.flows) {
// doesn't look like an interactive-auth failure
throw e;
throw err;
}

const dialogAesthetics = {
Expand All @@ -354,7 +355,7 @@ export default class AddThreepid {
const { finished } = Modal.createDialog(InteractiveAuthDialog, {
title: _t("Add Phone Number"),
matrixClient: MatrixClientPeg.get(),
authData: e.data,
authData: err.data,
makeRequest: this.makeAddThreepidOnlyRequest,
aestheticsForStagePhases: {
[SSOAuthEntry.LOGIN_TYPE]: dialogAesthetics,
Expand Down
19 changes: 18 additions & 1 deletion src/components/views/dialogs/ErrorDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,26 @@ limitations under the License.

import React from "react";

import { _t } from "../../../languageHandler";
import { _t, UserFriendlyError } from "../../../languageHandler";
import BaseDialog from "./BaseDialog";

/**
* Get a user friendly error message string from a given error. Useful for the
* `description` prop of the `ErrorDialog`
* @param err Error object in question to extract a useful message from. To make it easy
* to use with try/catch, this is typed as `any` because try/catch will type
* the error as `unknown`. And in any case we can use the fallback message.
* @param translatedFallbackMessage The fallback message to be used if the error doesn't have any message
* @returns a user friendly error message string from a given error
*/
export function extractErrorMessageFromError(err: any, translatedFallbackMessage: string): string {
return (
(err instanceof UserFriendlyError && err.translatedMessage) ||
(err instanceof Error && err.message) ||
translatedFallbackMessage
);
}

interface IProps {
onFinished: (success?: boolean) => void;
title?: string;
Expand Down
17 changes: 12 additions & 5 deletions src/components/views/dialogs/SetEmailDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ limitations under the License.

import React from "react";
import { logger } from "matrix-js-sdk/src/logger";
import { MatrixError } from "matrix-js-sdk/src/matrix";

import * as Email from "../../../email";
import AddThreepid from "../../../AddThreepid";
import { _t } from "../../../languageHandler";
import { _t, UserFriendlyError } from "../../../languageHandler";
import Modal from "../../../Modal";
import Spinner from "../elements/Spinner";
import ErrorDialog from "./ErrorDialog";
import ErrorDialog, { extractErrorMessageFromError } from "./ErrorDialog";
import QuestionDialog from "./QuestionDialog";
import BaseDialog from "./BaseDialog";
import EditableText from "../elements/EditableText";
Expand Down Expand Up @@ -88,7 +89,7 @@ export default class SetEmailDialog extends React.Component<IProps, IState> {
logger.error("Unable to add email address " + emailAddress + " " + err);
Modal.createDialog(ErrorDialog, {
title: _t("Unable to add email address"),
description: err && err.message ? err.message : _t("Operation failed"),
description: extractErrorMessageFromError(err, _t("Operation failed")),
});
},
);
Expand All @@ -114,7 +115,13 @@ export default class SetEmailDialog extends React.Component<IProps, IState> {
},
(err) => {
this.setState({ emailBusy: false });
if (err.errcode == "M_THREEPID_AUTH_FAILED") {

let underlyingError = err;
if (err instanceof UserFriendlyError) {
underlyingError = err.cause;
}

if (underlyingError instanceof MatrixError && underlyingError.errcode === "M_THREEPID_AUTH_FAILED") {
const message =
_t("Unable to verify email address.") +
" " +
Expand All @@ -131,7 +138,7 @@ export default class SetEmailDialog extends React.Component<IProps, IState> {
logger.error("Unable to verify email address: " + err);
Modal.createDialog(ErrorDialog, {
title: _t("Unable to verify email address."),
description: err && err.message ? err.message : _t("Operation failed"),
description: extractErrorMessageFromError(err, _t("Operation failed")),
});
}
},
Expand Down
Loading