Repository: atsign-foundation/noports (dart)

Issue:
npt atSign handling
### Describe the bug

NptParams accepts atSigns without the `@` prefix which leads to problems later (in the relay server auth check and possibly elsewhere)

### Steps to reproduce

Run npt with one or more atSign args which are not prefixed with `@`

### Expected behavior

- NptParams object should not allow malformed atSign args

Pull request description:
**- What I did**
- Fixes #1331
- ensure npt rejects malformed atSign args

**- How I did it**
- added atSign parameter checking (via AtUtils.fixAtSign method) to bin/npt.dart and to the NptParams constructor
- Note: did not modify SshnpParams nor the shared superclass ClientParamsBase because of the complication of `--list-devices` which should allow blank daemon and relay atSigns

**- How to verify it**
Tests pass

Patch:
diff --git a/packages/dart/noports_core/lib/src/admin/impl.dart b/packages/dart/noports_core/lib/src/admin/impl.dart
index bb89bc18c..54d80bab8 100644
--- a/packages/dart/noports_core/lib/src/admin/impl.dart
+++ b/packages/dart/noports_core/lib/src/admin/impl.dart
@@ -6,7 +6,8 @@ import 'package:at_utils/at_logger.dart';
 import 'package:noports_core/admin.dart';
 import 'package:noports_core/sshnp_foundation.dart';
 
