Problem/Motivation

Follow-up from #1606794: Implement new routing system.

The new routing system doesn't have access control yet. It needs it. :-)

There's also a long-standing list of limitations and annoyances in the current access system. We may as well try to address at least some of those along the way.

Proposed resolution

Symfony Full Stack uses a separate Security component for controlling route access that is far too complex for us to integrate in Drupal 8. We're not going there right now.

After talking with other Symfonians and Drupalers, it appears the correct approach, at a high level, is to "add metadata to Routes" and then "add post-matching request listeners that can reject access if appropriate".

There's two general ways that could be done:

1) Let every ACL module tack on whatever it wants to route definitions and implement its own request listener. So for example, we'd add a $request->setDefault('permission', 'access site configuration') value, and then add a listener that checks for a permission key on $request->attributes (where the matcher would stick it) and if it's set, runs user_access() on it (or whatever user_access() turns into). Other access checking systems could add their own properties and listeners.

Benefit: Really simple to implement, quite extensible, no significant architecture needs to be added.

Drawback: Potentially slower if there are lots of access listeners firing. Results in a "default allow" model. (That could be good or bad.) Possibility for namespace collision.

2) Implement a single listener that checks a series of conditions based on the values inside a sub-element of defaults. That may include permission checks but also anything else. What we'd add to that array then is not "the permission to use" but "the component to use and call with supplied information". This is conceptually closer to what we have now with "access callback" and "access arguments" but more robust and multi-plexed. We would likely want to make this either use the "Boolean plugin" that EclipseGc has been talking about -- in which case you need to pass all checks -- or model it on hook_node_access() and allow access plugins to return Yes, No, or I Don't Care -- and then you need "at least one Yes, and no Nos" in order to pass. Potentially we'd reference to service objects in the DIC so that we can keep the access checks testable.

Benefits: More robust implementation. *May* be faster. May be easier to extend depending on the details.

Drawbacks: More work and complexity to implement. I don't know that Boolean plugins are ready.

3) Nobody expects the Spanish inquisition! Even if we do option 2, there's nothing stopping option 1 from happening. They are not mutually exclusive except in terms of confusion.

Remaining tasks

Commit the routing issue above, figure out which of the above we want, and then code it up.

User interface changes

For now none, but it will have an impact on the SCOTCH interface work. Although they've probably thought about it already.

API changes

As above.

Related issues

#1807776: Support both simple and editorial workflows for translating entities postponed on this issue.

CommentFileSizeAuthor
#141 routing-perm-1793520-141.patch33.48 KBklausi
#141 routing-perm-1793520-141-interdiff.txt1.22 KBklausi
#138 routing-perm-1793520-138.patch33.48 KBklausi
#138 routing-perm-1793520-138-interdiff.txt3.2 KBklausi
#137 routing-perm-1793520-137.patch30.29 KBklausi
#137 routing-perm-1793520-137-interdiff.txt2.74 KBklausi
#134 drupal-routing_permissions-1793520-134.patch30.75 KBdisasm
#134 interdiff.txt1.18 KBdisasm
#131 routing-perm-1793520-131.patch30.07 KBklausi
#131 routing-perm-1793520-131-interdiff.txt3.88 KBklausi
#129 routing-perm-1793520-129.patch29.68 KBklausi
#129 routing-perm-1793520-129-interdiff.txt14.99 KBklausi
#127 drupal-routing_permissions-1793520-127-cron_bundle.patch31.05 KBdisasm
#127 drupal-routing_permissions-1793520-127-review-do-not-test.patch29.83 KBdisasm
#127 interdiff.txt1.41 KBdisasm
#126 drupal-routing_permissions-1793520-126-cron_bundle.patch30.37 KBdisasm
#126 drupal-routing_permissions-1793520-126-review-do-not-test.patch29.15 KBdisasm
#126 interdiff.txt8.12 KBdisasm
#124 drupal-routing_permissions-1793520-124.patch24.63 KBdisasm
#124 interdiff.txt589 bytesdisasm
#110 drupal-routing_permissions-1793520-110.patch24.56 KBdisasm
#107 drupal-routing_permissions-1793520-107.patch24.56 KBdisasm
#103 drupal-routing_permissions-1793520-102.patch38.11 KBdisasm
#103 drupal-routing_permissions-1793520-102-dx-do-not-test.patch24.42 KBdisasm
#99 1793520-99-routing-perm.patch33.57 KBklausi
#99 1793520-99-short-do-not-test.patch23.49 KBklausi
#97 1793520-97-interdiff.txt8.78 KBklausi
#92 drupal-routing_permissions-1793520-92.patch33.82 KBdisasm
#92 drupal-routing_permissions-1793520-92-do-not-test.patch23.96 KBdisasm
#92 interdiff.txt1.97 KBdisasm
#91 1793520-routing-permissions.patch33.15 KBCrell
#91 1793520-routing-permissions-do-not-test.patch31.71 KBCrell
#89 drupal-routing_permissions-1793520-89.patch33.16 KBdisasm
#89 drupal-routing_permissions-1793520-89-dx-do-not-test.patch22.89 KBdisasm
#85 drupal-routing_permissions-1793520-85.patch34.31 KBdisasm
#85 drupal-routing_permissions-1793520-85-dx-do-not-test.patch25.94 KBdisasm
#82 drupal-routing_permissions-1793520-82.patch156.56 KBdisasm
#82 drupal-routing_permissions-1793520-82-dx-do-not-test.patch25.94 KBdisasm
#81 drupal-routing_permissions-1793520-81.patch27.95 KBdisasm
#81 interdiff.txt4.11 KBdisasm
#78 drupal-routing_permissions-1793520-78.patch24.35 KBdisasm
#78 interdiff.txt1.52 KBdisasm
#77 drupal-routing_permissions-1793520-77.patch24.39 KBdisasm
#77 interdiff.txt1.28 KBdisasm
#69 interdiff.txt660 bytestnightingale
#69 1793520.access_control.69.patch24.4 KBtnightingale
#68 1793520.access_control.68.patch24.4 KBtnightingale
#66 1793520-65-router-access.patch24.31 KBklausi
#66 1793520-65-interdiff.txt758 bytesklausi
#55 1793520.access_control.55.patch23.78 KBtnightingale
#55 interdiff.txt9.12 KBtnightingale
#49 1793520.access_control.49.patch22.26 KBtnightingale
#45 1793520.access_control.45.patch18.46 KBtnightingale
#5 1793520-routing-permissions-complete.patch119.45 KBCrell
#3 1793520-routing-permissions.patch7.21 KBCrell
#3 1793520-routing-permissions-complete.patch0 bytesCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: base system » routing system

There.

Crell’s picture

I discussed this with some folks at Symfony Live today. We concluded that option 1 should be fine, and is effectively what Symfony full stack does, conceptually. If we find the performance becomes an issue if there are many listeners, we can, in a follow-up, precompile the list of access checkers that a given route will need, store that with the route, and then have a single listener that calls just those. That's for a follow-up if we find that it's actually necessary, though.

So option 1 is the way forward. Volunteers? :-)

Crell’s picture

Status: Active » Needs review
FileSize
0 bytes
7.21 KB

OK, here's a first stab, nice and easy. The first patch is rolled off of the routing branch, so you can see what's added. The second includes the routing branch, for testbot.

Assuming this works, we can add other listeners ("user is not logged in" and such) in subsequent patches. Permissions cover the 90% use case.

plach’s picture

The full patch is empty. Can you post it again, please?

Crell’s picture

Now how did that happen...

The first patch is the one you should bother reviewing anyway.

plach’s picture

Yeah, I reviewed #3/1 and looks pretty good already. It has a couple of minor code style issues but I guess this is not the right time to outline them :)

Didn't RTBC it as I'm pretty new to the routi-ng stuff and might have easily missed something. However the patch implements straightforwardly approach 1 from the OP, which sounds pretty sensible to me.

catch’s picture

Priority: Major » Critical

So the current patch is removing the relationship between router items and access. With access callback (or multiple access callables if we did something like that), then all the information about the routes is included with the router item definition.

It looks from here that if node module wants node/%node to run node_access(), it needs to implement the router item, then separately register a listener, which is going to run on every single request to any router item, see if there's a node Entity in the request, and call node_access() (or whatever that turns into) instead. This is significantly more work to do when defining a router iteum that's not using a straight user_access() and it seems potentially fragile - i.e. if you switch from a permission like access content to something else you're not just changing access callback and arguments any more.

