fix: respect isEncrypted:false if supplied in notify: command, and ensure the correct value is always transmitted onwards

packages/at_secondary_server/lib/src/notification/resource_manager.dart

@@ -235,10 +235,11 @@ class ResourceManager {
  commandBody =
  '${AtConstants.sharedKeyEncrypted}:${atNotification.atMetadata!.sharedKeyEnc}:$commandBody';
  }
- if (atNotification.atMetadata!.isEncrypted != null &&
- atNotification.atMetadata!.isEncrypted == true) {
- commandBody = '${AtConstants.isEncrypted}:true:$commandBody';
- }
+
+ String? isEncryptedStr =
+ (atNotification.atMetadata!.isEncrypted ?? false) ? 'true' : 'false';
+ commandBody = '${AtConstants.isEncrypted}:$isEncryptedStr:$commandBody';
+
  if (atMetaData.ttr != null) {
  commandBody =
  'ttr:${atMetaData.ttr}:ccd:${atMetaData.isCascade}:$commandBody';

packages/at_secondary_server/lib/src/verb/handler/notify_verb_handler.dart

@@ -375,7 +375,7 @@ class NotifyVerbHandler extends AbstractVerbHandler {
  atMetadata.skeEncAlgo =
  verbParams[AtConstants.sharedKeyEncryptedEncryptingAlgo];
  }
- atMetadata.isEncrypted = _getIsEncrypted(
+ atMetadata.isEncrypted = getIsEncrypted(
  getMessageType(verbParams[AtConstants.messageType]),
  verbParams[AtConstants.atKey]!,
  verbParams[AtConstants.isEncrypted]);
@@ -468,15 +468,21 @@ class NotifyVerbHandler extends AbstractVerbHandler {
  return NotificationType.sent;
  }
 
- bool _getIsEncrypted(
+ @visibleForTesting
+ bool getIsEncrypted(
  MessageType messageType, String key, String? isEncryptedStr) {
  if (messageType == MessageType.key && key.startsWith('public')) {
  return false;
- } else if (messageType == MessageType.text) {
+ }
+ if (messageType == MessageType.text) {
  return SecondaryUtil.getBoolFromString(isEncryptedStr);
- } else {
- return true;
  }
+ // respect the 'false' value if one was supplied
+ if (isEncryptedStr != null && isEncryptedStr.toLowerCase() == 'false') {
+ return false;
+ }
+ // At this point, has to return true for legacy reasons. See #1944
+ return true;
  }
 
  String _getFullFormedAtKey(

packages/at_secondary_server/test/notify_verb_test.dart

@@ -1519,7 +1519,7 @@ void main() {
  expect(response.data, isNotNull);
  var notificationIdBuzzKey = response.data;
 
- //3. Notify key without namepsace
+ //3. Notify key without namespace
  notifyCommand = 'notify:$bob:location$alice';
  notifyVerbParams = getVerbParam(VerbSyntax.notify, notifyCommand);
  await notifyVerbHandler.processVerb(
@@ -1570,7 +1570,7 @@ void main() {
  e.message ==
  'Connection with enrollment ID $newEnrollmentId is not authorized to fetch notify key: @bob:email.buzz@alice')));
 
- //3. fetching notification key with no namepsace should throw exception
+ //3. fetching notification key with no namespace should throw exception
  notifyFetchCommand = 'notify:fetch:$notificationIdKeyNoNamespace';
  notifyFetchVerbParams =
  getVerbParam(VerbSyntax.notifyFetch, notifyFetchCommand);
@@ -1661,7 +1661,7 @@ void main() {
  expect(response.data, isNotNull);
  var notificationIdBuzzKey = response.data;
 
- //3. Notify key without namepsace
+ //3. Notify key without namespace
  notifyCommand = 'notify:$bob:location$alice';
  notifyVerbParams = getVerbParam(VerbSyntax.notify, notifyCommand);
  await notifyVerbHandler.processVerb(
@@ -1754,7 +1754,7 @@ void main() {
  inboundConnection = DummyInboundConnection();
  registerFallbackValue(inboundConnection);
  });
- test('A test to verify notifylist authorization', () async {
+ test('A test to verify notify:list authorization', () async {
  // create a set of notifications on a connection with * namespace access.
  Response response = Response();
  inboundConnection.metadata.isAuthenticated =
@@ -1791,7 +1791,7 @@ void main() {
  response, notifyVerbParams, inboundConnection);
  expect(response.data, isNotNull);
 
- //3. Notify key without namepsace
+ //3. Notify key without namespace
  notifyCommand = 'notify:$alice:location$alice';
  notifyVerbParams = getVerbParam(VerbSyntax.notify, notifyCommand);
  await notifyVerbHandler.processVerb(
@@ -1847,6 +1847,99 @@ void main() {
  });
  tearDown(() async => await tearDownFunc());
  });
+
+ group('Notification data correctness tests', () {
+ late NotifyVerbHandler notifyVerbHandler;
+
+ setUp(() async {
+ keyStoreManager = await setUpFunc(storageDir, atsign: '@alice');
+ SecondaryKeyStore keyStore = keyStoreManager.getKeyStore();
+ notifyVerbHandler = NotifyVerbHandler(keyStore);
+ inboundConnection = DummyInboundConnection();
+ registerFallbackValue(inboundConnection);
+ });
+
+ tearDown(() async => await tearDownFunc());
+
+ test('test getIsEncrypted for MessageType.text', () {
+ expect(notifyVerbHandler.getIsEncrypted(MessageType.text, 'foo', null),
+ false);
+ expect(notifyVerbHandler.getIsEncrypted(MessageType.text, 'foo', 'false'),
+ false);
+ expect(notifyVerbHandler.getIsEncrypted(MessageType.text, 'foo', 'true'),
+ true);
+ });
+
+ test('test getIsEncrypted for public key notifications', () {
+ // TODO Is there any use currently of notifications on 'public' keys?
+ // TODO Are there any functional or e2e tests of the same?
+ // TODO Could be quite useful in some scenarios.
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, 'public:foo.bar@alice', null),
+ false);
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, 'public:foo.bar@alice', 'false'),
+ false);
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, 'public:foo.bar@alice', 'true'),
+ false);
+ });
+
+ test('test getIsEncrypted for key notifications', () {
+ // In order not to break old clients which had faulty assumptions, we
+ // need to return true when isEncrypted is not provided in the notify
+ // request rather than returning false as one would expect.
+ // The reason is that those broken old clients do not set `isEncrypted`
+ // on notifications when they ought to be doing so.
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, '@bob:foo.bar@alice', null),
+ true);
+
+ // This used to return 'true' as well, which is definitively wrong.
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, '@bob:foo.bar@alice', 'false'),
+ false);
+
+ expect(
+ notifyVerbHandler.getIsEncrypted(
+ MessageType.key, '@bob:foo.bar@alice', 'true'),
+ true);
+ });
+
+ test('test notify:isEncrypted:false is respected', () async {
+ Response response = Response();
+ String notifyCommand =
+ 'notify:update:isEncrypted:false:@bob:metadata.notify.test@alice:hello';
+ HashMap<String, String?> notifyVerbParams =
+ getVerbParam(VerbSyntax.notify, notifyCommand);
+
+ // execute the notify verb
+ inboundConnection.metadata.isAuthenticated = true;
+ await notifyVerbHandler.processVerb(
+ response, notifyVerbParams, inboundConnection);
+ expect(response.isError, false);
+ expect(response.data, isNotNull);
+
+ // verify that data in the notification keyStore is as expected
+ var notifId = response.data;
+ var stored = await AtNotificationKeystore.getInstance().get(notifId);
+ expect(stored, isNotNull);
+ print(stored!.toJson());
+ expect(stored.toAtSign, '@bob');
+ expect(stored.notification, '@bob:metadata.notify.test@alice');
+ expect(stored.fromAtSign, '@alice');
+ expect(stored.opType, OperationType.update);
+ expect(stored.messageType, MessageType.key);
+ expect(stored.atMetadata, isNotNull);
+ expect(stored.atMetadata!.ttr, isNull);
+ expect(stored.atMetadata!.isEncrypted, false);
+ });
+ });
 }
 
 Future<SecondaryKeyStoreManager> setUpFunc(storageDir, {String? atsign}) async {
@@ -1866,9 +1959,9 @@ Future<SecondaryKeyStoreManager> setUpFunc(storageDir, {String? atsign}) async {
  .getCommitLog(atsign ?? '@test_user_1', commitLogPath: storageDir);
  await AtAccessLogManagerImpl.getInstance()
  .getAccessLog(atsign ?? '@test_user_1', accessLogPath: storageDir);
- var notificationInstance = AtNotificationKeystore.getInstance();
- notificationInstance.currentAtSign = atsign ?? '@test_user_1';
- await notificationInstance.init(storageDir);
+ var notificationKeystore = AtNotificationKeystore.getInstance();
+ notificationKeystore.currentAtSign = atsign ?? '@test_user_1';
+ await notificationKeystore.init(storageDir);
  return keyStoreManager;
 }
 

packages/at_secondary_server/test/resource_manager_test.dart

@@ -178,6 +178,7 @@ void main() async {
  'id:1234:update:messageType:key:notifier:system'
  ':ttln:$ttln'
  ':ttr:1:ccd:true'
+ ':isEncrypted:false'
  ':sharedKeyEnc:abc:pubKeyCS:123'
  ':encKeyName:ekn:encAlgo:ea:ivNonce:ivn'
  ':skeEncKeyName:ske_ekn:skeEncAlgo:ske_ea'