fix: Commit log special key bug

packages/at_persistence_secondary_server/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 3.0.65
+- fix: Modified checks in _specialKey method in commit log keystore to match only reserved shared_key and encryption public key.
 ## 3.0.64
 - build[deps]: Upgraded the following packages:
   - at_commons to v5.0.0

packages/at_persistence_secondary_server/lib/src/log/commitlog/commit_log_keystore.dart

@@ -61,7 +61,7 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {
   Future<int?> lastCommittedSequenceNumberWithRegex(String regex,
       {List<String>? enrolledNamespace}) async {
     var lastCommittedEntry = (getBox() as Box).values.lastWhere(
-        (entry) => (_acceptKey(entry.atKey, regex,
+        (entry) => (_shouldIncludeKeyInSyncResponse(entry.atKey, regex,
             enrolledNamespace: enrolledNamespace)),
         orElse: () => NullCommitEntry());
     var lastCommittedSequenceNum =
@@ -179,10 +179,13 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {
     }
   }
 
-  bool _acceptKey(String atKey, String regex,
+  /// match a key to be passed in getEntries/getChanges when these conditions are met
+  /// if enrolledNamespace is passed, key namespace has be in list of enrolled namespace with required authorization
+  /// if regex is passed, key has to match the regex or it has to be a special key.
+  bool _shouldIncludeKeyInSyncResponse(String atKey, String regex,
       {List<String>? enrolledNamespace}) {
     return _isNamespaceAuthorised(atKey, enrolledNamespace) &&
-        (_isRegexMatches(atKey, regex) || _isSpecialKey(atKey));
+        (_keyMatchesRegex(atKey, regex) || _alwaysIncludeInSync(atKey));
   }
 
   bool _isNamespaceAuthorised(
@@ -214,15 +217,18 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {
     return false;
   }
 
-  bool _isRegexMatches(String atKey, String regex) {
+  bool _keyMatchesRegex(String atKey, String regex) {
     return RegExp(regex).hasMatch(atKey);
   }
 
-  bool _isSpecialKey(String atKey) {
-    return atKey.contains(AtConstants.atEncryptionSharedKey) ||
-        atKey.startsWith('public:') ||
-        atKey.contains(AtConstants.atPkamSignature) ||
-        atKey.contains(AtConstants.atSigningPrivateKey);
+  /// match keys which have to included in sync irrespective of whether regex matches
+  /// e.g @bob:shared_key@alice, shared_key.bob@alice, public:publickey@alice,
+  /// public:phone@alice (public key without namespace)
+  bool _alwaysIncludeInSync(String atKey) {
+    return (atKey.contains(AtConstants.atEncryptionSharedKey) &&
+            RegexUtil.keyType(atKey, false) == KeyType.reservedKey) ||
+        atKey.startsWith(AtConstants.atEncryptionPublicKey) ||
+        (atKey.startsWith('public:') && !atKey.contains('.'));
   }
 
   /// Returns the latest commitEntry of the key.
@@ -238,7 +244,7 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {
             .entriesList()
             .where((element) =>
                 element.value.commitId! >= commitId &&
-                _acceptKey(element.value.atKey!, regex))
+                _shouldIncludeKeyInSyncResponse(element.value.atKey!, regex))
             .take(limit);
     return commitEntriesIterable.iterator;
   }
@@ -420,7 +426,7 @@ class ClientCommitLogKeyStore extends CommitLogKeyStore {
       // Iterate through the regex's in the _lastSyncedEntryCacheMap.
       // Updates the commitEntry against the matching regexes.
       for (var regex in _lastSyncedEntryCacheMap.keys) {
-        if (_acceptKey(commitEntry.atKey!, regex)) {
+        if (_shouldIncludeKeyInSyncResponse(commitEntry.atKey!, regex)) {
           _lastSyncedEntryCacheMap[regex] = commitEntry;
         }
       }
@@ -446,7 +452,7 @@ class ClientCommitLogKeyStore extends CommitLogKeyStore {
       limit ??= values.length + 1;
       for (CommitEntry element in values) {
         if (element.key >= startKey &&
-            _acceptKey(element.atKey!, regexString) &&
+            _shouldIncludeKeyInSyncResponse(element.atKey!, regexString) &&
             changes.length <= limit) {
           if (element.commitId == null) {
             changes.add(element);
@@ -483,8 +489,8 @@ class ClientCommitLogKeyStore extends CommitLogKeyStore {
     // Returns the commitEntry with maximum commitId matching the given regex.
     // otherwise returns NullCommitEntry
     lastSyncedEntry = values.lastWhere(
-        (entry) =>
-            (_acceptKey(entry!.atKey!, regex) && (entry.commitId != null)),
+        (entry) => (_shouldIncludeKeyInSyncResponse(entry!.atKey!, regex) &&
+            (entry.commitId != null)),
         orElse: () => NullCommitEntry());
 
     if (lastSyncedEntry == null || lastSyncedEntry is NullCommitEntry) {
@@ -564,7 +570,8 @@ class CommitLogCache {
     if (existingCommitId != null &&
         commitEntry.commitId != null &&
         existingCommitId > commitEntry.commitId!) {
-      _logger.shout('Ignoring commit entry update to cache. existingCommitId: $existingCommitId | toUpdateWithCommitId: ${commitEntry.commitId}');
+      _logger.shout(
+          'Ignoring commit entry update to cache. existingCommitId: $existingCommitId | toUpdateWithCommitId: ${commitEntry.commitId}');
       return;
     }
     _updateCacheLog(key, commitEntry);

packages/at_persistence_secondary_server/pubspec.yaml

@@ -1,6 +1,6 @@
 name: at_persistence_secondary_server
 description: A Dart library with the implementation classes for the persistence layer of the secondary server.
-version: 3.0.64
+version: 3.0.65
 repository: https://github.com/atsign-foundation/at_server
 homepage: https://docs.atsign.com/
 
@@ -19,6 +19,14 @@ dependencies:
   at_persistence_spec: ^2.0.14
   meta: ^1.8.0
 
+#TODO replace with published version
+dependency_overrides:
+  at_commons:
+    git:
+      url: https://github.com/atsign-foundation/at_libraries.git
+      ref: special_key_bug
+      path: packages/at_commons
+
 dev_dependencies:
   lints: ^2.0.1
   test: ^1.22.1

packages/at_persistence_secondary_server/test/commit_log_test.dart

@@ -134,12 +134,12 @@ void main() async {
         //loop to create 10 keys - even keys have commitId null - odd keys have commitId
         for (int i = 0; i < 10; i++) {
           if (i % 2 == 0) {
-            await commitLogKeystore.getBox().add(CommitEntry(
+            await commitLogKeystore.add(CommitEntry(
                 'test_key_false_$i.wavi@alice',
                 CommitOp.UPDATE,
                 DateTime.now()));
           } else {
-            await commitLogKeystore.getBox().add(CommitEntry(
+            await commitLogKeystore.add(CommitEntry(
                 'test_key_false_$i.wavi@alice', CommitOp.UPDATE, DateTime.now())
               ..commitId = i);
           }
@@ -250,6 +250,88 @@ void main() async {
         expect(commitId, 0);
         expect(commitLogInstance?.lastCommittedSequenceNumber(), 0);
       });
+      test('test to verify last committed sequenceNumber with regex', () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        await commitLogInstance?.commit(
+            'public:location_1.wavi@alice', CommitOp.UPDATE);
+        await commitLogInstance?.commit(
+            'public:phone.buzz@alice', CommitOp.UPDATE);
+        await commitLogInstance?.commit(
+            'public:location_2.wavi@alice', CommitOp.UPDATE);
+        await commitLogInstance?.commit(
+            'public:email.buzz@alice', CommitOp.UPDATE);
+        expect(
+            await commitLogInstance
+                ?.lastCommittedSequenceNumberWithRegex('buzz'),
+            3);
+        expect(
+            await commitLogInstance
+                ?.lastCommittedSequenceNumberWithRegex('wavi'),
+            2);
+        await commitLogInstance?.commit(
+            'public:location_1.wavi@alice', CommitOp.UPDATE);
+        await commitLogInstance?.commit(
+            'public:location_2.wavi@alice', CommitOp.DELETE);
+        await commitLogInstance?.commit(
+            'public:phone.buzz@alice', CommitOp.DELETE);
+        await commitLogInstance?.commit(
+            'public:email.buzz@alice', CommitOp.DELETE);
+        expect(
+            await commitLogInstance
+                ?.lastCommittedSequenceNumberWithRegex('buzz'),
+            7);
+        expect(
+            await commitLogInstance
+                ?.lastCommittedSequenceNumberWithRegex('wavi'),
+            5);
+      });
+      test(
+          'A test to verify lastCommittedSequenceNumber does not  include key which does not match regex',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(CommitEntry(
+            'public:phone.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'public:email.wavi@alice', CommitOp.UPDATE, DateTime.now()));
+        final lastCommittedSeq = await commitLogKeystore
+            .lastCommittedSequenceNumberWithRegex('.buzz');
+        expect(lastCommittedSeq, 0);
+      });
+      test(
+          'A test to verify lastCommittedSequenceNumber include key which matches regex and enrollednamespace',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(
+            CommitEntry('phone.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(
+            CommitEntry('phone.wavi@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'location.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        final lastCommittedSeq = await commitLogKeystore
+            .lastCommittedSequenceNumberWithRegex('.buzz',
+                enrolledNamespace: ['buzz']);
+        expect(lastCommittedSeq, 2);
+      });
+      test(
+          'A test to verify lastCommittedSequenceNumber does not include key which does not match enrollednamespace',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(
+            CommitEntry('phone.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(
+            CommitEntry('phone.wavi@alice', CommitOp.UPDATE, DateTime.now()));
+        final lastCommittedSeq = await commitLogKeystore
+            .lastCommittedSequenceNumberWithRegex('.*',
+                enrolledNamespace: ['buzz']);
+        expect(lastCommittedSeq, 0);
+      });
     });
     group('A group of commit log compaction tests', () {
       setUp(() async => await setUpFunc(storageDir));
@@ -413,19 +495,18 @@ void main() async {
         var commitLogInstance =
             await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
         // Inserting commitEntry with commitId 0
-        await commitLogInstance!.commitLogKeyStore.getBox().add(
+        await commitLogInstance!.commitLogKeyStore.add(
             CommitEntry('location@alice', CommitOp.UPDATE, DateTime.now())
               ..commitId = 0);
         // Inserting commitEntry with null commitId
-        await commitLogInstance.commitLogKeyStore.getBox().add(
+        await commitLogInstance.commitLogKeyStore.add(
             CommitEntry('location@alice', CommitOp.UPDATE, DateTime.now()));
         // Inserting commitEntry with commitId 2
-        await commitLogInstance.commitLogKeyStore.getBox().add(
+        await commitLogInstance.commitLogKeyStore.add(
             CommitEntry('phone@alice', CommitOp.UPDATE, DateTime.now())
               ..commitId = 2);
         // Inserting commitEntry with null commitId
         await commitLogInstance.commitLogKeyStore
-            .getBox()
             .add(CommitEntry('mobile@alice', CommitOp.UPDATE, DateTime.now()));
 
         var commitLogMap = await commitLogInstance.commitLogKeyStore.toMap();
@@ -627,28 +708,23 @@ void main() async {
         //loop to create 10 keys - even keys have commitId null - odd keys have commitId
         for (int i = 0; i < 10; i++) {
           if (i % 2 == 0) {
-            await commitLogKeystore.getBox().add(CommitEntry(
-                'test_key_true_$i', CommitOp.UPDATE, DateTime.now()));
+            await commitLogKeystore.add(CommitEntry(
+                'test_key_true_$i@alice', CommitOp.UPDATE, DateTime.now()));
           } else {
-            await commitLogKeystore.getBox().add(
-                CommitEntry('test_key_true_$i', CommitOp.UPDATE, DateTime.now())
-                  ..commitId = i);
+            await commitLogKeystore.add(CommitEntry(
+                'test_key_true_$i@alice', CommitOp.UPDATE, DateTime.now())
+              ..commitId = i);
           }
         }
         Iterator<MapEntry<String, CommitEntry>>? changes =
             commitLogInstance.commitLogKeyStore.getEntries(-1);
         //run loop to ensure all commit entries have been returned; irrespective of commitId null or not
         int i = 0;
         while (changes.moveNext()) {
-          if (i % 2 == 0) {
-            //while creation of commit entries, even keys have been set with commitId == null
-            expect(changes.current.value.commitId, null);
-          } else {
-            //while creation of commit entries, even keys have been set with commitId equal to iteration count
-            expect(changes.current.value.commitId, i);
-          }
+          expect(changes.current.value.commitId, i);
           i++;
         }
+        expect(i, 10);
       });
       test(
           'verify that CommitEntry with higher commitId is retained in cache for the same key',
@@ -671,6 +747,99 @@ void main() async {
             thirdCommitId! > firstCommitId! && thirdCommitId > secondCommitId!);
         expect(commitEntryInCache?.commitId, thirdCommitId);
       });
+      test(
+          'A test to verify only reserved key - shared key is returned in getEntries when namespace is passed',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(CommitEntry(
+            '@bob:shared_key@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'shared_key.bob@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'bob:test_shared_key@alice', CommitOp.UPDATE, DateTime.now()));
+        Iterator<MapEntry<String, CommitEntry>>? changes =
+            commitLogInstance.commitLogKeyStore.getEntries(-1, regex: '.buzz');
+        Map<String?, CommitEntry> commitEntriesMap = {};
+        while (changes.moveNext()) {
+          final commitEntry = changes.current.value;
+          commitEntriesMap[commitEntry.atKey] = commitEntry;
+        }
+        expect(commitEntriesMap.containsKey('@bob:shared_key@alice'), true);
+        expect(commitEntriesMap.containsKey('shared_key.bob@alice'), true);
+        expect(
+            commitEntriesMap.containsKey('bob:test_shared_key@alice'), false);
+      });
+
+      test(
+          'A test to verify non reserved key - shared key is returned in getEntries only when namespace is passed and key namespace matches passed namespace',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(CommitEntry(
+            '@bob:shared_key@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'shared_key.bob@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'bob:test_shared_key.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        Iterator<MapEntry<String, CommitEntry>>? changes =
+            commitLogInstance.commitLogKeyStore.getEntries(-1, regex: '.buzz');
+        Map<String?, CommitEntry> commitEntriesMap = {};
+        while (changes.moveNext()) {
+          final commitEntry = changes.current.value;
+          commitEntriesMap[commitEntry.atKey] = commitEntry;
+        }
+        expect(commitEntriesMap.containsKey('@bob:shared_key@alice'), true);
+        expect(commitEntriesMap.containsKey('shared_key.bob@alice'), true);
+        expect(commitEntriesMap.containsKey('bob:test_shared_key.buzz@alice'),
+            true);
+      });
+      test(
+          'A test to verify public key without namespace is returned in getEntries when no regex is passed',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(
+            CommitEntry('public:phone@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(
+            CommitEntry('public:email@alice', CommitOp.UPDATE, DateTime.now()));
+        Iterator<MapEntry<String, CommitEntry>>? changes =
+            commitLogInstance.commitLogKeyStore.getEntries(-1);
+        Map<String?, CommitEntry> commitEntriesMap = {};
+        while (changes.moveNext()) {
+          final commitEntry = changes.current.value;
+          commitEntriesMap[commitEntry.atKey] = commitEntry;
+        }
+        expect(commitEntriesMap.containsKey('public:phone@alice'), true);
+        expect(commitEntriesMap.containsKey('public:email@alice'), true);
+      });
+
+      test(
+          'A test to verify public keys with matched namespace and no namespace is returned in getEntries when regex is passed',
+          () async {
+        var commitLogInstance =
+            await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
+        var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
+        await commitLogKeystore.add(CommitEntry(
+            'public:phone.buzz@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'public:email.wavi@alice', CommitOp.UPDATE, DateTime.now()));
+        await commitLogKeystore.add(CommitEntry(
+            'public:location@alice', CommitOp.UPDATE, DateTime.now()));
+        Iterator<MapEntry<String, CommitEntry>>? changes =
+            commitLogInstance.commitLogKeyStore.getEntries(-1, regex: '.buzz');
+        Map<String?, CommitEntry> commitEntriesMap = {};
+        while (changes.moveNext()) {
+          final commitEntry = changes.current.value;
+          commitEntriesMap[commitEntry.atKey] = commitEntry;
+        }
+        expect(commitEntriesMap.containsKey('public:phone.buzz@alice'), true);
+        expect(commitEntriesMap.containsKey('public:phone.wavi@alice'), false);
+        expect(commitEntriesMap.containsKey('public:location@alice'), true);
+      });
     });
     tearDown(() async => await tearDownFunc());
   });