From fbaf13c7aa9c593324ef1fc0d93f1c635e9c9e99 Mon Sep 17 00:00:00 2001 From: Felipe Martin <812088+fmartingr@users.noreply.github.com> Date: Tue, 27 Jan 2026 15:52:37 +0100 Subject: [PATCH] fix: prevent removal of pre-auth secret on logout (#9411) * fix: prevent removal of pre-auth secret on logout * fix: use pre-auth secret when reconnecting to server after logout * test: verify pre-auth secret usage in switchToServerAndLogin --- app/actions/app/server.test.ts | 52 ++++ app/actions/app/server.ts | 6 +- app/actions/local/session/index.test.ts | 24 ++ app/database/manager/index.ts | 4 + app/init/credentials.test.ts | 390 ++++++++++++++++++++++++ app/init/credentials.ts | 15 + 6 files changed, 490 insertions(+), 1 deletion(-) create mode 100644 app/init/credentials.test.ts diff --git a/app/actions/app/server.test.ts b/app/actions/app/server.test.ts index 43ad00d11..077e1745a 100644 --- a/app/actions/app/server.test.ts +++ b/app/actions/app/server.test.ts @@ -10,6 +10,7 @@ import {fetchConfigAndLicense} from '@actions/remote/systems'; import {Events, Preferences, Screens} from '@constants'; import DatabaseManager from '@database/manager'; import {DEFAULT_LOCALE, getTranslations} from '@i18n'; +import {getPreauthSecret} from '@init/credentials'; import SecurityManager from '@managers/security_manager'; import WebsocketManager from '@managers/websocket_manager'; import {getServer, getServerByIdentifier} from '@queries/app/servers'; @@ -26,6 +27,7 @@ jest.mock('@queries/app/servers'); jest.mock('@queries/servers/system'); jest.mock('@database/manager'); jest.mock('@managers/security_manager'); +jest.mock('@init/credentials'); jest.mock('@managers/websocket_manager'); jest.mock('@utils/log'); @@ -182,4 +184,54 @@ describe('switchToServerAndLogin', () => { expect(SecurityManager.isDeviceJailbroken).toHaveBeenCalledWith('serverUrl'); expect(callback).toHaveBeenCalled(); }); + + it('should retrieve and use pre-auth secret when reconnecting after logout', async () => { + const server = {url: 'serverUrl', displayName: 'Server'} as ServersModel; + const config = {DiagnosticId: 'diagId'} as ClientConfig; + const license = {} as ClientLicense; + const preauthSecret = 'test-secret-123'; + + jest.mocked(getServer).mockResolvedValueOnce(server); + jest.mocked(getPreauthSecret).mockResolvedValueOnce(preauthSecret); + jest.mocked(doPing).mockResolvedValueOnce({}); + jest.mocked(fetchConfigAndLicense).mockResolvedValueOnce({config, license}); + jest.mocked(getServerByIdentifier).mockResolvedValueOnce(undefined); + + await Actions.switchToServerAndLogin('serverUrl', theme, intl, jest.fn()); + + expect(getPreauthSecret).toHaveBeenCalledWith('serverUrl'); + expect(doPing).toHaveBeenCalledWith('serverUrl', true, 5000, preauthSecret); + }); + + it('should work correctly when no pre-auth secret is stored', async () => { + const server = {url: 'serverUrl', displayName: 'Server'} as ServersModel; + const config = {DiagnosticId: 'diagId'} as ClientConfig; + const license = {} as ClientLicense; + + jest.mocked(getServer).mockResolvedValueOnce(server); + jest.mocked(getPreauthSecret).mockResolvedValueOnce(undefined); + jest.mocked(doPing).mockResolvedValueOnce({}); + jest.mocked(fetchConfigAndLicense).mockResolvedValueOnce({config, license}); + jest.mocked(getServerByIdentifier).mockResolvedValueOnce(undefined); + + await Actions.switchToServerAndLogin('serverUrl', theme, intl, jest.fn()); + + expect(getPreauthSecret).toHaveBeenCalledWith('serverUrl'); + expect(doPing).toHaveBeenCalledWith('serverUrl', true, 5000, undefined); + }); + + it('should pass pre-auth secret to doPing even when ping fails', async () => { + const server = {url: 'serverUrl'} as ServersModel; + const preauthSecret = 'test-secret-456'; + + jest.mocked(getServer).mockResolvedValueOnce(server); + jest.mocked(getPreauthSecret).mockResolvedValueOnce(preauthSecret); + jest.mocked(doPing).mockResolvedValueOnce({error: 'ping error'}); + + await Actions.switchToServerAndLogin('serverUrl', theme, intl, jest.fn()); + + expect(getPreauthSecret).toHaveBeenCalledWith('serverUrl'); + expect(doPing).toHaveBeenCalledWith('serverUrl', true, 5000, preauthSecret); + expect(alertServerError).toHaveBeenCalledWith(intl, 'ping error'); + }); }); diff --git a/app/actions/app/server.ts b/app/actions/app/server.ts index e586a52b8..0f411033f 100644 --- a/app/actions/app/server.ts +++ b/app/actions/app/server.ts @@ -7,6 +7,7 @@ import {doPing} from '@actions/remote/general'; import {fetchConfigAndLicense} from '@actions/remote/systems'; import {Screens} from '@constants'; import DatabaseManager from '@database/manager'; +import {getPreauthSecret} from '@init/credentials'; import SecurityManager from '@managers/security_manager'; import WebsocketManager from '@managers/websocket_manager'; import {getServer, getServerByIdentifier} from '@queries/app/servers'; @@ -53,7 +54,10 @@ export async function switchToServerAndLogin(serverUrl: string, theme: Theme, in return; } - const result = await doPing(server.url, true); + // Retrieve pre-auth secret from keychain if it exists + const preauthSecret = await getPreauthSecret(server.url); + + const result = await doPing(server.url, true, 5000, preauthSecret); if (result.error) { alertServerError(intl, result.error); callback?.(); diff --git a/app/actions/local/session/index.test.ts b/app/actions/local/session/index.test.ts index f3322bb19..956c4ea9b 100644 --- a/app/actions/local/session/index.test.ts +++ b/app/actions/local/session/index.test.ts @@ -342,5 +342,29 @@ describe('session actions', () => { expect(resetMomentLocale).toHaveBeenCalledWith(); }); + + it('should remove user credentials but preserve pre-auth secret on logout (removeServer=false)', async () => { + await terminateSession(mockServerUrl, false); + + // Verify user credentials are removed + expect(removeServerCredentials).toHaveBeenCalledWith(mockServerUrl); + + // Verify database is deleted (not destroyed) + expect(DatabaseManager.deleteServerDatabase).toHaveBeenCalledWith(mockServerUrl); + expect(DatabaseManager.destroyServerDatabase).not.toHaveBeenCalled(); + + // Note: removeServerCredentials now only removes the token, not the pre-auth secret + // The pre-auth secret is only removed when destroyServerDatabase is called + }); + + it('should call destroyServerDatabase which removes pre-auth secret on server removal (removeServer=true)', async () => { + await terminateSession(mockServerUrl, true); + + // Verify database is destroyed (which will remove pre-auth secret internally) + expect(DatabaseManager.destroyServerDatabase).toHaveBeenCalledWith(mockServerUrl); + expect(DatabaseManager.deleteServerDatabase).not.toHaveBeenCalled(); + + // Note: destroyServerDatabase internally calls removePreauthSecret + }); }); }); diff --git a/app/database/manager/index.ts b/app/database/manager/index.ts index 200e1bbd4..317b5320a 100644 --- a/app/database/manager/index.ts +++ b/app/database/manager/index.ts @@ -24,6 +24,7 @@ import ServerDataOperator from '@database/operator/server_data_operator'; import {schema as appSchema} from '@database/schema/app'; import {serverSchema} from '@database/schema/server'; import {beforeUpgrade} from '@helpers/database/upgrade'; +import {removePreauthSecret} from '@init/credentials'; import {PlaybookRunModel, PlaybookChecklistModel, PlaybookChecklistItemModel, PlaybookRunPropertyFieldModel, PlaybookRunPropertyValueModel} from '@playbooks/database/models'; import {getActiveServer, getServer, getServerByIdentifier} from '@queries/app/servers'; import {logDebug, logError} from '@utils/log'; @@ -391,6 +392,9 @@ class DatabaseManagerSingleton { delete this.serverDatabases[serverUrl]; this.deleteServerDatabaseFiles(serverUrl); + + // Remove pre-auth secret when server is destroyed + await removePreauthSecret(serverUrl); } } }; diff --git a/app/init/credentials.test.ts b/app/init/credentials.test.ts new file mode 100644 index 000000000..a1cbd13b6 --- /dev/null +++ b/app/init/credentials.test.ts @@ -0,0 +1,390 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {Platform} from 'react-native'; +import * as KeyChain from 'react-native-keychain'; + +import { + getAllServerCredentials, + getServerCredentials, + setServerCredentials, + removeServerCredentials, + removePreauthSecret, +} from './credentials'; + +jest.mock('react-native-keychain', () => ({ + SECURITY_LEVEL: { + SECURE_SOFTWARE: 'SECURE_SOFTWARE', + }, + STORAGE_TYPE: { + FB: 'FB', + AES: 'AES', + RSA: 'RSA', + }, + setInternetCredentials: jest.fn(), + getInternetCredentials: jest.fn(), + resetInternetCredentials: jest.fn(), + setGenericPassword: jest.fn(), + getGenericPassword: jest.fn(), + resetGenericPassword: jest.fn(), + getAllInternetPasswordServers: jest.fn(), + getAllGenericPasswordServices: jest.fn(), +})); +jest.mock('@utils/log'); +jest.mock('@utils/mattermost_managed', () => ({ + getIOSAppGroupDetails: jest.fn().mockReturnValue({ + appGroupIdentifier: 'group.com.mattermost.test', + }), +})); +jest.mock('@database/manager', () => ({ + getActiveServerUrl: jest.fn().mockResolvedValue('https://example.com'), + serverDatabases: {}, +})); + +describe('credentials', () => { + const mockServerUrl = 'https://example.com'; + const mockToken = 'test-token-123'; + const mockUserId = 'user-id-123'; + const mockPreauthSecret = 'preauth-secret-123'; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('setServerCredentials', () => { + it('should store credentials with pre-auth secret', () => { + setServerCredentials(mockServerUrl, mockToken, mockPreauthSecret); + + expect(KeyChain.setInternetCredentials).toHaveBeenCalledWith( + mockServerUrl, + mockToken, + mockToken, + expect.objectContaining({ + securityLevel: KeyChain.SECURITY_LEVEL.SECURE_SOFTWARE, + }), + ); + + expect(KeyChain.setGenericPassword).toHaveBeenCalledWith( + 'preshared_secret', + mockPreauthSecret, + expect.objectContaining({ + server: mockServerUrl, + securityLevel: KeyChain.SECURITY_LEVEL.SECURE_SOFTWARE, + }), + ); + }); + + it('should store credentials without pre-auth secret', () => { + setServerCredentials(mockServerUrl, mockToken); + + expect(KeyChain.setInternetCredentials).toHaveBeenCalledWith( + mockServerUrl, + mockToken, + mockToken, + expect.any(Object), + ); + + expect(KeyChain.resetGenericPassword).toHaveBeenCalledWith( + expect.objectContaining({ + server: mockServerUrl, + }), + ); + }); + + it('should not store credentials when serverUrl is missing', () => { + setServerCredentials('', mockToken, mockPreauthSecret); + + expect(KeyChain.setInternetCredentials).not.toHaveBeenCalled(); + expect(KeyChain.setGenericPassword).not.toHaveBeenCalled(); + }); + + it('should not store credentials when token is missing', () => { + setServerCredentials(mockServerUrl, '', mockPreauthSecret); + + expect(KeyChain.setInternetCredentials).not.toHaveBeenCalled(); + expect(KeyChain.setGenericPassword).not.toHaveBeenCalled(); + }); + + it('should use iOS app group on iOS platform', () => { + Platform.OS = 'ios'; + + setServerCredentials(mockServerUrl, mockToken, mockPreauthSecret); + + expect(KeyChain.setInternetCredentials).toHaveBeenCalledWith( + mockServerUrl, + mockToken, + mockToken, + expect.objectContaining({ + accessGroup: 'group.com.mattermost.test', + }), + ); + }); + + it('should not use app group on Android platform', () => { + Platform.OS = 'android'; + + setServerCredentials(mockServerUrl, mockToken, mockPreauthSecret); + + expect(KeyChain.setInternetCredentials).toHaveBeenCalledWith( + mockServerUrl, + mockToken, + mockToken, + expect.objectContaining({ + accessGroup: undefined, + }), + ); + }); + }); + + describe('getServerCredentials', () => { + it('should retrieve credentials with pre-auth secret', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: mockUserId, + password: mockToken, + service: mockServerUrl, + storage: 'keychain' as any, + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue({ + username: 'preshared_secret', + password: mockPreauthSecret, + service: mockServerUrl, + storage: 'keychain' as any, + }); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toEqual({ + serverUrl: mockServerUrl, + userId: mockUserId, + token: mockToken, + preauthSecret: mockPreauthSecret, + }); + }); + + it('should retrieve credentials without pre-auth secret', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: mockUserId, + password: mockToken, + service: mockServerUrl, + storage: 'keychain' as any, + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue(false); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toEqual({ + serverUrl: mockServerUrl, + userId: mockUserId, + token: mockToken, + preauthSecret: undefined, + }); + }); + + it('should handle legacy token format with device token', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: 'device-token-123,user-id-456', + password: mockToken, + service: mockServerUrl, + storage: 'keychain' as any, + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue(false); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toEqual({ + serverUrl: mockServerUrl, + userId: 'user-id-456', + token: mockToken, + preauthSecret: undefined, + }); + }); + + it('should return null when credentials do not exist', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue(false); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toBeNull(); + }); + + it('should return null when token is undefined', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: mockUserId, + password: 'undefined', + service: mockServerUrl, + storage: 'keychain' as any, + }); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toBeNull(); + }); + + it('should gracefully handle errors when retrieving pre-auth secret', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: mockUserId, + password: mockToken, + service: mockServerUrl, + storage: 'keychain' as any, + }); + + jest.mocked(KeyChain.getGenericPassword).mockRejectedValue(new Error('Keychain error')); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toEqual({ + serverUrl: mockServerUrl, + userId: mockUserId, + token: mockToken, + preauthSecret: undefined, + }); + }); + + it('should return null on error retrieving main credentials', async () => { + jest.mocked(KeyChain.getInternetCredentials).mockRejectedValue(new Error('Keychain error')); + + const result = await getServerCredentials(mockServerUrl); + + expect(result).toBeNull(); + }); + }); + + describe('removeServerCredentials', () => { + it('should remove internet credentials only', async () => { + await removeServerCredentials(mockServerUrl); + + expect(KeyChain.resetInternetCredentials).toHaveBeenCalledWith({ + server: mockServerUrl, + }); + + // Should NOT remove pre-auth secret + expect(KeyChain.resetGenericPassword).not.toHaveBeenCalled(); + }); + }); + + describe('removePreauthSecret', () => { + it('should remove pre-auth secret', async () => { + await removePreauthSecret(mockServerUrl); + + expect(KeyChain.resetGenericPassword).toHaveBeenCalledWith({ + server: mockServerUrl, + }); + }); + + it('should gracefully handle errors when pre-auth secret does not exist', async () => { + jest.mocked(KeyChain.resetGenericPassword).mockRejectedValue(new Error('Not found')); + + await expect(removePreauthSecret(mockServerUrl)).resolves.not.toThrow(); + + expect(KeyChain.resetGenericPassword).toHaveBeenCalledWith({ + server: mockServerUrl, + }); + }); + }); + + describe('getAllServerCredentials', () => { + beforeEach(() => { + Platform.OS = 'ios'; + }); + + it('should retrieve all server credentials on iOS', async () => { + const serverUrls = [ + 'https://server1.com', + 'https://server2.com', + ]; + + jest.mocked((KeyChain as any).getAllInternetPasswordServers).mockResolvedValue(serverUrls); + + jest.mocked(KeyChain.getInternetCredentials).mockImplementation(async (url) => { + if (url === 'https://server1.com') { + return { + username: 'user1', + password: 'token1', + service: url, + storage: 'keychain' as any, + }; + } else if (url === 'https://server2.com') { + return { + username: 'user2', + password: 'token2', + service: url, + storage: 'keychain' as any, + }; + } + return false; + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue(false); + + const result = await getAllServerCredentials(); + + expect(result).toHaveLength(2); + expect(result[0]).toEqual({ + serverUrl: 'https://server1.com', + userId: 'user1', + token: 'token1', + preauthSecret: undefined, + }); + expect(result[1]).toEqual({ + serverUrl: 'https://server2.com', + userId: 'user2', + token: 'token2', + preauthSecret: undefined, + }); + }); + + it('should retrieve all server credentials on Android', async () => { + Platform.OS = 'android'; + + const serverUrls = ['https://server1.com']; + + jest.mocked(KeyChain.getAllGenericPasswordServices).mockResolvedValue(serverUrls); + + jest.mocked(KeyChain.getInternetCredentials).mockResolvedValue({ + username: 'user1', + password: 'token1', + service: 'https://server1.com', + storage: 'keychain' as any, + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue(false); + + const result = await getAllServerCredentials(); + + expect(result).toHaveLength(1); + expect(KeyChain.getAllGenericPasswordServices).toHaveBeenCalled(); + expect((KeyChain as any).getAllInternetPasswordServers).not.toHaveBeenCalled(); + }); + + it('should filter out null credentials', async () => { + const serverUrls = [ + 'https://server1.com', + 'https://server2.com', + ]; + + jest.mocked((KeyChain as any).getAllInternetPasswordServers).mockResolvedValue(serverUrls); + + jest.mocked(KeyChain.getInternetCredentials).mockImplementation(async (url) => { + if (url === 'https://server1.com') { + return { + username: 'user1', + password: 'token1', + service: url, + storage: 'keychain' as any, + }; + } + return false; + }); + + jest.mocked(KeyChain.getGenericPassword).mockResolvedValue(false); + + const result = await getAllServerCredentials(); + + expect(result).toHaveLength(1); + expect(result[0].serverUrl).toBe('https://server1.com'); + }); + }); +}); diff --git a/app/init/credentials.ts b/app/init/credentials.ts index 640c7f1d8..0d949d34b 100644 --- a/app/init/credentials.ts +++ b/app/init/credentials.ts @@ -84,6 +84,9 @@ export const setServerCredentials = (serverUrl: string, token: string, preauthSe export const removeServerCredentials = async (serverUrl: string) => { await KeyChain.resetInternetCredentials({server: serverUrl}); +}; + +export const removePreauthSecret = async (serverUrl: string) => { try { await KeyChain.resetGenericPassword({server: serverUrl}); } catch (e) { @@ -91,6 +94,18 @@ export const removeServerCredentials = async (serverUrl: string) => { } }; +export const getPreauthSecret = async (serverUrl: string): Promise => { + try { + const preauthCredentials = await KeyChain.getGenericPassword({ + server: serverUrl, + }); + const secret = preauthCredentials ? preauthCredentials.password : undefined; + return secret; + } catch (e) { + return undefined; + } +}; + export const removeActiveServerCredentials = async () => { const serverUrl = await getActiveServerUrl(); if (serverUrl) {