atsign-foundation / noports

Connect to any device with no external listening ports open
https://noports.com
BSD 3-Clause "New" or "Revised" License
267 stars 15 forks source link

fix: sshnp: various arg parsing issues #1047

Closed gkc closed 4 months ago

gkc commented 5 months ago

- What I did Fixes #1046

- How I did it

- How to verify it Tests pass

gkc commented 5 months ago

If we're not changing --device to mandatory then LGTM.

This PR does change it to mandatory see sshnp_arg.dart file diff

gkc commented 5 months ago

We cannot make device mandatory, it will break the ability to use list-devices, it instead should be validated similar to how --from and --to are.

  // models/sshnp_params.dart:274

  factory SshnpParams.fromPartial(SshnpPartialParams partial) {
    // Always need the clientAtSign
    partial.clientAtSign ??
        (throw ArgumentError('from (clientAtSign) is mandatory'));

    if (!(partial.listDevices ?? DefaultSshnpArgs.listDevices)) {
      // if list-devices is not set, then ensure sshnpdAtSign and srvdAtSign are set
      partial.sshnpdAtSign ??
          (throw ArgumentError(
              'Option to is mandatory, unless list-devices is passed.'));
      partial.srvdAtSign ??
          (throw ArgumentError(
              'srvdAtSign is mandatory, unless list-devices is passed.'));
    }

    return SshnpParams(
      profileName: partial.profileName,
      clientAtSign: partial.clientAtSign!,
      sshnpdAtSign: partial.sshnpdAtSign ?? "",
      srvdAtSign: partial.srvdAtSign ?? "",
      device: partial.device ?? DefaultSshnpArgs.device,
      ...
);

@XavierChanth ack

gkc commented 5 months ago

@XavierChanth as per your comment on #1046 I've reverted the change to make --device mandatory and instead am outputting a warning from the sshnp CLI if device is "default"

XavierChanth commented 5 months ago

What I should have probably done from the start is make list-devices a subcommand instead of a flag. We can change it in v6, which will then allow us to enforce the arguments (I think... args is not the most flexible package)

gkc commented 5 months ago

@XavierChanth There's a flurry of PRs right now so I'll need to rerun the e2e_all check before I merge, but those tests are working fine