Once there's multiple listeners on multiple paths it also feels like it'll be a lot harder to debug as well - you'd need to figure out all the subscribed listeners, whereas now you can use menu_get_item() and see what's going on.

Not only that, but for the display of menu links, we currently run router access checks on every single menu item that could possibly be dispplayed, on every request, since there's no other way to check if they can be shown or not. So the likely performance overhead of using event subscribers here could be potentially a serious regression and don't want to leave that until way after the fact (i.e. when the last router item conversion lands and someone profiles it). This isn't going to run just once when a request comes in, it's going to run potentially several hundred times for different paths.

So I really prefer option #2 here architecturally at the moment, something closer to what we currently have feels very desirable here.

Bumping this to critical since the actual router patch is supposed to get merged by Dries today, and we can't do any conversions until this is resolved.

Crell’s picture

catch: You couldn't have said something in the week this was open before I tried coding something?

Menu links and menu items are tightly coupled right now in D7. We're ripping them apart in D8, so I don't know to what degree the same performance issues you mention there will apply.

I noted above that I don't expect in practice that we'll have a zillion listeners. MOST page callbacks now use a simple permission check. After talking with the Symfonians, I think switching to a compile step is likely going to be more effective. I am not convince that's something we should do now, though, until it becomes an issue. Don't prematurely optimize.

catch’s picture

@Crell, no I can't do in-depth architectural review of every issue in the queue within one week of people posting on them. Also you only posted up about going for option #2 three working days ago. Did you ask any of the menu maintainers about this approach before coding it up?

I don't see how the decision for whether or not to display a menu link can be decided by anything other than whether the current user has access to view the router item that the menu link points to. If it's decided in some other way, then by definition there will either be links that don't display when they would lead to a 200, or links that display but lead to a 403. The coupling of access for the links to the underlying router item isn't the same as the definition of the links and router items being so tightly coupled as they currently are.

Also if this uses request listeners, then wouldn't it require a full sub-request in order to check menu link access then?

catch’s picture

Also just to point out, I'm not say "you must implement option #2 'cos I'm the Drupal 8 maintainer and I prefer that one", I'm just trying to review this since the main router is about to go in and this is presumably the next bit, but I really don't see how option #1 can work when access for menu links is taken into account.

dbu’s picture

the drawback i see in tying the access control into routing is that access control on content will have to be implemented differently - or you would hook the same system into content loading (thinking about aggregation things here that load content which has no explicit route of its own where you could add access control).

Crell’s picture

We already have separate access systems for routes and node/content, which is fine. With multiple access checks per route we could do both checks on paths where a node is used, which actually makes a lot of sense, I think.

catch: Switching to a precompiled list of access checkers is going to be a larger task, since I'd need to refactor how the Route compiler works and gets saved. I don't know if that's something you're willing to do in this issue as it touches on a few other areas. (The Generator issue would touch on the route compiler, too.)

catch’s picture

@Crell do you mean refactoring it so it saves the access checkers? If so that seems completely reasonable to do here.

If it's a massive refactor then I guess the answer is 'it depends'.

Crell’s picture

Things that would be involved in a pre-compile approach:

- Switch RouteCompiler and CompiledRoute to be subclasses of Symfony's (which we likely want to do anyway for the Generator).
- That may involve some upstream tweaking of RouteCompiler, because right now it's not very well designed for extension.
- In our subclass, add a listener. However, the way the Route works right now it expects a static RouteCompiler class name, hence why it may need upstream refactoring.
- Fire some new event, which allows us to extend the route compilation.
- One of the listeners on that event examines the list of defaults and requires and adds information to the compiled route about what access checkers we'd want to fire on it.
- Store the compiled route. This may be troublesome for serialization reasons. (The Route loses its compiled route when serializing by design, because I got a fatal otherwise.)
- When locating routes in initial matcher, pull back and set the compiled route object, too. Which, thinking about it, I don't think the Route object has a way to do.
- Run access checks just on the information specified in the compiled route.

Looking at that again, I hate it. :-) Way too complicated, and too many upstream dependencies.

Alternatively, we could store the access checkers to be called on the route, rather than the compiled route. That would skip at least some bits of that, but not all. Likely, what we'd be storing are service IDs, which means we need a container aware request listener that can coordinate the access checkers. And it would need access to the route, which by default Symfony does not put on the request attributes. (There's no reason we couldn't, though...)

To be fair, the "store a list of service IDs to invoke on self" logic is something we had discussed for partial matchers in relation to sdboyer's concerns in #1784040: NestedMatcher may be too slow and non-deterministic, although again the container dependency makes me uneasy.

In any case, it's non-trivial either way.

catch’s picture

Yeah this is why I would've preferred a lot more discussion in this issue before the router patch got in at all :/

If the access checkers stored with the route still end up being a request listener, how do we go about firing those for checking menu link access? Presumably we could fire the event in a different code path without it actually being a sub-request?

Crell’s picture

To be clear, there's no technical reason we cannot simply stick an _access_callback and _access_arguments property set on a Route and have a listener that just calls one with the other. That would be simple to do; I just think it's an incredibly bad idea for multiple reasons (not the least of which is the use of function callbacks within an otherwise cleanly OO system).

If we attach the full Route object to the request attributes, then a later listener can just toss the Route into a stand-alone object that does the access checks. The listener is just glue code then, which is a Good Thing(tm). That same object then would be available via the DIC at other times, and you just throw a Route at it; I suppose that object is the one that would then be container aware.

Of course, we still need to factor the user out of a global and into the session where it belongs, but that's not going to happen in this issue. :-(

sdboyer’s picture

To be clear, there's no technical reason we cannot simply stick an _access_callback and _access_arguments property set on a Route and have a listener that just calls one with the other. That would be simple to do; I just think it's an incredibly bad idea for multiple reasons (not the least of which is the use of function callbacks within an otherwise cleanly OO system).

we're defining routes through hook_route_info() - it's not cleanly OO. so it seems premature to cite that as justification for making (dev) user interaction with the system more complex (i.e., #7) than the single point of interaction hook_menu() currently provides.

access control is one of those cases where, i think, the needs of a CMS diverge more sharply from a framework. the vast majority of the routes & access logic for any given site will be provided either by core or contrib, not by the people working on that site itself. it seems to me that there's a fair bit of inherent risk in a system where a poorly implemented/targeted access listener (or combination of listeners) could potentially end up granting access to a variety of unintended routes, with no notification to the site admin.

it's probably not *that* big of a concern, but i wanted to include it as a concern in addition to those cited in #7.

Crell’s picture

hook_route_info() is, hopefully, not long for this world: #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent

I agree that we don't want to make a system so complicated that you cannot secure it. But with multiple access checks per route (something just about everyone I've talked to has said they want), I don't see how it can be not more complex than a single callback function.

plach’s picture

I'm not diving into the implementation details, but I think multiple access checks per-route are definitely a win: currently we make all sort of hacks everytime we have to stack another check onto a previously existing access callback. The Entity Translation project has a nice example of how we are pathetically trying to cope with this limitation in D7 (basically storing the previous information as an access argument and proxying it in the replacing callback):

<?php
/**
 * Entity edit form access callback.
 */
function entity_translation_edit_access() {
  $args = func_get_args();
  $entity_type = array_shift($args);
  $entity = array_shift($args);
  $langcode = array_shift($args);

  $handler = entity_translation_get_handler($entity_type, $entity);
  $translations = $handler->getTranslations();
  $langcode = entity_translation_form_language($langcode, $handler);

  // The user must be explicitly allowed to access the original values.
  if (!$handler->getTranslationAccess($langcode)) {
    return FALSE;
  }

  // If the translation exists or no translation was specified, we can show the
  // corresponding local task. If translations have not been initialized yet, we
  // need to grant access to the user.
  if (empty($translations->data) || isset($translations->data[$langcode])) {
    // Check that the requested language is actually accessible. If the entity
    // is language neutral we need to let editors access it.
    $enabled_languages = entity_translation_languages($entity_type, $entity);
    if (isset($enabled_languages[$langcode]) || $langcode == LANGUAGE_NONE) {
      $edit_form_item = array_shift($args);
      return _entity_translation_callback($edit_form_item['access callback'], $args, $edit_form_item);
    }
  }

  return FALSE;
}
?>

Being able to append another check would be just what we need.

sdboyer’s picture

ok good, a concrete example. what entity_translation has to do there is indeed a bit hacky, and that sucks. certainly, multiple access checks would help with that. but it doesn't allay my concerns about DX (though the discussion about replacing hook_router_info() helps on that front), or what seems like the potential for unintended information disclosure (et. all) vectors.

