Skip to content

Commit

Permalink
Fix MM-61975 (#8459)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
larkox authored Feb 17, 2025
1 parent 24e6750 commit 779fe31
Show file tree
Hide file tree
Showing 11 changed files with 220 additions and 25 deletions.
122 changes: 115 additions & 7 deletions app/actions/remote/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}
});
});
82 changes: 78 additions & 4 deletions app/actions/remote/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);
Expand Down Expand Up @@ -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};
}

Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions app/client/rest/tracking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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: {
Expand All @@ -384,6 +386,7 @@ export default class ClientTracking {
},
url,
details: error,
status_code,
});
} finally {
if (groupLabel && CollectNetworkMetrics) {
Expand Down
2 changes: 1 addition & 1 deletion app/managers/session_manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
Expand Down
6 changes: 5 additions & 1 deletion app/managers/session_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
4 changes: 2 additions & 2 deletions app/screens/select_team/header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions app/screens/terms_of_service/terms_of_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading

0 comments on commit 779fe31

Please sign in to comment.