From 88a782c23db14d5337d7dbfb48c76a5c8b74ecc3 Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Fri, 31 Jan 2025 08:37:00 +0200 Subject: [PATCH] [MM-62777] Fix errors being nested. Make MFA code more resilient to this situation (#8531) (#8542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix errors being nested. Make code more resilient to this situation * address review comments * code review comment (cherry picked from commit 8782f4e3a4e394631f28b558066b41e98a10aa6f) Co-authored-by: Guillermo Vayá --- app/client/rest/tracking.test.ts | 15 ++++---- app/client/rest/tracking.ts | 65 ++++++++++++++++---------------- app/screens/login/form.tsx | 19 ++++++---- app/utils/errors.test.ts | 11 ++++++ app/utils/errors.ts | 13 +++++++ 5 files changed, 76 insertions(+), 47 deletions(-) diff --git a/app/client/rest/tracking.test.ts b/app/client/rest/tracking.test.ts index 99dd20fa79b..31960d36006 100644 --- a/app/client/rest/tracking.test.ts +++ b/app/client/rest/tracking.test.ts @@ -9,9 +9,9 @@ import {Events} from '@constants'; import test_helper from '@test/test_helper'; import * as ClientConstants from './constants'; +import ClientError from './error'; import ClientTracking, {testExports} from './tracking'; -import type ClientError from './error'; import type {APIClientInterface, ClientResponseMetrics} from '@mattermost/react-native-network-client'; type ParallelGroup = typeof testExports.ParallelGroup; @@ -244,13 +244,14 @@ describe('ClientTracking', () => { try { await client.doFetchWithTracking('https://example.com/api', options); - fail('Expected error to be thrown'); } catch (error: unknown) { + expect(error).toBeInstanceOf(ClientError); + const clientError = error as ClientError; - expect((clientError.details as {message: string}).message).toBe('Custom error message'); - expect((clientError.details as {server_error_id: string}).server_error_id).toBe('error_id_123'); - expect((clientError.details as {status_code: string}).status_code).toBe(400); + expect((clientError as {message: string}).message).toBe('Custom error message'); + expect((clientError as {server_error_id: string}).server_error_id).toBe('error_id_123'); + expect((clientError as {status_code: number}).status_code).toBe(400); } }); @@ -272,8 +273,8 @@ describe('ClientTracking', () => { } catch (error: unknown) { const clientError = error as ClientError; - expect((clientError.details as {message: string}).message).toBe('Response with status code 500'); - expect((clientError.details as {status_code: string}).status_code).toBe(500); + expect((clientError as {message: string}).message).toBe('Response with status code 500'); + expect((clientError as {status_code: number}).status_code).toBe(500); } }); diff --git a/app/client/rest/tracking.ts b/app/client/rest/tracking.ts index ec19ee79945..027b320c200 100644 --- a/app/client/rest/tracking.ts +++ b/app/client/rest/tracking.ts @@ -16,7 +16,7 @@ import {semverFromServerVersion} from '@utils/server'; import * as ClientConstants from './constants'; import ClientError from './error'; -import type {APIClientInterface, ClientHeaders, ClientResponseMetrics, RequestOptions} from '@mattermost/react-native-network-client'; +import type {APIClientInterface, ClientHeaders, ClientResponse, ClientResponseMetrics, RequestOptions} from '@mattermost/react-native-network-client'; type UrlData = { count: number; @@ -372,38 +372,9 @@ export default class ClientTracking { this.incrementRequestCount(groupLabel); } + let response: ClientResponse; try { - const response = await request!(url, this.buildRequestOptions(options)); - const headers: ClientHeaders = response.headers || {}; - if (groupLabel && CollectNetworkMetrics) { - this.trackRequest(groupLabel, url, response.metrics); - } - const serverVersion = semverFromServerVersion( - headers[ClientConstants.HEADER_X_VERSION_ID] || headers[ClientConstants.HEADER_X_VERSION_ID.toLowerCase()], - ); - const hasCacheControl = Boolean( - headers[ClientConstants.HEADER_CACHE_CONTROL] || headers[ClientConstants.HEADER_CACHE_CONTROL.toLowerCase()], - ); - if (serverVersion && !hasCacheControl && this.serverVersion !== serverVersion) { - this.serverVersion = serverVersion; - DeviceEventEmitter.emit(Events.SERVER_VERSION_CHANGED, {serverUrl: this.apiClient.baseUrl, serverVersion}); - } - - const bearerToken = headers[ClientConstants.HEADER_TOKEN] || headers[ClientConstants.HEADER_TOKEN.toLowerCase()]; - if (bearerToken) { - this.setBearerToken(bearerToken); - } - - if (response.ok) { - return returnDataOnly ? (response.data || {}) : response; - } - - throw new ClientError(this.apiClient.baseUrl, { - message: response.data?.message as string || `Response with status code ${response.code}`, - server_error_id: response.data?.id as string, - status_code: response.code, - url, - }); + response = await request!(url, this.buildRequestOptions(options)); } catch (error) { throw new ClientError(this.apiClient.baseUrl, { message: 'Received invalid response from the server.', @@ -419,5 +390,35 @@ export default class ClientTracking { this.decrementRequestCount(groupLabel); } } + const headers: ClientHeaders = response.headers || {}; + if (groupLabel && CollectNetworkMetrics) { + this.trackRequest(groupLabel, url, response.metrics); + } + const serverVersion = semverFromServerVersion( + headers[ClientConstants.HEADER_X_VERSION_ID] || headers[ClientConstants.HEADER_X_VERSION_ID.toLowerCase()], + ); + const hasCacheControl = Boolean( + headers[ClientConstants.HEADER_CACHE_CONTROL] || headers[ClientConstants.HEADER_CACHE_CONTROL.toLowerCase()], + ); + if (serverVersion && !hasCacheControl && this.serverVersion !== serverVersion) { + this.serverVersion = serverVersion; + DeviceEventEmitter.emit(Events.SERVER_VERSION_CHANGED, {serverUrl: this.apiClient.baseUrl, serverVersion}); + } + + const bearerToken = headers[ClientConstants.HEADER_TOKEN] || headers[ClientConstants.HEADER_TOKEN.toLowerCase()]; + if (bearerToken) { + this.setBearerToken(bearerToken); + } + + if (response.ok) { + return returnDataOnly ? (response.data || {}) : response; + } + + throw new ClientError(this.apiClient.baseUrl, { + message: response.data?.message as string || `Response with status code ${response.code}`, + server_error_id: response.data?.id as string, + status_code: response.code, + url, + }); }; } diff --git a/app/screens/login/form.tsx b/app/screens/login/form.tsx index 325a5896188..ca25b43cb90 100644 --- a/app/screens/login/form.tsx +++ b/app/screens/login/form.tsx @@ -17,7 +17,7 @@ import {useAvoidKeyboard} from '@hooks/device'; import {t} from '@i18n'; import {goToScreen, loginAnimationOptions, resetToHome} from '@screens/navigation'; import {buttonBackgroundStyle, buttonTextStyle} from '@utils/buttonStyles'; -import {getFullErrorMessage, isErrorWithMessage, isServerError} from '@utils/errors'; +import {getFullErrorMessage, getServerError, isErrorWithMessage, isServerError} from '@utils/errors'; import {preventDoubleTap} from '@utils/tap'; import {changeOpacity, makeStyleSheetFromTheme} from '@utils/theme'; import {tryOpenURL} from '@utils/url'; @@ -117,20 +117,23 @@ const LoginForm = ({config, extra, keyboardAwareRef, serverDisplayName, launchEr resetToHome({extra, launchError: hasError, launchType, serverUrl}); }; - const checkLoginResponse = (data: LoginActionResponse) => { - let errorId = ''; - const loginError = data.error; - if (isServerError(loginError) && loginError.server_error_id) { - errorId = loginError.server_error_id; + const isMFAError = (loginError: unknown): boolean => { + const serverError = getServerError(loginError); + if (serverError) { + return MFA_EXPECTED_ERRORS.includes(serverError); } + return false; + }; - if (data.failed && MFA_EXPECTED_ERRORS.includes(errorId)) { + const checkLoginResponse = (data: LoginActionResponse) => { + const {failed, error: loginError} = data; + if (failed && isMFAError(loginError)) { goToMfa(); setIsLoading(false); return false; } - if (loginError && data.failed) { + if (failed && loginError) { setIsLoading(false); setError(getLoginErrorMessage(loginError)); return false; diff --git a/app/utils/errors.test.ts b/app/utils/errors.test.ts index 5ca5b8675d2..bfdc383b87f 100644 --- a/app/utils/errors.test.ts +++ b/app/utils/errors.test.ts @@ -9,6 +9,7 @@ import { isErrorWithStatusCode, isErrorWithUrl, getFullErrorMessage, + getServerError, } from './errors'; import {getIntlShape} from './general'; @@ -69,4 +70,14 @@ describe('Errors', () => { details: 'more info', })).toBe('Unknown error; more info'); }); + + test('getServerError', () => { + expect(getServerError({message: 'error message'})).toBe(undefined); + expect(getServerError({details: 'more info', message: 'error message'})).toBe(undefined); + expect(getServerError({server_error_id: 'server error', message: 'error message'})).toBe('server error'); + expect(getServerError({details: {server_error_id: 'deep error'}, message: 'error message'})).toBe('deep error'); + expect(getServerError({details: {details: {server_error_id: 'deeper error'}}, message: 'error message'})).toBe('deeper error'); + expect(getServerError({details: {details: {details: {details: {server_error_id: 'too deep error'}}}}, message: 'error message'})).toBe(undefined); + expect(getServerError({server_error_id: 'server error', details: {server_error_id: 'not this'}, message: 'error message'})).toBe('server error'); + }); }); diff --git a/app/utils/errors.ts b/app/utils/errors.ts index c68f9e87ad2..720816eec76 100644 --- a/app/utils/errors.ts +++ b/app/utils/errors.ts @@ -87,3 +87,16 @@ export const getErrorMessage = (error: unknown, intl?: IntlShape) => { return 'Unknown error'; }; + +export const getServerError = (error: unknown, depth = 0): string | undefined => { + if (isServerError(error)) { + return error.server_error_id!; + } + if (isErrorWithDetails(error)) { + if (depth > 2) { + return undefined; + } + return getServerError(error.details, depth + 1); + } + return undefined; +};