Do not depend on event subscribers for security: Replace AccessRouteSubscriber with build-in checks

So my understanding of this issue is:

Core is currently not secure by default as you could easily set a priority that overrides the magic subscriber.

I think access checks as subscribers is a good idea, but having the access system be a subscriber itself is not.

The router is tied to the access system as it is a fundamental principle of Drupals system, so I don't see a problem hardcoding the security model - and having someone replace the whole RouterBuilder if they really need a different model.

while still allowing the flexibility of Access Route Subscribers ourselves.

So, it does seems like at least the access portion should be hard-coded here, but why don't we start with a patch for just that?

Also, it seems like we should be injecting the AccessManager into the constructor of the RouteBuilder?

I think we could do with more docs on RouteBuilder::isAdminRoute() just to clarify exactly what the rules are. At one point we wanted paths under admin/* to default to admin, but with the option to override (might have been block preview use case), but not sure if that's still relevant.

Does any of the test coverage removed need refactoring rather than removal?

Otherwise looks like a great change and #10 summarizes the trade-offs very well.

Agree with #11 though - would be easier to review if we split into security and not-security chunks.

Yeah, security is a feature of the routing system, I would not be sure whether admin themes would be one of it. For request formats I would just drop that and keep logic on runtime to fallback to GET, POST.

Awesome!

Circular reference detected for service "router.builder", path:          [error]
"router.builder -> access_manager -> router.route_provider ->
router.builder".].

Let's use usual setter injection in that case.

reroll

ah

here we go

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -101,14 +107,17 @@ class RouteBuilder implements RouteBuilderInterface {
+  public function __construct(MatcherDumperInterface $dumper, LockBackendInterface $lock, EventDispatcherInterface $dispatcher, ModuleHandlerInterface $module_handler, ControllerResolverInterface $controller_resolver, RouteBuilderIndicatorInterface $route_build_indicator = NULL, CheckProviderInterface $check_provider) {

we need to re-order here, no sense having optional argument before non-optional

Other than that looks rtbc

assuming green - thanks

+1

+1

RTBC + 1

Fixing tags and title.

Tagging a parent issue and re-titling related to the new security initiative

Have been following along with this issue as well as the two sub-issues that sorted out the dependency chain. An implicit dependency that depends on subscriber priority and is always called is both riskier and more obfuscated than just having the explicit dependency here. Committed/pushed to 8.0.x, thanks!