From c7eff31db3fa4bae5321ea48b655ee88d30a7f53 Mon Sep 17 00:00:00 2001 From: Herbert Poul Date: Wed, 12 Aug 2020 10:47:33 +0200 Subject: [PATCH] fix creating of history entries. added test for correct dirtyObject events. --- CHANGELOG.md | 3 +- lib/src/kdbx_custom_data.dart | 3 +- lib/src/kdbx_entry.dart | 72 ++++++++++++------------ lib/src/kdbx_file.dart | 5 +- lib/src/kdbx_group.dart | 24 ++++---- lib/src/kdbx_object.dart | 58 +++++++++++++------ lib/src/kdbx_xml.dart | 31 ++++++----- pubspec.yaml | 1 + test/internal/test_utils.dart | 11 +++- test/kdbx_binaries_test.dart | 26 +++++++-- test/kdbx_history_test.dart | 102 ++++++++++++++++++++++++++++++++++ 11 files changed, 243 insertions(+), 93 deletions(-) create mode 100644 test/kdbx_history_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 835b412..c6ce9eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## unreleased -- fixed bug saving files with history entries which contain attachments. +- fix bug saving files with history entries which contain attachments. +- fix bug which would create wrong history entries. ## 0.4.0+1 diff --git a/lib/src/kdbx_custom_data.dart b/lib/src/kdbx_custom_data.dart index fe58f21..ae6426e 100644 --- a/lib/src/kdbx_custom_data.dart +++ b/lib/src/kdbx_custom_data.dart @@ -25,8 +25,7 @@ class KdbxCustomData extends KdbxNode { String operator [](String key) => _data[key]; void operator []=(String key, String value) { - _data[key] = value; - isDirty = true; + modify(() => _data[key] = value); } @override diff --git a/lib/src/kdbx_entry.dart b/lib/src/kdbx_entry.dart index 014c2b3..8556906 100644 --- a/lib/src/kdbx_entry.dart +++ b/lib/src/kdbx_entry.dart @@ -74,11 +74,14 @@ class KdbxEntry extends KdbxObject { return MapEntry(key, KdbxBinary.readBinaryXml(valueNode, isInline: true)); })); - history.addAll(_historyElement - .findElements('Entry') - .map( - (entry) => KdbxEntry.read(ctx, parent, entry, isHistoryEntry: true)) - .toList()); + history.addAll(node + .findElements(KdbxXml.NODE_HISTORY) + .singleOrNull + ?.findElements('Entry') + ?.map((entry) => + KdbxEntry.read(ctx, parent, entry, isHistoryEntry: true)) + ?.toList() ?? + []); } final bool isHistoryEntry; @@ -95,21 +98,10 @@ class KdbxEntry extends KdbxObject { } } - XmlElement get _historyElement => node - .findElements(KdbxXml.NODE_HISTORY) - .singleWhere((_) => true, orElse: () { - final el = XmlElement(XmlName(KdbxXml.NODE_HISTORY)); - node.children.add(el); - return el; - }); - @override - set isDirty(bool newDirty) { - if (!isDirty && newDirty) { - final history = _historyElement; - history.children.add(toXml()); - } - super.isDirty = newDirty; + void onBeforeModify() { + super.onBeforeModify(); + history.add(KdbxEntry.read(ctx, parent, toXml())..file = file); } @override @@ -180,12 +172,13 @@ class KdbxEntry extends KdbxObject { _logger.finest('Value did not change for $key'); return; } - isDirty = true; - if (value == null) { - _strings.remove(key); - } else { - _strings[key] = value; - } + modify(() { + if (value == null) { + _strings.remove(key); + } else { + _strings[key] = value; + } + }); } void renameKey(KdbxKey oldKey, KdbxKey newKey) { @@ -224,22 +217,23 @@ class KdbxEntry extends KdbxObject { isProtected: isProtected, value: bytes, ); - file.ctx.addBinary(binary); - _binaries[key] = binary; - isDirty = true; + modify(() { + file.ctx.addBinary(binary); + _binaries[key] = binary; + }); return binary; } void removeBinary(KdbxKey binaryKey) { - final binary = _binaries.remove(binaryKey); - if (binary == null) { - throw StateError( - 'Trying to remove binary key $binaryKey does not exist.'); - } - if (!binary.isInline) { - file.ctx.removeBinary(binary); - } - isDirty = true; + modify(() { + final binary = _binaries.remove(binaryKey); + if (binary == null) { + throw StateError( + 'Trying to remove binary key $binaryKey does not exist.'); + } + // binary will not be removed (yet) from file, because it will + // be referenced in history. + }); } KdbxKey _uniqueBinaryName(String fileName) { @@ -261,3 +255,7 @@ class KdbxEntry extends KdbxObject { return 'KdbxGroup{uuid=$uuid,name=$label}'; } } + +extension on Iterable { + T get singleOrNull => singleWhere((element) => true, orElse: () => null); +} diff --git a/lib/src/kdbx_file.dart b/lib/src/kdbx_file.dart index 69fbb30..501b0af 100644 --- a/lib/src/kdbx_file.dart +++ b/lib/src/kdbx_file.dart @@ -1,6 +1,7 @@ import 'dart:async'; import 'dart:typed_data'; +import 'package:collection/collection.dart'; import 'package:kdbx/src/crypto/protected_value.dart'; import 'package:kdbx/src/kdbx_consts.dart'; import 'package:kdbx/src/kdbx_dao.dart'; @@ -51,7 +52,7 @@ class KdbxFile { /// Marks all dirty objects as clean. Called by [KdbxFormat.save]. void onSaved() { dirtyObjects.clear(); - _dirtyObjectsChanged.add(dirtyObjects); + _dirtyObjectsChanged.add(const {}); } Iterable get _allObjects => body.rootGroup @@ -61,7 +62,7 @@ class KdbxFile { void dirtyObject(KdbxObject kdbxObject) { dirtyObjects.add(kdbxObject); - _dirtyObjectsChanged.add(dirtyObjects); + _dirtyObjectsChanged.add(UnmodifiableSetView(Set.of(dirtyObjects))); } void dispose() { diff --git a/lib/src/kdbx_group.dart b/lib/src/kdbx_group.dart index 319b2e6..9cc7517 100644 --- a/lib/src/kdbx_group.dart +++ b/lib/src/kdbx_group.dart @@ -65,8 +65,7 @@ class KdbxGroup extends KdbxObject { throw StateError( 'Invalid operation. Trying to add entry which is already in another group.'); } - _entries.add(entry); - isDirty = true; + modify(() => _entries.add(entry)); } void addGroup(KdbxGroup group) { @@ -74,22 +73,23 @@ class KdbxGroup extends KdbxObject { throw StateError( 'Invalid operation. Trying to add group which is already in another group.'); } - _groups.add(group); - isDirty = true; + modify(() => _groups.add(group)); } void internalRemoveGroup(KdbxGroup group) { - if (!_groups.remove(group)) { - throw StateError('Unable to remove $group from $this (Not found)'); - } - isDirty = true; + modify(() { + if (!_groups.remove(group)) { + throw StateError('Unable to remove $group from $this (Not found)'); + } + }); } void internalRemoveEntry(KdbxEntry entry) { - if (!_entries.remove(entry)) { - throw StateError('Unable to remove $entry from $this (Not found)'); - } - isDirty = true; + modify(() { + if (!_entries.remove(entry)) { + throw StateError('Unable to remove $entry from $this (Not found)'); + } + }); } /// returns all parents recursively including this group. diff --git a/lib/src/kdbx_object.dart b/lib/src/kdbx_object.dart index 6bf7b63..5aacad0 100644 --- a/lib/src/kdbx_object.dart +++ b/lib/src/kdbx_object.dart @@ -35,14 +35,38 @@ mixin Changeable { bool _isDirty = false; - set isDirty(bool dirty) { -// _logger.finest('changing dirty (old:$_isDirty) $dirty'); - if (!_isDirty && !dirty) { - // no need for change events when already not-dirty. + /// Called before the *first* modification (ie. before `isDirty` changes + /// from false to true) + @protected + @mustCallSuper + void onBeforeModify() {} + + /// Called after the *first* modification (ie. after `isDirty` changed + /// from false to true) + @protected + @mustCallSuper + void onAfterModify() {} + + RET modify(RET Function() modify) { + if (_isDirty) { + return modify(); + } + onBeforeModify(); + try { + return modify(); + } finally { + _isDirty = true; + onAfterModify(); + _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty)); + } + } + + void clean() { + if (!_isDirty) { return; } - _isDirty = dirty; - _controller.add(ChangeEvent(object: this as T, isDirty: dirty)); + _isDirty = false; + _controller.add(ChangeEvent(object: this as T, isDirty: _isDirty)); } bool get isDirty => _isDirty; @@ -59,6 +83,9 @@ abstract class KdbxNode with Changeable { KdbxNode.read(this.node); + /// XML Node used while reading this KdbxNode. + /// Must NOT be modified. Only copies which are obtained through [toXml]. + /// this node should always represent the original loaded state. final XmlElement node; // @protected @@ -68,7 +95,7 @@ abstract class KdbxNode with Changeable { /// will mark this object as not dirty. @mustCallSuper XmlElement toXml() { - isDirty = false; + clean(); final el = node.copy() as XmlElement; return el; } @@ -108,15 +135,11 @@ abstract class KdbxObject extends KdbxNode { KdbxGroup _parent; @override - set isDirty(bool dirty) { - if (dirty) { - times.modifiedNow(); - if (/*!isDirty && */ dirty) { - // during initial `create` the file will be null. - file?.dirtyObject(this); - } - } - super.isDirty = dirty; + void onAfterModify() { + super.onAfterModify(); + times.modifiedNow(); + // during initial `create` the file will be null. + file?.dirtyObject(this); } @override @@ -128,8 +151,7 @@ abstract class KdbxObject extends KdbxNode { } void internalChangeParent(KdbxGroup parent) { - _parent = parent; - isDirty = true; + modify(() => _parent = parent); } } diff --git a/lib/src/kdbx_xml.dart b/lib/src/kdbx_xml.dart index 572f77d..e93645b 100644 --- a/lib/src/kdbx_xml.dart +++ b/lib/src/kdbx_xml.dart @@ -75,22 +75,23 @@ abstract class KdbxSubTextNode extends KdbxSubNode { if (get() == value) { return; } - node.isDirty = true; - final el = - node.node.findElements(name).singleWhere((x) => true, orElse: () { - final el = XmlElement(XmlName(name)); - node.node.children.add(el); - return el; + node.modify(() { + final el = + node.node.findElements(name).singleWhere((x) => true, orElse: () { + final el = XmlElement(XmlName(name)); + node.node.children.add(el); + return el; + }); + el.children.clear(); + if (value == null) { + return; + } + final stringValue = encode(value); + if (stringValue == null) { + return; + } + el.children.add(XmlText(stringValue)); }); - el.children.clear(); - if (value == null) { - return; - } - final stringValue = encode(value); - if (stringValue == null) { - return; - } - el.children.add(XmlText(stringValue)); } @override diff --git a/pubspec.yaml b/pubspec.yaml index ba50cdd..f48d021 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -35,3 +35,4 @@ dependencies: dev_dependencies: pedantic: '>=1.7.0 <2.0.0' test: '>=1.6.0 <2.0.0' + fake_async: ^1.1.0 diff --git a/test/internal/test_utils.dart b/test/internal/test_utils.dart index 08ccb1d..ee1d0f5 100644 --- a/test/internal/test_utils.dart +++ b/test/internal/test_utils.dart @@ -12,6 +12,8 @@ import 'package:logging/logging.dart'; final _logger = Logger('test_utils'); class TestUtil { + static final keyTitle = KdbxKey('Title'); + static KdbxFormat kdbxFormat() { Argon2.resolveLibraryForceDynamic = true; return KdbxFormat(Argon2FfiFlutter(resolveLibrary: (path) { @@ -35,10 +37,15 @@ class TestUtil { } static Future readKdbxFileBytes(Uint8List data, - {String password = 'asdf'}) async { + {String password = 'asdf', Credentials credentials}) async { final kdbxFormat = TestUtil.kdbxFormat(); final file = await kdbxFormat.read( - data, Credentials(ProtectedValue.fromString(password))); + data, credentials ?? Credentials(ProtectedValue.fromString(password))); return file; } + + static Future saveAndRead(KdbxFile file) async { + return await readKdbxFileBytes(await file.save(), + credentials: file.credentials); + } } diff --git a/test/kdbx_binaries_test.dart b/test/kdbx_binaries_test.dart index 6b60ef1..4d2bdd4 100644 --- a/test/kdbx_binaries_test.dart +++ b/test/kdbx_binaries_test.dart @@ -85,6 +85,18 @@ void main() { final entry = file.body.rootGroup.entries.first; expectKeepass2binariesContents(entry); }); + test('modify file with binary in history', () async { + final fileRead = + await TestUtil.readKdbxFile('test/keepass2binaries.kdbx'); + final updateEntry = (KdbxFile file) { + final entry = fileRead.body.rootGroup.entries.first; + entry.setString(KdbxKey('title'), PlainValue('example')); + }; + updateEntry(fileRead); + final saved = await fileRead.save(); + final file = await TestUtil.readKdbxFileBytes(saved); + await file.save(); + }); test('Add new attachment', () async { await _testAddNewAttachment('test/keepass2binaries.kdbx'); }); @@ -95,7 +107,7 @@ void main() { expectKeepass2binariesContents(entry); expect(file.ctx.binariesIterable, hasLength(3)); entry.removeBinary(KdbxKey('example1.txt')); - expect(file.ctx.binariesIterable, hasLength(2)); + expect(file.ctx.binariesIterable, hasLength(3)); return await file.save(); })(); final file = await TestUtil.readKdbxFileBytes(saved); @@ -103,7 +115,12 @@ void main() { expect(entry.binaryEntries, hasLength(2)); expect(entry.binaryEntries.map((e) => (e.key.key)), ['example2.txt', 'keepasslogo.jpeg']); - expect(file.ctx.binariesIterable, hasLength(2)); + // the file itself will contain 3 items, because it is still + // available in history. + expect(file.ctx.binariesIterable, hasLength(3)); + expect(entry.history.last.binaryEntries, hasLength(3)); + // make sure the file can still be saved. + await file.save(); }); }); group('kdbx4 attachment', () { @@ -139,7 +156,8 @@ void main() { hasLength(7092)); expect(file.ctx.binariesIterable, hasLength(2)); entry.removeBinary(KdbxKey('example2.txt')); - expect(file.ctx.binariesIterable, hasLength(1)); + // the binary remains in the file, since it is referenced in the history + expect(file.ctx.binariesIterable, hasLength(2)); expect(file.dirtyObjects, [entry]); return await file.save(); })(); @@ -148,7 +166,7 @@ void main() { expect(entry.binaryEntries, hasLength(0)); expectBinary(file.body.rootGroup.entries.last, 'keepasslogo.jpeg', hasLength(7092)); - expect(file.ctx.binariesIterable, hasLength(1)); + expect(file.ctx.binariesIterable, hasLength(2)); }); test('Add new attachment kdbx4', () async { await _testAddNewAttachment('test/keepass2kdbx4binaries.kdbx'); diff --git a/test/kdbx_history_test.dart b/test/kdbx_history_test.dart new file mode 100644 index 0000000..f9171bc --- /dev/null +++ b/test/kdbx_history_test.dart @@ -0,0 +1,102 @@ +import 'dart:async'; + +import 'package:kdbx/kdbx.dart'; +import 'package:logging_appenders/logging_appenders.dart'; +import 'package:quiver/core.dart'; +import 'package:test/test.dart'; + +import 'internal/test_utils.dart'; + +class StreamExpect { + StreamExpect(this.stream) { + stream.listen((event) { + if (_expectNext == null) { + fail('Got event, but none was expected. $event'); + } + expect(event, _expectNext.orNull); + _expectNext = null; + }, onDone: () { + expect(_expectNext, isNull); + isDone = true; + }, onError: (dynamic error) { + expect(_expectNext, isNull); + this.error = error; + }); + } + + Future expectNext(T value, FutureOr Function() cb) async { + if (_expectNext != null) { + fail('The last event was never received. last: $_expectNext'); + } + _expectNext = Optional.fromNullable(value); + try { + return await cb(); + } finally { + await pumpEventQueue(); + } + } + + void expectFinished() { + expect(isDone, true); + } + + final Stream stream; + bool isDone = false; + dynamic error; + Optional _expectNext; +} + +void main() { + PrintAppender.setupLogging(); + group('test history for values', () { + test('check history creation', () async { + final file = await TestUtil.readKdbxFile('test/keepass2test.kdbx'); + const valueOrig = 'Sample Entry'; + const value1 = 'new'; + const value2 = 'new2'; + final dirtyExpect = StreamExpect(file.dirtyObjectsChanged); + { + final first = file.body.rootGroup.entries.first; + expect(file.header.versionMajor, 3); + expect(first.getString(TestUtil.keyTitle).getText(), valueOrig); + await dirtyExpect.expectNext({first}, () { + first.setString(TestUtil.keyTitle, PlainValue(value1)); + }); + } + expect(file.dirtyObjects, hasLength(1)); + final f2 = + await dirtyExpect.expectNext({}, () => TestUtil.saveAndRead(file)); + expect(file.dirtyObjects, isEmpty); + { + final first = f2.body.rootGroup.entries.first; + expect(first.getString(TestUtil.keyTitle).getText(), value1); + expect(first.history.last.getString(TestUtil.keyTitle).getText(), + valueOrig); + await dirtyExpect.expectNext({}, () => file.save()); + } + + // edit the original file again, and there should be a second history + { + final first = file.body.rootGroup.entries.first; + await dirtyExpect.expectNext({first}, + () => first.setString(TestUtil.keyTitle, PlainValue(value2))); + } + final f3 = + await dirtyExpect.expectNext({}, () => TestUtil.saveAndRead(file)); + expect(file.dirtyObjects, isEmpty); + { + final first = f3.body.rootGroup.entries.first; + expect(first.getString(TestUtil.keyTitle).getText(), value2); + expect(first.history, hasLength(2)); + expect( + first.history.last.getString(TestUtil.keyTitle).getText(), value1); + expect(first.history.first.getString(TestUtil.keyTitle).getText(), + valueOrig); + await dirtyExpect.expectNext({}, () => file.save()); + } + file.dispose(); + await pumpEventQueue(); + dirtyExpect.expectFinished(); + }); + }); +}