Crell’s picture

The natural way to handle information disclosure issues is to adopt the model used by hook_node_access: allow access if at least one handler says Yes, and no handlers say No. That is an implicitly a deny-by-default approach. The caveat there is that we'd definitely need preprocessing on Route objects, then.

Crell’s picture

Issue tags: +WSCCI, +wscci-hitlist

Forgot tags.

sdboyer’s picture

The natural way to handle information disclosure issues is to adopt the model used by hook_node_access: allow access if at least one handler says Yes, and no handlers say No. That is an implicitly a deny-by-default approach. The caveat there is that we'd definitely need preprocessing on Route objects, then.

of course - if that's the model that's used. i'm under the impression there's no guarantee of that, judging from from one of the drawbacks to this approach given in the original post:

Results in a "default allow" model. (That could be good or bad.)

to be clear, i'm mostly convinced of the architectural approach at this point, and am playing devil's advocate to make sure we've thought of these things. my concerns are re: performance and complexity/understandability/debuggability.

Crell’s picture

Mostly convinced of which approach? :-)

Understandability-wise, if we want multiple access checkers then matching the logic for hook_node_access() would, I think, be best as it is then consistent.

Performance-wise, the best approach would be a precompile step so that we have information in defaults or in requirements on the Route that we can leverage directly, and just loop over.

Complexity-wise, the patch in #3 is the simplest we're going to get. At scale, though, it's not going to be as performant as something precompiled. Something precompiled is going to be more hard/complex to implement and involve more magic.

Which poison do we want?

sdboyer’s picture

Mostly convinced of which approach? :-)

the separated access listener approach that decouples access from the route.

Understandability-wise, if we want multiple access checkers then matching the logic for hook_node_access() would, I think, be best as it is then consistent.

dear god. are we having a serious conversation about FOLLOWING the node_access pattern for DX purposes?

Performance-wise, the best approach would be a precompile step so that we have information in defaults or in requirements on the Route that we can leverage directly, and just loop over.

agreed; precompilation and attaching to...something is the same remedy i've had in mind for the concerns about excessive global registration of matchers. this case is easier, since Routes already exist as a logical unit to attach things to.

Complexity-wise, the patch in #3 is the simplest we're going to get. At scale, though, it's not going to be as performant as something precompiled. Something precompiled is going to be more hard/complex to implement and involve more magic.

complexity isn't entirely encapsulated by the patch itself. we can effectively reduce the barrier to understanding by providing some tools that help inquisitive minds introspect over the set of routes as they've been compiled.

if we don't go for something precompiled, then what's the mechanism we use to let access listeners target only specific routes?

Crell’s picture

node_access the hook, not the grants system. Different animals. :-)

If we don't compile, then I think it's up to each listener to decide if it cares. Vis, the one in the patch above checks if there's a _permission key. If there isn't, it ignores the route and does nothing. If there is, it either does nothing (passes) or throws an access denied exception (fails). The listeners all get called on all requests, but can self-terminate as fast as they want with an if statement.

My biggest concern is that a compile step means we need something in the access system to be ContainerAware, and the fewer objects are ContainerAware the more decoupled everything gets.

sdboyer’s picture

ohh - yes, the hook is definitely better.

it may be premature microoptimizing, but *even* if it's a single method call with a quick escape hatch, i imagine we're talking about not only using this during the initial routing process, but also for url generation (and of course, subrequests). url generation is really the concerning area - you've got the number of listeners attached * the number of urls being generated, and we're spending maybe 20-30ms just checking access on links.

it certainly makes sense to proceed with this for now, but it needs performance testing and we ought to at least keep an eye on a more route-targeted/compiled approach in the event it comes up bad.

Crell’s picture

sdboyer: So you're saying to proceed with #3, but keep open a (post-1 December?) door to refactor to compiled?

catch, Dries, how do you feel about that?

catch’s picture

@Crell, I don't feel very good about it, because it seems to leave all the questions I raised still open with no real path forward to fixing them. The access check is right in the critical path for performance so we need to get it right.

Also there's going to be a /lot/ of router items to convert to the new system, so I'd prefer to try to have something we intend to commit to rather than something that's almost definitely going to be refactored with API changes later - access control on the router isn't a feature and in fact it's currently blocking release, so I'm not sure why we have to rush to commit this.

pwolanin’s picture

Status: Needs review » Needs work

While we might be de-coupling links from routes more clearly, the ability in D6 to hide links that the current user didn't have access to was a huge UX win over D5.

I agree with sdboyer, the critical performance path here may not even be serving the single page request, but checking access on 10's or more links and subrequests. I'll note that an example of 7 where this can already cause performance issues is with Views - since the whole View needs to be loaded and run before access can be determined.

Also - we made API changes to D6 because people didn't understand the system where access callbacks inherited - we need to make sure that doesn't happen again, and maybe have a lot of default deny behavior.

Chaining access callbacks is not hard - i think chx and I rolled a POC a couple times - I'd rather focus on making sure the basic access control model is clear and comprehensible to anyone writing a route for their module.

sdboyer’s picture

Status: Needs work » Needs review

ok, since pwolanin is backing up my speculation with evidence, i'll flip-flop: the performance concerns associated with link generation suggest to me that our initial approach to this should not allow for arbitrarily large numbers of access listeners. we're just asking for trouble.

bind them to the route, a la the second option in the summary. maybe we set them free later.

sdboyer’s picture

Status: Needs review » Needs work

xpost, sorry.

YesCT’s picture

Crell’s picture

After assorted IRC conversations and further thought, here's my recommendation for how to move forward:

- Conditional Plugins (#1743686: Condition Plugin System) could be nice to use here. They are not in core yet, though, so we won't wait for them. Possible refactoring for that is a later task.

- We want to define a new type of service, that is an access checker. This will need a new interface, which has two methods: access(Route $route) and applies(Route $route). (Alternate names welcome.) An AccessCheck service must implement that interface and tag itself in the Container as an access_check.

- We have an AccessCheckManager that is ContainerAware.

- When we are compiling routes, we need to introduce a step where the AccessCheck manager iterates over *all* routes before they get saved (there is no listener for that yet; we'll need to add one, which overlaps with #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent) and allows each access checker to decide if it will care about the route. Generally that will be based on a Requirement on the route. Eg, if you specify a "_permission" requirement, then the PermissionAccessChecker::applies() method will return TRUE. If you don't, it will return FALSE.

- The AccessCheckManager records an array on the route in options, called _access_checkers. That is an array of service IDs (or some other way of identifying the access checkers) that are relevant to that route.

- The route object then gets saved as it already does.

- In the kernel.request event, there is a listener that fires after matching happens that bridges to the AccessCheckManager. AccessCheckManager takes the Route, and looks at its $route->getOption('_access_checkers') property. It then retrieves *just* those access checkers and calls $checker->access($route) on them in turn. $checker->access() may return one of TRUE, FALSE, or NULL. (Or constants set to those values, as the node access system does.)

- AccessCheckManager implements the same logic as hook_node_access: If at least one checker returns TRUE, and no checker returns FALSE, then it does nothing, aka it allows the request to continue. Otherwise, it will throw an access denied exception.

That buys us about as targeted a runtime situation as we can manage, so we're not running a dozen useless access checkers on each route. However, we still get really-simple DX for routes (you care about permissions? Add _permission and you're done. You care about X? Add _X and you're done.), and it's very simple for modules to add additional access checkers. Just conform to the interface and throw yourself into the Container with the right tag. It makes the route compilation/dump process more involved, but I think that's an acceptable trade off.

Note: It's possible that the plugin system would help simplify the access check logic. I'm not sure. The registration mechanism I have above may not fly, since we don't have Container tags at runtime, only compile time. We should check with EclipseGc on that. But for now I don't want to wait on Conditional plugins, and want to keep the Route definition DX as simple as possible.

Actually, there's no reason that you couldn't use a single access checker that deals with conditional plugins defined in some structure that lives under the _conditionals requirement on the route. Hell, that could even be done in contrib if we wanted to. So, with this approach we should be able to punt on Conditional plugins indefinitely and not even refactor later. Just add. Which is a good sign.

Also, all names used above are subject to change. "Access checker" sounds dumb to me, but it's the best I could come up with off the top of my head. *shrug*

sdboyer, catch, I have a volunteer willing to code up the above, so do you see a problem with it before he starts? :-)

Crell’s picture

To the point about menu links, if a link is coupled to a router item, then the access check would be:

// We'd have to do this part somehow anyway.
$route = get_route_from_link_somehow($link);

$access = $container->get('access_check_manager')->check($route);

Which would run only the checkers that are listed as being relevant. I don't think we can get more performant than that.

tnightingale’s picture

Assigned: Unassigned » tnightingale

Taking a stab at turning #34 into a patch.

dbu’s picture

sounds like a good solution to me, from the outside :-)

