Omit standard protocol ports from the default hostname

[compwright]

When running on the standard port for the protocol (80 for http, 443 for https), the default oauth redirect_uri contains the port, which is redundant and causes the authentication request to fail in most cases.

[daffl]

Thank you for the pull request! What do you think about checking for app.get('env') === 'production' instead of the port? I'm not sure if checking for the port is enough since the app often runs behind an NginX proxy on a different port.

[compwright]

@daffl I questioned that, too. There are myriads of configurations. Even in dev environments, it isn't that uncommon to have a dockerized environment behind an nginx running HTTPS (https://github.com/jwilder/nginx-proxy is a good one).

Perhaps a better way to handle this would be to not add the port at all, and assume that it is included in the host config if it is non-standard? WDYT?

[daffl]

The reason I was suggesting based on the environment is that I would like to cover the following two cases without having to configure anything:

1. When following the guide (this usually means it's in development and runs on a non-standard port)
2. In a production environment where the app is running on a standard port either behind a proxy or directly

For everything else it's always possible to customize the port and hostname oauth settings.

[compwright]

Okay, let's game this out:

1. HTTP, env=development, port 80: url = http://localhost ✅
2. HTTP, env=development, port 8000: url = http://localhost:8000 ✅
3. HTTPS, env=development, port 443: url = http://localhost:443 ❌
4. HTTPS, env=development, port 8443: url = http://localhost:8443 ❌
5. HTTP, env=production, port 80: url = https://domain:80 ❌
6. HTTP, env=production, port 8000: url = https://domain:8000 ❌
7. HTTPS, env=production, port 443: url = https://domain ✅
8. HTTPS, env=production, port 8443: url = https://domain:8443 ✅

Do you care about 3-6 being wrong by default, thus needing special configuration? If so, then we need to change the assumption of https in production/http everywhere else.

[daffl]

I think that's ok especially since all it takes is a configuration setting. 5. and 6. should definitely be discouraged and 3. and 4. is not covered by the standard environment that's generated by the Feathers CLI.

[daffl]

Does that make sense? Let me know if you are interested in updating the PR. Definitely agree that this should be improved.

[compwright]

@daffl since you don't care about 3-6, nothing needs to be changed on this PR. It will fulfill the behaviors listed in 1-2 and 7-8 by default. Am I missing something?

[KidkArolis]

The idea is to make sure the following are true (based on this PR and on some previous issues/discussions) :

1. In production the port is never added since most likely people are using a proxy (missing in this PR)
2. Regardless of environment, ports http/80 and https/443 should not be appended (PR does that) (which also helps if node is getting direct traffic but using default ports)

For everything else, user's would configure a custom host, but these should cover most common cases.

I think your PR just needs the following change:

-if ((protocol === 'https' && port !== '443') || (protocol === 'http' && port !== '80'))
+if (env !== 'production' && ((protocol === 'https' && port !== '443') || (protocol === 'http' && port !== '80')))

and ideally tests to go over the different permutations to we avoid future regressions.

[KidkArolis]

On a related, but slightly different topic, wondering how often Feathers checks for env === 'production'. I would suggest perhaps reversing that to env !== 'development' since there could be multiple production like environments (staging in particular is something I often use).

[compwright]

Thanks for that clarification and suggestion. This version should accomplish all our goals and is much easier to read to boot.

[KidkArolis]

This looks good to me, just will need to see what @daffl thinks about the use of 'development' – it depends a bit on what Feathers uses elsewhere for consistency. Though it looks like it's using a bit of both. Wondering, for example, if we should do 'development' || 'test'..

[daffl]

Oh you're right, I didn't pay attention, sorry about that. Good to go then, thank you!