From a01edabad36ed27f2a57902cd38da02bd57a4a74 Mon Sep 17 00:00:00 2001 From: Per Nyfelt Date: Fri, 24 Jul 2026 13:31:09 +0200 Subject: [PATCH 1/3] =?UTF-8?q?l=C3=A4gg=20till=20dokumenterad=20r=C3=A4tt?= =?UTF-8?q?else=20f=C3=B6r=20auditloggens=20integritetskontroll?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ett tidigare fel (felaktigt tolkade ingående balanser, sedan patchade direkt i databasen) visade att vi saknade ett sätt att förklara en känd, redan utredd avvikelse i hashkedjan utan att antingen (a) tysta integritetskontrollen permanent genom att skriva om entry_hash/ previous_hash, vilket gör kedjan om intet, eller (b) låta ett förklarat historiskt fel blockera rapportexport och andra åtgärder för evigt. recordIntegrityRemediation(companyId, auditLogId, reason) löser detta genom att lägga till en ny, normalt kedjad INTEGRITY_REMEDIATION-post som pekar ut och förklarar den trasiga raden - originalraden rörs aldrig. validateIntegrity() delas nu upp i två nivåer: en dokumenterad avvikelse räknas inte längre som kritisk (och blockerar därmed inte ReportIntegrityService), men förblir synlig via den nya listDocumentedExceptions(companyId). Rättelsen kräver att målraden faktiskt har ett odokumenterat problem just nu, så metoden inte kan användas för att stämpla friska rader i förväg. Co-Authored-By: Claude Sonnet 5 --- .../accounting/service/AuditLogService.groovy | 135 ++++++++++++++++-- .../service/AuditLogServiceTest.groovy | 72 ++++++++++ 2 files changed, 197 insertions(+), 10 deletions(-) diff --git a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy index ef42173..96bfbf2 100644 --- a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy +++ b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy @@ -41,6 +41,7 @@ final class AuditLogService { static final String DELETE_FISCAL_YEAR = 'DELETE_FISCAL_YEAR' static final String ARCHIVE_COMPANY = 'ARCHIVE_COMPANY' static final String UNARCHIVE_COMPANY = 'UNARCHIVE_COMPANY' + static final String INTEGRITY_REMEDIATION = 'INTEGRITY_REMEDIATION' private static final String DEFAULT_ACTOR = 'desktop-app' private static final DateTimeFormatter HASH_TIMESTAMP_FORMAT = DateTimeFormatter.ofPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSS") @@ -137,12 +138,19 @@ final class AuditLogService { } } + /** + * Critical, unexplained integrity problems only. A row with a hash mismatch that has a + * matching {@link #recordIntegrityRemediation} entry is excluded here - see + * {@link #listDocumentedExceptions(long)} for those. This is the list report exports and + * other operations gate on (see ReportIntegrityService); a documented exception must not + * keep blocking business operations forever once it has been explained. + */ List validateIntegrity() { databaseService.withSql { Sql sql -> List problems = [] sql.rows('select distinct company_id as companyId from audit_log').each { GroovyRowResult companyRow -> long cid = ((Number) companyRow.get('companyId')).longValue() - problems.addAll(validateIntegrityForCompany(sql, cid)) + problems.addAll(checkIntegrityForCompany(sql, cid).criticalProblems) } problems } @@ -151,12 +159,64 @@ final class AuditLogService { List validateIntegrity(long companyId) { CompanyService.requireValidCompanyId(companyId) databaseService.withSql { Sql sql -> - validateIntegrityForCompany(sql, companyId) + checkIntegrityForCompany(sql, companyId).criticalProblems + } + } + + /** + * Hash mismatches that have a matching {@link #recordIntegrityRemediation} entry + * explaining them - visible for transparency, but excluded from + * {@link #validateIntegrity(long)} and therefore no longer blocking. + */ + List listDocumentedExceptions(long companyId) { + CompanyService.requireValidCompanyId(companyId) + databaseService.withSql { Sql sql -> + checkIntegrityForCompany(sql, companyId).documentedExceptions } } - private static List validateIntegrityForCompany(Sql sql, long companyId) { - List problems = [] + /** + * Documents that a hash mismatch on {@code remediatedAuditLogId} is a known, explained + * historical anomaly rather than undetected tampering - e.g. data corrupted by a bug that + * has since been fixed, where the underlying record was already corrected through its + * normal domain operation (which itself produced its own, properly-chained audit entry). + * + * This deliberately never touches the broken row's own entry_hash/previous_hash: doing so + * would mean the chain no longer commits to what actually happened, which is the entire + * point of keeping it append-only. It appends a new, normally-chained + * {@link #INTEGRITY_REMEDIATION} entry instead, pointing at the row it explains. The + * target row must currently have an integrity problem - this is for documenting real, + * already-diagnosed anomalies, not for pre-emptively annotating healthy rows. + */ + AuditLogEntry recordIntegrityRemediation(long companyId, long remediatedAuditLogId, String reason) { + CompanyService.requireValidCompanyId(companyId) + String safeReason = requireText(reason, 'Anledning') + databaseService.withTransaction { Sql sql -> + GroovyRowResult target = sql.firstRow( + 'select id from audit_log where id = ? and company_id = ?', + [remediatedAuditLogId, companyId] + ) as GroovyRowResult + if (target == null) { + throw new IllegalArgumentException("Auditlogg ${remediatedAuditLogId} finns inte för detta företag.") + } + IntegrityCheckResult current = checkIntegrityForCompany(sql, companyId) + if (!current.criticalAuditLogIds.contains(remediatedAuditLogId)) { + throw new IllegalStateException( + "Auditlogg ${remediatedAuditLogId} har inget odokumenterat integritetsproblem att förklara." + ) + } + recordEvent(sql, INTEGRITY_REMEDIATION, AuditReferences.EMPTY, + "Manuell rättelse dokumenterad för auditlogg ${remediatedAuditLogId}", + formatDetails([ + remediatesAuditLogId: remediatedAuditLogId, + reason : safeReason + ]), companyId) + } + } + + private static IntegrityCheckResult checkIntegrityForCompany(Sql sql, long companyId) { + List rowProblems = [] + List structuralProblems = [] // Archiving a fiscal year (see FiscalYearReplacementService.archiveFiscalYearAuditLogRows) // resets the chain head to the latest *live* row so the next insert skips over the rows // being archived, rather than chaining off them. So the row immediately following an @@ -188,7 +248,7 @@ final class AuditLogService { AuditLogEntry entry = mapEntry(row) boolean archived = row.get('archived') as Boolean if (entry.previousHash != expectedPreviousHash && entry.previousHash != expectedLastLiveHash) { - problems << ("Auditlogg ${entry.id} har fel föregående hash." as String) + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har fel föregående hash." as String) } String calculated = calculateHash(new AuditEntrySeed( eventType: entry.eventType, @@ -204,7 +264,7 @@ final class AuditLogService { createdAt: entry.createdAt )) if (entry.entryHash != calculated) { - problems << ("Auditlogg ${entry.id} har ogiltig hash." as String) + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har ogiltig hash." as String) } expectedPreviousHash = entry.entryHash if (!archived) { @@ -214,11 +274,42 @@ final class AuditLogService { } AuditChainHead chainHead = loadChainHead(sql, companyId) if (chainHead == null) { - problems << 'Auditloggkedjans huvud saknas.' + structuralProblems << 'Auditloggkedjans huvud saknas.' } else if (chainHead.lastEntryHash != actualLastLiveHash) { - problems << 'Auditloggkedjans huvud pekar inte på sista auditraden.' + structuralProblems << 'Auditloggkedjans huvud pekar inte på sista auditraden.' } - problems + + Set remediatedIds = loadRemediatedAuditLogIds(sql, companyId) + List critical = [] + List documented = [] + Set criticalIds = [] as Set + rowProblems.each { RowProblem problem -> + if (remediatedIds.contains(problem.auditLogId)) { + documented << problem.message + } else { + critical << problem.message + criticalIds << problem.auditLogId + } + } + // Structural problems (a missing/misdirected chain head) aren't tied to one explainable + // row, so a documented remediation can never resolve them - they always stay critical. + critical.addAll(structuralProblems) + new IntegrityCheckResult(critical, documented, criticalIds) + } + + private static Set loadRemediatedAuditLogIds(Sql sql, long companyId) { + Set ids = [] as Set + sql.rows( + 'select details from audit_log where company_id = ? and event_type = ?', + [companyId, INTEGRITY_REMEDIATION] + ).each { GroovyRowResult row -> + Map parsed = parseDetails(SqlValueMapper.toClob(row.get('details'))) + Long remediatedId = longOrNull(parsed.remediatesAuditLogId) + if (remediatedId != null) { + ids << remediatedId + } + } + ids } AuditLogEntry logImport(String summary, String details = null, long companyId = CompanyService.LEGACY_COMPANY_ID) { @@ -537,7 +628,7 @@ final class AuditLogService { repairArchivedReferencesFromDetails(sql) sql.rows('select distinct company_id as companyId from audit_log').each { GroovyRowResult row -> long companyId = ((Number) row.get('companyId')).longValue() - if (!validateIntegrityForCompany(sql, companyId).isEmpty()) { + if (!checkIntegrityForCompany(sql, companyId).criticalProblems.isEmpty()) { rebuildIntegrityChain(sql, companyId) } } @@ -801,4 +892,28 @@ final class AuditLogService { this.lastEntryHash = lastEntryHash } } + + private static final class RowProblem { + + final long auditLogId + final String message + + private RowProblem(long auditLogId, String message) { + this.auditLogId = auditLogId + this.message = message + } + } + + private static final class IntegrityCheckResult { + + final List criticalProblems + final List documentedExceptions + final Set criticalAuditLogIds + + private IntegrityCheckResult(List criticalProblems, List documentedExceptions, Set criticalAuditLogIds) { + this.criticalProblems = criticalProblems + this.documentedExceptions = documentedExceptions + this.criticalAuditLogIds = criticalAuditLogIds + } + } } diff --git a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy index 8783747..773d47d 100644 --- a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy +++ b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy @@ -2,6 +2,7 @@ package se.alipsa.accounting.service import static org.junit.jupiter.api.Assertions.assertEquals import static org.junit.jupiter.api.Assertions.assertFalse +import static org.junit.jupiter.api.Assertions.assertThrows import static org.junit.jupiter.api.Assertions.assertTrue import groovy.sql.GroovyRowResult @@ -71,6 +72,68 @@ class AuditLogServiceTest { assertTrue(problems.any { String problem -> problem.contains('ogiltig hash') }) } + @Test + void recordIntegrityRemediationMovesRowFromCriticalToDocumented() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long tamperedId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', tamperedId]) + } + assertTrue(auditLogService.validateIntegrity(companyId).any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }) + + auditLogService.recordIntegrityRemediation( + companyId, tamperedId, 'Rättad efter felsökning: fel bugg orsakade nollställd summary.' + ) + + List critical = auditLogService.validateIntegrity(companyId) + assertFalse(critical.any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }, + 'A documented, explained mismatch must no longer block operations as a critical problem') + List documented = auditLogService.listDocumentedExceptions(companyId) + assertTrue(documented.any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }, + 'The documented exception must still be visible, just not critical') + assertTrue(critical.isEmpty(), + 'The remediation entry itself, and everything else, must still form a valid chain') + } + + @Test + void recordIntegrityRemediationRejectsARowWithNoIntegrityProblem() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long healthyId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + IllegalStateException exception = assertThrows(IllegalStateException) { + auditLogService.recordIntegrityRemediation(companyId, healthyId, 'Ingen anledning egentligen') + } + assertTrue(exception.message.contains('inget odokumenterat integritetsproblem')) + } + + @Test + void recordIntegrityRemediationRejectsUnknownAuditLogId() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + + IllegalArgumentException exception = assertThrows(IllegalArgumentException) { + auditLogService.recordIntegrityRemediation(companyId, 999999L, 'Motivering') + } + assertTrue(exception.message.contains('finns inte')) + } + + @Test + void recordIntegrityRemediationRequiresAReason() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long tamperedId = idOfRow(databaseService, "summary = 'Importerade SIE'") + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', tamperedId]) + } + + assertThrows(IllegalArgumentException) { + auditLogService.recordIntegrityRemediation(companyId, tamperedId, ' ') + } + } + @Test void rebuildIntegrityChainMakesLiveEntriesSkipArchivedOnesInBetween() { long companyId = CompanyService.LEGACY_COMPANY_ID @@ -169,6 +232,15 @@ class AuditLogServiceTest { row.get('columnValue') as String } + private static long idOfRow(DatabaseService databaseService, String whereClause) { + databaseService.withSql { Sql sql -> + GroovyRowResult row = sql.firstRow( + "select id from audit_log where ${whereClause}".toString() + ) as GroovyRowResult + ((Number) row.get('id')).longValue() + } + } + private static void restoreProperty(String name, String value) { if (value == null) { System.clearProperty(name) From 37b8e16828d8dd89ad43d7cca43099506975d9a5 Mon Sep 17 00:00:00 2001 From: Per Nyfelt Date: Fri, 24 Jul 2026 13:31:21 +0200 Subject: [PATCH 2/3] =?UTF-8?q?dokumentera=20hur=20data=C3=A4ttelser=20ska?= =?UTF-8?q?=20hanteras=20i=20AGENTS.md?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skriver ner regeln som den nya rättelsemekanismen (och det bakomliggande incidenten den löser) bygger på, så framtida agent-sessioner inte återupprepar misstaget: aldrig patcha ledgertabeller direkt med SQL, rättelser går via applikationens egna domänoperationer (samma append-only-mönster som korrigeringsverifikationer), och en upptäckt hashintegritetsavvikelse dokumenteras via AuditLogService.recordIntegrityRemediation() - inte genom att skriva om entry_hash/previous_hash eller köra rebuildIntegrityChain()/ repairIntegrityForAllCompanies() som allmänna reparationsverktyg. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index c315cf9..9cc318a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,6 +63,14 @@ Recent history favors short, descriptive commit messages, often in Swedish imper - All schema changes must go through migrations in `app/src/main/resources/db/migrations/`, and new migrations must be registered in `DatabaseService.MIGRATIONS`. - Do not edit generated build output under `app/build/`, root `build/`, `.gradle/`, `.gradle-user/`, `dist/`, or `releases/`. +## Data Integrity & Corrections +`audit_log` is an append-only, SHA-256 hash-chained ledger (see `AuditLogService`): each row's `entry_hash` commits to its own content plus the previous row's hash, so altering anything after the fact breaks the chain in a way `validateIntegrity()` is specifically designed to catch. This is intentional tamper-evidence, not an implementation detail to work around. + +- **Never directly `UPDATE`/`DELETE` rows in `audit_log`, `voucher`, `voucher_line`, `opening_balance`, or other ledger tables via raw SQL to fix bad data** - not even when the fix is obviously correct and the user asks for it directly. This is exactly what caused a real incident: a prior session patched incorrect opening balances straight in the database, which silently broke `entry_hash` for the affected rows without anyone noticing until much later. +- Data corrections must go through the application's own domain operations, not raw SQL, so the correction itself produces a new, properly-chained audit entry describing what changed and why. `VoucherService`'s correction-voucher pattern (`CORRECTION_VOUCHER`, `recordCorrectionVoucher`) is the model to follow: never edit a wrong voucher in place, post a new one that references and reverses it. Extend that same append-only pattern to other data types rather than reaching for an `UPDATE` statement. +- If you discover a hash-chain integrity violation - from a bug, a previous manual patch, or anything else - do not "fix" it by rewriting `entry_hash`/`previous_hash` on the broken row, and do not call `AuditLogService.rebuildIntegrityChain`/`repairIntegrityForAllCompanies` casually. Those exist solely as one-time, migration-triggered repair tools for specific historical bugs (see `V27__audit_log_decouple_references.sql` and `DatabaseService.initialize()`), gated to run exactly once, never as general-purpose "make the warning go away" utilities - doing so would silently mask real tampering going forward. Use `AuditLogService.recordIntegrityRemediation(companyId, auditLogId, reason)` instead: it appends a new, normally-chained entry documenting *why* a specific row's mismatch is a known, explained anomaly, which moves it from `validateIntegrity()`'s blocking critical list to `listDocumentedExceptions()` - visible and no longer blocking, without ever touching the original row. +- If you are unsure whether something needs a proper code fix versus a one-off manual correction, stop and ask the user before running SQL against a live database - do not decide unilaterally. + ## Agent-Specific Notes - Always ask before creating a new git branch. - Never push directly to `main` unless the user explicitly says to push `main`, or you ask for and receive confirmation immediately before pushing. From 6f0ede6111069162597542f8a25171912bb37613 Mon Sep 17 00:00:00 2001 From: Per Nyfelt Date: Fri, 24 Jul 2026 15:44:28 +0200 Subject: [PATCH 3/3] =?UTF-8?q?=C3=A5tg=C3=A4rda=20granskningsfynd:=20bind?= =?UTF-8?q?=20r=C3=A4ttelse=20till=20exakt=20observerad=20avvikelse?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1: exemptionen nycklades bara på auditLogId, så när en rad väl fått en rättelsepost flyttades varje nuvarande OCH framtida hash-/föregående- hash-avvikelse på den raden ur validateIntegrity()'s blockerande lista - oavsett om den nya avvikelsen hade något med den ursprungligen förklarade att göra. En andra, annorlunda manipulation av en redan rättad rad maskerades därmed tyst. recordIntegrityRemediation() fångar nu radens exakta avvikande tillstånd vid rättelsetillfället (lagrat entry_hash, lagrat previous_hash, samt den hash som innehållet borde ge om det inte var manipulerat) och lagrar det i rättelsepostens details-text. checkIntegrityForCompany() undantar en rad bara när dess NUVARANDE tillstånd exakt matchar en tidigare dokumenterad avvikelse - ändras raden igen, på något sätt, matchar inget tidigare dokumenterat tillstånd längre och den flaggas som kritisk på nytt. Nytt regressionstest bevisar detta: rättar en rad, manipulerar den sedan en andra gång på ett annat sätt, och verifierar att den dyker upp i den kritiska listan igen i stället för att tystas av den första rättelsen. Justerade också en CodeNarc-varning (onödig null-check) och synkade det kvarvarande hårdkodade "id = 1" i validateIntegrityDetectsTamperedAuditRow med idOfRow-hjälparen, enligt granskningens förslag. Co-Authored-By: Claude Sonnet 5 --- .../accounting/service/AuditLogService.groovy | 99 +++++++++++++++---- .../service/AuditLogServiceTest.groovy | 27 ++++- 2 files changed, 105 insertions(+), 21 deletions(-) diff --git a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy index 96bfbf2..0b50ae1 100644 --- a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy +++ b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy @@ -184,18 +184,40 @@ final class AuditLogService { * This deliberately never touches the broken row's own entry_hash/previous_hash: doing so * would mean the chain no longer commits to what actually happened, which is the entire * point of keeping it append-only. It appends a new, normally-chained - * {@link #INTEGRITY_REMEDIATION} entry instead, pointing at the row it explains. The - * target row must currently have an integrity problem - this is for documenting real, + * {@link #INTEGRITY_REMEDIATION} entry instead, capturing the row's exact anomalous state + * (its stored entry_hash and previous_hash, plus what entry_hash should be given its + * current content) at the moment it is explained. Suppression in + * {@link #checkIntegrityForCompany} only ever applies while the row still matches that + * exact captured state - if the row changes again afterwards, by any means, none of those + * three values will still match and it reverts to critical. Without this, one remediation + * would permanently silence *any* future tampering of the same row, not just the anomaly + * it was written to explain. + * + * The target row must currently have an integrity problem - this is for documenting real, * already-diagnosed anomalies, not for pre-emptively annotating healthy rows. */ AuditLogEntry recordIntegrityRemediation(long companyId, long remediatedAuditLogId, String reason) { CompanyService.requireValidCompanyId(companyId) String safeReason = requireText(reason, 'Anledning') databaseService.withTransaction { Sql sql -> - GroovyRowResult target = sql.firstRow( - 'select id from audit_log where id = ? and company_id = ?', - [remediatedAuditLogId, companyId] - ) as GroovyRowResult + GroovyRowResult target = sql.firstRow(''' + select id, + event_type as eventType, + voucher_id as voucherId, + attachment_id as attachmentId, + fiscal_year_id as fiscalYearId, + accounting_period_id as accountingPeriodId, + vat_period_id as vatPeriodId, + actor, + summary, + details, + previous_hash as previousHash, + entry_hash as entryHash, + created_at as createdAt + from audit_log + where id = ? + and company_id = ? + ''', [remediatedAuditLogId, companyId]) as GroovyRowResult if (target == null) { throw new IllegalArgumentException("Auditlogg ${remediatedAuditLogId} finns inte för detta företag.") } @@ -208,8 +230,11 @@ final class AuditLogService { recordEvent(sql, INTEGRITY_REMEDIATION, AuditReferences.EMPTY, "Manuell rättelse dokumenterad för auditlogg ${remediatedAuditLogId}", formatDetails([ - remediatesAuditLogId: remediatedAuditLogId, - reason : safeReason + remediatesAuditLogId : remediatedAuditLogId, + remediatedEntryHash : target.get('entryHash'), + remediatedPreviousHash : target.get('previousHash'), + remediatedContentHash : calculatedHashOfRow(target), + reason : safeReason ]), companyId) } } @@ -247,9 +272,6 @@ final class AuditLogService { ''', [companyId]).each { GroovyRowResult row -> AuditLogEntry entry = mapEntry(row) boolean archived = row.get('archived') as Boolean - if (entry.previousHash != expectedPreviousHash && entry.previousHash != expectedLastLiveHash) { - rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har fel föregående hash." as String) - } String calculated = calculateHash(new AuditEntrySeed( eventType: entry.eventType, voucherId: entry.voucherId, @@ -263,8 +285,12 @@ final class AuditLogService { previousHash: entry.previousHash, createdAt: entry.createdAt )) + String signature = integritySignature(entry.entryHash, entry.previousHash, calculated) + if (entry.previousHash != expectedPreviousHash && entry.previousHash != expectedLastLiveHash) { + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har fel föregående hash." as String, signature) + } if (entry.entryHash != calculated) { - rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har ogiltig hash." as String) + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har ogiltig hash." as String, signature) } expectedPreviousHash = entry.entryHash if (!archived) { @@ -279,12 +305,13 @@ final class AuditLogService { structuralProblems << 'Auditloggkedjans huvud pekar inte på sista auditraden.' } - Set remediatedIds = loadRemediatedAuditLogIds(sql, companyId) + Map> remediatedSignatures = loadRemediationSignatures(sql, companyId) List critical = [] List documented = [] Set criticalIds = [] as Set rowProblems.each { RowProblem problem -> - if (remediatedIds.contains(problem.auditLogId)) { + Set signaturesForRow = remediatedSignatures[problem.auditLogId] + if (signaturesForRow?.contains(problem.signature)) { documented << problem.message } else { critical << problem.message @@ -297,19 +324,49 @@ final class AuditLogService { new IntegrityCheckResult(critical, documented, criticalIds) } - private static Set loadRemediatedAuditLogIds(Sql sql, long companyId) { - Set ids = [] as Set + /** + * For each remediated row id, the set of exact anomalous states (entry_hash|previous_hash| + * calculated-content-hash) that have been explained for it. A row problem only gets + * suppressed when its *current* state matches one of these exactly - see + * {@link #recordIntegrityRemediation} for why. + */ + private static Map> loadRemediationSignatures(Sql sql, long companyId) { + Map> signatures = [:] sql.rows( 'select details from audit_log where company_id = ? and event_type = ?', [companyId, INTEGRITY_REMEDIATION] ).each { GroovyRowResult row -> Map parsed = parseDetails(SqlValueMapper.toClob(row.get('details'))) Long remediatedId = longOrNull(parsed.remediatesAuditLogId) - if (remediatedId != null) { - ids << remediatedId + if (remediatedId == null) { + return } + String signature = integritySignature( + parsed.remediatedEntryHash, parsed.remediatedPreviousHash, parsed.remediatedContentHash + ) + signatures.computeIfAbsent(remediatedId) { [] as Set } << signature } - ids + signatures + } + + private static String integritySignature(String storedEntryHash, String storedPreviousHash, String calculatedContentHash) { + "${storedEntryHash ?: ''}|${storedPreviousHash ?: ''}|${calculatedContentHash ?: ''}" + } + + private static String calculatedHashOfRow(GroovyRowResult row) { + calculateHash(new AuditEntrySeed( + eventType: row.get('eventType') as String, + voucherId: longOrNull(row.get('voucherId')), + attachmentId: longOrNull(row.get('attachmentId')), + fiscalYearId: longOrNull(row.get('fiscalYearId')), + accountingPeriodId: longOrNull(row.get('accountingPeriodId')), + vatPeriodId: longOrNull(row.get('vatPeriodId')), + actor: row.get('actor') as String, + summary: row.get('summary') as String, + details: SqlValueMapper.toClob(row.get('details')), + previousHash: row.get('previousHash') as String, + createdAt: SqlValueMapper.toLocalDateTime(row.get('createdAt')) + )) } AuditLogEntry logImport(String summary, String details = null, long companyId = CompanyService.LEGACY_COMPANY_ID) { @@ -897,10 +954,12 @@ final class AuditLogService { final long auditLogId final String message + final String signature - private RowProblem(long auditLogId, String message) { + private RowProblem(long auditLogId, String message, String signature) { this.auditLogId = auditLogId this.message = message + this.signature = signature } } diff --git a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy index 773d47d..b95bf49 100644 --- a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy +++ b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy @@ -61,9 +61,10 @@ class AuditLogServiceTest { @Test void validateIntegrityDetectsTamperedAuditRow() { auditLogService.logImport('Importerade SIE', 'jobId=1') + long rowId = idOfRow(databaseService, "summary = 'Importerade SIE'") databaseService.withTransaction { Sql sql -> - sql.executeUpdate("update audit_log set summary = 'tampered' where id = 1") + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', rowId]) } List problems = auditLogService.validateIntegrity() @@ -97,6 +98,30 @@ class AuditLogServiceTest { 'The remediation entry itself, and everything else, must still form a valid chain') } + @Test + void recordIntegrityRemediationDoesNotMaskALaterDifferentTamperOfTheSameRow() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long rowId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered once', rowId]) + } + auditLogService.recordIntegrityRemediation(companyId, rowId, 'Rättad efter felsökning av det första felet.') + assertTrue(auditLogService.validateIntegrity(companyId).isEmpty(), + 'Sanity check: the first anomaly must be documented before the real assertion below') + + // A second, unrelated change to the very same row - the remediation above explained the + // *first* anomaly, not a blanket exemption for this row forever. + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered twice, differently', rowId]) + } + + List critical = auditLogService.validateIntegrity(companyId) + assertTrue(critical.any { String problem -> problem.contains("Auditlogg ${rowId} ") }, + 'A second, different tamper of an already-remediated row must NOT be silently masked by the earlier remediation') + } + @Test void recordIntegrityRemediationRejectsARowWithNoIntegrityProblem() { long companyId = CompanyService.LEGACY_COMPANY_ID