just a note on the container usage in #35: you are aware that you should try to inject as much as you can, instead of pulling services from the container? it makes the code more reusable, i.e. if for some reason you want to use the same system with several instances of the same services that uses separate instances of other services...

klausi’s picture

@tnightingale: any ETA for the first patch? Would be interested to work on this, too, for access control in #1816354: Add a REST module, starting with DELETE.

Crell’s picture

dbu: Yes, but we also don't want to initialize a whole bunch of services that we won't use. Avoiding instantiating a dozen access checker objects that we won't need is why we're not just making everything it's own listener.

We've also concluded that small factories being container aware is not a problem. See the first half of: #1810912: [meta] Decide on pluggability.

tnightingale’s picture

klausi: I have a rough start sitting on my local, nothing functional yet. I'm probably not going to be able to return to this tonight but plan to post something tomorrow evening. If you want to get stuck in earlier, I can post what I've got so far.

klausi’s picture

@tnightingale: not urgent, Crell has given me some input for #1816354: Add a REST module, starting with DELETE, so I'm covered for the next two days.

tnightingale’s picture

With the help of katbailey and after reading through what feels like half of core, I'm starting to get a grasp on the routing system. Specifically around the dump/compile step.
Which leads me to:

When we are compiling routes, we need to introduce a step where the AccessCheck manager iterates over *all* routes before they get saved (there is no listener for that yet; we'll need to add one

I assume we'll want to use the EventDispatcher for this which means the routing system needs to define an event :). Should a RoutesEvent be fired from the RouteBuilder or MatcherDumper? (we're going to need to inject the dispatcher in to one of these)

My guess is the RouteBuilder as that seems like a place that is going to be refactored sooner rather than later and I'm totally not going to get this on my first pass. On a side note: due to how we currently dump routes one module at a time, the RoutesEvent has to be fired once per module.

Once the event fires and we're into the AccessCheckManager I think I have things worked out. I have loosely modeled the AccessCheck registration and execution process on the param converters and manager in #1798214: Upcast request arguments/attributes to full objects.

dbu’s picture

@Crell: yes, pulling in those services (access control checkers in this case) you actually need based on the current data makes sense. i was referring to the code sample where you pull 'access_check_manager' - everything you will need for sure you want to inject.

Crell’s picture

Yes, we'll need an event in the RouteBuilder. The MatcherDumper is concerned just with dumping, so it shouldn't go in there. RouteBuilder is our glue object for all of that stuff.

There's some related Symfony CMF discussion here: https://github.com/symfony-cmf/Routing/issues/28

tnightingale’s picture

Crell: In #34 you say:

In the kernel.request event, there is a listener that fires after matching happens that bridges to the AccessCheckManager. AccessCheckManager takes the Route, and looks at its $route->getOption('_access_checkers') property.

I am having trouble figuring out how to get the Route from the Request. I notice in #14 you alluded that it's probably not there:

And it would need access to the route, which by default Symfony does not put on the request attributes. (There's no reason we couldn't, though...)

Has this changed? If not, I think I figure out how one might add the Route to the request ($request->attributes->set('route', $route)) but I am unsure where the best place for this to occur would be. Do you have any thoughts/suggestions?

I've attached a WIP patch so you can see what i've got - definitely not functional or review-worthy yet.

Crell’s picture

Status: Needs work » Needs review

No, that hasn't changed. We need to add it in the FinalMatcher. I think there's already a "route" property that has the route name; I'm not sure if it's better to take that over, or add a new property like route_object or something. dbu, any opinion?

Status: Needs review » Needs work

The last submitted patch, 1793520.access_control.45.patch, failed testing.

sdboyer’s picture

i'd prefer the route object be stored in a separate property, and we continue to store the route name in '_route'.

tnightingale’s picture

Status: Needs work » Needs review
FileSize
22.26 KB

Storing route object in 'route_object' property.

Status: Needs review » Needs work

The last submitted patch, 1793520.access_control.49.patch, failed testing.

tnightingale’s picture

RouterTest failing because of 'default deny' approach, I assumed this is what we wanted based on:

- AccessCheckManager implements the same logic as hook_node_access: If at least one checker returns TRUE, and no checker returns FALSE, then it does nothing, aka it allows the request to continue. Otherwise, it will throw an access denied exception.

-> No checks return TRUE or FALSE == access denied. If this is correct then the test needs to be changed.

Not familiar enough with upgrade tests to know why DIC is passing old parameters.

We likely need more test coverage on the access check service stuff.

aspilicious’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterPermissionTest.phpundefined
@@ -0,0 +1,59 @@
\ No newline at end of file

Neeeds newline :)

Crell’s picture

Yes, default-deny is what we want to have. Which probably means one of the supported checkers is something that just checks _access, which is boolean false or boolean true. That becomes the bypass flag for routes we want to be always-available. (You could set _access FALSE and cause the route to never, ever be available, but that seems a wee bit silly.)

Crell’s picture

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -0,0 +1,143 @@
+  /**
+   * Array of AccessChecks.
+   *
+   * The array keys are fully qualified class names; The value is an associative
+   * array containing service_id (the ID of the container service that provides
+   * the corresponding checker) and arguments, which is an associative array
+   * of options to pass to the converter at conversion time.
+   *
+   * @var array
+   */
+  protected $checkIds;

