From ecb54cebf4f4e04d918527e23bbab99de98b3816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Espino=20Garc=C3=ADa?= Date: Mon, 17 Feb 2025 13:45:12 +0100 Subject: [PATCH] Fix MM-61975 (#8459) * Fix MM-61975 * Add missing strings * Ensure the app gets logged out after not accepting the ToS * Fix i18n and add comment * Fix tests and address feedback * Check for the right value coming from status * Update texts (cherry picked from commit 779fe31c3bcb1c8753402e6424219b27a11b2db7) --- app/actions/remote/session.test.ts | 122 +++++++++++++++++- app/actions/remote/session.ts | 82 +++++++++++- app/client/rest/tracking.ts | 3 + app/managers/session_manager.test.ts | 2 +- app/managers/session_manager.ts | 6 +- .../components/options/logout/index.tsx | 2 +- .../categories_list/header/header.tsx | 4 +- .../servers_list/server_item/server_item.tsx | 10 +- app/screens/select_team/header.tsx | 4 +- .../terms_of_service/terms_of_service.tsx | 4 +- assets/base/i18n/en.json | 6 + 11 files changed, 220 insertions(+), 25 deletions(-) diff --git a/app/actions/remote/session.test.ts b/app/actions/remote/session.test.ts index 09051d2b8c6..42ad69a485a 100644 --- a/app/actions/remote/session.test.ts +++ b/app/actions/remote/session.test.ts @@ -3,11 +3,14 @@ /* eslint-disable max-lines */ -import {Platform} from 'react-native'; +import {createIntl} from 'react-intl'; +import {Alert, DeviceEventEmitter, Platform} from 'react-native'; +import {Events} from '@constants'; import {GLOBAL_IDENTIFIERS, SYSTEM_IDENTIFIERS} from '@constants/database'; import DatabaseManager from '@database/manager'; import NetworkManager from '@managers/network_manager'; +import {logWarning} from '@utils/log'; import { addPushProxyVerificationStateFromLogin, @@ -25,6 +28,11 @@ import { import type ServerDataOperator from '@database/operator/server_data_operator'; import type {LoginArgs} from '@typings/database/database'; +const intl = createIntl({ + locale: 'en', + messages: {}, +}); + const serverUrl = 'baseHandler.test.com'; let operator: ServerDataOperator; @@ -45,6 +53,7 @@ const mockClient = { getSessions: jest.fn(() => [session1]), sendPasswordResetEmail: jest.fn(() => ({status: 200})), getMe: jest.fn(() => user1), + logout: jest.fn(), }; let mockGetPushProxyVerificationState: jest.Mock; @@ -90,6 +99,14 @@ jest.mock('@utils/security', () => { }; }); +jest.mock('@utils/log', () => { + const original = jest.requireActual('@utils/log'); + return { + ...original, + logWarning: jest.fn(), + }; +}); + beforeAll(() => { // eslint-disable-next-line // @ts-ignore @@ -194,12 +211,6 @@ describe('sessions', () => { expect(result.failed).toBe(false); }); - it('logout - base case', async () => { - const result = await logout(serverUrl, true, true, true); - expect(result).toBeDefined(); - expect(result.data).toBeDefined(); - }); - it('cancelSessionNotification - handle not found database', async () => { const result = await cancelSessionNotification('foo'); expect(result).toBeDefined(); @@ -370,3 +381,100 @@ describe('sessions', () => { expect(result).toBeUndefined(); }); }); + +describe('logout', () => { + const mockEmit = jest.spyOn(DeviceEventEmitter, 'emit').mockImplementation(() => true); + const mockAlert = jest.spyOn(Alert, 'alert').mockImplementation(() => true); + + type TestCase = { + options: { + skipServerLogout: boolean; + logoutOnAlert: boolean; + removeServer: boolean; + skipEvents: boolean; + }; + withIntl: boolean; + clientReturnError: boolean; + } + + const testCases: TestCase[] = []; + for (const skipServerLogout of [false, true]) { + for (const logoutOnAlert of [false, true]) { + for (const removeServer of [false, true]) { + for (const skipEvents of [false, true]) { + for (const withIntl of [false, true]) { + for (const clientReturnError of [false, true]) { + testCases.push({options: { + skipServerLogout, + logoutOnAlert, + removeServer, + skipEvents}, + withIntl, + clientReturnError}); + } + } + } + } + } + } + + test.each(testCases)('%j', async ({clientReturnError, options, withIntl}) => { + if (clientReturnError) { + mockClient.logout.mockImplementationOnce(() => { + throw new Error('logout error'); + }); + } else { + mockClient.logout = jest.fn(() => ({status: 'OK'})); + } + + const mockFormatMessage = jest.spyOn(intl, 'formatMessage'); + + const shouldCallClient = !options.skipServerLogout; + const shouldLogWarning = shouldCallClient && clientReturnError; + const shouldShowAlert = shouldCallClient && clientReturnError; + const shouldEmit = !options.skipEvents; + const shouldEmitBeforeAlert = shouldEmit && (!shouldShowAlert || options.logoutOnAlert); + const shouldEmitAfterAlert = shouldEmit && !shouldEmitBeforeAlert; + const expectedResult = !(shouldShowAlert && !options.logoutOnAlert); + + const clientCalls = shouldCallClient ? 1 : 0; + const alertButtons = options.logoutOnAlert ? 1 : 2; + + const result = await logout(serverUrl, withIntl ? intl : undefined, {...options}); + + expect(result.data).toBe(expectedResult); + expect(mockClient.logout).toHaveBeenCalledTimes(clientCalls); + if (shouldLogWarning) { + expect(logWarning).toHaveBeenCalled(); + } + + if (shouldEmitBeforeAlert) { + expect(mockEmit).toHaveBeenCalledWith(Events.SERVER_LOGOUT, {serverUrl, removeServer: options.removeServer}); + } else { + expect(mockEmit).not.toHaveBeenCalled(); + } + if (shouldShowAlert) { + if (withIntl) { + expect(mockFormatMessage).toHaveBeenCalled(); + } + expect(mockAlert).toHaveBeenCalled(); + expect(mockAlert.mock.calls[0][2]).toHaveLength(alertButtons); + if (alertButtons === 2) { + mockAlert.mock.calls[0][2]?.[0].onPress?.(); + expect(mockClient.logout).toHaveBeenCalledTimes(clientCalls); + expect(mockEmit).toHaveBeenCalledTimes(0); + } + + mockAlert.mock.calls[0][2]?.[alertButtons - 1].onPress?.(); // Last button should be confirm + expect(mockClient.logout).toHaveBeenCalledTimes(clientCalls); + if (shouldEmitAfterAlert) { + expect(mockEmit).toHaveBeenCalledWith(Events.SERVER_LOGOUT, {serverUrl, removeServer: options.removeServer}); + } else { + expect(mockEmit).toHaveBeenCalledTimes(shouldEmitBeforeAlert ? 1 : 0); + } + } else { + expect(mockFormatMessage).not.toHaveBeenCalled(); + expect(mockAlert).not.toHaveBeenCalled(); + } + }); +}); diff --git a/app/actions/remote/session.ts b/app/actions/remote/session.ts index fbe42bfb29e..69a67e5fe66 100644 --- a/app/actions/remote/session.ts +++ b/app/actions/remote/session.ts @@ -2,7 +2,8 @@ // See LICENSE.txt for license information. import NetInfo from '@react-native-community/netinfo'; -import {DeviceEventEmitter, Platform} from 'react-native'; +import {defineMessages, type IntlShape} from 'react-intl'; +import {Alert, DeviceEventEmitter, Platform, type AlertButton} from 'react-native'; import {Database, Events} from '@constants'; import {SYSTEM_IDENTIFIERS} from '@constants/database'; @@ -25,6 +26,33 @@ import type {LoginArgs} from '@typings/database/database'; const HTTP_UNAUTHORIZED = 401; +const logoutMessages = defineMessages({ + title: { + id: 'logout.fail.title', + defaultMessage: 'Logout not complete', + }, + bodyForced: { + id: 'logout.fail.message.forced', + defaultMessage: 'We could not log you out of the server. Some data may continue to be accessible to this device once the device goes back online.', + }, + body: { + id: 'logout.fail.message', + defaultMessage: 'You’re not fully logged out. Some data may continue to be accessible to this device once the device goes back online. What do you want to do?', + }, + cancel: { + id: 'logout.fail.cancel', + defaultMessage: 'Cancel', + }, + continue: { + id: 'logout.fail.continue_anyway', + defaultMessage: 'Continue Anyway', + }, + ok: { + id: 'logout.fail.ok', + defaultMessage: 'OK', + }, +}); + export const addPushProxyVerificationStateFromLogin = async (serverUrl: string) => { try { const {operator} = DatabaseManager.getServerDatabaseAndOperator(serverUrl); @@ -56,7 +84,7 @@ export const forceLogoutIfNecessary = async (serverUrl: string, err: unknown) => const currentUserId = await getCurrentUserId(database); if (isErrorWithStatusCode(err) && err.status_code === HTTP_UNAUTHORIZED && isErrorWithUrl(err) && err.url?.indexOf('/login') === -1 && currentUserId) { - await logout(serverUrl); + await logout(serverUrl, undefined, {skipServerLogout: true}); return {error: null, logout: true}; } @@ -135,15 +163,61 @@ export const login = async (serverUrl: string, {ldapOnly = false, loginId, mfaTo } }; -export const logout = async (serverUrl: string, skipServerLogout = false, removeServer = false, skipEvents = false) => { +type LogoutOptions = { + skipServerLogout?: boolean; + removeServer?: boolean; + skipEvents?: boolean; + logoutOnAlert?: boolean; +}; + +export const logout = async ( + serverUrl: string, + intl: IntlShape | undefined, + { + skipServerLogout = false, + removeServer = false, + skipEvents = false, + logoutOnAlert = false, + }: LogoutOptions = {}) => { if (!skipServerLogout) { + let loggedOut = false; try { const client = NetworkManager.getClient(serverUrl); - await client.logout(); + const response = await client.logout(); + if (response.status === 'OK') { + loggedOut = true; + } } catch (error) { // We want to log the user even if logging out from the server failed logWarning('An error occurred logging out from the server', serverUrl, getFullErrorMessage(error)); } + + if (!loggedOut) { + const title = intl?.formatMessage(logoutMessages.title) || logoutMessages.title.defaultMessage; + + const bodyMessage = logoutOnAlert ? logoutMessages.bodyForced : logoutMessages.body; + const confirmMessage = logoutOnAlert ? logoutMessages.ok : logoutMessages.continue; + const body = intl?.formatMessage(bodyMessage) || bodyMessage.defaultMessage; + const cancel = intl?.formatMessage(logoutMessages.cancel) || logoutMessages.cancel.defaultMessage; + const confirm = intl?.formatMessage(confirmMessage) || confirmMessage.defaultMessage; + + const buttons: AlertButton[] = logoutOnAlert ? [] : [{text: cancel, style: 'cancel'}]; + buttons.push({ + text: confirm, + onPress: logoutOnAlert ? undefined : () => { + logout(serverUrl, intl, {skipEvents, removeServer, logoutOnAlert, skipServerLogout: true}); + }, + }); + Alert.alert( + title, + body, + buttons, + ); + + if (!logoutOnAlert) { + return {data: false}; + } + } } if (!skipEvents) { diff --git a/app/client/rest/tracking.ts b/app/client/rest/tracking.ts index 027b320c200..4b149201d35 100644 --- a/app/client/rest/tracking.ts +++ b/app/client/rest/tracking.ts @@ -9,6 +9,7 @@ import {t} from '@i18n'; import {setServerCredentials} from '@init/credentials'; import PerformanceMetricsManager from '@managers/performance_metrics_manager'; import {NetworkRequestMetrics} from '@managers/performance_metrics_manager/constant'; +import {isErrorWithStatusCode} from '@utils/errors'; import {getFormattedFileSize} from '@utils/file'; import {logDebug, logInfo} from '@utils/log'; import {semverFromServerVersion} from '@utils/server'; @@ -376,6 +377,7 @@ export default class ClientTracking { try { response = await request!(url, this.buildRequestOptions(options)); } catch (error) { + const status_code = isErrorWithStatusCode(error) ? error.status_code : undefined; throw new ClientError(this.apiClient.baseUrl, { message: 'Received invalid response from the server.', intl: { @@ -384,6 +386,7 @@ export default class ClientTracking { }, url, details: error, + status_code, }); } finally { if (groupLabel && CollectNetworkMetrics) { diff --git a/app/managers/session_manager.test.ts b/app/managers/session_manager.test.ts index dec87886bc0..c9d95bc0137 100644 --- a/app/managers/session_manager.test.ts +++ b/app/managers/session_manager.test.ts @@ -114,7 +114,7 @@ describe('SessionManager', () => { await new Promise((resolve) => setImmediate(resolve)); - expect(logout).toHaveBeenCalledWith(mockServerUrl, false, false, true); + expect(logout).toHaveBeenCalledWith(mockServerUrl, undefined, {skipEvents: true, skipServerLogout: true}); expect(relaunchApp).toHaveBeenCalled(); }); }); diff --git a/app/managers/session_manager.ts b/app/managers/session_manager.ts index 5bccfe5c06a..f412a94c132 100644 --- a/app/managers/session_manager.ts +++ b/app/managers/session_manager.ts @@ -187,7 +187,11 @@ export class SessionManager { private onSessionExpired = async (serverUrl: string) => { this.terminatingSessionUrl.add(serverUrl); - await logout(serverUrl, false, false, true); + + // logout is not doing anything in this scenario, but we keep it + // to keep the same flow as other logout scenarios. + await logout(serverUrl, undefined, {skipServerLogout: true, skipEvents: true}); + await this.terminateSession(serverUrl, false); const activeServerUrl = await DatabaseManager.getActiveServerUrl(); diff --git a/app/screens/home/account/components/options/logout/index.tsx b/app/screens/home/account/components/options/logout/index.tsx index 742e3d51148..ee9cd809655 100644 --- a/app/screens/home/account/components/options/logout/index.tsx +++ b/app/screens/home/account/components/options/logout/index.tsx @@ -33,7 +33,7 @@ const LogOut = () => { const onLogout = useCallback(preventDoubleTap(() => { Navigation.updateProps(Screens.HOME, {extra: undefined}); - alertServerLogout(serverDisplayName, () => logout(serverUrl), intl); + alertServerLogout(serverDisplayName, () => logout(serverUrl, intl), intl); }), [serverDisplayName, serverUrl, intl]); return ( diff --git a/app/screens/home/channel_list/categories_list/header/header.tsx b/app/screens/home/channel_list/categories_list/header/header.tsx index 38a9b696315..3b9cb42a5c9 100644 --- a/app/screens/home/channel_list/categories_list/header/header.tsx +++ b/app/screens/home/channel_list/categories_list/header/header.tsx @@ -169,8 +169,8 @@ const ChannelListHeader = ({ }, [pushProxyStatus, intl]); const onLogoutPress = useCallback(() => { - alertServerLogout(serverDisplayName, () => logout(serverUrl), intl); - }, []); + alertServerLogout(serverDisplayName, () => logout(serverUrl, intl), intl); + }, [intl, serverDisplayName, serverUrl]); let header; if (displayName) { diff --git a/app/screens/home/channel_list/servers/servers_list/server_item/server_item.tsx b/app/screens/home/channel_list/servers/servers_list/server_item/server_item.tsx index 42f0a85da8c..3929e473c8d 100644 --- a/app/screens/home/channel_list/servers/servers_list/server_item/server_item.tsx +++ b/app/screens/home/channel_list/servers/servers_list/server_item/server_item.tsx @@ -181,21 +181,21 @@ const ServerItem = ({ const logoutServer = useCallback(async () => { Navigation.updateProps(Screens.HOME, {extra: undefined}); - await logout(server.url); + await logout(server.url, intl); if (isActive) { dismissBottomSheet(); } else { DeviceEventEmitter.emit(Events.SWIPEABLE, ''); } - }, [isActive, server.url]); + }, [intl, isActive, server.url]); const removeServer = useCallback(async () => { - const skipLogoutFromServer = server.lastActiveAt === 0; + const skipServerLogout = server.lastActiveAt === 0; await dismissBottomSheet(); Navigation.updateProps(Screens.HOME, {extra: undefined}); - await logout(server.url, skipLogoutFromServer, true); - }, [server.lastActiveAt, server.url]); + await logout(server.url, intl, {skipServerLogout, removeServer: true}); + }, [intl, server.lastActiveAt, server.url]); const startTutorial = () => { viewRef.current?.measureInWindow((x, y, w, h) => { diff --git a/app/screens/select_team/header.tsx b/app/screens/select_team/header.tsx index b38018f7adb..28ba8c75b5b 100644 --- a/app/screens/select_team/header.tsx +++ b/app/screens/select_team/header.tsx @@ -55,8 +55,8 @@ function Header() { const headerStyle = useMemo(() => ({...styles.header, marginLeft: canAddOtherServers ? MARGIN_WITH_SERVER_ICON : undefined}), [canAddOtherServers]); const onLogoutPress = useCallback(() => { - alertServerLogout(serverDisplayName, () => logout(serverUrl), intl); - }, [serverUrl, serverDisplayName]); + alertServerLogout(serverDisplayName, () => logout(serverUrl, intl), intl); + }, [serverDisplayName, intl, serverUrl]); const onLabelPress = useCallback(() => { serverButtonRef.current?.openServers(); diff --git a/app/screens/terms_of_service/terms_of_service.tsx b/app/screens/terms_of_service/terms_of_service.tsx index be3360621a6..5d6818d0e2a 100644 --- a/app/screens/terms_of_service/terms_of_service.tsx +++ b/app/screens/terms_of_service/terms_of_service.tsx @@ -125,8 +125,8 @@ const TermsOfService = ({ const closeTermsAndLogout = useCallback(() => { dismissOverlay(componentId); - logout(serverUrl); - }, [serverUrl, componentId]); + logout(serverUrl, intl, {logoutOnAlert: true}); + }, [serverUrl, componentId, intl]); const alertError = useCallback((retry: () => void) => { Alert.alert( diff --git a/assets/base/i18n/en.json b/assets/base/i18n/en.json index 1be2087178b..160b1fed013 100644 --- a/assets/base/i18n/en.json +++ b/assets/base/i18n/en.json @@ -465,6 +465,12 @@ "login.signIn": "Log In", "login.signingIn": "Logging In", "login.username": "Username", + "logout.fail.cancel": "Cancel", + "logout.fail.continue_anyway": "Continue Anyway", + "logout.fail.message": "You’re not fully logged out. Some data may continue to be accessible to this device once the device goes back online. What do you want to do?", + "logout.fail.message.forced": "We could not log you out of the server. Some data may continue to be accessible to this device once the device goes back online.", + "logout.fail.ok": "OK", + "logout.fail.title": "Logout not complete", "markdown.latex.error": "Latex render error", "markdown.max_nodes.error": "This message is too long to by shown fully on a mobile device. Please view it on desktop or contact an admin to increase this limit.", "markdown.parse_error": "An error occurred while parsing this text",