feat: Persist OTPs in keystore and prevent reuse of OTPs

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

@@ -50,15 +50,15 @@ class EnrollVerbHandler extends AbstractVerbHandler {
     try {
       // Ensure that enrollParams are present for all enroll operation
       // Exclude operation 'list' which does not have enrollParams
-      if (verbParams[enrollParams] == null) {
+      if (verbParams[AtConstants.enrollParams] == null) {
         if (operation != 'list') {
           logger.severe(
-              'Enroll params is empty | EnrollParams: ${verbParams[enrollParams]}');
+              'Enroll params is empty | EnrollParams: ${verbParams[AtConstants.enrollParams]}');
           throw IllegalArgumentException('Enroll parameters not provided');
         }
       } else {
-        enrollVerbParams =
-            EnrollParams.fromJson(jsonDecode(verbParams[enrollParams]!));
+        enrollVerbParams = EnrollParams.fromJson(
+            jsonDecode(verbParams[AtConstants.enrollParams]!));
       }
       switch (operation) {
         case 'request':
@@ -116,14 +116,18 @@ class EnrollVerbHandler extends AbstractVerbHandler {
       throw AtThrottleLimitExceeded(
           'Enrollment requests have exceeded the limit within the specified time frame');
     }
-    if (!atConnection.getMetaData().isAuthenticated) {
-      var otp = enrollParams.otp;
-      if (otp == null ||
-          (await OtpVerbHandler.cache.get(otp.toString()) == null)) {
+
+    // OTP is sent only in enrollment request which is submitted on
+    // unauthenticated connection.
+    if (atConnection.getMetaData().isAuthenticated == false) {
+      Response otpResponse = await OtpVerbHandler(keyStore)
+          .processInternal('otp:validate:${enrollParams.otp}', atConnection);
+      if (otpResponse.data == 'invalid') {
         throw AtEnrollmentException(
             'invalid otp. Cannot process enroll request');
       }
     }
+
     var enrollNamespaces = enrollParams.namespaces ?? {};
     var newEnrollmentId = Uuid().v4();
     var key =
@@ -154,8 +158,8 @@ class EnrollVerbHandler extends AbstractVerbHandler {
       await _storeEncryptionKeys(newEnrollmentId, enrollParams, currentAtSign);
       // store this apkam as default pkam public key for old clients
       // The keys with AT_PKAM_PUBLIC_KEY does not sync to client.
-      await keyStore.put(
-          AT_PKAM_PUBLIC_KEY, AtData()..data = enrollParams.apkamPublicKey!);
+      await keyStore.put(AtConstants.atPkamPublicKey,
+          AtData()..data = enrollParams.apkamPublicKey!);
       enrollData = AtData()..data = jsonEncode(enrollmentValue.toJson());
     } else {
       enrollmentValue.approval = EnrollApproval(EnrollStatus.pending.name);
@@ -224,10 +228,6 @@ class EnrollVerbHandler extends AbstractVerbHandler {
     // If an enrollment is approved, we need the enrollment to be active
     // to subsequently revoke the enrollment. Hence reset TTL and
     // expiredAt on metadata.
-    /* TODO: Currently TTL is reset on all the enrollments.
-        However, if the enrollment state is denied or revoked,
-        unless we wanted to display denied or revoked enrollments in the UI,
-        we can let the TTL be, so that the enrollment will be deleted subsequently.*/
     await _updateEnrollmentValueAndResetTTL(
         '$enrollmentKey$currentAtSign', enrollDataStoreValue);
     // when enrollment is approved store the apkamPublicKey of the enrollment
@@ -252,13 +252,13 @@ class EnrollVerbHandler extends AbstractVerbHandler {
     var privKeyJson = {};
     privKeyJson['value'] = enrollParams.encryptedDefaultEncryptedPrivateKey;
     await keyStore.put(
-        '$newEnrollmentId.$defaultEncryptionPrivateKey.$enrollManageNamespace$atSign',
+        '$newEnrollmentId.${AtConstants.defaultEncryptionPrivateKey}.$enrollManageNamespace$atSign',
         AtData()..data = jsonEncode(privKeyJson),
         skipCommit: true);
     var selfKeyJson = {};
     selfKeyJson['value'] = enrollParams.encryptedDefaultSelfEncryptionKey;
     await keyStore.put(
-        '$newEnrollmentId.$defaultSelfEncryptionKey.$enrollManageNamespace$atSign',
+        '$newEnrollmentId.${AtConstants.defaultSelfEncryptionKey}.$enrollManageNamespace$atSign',
         AtData()..data = jsonEncode(selfKeyJson),
         skipCommit: true);
   }
@@ -338,7 +338,7 @@ class EnrollVerbHandler extends AbstractVerbHandler {
       String key, EnrollParams enrollParams, String atSign) async {
     try {
       var notificationValue = {};
-      notificationValue[apkamEncryptedSymmetricKey] =
+      notificationValue[AtConstants.apkamEncryptedSymmetricKey] =
           enrollParams.encryptedAPKAMSymmetricKey;
       logger.finer('notificationValue:$notificationValue');
       final atNotification = (AtNotificationBuilder()
@@ -355,10 +355,10 @@ class EnrollVerbHandler extends AbstractVerbHandler {
       logger.finer('notification generated: $notificationId');
     } on Exception catch (e, trace) {
       logger.severe(
-          'Exception while storing notification key $enrollmentId. Exception $e. Trace $trace');
+          'Exception while storing notification key ${AtConstants.enrollmentId}. Exception $e. Trace $trace');
     } on Error catch (e, trace) {
       logger.severe(
-          'Error while storing notification key $enrollmentId. Error $e. Trace $trace');
+          'Error while storing notification key ${AtConstants.enrollmentId}. Error $e. Trace $trace');
     }
   }
 

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

@@ -1,24 +1,25 @@
 import 'dart:collection';
 import 'dart:math';
 import 'package:at_commons/at_commons.dart';
+import 'package:at_secondary/src/server/at_secondary_impl.dart';
+import 'package:at_secondary/src/utils/secondary_util.dart';
 import 'package:at_server_spec/at_server_spec.dart';
+import 'package:meta/meta.dart';
 import 'package:uuid/uuid.dart';
 import 'abstract_verb_handler.dart';
 import 'package:at_server_spec/at_verb_spec.dart';
 import 'package:at_persistence_secondary_server/at_persistence_secondary_server.dart';
-import 'package:expire_cache/expire_cache.dart';
 
 class OtpVerbHandler extends AbstractVerbHandler {
   static Otp otpVerb = Otp();
-  static final expireDuration = Duration(seconds: 90);
-  static ExpireCache<String, String> cache =
-      ExpireCache<String, String>(expireDuration: expireDuration);
+
+  @visibleForTesting
+  int otpExpiryInMills = Duration(minutes: 5).inMilliseconds;
 
   OtpVerbHandler(SecondaryKeyStore keyStore) : super(keyStore);
 
   @override
-  bool accept(String command) =>
-      command == 'otp:get' || command.startsWith('otp:validate');
+  bool accept(String command) => command.startsWith('otp');
 
   @override
   Verb getVerb() => otpVerb;
@@ -29,6 +30,10 @@ class OtpVerbHandler extends AbstractVerbHandler {
       HashMap<String, String?> verbParams,
       InboundConnection atConnection) async {
     final operation = verbParams['operation'];
+    if (verbParams[AtConstants.ttl] != null &&
+        verbParams[AtConstants.ttl]!.isNotEmpty) {
+      otpExpiryInMills = int.parse(verbParams[AtConstants.ttl]!);
+    }
     switch (operation) {
       case 'get':
         if (!atConnection.getMetaData().isAuthenticated) {
@@ -40,17 +45,38 @@ class OtpVerbHandler extends AbstractVerbHandler {
         }
         // If OTP generated do not have digits, generate again.
         while (RegExp(r'\d').hasMatch(response.data!) == false);
-        await cache.set(response.data!, response.data!);
+        await keyStore.put(
+            'private:${response.data}${AtSecondaryServerImpl.getInstance().currentAtSign}',
+            AtData()
+              ..data =
+                  '${DateTime.now().toUtc().add(Duration(milliseconds: otpExpiryInMills)).millisecondsSinceEpoch}'
+              ..metaData = (AtMetaData()..ttl = otpExpiryInMills));
         break;
       case 'validate':
-        String? otp = verbParams['otp'];
-        if (otp != null && (await cache.get(otp)) == otp) {
+        var isValid = await isOTPValid(verbParams['otp']);
+        if (isValid) {
           response.data = 'valid';
-        } else {
-          response.data = 'invalid';
+          return;
         }
-        break;
+        response.data = 'invalid';
+    }
+  }
+
+  Future<bool> isOTPValid(String? otp) async {
+    if (otp == null) {
+      return false;
+    }
+    String otpKey =
+        'private:${otp.toLowerCase()}${AtSecondaryServerImpl.getInstance().currentAtSign}';
+    AtData otpAtData;
+    try {
+      otpAtData = await keyStore.get(otpKey);
+    } on KeyNotFoundException {
+      return false;
     }
+    // Remove the key from keystore to prevent reuse of OTP.
+    await keyStore.remove(otpKey);
+    return SecondaryUtil.isActiveKey(otpAtData);
   }
 
   /// This function generates a UUID and converts it into a 6-character alpha-numeric string.

packages/at_secondary_server/pubspec.yaml

@@ -26,7 +26,6 @@ dependencies:
   at_server_spec: 3.0.15
   at_persistence_spec: 2.0.14
   at_persistence_secondary_server: 3.0.57
-  expire_cache: ^2.0.1
   intl: ^0.18.1
   json_annotation: ^4.8.0
   version: 3.0.2
@@ -35,6 +34,13 @@ dependencies:
   yaml: 3.1.2
   logging: 1.2.0
 
+dependency_overrides:
+  at_commons:
+    git:
+      url: https://github.com/atsign-foundation/at_libraries.git
+      path: packages/at_commons
+      ref: expire_otp_changes
+
 dev_dependencies:
   test: ^1.24.4
   coverage: ^1.6.1

packages/at_secondary_server/test/enroll_verb_test.dart

@@ -316,7 +316,7 @@ void main() {
     test('A test to verify enrollment request without otp throws exception',
         () async {
       String enrollmentRequest =
-          'enroll:request:{"appname":"wavi","devicename":"mydevice","namespaces":{"wavi":"r"},"apkampublickey":"dummy_apkam_public_key"}';
+          'enroll:request:{"appName":"wavi","deviceName":"mydevice","namespaces":{"wavi":"r"},"apkamPublicKey":"dummy_apkam_public_key"}';
       HashMap<String, String?> verbParams =
           getVerbParam(VerbSyntax.enroll, enrollmentRequest);
       inboundConnection.getMetaData().isAuthenticated = false;

packages/at_secondary_server/test/otp_verb_test.dart

@@ -3,7 +3,6 @@ import 'dart:collection';
 import 'package:at_commons/at_commons.dart';
 import 'package:at_secondary/src/utils/handler_util.dart';
 import 'package:at_secondary/src/verb/handler/otp_verb_handler.dart';
-import 'package:expire_cache/expire_cache.dart';
 import 'package:test/test.dart';
 
 import 'test_utils.dart';
@@ -13,27 +12,29 @@ void main() {
     setUp(() async {
       await verbTestsSetUp();
     });
-    test('A test to verify OTP generated is 6-character length', () {
+
+    test('A test to verify OTP generated is 6-character length', () async {
       Response response = Response();
       HashMap<String, String?> verbParams =
           getVerbParam(VerbSyntax.otp, 'otp:get');
       inboundConnection.getMetaData().isAuthenticated = true;
       OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
-      otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
       expect(response.data, isNotNull);
       expect(response.data!.length, 6);
       assert(RegExp('\\d').hasMatch(response.data!));
     });
 
-    test('A test to verify same OTP is not returned', () {
+    test('A test to verify same OTP is not returned', () async {
       Set<String> otpSet = {};
       for (int i = 1; i <= 1000; i++) {
         Response response = Response();
         HashMap<String, String?> verbParams =
             getVerbParam(VerbSyntax.otp, 'otp:get');
         inboundConnection.getMetaData().isAuthenticated = true;
         OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
-        otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+        await otpVerbHandler.processVerb(
+            response, verbParams, inboundConnection);
         expect(response.data, isNotNull);
         expect(response.data!.length, 6);
         assert(RegExp('\\d').hasMatch(response.data!));
@@ -42,6 +43,35 @@ void main() {
       }
       expect(otpSet.length, 1000);
     });
+
+    test('A test to verify otp:get with TTL set is active before TTL is met',
+        () async {
+      Response response = Response();
+      inboundConnection.getMetaData().isAuthenticated = true;
+      HashMap<String, String?> verbParams =
+          getVerbParam(VerbSyntax.otp, 'otp:get:ttl:1000');
+      OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      String? otp = response.data;
+      verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      expect(response.data, 'valid');
+    });
+
+    test('A test to verify otp:get with TTL set expires after the TTL is met',
+        () async {
+      Response response = Response();
+      inboundConnection.getMetaData().isAuthenticated = true;
+      HashMap<String, String?> verbParams =
+          getVerbParam(VerbSyntax.otp, 'otp:get:ttl:1');
+      OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      String? otp = response.data;
+      await Future.delayed(Duration(seconds: 1));
+      verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      expect(response.data, 'invalid');
+    }, timeout: Timeout(Duration(minutes: 10)));
     tearDown(() async => await verbTestsTearDown());
   });
 
@@ -90,14 +120,45 @@ void main() {
       HashMap<String, String?> verbParams =
           getVerbParam(VerbSyntax.otp, 'otp:get');
       inboundConnection.getMetaData().isAuthenticated = true;
-      OtpVerbHandler.cache =
-          ExpireCache(expireDuration: Duration(microseconds: 1));
       OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
-      print(OtpVerbHandler.cache.expireDuration.inMicroseconds);
-      await Future.delayed(Duration(microseconds: 2));
+      otpVerbHandler.otpExpiryInMills = 1;
       await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
-      verbParams =
-          getVerbParam(VerbSyntax.otp, 'otp:validate:${response.data}');
+      String? otp = response.data;
+      await Future.delayed(Duration(milliseconds: 2));
+      response = Response();
+      verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      expect(response.data, 'invalid');
+    });
+
+    test(
+        'A test to verify otp:validate return invalid when otp does not exist in keystore',
+        () async {
+      Response response = Response();
+      String otp = 'ABC123';
+      HashMap<String, String?> verbParams =
+          getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
+      OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      expect(response.data, 'invalid');
+    });
+
+    test('A test to verify otp:validate invalidates an OTP after it is used',
+        () async {
+      Response response = Response();
+      HashMap<String, String?> verbParams =
+          getVerbParam(VerbSyntax.otp, 'otp:get');
+      inboundConnection.getMetaData().isAuthenticated = true;
+
+      OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      String? otp = response.data;
+      // attempt #1 should return a valid response
+      verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
+      await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
+      expect(response.data, 'valid');
+      // attempt #2 should return a invalid response
+      verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp');
       await otpVerbHandler.processVerb(response, verbParams, inboundConnection);
       expect(response.data, 'invalid');
     });