Does this need to be keyed by class name? Class names should be irrelevant here. (They're relevant for upcasting, but not for access checks.)

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -0,0 +1,143 @@
+  /**
+   * Array of instantiated check objects.
+   *
+   * The array keys are full qualified class names; the values are the
+   * instantiated AccessCheck object.
+   *
+   * @var array
+   */
+  protected $checks;

Actually, the code below suggests it's keyed by service ID.

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -0,0 +1,143 @@
+  /**
+   * Check route against applicable access check services to determine whether
+   * it is accessible.

One line rule. :-)

+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
@@ -43,6 +66,7 @@ public function onKernelRequestAccessCheck(GetResponseEvent $event) {
+    $events[RoutingEvents::ROUTES][] = 'onRoutingRoutesSetAccessCheck';

I'm not sure we need to use a constant here. Symfony does for kernel events, but not for other events. I'm not sure what the pattern is.

Although perhaps that's one possible solution to #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners ?

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.php
@@ -53,7 +53,7 @@ public function matchRequest(Request $request) {
-      return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name));
+      return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name, 'route_object' => $route));

To be consistent, I think we should use _route_name, not route_name. All of the other "special and magic" keys have an _ prefix.

+++ b/core/lib/Drupal/Core/Routing/RoutingEvents.php
@@ -0,0 +1,26 @@
+/**
+ * Contains all events thrown in the core routing component.
+ */
+final class RoutingEvents
+{
+    /**

Drupal coding standards, please. :-)

+++ b/core/lib/Drupal/Core/Routing/RoutingEvents.php
@@ -0,0 +1,26 @@
+    const ROUTES = 'routing.routes';

"routes" seems like too generic a name here. Maybe routing.dump, since that's what we're doing. This happens on dump.

Actually... in a way, this could use the same event as the "route alter" system, no? It just needs to have a very low priority and then it will happen after all other routes are available. No need for an extra event.

tnightingale’s picture

Status: Needs work » Needs review
FileSize
9.12 KB
23.78 KB

Changes address issues outlined above.

Added a DefaultAccessCheck (not feeling particularly imaginative with names today) service that checks '_access' boolean property.

I've left the RoutingEvents const class in as an example; I think it is potentially a good solution for #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners. No issues with removing if people disagree though.

Actually... in a way, this could use the same event as the "route alter" system, no? It just needs to have a very low priority and then it will happen after all other routes are available. No need for an extra event.

From what I can tell this "route alter" system doesn't exist yet? I agree with the ambiguity of the ROUTES event, so I have renamed it to ROUTE_ALTER. Part of me thinks this will work fine while the other thinks that the access check attaching process should be an explicit step in the routes lifecycle in order to maintain some control over the sequence.

Status: Needs review » Needs work

The last submitted patch, 1793520.access_control.55.patch, failed testing.

dbu’s picture

No, that hasn't changed. We need to add it in the FinalMatcher. I think there's already a "route" property that has the route name; I'm not sure if it's better to take that over, or add a new property like route_object or something. dbu, any opinion?

for the symfony cmf router we ended up putting the object into _route. and tell the generator to generate a route from the object in that case, instead of thinking its a string. we have the https://github.com/symfony-cmf/Routing/blob/master/ChainedRouterInterfac... interface to check if the router supports anything else than a standard symfony route name (which additionally to only want string also has a restricted regexp on the name).

sdboyer’s picture

oh good - let's obey the same pattern as symfony CMF on this, then. i actually expected _route to contain an object at first, and was surprised when i found strings there.

now, re: #34, since i never really responded.

agreed that conditional plugins are well-suited to this general case, but there's no reason to wait for them (not that we could or would), because i think they're a case that's probably more suited to UI-driven choices. (...reading further, per your comment about a _conditionals property. regardless, the base system oughtn't concern itself with that.

the basic compile-time mechanism you're describing does sound sane, and sounds like it should adequately address the runtime performance impact for route generation. so...

That buys us about as targeted a runtime situation as we can manage, so we're not running a dozen useless access checkers on each route.

agreed. however:

However, we still get really-simple DX for routes (you care about permissions? Add _permission and you're done. You care about X? Add _X and you're done.), and it's very simple for modules to add additional access checkers. Just conform to the interface and throw yourself into the Container with the right tag.

not sure i agree about the DX here. several individually simple components do not necessarily make a simple overall system. as we've discussed, the fact that we are decoupling the declaration of access checks from the route itself introduces more unknowns into the picture for devs. i'd really like, for my sake and others, to see a diagram that explains how these various axes interact.

klausi’s picture

Tested the patch from #55 with rest.module for #1816354: Add a REST module, starting with DELETE, works! You can find the code in the "access" branch in http://drupal.org/sandbox/klausi/1815566

Per default rest.module handles access with _permission on the routes. Now let's say I want to do authorization with an OAuth module that pulls an access token from the request query. That module registers its own OAuthAccessCheck service. Problem 1: how is the request object injected into it? The access() method only gets passed the route object, which is not enough. Problem 2: how does the module get rid of the _permission checks on the routes? The _permission service will deny access because there is no cookie it could derive a user from. So OAuth module would need some kind of hook_route_info_alter() to kick out the _permission properties on the routes?

I'm not sure if it is a good idea to deny access as soon as a service returns FALSE. It only means that the particular service could not successfully authorize with its own methodology. Other services do not get a chance to try a different approach. Would it be a good idea to reverse the logic? As soon as a service determines that the request is legit we stop and grant access?

Crell’s picture

klausi: The deny-by-default logic is what we're doing for nodes, because it's more secure. I think I'd prefer to stick to that approach for consistency.

For OAuth, IIRC that works by adding tokens to the URL GET string, right? In that case, it would be the responsibility of the Generator (#1705488: Implement a generator for Drupal paths) to add in a token when generating the URL, and then the OAuthAccessCheck service simply checks for the presence of that token. It would actually be essentially the same logic as #1798296: Integrate CSRF link token directly into routing system.

I'm totally down with making _route be the actual route object like CMF. We want to merge code with them anyway so let's do.

Route_alter would come in from #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent, but we can totally get the jump on them. :-) We can sort out the common code easily enough.

klausi’s picture

@Crell: Ok, so we manage that OAuthAccessCheck has all data it needs. But what about the problem that it never gets called because PermissionAccessCheck has already determined that access should be denied? What should the Oauth module do in order to disable the permission checks?

Crell’s picture

Nothing. If there is no _permission key, PermissionAccessCheck never gets called. If there is, then we're explicitly saying that we WANT that route to have to pass BOTH checks.

If there's a _permission key but you want to rip it out, there will be an alter for routes roughly analogous to hook_menu_alter where you can futz with it.

tnightingale’s picture

FTR the ability to alter routes exists in this patch via the RoutingEvents::ROUTE_ALTER event. It just could do with a drupal-style 'alter' API around it.

Crell’s picture

We're trying to avoid hooks in the new routing system, as they introduce a host of other dependencies. Events are cleaner for this. A unified "alterable" event type is something we can look into post-feature-freeze as an API cleanup.

klausi’s picture

Ok, so if I have the use case that I want the web API to be accessible by javascript callbacks with cookies AND OAuth from somewhere else I have to do:

* oauth.module that provides OAuthAccessCheck and let's say something in _oauth in the route.
* oauth_and_permission.module that provides OAuthAndPermissionAccessCheck that manually calls PermissionAccessCheck and if that fails OAuthAccessCheck. It also has to listen to the route alter event, copy stuff from _permission and _oauth to _oauth_and_permission in the routes and unset _oauth and _permission on them.

I'm starting to think that it would be easier if oauth.module would just map its request to a global user account before the routing system is invoked. Then it does not have to mess with access stuff in Routing at all, it just has to make sure that the account has the necessary permissions and the standard PermissionAccessCheck could be used.

klausi’s picture

Status: Needs work » Needs review
FileSize
758 bytes
24.31 KB

This should fix the simpletest errors in update.php, just added one line.

Crell’s picture

That sounds more like a case where full on conditional plugins (nested if-and-or support) would be useful. Those aren't in core yet, so we can't use them. They could be added as another access checker, though, so I'm comfortable moving forward as is for now.

The alternative is to change the Permission access checker to never return FALSE; it returns TRUE or NULL/don't care. That is, actually, more akin to how permissions work for node access now. With a default-deny model, "TRUE/ignore" is the typical case since never returning TRUE implies FALSE.

So if we recommend that most checkers not return FALSE, as is the case for hook_node_access() now, then we implicitly get OR behavior rather than AND behavior. Which is consistent with node_access now, and I don't think is insecure.

That should be a trivial change here. klausi, tnightengale, who wants to get to it first? :-)

tnightingale’s picture

PermissionAccessCheck now returning NULL on failed user_access().

tnightingale’s picture

Whoops forgot interdiff and didn't pull HEAD :-P

Berdir’s picture

Yes, I wanted to suggest the TRUE/NULL behavior too. That's different to how it works now, but this system is designed to support multiple checks unlike access callback, it does in fact more work like hook_node_access(), so this makes perfect sense I think.

That doesn't really solve the question whether OAuth should be doing authorization or authentication, because the main problem that I see is that if the specific route does e.g. stuff with the currently logged in user (like a author assignment on node save, additional user_access() checks on which information is visible, ...), unexpected things might happen if the OAuth allows access without also doing a "log in" of the user it belongs to, if any.

That's also an issue you have in 7.x with services.module, when you have a different authorization method.

That's not a problem that needs to be solved in this issue, however, so we can happily ignore it :)

klausi’s picture

I'm not so much worried about node_save(), we can do clean API-only saves with entity_create() + $entit->save(). That works quite well since D7. Services is ugly in this regard as it executes node forms which can have 1000 random side effects.

Field access checks for example are a bigger problem. We are currently building our access APIs always around $account user objects, which are normally derived from the global state. That scares me.

Crell’s picture

Status: Needs review » Needs work

$account should be derived from the session on-demand, which should be managed by the request object, not our current code. cf, #335411: Switch to Symfony2-based session handling. No need to worry about that here.

+++ b/core/lib/Drupal/Core/Routing/RoutingEvents.php
@@ -0,0 +1,26 @@
+final class RoutingEvents
+{
+    /**
+     * The ROUTE_ALTER event is fired when a collection of routes have been
+     * added to the route builder.
+     *
+     * This event is used to process new routes before they get saved.
+     *
+     * @var string
+     *
+     * @api
+     */
+    const ROUTE_ALTER = 'routing.route_alter';
+}

I think we should consider merging this class with the RouteEvent itself. I'm not sure what the value is to separating it. I know Symfony does, but I don't know why. :-) I'm OK with leaving it separate for now, but we should discuss merging it later. That's an easy follow up.

That said, the first line needs to be one line, not two. :-(

+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
@@ -43,6 +66,8 @@ public function onKernelRequestAccessCheck(GetResponseEvent $event) {
+    // Setting very low priority to ensure access checks are run after alters.
+    $events[RoutingEvents::ROUTE_ALTER][] = array('onRoutingRouteAlterSetAccessCheck', 900);

This is backward. High priorities happen first. If we want this to happen last, it should be 0 or something.

+++ b/core/lib/Drupal/Core/Routing/FirstEntryFinalMatcher.php
@@ -53,7 +53,7 @@ public function matchRequest(Request $request) {
-      return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name));
+      return array_merge($this->mergeDefaults($matches, $route->getDefaults()), array('_route' => $name, '_route_object' => $route));

Didn't we say we're going to just merge this into _route being the route object, a la Symfony CMF, for compatibility and consistency?

Berdir’s picture

@Crell: And when we switched to symfony session handling, magic symfony fairies will show up and and invent a user object when there is none? :) OAuth still needs to provide it, either through global $user or through some other way if the code behind that route requires one.

