API / BUG - Introduce new request resolver middleware and fix broken forceWWW / forceSSL

[tractorcow]

Fixes #7492

Requires silverstripe/silverstripe-cms#2007 to fix regressions in Director::makeRelative

[sminnee]

I would have expected a DomainCanoncialisationMiddleware instead of this more abstract one?

[sminnee]

And then a DomainCanoncialisationMiddleware.force_www config option or something...

[sminnee]

The thing that weirds me out about this a bit is that a "registered event" and a "middleware" are two different abstractions that do very different things, in addition to the legacy "request filter" abstraction.

[sminnee]

The alternative approach would be to turn the forceWWW logic into its own middleware, and then add that middleware to Director instead of registering an event.

[tractorcow]

@chillu do you have any thoughts on this pr?

[tractorcow]

I think if we were to move to a less-generic middleware, I'd probably just have a separate WWW and SSL redirect middleware, rather than trying to find a middle ground. What do you think about this @sminnee ?

If we were to cater to some yet-unknown "third" redirect type, it would be hard to pre-code this into such a middleware, thus the over-generalisation of the initial solution.

The question is, is "whenRequestIsAvailable" going to be useful in cases OTHER than just ssl / www redirection?

In the future I'd like to implement PHP promises officially. See silverstripe/silverstripe-assets#88. I had in mind that we could shift the ssl / www redirection into the promise api eventually too.

[chillu]

I agree with Sam, this adds unnecessary complexity. In my opinion, the benefits of middleware and the SS4 bootstrap is the relative simplicity and predictability of the logic flow (request in, request out). Modifying this behaviour with procedural logic (like calling Director::force_ssl() from _config.php) should be considered legacy behaviour, rather than encouraged through some kind of middleware events system. If you need influence over the request that early, you can add custom middleware in your index.php.

Laravel has three ways you can do this: App::before() (which looks like inlined middleware), global route filtering, or route specific "before" settings. If you need SSL redirection in your logic flow, you can use Redirect::secure() directly and return a HTTPresponse. This makes a lot of sense to me. https://stackoverflow.com/questions/19967788/laravel-redirect-all-requests-to-https

So, as a short term fix for keeping support for procedural invocation of Director::force_ssl(), I'd follow Sam's lead and suggest to set a config value in a domain canonicalisation middleware singleton. That's assuming injector is already booted at the time we parse/exec _config.php.

[tractorcow]

Ok, let's go with the DomainCanonicalisationMiddleware

[sminnee]

In terms of API for the middleware, I would suggest:

• Force WWW can be set to "always" (maybe a "*" or true) or "if the hostname is in the following list". This lets a sysadmin be explicit about which hostnames are have their WWW DNS configured on the machine in question.
• Force SSL can be set to "always" (maybe a "*" or true) or "if the hostname is in the following list". This lets a sysadmin be explicit about which hostnames are actually configured for SSL on the machine in question.

Beyond the requirements of the issue at hand I would also suggest:

• A canonical hostname to redirect to
• A list of other allowed hostnames that won't redirect to the canonical hostname. This lets you have canonicalisation in most cases while also allowing a few special cases (I remember when working on a pre-production site I needed to access it directly by its IP, so I added that as a 2nd acceptable domain)

In terms of processing order I would do:

• Auto-add www
• Check canonical hostname
• Auto-add HTTPS

Outside the scope of this, but for subsites I'd probably look at a whole separate implementation of this.

Additionally, if one was to build a vanity domain -> redirection mapping system, I would probably include that as a middleware that is processed prior to this one.

[chillu]

I'd prefer to limit the implementation (at least for 4.0) to something that allows use of the Director::forceSSL() API, which means implementing the $patterns argument (for regex path checks), and the $secureDomain argument. If domain whitelists and maps are little implementation overhead, go ahead. But it's creating an API where I'm not sure we need one.

Overall I agree with Dan that this stuff is better handled by the web server layer (.htaccess or nginx rules). forceSSL() made sense at a time when it was considered a special case (e.g. ensuring a form displayed on SSL for a secure submission). Best practice across the web now is to auto-redirect to SSL for every request, which makes those initial non-SSL requests to the homepage slower if you're doing them in a PHP layer. And conditional logic (only some paths, only some domains) is becoming less common over time as SSL adoption grows.

[tractorcow]

Director::forceSSL() would still need to work after middleware has been initialised... e.g. in Controller::init(), so we would need it still, even if we implement it at the middleware level. My intention is to retain the "if request is available" condition to determine which is activated.

[sminnee]

refactoring as discussed

[tractorcow]

Updated and pushed... another rather major refactor, so look at it with a fresh mind from the last solution. :)

[dhensby]

As reported in #7492 this will provide a false positive if the SS_BASE_URL is set to an https scheme even if the request isn't. I recommend checking against the request only.

[tractorcow]

It's nothing to do with SS_BASE_URL; Director::is_https() prefers the Request object before checking SS_BASE_URL.

When this line is called in CanonicalURLMiddleware, the $request will always be populated (since this middleware only activates when a request is available), so the SS_BASE_URL will never be checked by this code.

[tractorcow]

I think in that case, it shouldn't be functionally incorrect to do as you suggest, and just inline the small part of Director::is_https(). I think I see your point. :)

[tractorcow]

Ok, this identified a painful flaw in Director::makeRelative(). For sites with multiple domains this method wouldn't work, meaning that Director::mockRequest() was failing in tests.

After a bit of work I've fixed and tested this, however. Now we can directly use the request instead of needing Director for getting request components.

[dhensby]

should we check for www.?

[tractorcow]

Ah true, will update.

[tractorcow]

All updated. :)

[tractorcow]

Fixed up some regressions in Director::makeRelative(). I've added a note that user code should check is_site_url() BEFORE calling makeRelative(), if using un-trusted urls.

Added silverstripe/silverstripe-cms#2007

Suggestion implemented

[flamerohr]

I've reviewed the feedback and suggestions - they looks to be implemented.

The changes look good and I'm quite excited to see this included.

I'll leave merging for tomorrow morning (2 Nov - 10am) In case @sminnee or @dhensby would like to review the new changes again.