Add support for ngrok

[arunsathiya]

This PR adds support for ngrok. Fixes #156

Testing instructions

• Sign up for ngrok.
• Get Auth Token from https://dashboard.ngrok.com/get-started/your-authtoken and an API token from https://dashboard.ngrok.com/api
• Checkout this git branch and run make ngrok/build
• Run op plugin init ngrok
• Since you do not have a ngrok config file at this point in time, you will be able to manually type your auth token and your API key.
• Ensure that the two are stored to a 1Password item.
• Also test environment variable-based import and config file imports.
• • For env var import test, first run export NGROK_AUTHTOKEN=token and export NGROK_API_KEY=token and then run op plugin init ngrok
• • For config file import test, save the below config file at ~/Library/Application Support/ngrok/ngrok.yml if you are on Mac, or at ~/.config/ngrok/ngrok.yml if you are on Linux, and then run op plugin init ngrok
• Make sure that ngrok without any commands or args does not prompt for 1Password auth.
• Make sure that ngrok api tunnels list and ngrok http 80 work (first is API key test and the second is Auth Token test; for the second, you can see your account name on the generated tunnel)

Sample config

version: "2"
authtoken: uSuQ7LUOJLs4xRbIySZ15F4v5KxfTnMknMdFEXAMPLE
api_key: L4STpMP3K8FNaQjBo5EAsXA2SThzq0J7BKD3jUZgtEXAMPLE

Notes

• The CLI supports two environment variables NGROK_AUTHTOKEN and NGROK_API_KEY but ngrok api commands fail even with the second env var set. I have asked about it here: ngrok API doesn't work with environment variable set inconshreveable/ngrok#895
• For now, the shell plugin provisions creds using a temp config file. When the env var issue is resolved on ngrok side, we can provision using environment variables if that's preferred.
• "version" is a mandatory field in the config file. Provisioning fails without it. For some reason, that requirement is not mentioned here: https://ngrok.com/docs/ngrok-agent/ngrok

[arunsathiya]

This PR is now ready for review. ✅

[hculea]

Thank you for this addition, @arunsathiya ! You're on fire with all these new plugins, absolutely love it ❤️ I've left a few comments below.

[arunsathiya]

Thanks for all the comments, @hculea. I have addressed your feedback above, but as a last step, we are waiting for ngrok staff to respond to my Slack question. I'll followup as soon as I hear from them.

[arunsathiya]

Just so it isn't missed, I shared the latest update here.

[AndyTitu]

From my understanding after reading ngrok's documentation, it authenticates with either the auth token or the api key (they are not both required at the same time right?).

It could be the case that they are both required, as I am not too familiar with ngrok documentation. In this case, the current solution is definitely the way to go for now, since the env vars are out of the question because the api key is not covered.

In the case where they are not both required at the same time, I'm understanding that your reason for going for the config file provisioner is to allow users to choose between the 2 different authentication formulas, since the config file supports both while env vars do not.

• However, the structure of the current plugin is in such a way that users cannot really choose between them, but they should rather have both, because these fields are both denoted with Optional: false.

• If you want users to be able to choose between both of them you would have to specify Optional:true for both. Then this would also not be right because at least 1 is required.

To avoid these complications I would suggest just going ahead with the env var approach, and only support the auth token. In this way we also avoid storing the non-secret version in 1Password.

What do you think @arunsathiya ?

[arunsathiya]

Thanks for your detailed review, @AndyTitu! Regarding the Authtoken vs API Key confusion, all commands require the Authtoken with the exception of ngrok api and its subcommands. It's not clearly mention on the CLI docs, but this gives me that impression:

https://ngrok.com/docs/ngrok-agent/ngrok#ngrok-api

I have also tested running ngrok api subcommands with just the Authtoken, and they fail. It's only after provisioning a API key as well, the commands work.

Having said that, the ngrok team fixed envvar support for API key as of yesterday. It's just a matter of waiting for the update to be available to the general public, which I don't have a timeline on:

At this point, we could wait for that updated version to be available and then provision credentials as envvars, or if we want to get this out right now, we can:

• retain the current version of supporting both Authtoken and API key,
• have both as required fields, because without the API key stored in 1Password, all ngrok api subcommands will fail,
• and, switch to envvar-based approach when the new ngrok client update is out.

In this way we also avoid storing the non-secret version in 1Password.

Could you elaborate on this part, please? Did you mean to say that API Token is not considered a secret credential?

[AndyTitu]

Thanks for the additional information @arunsathiya ! In my opinion, the best way ahead given our current situation would be to go ahead with the current state of this PR, with only a small change added. To clarify, this is what I'm looking for (mostly we should already be there):

• use the config file provisioner where both auth token and api key are provisioned together, alongside the version, as we are doing now
• keep the version as it is now (not hardcoded, and imported from a config file/manually added into 1Password) - even though a user mistake would cause the plugin to crash, my back-of-a-napkin estimate is that in 90% of the cases users will know what the version should be
• make the api token optional. Most users will have an auth token, and most of the cases could be handled by only provisioning an auth token. For these users, the API key is therefore optional. For users who do use an api, they will need to take the extra step of creating and adding the API key into 1Password.

This will avoid having to wait on 3rd party releases. When their release is out we could file a change for moving over to env vars to get rid of the need to specify the version.

[SimonBarendse]

I have opened #182 for us to look into supporting different credential types for different commands. In the meantime, we can proceed with either having support for just the auth token and no support for the ngrok api command yet, or we can proceed with accepting the API key as optional field. I'm okay with either approach.

[arunsathiya]

Thanks @AndyTitu and @SimonBarendse, I am on board with this approach as well. This PR is ready for one final look. 🤞🏼 But, the test is failing with undefined credname.Credentials. I see it okay here though:

https://github.com/1Password/shell-plugins/blob/b957d2877a71a3d299e3e09271cf75d328e6f576/sdk/schema/credname/names.go

Is there a way to re-run tests without having to push a new commit?

[SimonBarendse]

We need to re-add the credential type, we removed it assuming it wasn't used here: #161 (comment)

Could you pull in main to your branch, and then make sure it's added back in?

[SimonBarendse]

@arunsathiya I've re-opened an old discussion here: #165 (comment)

[arunsathiya]

We need to re-add the credential type, we removed it assuming it wasn't used here: #161 (comment)
Could you pull in main to your branch, and then make sure it's added back in?

Thanks for the pointer, Simon. Rebased and tests now pass. ✅

Agree on hardcoding the version number; precisely why I had set that up as such in the first place.

[SimonBarendse]

Love your work on this @arunsathiya! Thank you for figuring out the differences between Auth Token and API Key and a way to move forward with this. The plugin is in a great spot! Looking forward to release this one soon 🚀

Once we've tackled #182, we can re-visit this plugin to polish it further with first-class support for the ngrok api commands.

Thanks again! 🙌

[arunsathiya]

@SimonBarendse @hculea A quick note that I had previously incorrectly understood that NotForExactArgs() check meant both the command and its subcommands, which resulted in this breakage:

• 1password wrapper breaks some of the ngrok cli commands that do not require authtoken #185

I was testing only ngrok update which had given me that impression.

I now have a fix for it here:

• fix(ngrok): Do not require 1Password authentication for certain (sub)commands #187

Would we be able to release a patch build? My apologies for the hassle here.