-class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings {
+class PolicyServiceWithAtClient extends PolicyServiceInMem
+    with AtClientBindings {
   @override
   final logger = AtSignLogger('PolicyServiceWithAtClient');
   @override
@@ -20,12 +21,15 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
   Future<void> init() async {
     await super.init();
 
-    atClient.notificationService.subscribe(
+    atClient.notificationService
+        .subscribe(
       regex: r'.*\.groups\.policy\.sshnp',
       shouldDecrypt: true,
-    ).listen((AtNotification n) {
+    )
+        .listen((AtNotification n) {
       String groupId = n.key.split(':')[1].split('.').first;
-      logger.info('Received ${n.operation} notification for group ${n.key} - ID is $groupId');
+      logger.info(
+          'Received ${n.operation} notification for group ${n.key} - ID is $groupId');
       if (n.operation == 'delete') {
         groups.remove(groupId);
       } else {
@@ -38,7 +42,8 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
       regex: r'.*\.logs\.policy\.sshnp',
       shouldDecrypt: true,
     ).listen((AtNotification n) {
-      logger.shout('Received policy log notification from ${jsonDecode(n.value!)['daemon']}');
+      logger.shout(
+          'Received policy log notification from ${jsonDecode(n.value!)['daemon']}');
       // TODO Make a PolicyLogEvent and use PolicyLogEvent.fromJson()
       onPolicyLogEvent(n.value!);
     });
@@ -60,8 +65,7 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
     logger.shout('Loading groups via AtClient');
     // Fetch all the groups
     List<AtKey> groupKeys = await atClient.getAtKeys(
-        regex: '.*.groups.policy.sshnp',
-        sharedBy: atClient.getCurrentAtSign());
+        regex: '.*.groups.policy.sshnp', sharedBy: atClient.getCurrentAtSign());
     for (final AtKey groupKey in groupKeys) {
       logger.shout('Loading group from atKey: $groupKey');
       final v = await atClient.get(
@@ -82,6 +86,7 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
   AtKey _groupAtKey(String id) {
     return AtKey.fromString(_groupKey(id));
   }
+
   @override
   Future<UserGroup> createUserGroup(UserGroup group) async {
     if (group.id != null) {
@@ -96,7 +101,8 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
     );
     await atClient.notificationService.notify(
       NotificationParams.forUpdate(
-        AtKey.fromString('${atClient.getCurrentAtSign()}:${_groupAtKey(group.id!)}'),
+        AtKey.fromString(
+            '${atClient.getCurrentAtSign()}:${_groupAtKey(group.id!)}'),
         value: jsonEncode(group),
       ),
     );
@@ -116,7 +122,8 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
     );
     await atClient.notificationService.notify(
       NotificationParams.forUpdate(
-        AtKey.fromString('${atClient.getCurrentAtSign()}:${_groupAtKey(group.id!)}'),
+        AtKey.fromString(
+            '${atClient.getCurrentAtSign()}:${_groupAtKey(group.id!)}'),
         value: jsonEncode(group),
       ),
     );
@@ -125,7 +132,8 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
 
   @override
   Future<bool> deleteUserGroup(String id) async {
-    await atClient.delete(_groupAtKey(id),
+    await atClient.delete(
+      _groupAtKey(id),
       deleteRequestOptions: DeleteRequestOptions()..useRemoteAtServer = true,
     );
     await atClient.notificationService.notify(
@@ -139,9 +147,7 @@ class PolicyServiceWithAtClient extends PolicyServiceInMem with AtClientBindings
 
 class PolicyServiceInMem implements PolicyService {
   @override
-  Future<void> init() async {
-
-  }
+  Future<void> init() async {}
 
   @override
   final Map<String, UserGroup> groups = {};
@@ -180,7 +186,8 @@ class PolicyServiceInMem implements PolicyService {
       'type': 'PolicyCheck',
       'daemon': pe['daemon'],
       'deviceName': pe['payload']['request']['payload']['daemonDeviceName'],
-      'deviceGroupName': pe['payload']['request']['payload']['daemonDeviceGroupName'],
+      'deviceGroupName': pe['payload']['request']['payload']
+          ['daemonDeviceGroupName'],
       'user': pe['payload']['request']['payload']['clientAtsign'],
       'authorized': pe['payload']['response']['payload']['authorized'],
       'message': pe['payload']['response']['payload']['message'],
diff --git a/packages/dart/noports_core/lib/src/admin/interface.dart b/packages/dart/noports_core/lib/src/admin/interface.dart
index de0c5e42e..fac935c98 100644
--- a/packages/dart/noports_core/lib/src/admin/interface.dart
+++ b/packages/dart/noports_core/lib/src/admin/interface.dart
@@ -53,4 +53,3 @@ abstract interface class PolicyService {
     return PolicyServiceInMem();
   }
 }
-
diff --git a/packages/dart/noports_core/lib/src/admin/models.dart b/packages/dart/noports_core/lib/src/admin/models.dart
index c603f3b8e..a46649d86 100644
--- a/packages/dart/noports_core/lib/src/admin/models.dart
+++ b/packages/dart/noports_core/lib/src/admin/models.dart
@@ -63,7 +63,8 @@ class UserGroup {
 
   final List<String> userAtSigns;
 
-  factory UserGroup.empty({String? id, required String name, required String description}) {
+  factory UserGroup.empty(
+      {String? id, required String name, required String description}) {
     return UserGroup(
         id: id,
         name: name,
diff --git a/packages/dart/noports_core/lib/src/sshnp/models/sshnp_params.dart b/packages/dart/noports_core/lib/src/sshnp/models/sshnp_params.dart
index 6439076a8..e6c2eb950 100644
--- a/packages/dart/noports_core/lib/src/sshnp/models/sshnp_params.dart
+++ b/packages/dart/noports_core/lib/src/sshnp/models/sshnp_params.dart
@@ -1,6 +1,7 @@
 import 'dart:convert';
 
 import 'package:at_chops/at_chops.dart';
+import 'package:at_commons/at_commons.dart';
 import 'package:at_utils/at_utils.dart';
 import 'package:noports_core/src/sshnp/models/config_file_repository.dart';
 import 'package:noports_core/src/sshnp/models/sshnp_arg.dart';
@@ -153,7 +154,15 @@ class NptParams extends ClientParamsBase
     required this.inline,
     super.daemonPingTimeout,
     required this.timeout,
-  });
+  }) {
+    try {
+      AtUtils.fixAtSign(clientAtSign);
+      AtUtils.fixAtSign(sshnpdAtSign);
+      AtUtils.fixAtSign(srvdAtSign);
+    } on InvalidAtSignException catch (e) {
+      throw ArgumentError(e.message);
+    }
+  }
 
   /// not relevant for Npt
   @override
diff --git a/packages/dart/noports_core/lib/src/sshnpd/sshnpd_impl.dart b/packages/dart/noports_core/lib/src/sshnpd/sshnpd_impl.dart
index b98a08ea9..ed12bd170 100644
--- a/packages/dart/noports_core/lib/src/sshnpd/sshnpd_impl.dart
+++ b/packages/dart/noports_core/lib/src/sshnpd/sshnpd_impl.dart
@@ -1309,6 +1309,7 @@ class SshnpdImpl implements Sshnpd {
           ..namespaceAware = true);
 
       logger.info('Sending heartbeat to policy service $policyManagerAtsign');
+
       /// send it
       await _notify(
         atKey: atKey,
diff --git a/packages/dart/sshnoports/bin/npp_atserver.dart b/packages/dart/sshnoports/bin/npp_atserver.dart
index b0c5920c3..13028ae71 100644
--- a/packages/dart/sshnoports/bin/npp_atserver.dart
+++ b/packages/dart/sshnoports/bin/npp_atserver.dart
@@ -24,7 +24,6 @@ void main(List<String> args) async {
   }
   AtSignLogger.defaultLoggingHandler = AtSignLogger.stdErrLoggingHandler;
 
-
   logger = AtSignLogger(' npp ');
   AtClient atClient = await createAtClientCli(
     atsign: p.authorizerAtsign,
@@ -56,10 +55,12 @@ void main(List<String> args) async {
 
   Set<String> notifiedDaemonAtSigns = {};
 
-  atClient.notificationService.subscribe(
+  atClient.notificationService
+      .subscribe(
     regex: r'.*\.devices\.policy\.sshnp',
     shouldDecrypt: true,
-  ).listen((AtNotification n) {
+  )
+      .listen((AtNotification n) {
     notifiedDaemonAtSigns.add(n.from);
     sshnpa.daemonAtsigns.clear();
     sshnpa.daemonAtsigns.addAll(handler.api.daemonAtSigns);
@@ -67,10 +68,12 @@ void main(List<String> args) async {
     logger.info('daemonAtSigns is now ${sshnpa.daemonAtsigns}');
   });
 
-  atClient.notificationService.subscribe(
+  atClient.notificationService
+      .subscribe(
     regex: r'.*\.groups\.policy\.sshnp',
     shouldDecrypt: true,
-  ).listen((AtNotification n) {
+  )
+      .listen((AtNotification n) {
     sshnpa.daemonAtsigns.clear();
     sshnpa.daemonAtsigns.addAll(handler.api.daemonAtSigns);
     sshnpa.daemonAtsigns.addAll(notifiedDaemonAtSigns);
@@ -97,7 +100,6 @@ class Handler implements NPARequestHandler {
   @override
   Future<NPAAuthCheckResponse> doAuthCheck(
       NPAAuthCheckRequest authCheckRequest) async {
-
     logger.info('Checking policy for request: $authCheckRequest');
     // member of any groups?
     final groups = await api.getGroupsForUser(authCheckRequest.clientAtsign);
@@ -116,7 +118,6 @@ class Handler implements NPARequestHandler {
     // does it contain the authCheckRequest.daemonAtsign?
     for (final group in groups) {
       if (group.daemonAtSigns.contains(authCheckRequest.daemonAtsign)) {
-
         // does it contain a matching deviceName? if so, add the permitOpens
         for (final d in group.devices) {
           if (d.name == authCheckRequest.daemonDeviceName) {
@@ -129,7 +130,6 @@ class Handler implements NPARequestHandler {
             permitOpens.addAll(dg.permitOpens);
           }
         }
-
       }
     }
 
diff --git a/packages/dart/sshnoports/bin/npt.dart b/packages/dart/sshnoports/bin/npt.dart
index 091646f3f..c0f0792bc 100644
--- a/packages/dart/sshnoports/bin/npt.dart
+++ b/packages/dart/sshnoports/bin/npt.dart
@@ -6,8 +6,8 @@ import 'dart:io';
 import 'package:args/args.dart';
 
 // atPlatform packages
-import 'package:at_utils/at_logger.dart';
 import 'package:at_cli_commons/at_cli_commons.dart' as cli;
+import 'package:at_utils/at_utils.dart';
 import 'package:duration/duration.dart';
 import 'package:noports_core/npt.dart';
 import 'package:noports_core/sshnp_foundation.dart';
@@ -212,8 +212,17 @@ void main(List<String> args) async {
       }
 
       verbose = parsedArgs['verbose'];
+      String clientAtSign = parsedArgs['from'];
       String daemonAtSign = parsedArgs['to'];
       String srvdAtSign = parsedArgs['srvd'];
+      try {
+        clientAtSign = AtUtils.fixAtSign(clientAtSign);
+        daemonAtSign = AtUtils.fixAtSign(daemonAtSign);
+        srvdAtSign = AtUtils.fixAtSign(srvdAtSign);
+      } catch (e) {
+        throw ArgumentError(e.toString());
+      }
+
       int remotePort = int.parse(parsedArgs['remote-port']);
       String remoteHost = parsedArgs['remote-host'];
       String device = parsedArgs['device'];
@@ -242,8 +251,6 @@ void main(List<String> args) async {
 
       // Windows will not let us delete files in use so
       // We will point storage to temp directory and let OS clean up
-      var clientAtSign = parsedArgs['from'];
-
       late String uniqueID;
       if (perSessionStorage) {
         uniqueID = DateTime.now().millisecondsSinceEpoch.toString();


Test patch:
diff --git a/packages/dart/noports_core/test/sshnp/models/sshnp_params_test.dart b/packages/dart/noports_core/test/sshnp/models/sshnp_params_test.dart
index 68afb1e1b..9c94d6504 100644
--- a/packages/dart/noports_core/test/sshnp/models/sshnp_params_test.dart
+++ b/packages/dart/noports_core/test/sshnp/models/sshnp_params_test.dart
@@ -3,6 +3,50 @@ import 'package:noports_core/utils.dart';
 import 'package:test/test.dart';
 
 void main() {
+  group('NptParams', () {
+    test('malformed client atSign missing prefix', () {
+      expect(
+          () => NptParams(
+                clientAtSign: '',
+                sshnpdAtSign: '@daemon',
+                srvdAtSign: '@relay',
+                device: 'foo',
+                inline: false,
+                remoteHost: 'localhost',
+                remotePort: 3389,
+                timeout: DefaultArgs.srvTimeout,
+              ),
+          throwsA(TypeMatcher<ArgumentError>()));
+    });
+    test('malformed daemon atSign missing prefix', () {
+      expect(
+          () => NptParams(
+                clientAtSign: '@client',
+                sshnpdAtSign: '',
+                srvdAtSign: '@relay',
+                device: 'foo',
+                inline: false,
+                remoteHost: 'localhost',
+                remotePort: 3389,
+                timeout: DefaultArgs.srvTimeout,
+              ),
+          throwsA(TypeMatcher<ArgumentError>()));
+    });
+    test('malformed relay atSign missing prefix', () {
+      expect(
+          () => NptParams(
+                clientAtSign: '@client',
+                sshnpdAtSign: '@daemon',
+                srvdAtSign: '',
+                device: 'foo',
+                inline: false,
+                remoteHost: 'localhost',
+                remotePort: 3389,
+                timeout: DefaultArgs.srvTimeout,
+              ),
+          throwsA(TypeMatcher<ArgumentError>()));
+    });
+  });
   group('SshnpParams', () {
     test('public API test', () {
       final params =