Which means that implementing the access checker is pointless because if it needs to fake the logged in user, then that can also be used for the default _permission check.

Anyway, again, this isn't the problem here, there are many valid use cases to require multiple access checks and this issue allows to do that. It can't solve that other problem.

tnightingale’s picture

I think we should consider merging this class with the RouteEvent itself. I'm not sure what the value is to separating it. I know Symfony does, but I don't know why. :-) I'm OK with leaving it separate for now, but we should discuss merging it later. That's an easy follow up.

My guess for the leaving it separate as a convention is that it is somewhat self-documenting particularly when there are multiple events: re: #1751116: [policy, then patch] Determine standard for documenting Events and Subscribers/Listeners.

Didn't we say we're going to just merge this into _route being the route object, a la Symfony CMF, for compatibility and consistency?

Sorry I skipped that. If we do this, what happens to the code that's expecting the '_route' param to be a string? I assume that's where the interface that dbu was referring to in http://drupal.org/node/1793520#comment-6661686.

Crell’s picture

I don't think we have any code that looks for _route yet, so it's fine. I need to get off my arse and push NestedMatcher upstream, then we can get the code in sync again. If it doesn't break, go ahead and do it now.

disasm’s picture

I originally tried rebasing the routing-permissions branch and applying the interdiffs to keep history, but that didn't go to well at all. As such, I've taken the latest patch and applied it pushing a branch call routing-permissions-issue on the WSCCI sandbox.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
24.39 KB

Attached is a patch fixing the brace on the wrong line and the priority Crell mentioned in #72. Interdiff to #69. This is also pushed to the routing-permissions-issue branch of WSCCI sandbox.

disasm’s picture

attached renames _route_object to _route as Creell suggested in #72.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-78.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

It looks like all the test failures are because the tests are asserting that _route contains the name of the route, which makes sense that they failed. Since $attribute['_route'] now contains the route object instead of the name, what should I assert in the tests?

disasm’s picture

attached is a patch that fixes the tests by adding an option _name to the options for the route object.

disasm’s picture

rebased patch off of #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent. The patch to review is the one that ends in dx-do-not-test. The other patch is a merge of the two issues.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-82.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
@@ -18,21 +21,41 @@
+    $this->accessManager->check($request->attributes->get('_route'));

It's probably a good idea to document that access checkers will throw exceptions if they object, which is why the listener doesn't seem to be doing anything. That will help avoid confusion for developers later.

This will likely need to reroll on top of #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent, but otherwise I think we're good to go here.

disasm’s picture

Bad reroll... Attached is a patch that doesn't contain twig and views ;-)

Status: Needs review » Needs work
Issue tags: -WSCCI, -wscci-hitlist

The last submitted patch, drupal-routing_permissions-1793520-85.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI, +wscci-hitlist

The last submitted patch, drupal-routing_permissions-1793520-85.patch, failed testing.

disasm’s picture

Had some problems where both issues were doing the same thing, but in slight different ways, mainly with RoutesEvent and RouteBuildEvent. I think I found all the issues in the rebase and got them fixed.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-89.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
31.71 KB
33.15 KB

Found the problem with #89. Attached patch is passing all router tests for me.

(Review the do-not-test; the other is based on the DX/YAML conversion.)

disasm’s picture

Attached is a patch that adds a default access denied test. Review the do-not-test patch.

Status: Needs review » Needs work
Issue tags: -WSCCI, -wscci-hitlist

The last submitted patch, drupal-routing_permissions-1793520-92.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-92.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +wscci-hitlist
klausi’s picture

FileSize
8.78 KB

I pushed some documentation fixes to the 1793520-routing-permissions branch in the wscci sandbox: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/013fa9f?hp=92...
Not sure how to create patches against core ... do we merge or delete/rebase that branch?

disasm’s picture

@klausi: Crell and I talked about this on Saturday. We're rebasing off of 1801570-routing-dx, not upstream 8.x. If it gets rebased (i.e. needs rerolled to apply to 8.x), then we'll delete the branch and push the new rebased one off of 8.x.

As such, I cloned upstream 8.x, and removed all the commits after:

commit ee2acd68cca6ecfb2a1f3436c6adb593659489a8
Author: Dries <dries@buytaert.net>
Date:   Sat Nov 3 10:36:10 2012 -0700

    Issue #1696786 by Fabianx, jenlampton, stevector, steveoliver, jwilson3, amateescu, chx: Integrate Twig into core: Implemen

Then to create the patches:

git diff 1801570-routing-dx > drupal-routing_permissions-1793520-#-do-not-test.patch (Patch to review)
git diff 8.x > drupal-routing_permissions-1793520-#.patch (Patch for testbot)
klausi’s picture

Resolved a conflict in CoreBundle due to the added Serializer stuff. No other changes.
Git commands I used for the patches:

git checkout 1793520-routing-permissions
git branch 1793520-routing-permissions-klausi
git checkout 1793520-routing-permissions-klausi
git merge 1801570-routing-dx
git rebase origin/8.x
git diff origin/8.x > 1793520-99-routing-perm.patch
git diff 1801570-routing-dx-klausi > 1793520-99-short-do-not-test.patch # that branch is a rebase of 1801570-routing-dx against 8.x

Status: Needs review » Needs work
Issue tags: -WSCCI, -wscci-hitlist

The last submitted patch, 1793520-99-routing-perm.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

#99: 1793520-99-routing-perm.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +wscci-hitlist

The last submitted patch, 1793520-99-routing-perm.patch, failed testing.

disasm’s picture

reroll against dx branch. Some route_tests in the YML were renamed because of added ones in DX, and an access permission was added to a new Dynamic Route Test.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-102.patch, failed testing.

disasm’s picture

Status: Needs work » Postponed

This needs rerolled eventually, however; Crell suggested we wait for the DX patch to get in since it's been switched back to RTBC.

disasm’s picture

Status: Postponed » Needs work
disasm’s picture

attached is a reroll against latest 8.x. DX issue was committed, so we're unblocked.

Latest sandbox branch pushed as well with rebase to 8.x, so delete your branch and repull if you work on the issue.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-107.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
24.56 KB

Rebased off of 8.x with the re-push of 1801570. Attached is a reroll.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-110.patch, failed testing.

moshe weitzman’s picture

I think the DX here is poor, but we really need to see some conversions in order to be sure. I suggest converting the rest.module routes since they already use new router. I also request that you convert a route that uses custom access check like user/

Crell’s picture

We can't convert a route that isn't already ported to the new router, and this issue has been one of the blockers of doing so. :-) That sort of conversion would be part of #1801356: Entity reference autocomplete using routes

moshe weitzman’s picture

That route doesn't have custom access control. My bottom line is that this needs a non-test implementation in order for me to get comfortable with it. Don't say that this is impossible - just say that you don't want to.

Right now, my gut is that yet another event system complicates route processing in an unnecessary way. The patch asks each and every access control implementation if they care to apply access to the current path. Not so efficient. I think some more centralization would be helpful.

Crell’s picture

That request happens only on dump, not at runtime. At runtime, only the checkers that we know care will be run.

If you prefer, fine, "I don't want to". :-) Because converting a page callback to a controller is outside the scope of just ACL, and will have its own niggling bits when we do that. That's kitten maiming, since then we're refactoring a page callback at the same time we're refactoring the access system.

moshe weitzman’s picture

OK, so the list of checkers that care for a given route is cached somewhere. That sounds like the reviled theme registry which stores what preprocess functions are active for a given theme hook.

Thanks for the honesty :). We're at a standstill because I don't think this should go in with an implementation and you would rather not implement without this patch. We need some more opinions in here.

effulgentsia’s picture

Let's get #1801356: Entity reference autocomplete using routes in quickly then. I posted a review there. I'm tempted to postpone this issue on that one, but not doing so, because more test fixing and reviews can be happening here in the meantime.

effulgentsia’s picture

#117 would give us an example of something that uses a permission-based check. #112 also asks for a demonstration route with a custom access function. I suggest we therefore convert user/register to the new routing system. I'm happy to do so in a separate issue if that's what's wanted, but I don't see any reason it can't be included in this issue: any reason we can't make 'user_register' into the _content value and be done with it? We can have a follow up to turn it into a class method, but functions are allowed as controllers too.

