From ba601360de8113f3ffdbbb3d77c8fe34f00faf59 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 16 Aug 2024 13:43:52 +1000 Subject: [PATCH 1/6] index on fix-headers-fileserver: 40e6cfd14 fix: fs needs headers not body --- preload.js | 1 - ts/components/inputs/SessionInput.tsx | 1 + ts/types/attachments/VisualAttachment.ts | 5 +++-- ts/window.d.ts | 1 - 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/preload.js b/preload.js index 9ee45d477..c143899f3 100644 --- a/preload.js +++ b/preload.js @@ -30,7 +30,6 @@ window.getNodeVersion = () => configAny.node_version; window.sessionFeatureFlags = { useOnionRequests: true, useTestNet: isTestNet() || isTestIntegration(), - integrationTestEnv: isTestIntegration(), useClosedGroupV3: false, debug: { debugLogging: !_.isEmpty(process.env.SESSION_DEBUG), diff --git a/ts/components/inputs/SessionInput.tsx b/ts/components/inputs/SessionInput.tsx index 78f92d6ba..e7e5a9b66 100644 --- a/ts/components/inputs/SessionInput.tsx +++ b/ts/components/inputs/SessionInput.tsx @@ -414,6 +414,7 @@ export const SessionInput = (props: Props) => { ) : ( { - if (window.sessionFeatureFlags.integrationTestEnv) { - window.log.info( + if (isTestIntegration()) { + window.log.warn( 'shorting pickFileForAvatar as it does not work in playwright/notsending the filechooser event' ); diff --git a/ts/window.d.ts b/ts/window.d.ts index e7f973264..b7281c1dc 100644 --- a/ts/window.d.ts +++ b/ts/window.d.ts @@ -34,7 +34,6 @@ declare global { useOnionRequests: boolean; useTestNet: boolean; useClosedGroupV3: boolean; - integrationTestEnv: boolean; debug: { debugLogging: boolean; debugLibsessionDumps: boolean; From e9a03bdfba28828bf1b089801f1ccb62ec3584ee Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Tue, 27 Aug 2024 15:19:49 +1000 Subject: [PATCH 2/6] fix: wait for password input in settings page --- preload.js | 12 ++++ ts/components/SessionPasswordPrompt.tsx | 5 +- ts/components/dialog/EnterPasswordModal.tsx | 55 +++++++++--------- .../section/CategoryRecoveryPassword.tsx | 33 +++-------- ts/hooks/usePasswordModal.ts | 56 +++++-------------- ts/mains/main_node.ts | 22 ++++++++ ts/util/passwordUtils.ts | 11 ++-- ts/window.d.ts | 3 +- 8 files changed, 95 insertions(+), 102 deletions(-) diff --git a/preload.js b/preload.js index c143899f3..eb1e0afd9 100644 --- a/preload.js +++ b/preload.js @@ -71,6 +71,18 @@ window.setPassword = async (passPhrase, oldPhrase) => ipc.send('set-password', passPhrase, oldPhrase); }); +// called to verify that the password is correct when showing the recovery from seed modal +window.onTryPassword = passPhrase => + new Promise((resolve, reject) => { + ipcRenderer.once('password-recovery-phrase-response', (event, error) => { + if (error) { + return reject(error); + } + return resolve(); + }); + ipcRenderer.send('password-recovery-phrase', passPhrase); + }); + window.setStartInTray = async startInTray => new Promise((resolve, reject) => { ipc.once('start-in-tray-on-start-response', (_event, error) => { diff --git a/ts/components/SessionPasswordPrompt.tsx b/ts/components/SessionPasswordPrompt.tsx index 290a21638..1599b5daa 100644 --- a/ts/components/SessionPasswordPrompt.tsx +++ b/ts/components/SessionPasswordPrompt.tsx @@ -112,10 +112,9 @@ class SessionPasswordPromptInner extends PureComponent { } public async onLogin(passPhrase: string) { - const passPhraseTrimmed = passPhrase.trim(); - + // Note: we don't trim the password anymore. If the user entered a space at the end, so be it. try { - await window.onLogin(passPhraseTrimmed); + await window.onLogin(passPhrase); } catch (error) { // Increment the error counter and show the button if necessary this.setState({ diff --git a/ts/components/dialog/EnterPasswordModal.tsx b/ts/components/dialog/EnterPasswordModal.tsx index b5df86494..c6fe3981a 100644 --- a/ts/components/dialog/EnterPasswordModal.tsx +++ b/ts/components/dialog/EnterPasswordModal.tsx @@ -2,9 +2,9 @@ import { useDispatch } from 'react-redux'; import styled from 'styled-components'; import { useRef } from 'react'; +import useAsyncFn from 'react-use/lib/useAsyncFn'; import useMount from 'react-use/lib/useMount'; import { ToastUtils } from '../../session/utils'; -import { matchesHash } from '../../util/passwordUtils'; import { updateEnterPasswordModal } from '../../state/ducks/modalDialog'; import { SpacerSM } from '../basic/Text'; @@ -18,47 +18,48 @@ const StyledModalContainer = styled.div` `; export type EnterPasswordModalProps = { - passwordHash: string; - passwordValid: boolean; setPasswordValid: (value: boolean) => void; - onClickOk?: () => any; - onClickClose?: () => any; - title?: string; + onClickOk?: () => void; + onClickClose?: () => void; }; export const EnterPasswordModal = (props: EnterPasswordModalProps) => { - const { passwordHash, setPasswordValid, onClickOk, onClickClose, title } = props; + const { setPasswordValid, onClickOk, onClickClose } = props; + const title = window.i18n('sessionRecoveryPassword'); const passwordInputRef = useRef(null); const dispatch = useDispatch(); - const onClose = () => { - if (onClickClose) { - onClickClose(); - } + const onPasswordVerified = () => { + onClickOk?.(); dispatch(updateEnterPasswordModal(null)); }; - const confirmPassword = () => { - const passwordValue = passwordInputRef.current?.value; - if (!passwordValue) { - ToastUtils.pushToastError('enterPasswordErrorToast', window.i18n('noGivenPassword')); + const [, verifyPassword] = useAsyncFn(async () => { + try { + const passwordValue = passwordInputRef.current?.value; + if (!passwordValue) { + ToastUtils.pushToastError('enterPasswordErrorToast', window.i18n('noGivenPassword')); - return; - } + return; + } - const isPasswordValid = matchesHash(passwordValue as string, passwordHash); - if (passwordHash && !isPasswordValid) { - ToastUtils.pushToastError('enterPasswordErrorToast', window.i18n('invalidPassword')); + // this throws if the password is invalid. + await window.onTryPassword(passwordValue); - return; + setPasswordValid(true); + onPasswordVerified(); + } catch (e) { + window.log.error('window.onTryPassword failed with', e); + ToastUtils.pushToastError('enterPasswordErrorToast', window.i18n('invalidPassword')); } + }); - setPasswordValid(true); - - if (onClickOk) { - void onClickOk(); + const onClose = () => { + if (onClickClose) { + onClickClose(); } + dispatch(updateEnterPasswordModal(null)); }; useMount(() => { @@ -69,7 +70,7 @@ export const EnterPasswordModal = (props: EnterPasswordModalProps) => { useHotkey('Enter', (event: KeyboardEvent) => { if (event.target === passwordInputRef.current) { - confirmPassword(); + void verifyPassword(); } }); @@ -100,7 +101,7 @@ export const EnterPasswordModal = (props: EnterPasswordModalProps) => { { - const [loadingSeed, setLoadingSeed] = useState(true); - const [recoveryPhrase, setRecoveryPhrase] = useState(''); - const [hexEncodedSeed, setHexEncodedSeed] = useState(''); + const recoveryPhrase = getCurrentRecoveryPhrase(); + if (!recoveryPhrase || isEmpty(recoveryPhrase)) { + throw new Error('SettingsCategoryRecoveryPassword recovery seed is empty'); + } + const hexEncodedSeed = mnDecode(recoveryPhrase, 'english'); const [isQRVisible, setIsQRVisible] = useState(false); const hideRecoveryPassword = useHideRecoveryPasswordEnabled(); @@ -82,27 +83,11 @@ export const SettingsCategoryRecoveryPassword = () => { const dispatch = useDispatch(); const { hasPassword, passwordValid } = usePasswordModal({ - title: window.i18n('sessionRecoveryPassword'), onClose: () => { dispatch(showSettingsSection('privacy')); }, }); - const fetchRecoverPhrase = () => { - const newRecoveryPhrase = getCurrentRecoveryPhrase(); - setRecoveryPhrase(newRecoveryPhrase); - if (!isEmpty(newRecoveryPhrase)) { - setHexEncodedSeed(mnDecode(newRecoveryPhrase, 'english')); - } - setLoadingSeed(false); - }; - - useMount(() => { - if (!hasPassword || (hasPassword && passwordValid)) { - fetchRecoverPhrase(); - } - }); - useHotkey( 'v', () => { @@ -110,10 +95,10 @@ export const SettingsCategoryRecoveryPassword = () => { setIsQRVisible(!isQRVisible); } }, - (hasPassword && !passwordValid) || loadingSeed || hideRecoveryPassword + (hasPassword && !passwordValid) || hideRecoveryPassword ); - if ((hasPassword && !passwordValid) || loadingSeed || hideRecoveryPassword) { + if ((hasPassword && !passwordValid) || hideRecoveryPassword) { return null; } diff --git a/ts/hooks/usePasswordModal.ts b/ts/hooks/usePasswordModal.ts index 3fd24f5d9..511883710 100644 --- a/ts/hooks/usePasswordModal.ts +++ b/ts/hooks/usePasswordModal.ts @@ -1,4 +1,3 @@ -import { isEmpty } from 'lodash'; import { useState } from 'react'; import { useDispatch } from 'react-redux'; import useMount from 'react-use/lib/useMount'; @@ -7,63 +6,38 @@ import { getPasswordHash } from '../util/storage'; /** * Password protection for a component if a password has been set - * @param title - Title of the password modal * @param onSuccess - Callback when password is correct * @param onClose - Callback when modal is cancelled or closed. Definitely use this if your component returns null until a password is entered * @returns An object with two properties - hasPassword which is true if a password has been set, passwordValid which is true if the password entered is correct */ export function usePasswordModal({ - title, onSuccess, onClose, }: { - title?: string; onSuccess?: () => void; onClose?: () => void; }) { - const [hasPassword, setHasPassword] = useState(false); - const [passwordHash, setPasswordHash] = useState(''); - const [passwordValid, setPasswordValid] = useState(false); - const dispatch = useDispatch(); - const validateAccess = () => { - if (!isEmpty(passwordHash)) { - return; - } + const hashFromStorage = getPasswordHash(); + const [hasPassword] = useState(!!hashFromStorage); - const hash = getPasswordHash(); - setHasPassword(!!hash); + const [passwordValid, setPasswordValid] = useState(!hasPassword); - if (hash) { - setPasswordHash(hash); - dispatch( - updateEnterPasswordModal({ - passwordHash, - passwordValid, - setPasswordValid, - onClickOk: () => { - if (onSuccess) { - onSuccess(); - } - setPasswordHash(''); - dispatch(updateEnterPasswordModal(null)); - }, - onClickClose: () => { - if (onClose) { - onClose(); - } - setPasswordHash(''); - dispatch(updateEnterPasswordModal(null)); - }, - title, - }) - ); + useMount(() => { + // if no hash is set, the user didn't set a password. + // we can just show whatever was password protected + if (!hashFromStorage || passwordValid) { + return; } - }; - useMount(() => { - validateAccess(); + dispatch( + updateEnterPasswordModal({ + setPasswordValid, + onClickOk: onSuccess, + onClickClose: onClose, + }) + ); }); return { hasPassword, passwordValid }; diff --git a/ts/mains/main_node.ts b/ts/mains/main_node.ts index b9b221bc0..64206ea48 100644 --- a/ts/mains/main_node.ts +++ b/ts/mains/main_node.ts @@ -995,6 +995,28 @@ ipc.on('password-window-login', async (event, passPhrase) => { } }); +ipc.on('password-recovery-phrase', async (event, passPhrase) => { + const sendResponse = (e: string | undefined) => { + event.sender.send('password-recovery-phrase-response', e); + }; + + try { + // Check if the hash we have stored matches the given password. + const hash = sqlNode.getPasswordHash(); + + const hashMatches = passPhrase && PasswordUtil.matchesHash(passPhrase, hash); + if (hash && !hashMatches) { + throw new Error('Invalid password'); + } + // no issues. send back undefined, meaning OK + sendResponse(undefined); + } catch (e) { + const localisedError = locale.messages.removePasswordInvalid; + // send back the error + sendResponse(localisedError); + } +}); + ipc.on('start-in-tray-on-start', (event, newValue) => { try { userConfig.set('startInTray', newValue); diff --git a/ts/util/passwordUtils.ts b/ts/util/passwordUtils.ts index d6c12d1e0..5bb9cbda5 100644 --- a/ts/util/passwordUtils.ts +++ b/ts/util/passwordUtils.ts @@ -14,27 +14,26 @@ const sha512 = (text: string) => { export const MAX_PASSWORD_LENGTH = 64; -export const generateHash = (phrase: string) => phrase && sha512(phrase.trim()); +export const generateHash = (phrase: string) => phrase && sha512(phrase); export const matchesHash = (phrase: string | null, hash: string) => - phrase && sha512(phrase.trim()) === hash.trim(); + phrase && sha512(phrase) === hash; export const validatePassword = (phrase: string) => { if (typeof phrase !== 'string') { return window?.i18n ? window?.i18n('passwordTypeError') : ERRORS.TYPE; } - const trimmed = phrase.trim(); - if (trimmed.length === 0) { + if (phrase.length === 0) { return window?.i18n ? window?.i18n('noGivenPassword') : ERRORS.LENGTH; } - if (trimmed.length < 6 || trimmed.length > MAX_PASSWORD_LENGTH) { + if (phrase.length < 6 || phrase.length > MAX_PASSWORD_LENGTH) { return window?.i18n ? window?.i18n('passwordLengthError') : ERRORS.LENGTH; } // Restrict characters to letters, numbers and symbols const characterRegex = /^[a-zA-Z0-9-!?/\\()._`~@#$%^&*+=[\]{}|<>,;: ]+$/; - if (!characterRegex.test(trimmed)) { + if (!characterRegex.test(phrase)) { return window?.i18n ? window?.i18n('passwordCharacterError') : ERRORS.CHARACTER; } diff --git a/ts/window.d.ts b/ts/window.d.ts index b7281c1dc..9d3eef2af 100644 --- a/ts/window.d.ts +++ b/ts/window.d.ts @@ -42,7 +42,8 @@ declare global { debugOnionRequests: boolean; }; }; - onLogin: (pw: string) => Promise; + onLogin: (pw: string) => Promise; // only set on the password window + onTryPassword: (pw: string) => Promise; // only set on the main window persistStore?: Persistor; restart: () => void; getSeedNodeList: () => Array | undefined; From 7a3a80825bda0dc981a90224e340f95405120549 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Tue, 27 Aug 2024 17:10:40 +1000 Subject: [PATCH 3/6] fix: onboarding job runner and min snode count for broken testnet --- ts/session/apis/snode_api/onsResolve.ts | 1 + ts/session/apis/snode_api/snodePool.ts | 3 +- .../disappearing_messages/timerOptions.ts | 4 +-- ts/session/onions/onionPath.ts | 8 ++---- ts/session/utils/job_runners/PersistedJob.ts | 14 +++++++++- .../job_runners/jobs/ConfigurationSyncJob.ts | 28 +++++-------------- ts/shared/env_vars.ts | 9 ++---- 7 files changed, 31 insertions(+), 36 deletions(-) diff --git a/ts/session/apis/snode_api/onsResolve.ts b/ts/session/apis/snode_api/onsResolve.ts index f15c2054b..ad31bb3f2 100644 --- a/ts/session/apis/snode_api/onsResolve.ts +++ b/ts/session/apis/snode_api/onsResolve.ts @@ -39,6 +39,7 @@ async function getSessionIDForOnsName(onsNameCase: string) { if (isTestNet()) { window.log.info('OnsResolve response are not registered to anything on testnet'); + throw new Error('OnsResolve response are not registered to anything on testnet'); } const onsResolveRequests = buildOnsResolveRequests(base64EncodedNameHash); diff --git a/ts/session/apis/snode_api/snodePool.ts b/ts/session/apis/snode_api/snodePool.ts index 788b280e8..a0c724905 100644 --- a/ts/session/apis/snode_api/snodePool.ts +++ b/ts/session/apis/snode_api/snodePool.ts @@ -10,6 +10,7 @@ import { ed25519Str } from '../../utils/String'; import { SeedNodeAPI } from '../seed_node_api'; import { ServiceNodesList } from './getServiceNodesList'; import { requestSnodesForPubkeyFromNetwork } from './getSwarmFor'; +import { minimumGuardCount, ONION_REQUEST_HOPS } from '../../onions/onionPath'; /** * If we get less than this snode in a swarm, we fetch new snodes for this pubkey @@ -20,7 +21,7 @@ const minSwarmSnodeCount = 3; * If we get less than minSnodePoolCount we consider that we need to fetch the new snode pool from a seed node * and not from those snodes. */ -export const minSnodePoolCount = 12; +export const minSnodePoolCount = minimumGuardCount * (ONION_REQUEST_HOPS + 1) * 2; /** * If we get less than this amount of snodes (24), lets try to get an updated list from those while we can diff --git a/ts/session/disappearing_messages/timerOptions.ts b/ts/session/disappearing_messages/timerOptions.ts index b5b93aa2f..1dc1267f3 100644 --- a/ts/session/disappearing_messages/timerOptions.ts +++ b/ts/session/disappearing_messages/timerOptions.ts @@ -1,5 +1,5 @@ import moment from 'moment'; -import { isCI, isDevProd } from '../../shared/env_vars'; +import { isDevProd } from '../../shared/env_vars'; import { LocalizerKeys } from '../../types/LocalizerKeys'; type TimerOptionsEntry = { name: string; value: number }; @@ -67,7 +67,7 @@ const VALUES: Array = timerOptionsDurations.map(t => { }); const filterOutDebugValues = (option: number) => { - return isDevProd() || isCI() || option > 60; // when not a dev build nor on CI, filter out options with less than 60s + return isDevProd() || option > 60; // when not a dev build, filter out options with less than 60s }; const DELETE_AFTER_READ = VALUES.filter(option => { diff --git a/ts/session/onions/onionPath.ts b/ts/session/onions/onionPath.ts index a52aa9921..ee0d9f280 100644 --- a/ts/session/onions/onionPath.ts +++ b/ts/session/onions/onionPath.ts @@ -18,9 +18,9 @@ import { UserUtils } from '../utils'; import { allowOnlyOneAtATime } from '../utils/Promise'; import { ed25519Str } from '../utils/String'; -const desiredGuardCount = 3; -const minimumGuardCount = 2; -const ONION_REQUEST_HOPS = 3; +export const desiredGuardCount = 2; +export const minimumGuardCount = 1; +export const ONION_REQUEST_HOPS = 3; export function getOnionPathMinTimeout() { return DURATION.SECONDS; @@ -31,9 +31,7 @@ export let onionPaths: Array> = []; /** * Used for testing only * @returns a copy of the onion path currently used by the app. - * */ - export const TEST_getTestOnionPath = () => { return _.cloneDeep(onionPaths); }; diff --git a/ts/session/utils/job_runners/PersistedJob.ts b/ts/session/utils/job_runners/PersistedJob.ts index 39ccef90f..0ad07b98a 100644 --- a/ts/session/utils/job_runners/PersistedJob.ts +++ b/ts/session/utils/job_runners/PersistedJob.ts @@ -108,7 +108,19 @@ export abstract class PersistedJob { public async runJob() { if (!this.runningPromise) { - this.runningPromise = this.run(); + // eslint-disable-next-line more/no-then + this.runningPromise = this.run() + .then(jobResult => { + this.runningPromise = null; + return jobResult; + }) + .catch(e => { + window.log.warn( + 'runJob() threw. this cannot happen, but rehtrowing as this should be handled in each jobs run()', + e + ); + throw e; + }); } return this.runningPromise; } diff --git a/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts b/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts index f2dbdfe20..0fe1a7fbd 100644 --- a/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts +++ b/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts @@ -23,8 +23,8 @@ import { } from '../PersistedJob'; import { DURATION } from '../../../constants'; -const defaultMsBetweenRetries = 15000; // a long time between retries, to avoid running multiple jobs at the same time, when one was postponed at the same time as one already planned (5s) -const defaultMaxAttempts = 2; +const defaultMsBetweenRetries = 5000; // a long time between retries, to avoid running multiple jobs at the same time, when one was postponed at the same time as one already planned (5s) +const defaultMaxAttempts = 4; /** * We want to run each of those jobs at least 3seconds apart. @@ -56,8 +56,6 @@ async function retrieveSingleDestinationChanges( return { messages: outgoingConfResults, allOldHashes: compactedHashes }; } -let firstJobStart: number | undefined; - /** * This function is run once we get the results from the multiple batch-send. */ @@ -194,18 +192,6 @@ class ConfigurationSyncJob extends PersistedJob return RunJobResult.Success; } const singleDestChanges = await retrieveSingleDestinationChanges(thisJobDestination); - if (!firstJobStart) { - firstJobStart = Date.now(); - } - - // not ideal, but we need to postpone the first sync job to after we've handled the incoming config messages - // otherwise we are pushing an incomplete config to the network, which will need to be merged and that action alone - // will bump the timestamp of the config. - // We rely on the timestamp of configs to know when to drop messages that would unhide/unremove a conversation. - // The whole thing is a dirty fix of a dirty fix, that will **eventually** need proper fixing - if (Date.now() - firstJobStart <= 20 * DURATION.SECONDS) { - return RunJobResult.RetryJobIfPossible; - } // If there are no pending changes then the job can just complete (next time something // is updated we want to try and run immediately so don't scuedule another run in this case) @@ -332,18 +318,18 @@ async function queueNewJobIfNeeded() { !lastRunConfigSyncJobTimestamp || lastRunConfigSyncJobTimestamp < Date.now() - defaultMsBetweenRetries ) { - // window.log.debug('Scheduling ConfSyncJob: ASAP'); - // we postpone by 1000ms to make sure whoever is adding this job is done with what is needs to do first + // Note: we postpone by 3s for two reasons: + // - to make sure whoever is adding this job is done with what is needs to do first + // - to allow a just created device to process incoming config messages before pushing a new one // this call will make sure that there is only one configuration sync job at all times await runners.configurationSyncRunner.addJob( - new ConfigurationSyncJob({ nextAttemptTimestamp: Date.now() + 1000 }) + new ConfigurationSyncJob({ nextAttemptTimestamp: Date.now() + 3 * DURATION.SECONDS }) ); } else { // if we did run at t=100, and it is currently t=110, the difference is 10 const diff = Math.max(Date.now() - lastRunConfigSyncJobTimestamp, 0); // but we want to run every 30, so what we need is actually `30-10` from now = 20 - const leftBeforeNextTick = Math.max(defaultMsBetweenRetries - diff, 1000); - // window.log.debug('Scheduling ConfSyncJob: LATER'); + const leftBeforeNextTick = Math.max(defaultMsBetweenRetries - diff, DURATION.SECONDS); await runners.configurationSyncRunner.addJob( new ConfigurationSyncJob({ nextAttemptTimestamp: Date.now() + leftBeforeNextTick }) diff --git a/ts/shared/env_vars.ts b/ts/shared/env_vars.ts index d87b8b428..8be7390e5 100644 --- a/ts/shared/env_vars.ts +++ b/ts/shared/env_vars.ts @@ -5,19 +5,16 @@ function envAppInstanceIncludes(prefix: string) { return !!process.env.NODE_APP_INSTANCE.includes(prefix); } -export function isCI() { - // this is set by session-playwright to run a build on CI - return !!process.env.CI; -} + export function isDevProd() { return envAppInstanceIncludes('devprod'); } export function isTestNet() { - return envAppInstanceIncludes('testnet') || isCI(); // when running on CI, we always want to use testnet + return envAppInstanceIncludes('testnet'); } export function isTestIntegration() { - return envAppInstanceIncludes('test-integration') || isCI(); // when running on CI, we always want the 'test-integration' behavior + return envAppInstanceIncludes('test-integration'); } From 96e69d13be4a3d963f6a934b0f2f2fe472954723 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 28 Aug 2024 10:06:31 +1000 Subject: [PATCH 4/6] test: fix unit tests --- ts/shared/env_vars.ts | 2 -- ts/test/session/unit/onion/GuardNodes_test.ts | 33 +++++++++---------- .../unit/onion/SnodePoolUpdate_test.ts | 8 ++--- .../unit/utils/job_runner/JobRunner_test.ts | 11 +++++-- 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/ts/shared/env_vars.ts b/ts/shared/env_vars.ts index 8be7390e5..8d95a89cd 100644 --- a/ts/shared/env_vars.ts +++ b/ts/shared/env_vars.ts @@ -5,8 +5,6 @@ function envAppInstanceIncludes(prefix: string) { return !!process.env.NODE_APP_INSTANCE.includes(prefix); } - - export function isDevProd() { return envAppInstanceIncludes('devprod'); } diff --git a/ts/test/session/unit/onion/GuardNodes_test.ts b/ts/test/session/unit/onion/GuardNodes_test.ts index a43680eb0..1295e8a69 100644 --- a/ts/test/session/unit/onion/GuardNodes_test.ts +++ b/ts/test/session/unit/onion/GuardNodes_test.ts @@ -14,6 +14,7 @@ import { } from '../../../test-utils/utils'; import { SeedNodeAPI } from '../../../../session/apis/seed_node_api'; import { Snode } from '../../../../data/types'; +import { minSnodePoolCount } from '../../../../session/apis/snode_api/snodePool'; chai.use(chaiAsPromised as any); chai.should(); @@ -51,7 +52,7 @@ describe('GuardNodes', () => { Sinon.restore(); }); - it('does not fetch from seed if we got 12 or more snodes in the db', async () => { + it('does not fetch from seed if we got 8 or more snodes in the db', async () => { stubData('getSnodePoolFromDb').resolves(fakeSnodePool); getSnodePoolFromDBOrFetchFromSeed = Sinon.stub( @@ -76,14 +77,12 @@ describe('GuardNodes', () => { fetchFromSeedWithRetriesAndWriteToDb.callCount, 'fetchFromSeedWithRetriesAndWriteToDb should not have been called' ).to.be.eq(0); - expect( - testGuardNode.callCount, - 'firstGuardNode should have been called three times' - ).to.be.eq(3); + expect(testGuardNode.callCount, 'testGuardNode should have been called two times').to.be.eq( + 2 + ); // this should be desiredGuardCount const firstGuardNode = testGuardNode.firstCall.args[0]; const secondGuardNode = testGuardNode.secondCall.args[0]; - const thirdGuardNode = testGuardNode.thirdCall.args[0]; - expect(fetchedGuardNodes).to.deep.equal([firstGuardNode, secondGuardNode, thirdGuardNode]); + expect(fetchedGuardNodes).to.deep.equal([firstGuardNode, secondGuardNode]); }); it('throws an error if we got enough snodes in the db but none test passes', async () => { @@ -116,10 +115,9 @@ describe('GuardNodes', () => { fetchFromSeedWithRetriesAndWriteToDb.callCount, 'fetchFromSeedWithRetriesAndWriteToDb should not have been called' ).to.be.eq(0); - expect( - testGuardNode.callCount, - 'firstGuardNode should have been called three times' - ).to.be.eq(18); + expect(testGuardNode.callCount, 'testGuardNode should have been called 12 times').to.be.eq( + 12 + ); expect(throwedError).to.be.equal('selectGuardNodes stopping after attempts: 6'); }); @@ -168,13 +166,14 @@ describe('GuardNodes', () => { // run the command const guardNodes = await OnionPaths.selectGuardNodes(); - expect(guardNodes.length).to.be.equal(3); - expect(testGuardNode.callCount).to.be.equal(3); + expect(guardNodes.length).to.be.equal(2); + expect(testGuardNode.callCount).to.be.equal(2); }); it('throws if we have to fetch from seed, fetch from seed but not have enough fetched snodes', async () => { - const invalidSndodePool = fakeSnodePool.slice(0, 11); - stubData('getSnodePoolFromDb').resolves(invalidSndodePool); + const invalidLength = minSnodePoolCount - 1; + const invalidSnodePool = fakeSnodePool.slice(0, invalidLength); + stubData('getSnodePoolFromDb').resolves(invalidSnodePool); TestUtils.stubWindow('getSeedNodeList', () => [{ url: 'whatever' }]); getSnodePoolFromDBOrFetchFromSeed = Sinon.stub( @@ -184,7 +183,7 @@ describe('GuardNodes', () => { fetchFromSeedWithRetriesAndWriteToDb = Sinon.stub( SeedNodeAPI, 'fetchSnodePoolFromSeedNodeWithRetries' - ).resolves(invalidSndodePool); + ).resolves(invalidSnodePool); stubData('updateGuardNodes').resolves(); // run the command @@ -195,7 +194,7 @@ describe('GuardNodes', () => { throwedError = e.message; } expect(throwedError).to.be.equal( - 'Could not select guard nodes. Not enough nodes in the pool: 11' + 'Could not select guard nodes. Not enough nodes in the pool: 7' // this is invalidLength but we want this test to fail if we change minSnodePoolCount ); }); }); diff --git a/ts/test/session/unit/onion/SnodePoolUpdate_test.ts b/ts/test/session/unit/onion/SnodePoolUpdate_test.ts index 7f6d8cdde..7f141da27 100644 --- a/ts/test/session/unit/onion/SnodePoolUpdate_test.ts +++ b/ts/test/session/unit/onion/SnodePoolUpdate_test.ts @@ -67,10 +67,10 @@ describe('OnionPaths', () => { expect(fetched).to.deep.equal(fakeSnodePool); }); - it('if the cached snode pool 12 or less snodes, trigger a fetch from the seed nodes', async () => { - const length12 = fakeSnodePool.slice(0, 12); - expect(length12.length).to.eq(12); - getSnodePoolFromDb = stubData('getSnodePoolFromDb').resolves(length12); + it('if the cached snode pool 8 or less snodes, trigger a fetch from the seed nodes', async () => { + const length8 = fakeSnodePool.slice(0, 8); + expect(length8.length).to.eq(8); + getSnodePoolFromDb = stubData('getSnodePoolFromDb').resolves(length8); stubData('updateSnodePoolOnDb').resolves(); fetchFromSeedWithRetriesAndWriteToDb = Sinon.stub( diff --git a/ts/test/session/unit/utils/job_runner/JobRunner_test.ts b/ts/test/session/unit/utils/job_runner/JobRunner_test.ts index 121358a0d..090b3d377 100644 --- a/ts/test/session/unit/utils/job_runner/JobRunner_test.ts +++ b/ts/test/session/unit/utils/job_runner/JobRunner_test.ts @@ -271,6 +271,7 @@ describe('JobRunner', () => { it('adding one job after the first is done schedules it', async () => { await runnerMulti.loadJobsFromDb(); + TestUtils.stubWindowLog(); const job = getFakeSleepForMultiJob({ timestamp: 100 }); runnerMulti.startProcessing(); clock.tick(110); @@ -303,6 +304,8 @@ describe('JobRunner', () => { await job2.waitForCurrentTry(); await runnerMulti.waitCurrentJob(); + // we need to give sometime to the jobrunner to handle the return of job2 and remove it + await sleepFor(100); expect(runnerMulti.getJobList()).to.deep.eq([]); }); @@ -401,7 +404,7 @@ describe('JobRunner', () => { currentRetry: 1, }; // just give time for the runnerMulti to pick up a new job - await sleepFor(10); + await sleepFor(100); // the job failed, so the job should still be there expect(runnerMulti.getJobList()).to.deep.eq([jobUpdated]); @@ -409,14 +412,16 @@ describe('JobRunner', () => { // that job should be retried now clock.tick(11000); await runner.waitCurrentJob(); + await runnerMulti.waitCurrentJob(); + const jobUpdated2 = { ...job.serializeJob(), - nextAttemptTimestamp: clock.now + 10000, + nextAttemptTimestamp: clock.now + job.persistedData.delayBetweenRetries, currentRetry: 2, }; + await sleepFor(10); - await runnerMulti.waitCurrentJob(); expect(runnerMulti.getJobList()).to.deep.eq([jobUpdated2]); // that job should be retried one more time and then removed from the list of jobs to be run From 996db72f96f405d5556fa12f64ba14723e08204d Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 28 Aug 2024 14:08:20 +1000 Subject: [PATCH 5/6] chore: address PR comments --- preload.js | 4 ++-- .../settings/section/CategoryRecoveryPassword.tsx | 3 +-- ts/session/constants.ts | 11 +++++++++++ .../utils/job_runners/jobs/ConfigurationSyncJob.ts | 4 ++-- ts/test/session/unit/onion/GuardNodes_test.ts | 3 ++- ts/test/session/unit/onion/SnodePoolUpdate_test.ts | 2 +- .../session/unit/utils/job_runner/JobRunner_test.ts | 2 +- ts/util/passwordUtils.ts | 4 ++-- 8 files changed, 22 insertions(+), 11 deletions(-) diff --git a/preload.js b/preload.js index eb1e0afd9..801364155 100644 --- a/preload.js +++ b/preload.js @@ -72,9 +72,9 @@ window.setPassword = async (passPhrase, oldPhrase) => }); // called to verify that the password is correct when showing the recovery from seed modal -window.onTryPassword = passPhrase => +window.onTryPassword = async passPhrase => new Promise((resolve, reject) => { - ipcRenderer.once('password-recovery-phrase-response', (event, error) => { + ipcRenderer.once('password-recovery-phrase-response', (_event, error) => { if (error) { return reject(error); } diff --git a/ts/components/settings/section/CategoryRecoveryPassword.tsx b/ts/components/settings/section/CategoryRecoveryPassword.tsx index 06dd9120f..a56154db6 100644 --- a/ts/components/settings/section/CategoryRecoveryPassword.tsx +++ b/ts/components/settings/section/CategoryRecoveryPassword.tsx @@ -1,4 +1,3 @@ -import { isEmpty } from 'lodash'; import { useState } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import styled from 'styled-components'; @@ -68,7 +67,7 @@ const qrLogoProps: QRCodeLogoProps = { export const SettingsCategoryRecoveryPassword = () => { const recoveryPhrase = getCurrentRecoveryPhrase(); - if (!recoveryPhrase || isEmpty(recoveryPhrase)) { + if (!recoveryPhrase) { throw new Error('SettingsCategoryRecoveryPassword recovery seed is empty'); } const hexEncodedSeed = mnDecode(recoveryPhrase, 'english'); diff --git a/ts/session/constants.ts b/ts/session/constants.ts index fa3782faf..d1e5eb7c6 100644 --- a/ts/session/constants.ts +++ b/ts/session/constants.ts @@ -92,3 +92,14 @@ export const ONBOARDING_TIMES = { /** 0.2 seconds */ RECOVERY_FINISHED: 0.2 * DURATION.SECONDS, }; + +export const PASSWORD_LENGTH = { + /** + * 6 chars + */ + MIN_PASSWORD_LEN: 6, + /** + * 64 chars + */ + MAX_PASSWORD_LEN: 64, +}; diff --git a/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts b/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts index 0fe1a7fbd..3350d4070 100644 --- a/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts +++ b/ts/session/utils/job_runners/jobs/ConfigurationSyncJob.ts @@ -23,7 +23,7 @@ import { } from '../PersistedJob'; import { DURATION } from '../../../constants'; -const defaultMsBetweenRetries = 5000; // a long time between retries, to avoid running multiple jobs at the same time, when one was postponed at the same time as one already planned (5s) +const defaultMsBetweenRetries = 5 * DURATION.SECONDS; // a long time between retries, to avoid running multiple jobs at the same time, when one was postponed at the same time as one already planned (5s) const defaultMaxAttempts = 4; /** @@ -320,7 +320,7 @@ async function queueNewJobIfNeeded() { ) { // Note: we postpone by 3s for two reasons: // - to make sure whoever is adding this job is done with what is needs to do first - // - to allow a just created device to process incoming config messages before pushing a new one + // - to allow a recently created device to process incoming config messages before pushing a new one // this call will make sure that there is only one configuration sync job at all times await runners.configurationSyncRunner.addJob( new ConfigurationSyncJob({ nextAttemptTimestamp: Date.now() + 3 * DURATION.SECONDS }) diff --git a/ts/test/session/unit/onion/GuardNodes_test.ts b/ts/test/session/unit/onion/GuardNodes_test.ts index 1295e8a69..dee95c772 100644 --- a/ts/test/session/unit/onion/GuardNodes_test.ts +++ b/ts/test/session/unit/onion/GuardNodes_test.ts @@ -52,7 +52,7 @@ describe('GuardNodes', () => { Sinon.restore(); }); - it('does not fetch from seed if we got 8 or more snodes in the db', async () => { + it('does not fetch from seed if we have 8 or more snodes in the db', async () => { stubData('getSnodePoolFromDb').resolves(fakeSnodePool); getSnodePoolFromDBOrFetchFromSeed = Sinon.stub( @@ -166,6 +166,7 @@ describe('GuardNodes', () => { // run the command const guardNodes = await OnionPaths.selectGuardNodes(); + // 2 because our desiredGuardCount is 2 (not putting the variable to make the test fails if we ever change it) expect(guardNodes.length).to.be.equal(2); expect(testGuardNode.callCount).to.be.equal(2); }); diff --git a/ts/test/session/unit/onion/SnodePoolUpdate_test.ts b/ts/test/session/unit/onion/SnodePoolUpdate_test.ts index 7f141da27..afce0a793 100644 --- a/ts/test/session/unit/onion/SnodePoolUpdate_test.ts +++ b/ts/test/session/unit/onion/SnodePoolUpdate_test.ts @@ -67,7 +67,7 @@ describe('OnionPaths', () => { expect(fetched).to.deep.equal(fakeSnodePool); }); - it('if the cached snode pool 8 or less snodes, trigger a fetch from the seed nodes', async () => { + it('if the cached snode pool is 8 or less snodes, trigger a fetch from the seed nodes', async () => { const length8 = fakeSnodePool.slice(0, 8); expect(length8.length).to.eq(8); getSnodePoolFromDb = stubData('getSnodePoolFromDb').resolves(length8); diff --git a/ts/test/session/unit/utils/job_runner/JobRunner_test.ts b/ts/test/session/unit/utils/job_runner/JobRunner_test.ts index 090b3d377..1c6d0ef70 100644 --- a/ts/test/session/unit/utils/job_runner/JobRunner_test.ts +++ b/ts/test/session/unit/utils/job_runner/JobRunner_test.ts @@ -304,7 +304,7 @@ describe('JobRunner', () => { await job2.waitForCurrentTry(); await runnerMulti.waitCurrentJob(); - // we need to give sometime to the jobrunner to handle the return of job2 and remove it + // we need to give some time for the jobrunner to handle the return of job2 and remove it await sleepFor(100); expect(runnerMulti.getJobList()).to.deep.eq([]); diff --git a/ts/util/passwordUtils.ts b/ts/util/passwordUtils.ts index 5bb9cbda5..bec0e4415 100644 --- a/ts/util/passwordUtils.ts +++ b/ts/util/passwordUtils.ts @@ -1,4 +1,5 @@ import * as crypto from 'crypto'; +import { PASSWORD_LENGTH } from '../session/constants'; const ERRORS = { TYPE: 'Password must be a string', @@ -12,7 +13,6 @@ const sha512 = (text: string) => { return hash.digest('hex'); }; -export const MAX_PASSWORD_LENGTH = 64; export const generateHash = (phrase: string) => phrase && sha512(phrase); export const matchesHash = (phrase: string | null, hash: string) => @@ -27,7 +27,7 @@ export const validatePassword = (phrase: string) => { return window?.i18n ? window?.i18n('noGivenPassword') : ERRORS.LENGTH; } - if (phrase.length < 6 || phrase.length > MAX_PASSWORD_LENGTH) { + if (phrase.length < PASSWORD_LENGTH.MIN_PASSWORD_LEN || phrase.length > PASSWORD_LENGTH.MAX_PASSWORD_LEN) { return window?.i18n ? window?.i18n('passwordLengthError') : ERRORS.LENGTH; } From 6068b63afc48f931a2be7a857058a197692d3ae5 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Wed, 28 Aug 2024 14:27:30 +1000 Subject: [PATCH 6/6] fix: fix remove password show errors --- preload.js | 6 +++++- ts/components/dialog/SessionSetPasswordDialog.tsx | 11 ++++++++++- ts/session/utils/job_runners/PersistedJob.ts | 2 +- ts/window.d.ts | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/preload.js b/preload.js index 801364155..72dd8a78a 100644 --- a/preload.js +++ b/preload.js @@ -64,7 +64,11 @@ window.setPassword = async (passPhrase, oldPhrase) => new Promise((resolve, reject) => { ipc.once('set-password-response', (_event, response) => { if (!response) { - return reject('window.setPassword: No response from main process'); + // We don't reject here, but return undefined and handle the result in the caller. + // The reason is because we sometimes want to reject, but sometimes not depending on what the caller is doing (set/change/remove) + // For instance, removing a password makes `!response` true, and so we would reject here even if we + // technically didn't have an reason to. + return resolve(undefined); } return resolve(response); }); diff --git a/ts/components/dialog/SessionSetPasswordDialog.tsx b/ts/components/dialog/SessionSetPasswordDialog.tsx index f35466916..e032f9a89 100644 --- a/ts/components/dialog/SessionSetPasswordDialog.tsx +++ b/ts/components/dialog/SessionSetPasswordDialog.tsx @@ -195,6 +195,9 @@ export class SessionSetPasswordDialog extends Component { } try { const updatedHash = await window.setPassword(enteredPassword, null); + if (!updatedHash) { + throw new Error('window.setPassword expected updatedHash to be set for actionSet'); + } await Storage.put('passHash', updatedHash); ToastUtils.pushToastSuccess( @@ -245,6 +248,9 @@ export class SessionSetPasswordDialog extends Component { try { const updatedHash = await window.setPassword(newPassword, oldPassword); + if (!updatedHash) { + throw new Error('window.setPassword expected updatedHash to be set for actionChange'); + } await Storage.put('passHash', updatedHash); ToastUtils.pushToastSuccess( @@ -276,7 +282,10 @@ export class SessionSetPasswordDialog extends Component { } try { - await window.setPassword(null, oldPassword); + const updatedHash = await window.setPassword(null, oldPassword); + if (updatedHash) { + throw new Error('window.setPassword expected updatedHash to be unset for actionRemove'); + } await Storage.remove('passHash'); ToastUtils.pushToastWarning( diff --git a/ts/session/utils/job_runners/PersistedJob.ts b/ts/session/utils/job_runners/PersistedJob.ts index 0ad07b98a..022ce461f 100644 --- a/ts/session/utils/job_runners/PersistedJob.ts +++ b/ts/session/utils/job_runners/PersistedJob.ts @@ -116,7 +116,7 @@ export abstract class PersistedJob { }) .catch(e => { window.log.warn( - 'runJob() threw. this cannot happen, but rehtrowing as this should be handled in each jobs run()', + 'runJob() threw. this cannot happen, but rethrowing as this should be handled in each jobs run()', e ); throw e; diff --git a/ts/window.d.ts b/ts/window.d.ts index 9d3eef2af..158b4fe16 100644 --- a/ts/window.d.ts +++ b/ts/window.d.ts @@ -47,7 +47,7 @@ declare global { persistStore?: Persistor; restart: () => void; getSeedNodeList: () => Array | undefined; - setPassword: (newPassword: string | null, oldPassword: string | null) => Promise; + setPassword: (newPassword: string | null, oldPassword: string | null) => Promise; isOnline: boolean; toggleMediaPermissions: () => Promise; toggleCallMediaPermissionsTo: (enabled: boolean) => Promise;