Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic-auth flag to ngrok #4719

Merged
merged 3 commits into from
Apr 6, 2023
Merged

Add basic-auth flag to ngrok #4719

merged 3 commits into from
Apr 6, 2023

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Mar 6, 2023

The Issue

The documentation includes an example for ngrok:

# Share the current project using ngrok’s basic-auth argument
ddev share --basic-auth username:pass1234

The same example is provided by the ddev help command:

$ ddev help share
Use "ddev share" or add on extra ngrok commands, like "ddev share --basic-auth username:pass1234". Although a few ngrok commands are supported directly, any ngrok flag can be added in the ngrok_args section of .ddev/config.yaml. Requires a free or paid account on ngrok.com; use the "ngrok authtoken" command to set up ngrok.

Usage:
  ddev share [project] [flags]

Examples:
ddev share
ddev share --basic-auth username:pass1234
ddev share myproject

Flags:
  -h, --help               help for share
      --subdomain string   ngrok --subdomain argument, as in "ngrok --subdomain my-subdomain:, requires paid ngrok.com account"

Global Flags:
  -j, --json-output   If true, user-oriented output will be in JSON format.

But when you run it:

$ ddev share --basic-auth username:pass1234 
Error: unknown flag: --basic-auth
Usage:
  ddev share [project] [flags]
...

How This PR Solves The Issue

Adds --basic-auth flag to the ddev share command.

Manual Testing Instructions

ddev start
ddev share --basic-auth username:pass1234

Related Issue Link(s)

The same functionality can be achieved by running:

ddev config --ngrok-args '--basic-auth username:pass1234'
ddev share

@github-actions
Copy link

github-actions bot commented Mar 6, 2023

@rfay
Copy link
Member

rfay commented Mar 6, 2023

The docs failure is a result of move from buildkite.com/drud to buildkite.com/ddev. Not to worry.

Fix is in

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like the fundamental problem is that #3875 didn't make any sense. It dealt with the DDEV args instead of dealing with the ngrok args.

It might make sense to revert #3875 first, although that's what made you think this should work I think right?

@tyler36 could you take another look at this?

@stasadev
Copy link
Member Author

I personally prefer --ngrok-args, so you don't depend on ngrok changes.

If no one has noticed in almost a year that --basic-auth doesn't work, let's just remove it.

@rfay
Copy link
Member

rfay commented Mar 18, 2023

I agree, let's let @tyler36 weigh in and then we'll go forward. I'll need you to lead and test because I turned off the paid subscription after they hiked it so very much. I definitely think we should be exploring use of cloudflare or other possibilities too.

@stasadev
Copy link
Member Author

stasadev commented Mar 18, 2023

Alright, in the meantime I prepared #4766. I don't have a paid ngrok account myself, but I assume it works, because it gives me a legit error:

$ ddev share --subdomain asd
...
ERROR:  Custom subdomains are a feature on ngrok's paid plans.
ERROR:  Failed to bind the custom subdomain 'asd' for the account 'stasadev'.
ERROR:  This account is on the 'Free' plan.
ERROR:  
ERROR:  Upgrade to a paid plan at: https://dashboard.ngrok.com/billing/subscription
ERROR:  
ERROR:  ERR_NGROK_313
ERROR: 

@tyler36
Copy link
Collaborator

tyler36 commented Mar 27, 2023

Works as intended.

Test

Current

  1. Confirm current version.

    $ ddev -v
    ddev version v1.21.6
  2. Start DDEV.

    $ ddev start
    ...
    Project can be reached at https://d10-base-demo.ddev.site https://127.0.0.1:54802
  3. Attempt share.

    $ ddev share --basic-auth username:pass1234
    Error: unknown flag: --basic-auth
    ...
  4. Stop project.

This PR

  1. Confirm current version.

    $ ddev -v
    ddev version v1.21.5-2-g9cb0ab0a
  2. Attempt share.

    $ ddev share --basic-auth username:pass1234
    ...
    Account                       user13 (Plan: Free)
    Forwarding                    https://f591-124-37-96-190.ngrok.io -> http://127.0.0.1:54976
  3. Confirmed site is accessible only with username/password provided.

Global config

  1. Confirm current version.

    $ ddev -v
    ddev version v1.21.6
  2. Configure

    ddev config --ngrok-args '--basic-auth username:pass1234'
    ...
    Account                       user13 (Plan: Free)
    Forwarding                    https://3d3b-124-37-96-190.ngrok.io -> http://127.0.0.1:55097
  3. Confirmed site is accessible only with username/password provided.

Works as intended. However, ~/.ddev/config.yaml is typically managed by git which leads to authentication details potentially being committed. Giving "bad actors" insight into username/password combos is not good practice.
I think moving forward with this PR is the right way to proceed.

Alternative

Add a NGROK helper command (ddev ngrok) which passes all arguments through (similar to ddev composer).
This would:

  • Be future proof if they change their API
  • Could be aliased to ddev share for backwards compatible
  • Prepare DDEV for alternative sharing providers as/if required.

@stasadev stasadev requested review from a team as code owners March 27, 2023 10:28
@stasadev
Copy link
Member Author

.ddev/config.yaml is typically managed by git which leads to authentication details potentially being committed.

Yeah, that's a really good argument.

I rebased and added my changes from another PR, as some of the command descriptions were out of date. I've also moved the required ngrok account information to the top because people tend to pay attention to what's written first.

docs/content/users/topics/sharing.md Outdated Show resolved Hide resolved
Co-authored-by: Matt Stein <m@ttste.in>
@stasadev
Copy link
Member Author

@rfay it is ready, the only thing that has changed since @tyler36 review is the output of ddev share --help and the docs with @mattstein help.

If that's okay with you, let's merge 🚀

Alternative PR #4766 with revert should be closed.

@rfay
Copy link
Member

rfay commented Mar 29, 2023

Thanks, will certainly get to it. Sorry I'm slow, backed up on many things and have been gone for several days. I don't want to merge until I manually test it. Thanks so much for this!

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks and works great, thanks for all the work on it!

@rfay rfay requested a review from mattstein April 6, 2023 17:41
@rfay
Copy link
Member

rfay commented Apr 6, 2023

Looks like you already took a pass at this @mattstein but it may have changed since then.

Copy link
Sponsor Collaborator

@mattstein mattstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@rfay rfay merged commit 4af29d1 into ddev:master Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants