Merge pull request #1829 from atsign-foundation/fix/fix-otp-put

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

@@ -65,15 +65,19 @@ abstract class AbstractUpdateVerbHandler extends ChangeVerbHandler {
     final atData = AtData();
     atData.data = value;
 
-    final enrollApprovalId =
-        (atConnection.metaData as InboundConnectionMetadata).enrollmentId;
     bool isAuthorized = true; // for legacy clients allow access by default
-    if (enrollApprovalId != null) {
-      if (atKey.contains('.')) {
-        var keyNamespace = atKey.substring(atKey.lastIndexOf('.') + 1);
-        isAuthorized = await super.isAuthorized(enrollApprovalId, keyNamespace);
-      }
+
+    InboundConnectionMetadata inboundConnectionMetadata =
+        atConnection.metaData as InboundConnectionMetadata;
+
+    if (atKey.contains('.')) {
+      var keyNamespace = atKey.substring(atKey.lastIndexOf('.') + 1);
+      isAuthorized = await super.isAuthorized(
+        inboundConnectionMetadata,
+        keyNamespace,
+      );
     }
+
     // Get the key using verbParams (forAtSign, key, atSign)
     if (sharedWith != null && sharedWith.isNotEmpty) {
       atKey = '$sharedWith:$atKey';
@@ -87,7 +91,8 @@ abstract class AbstractUpdateVerbHandler extends ChangeVerbHandler {
     }
     if (!isAuthorized) {
       throw UnAuthorizedException(
-          'Enrollment Id: $enrollApprovalId is not authorized for update operation on the key: ${atKey.toString()}');
+          'Connection with enrollment ID ${inboundConnectionMetadata.enrollmentId}'
+              ' is not authorized to update key: ${atKey.toString()}');
     }
 
     var keyType = AtKey.getKeyType(atKey, enforceNameSpace: false);

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

@@ -2,6 +2,7 @@ import 'dart:collection';
 import 'dart:convert';
 import 'package:at_commons/at_commons.dart';
 import 'package:at_persistence_secondary_server/at_persistence_secondary_server.dart';
+import 'package:at_secondary/src/connection/inbound/inbound_connection_metadata.dart';
 import 'package:at_secondary/src/constants/enroll_constants.dart';
 import 'package:at_secondary/src/enroll/enroll_datastore_value.dart';
 import 'package:at_secondary/src/server/at_secondary_impl.dart';
@@ -102,57 +103,94 @@ abstract class AbstractVerbHandler implements VerbHandler {
     }
   }
 
-  /// Verifies whether the enrollment namespace for the enrollment
-  /// ID has the necessary permissions to modify, delete, or retrieve the data.
-  /// The enrollment should be in an approved state.
+  /// Verifies whether the current connection has permission to
+  /// modify, delete, or retrieve the data in a given namespace.
   ///
-  /// To execute a data retrieval (lookup or local lookup), the namespace must have
-  /// "r" (read) privileges within the namespace.
-  /// For update or delete actions, the namespace must have "rw" (read-write) privileges.
+  /// The connection's enrollment should be in an approved state.
   ///
-  /// Returns true, if the namespace has the required read or read-write
-  /// permissions to execute lookup/local-lookup or update/delete operations
-  /// respectively
+  /// To execute a data retrieval (lookup or local lookup), the connection
+  /// must have "r" or "rw" (read / read-write) access for the namespace.
   ///
-  /// Returns false
-  ///  - If the enrollment key is not present in the keystore.
-  ///  - If the enrollment is not in "approved" state
-  ///  - If the namespace does not have necessary permissions to perform the operation
+  /// For update or delete, the connection must have "rw" (read-write) access.
+  ///
+  /// Returns true if
+  /// - EITHER the connection has no enrollment ID (i.e. it was the first enrolled
+  ///   app)
+  /// - OR the connection has the required read or read-write
+  ///   permissions to execute lookup/local-lookup or update/delete operations
+  ///   respectively
+  ///
+  /// The connection will be deemed not to have permission if any of the
+  /// following are true:
+  ///  - the enrollment key is not present in the keystore.
+  ///  - the enrollment is not in "approved" state
+  ///  - the connection has no permissions for this namespace
+  ///  - the connection has insufficient permission for this namespace
+  ///    (for example, has "r" but needs "rw" for a delete operation)
   ///  - If enrollment is a part of "global" or "manage" namespace
-  Future<bool> isAuthorized(String enrollmentId, String keyNamespace) async {
+  Future<bool> isAuthorized(
+    InboundConnectionMetadata connectionMetadata,
+    String keyNamespace,
+  ) async {
+    final Verb verb = getVerb();
+
+    final enrollmentId = connectionMetadata.enrollmentId;
+
+    if (enrollmentId == null) {
+      return true;
+    }
+
+    final enrollmentKey =
+        '$enrollmentId.$newEnrollmentKeyPattern.$enrollManageNamespace';
+    final fullKey =
+        '$enrollmentKey${AtSecondaryServerImpl.getInstance().currentAtSign}';
+
+    final EnrollDataStoreValue enrollDataStoreValue;
     try {
-      final enrollmentKey =
-          '$enrollmentId.$newEnrollmentKeyPattern.$enrollManageNamespace';
-      final fullKey =
-          '$enrollmentKey${AtSecondaryServerImpl.getInstance().currentAtSign}';
+      enrollDataStoreValue = await getEnrollDataStoreValue(fullKey);
+    } on KeyNotFoundException {
+      logger.shout('Could not retrieve enrollment data for $fullKey');
+      return false;
+    }
 
-      final enrollDataStoreValue = await getEnrollDataStoreValue(fullKey);
+    if (enrollDataStoreValue.approval?.state !=
+        EnrollmentStatus.approved.name) {
+      logger.warning('Enrollment state for $fullKey'
+          ' is ${enrollDataStoreValue.approval?.state}');
+      return false;
+    }
 
-      if (enrollDataStoreValue.approval?.state !=
-          EnrollmentStatus.approved.name) {
+    final enrollNamespaces = enrollDataStoreValue.namespaces;
+    logger.finer('enrollNamespaces:$enrollNamespaces');
+    logger.finer('keyNamespace:$keyNamespace');
+    final access = enrollNamespaces.containsKey(allNamespaces)
+        ? enrollNamespaces[allNamespaces]
+        : enrollNamespaces[keyNamespace];
+    logger.finer('access:$access');
+
+    logger.shout('Verb: $verb, keyNamespace: $keyNamespace, access: $access');
+
+    if (access == null) {
+      return false;
+    }
+
+    if (keyNamespace == enrollManageNamespace) {
+      if (verb is! Otp && verb is! Enroll) {
+        // Only spp and enroll operations are allowed to access
+        // the enrollManageNamespace
         return false;
       }
+      return access == 'r' || access == 'rw';
+    }
 
-      final enrollNamespaces = enrollDataStoreValue.namespaces;
-      logger.finer('enrollNamespaces:$enrollNamespaces');
-      logger.finer('keyNamespace:$keyNamespace');
-      final access = enrollNamespaces.containsKey(allNamespaces)
-          ? enrollNamespaces[allNamespaces]
-          : enrollNamespaces[keyNamespace];
-      logger.finer('access:$access');
-      if (keyNamespace != enrollManageNamespace && access != null) {
-        final verb = getVerb();
-        if ((verb is LocalLookup || verb is Lookup) &&
-            (access == 'r' || access == 'rw')) {
-          return true;
-        } else if ((verb is Update || verb is Delete) && access == 'rw') {
-          return true;
-        }
-      }
-      return false;
-    } on KeyNotFoundException {
-      return false;
+    if ((verb is LocalLookup || verb is Lookup) &&
+        access == 'r' || access == 'rw') {
+      return true;
+    } else if ((verb is Update || verb is Delete) && access == 'rw') {
+      return true;
     }
+
+    return false;
   }
 
   /// This function checks the validity of a provided OTP.

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

@@ -93,15 +93,19 @@ class DeleteVerbHandler extends ChangeVerbHandler {
       await keyStore.put(
           AtConstants.atCramSecretDeleted, AtData()..data = 'true');
     }
-    final enrollApprovalId =
-        (atConnection.metaData as InboundConnectionMetadata).enrollmentId;
-    bool isAuthorized = true; // for legacy clients allow access by default
-    if (enrollApprovalId != null) {
-      isAuthorized = await super.isAuthorized(enrollApprovalId, keyNamespace);
-    }
+
+    InboundConnectionMetadata inboundConnectionMetadata =
+        atConnection.metaData as InboundConnectionMetadata;
+
+    bool isAuthorized = await super.isAuthorized(
+      inboundConnectionMetadata,
+      keyNamespace,
+    );
+
     if (!isAuthorized) {
       throw UnAuthorizedException(
-          'Enrollment Id: $enrollApprovalId is not authorized for delete operation on the key: $deleteKey');
+          'Connection with enrollment ID ${inboundConnectionMetadata.enrollmentId}'
+              ' is not authorized to delete key: $deleteKey');
     }
     try {
       var result = await keyStore.remove(deleteKey);

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

@@ -60,15 +60,20 @@ class LocalLookupVerbHandler extends AbstractVerbHandler {
     if (verbParams.containsKey('isCached')) {
       key = 'cached:$key';
     }
-    final enrollmentId =
-        (atConnection.metaData as InboundConnectionMetadata).enrollmentId;
+
+    InboundConnectionMetadata inboundConnectionMetadata =
+        atConnection.metaData as InboundConnectionMetadata;
+
     bool isAuthorized = true; // for legacy clients allow access by default
-    if (!isPublic && enrollmentId != null && keyNamespace != null) {
-      isAuthorized = await super.isAuthorized(enrollmentId, keyNamespace);
+
+    if (!isPublic && keyNamespace != null) {
+      isAuthorized = await super.isAuthorized(inboundConnectionMetadata, keyNamespace);
     }
+
     if (!isAuthorized) {
       throw UnAuthorizedException(
-          'Enrollment Id: $enrollmentId is not authorized for local lookup operation on the key: $key');
+          'Connection with enrollment ID ${inboundConnectionMetadata.enrollmentId}'
+              ' is not authorized to llookup key: $key');
     }
     AtData? atData = await keyStore.get(key);
     var isActive = false;

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

@@ -99,7 +99,8 @@ class LookupVerbHandler extends AbstractVerbHandler {
     bool isAuthorized = await _isAuthorizedToViewData(atConnection, lookupKey);
     if (!isAuthorized) {
       throw UnAuthorizedException(
-          'Enrollment Id: ${(atConnection.metaData as InboundConnectionMetadata).enrollmentId} is not authorized for lookup operation on the key: $lookupKey');
+          'Connection with enrollment ID ${(atConnection.metaData as InboundConnectionMetadata).enrollmentId}'
+              ' is not authorized to lookup key: $lookupKey');
     }
     if (keyOwnersAtSign == thisServersAtSign) {
       // We're looking up data owned by this server's atSign
@@ -271,17 +272,13 @@ class LookupVerbHandler extends AbstractVerbHandler {
     if (!lookupKey.contains('.')) {
       return true;
     }
-    final enrollmentId =
-        (atConnection.metaData as InboundConnectionMetadata).enrollmentId;
-    bool isAuthorized = true; // for legacy clients allow access by default
-    if (enrollmentId != null) {
-      // Extract namespace from the key - 'some_key.wavi@alice' where "wavi" is
-      // is the namespace.
-      String keyNamespace = lookupKey.substring(
-          lookupKey.lastIndexOf('.') + 1, lookupKey.lastIndexOf('@'));
-      isAuthorized = await super.isAuthorized(enrollmentId, keyNamespace);
-    }
-    return isAuthorized;
+
+    // Extract namespace from the key - 'some_key.wavi@alice' where "wavi" is
+    // is the namespace.
+    String keyNamespace = lookupKey.substring(
+        lookupKey.lastIndexOf('.') + 1, lookupKey.lastIndexOf('@'));
+
+    return await super.isAuthorized(atConnection.metaData as InboundConnectionMetadata, keyNamespace);
   }
 
   /// Resolves the value references and returns correct value if value is resolved with in depth of resolution.

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

@@ -125,21 +125,6 @@ class OtpVerbHandler extends AbstractVerbHandler {
   Future<bool> _isClientAuthorizedToStoreSPP(
       InboundConnectionMetadata atConnectionMetadata,
       String currentAtSign) async {
-    var enrollmentKey =
-        '${atConnectionMetadata.enrollmentId}.$newEnrollmentKeyPattern.$enrollManageNamespace$currentAtSign';
-    var enrollNamespaces =
-        (await getEnrollDataStoreValue(enrollmentKey)).namespaces;
-
-    if (enrollNamespaces.isEmpty) {
-      logger.finer(
-          'For the enrollmentId ${atConnectionMetadata.enrollmentId} no namespaces are enrolled. Returning empty list');
-      return false;
-    }
-    // If enrollment namespace contains ".*" return all keys.
-    if (enrollNamespaces.containsKey(enrollManageNamespace) ||
-        enrollNamespaces.containsKey(allNamespaces)) {
-      return true;
-    }
-    return false;
+    return super.isAuthorized(atConnectionMetadata, enrollManageNamespace);
   }
 }

packages/at_secondary_server/test/delete_verb_test.dart

@@ -265,7 +265,7 @@ void main() {
           throwsA(predicate((dynamic e) =>
               e is UnAuthorizedException &&
               e.message ==
-                  'Enrollment Id: $enrollmentId is not authorized for delete operation on the key: dummykey.wavi@alice')));
+                  'Connection with enrollment ID $enrollmentId is not authorized to delete key: dummykey.wavi@alice')));
     });
 
     test('A test to verify keys with unauthorized namespace are not deleted',
@@ -304,7 +304,7 @@ void main() {
           throwsA(predicate((dynamic e) =>
               e is UnAuthorizedException &&
               e.message ==
-                  'Enrollment Id: $enrollmentId is not authorized for delete operation on the key: dummykey.buzz@alice')));
+                  'Connection with enrollment ID $enrollmentId is not authorized to delete key: dummykey.buzz@alice')));
     });
     tearDown(() async => await verbTestsTearDown());
   });