Crell’s picture

Currently a function controller is assumed to be the old menu-routing system and massaged accordingly.

webchick’s picture

I'll also chime in as someone growing very weary of critical components of the new routing system being kicked further down the road as "not in scope." Let's get some real world cases in here already, so we can start comparing apples to apples. The amount of hand-waving going on around various components of hook_menu() is ringing huge alarm bells for me, and has since the original routing patch went in with no useful change notice, and even http://drupal.org/node/1822626 is still littered with "todo" and "ask Crell." :\

effulgentsia’s picture

Spin-off issue for #118: #1843084: Convert user/register to Route. It's a tiny patch: let's try to get it to RTBC and committed quickly.

Crell’s picture

catch’s picture

Yes one conversion of a real thing to a new system is not kitten maiming by any means at all. It's like calling removing one function a 'massacre'. The later those conversions happen, the more chance something gets in that's flawed with considerably less chance to fix it, and I also share webchick's concerns about the general state of hook_menu(). There's a lot of work to do to actually untangle all that stuff and not much of a plan for actually doing so. Ripping the router out from hook_menu() doesn't necessarily untangle things, it could just mean a big tangle spread out all over the floor...

@moshe: what happens is that the router is compiled, very, very similar to a menu_rebuild now, and it ends up in a table with much the same structure as menu_router in the end. When you see 'compile', think "one-off application-level cache from people who don't like application-level caching".

I think it's good if we can get multiple access callbacks per route. I don't like event dispatcher very much still (in general), but the original idea of the event listener running on runtime was unacceptable and doing it on compile time means it /ought/ to be efficient at least (since we have to compile anyway otherwise it's menu_rebuild() every request). Taking a look at the user/register patch now.

disasm’s picture

Status: Needs work » Needs review
FileSize
589 bytes
24.63 KB

Attached patch fixes the removed RegisterAccessChecksPass that was accidentally removed in a previous reroll. Should be green now.

Status: Needs review » Needs work

The last submitted patch, drupal-routing_permissions-1793520-124.patch, failed testing.

disasm’s picture

Attached is a patch that bundles #1843844: Convert cron to new routing system to show how an access subscriber can replace an access callback. This will still fail because rest dynamic route still needs an access parameter added, but I want to split out the changes so it's easier to review the changes. interdiff is to #124 without the commits from cron issue. Also, AccessManager now passes $route and $request to access method of all AccessCheckers.

disasm’s picture

attached patch fixes a quirk where PermissionAccessCheck was calling getDefault instead of getRequirement and fixes the rest routes by setting a _access requirement to TRUE. This should be green, and if it is, bring on the reviews!

fubhy’s picture

As mentioned earlier I am happy with the implementation in general. So here comes my nitpick review (more or less):

+++ b/core/lib/Drupal/Core/Access/AccessCheckInterface.phpundefined
@@ -0,0 +1,41 @@
+ * A access check service determines access rules for particular routes.

*An

+++ b/core/lib/Drupal/Core/Access/AccessCheckInterface.phpundefined
@@ -0,0 +1,41 @@
+   * @return mixed
+   *   TRUE if access is allowed.
+   *   FALSE if not.
+   *   NULL if no opinion.

That could be phrased a bit better.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -0,0 +1,145 @@
+ * Attaches access checks services to routes and runs them on request.

*check (not checks)

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -0,0 +1,145 @@
+   * Request Object
+   * @var object

Missing empty line. "Request Object" is not very descriptive either.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -0,0 +1,145 @@
+  }
+  /**

Missing empty line.

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+/**
+ * @file
+ * Definition of Drupal\Core\Access\DefaultAccessCheck.
+ */

These should be switched to "Contains" rather than "Definition of". @see http://drupal.org/coding-standards/docs#namespaces

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+    return (boolean)$route->getRequirement('_access');

