From df42cccce72cc2a482665503f5e5a35ae9023a07 Mon Sep 17 00:00:00 2001 From: Francesco Truzzi Date: Wed, 23 Oct 2024 10:38:33 +0200 Subject: [PATCH 1/7] Add Fineco to banks with limited history (#481) * Add Fineco to banks with limited history * update release notes --- src/app-gocardless/bank-factory.js | 2 ++ upcoming-release-notes/481.md | 6 ++++++ 2 files changed, 8 insertions(+) create mode 100644 upcoming-release-notes/481.md diff --git a/src/app-gocardless/bank-factory.js b/src/app-gocardless/bank-factory.js index 2bf09e1ec..dd38be563 100644 --- a/src/app-gocardless/bank-factory.js +++ b/src/app-gocardless/bank-factory.js @@ -75,6 +75,8 @@ export const BANKS_WITH_LIMITED_HISTORY = [ 'CESKA_SPORITELNA_LONG_GIBACZPX', 'COOP_EKRDEE22', 'DOTS_HYEEIT22', + 'FINECO_FEBIITM2XXX', + 'FINECO_UK_FEBIITM2XXX', 'HYPE_BUSINESS_HYEEIT22', 'HYPE_HYEEIT2', 'ILLIMITY_ITTPIT2M', diff --git a/upcoming-release-notes/481.md b/upcoming-release-notes/481.md new file mode 100644 index 000000000..ceced69ea --- /dev/null +++ b/upcoming-release-notes/481.md @@ -0,0 +1,6 @@ +--- +category: Enhancements +authors: [ftruzzi] +--- + +Add "Fineco" bank (IT, UK) to list of banks with limited history From f6f49b1fe72dd1e12d1a54340fd8c9357e4b5a1b Mon Sep 17 00:00:00 2001 From: Nuno Date: Wed, 23 Oct 2024 10:39:10 +0200 Subject: [PATCH 2/7] refactor: set correct log level (#478) * refactor: set correct log level Signed-off-by: rare-magma * docs: set correct pr number Signed-off-by: rare-magma --------- Signed-off-by: rare-magma --- src/app-gocardless/banks/integration-bank.js | 6 +++--- src/app-gocardless/banks/tests/integration-bank.spec.js | 2 +- upcoming-release-notes/478.md | 6 ++++++ 3 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 upcoming-release-notes/478.md diff --git a/src/app-gocardless/banks/integration-bank.js b/src/app-gocardless/banks/integration-bank.js index cbe6cd57f..2b4f358e3 100644 --- a/src/app-gocardless/banks/integration-bank.js +++ b/src/app-gocardless/banks/integration-bank.js @@ -28,7 +28,7 @@ export default { accessValidForDays: 90, normalizeAccount(account) { - console.log( + console.debug( 'Available account properties for new institution integration', { account: JSON.stringify(account) }, ); @@ -66,7 +66,7 @@ export default { }, sortTransactions(transactions = []) { - console.log( + console.debug( 'Available (first 10) transactions properties for new integration of institution in sortTransactions function', { top10Transactions: JSON.stringify(transactions.slice(0, 10)) }, ); @@ -74,7 +74,7 @@ export default { }, calculateStartingBalance(sortedTransactions = [], balances = []) { - console.log( + console.debug( 'Available (first 10) transactions properties for new integration of institution in calculateStartingBalance function', { balances: JSON.stringify(balances), diff --git a/src/app-gocardless/banks/tests/integration-bank.spec.js b/src/app-gocardless/banks/tests/integration-bank.spec.js index 7f1f235d3..f244d00a9 100644 --- a/src/app-gocardless/banks/tests/integration-bank.spec.js +++ b/src/app-gocardless/banks/tests/integration-bank.spec.js @@ -9,7 +9,7 @@ describe('IntegrationBank', () => { let consoleSpy; beforeEach(() => { - consoleSpy = jest.spyOn(console, 'log'); + consoleSpy = jest.spyOn(console, 'debug'); }); describe('normalizeAccount', () => { diff --git a/upcoming-release-notes/478.md b/upcoming-release-notes/478.md new file mode 100644 index 000000000..472eb8763 --- /dev/null +++ b/upcoming-release-notes/478.md @@ -0,0 +1,6 @@ +--- +category: Maintenance +authors: [rare-magma] +--- + +Set correct log level for bank integration messages From b8c92b98b83dcab85530e2626ffc8abd4a2cefa6 Mon Sep 17 00:00:00 2001 From: alcroito Date: Wed, 23 Oct 2024 10:46:24 +0200 Subject: [PATCH 3/7] Add "N26" to banks with limited history (#473) --- src/app-gocardless/bank-factory.js | 1 + upcoming-release-notes/473.md | 6 ++++++ 2 files changed, 7 insertions(+) create mode 100644 upcoming-release-notes/473.md diff --git a/src/app-gocardless/bank-factory.js b/src/app-gocardless/bank-factory.js index dd38be563..c7e2966e1 100644 --- a/src/app-gocardless/bank-factory.js +++ b/src/app-gocardless/bank-factory.js @@ -92,6 +92,7 @@ export const BANKS_WITH_LIMITED_HISTORY = [ 'LUMINOR_RIKOLV2X', 'MEDICINOSBANK_MDBALT22XXX', 'NORDEA_NDEADKKK', + 'N26_NTSBDEB1', 'OPYN_BITAITRRB2B', 'PAYTIPPER_PAYTITM1', 'REVOLUT_REVOLT21', diff --git a/upcoming-release-notes/473.md b/upcoming-release-notes/473.md new file mode 100644 index 000000000..71525a527 --- /dev/null +++ b/upcoming-release-notes/473.md @@ -0,0 +1,6 @@ +--- +category: Enhancements +authors: [alcroito] +--- + +Add "N26" to list of banks with limited history From 49c5adc9cf5705ff2658d195b0b2e1c70f7742e4 Mon Sep 17 00:00:00 2001 From: Koen van Staveren Date: Wed, 23 Oct 2024 10:53:12 +0200 Subject: [PATCH 4/7] feat: sort transactions better (#470) * feat: sort transactions better * chore: release note * chore: pr feedback --- src/app-gocardless/tests/utils.spec.js | 125 +++++++++++++++++++++++++ src/app-gocardless/utils.js | 39 +++++++- upcoming-release-notes/470.md | 6 ++ 3 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 upcoming-release-notes/470.md diff --git a/src/app-gocardless/tests/utils.spec.js b/src/app-gocardless/tests/utils.spec.js index 352525b9d..9d43448d9 100644 --- a/src/app-gocardless/tests/utils.spec.js +++ b/src/app-gocardless/tests/utils.spec.js @@ -33,5 +33,130 @@ describe('utils', () => { }, ]); }); + + it('should sort by valueDate if bookingDate is missing', () => { + const transactions = [ + { + valueDate: '2023-01-01', + transactionAmount: mockTransactionAmount, + }, + { + valueDate: '2023-01-20', + transactionAmount: mockTransactionAmount, + }, + { + valueDate: '2023-01-10', + transactionAmount: mockTransactionAmount, + }, + ]; + expect(sortByBookingDateOrValueDate(transactions)).toEqual([ + { + valueDate: '2023-01-20', + transactionAmount: mockTransactionAmount, + }, + { + valueDate: '2023-01-10', + transactionAmount: mockTransactionAmount, + }, + { + valueDate: '2023-01-01', + transactionAmount: mockTransactionAmount, + }, + ]); + }); + + it('should use bookingDate primarily even if bookingDateTime is on an other date', () => { + const transactions = [ + { + bookingDate: '2023-01-01', + bookingDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-10', + bookingDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-01', + bookingDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-10', + bookingDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + ]; + expect(sortByBookingDateOrValueDate(transactions)).toEqual([ + { + bookingDate: '2023-01-10', + bookingDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-10', + bookingDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-01', + bookingDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-01', + bookingDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + ]); + }); + + it('should sort on booking date if value date is widely off', () => { + const transactions = [ + { + bookingDate: '2023-01-01', + valueDateTime: '2023-01-31T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-02', + valueDateTime: '2023-01-02T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-30', + valueDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-30', + valueDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + ]; + expect(sortByBookingDateOrValueDate(transactions)).toEqual([ + { + bookingDate: '2023-01-30', + valueDateTime: '2023-01-01T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-30', + valueDateTime: '2023-01-01T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-02', + valueDateTime: '2023-01-02T12:00:00Z', + transactionAmount: mockTransactionAmount, + }, + { + bookingDate: '2023-01-01', + valueDateTime: '2023-01-31T00:00:00Z', + transactionAmount: mockTransactionAmount, + }, + ]); + }); }); }); diff --git a/src/app-gocardless/utils.js b/src/app-gocardless/utils.js index 7a3987b77..147276427 100644 --- a/src/app-gocardless/utils.js +++ b/src/app-gocardless/utils.js @@ -6,11 +6,40 @@ export const printIban = (account) => { } }; +const compareDates = ( + /** @type {string | number | Date | undefined} */ a, + /** @type {string | number | Date | undefined} */ b, +) => { + if (a == null && b == null) { + return 0; + } else if (a == null) { + return 1; + } else if (b == null) { + return -1; + } + + return +new Date(a) - +new Date(b); +}; + +/** + * @type {(function(*, *): number)[]} + */ +const compareFunctions = [ + (a, b) => compareDates(a.bookingDate, b.bookingDate), + (a, b) => compareDates(a.bookingDateTime, b.bookingDateTime), + (a, b) => compareDates(a.valueDate, b.valueDate), + (a, b) => compareDates(a.valueDateTime, b.valueDateTime), +]; + export const sortByBookingDateOrValueDate = (transactions = []) => - transactions.sort( - (a, b) => - +new Date(b.bookingDate || b.valueDate) - - +new Date(a.bookingDate || a.valueDate), - ); + transactions.sort((a, b) => { + for (const sortFunction of compareFunctions) { + const result = sortFunction(b, a); + if (result !== 0) { + return result; + } + } + return 0; + }); export const amountToInteger = (n) => Math.round(n * 100); diff --git a/upcoming-release-notes/470.md b/upcoming-release-notes/470.md new file mode 100644 index 000000000..91d8eaff9 --- /dev/null +++ b/upcoming-release-notes/470.md @@ -0,0 +1,6 @@ +--- +category: Enhancements +authors: [UnderKoen] +--- + +Sort bank transactions by more fields. So when there is a bookingDateTime it also sorts by the time. From cc347aef089ec1e9cb228d5de746c214e645b493 Mon Sep 17 00:00:00 2001 From: Tom Crasset <25140344+tcrasset@users.noreply.github.com> Date: Wed, 23 Oct 2024 11:03:23 +0200 Subject: [PATCH 5/7] integrate FileService for app-sync.js (#432) * integrate FilesService for app-sync.js * integrate comments * fix linter --------- Co-authored-by: Matt Fiddaman --- src/app-simplefin/app-simplefin.js | 2 +- src/app-sync.js | 339 +++++++++--------- src/app-sync.test.js | 50 ++- src/app-sync/errors.js | 13 + src/app-sync/services/files-service.js | 197 ++++++++++ .../tests/services/files-service.test.js | 214 +++++++++++ src/app-sync/validation.js | 77 ++++ src/util/middlewares.js | 18 +- upcoming-release-notes/432.md | 6 + 9 files changed, 732 insertions(+), 184 deletions(-) create mode 100644 src/app-sync/errors.js create mode 100644 src/app-sync/services/files-service.js create mode 100644 src/app-sync/tests/services/files-service.test.js create mode 100644 src/app-sync/validation.js create mode 100644 upcoming-release-notes/432.md diff --git a/src/app-simplefin/app-simplefin.js b/src/app-simplefin/app-simplefin.js index 7d27c1bd7..7951526e3 100644 --- a/src/app-simplefin/app-simplefin.js +++ b/src/app-simplefin/app-simplefin.js @@ -42,7 +42,7 @@ app.post( } } } - } catch (error) { + } catch { invalidToken(res); return; } diff --git a/src/app-sync.js b/src/app-sync.js index 447f9d966..793032166 100644 --- a/src/app-sync.js +++ b/src/app-sync.js @@ -13,6 +13,16 @@ import { getPathForUserFile, getPathForGroupFile } from './util/paths.js'; import * as simpleSync from './sync-simple.js'; import { SyncProtoBuf } from '@actual-app/crdt'; +import { + File, + FilesService, + FileUpdate, +} from './app-sync/services/files-service.js'; +import { FileNotFound } from './app-sync/errors.js'; +import { + validateSyncedFile, + validateUploadedFile, +} from './app-sync/validation.js'; const app = express(); app.use(errorMiddleware); @@ -26,11 +36,24 @@ export { app as handlers }; const OK_RESPONSE = { status: 'ok' }; -// This is a version representing the internal format of sync -// messages. When this changes, all sync files need to be reset. We -// will check this version when syncing and notify the user if they -// need to reset. -const SYNC_FORMAT_VERSION = 2; +function boolToInt(deleted) { + return deleted ? 1 : 0; +} + +const verifyFileExists = (fileId, filesService, res, errorObject) => { + try { + return filesService.get(fileId); + } catch (e) { + if (e instanceof FileNotFound) { + //FIXME: error code should be 404. Need to make sure frontend is ok with it. + //TODO: put this into a middleware that checks if FileNotFound is thrown and returns 404 and same error message + // for every FileNotFound error + res.status(400).send(errorObject); + return; + } + throw e; + } +}; app.post('/sync', async (req, res) => { let requestPb; @@ -43,10 +66,9 @@ app.post('/sync', async (req, res) => { return; } - let accountDb = getAccountDb(); - let file_id = requestPb.getFileid() || null; - let group_id = requestPb.getGroupid() || null; - let key_id = requestPb.getKeyid() || null; + let fileId = requestPb.getFileid() || null; + let groupId = requestPb.getGroupid() || null; + let keyId = requestPb.getKeyid() || null; let since = requestPb.getSince() || null; let messages = requestPb.getMessagesList(); @@ -58,68 +80,27 @@ app.post('/sync', async (req, res) => { }); } - let currentFiles = accountDb.all( - 'SELECT group_id, encrypt_keyid, encrypt_meta, sync_version FROM files WHERE id = ?', - [file_id], - ); + const filesService = new FilesService(getAccountDb()); - if (currentFiles.length === 0) { - res.status(400); - res.send('file-not-found'); - return; - } - - let currentFile = currentFiles[0]; - - if ( - currentFile.sync_version == null || - currentFile.sync_version < SYNC_FORMAT_VERSION - ) { - res.status(400); - res.send('file-old-version'); - return; - } - - // When resetting sync state, something went wrong. There is no - // group id and it's awaiting a file to be uploaded. - if (currentFile.group_id == null) { - res.status(400); - res.send('file-needs-upload'); - return; - } + const currentFile = verifyFileExists( + fileId, + filesService, + res, + 'file-not-found', + ); - // Check to make sure the uploaded file is valid and has been - // encrypted with the same key it is registered with (this might - // be wrong if there was an error during the key creation - // process) - let uploadedKeyId = currentFile.encrypt_meta - ? JSON.parse(currentFile.encrypt_meta).keyId - : null; - if (uploadedKeyId !== currentFile.encrypt_keyid) { - res.status(400); - res.send('file-key-mismatch'); + if (!currentFile) { return; } - // The changes being synced are part of an old group, which - // means the file has been reset. User needs to re-download. - if (group_id !== currentFile.group_id) { + const errorMessage = validateSyncedFile(groupId, keyId, currentFile); + if (errorMessage) { res.status(400); - res.send('file-has-reset'); + res.send(errorMessage); return; } - // The data is encrypted with a different key which is - // unacceptable. We can't accept these changes. Reject them and - // tell the user that they need to generate the correct key - // (which necessitates a sync reset so they need to re-download). - if (key_id !== currentFile.encrypt_keyid) { - res.status(400); - res.send('file-has-new-key'); - return false; - } - - let { trie, newMessages } = simpleSync.sync(messages, since, group_id); + let { trie, newMessages } = simpleSync.sync(messages, since, groupId); // encode it back... let responsePb = new SyncProtoBuf.SyncResponse(); @@ -132,57 +113,70 @@ app.post('/sync', async (req, res) => { }); app.post('/user-get-key', (req, res) => { - let accountDb = getAccountDb(); let { fileId } = req.body; - let rows = accountDb.all( - 'SELECT encrypt_salt, encrypt_keyid, encrypt_test FROM files WHERE id = ?', - [fileId], - ); - if (rows.length === 0) { - res.status(400).send('file-not-found'); + const filesService = new FilesService(getAccountDb()); + const file = verifyFileExists(fileId, filesService, res, 'file-not-found'); + + if (!file) { return; } - let { encrypt_salt, encrypt_keyid, encrypt_test } = rows[0]; res.send({ status: 'ok', - data: { id: encrypt_keyid, salt: encrypt_salt, test: encrypt_test }, + data: { + id: file.encryptKeyId, + salt: file.encryptSalt, + test: file.encryptTest, + }, }); }); app.post('/user-create-key', (req, res) => { - let accountDb = getAccountDb(); let { fileId, keyId, keySalt, testContent } = req.body; - accountDb.mutate( - 'UPDATE files SET encrypt_salt = ?, encrypt_keyid = ?, encrypt_test = ? WHERE id = ?', - [keySalt, keyId, testContent, fileId], + const filesService = new FilesService(getAccountDb()); + + if (!verifyFileExists(fileId, filesService, res, 'file not found')) { + return; + } + + filesService.update( + fileId, + new FileUpdate({ + encryptSalt: keySalt, + encryptKeyId: keyId, + encryptTest: testContent, + }), ); res.send(OK_RESPONSE); }); app.post('/reset-user-file', async (req, res) => { - let accountDb = getAccountDb(); let { fileId } = req.body; - let files = accountDb.all('SELECT group_id FROM files WHERE id = ?', [ + const filesService = new FilesService(getAccountDb()); + const file = verifyFileExists( fileId, - ]); - if (files.length === 0) { - res.status(400).send('User or file not found'); + filesService, + res, + 'User or file not found', + ); + + if (!file) { return; } - let { group_id } = files[0]; - accountDb.mutate('UPDATE files SET group_id = NULL WHERE id = ?', [fileId]); + const groupId = file.groupId; + + filesService.update(fileId, new FileUpdate({ groupId: null })); - if (group_id) { + if (groupId) { try { - await fs.unlink(getPathForGroupFile(group_id)); - } catch (e) { - console.log(`Unable to delete sync data for group "${group_id}"`); + await fs.unlink(getPathForGroupFile(groupId)); + } catch { + console.log(`Unable to delete sync data for group "${groupId}"`); } } @@ -190,7 +184,6 @@ app.post('/reset-user-file', async (req, res) => { }); app.post('/upload-user-file', async (req, res) => { - let accountDb = getAccountDb(); if (typeof req.headers['x-actual-name'] !== 'string') { // FIXME: Not sure how this cannot be a string when the header is // set. @@ -215,38 +208,25 @@ app.post('/upload-user-file', async (req, res) => { ? JSON.parse(encryptMeta).keyId : null; - let currentFiles = accountDb.all( - 'SELECT group_id, encrypt_keyid, encrypt_meta FROM files WHERE id = ?', - [fileId], - ); - if (currentFiles.length > 0) { - let currentFile = currentFiles[0]; - - // The uploading file is part of an old group, so reject - // it. All of its internal sync state is invalid because its - // old. The sync state has been reset, so user needs to - // either reset again or download from the current group. - if (groupId !== currentFile.group_id) { - res.status(400); - res.send('file-has-reset'); - return; - } + const filesService = new FilesService(getAccountDb()); + let currentFile; - // The key that the file is encrypted with is different than - // the current registered key. All data must always be - // encrypted with the registered key for consistency. Key - // changes always necessitate a sync reset, which means this - // upload is trying to overwrite another reset. That might - // be be fine, but since we definitely cannot accept a file - // encrypted with the wrong key, we bail and suggest the - // user download the latest file. - if (keyId !== currentFile.encrypt_keyid) { - res.status(400); - res.send('file-has-new-key'); - return; + try { + currentFile = filesService.get(fileId); + } catch (e) { + if (e instanceof FileNotFound) { + currentFile = null; + } else { + throw e; } } + const errorMessage = validateUploadedFile(groupId, keyId, currentFile); + if (errorMessage) { + res.status(400).send(errorMessage); + return; + } + try { await fs.writeFile(getPathForUserFile(fileId), req.body); } catch (err) { @@ -255,37 +235,44 @@ app.post('/upload-user-file', async (req, res) => { return; } - let rows = accountDb.all('SELECT id FROM files WHERE id = ?', [fileId]); - if (rows.length === 0) { + if (!currentFile) { // it's new groupId = uuid.v4(); - accountDb.mutate( - 'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta) VALUES (?, ?, ?, ?, ?)', - [fileId, groupId, syncFormatVersion, name, encryptMeta], - ); - res.send({ status: 'ok', groupId }); - } else { - if (!groupId) { - // sync state was reset, create new group - groupId = uuid.v4(); - accountDb.mutate('UPDATE files SET group_id = ? WHERE id = ?', [ - groupId, - fileId, - ]); - } - // Regardless, update some properties - accountDb.mutate( - 'UPDATE files SET sync_version = ?, encrypt_meta = ?, name = ? WHERE id = ?', - [syncFormatVersion, encryptMeta, name, fileId], + filesService.set( + new File({ + id: fileId, + groupId: groupId, + syncVersion: syncFormatVersion, + name: name, + encryptMeta: encryptMeta, + }), ); res.send({ status: 'ok', groupId }); + return; } + + if (!groupId) { + // sync state was reset, create new group + groupId = uuid.v4(); + filesService.update(fileId, new FileUpdate({ groupId: groupId })); + } + + // Regardless, update some properties + filesService.update( + fileId, + new FileUpdate({ + syncVersion: syncFormatVersion, + encryptMeta: encryptMeta, + name: name, + }), + ); + + res.send({ status: 'ok', groupId }); }); app.get('/download-user-file', async (req, res) => { - let accountDb = getAccountDb(); let fileId = req.headers['x-actual-file-id']; if (typeof fileId !== 'string') { // FIXME: Not sure how this cannot be a string when the header is @@ -294,13 +281,8 @@ app.get('/download-user-file', async (req, res) => { return; } - // Do some authentication - let rows = accountDb.all( - 'SELECT id FROM files WHERE id = ? AND deleted = FALSE', - [fileId], - ); - if (rows.length === 0) { - res.status(400).send('User or file not found'); + const filesService = new FilesService(getAccountDb()); + if (!verifyFileExists(fileId, filesService, res, 'User or file not found')) { return; } @@ -309,70 +291,69 @@ app.get('/download-user-file', async (req, res) => { }); app.post('/update-user-filename', (req, res) => { - let accountDb = getAccountDb(); let { fileId, name } = req.body; - // Do some authentication - let rows = accountDb.all( - 'SELECT id FROM files WHERE id = ? AND deleted = FALSE', - [fileId], - ); - if (rows.length === 0) { - res.status(400).send('file not found'); + const filesService = new FilesService(getAccountDb()); + + if (!verifyFileExists(fileId, filesService, res, 'file not found')) { return; } - accountDb.mutate('UPDATE files SET name = ? WHERE id = ?', [name, fileId]); - + filesService.update(fileId, new FileUpdate({ name: name })); res.send(OK_RESPONSE); }); app.get('/list-user-files', (req, res) => { - let accountDb = getAccountDb(); - let rows = accountDb.all('SELECT * FROM files'); - + const fileService = new FilesService(getAccountDb()); + const rows = fileService.find(); res.send({ status: 'ok', data: rows.map((row) => ({ - deleted: row.deleted, + deleted: boolToInt(row.deleted), fileId: row.id, - groupId: row.group_id, + groupId: row.groupId, name: row.name, - encryptKeyId: row.encrypt_keyid, + encryptKeyId: row.encryptKeyId, })), }); }); app.get('/get-user-file-info', (req, res) => { - let accountDb = getAccountDb(); let fileId = req.headers['x-actual-file-id']; - let rows = accountDb.all( - 'SELECT * FROM files WHERE id = ? AND deleted = FALSE', - [fileId], - ); + // TODO: Return 422 if fileId is not provided. Need to make sure frontend can handle it + // if (!fileId) { + // return res.status(422).send({ + // details: 'fileId-required', + // reason: 'unprocessable-entity', + // status: 'error', + // }); + // } - if (rows.length === 0) { - res.status(400).send({ status: 'error', reason: 'file-not-found' }); + const fileService = new FilesService(getAccountDb()); + + const file = verifyFileExists(fileId, fileService, res, { + status: 'error', + reason: 'file-not-found', + }); + + if (!file) { return; } - let row = rows[0]; - res.send({ status: 'ok', data: { - deleted: row.deleted, - fileId: row.id, - groupId: row.group_id, - name: row.name, - encryptMeta: row.encrypt_meta ? JSON.parse(row.encrypt_meta) : null, + deleted: boolToInt(file.deleted), // FIXME: convert to boolean, make sure it works in the frontend + fileId: file.id, + groupId: file.groupId, + name: file.name, + encryptMeta: file.encryptMeta ? JSON.parse(file.encryptMeta) : null, }, }); }); app.post('/delete-user-file', (req, res) => { - let accountDb = getAccountDb(); let { fileId } = req.body; if (!fileId) { @@ -383,12 +364,12 @@ app.post('/delete-user-file', (req, res) => { }); } - let rows = accountDb.all('SELECT * FROM files WHERE id = ?', [fileId]); - - if (rows.length === 0) { - return res.status(400).send('file-not-found'); + const filesService = new FilesService(getAccountDb()); + if (!verifyFileExists(fileId, filesService, res, 'file-not-found')) { + return; } - accountDb.mutate('UPDATE files SET deleted = TRUE WHERE id = ?', [fileId]); + filesService.update(fileId, new FileUpdate({ deleted: true })); + res.send(OK_RESPONSE); }); diff --git a/src/app-sync.test.js b/src/app-sync.test.js index 4c3328abd..b81d5f144 100644 --- a/src/app-sync.test.js +++ b/src/app-sync.test.js @@ -67,6 +67,54 @@ describe('/user-create-key', () => { status: 'error', }); }); + + it('returns 400 if the file is not found', async () => { + const res = await request(app) + .post('/user-create-key') + .set('x-actual-token', 'valid-token') + .send({ fileId: 'non-existent-file-id' }); + + expect(res.statusCode).toEqual(400); + expect(res.text).toBe('file not found'); + }); + + it('creates a new encryption key for the file', async () => { + const fileId = crypto.randomBytes(16).toString('hex'); + + const old_encrypt_salt = 'old-salt'; + const old_encrypt_keyid = 'old-key'; + const old_encrypt_test = 'old-encrypt-test'; + const encrypt_salt = 'test-salt'; + const encrypt_keyid = 'test-key-id'; + const encrypt_test = 'test-encrypt-test'; + + getAccountDb().mutate( + 'INSERT INTO files (id, encrypt_salt, encrypt_keyid, encrypt_test) VALUES (?, ?, ?, ?)', + [fileId, old_encrypt_salt, old_encrypt_keyid, old_encrypt_test], + ); + + const res = await request(app) + .post('/user-create-key') + .set('x-actual-token', 'valid-token') + .send({ + fileId, + keyId: encrypt_keyid, + keySalt: encrypt_salt, + testContent: encrypt_test, + }); + + expect(res.statusCode).toEqual(200); + expect(res.body).toEqual({ status: 'ok' }); + + const rows = getAccountDb().all( + 'SELECT encrypt_salt, encrypt_keyid, encrypt_test FROM files WHERE id = ?', + [fileId], + ); + + expect(rows[0].encrypt_salt).toEqual(encrypt_salt); + expect(rows[0].encrypt_keyid).toEqual(encrypt_keyid); + expect(rows[0].encrypt_test).toEqual(encrypt_test); + }); }); describe('/reset-user-file', () => { @@ -104,7 +152,7 @@ describe('/reset-user-file', () => { fileId, ]); - expect(rows[0].group_id).toBeNull; + expect(rows[0].group_id).toBeNull(); }); it('returns 400 if the file is not found', async () => { diff --git a/src/app-sync/errors.js b/src/app-sync/errors.js new file mode 100644 index 000000000..b57867cdd --- /dev/null +++ b/src/app-sync/errors.js @@ -0,0 +1,13 @@ +export class FileNotFound extends Error { + constructor(params = {}) { + super("File does not exist or you don't have access to it"); + this.details = params; + } +} + +export class GenericFileError extends Error { + constructor(message, params = {}) { + super(message); + this.details = params; + } +} diff --git a/src/app-sync/services/files-service.js b/src/app-sync/services/files-service.js new file mode 100644 index 000000000..63fe7f373 --- /dev/null +++ b/src/app-sync/services/files-service.js @@ -0,0 +1,197 @@ +import getAccountDb from '../../account-db.js'; +import { FileNotFound, GenericFileError } from '../errors.js'; + +class FileBase { + constructor( + name, + groupId, + encryptSalt, + encryptTest, + encryptKeyId, + encryptMeta, + syncVersion, + deleted, + ) { + this.name = name; + this.groupId = groupId; + this.encryptSalt = encryptSalt; + this.encryptTest = encryptTest; + this.encryptKeyId = encryptKeyId; + this.encryptMeta = encryptMeta; + this.syncVersion = syncVersion; + this.deleted = typeof deleted === 'boolean' ? deleted : Boolean(deleted); + } +} + +class File extends FileBase { + constructor({ + id, + name = null, + groupId = null, + encryptSalt = null, + encryptTest = null, + encryptKeyId = null, + encryptMeta = null, + syncVersion = null, + deleted = false, + }) { + super( + name, + groupId, + encryptSalt, + encryptTest, + encryptKeyId, + encryptMeta, + syncVersion, + deleted, + ); + this.id = id; + } +} + +/** + * Represents a file update. Will only update the fields that are defined. + * @class + * @extends FileBase + */ +class FileUpdate extends FileBase { + constructor({ + name = undefined, + groupId = undefined, + encryptSalt = undefined, + encryptTest = undefined, + encryptKeyId = undefined, + encryptMeta = undefined, + syncVersion = undefined, + deleted = undefined, + }) { + super( + name, + groupId, + encryptSalt, + encryptTest, + encryptKeyId, + encryptMeta, + syncVersion, + deleted, + ); + } +} + +const boolToInt = (bool) => { + return bool ? 1 : 0; +}; + +class FilesService { + constructor(accountDb) { + this.accountDb = accountDb; + } + + get(fileId) { + const rawFile = this.getRaw(fileId); + if (!rawFile || (rawFile && rawFile.deleted)) { + throw new FileNotFound(); + } + + return this.validate(rawFile); + } + + set(file) { + const deletedInt = boolToInt(file.deleted); + this.accountDb.mutate( + 'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_salt, encrypt_test, encrypt_keyid, deleted) VALUES (?, ?, ?, ?, ?, ?, ?,? ,?)', + [ + file.id, + file.groupId, + file.syncVersion.toString(), + file.name, + file.encryptMeta, + file.encryptSalt, + file.encrypt_test, + file.encrypt_keyid, + deletedInt, + ], + ); + } + + find(limit = 1000) { + return this.accountDb + .all('SELECT * FROM files WHERE deleted = 0 LIMIT ?', [limit]) + .map(this.validate); + } + + update(id, fileUpdate) { + let query = 'UPDATE files SET'; + const params = []; + const updates = []; + + if (fileUpdate.name !== undefined) { + updates.push('name = ?'); + params.push(fileUpdate.name); + } + if (fileUpdate.groupId !== undefined) { + updates.push('group_id = ?'); + params.push(fileUpdate.groupId); + } + if (fileUpdate.encryptSalt !== undefined) { + updates.push('encrypt_salt = ?'); + params.push(fileUpdate.encryptSalt); + } + if (fileUpdate.encryptTest !== undefined) { + updates.push('encrypt_test = ?'); + params.push(fileUpdate.encryptTest); + } + if (fileUpdate.encryptKeyId !== undefined) { + updates.push('encrypt_keyid = ?'); + params.push(fileUpdate.encryptKeyId); + } + if (fileUpdate.encryptMeta !== undefined) { + updates.push('encrypt_meta = ?'); + params.push(fileUpdate.encryptMeta); + } + if (fileUpdate.syncVersion !== undefined) { + updates.push('sync_version = ?'); + params.push(fileUpdate.syncVersion); + } + if (fileUpdate.deleted !== undefined) { + updates.push('deleted = ?'); + params.push(boolToInt(fileUpdate.deleted)); + } + + if (updates.length > 0) { + query += ' ' + updates.join(', ') + ' WHERE id = ?'; + params.push(id); + + const res = this.accountDb.mutate(query, params); + + if (res.changes != 1) { + throw new GenericFileError('Could not update File', { id }); + } + } + + // Return the modified object + return this.validate(this.getRaw(id)); + } + + getRaw(fileId) { + return this.accountDb.first(`SELECT * FROM files WHERE id = ?`, [fileId]); + } + + validate(rawFile) { + return new File({ + id: rawFile.id, + name: rawFile.name, + groupId: rawFile.group_id, + encryptSalt: rawFile.encrypt_salt, + encryptTest: rawFile.encrypt_test, + encryptKeyId: rawFile.encrypt_keyid, + encryptMeta: rawFile.encrypt_meta, + syncVersion: rawFile.sync_version, + deleted: Boolean(rawFile.deleted), + }); + } +} + +const filesService = new FilesService(getAccountDb()); + +export { filesService, FilesService, File, FileUpdate }; diff --git a/src/app-sync/tests/services/files-service.test.js b/src/app-sync/tests/services/files-service.test.js new file mode 100644 index 000000000..7d3dadd6d --- /dev/null +++ b/src/app-sync/tests/services/files-service.test.js @@ -0,0 +1,214 @@ +import getAccountDb from '../../../account-db.js'; +import { FileNotFound } from '../../errors.js'; +import { + FilesService, + File, + FileUpdate, +} from '../../services/files-service.js'; // Adjust the path as necessary +import crypto from 'node:crypto'; +describe('FilesService', () => { + let filesService; + let accountDb; + + const insertToyExampleData = () => { + accountDb.mutate( + 'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_salt, encrypt_test, encrypt_keyid, deleted) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', + [ + '1', + 'group1', + 1, + 'file1', + '{"key":"value"}', + 'salt', + 'test', + 'keyid', + 0, + ], + ); + }; + + const clearDatabase = () => { + accountDb.mutate('DELETE FROM files'); + }; + + beforeAll((done) => { + accountDb = getAccountDb(); + filesService = new FilesService(accountDb); + done(); + }); + + beforeEach((done) => { + insertToyExampleData(); + done(); + }); + + afterEach((done) => { + clearDatabase(); + done(); + }); + + test('get should return a file', () => { + const file = filesService.get('1'); + const expectedFile = new File({ + id: '1', + groupId: 'group1', + syncVersion: 1, + name: 'file1', + encryptMeta: '{"key":"value"}', + encryptSalt: 'salt', + encryptTest: 'test', + encryptKeyId: 'keyid', + deleted: false, + }); + + expect(file).toEqual(expectedFile); + }); + + test('get should throw FileNotFound if file is deleted or does not exist', () => { + const fileId = crypto.randomBytes(16).toString('hex'); + accountDb.mutate( + 'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_salt, encrypt_test, encrypt_keyid, deleted) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', + [ + fileId, + 'group1', + 1, + 'file1', + '{"key":"value"}', + 'salt', + 'test', + 'keyid', + 1, + ], + ); + + expect(() => { + filesService.get(fileId); + }).toThrow(FileNotFound); + + expect(() => { + filesService.get(crypto.randomBytes(16).toString('hex')); + }).toThrow(FileNotFound); + }); + + test.each([true, false])( + 'set should insert a new file with deleted: %p', + (deleted) => { + const fileId = crypto.randomBytes(16).toString('hex'); + const newFile = new File({ + id: fileId, + groupId: 'group2', + syncVersion: 1, + name: 'file2', + encryptMeta: '{"key":"value2"}', + deleted: deleted, + }); + + filesService.set(newFile); + + const file = filesService.validate(filesService.getRaw(fileId)); + const expectedFile = new File({ + id: fileId, + groupId: 'group2', + syncVersion: 1, + name: 'file2', + encryptMeta: '{"key":"value2"}', + encryptSalt: null, // default value + encryptTest: null, // default value + encryptKeyId: null, // default value + deleted: deleted, + }); + + expect(file).toEqual(expectedFile); + }, + ); + + test('find should return a list of files', () => { + const files = filesService.find(); + expect(files.length).toBe(1); + expect(files[0]).toEqual( + new File({ + id: '1', + groupId: 'group1', + syncVersion: 1, + name: 'file1', + encryptMeta: '{"key":"value"}', + encryptSalt: 'salt', + encryptTest: 'test', + encryptKeyId: 'keyid', + deleted: false, + }), + ); + }); + + test('find should respect the limit parameter', () => { + filesService.set( + new File({ + id: crypto.randomBytes(16).toString('hex'), + groupId: 'group2', + syncVersion: 1, + name: 'file2', + encryptMeta: '{"key":"value2"}', + deleted: false, + }), + ); + // Make sure that the file was inserted + const allFiles = filesService.find(); + expect(allFiles.length).toBe(2); + + // Limit the number of files returned + const limitedFiles = filesService.find(1); + expect(limitedFiles.length).toBe(1); + }); + + test('update should modify all attributes of an existing file', () => { + const fileUpdate = new FileUpdate({ + name: 'updatedFile1', + groupId: 'updatedGroup1', + encryptSalt: 'updatedSalt', + encryptTest: 'updatedTest', + encryptKeyId: 'updatedKeyId', + encryptMeta: '{"key":"updatedValue"}', + syncVersion: 2, + deleted: true, + }); + const updatedFile = filesService.update('1', fileUpdate); + + expect(updatedFile).toEqual( + new File({ + id: '1', + name: 'updatedFile1', + groupId: 'updatedGroup1', + encryptSalt: 'updatedSalt', + encryptTest: 'updatedTest', + encryptMeta: '{"key":"updatedValue"}', + encryptKeyId: 'updatedKeyId', + syncVersion: 2, + deleted: true, + }), + ); + }); + + test.each([['update-group', null]])( + 'update should modify a single attribute with groupId = $groupId', + (newGroupId) => { + const fileUpdate = new FileUpdate({ + groupId: newGroupId, + }); + const updatedFile = filesService.update('1', fileUpdate); + + expect(updatedFile).toEqual( + new File({ + id: '1', + name: 'file1', + groupId: newGroupId, + syncVersion: 1, + encryptMeta: '{"key":"value"}', + encryptSalt: 'salt', + encryptTest: 'test', + encryptKeyId: 'keyid', + deleted: false, + }), + ); + }, + ); +}); diff --git a/src/app-sync/validation.js b/src/app-sync/validation.js new file mode 100644 index 000000000..4fe3fff2c --- /dev/null +++ b/src/app-sync/validation.js @@ -0,0 +1,77 @@ +// This is a version representing the internal format of sync +// messages. When this changes, all sync files need to be reset. We +// will check this version when syncing and notify the user if they +// need to reset. +const SYNC_FORMAT_VERSION = 2; + +const validateSyncedFile = (groupId, keyId, currentFile) => { + if ( + currentFile.syncVersion == null || + currentFile.syncVersion < SYNC_FORMAT_VERSION + ) { + return 'file-old-version'; + } + + // When resetting sync state, something went wrong. There is no + // group id and it's awaiting a file to be uploaded. + if (currentFile.groupId == null) { + return 'file-needs-upload'; + } + + // Check to make sure the uploaded file is valid and has been + // encrypted with the same key it is registered with (this might + // be wrong if there was an error during the key creation + // process) + let uploadedKeyId = currentFile.encryptMeta + ? JSON.parse(currentFile.encryptMeta).keyId + : null; + if (uploadedKeyId !== currentFile.encryptKeyId) { + return 'file-key-mismatch'; + } + + // The changes being synced are part of an old group, which + // means the file has been reset. User needs to re-download. + if (groupId !== currentFile.groupId) { + return 'file-has-reset'; + } + + // The data is encrypted with a different key which is + // unacceptable. We can't accept these changes. Reject them and + // tell the user that they need to generate the correct key + // (which necessitates a sync reset so they need to re-download). + if (keyId !== currentFile.encryptKeyId) { + return 'file-has-new-key'; + } + + return null; +}; + +const validateUploadedFile = (groupId, keyId, currentFile) => { + if (!currentFile) { + // File is new, so no need to validate + return null; + } + // The uploading file is part of an old group, so reject + // it. All of its internal sync state is invalid because its + // old. The sync state has been reset, so user needs to + // either reset again or download from the current group. + if (groupId !== currentFile.groupId) { + return 'file-has-reset'; + } + + // The key that the file is encrypted with is different than + // the current registered key. All data must always be + // encrypted with the registered key for consistency. Key + // changes always necessitate a sync reset, which means this + // upload is trying to overwrite another reset. That might + // be be fine, but since we definitely cannot accept a file + // encrypted with the wrong key, we bail and suggest the + // user download the latest file. + if (keyId !== currentFile.encryptKeyId) { + return 'file-has-new-key'; + } + + return null; +}; + +export { validateSyncedFile, validateUploadedFile }; diff --git a/src/util/middlewares.js b/src/util/middlewares.js index 14e6e4dc1..5bbec6946 100644 --- a/src/util/middlewares.js +++ b/src/util/middlewares.js @@ -7,10 +7,22 @@ import * as expressWinston from 'express-winston'; * @param {Error} err * @param {import('express').Request} req * @param {import('express').Response} res - * @param {import('express').NextFunction} _next + * @param {import('express').NextFunction} next */ -async function errorMiddleware(err, req, res, _next) { - console.log('ERROR', err); +async function errorMiddleware(err, req, res, next) { + if (res.headersSent) { + // If you call next() with an error after you have started writing the response + // (for example, if you encounter an error while streaming the response + // to the client), the Express default error handler closes + // the connection and fails the request. + + // So when you add a custom error handler, you must delegate + // to the default Express error handler, when the headers + // have already been sent to the client + // Source: https://expressjs.com/en/guide/error-handling.html + return next(err); + } + console.log(`Error on endpoint ${req.url}`, err.message, err.stack); res.status(500).send({ status: 'error', reason: 'internal-error' }); } diff --git a/upcoming-release-notes/432.md b/upcoming-release-notes/432.md new file mode 100644 index 000000000..4eb7e7d63 --- /dev/null +++ b/upcoming-release-notes/432.md @@ -0,0 +1,6 @@ +--- +category: Maintenance +authors: [tcrasset] +--- + +Integrate FileService for app-sync.js \ No newline at end of file From c9e6d7897b09c534e0ad9f1d99759a3ffe357ea8 Mon Sep 17 00:00:00 2001 From: Robert Dyer Date: Wed, 23 Oct 2024 04:04:32 -0500 Subject: [PATCH 6/7] Do not request transactions when listing accounts (#482) * Do not request transactions when listing accounts * add release note * fix linter --- src/app-simplefin/app-simplefin.js | 33 +++++++++++++++++------------- upcoming-release-notes/482.md | 6 ++++++ 2 files changed, 25 insertions(+), 14 deletions(-) create mode 100644 upcoming-release-notes/482.md diff --git a/src/app-simplefin/app-simplefin.js b/src/app-simplefin/app-simplefin.js index 7951526e3..a1729b1ce 100644 --- a/src/app-simplefin/app-simplefin.js +++ b/src/app-simplefin/app-simplefin.js @@ -47,12 +47,8 @@ app.post( return; } - const now = new Date(); - const startDate = new Date(now.getFullYear(), now.getMonth(), 1); - const endDate = new Date(now.getFullYear(), now.getMonth() + 1, 1); - try { - const accounts = await getAccounts(accessKey, startDate, endDate); + const accounts = await getAccounts(accessKey, null, null, true); res.send({ status: 'ok', @@ -313,7 +309,12 @@ function normalizeDate(date) { return (date.valueOf() - date.getTimezoneOffset() * 60 * 1000) / 1000; } -async function getAccounts(accessKey, startDate, endDate) { +async function getAccounts( + accessKey, + startDate, + endDate, + noTransactions = false, +) { const sfin = parseAccessKey(accessKey); const options = { headers: { @@ -323,16 +324,20 @@ async function getAccounts(accessKey, startDate, endDate) { }, }; const params = []; - let queryString = ''; - if (startDate) { - params.push(`start-date=${normalizeDate(startDate)}`); - } - if (endDate) { - params.push(`end-date=${normalizeDate(endDate)}`); - } + if (!noTransactions) { + if (startDate) { + params.push(`start-date=${normalizeDate(startDate)}`); + } + if (endDate) { + params.push(`end-date=${normalizeDate(endDate)}`); + } - params.push(`pending=1`); + params.push(`pending=1`); + } else { + params.push(`balances-only=1`); + } + let queryString = ''; if (params.length > 0) { queryString += '?' + params.join('&'); } diff --git a/upcoming-release-notes/482.md b/upcoming-release-notes/482.md new file mode 100644 index 000000000..80004c475 --- /dev/null +++ b/upcoming-release-notes/482.md @@ -0,0 +1,6 @@ +--- +category: Enhancements +authors: [psybers] +--- + +Don't pull transactions from SimpleFIN when asking for the list of accounts. From 603f970f12480c4d871d1a40d277213511e429ac Mon Sep 17 00:00:00 2001 From: Michael Clark <5285928+MikesGlitch@users.noreply.github.com> Date: Mon, 28 Oct 2024 22:17:35 +0000 Subject: [PATCH 7/7] =?UTF-8?q?=F0=9F=90=9B=20=20Fix=20migrations=20not=20?= =?UTF-8?q?working=20on=20fresh=20clone=20(#487)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app.js | 6 ++++-- upcoming-release-notes/487.md | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 upcoming-release-notes/487.md diff --git a/app.js b/app.js index 4e254b5da..d0647fb88 100644 --- a/app.js +++ b/app.js @@ -1,8 +1,10 @@ -import run from './src/app.js'; import runMigrations from './src/migrations.js'; runMigrations() - .then(run) + .then(() => { + //import the app here becasue initial migrations need to be run first - they are dependencies of the app.js + import('./src/app.js').then((app) => app.default()); // run the app + }) .catch((err) => { console.log('Error starting app:', err); process.exit(1); diff --git a/upcoming-release-notes/487.md b/upcoming-release-notes/487.md new file mode 100644 index 000000000..c0c53da46 --- /dev/null +++ b/upcoming-release-notes/487.md @@ -0,0 +1,6 @@ +--- +category: Bugfix +authors: [MikesGlitch] +--- + +Fix migrations not running properly on inital setup