@@ -350,7 +350,7 @@ void main() {
             throwsA(predicate((dynamic e) =>
                 e is UnAuthorizedException &&
                 e.message ==
-                    'Enrollment Id: $enrollmentId is not authorized for delete operation on the key: @alice:dummykey.wavi@alice')));
+                    'Connection with enrollment ID $enrollmentId is not authorized to delete key: @alice:dummykey.wavi@alice')));
       });
     }
     tearDown(() async => await verbTestsTearDown());

packages/at_secondary_server/test/enroll_verb_test.dart

@@ -568,10 +568,10 @@ void main() {
       expect(status, 'pending');
       //Deny enrollment
       await Future.delayed(Duration(milliseconds: 1));
-      String approveEnrollmentCommand =
+      String denyEnrollmentCommand =
           'enroll:deny:{"enrollmentId":"$enrollmentId"}';
       enrollVerbParams =
-          getVerbParam(VerbSyntax.enroll, approveEnrollmentCommand);
+          getVerbParam(VerbSyntax.enroll, denyEnrollmentCommand);
       inboundConnection.metaData.isAuthenticated = true;
       inboundConnection.metaData.sessionID = 'dummy_session_id';
       await enrollVerbHandler.processVerb(

packages/at_secondary_server/test/local_lookup_verb_test.dart

@@ -356,7 +356,7 @@ void main() {
           throwsA(predicate((dynamic e) =>
               e is UnAuthorizedException &&
               e.message ==
-                  'Enrollment Id: $enrollmentId is not authorized for local lookup operation on the key: $alice:mobile.buzz$alice')));
+                  'Connection with enrollment ID $enrollmentId is not authorized to llookup key: $alice:mobile.buzz$alice')));
     });
   });
 