missing whitespace between (boolean) and $route.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAccessChecksPass.phpundefined
@@ -0,0 +1,29 @@
+class RegisterAccessChecksPass implements CompilerPassInterface {

Missing docblock.

+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.phpundefined
@@ -18,21 +21,41 @@
+   * @param AccessCheckManager $access_check_manager
...
+  public function __construct(AccessManager $access_manager) {

These are out of sync.

I guess you can just call that param $manager.

+++ b/core/modules/system/lib/Drupal/system/Access/CronAccessCheck.phpundefined
@@ -0,0 +1,41 @@
+   * Implements Drupal\system\Access\AccessCheckInterface::applies().
...
+   * Implements Drupal\system\Access\AccessCheckInterface::access().

The full namespace is not required here. (You are already in the Drupal\system\Access namespace. A simple "Implements AccessCheckInterface::applies()."
Is sufficient here.

+++ b/core/modules/system/lib/Drupal/system/CronController.phpundefined
@@ -0,0 +1,31 @@
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;

That is used nowhere in the file.

klausi’s picture

Documentation fixes reported by fubhy.
More documentation fixes, updated flawed simpletest for _permission route.

So the tests are currently failing because _permission does not work. I had a look with xdebug and AccessSubscriber::onKernelRequestAccessCheck() gets called before FirstEntryFinalMatcher::matchRequest(). Therefore the _route property is not available yet and the access checks are never applied. Not sure what is going on.

Status: Needs review » Needs work

The last submitted patch, routing-perm-1793520-129.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
30.07 KB

Finally found the bug:

@@ -19,7 +19,7 @@ class PermissionAccessCheck implements AccessCheckInterface {
    * Implements AccessCheckInterface::applies().
    */
   public function applies(Route $route) {
-    return $route->hasDefault('_permission');
+    return (boolean) $route->getRequirement('_permission');
   }

Fixed permission apply check, simplified test case to one test method for performance reasons.
Fixed MIME matcher tests for _route objects.

Status: Needs review » Needs work
Issue tags: -WSCCI, -wscci-hitlist

The last submitted patch, routing-perm-1793520-131.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +wscci-hitlist

#131: routing-perm-1793520-131.patch queued for re-testing.

disasm’s picture

I'm not sure if this is a big deal or not, but it makes more sense to me to mimic the behavior of hasDefault than to cast the _permission to a boolean. Here's a patch that changes the behavior to call $route->getRequirements() and then check for a key existing in the array matching '_permission'.

disasm’s picture

Issue tags: +Stalking Crell

tagging for Crell.

Crell’s picture

There really should be a hasRequirement() method. Filed: https://github.com/symfony/symfony/pull/6098 Until that goes in and we resync Drupal (we're way behind right now) I don't care what code we use inside that method for now. :-)

+++ b/core/modules/system/lib/Drupal/system/Access/CronAccessCheck.php
@@ -0,0 +1,42 @@
+/**
+ * Allows access to routes to be controlled by an '_access' boolean parameter.
+ */
+class CronAccessCheck implements AccessCheckInterface {

Looks like the docblock was copy/paste/not-modified. :-(

+++ b/core/modules/system/lib/Drupal/system/CronController.php
@@ -22,34 +22,10 @@ class CronController {
   public function run($key) {
-    if (!$this->access($key)) {
-      throw new AccessDeniedHttpException();
-    }

We can probably even remove the $key here, since it's never used in this method.

+++ b/core/modules/system/system.routing.yml
@@ -2,3 +2,5 @@ cron:
+  requirements:
+    _type: 'cron'

This feels odd to me. What does _type mean here? Access checkers can use anything they want to determine if they should apply. I don't quite get why this approach was chosen, since _type tells me nothing. Is there nothing else we could use to trigger that checker?

klausi’s picture

Fixed documentation, removed unused key in cron controller, renamed cron route and access requirement property.

system.cron:
  pattern: '/cron/{key}'
  defaults:
    _controller: '\Drupal\system\CronController::run'
  requirements:
    _system_cron_access: 'TRUE'

I used the pattern _[module name]_[component_name]_access for the requirement key to be more descriptive.

klausi’s picture

Converted user register route access.

klausi’s picture

Priority: Critical » Major

Demoting this issue, because we have now a general critical issue for the new routing system: #1848648: [meta] Convert menu system to new routing system.

Crell’s picture

+++ b/core/modules/user/lib/Drupal/user/Access/RegisterAccessCheck.php
@@ -0,0 +1,32 @@
+class RegisterAccessCheck implements AccessCheckInterface {

If this class is being registered by UserBundle, I think it should also be in the user module rather than in core.

I think that's my only remaining issue here. :-) Let's move that class and get this in.

klausi’s picture

The RegisterAccessCheck class is in the user module? It just does not have the word user in its name.

Renamed cron access key in route to be more consistent.

fubhy’s picture

Great! I can't find anything either. For me this is RTBC, will leave it @ NR for others to take a look at it first.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I've run out of things to complain about, methinks. :-) Awesome work, team!

catch’s picture

OK I discussed this a bit with klausi in irc.

Several things make me uncomfortable here, and most of it is that access for menu links continue to be an afterthought. While we're trying to build a decoupled system where router items aren't so closely tied to menu links, we currently have no plans for (internal) menu links to be uncoupled from router items. This means that an access check for a menu link is based on access to the router item same as now and that won't change, this is what stops you from getting 403s when you click on a menu link. It's this fundamental performance issue that I brought up in #7 and the current patch attempts to deal with part of it but it's a real shame the actual access check doesn't happen here.

What is happening is that when you visit a router item directly, the new system handles that. But when you render a menu link, it's handled by the old one. Handling that is done in #1845402: Update menu link access checks for new router - and from that patch you can see we're issuing a separate query (potentially multiple ones) to find the router item attached to a menu link with a hard-coded database dependency, and that we need to create a new request object for each menu item.

There's absolutely no way that issue can be committed in that state, because it would be a very severe performance regression (unless I'm overstating the issue which is possible, but I don't think I am), and much of what contributes to it is what looks fine in isolation this issue if you forget about menu links.

So we're left with either combining the two issues (which klausi was firmly against). Or committing this (in the knowledge that it doesn't actually implement checks of router items in all the cases they're checked in core), then doing a full review of the working system including profiling in the menu_valid_item() issue - where we may or may not find out that the request object creation or router item querying is too heavy, requiring revisiting many of the assumptions here.

I'm leaning towards going ahead and doing that, then insisting on some proper performance testing in #1845402: Update menu link access checks for new router but need to mull over it a bit more. I'd be interested to see what others think in the meantime.

webchick’s picture

I'm leaning towards going ahead and [committing this patch], then insisting on some proper performance testing in #1845402: Update drupal_valid_path() for new router but need to mull over it a bit more. I'd be interested to see what others think in the meantime.

I think this is a sensible way to move forward. It helps balance the need of touching/feeling the new router system in more than hypothetical places, while still allowing us to not turn Drupal 8 into a steaming pile of performance suck.

catch’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Alright. Committed/pushed this one to 8.x. I stick by my concerns here, but let's thrash it out in #1845402: Update menu link access checks for new router. This will need the existing change notification updated.

effulgentsia’s picture

Title: Add access control mechanism for new router system » Update change notice: Add access control mechanism for new router system
Issue tags: +Needs change record

There are two change notices currently listed at the bottom of the issue summary. I'm assuming it's one or both of those that need updating.

Crell’s picture

Title: Update change notice: Add access control mechanism for new router system » Add access control mechanism for new router system
Issue tags: -Needs change record

We discussed this on the REST Team call today as well. The basic problem is that in D7, we're doing a simulated request (via menu_get_item()) for every link to access check it. In D8, the simulated request process is incrementally more expensive. Not horrifically so, but when there's 100 links on a page a small incremental cost increase gets magnified dramatically. So going from n to n+0.5 is no biggie but going from n*100 to (n = 0.5)*100 is.

We didn't come up with a solution, but generally agreed that the solution is to reduce the 100, not reduce the 0.5. Vis, get smarter about when we actually do access checks. I agree that's an issue for another thread, though. I'll try to post a change notice tonight if no one beats me to it (but feel free to beat me to it).

webchick’s picture

Title: Add access control mechanism for new router system » Change notice: Add access control mechanism for new router system
Issue tags: +Needs change record

Clarifying what's left to do here.

catch’s picture

Title: Change notice: Add access control mechanism for new router system » Add access control mechanism for new router system
Issue tags: -Needs change record

There is #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees open. That only helps in the cache hit case, but being able to skip the access checks altogether would improve things.

Crell’s picture

Title: Add access control mechanism for new router system » Change notice: Add access control mechanism for new router system
Status: Active » Reviewed & tested by the community
Issue tags: +Needs change record

Come ON, Drupal...

sun’s picture

Status: Reviewed & tested by the community » Active
Fatal error: Call to a member function setChecks() on a non-object in \core\lib\Drupal\Core\EventSubscriber\AccessSubscriber.php on line 57

Call Stack:
    0.0001     327088   1. {main}() \index.php:0
    0.0096     562640   2. Symfony\Component\HttpKernel\Kernel->handle() \index.php:35
    0.1682    2595664   3. Drupal\Core\HttpKernel->handle() \core\vendor\symfony\http-kernel\Symfony\Component\HttpKernel\Kernel.php:193
    0.1683    2596368   4. Symfony\Component\HttpKernel\HttpKernel->handle() \core\lib\Drupal\Core\HttpKernel.php:51
    0.1683    2596368   5. Symfony\Component\HttpKernel\HttpKernel->handleRaw() \core\vendor\symfony\http-kernel\Symfony\Component\HttpKernel\HttpKernel.php:73
    0.3408    3154136   6. call_user_func_array() \core\vendor\symfony\http-kernel\Symfony\Component\HttpKernel\HttpKernel.php:129
    0.3408    3154272   7. Drupal\Core\EventSubscriber\{closure}() \core\vendor\symfony\http-kernel\Symfony\Component\HttpKernel\HttpKernel.php:129
    0.3408    3154304   8. call_user_func_array() \core\lib\Drupal\Core\EventSubscriber\LegacyControllerSubscriber.php:52
    0.3408    3154440   9. admin_menu_flush_cache() \core\lib\Drupal\Core\EventSubscriber\LegacyControllerSubscriber.php:52
    0.3410    3155120  10. drupal_flush_all_caches() \sites\all\modules\admin_menu\admin_menu.inc:811
    1.4288    3661288  11. Drupal\Core\Routing\RouteBuilder->rebuild() \core\includes\common.inc:6667
    1.4735    3673224  12. Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() \core\lib\Drupal\Core\Routing\RouteBuilder.php:95
    1.4735    3674120  13. Symfony\Component\EventDispatcher\EventDispatcher->dispatch() \core\vendor\symfony\event-dispatcher\Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher.php:165
    1.4736    3674352  14. Symfony\Component\EventDispatcher\EventDispatcher->doDispatch() \core\vendor\symfony\event-dispatcher\Symfony\Component\EventDispatcher\EventDispatcher.php:53
    1.4736    3674552  15. call_user_func() \core\vendor\symfony\event-dispatcher\Symfony\Component\EventDispatcher\EventDispatcher.php:164
    1.4736    3674568  16. Drupal\Core\EventSubscriber\AccessSubscriber->onRoutingRouteAlterSetAccessCheck() \core\vendor\symfony\event-dispatcher\Symfony\Component\EventDispatcher\EventDispatcher.php:164
tim.plunkett’s picture

Status: Active » Reviewed & tested by the community

I also had that error, but it went away when I re-installed.
Also note that admin_menu_flush_cache() is in there...

sun’s picture

Can someone clarify what's RTBC here? The patch is already in HEAD...?

To clarify #152:
drupal_flush_all_caches() is not able to recover from this change, which generally means that

1) either the new implemented services are making too bold assumptions (e.g., always assuming something to exist and not catering for the case that there might be nothing),

or

2) the new services are registered / flushed / built/rebuilt at a wrong point in time.

Closely related to that is [#1847504]

ParisLiakos’s picture

Status: Reviewed & tested by the community » Active

patch committed, #151 set it RTBC i guess by accident
We need a change notice here, might need a follow up per #154?

Crell’s picture

Title: Change notice: Add access control mechanism for new router system » Add access control mechanism for new router system
Priority: Critical » Major
Status: Active » Fixed

New change notice here: http://drupal.org/node/1851490
Also updated the change notice here: http://drupal.org/node/1822626

Setting back to fixed so we can continue in follow-ups.

SebCorbin’s picture

Correct me if I'm wrong, but I fixed a non-closed comment in the notice http://drupal.org/node/1851490 (http://drupal.org/node/1851490/revisions/view/2460872/2460954)

catch’s picture

Category: task » bug
Priority: Major » Critical
Status: Fixed » Active

Where's the follow-up for #154?

klausi’s picture

Category: bug » task
Priority: Critical » Major
Status: Active » Fixed

it is in [#1847504].

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record
xjm’s picture

Issue summary: View changes

Updated issue summary. added #1807776: Support both simple and editorial workflows for translating entities postponed on this issue. as related issue