From 54b71140b8fe2ec553cf6ab42f11f61344340fff Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Thu, 11 Jan 2024 16:11:47 +0000 Subject: [PATCH] Gracefully handle rate limit response when attempting to Sign in with QR (#11809) * Gracefully hand rate limit response when attempting to Sign in with QR * Don't cancel after rate limit * Update src/components/views/auth/LoginWithQR.tsx Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> * Add missing import * Fix mock of error --------- Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/auth/LoginWithQR.tsx | 23 +++++++- src/components/views/auth/LoginWithQRFlow.tsx | 7 ++- src/i18n/strings/en_EN.json | 1 + .../settings/devices/LoginWithQR-test.tsx | 55 +++++++++++++++---- .../settings/devices/LoginWithQRFlow-test.tsx | 14 ++++- .../LoginWithQRFlow-test.tsx.snap | 49 +++++++++++++++++ 6 files changed, 131 insertions(+), 18 deletions(-) diff --git a/src/components/views/auth/LoginWithQR.tsx b/src/components/views/auth/LoginWithQR.tsx index 82e1e117064..1e2efb5106f 100644 --- a/src/components/views/auth/LoginWithQR.tsx +++ b/src/components/views/auth/LoginWithQR.tsx @@ -19,7 +19,7 @@ import { MSC3906Rendezvous, MSC3906RendezvousPayload, RendezvousFailureReason } import { MSC3886SimpleHttpRendezvousTransport } from "matrix-js-sdk/src/rendezvous/transports"; import { MSC3903ECDHPayload, MSC3903ECDHv2RendezvousChannel } from "matrix-js-sdk/src/rendezvous/channels"; import { logger } from "matrix-js-sdk/src/logger"; -import { MatrixClient } from "matrix-js-sdk/src/matrix"; +import { HTTPError, MatrixClient } from "matrix-js-sdk/src/matrix"; import { _t } from "../../../languageHandler"; import { wrapRequestWithDialog } from "../../../utils/UserInteractiveAuth"; @@ -63,10 +63,16 @@ interface IState { phase: Phase; rendezvous?: MSC3906Rendezvous; confirmationDigits?: string; - failureReason?: RendezvousFailureReason; + failureReason?: FailureReason; mediaPermissionError?: boolean; } +export enum LoginWithQRFailureReason { + RateLimited = "rate_limited", +} + +export type FailureReason = RendezvousFailureReason | LoginWithQRFailureReason; + /** * A component that allows sign in and E2EE set up with a QR code. * @@ -136,16 +142,27 @@ export default class LoginWithQR extends React.Component { // user denied return; } - if (!this.props.client.crypto) { + if (!this.props.client.getCrypto()) { // no E2EE to set up this.props.onFinished(true); return; } this.setState({ phase: Phase.Verifying }); await this.state.rendezvous.verifyNewDeviceOnExistingDevice(); + // clean up our state: + try { + await this.state.rendezvous.close(); + } finally { + this.setState({ rendezvous: undefined }); + } this.props.onFinished(true); } catch (e) { logger.error("Error whilst approving sign in", e); + if (e instanceof HTTPError && e.httpStatus === 429) { + // 429: rate limit + this.setState({ phase: Phase.Error, failureReason: LoginWithQRFailureReason.RateLimited }); + return; + } this.setState({ phase: Phase.Error, failureReason: RendezvousFailureReason.Unknown }); } }; diff --git a/src/components/views/auth/LoginWithQRFlow.tsx b/src/components/views/auth/LoginWithQRFlow.tsx index 511b4bca003..526496246a9 100644 --- a/src/components/views/auth/LoginWithQRFlow.tsx +++ b/src/components/views/auth/LoginWithQRFlow.tsx @@ -25,13 +25,13 @@ import { Icon as BackButtonIcon } from "../../../../res/img/element-icons/back.s import { Icon as DevicesIcon } from "../../../../res/img/element-icons/devices.svg"; import { Icon as WarningBadge } from "../../../../res/img/element-icons/warning-badge.svg"; import { Icon as InfoIcon } from "../../../../res/img/element-icons/i.svg"; -import { Click, Phase } from "./LoginWithQR"; +import { Click, FailureReason, LoginWithQRFailureReason, Phase } from "./LoginWithQR"; interface IProps { phase: Phase; code?: string; onClick(type: Click): Promise; - failureReason?: RendezvousFailureReason; + failureReason?: FailureReason; confirmationDigits?: string; } @@ -102,6 +102,9 @@ export default class LoginWithQRFlow extends React.Component { case RendezvousFailureReason.UserCancelled: cancellationMessage = _t("auth|qr_code_login|error_request_cancelled"); break; + case LoginWithQRFailureReason.RateLimited: + cancellationMessage = _t("auth|qr_code_login|error_rate_limited"); + break; case RendezvousFailureReason.Unknown: cancellationMessage = _t("auth|qr_code_login|error_unexpected"); break; diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index c94a36c1640..cc1cd4d79bb 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -256,6 +256,7 @@ "error_homeserver_lacks_support": "The homeserver doesn't support signing in another device.", "error_invalid_scanned_code": "The scanned code is invalid.", "error_linking_incomplete": "The linking wasn't completed in the required time.", + "error_rate_limited": "Too many attempts in a short time. Wait some time before trying again.", "error_request_cancelled": "The request was cancelled.", "error_request_declined": "The request was declined on the other device.", "error_unexpected": "An unexpected error occurred.", diff --git a/test/components/views/settings/devices/LoginWithQR-test.tsx b/test/components/views/settings/devices/LoginWithQR-test.tsx index 3f72b96ccb8..cdbb46a8b68 100644 --- a/test/components/views/settings/devices/LoginWithQR-test.tsx +++ b/test/components/views/settings/devices/LoginWithQR-test.tsx @@ -15,10 +15,10 @@ limitations under the License. */ import { cleanup, render, waitFor } from "@testing-library/react"; -import { mocked } from "jest-mock"; +import { MockedObject, mocked } from "jest-mock"; import React from "react"; import { MSC3906Rendezvous, RendezvousFailureReason } from "matrix-js-sdk/src/rendezvous"; -import { LoginTokenPostResponse } from "matrix-js-sdk/src/matrix"; +import { HTTPError, LoginTokenPostResponse } from "matrix-js-sdk/src/matrix"; import LoginWithQR, { Click, Mode, Phase } from "../../../../../src/components/views/auth/LoginWithQR"; import type { MatrixClient } from "matrix-js-sdk/src/matrix"; @@ -52,6 +52,8 @@ function makeClient() { on: jest.fn(), }, getClientWellKnown: jest.fn().mockReturnValue({}), + getCrypto: jest.fn().mockReturnValue({}), + crypto: {}, } as unknown as MatrixClient); } @@ -60,7 +62,7 @@ function unresolvedPromise(): Promise { } describe("", () => { - let client = makeClient(); + let client!: MockedObject; const defaultProps = { mode: Mode.Show, onFinished: jest.fn(), @@ -78,6 +80,7 @@ describe("", () => { beforeEach(() => { mockedFlow.mockReset(); jest.resetAllMocks(); + client = makeClient(); jest.spyOn(MSC3906Rendezvous.prototype, "generateCode").mockResolvedValue(); // @ts-ignore // workaround for https://github.com/facebook/jest/issues/9675 @@ -233,10 +236,9 @@ describe("", () => { }); test("approve - no crypto", async () => { - // @ts-ignore - client.crypto = undefined; + (client as any).crypto = undefined; + (client as any).getCrypto = () => undefined; const onFinished = jest.fn(); - // jest.spyOn(MSC3906Rendezvous.prototype, 'approveLoginOnExistingDevice').mockReturnValue(unresolvedPromise()); render(getComponent({ client, onFinished })); const rendezvous = mocked(MSC3906Rendezvous).mock.instances[0]; @@ -274,8 +276,6 @@ describe("", () => { test("approve + verifying", async () => { const onFinished = jest.fn(); - // @ts-ignore - client.crypto = {}; jest.spyOn(MSC3906Rendezvous.prototype, "verifyNewDeviceOnExistingDevice").mockImplementation(() => unresolvedPromise(), ); @@ -316,8 +316,6 @@ describe("", () => { test("approve + verify", async () => { const onFinished = jest.fn(); - // @ts-ignore - client.crypto = {}; render(getComponent({ client, onFinished })); const rendezvous = mocked(MSC3906Rendezvous).mock.instances[0]; @@ -341,6 +339,43 @@ describe("", () => { await onClick(Click.Approve); expect(rendezvous.approveLoginOnExistingDevice).toHaveBeenCalledWith("token"); expect(rendezvous.verifyNewDeviceOnExistingDevice).toHaveBeenCalled(); + expect(rendezvous.close).toHaveBeenCalled(); expect(onFinished).toHaveBeenCalledWith(true); }); + + test("approve - rate limited", async () => { + mocked(client.requestLoginToken).mockRejectedValue(new HTTPError("rate limit reached", 429)); + const onFinished = jest.fn(); + render(getComponent({ client, onFinished })); + const rendezvous = mocked(MSC3906Rendezvous).mock.instances[0]; + + await waitFor(() => + expect(mockedFlow).toHaveBeenLastCalledWith( + expect.objectContaining({ + phase: Phase.Connected, + }), + ), + ); + expect(mockedFlow).toHaveBeenLastCalledWith({ + phase: Phase.Connected, + confirmationDigits: mockConfirmationDigits, + onClick: expect.any(Function), + }); + expect(rendezvous.generateCode).toHaveBeenCalled(); + expect(rendezvous.startAfterShowingCode).toHaveBeenCalled(); + + // approve + const onClick = mockedFlow.mock.calls[0][0].onClick; + await onClick(Click.Approve); + + // the 429 error should be handled and mapped + await waitFor(() => + expect(mockedFlow).toHaveBeenLastCalledWith( + expect.objectContaining({ + phase: Phase.Error, + failureReason: "rate_limited", + }), + ), + ); + }); }); diff --git a/test/components/views/settings/devices/LoginWithQRFlow-test.tsx b/test/components/views/settings/devices/LoginWithQRFlow-test.tsx index df2f835ba8b..614a8d3ffd4 100644 --- a/test/components/views/settings/devices/LoginWithQRFlow-test.tsx +++ b/test/components/views/settings/devices/LoginWithQRFlow-test.tsx @@ -19,7 +19,12 @@ import React from "react"; import { RendezvousFailureReason } from "matrix-js-sdk/src/rendezvous"; import LoginWithQRFlow from "../../../../../src/components/views/auth/LoginWithQRFlow"; -import { Click, Phase } from "../../../../../src/components/views/auth/LoginWithQR"; +import { + Click, + Phase, + LoginWithQRFailureReason, + FailureReason, +} from "../../../../../src/components/views/auth/LoginWithQR"; describe("", () => { const onClick = jest.fn(); @@ -31,7 +36,7 @@ describe("", () => { const getComponent = (props: { phase: Phase; onClick?: () => Promise; - failureReason?: RendezvousFailureReason; + failureReason?: FailureReason; code?: string; confirmationDigits?: string; }) => ; @@ -97,7 +102,10 @@ describe("", () => { }); describe("errors", () => { - for (const failureReason of Object.values(RendezvousFailureReason)) { + for (const failureReason of [ + ...Object.values(RendezvousFailureReason), + ...Object.values(LoginWithQRFailureReason), + ]) { it(`renders ${failureReason}`, async () => { const { container } = render( getComponent({ diff --git a/test/components/views/settings/devices/__snapshots__/LoginWithQRFlow-test.tsx.snap b/test/components/views/settings/devices/__snapshots__/LoginWithQRFlow-test.tsx.snap index ba0a051b1c3..9ba678685b0 100644 --- a/test/components/views/settings/devices/__snapshots__/LoginWithQRFlow-test.tsx.snap +++ b/test/components/views/settings/devices/__snapshots__/LoginWithQRFlow-test.tsx.snap @@ -294,6 +294,55 @@ exports[` errors renders other_device_not_signed_in 1`] = ` `; +exports[` errors renders rate_limited 1`] = ` +
+
+
+

+
+ Connection failed +

+
+
+

+ Too many attempts in a short time. Wait some time before trying again. +

+
+
+
+ Try again +
+
+ Cancel +
+
+
+
+`; + exports[` errors renders unknown 1`] = `