@@ -401,7 +401,7 @@ void main() {
             throwsA(predicate((dynamic e) =>
                 e is UnAuthorizedException &&
                 e.message ==
-                    'Enrollment Id: $enrollmentId is not authorized for local lookup operation on the key: @alice:dummykey.wavi@alice')));
+                    'Connection with enrollment ID $enrollmentId is not authorized to llookup key: @alice:dummykey.wavi@alice')));
       });
     }
   });

packages/at_secondary_server/test/lookup_verb_test.dart

@@ -842,7 +842,7 @@ void main() {
           throwsA(predicate((e) =>
               e is UnAuthorizedException &&
               e.message ==
-                  'Enrollment Id: $enrollmentId is not authorized for lookup operation on the key: @alice:some_key.buzz@alice')));
+                  'Connection with enrollment ID $enrollmentId is not authorized to lookup key: @alice:some_key.buzz@alice')));
     });
 
     test(
@@ -875,7 +875,7 @@ void main() {
           throwsA(predicate((e) =>
               e is UnAuthorizedException &&
               e.message ==
-                  'Enrollment Id: $enrollmentId is not authorized for lookup operation on the key: @alice:some_key.buzz@bob')));
+                  'Connection with enrollment ID $enrollmentId is not authorized to lookup key: @alice:some_key.buzz@bob')));
     });
     tearDown(() async => await verbTestsTearDown());
   });
