Skip to content

Commit

Permalink
[MM-62777] Fix errors being nested. Make MFA code more resilient to t…
Browse files Browse the repository at this point in the history 
…his situation (#8531) (#8542)

* Fix errors being nested. Make code more resilient to this situation

* address review comments

* code review comment

(cherry picked from commit 8782f4e)

Co-authored-by: Guillermo Vayá <[email protected]>
  • Loading branch information
@mattermost-build @Willyfrog
mattermost-build and Willyfrog authored Jan 31, 2025
1 parent 74660c2 commit 88a782c
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 47 deletions.
15 changes: 8 additions & 7 deletions app/client/rest/tracking.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
});

Expand All @@ -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);
}
});

Expand Down
65 changes: 33 additions & 32 deletions app/client/rest/tracking.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.',
Expand All @@ -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,
});
};
}
19 changes: 11 additions & 8 deletions app/screens/login/form.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
11 changes: 11 additions & 0 deletions app/utils/errors.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
isErrorWithStatusCode,
isErrorWithUrl,
getFullErrorMessage,
getServerError,
} from './errors';
import {getIntlShape} from './general';

Expand Down Expand Up @@ -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');
});
});
13 changes: 13 additions & 0 deletions app/utils/errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

0 comments on commit 88a782c

Please sign in to comment.