Sturdier approveDomains logic

[martinheidegger]

This PR is making approveDomains sturdier for following cases:

• Former logic didn't work if config.hostname was a subdomain (or a sub-subdomain). example: service.mydomain.com
• Former logic used a <archive>.<user>.<hostname* in per-archive mode, instead of the correct <archive>-<user>.<hostname>.
• Former logic accepted all subdomains in per-user mode, no matter if the user existed or not. (userRecord can be null)
• Former logic was written in return-first-on-success code logic order. This is written in return-first-on-error making it easier to read.

I would love to get some guidance/mentoring on where to write proper tests for this. Also: it seems like there should be some central logic that maps domains to domain-render-object. I.e. app.getDomainContext(domain) to return either the "main" domain context or the context for user/archive.

[pfrazee]

Should we throw in this case?

[pfrazee]

Seems like this would just be a misconfiguration. We could probably throw an error as config validation around https://github.com/beakerbrowser/hashbase/blob/master/lib/config.js#L30

[martinheidegger]

It seems like the default configuration of most tests omits the hostname: https://travis-ci.org/beakerbrowser/hashbase/builds/327215556#L1654

[pfrazee]

This is looking good, I appreciate the update.

I would love to get some guidance/mentoring on where to write proper tests for this.

The best way to debug the letsencrypt config is to set debug to true in the config. (https://github.com/beakerbrowser/hashbase#lets-encrypt) You'll get console output telling you about the success or failure, without doing any live changes.

99% sure you have to setup a real hosting environment to test, sadly.

Also: it seems like there should be some central logic that maps domains to domain-render-object. I.e. app.getDomainContext(domain) to return either the "main" domain context or the context for user/archive.

Could you elaborate on this? I'm not sure what you're referring to with contexts.

[martinheidegger]

99% sure you have to setup a real hosting environment to test, sadly.

I would usually encapsulate the logic in its own function app.isDomain() and then run this against the settings. This way the letsencrypt logic would be abstracted away.

Could you elaborate on this? I'm not sure what you're referring to with contexts.

Right now the distinction between main and user is done in the index. The suggestion would be to change this code using something like:

app.use(app.getRouter(config.sites))

and then use the same (or part of the getRouter implementation) to also check letsencrypt. So if you change the domains it will automatically adjust the letsencrypt check.

that being said I just noticed that letsencrypt will soon offer wildcard certificates Maybe that would make things quicker and better?

[pfrazee]

It seems like the default configuration of most tests omits the hostname:

Ok in that case, we should disable letsencrypt in the config validation if letsencrypt is configured to 'on' and no hostname is set. It should emit a warning.

that being said I just noticed that letsencrypt will soon offer wildcard certificates Maybe that would make things quicker and better?

That's true. I also dont think we need this logic to be too much more sophisticated than it is. It's going to need to be updated for custom domains anyway.