@@ -924,7 +924,7 @@ void main() {
             throwsA(predicate((dynamic e) =>
                 e is UnAuthorizedException &&
                 e.message ==
-                    'Enrollment Id: $enrollmentId is not authorized for lookup operation on the key: @alice:dummykey.wavi@bob')));
+                    'Connection with enrollment ID $enrollmentId is not authorized to lookup key: @alice:dummykey.wavi@bob')));
       });
     }
   });

packages/at_secondary_server/test/otp_verb_test.dart

@@ -157,7 +157,8 @@ void main() {
           '$enrollmentId.$newEnrollmentKeyPattern.$enrollManageNamespace$alice';
       EnrollDataStoreValue enrollDataStoreValue = EnrollDataStoreValue(
           'dummy_session_id', 'dummy-app', 'dummy-device', 'dummy-apkam-key')
-        ..namespaces = {enrollManageNamespace: 'rw'};
+        ..namespaces = {enrollManageNamespace: 'rw'}
+        ..approval = EnrollApproval(EnrollmentStatus.approved.name);
       await secondaryKeyStore.put(
           enrollmentKey, AtData()..data = jsonEncode(enrollDataStoreValue));
     });

packages/at_secondary_server/test/test_utils.dart

@@ -99,6 +99,10 @@ verbTestsSetUp() async {
 
   secondaryKeyStore = secondaryPersistenceStore!.getSecondaryKeyStore()!;
 
+  var notificationKeystore = AtNotificationKeystore.getInstance();
+  notificationKeystore.currentAtSign = alice;
+  await notificationKeystore.init(storageDir);
+
   mockSecondaryAddressFinder = MockSecondaryAddressFinder();
   when(() => mockSecondaryAddressFinder.findSecondary(bob))
       .thenAnswer((_) async {