Problem/Motivation
In #2323721: [sechole] Link field item and menu link information leakage we have re-introduced the single access check for linking in menu links, link fields and shortcuts. This issue follows up by adding the same access check to the Url
object so that links created via that object can be access checked as well. We will follow with LinkGenerator and UrlGenerator in separate issues.
Proposed resolution
- Make
AccountManager::checkNamedRoute
default to@current_user
. - Add a
access(AccountInterface $account = NULL)
method to theUrl
class. - Add
#access => $this->access()
inUrl::toRenderArray
. (Correction: to make this render cache compatible, use a new#access_callback
.) - Convert
SystemBrandingBlock
as a demonstration. Note: this is a bugfix as the current implementation does not consider that the relevant paths might have their access checks altered.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#73 | interdiff.txt | 3.09 KB | chx |
#73 | 2302563_73.patch | 31.62 KB | chx |
#69 | 2302563_69.patch | 31.83 KB | chx |
#69 | interdiff.txt | 4.62 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
dawehnerThank you for this glorious title.
There is no service which would match at the moment, given that we do have things separated now.
Does that mean that you have to check access just to load a route? This would be horrible for performance.
Comment #3
chx CreditAttribution: chx commented> Does that mean that you have to check access just to load a route?
If it gets passed to the outside of the routing system? Yes, absolutely! Why on earth would you need route objects you can't access???? I am missing something, please tell me what.
Comment #4
dawehnerRandom examples: ConfigNamesWrapper::getBaseRoute(), EntityDerivative::getDerivativeDefinitions()
People work with routes not just to display stuff.
Comment #5
catchDoesn't current route match deal with access checks already? That's the main use case for checking access - on the current route.
I can see the value in having a helper for access checks on other routes as well, but don't see why that's critical - is that really used much either in core or contrib outside of the menu system?
Comment #6
dawehnerThe CurrentRouteMatch class uses the $request, so it relies on the AccessSubscriber to be triggered during the request event flow, which we will just have to accept, otherwise the world is broken already.
Comment #7
chx CreditAttribution: chx commentedUnless of course you are a KernelEvents::REQUEST listener with a weight of 31 at which point routing happened but access check not yet. I am reasonably sure that if such a gap exists we will find other code that might run in that gap. This time I do not have the change notice to help me out with a handy exploit code (...) but the separation of routing and access checking is dangerous, no matter what. You rely on the default page flow -- that's like calling the access checker from menu_execute_active_handler instead of menu_get_item. I do not like this and it will come back with a vengeance.
I see your examples in #4; but is any of that performance sensitive? For example, aren't we caching getDerivativeDefinitions?
Comment #8
chx CreditAttribution: chx commentedComment #9
moshe weitzman CreditAttribution: moshe weitzman commentedDowngrade per https://www.drupal.org/core/issue-priority. Still a big issue but not quite release blocking.
Comment #10
chx CreditAttribution: chx commentedYou can't downgrade security regressions.
Comment #11
tim.plunkettI don't see how "call this function and do magic with the results" is that different from "call this method and then call another method".
Not getting into the status war.
Comment #12
chx CreditAttribution: chx commentedSo you can't see a difference between "you get a route from the route provider and then need to get a request, run a match and run an access check" and if ($item && $item['access']) ?
Comment #13
tim.plunkettI'm not saying it couldn't be easier, just that it's not a regression.
Comment #14
dawehnerYou want to use the system as API. In this case you want a route from a given name you would need the parameters
as well as you can't really do anything without them. IN case you have them, there is the access manager, done.
Things we can improve:
Comment #15
Crell CreditAttribution: Crell commentedI don't understand the issue here. chx or someone, can you provide a code sample (not description in a paragraph) of the offending/problematic API usage? Or whatever it is that needs to be fixed here?
"Access control is not tightly coupled to the route anymore" isn't a bug, it's a feature. As in, things not being tightly coupled anymore is the story of Drupal 8, for a reason. If the objection is other than that then I don't understand it.
I'm pretty sure this wouldn't be accepted; Route names are part of the collection, not the route itself, for better or worse. (I think it makes sense, but I do not expect the Symfony PTB to agree.)
Comment #16
jibranNR for #14
Comment #18
chx CreditAttribution: chx commentedThis is how I would do this. ->access() belongs on the route, plain and simple, every theoretical mumbo-jumbo aside.
Comment #20
chx CreditAttribution: chx commentedSo this would require moving checkNamedRoute on the route provider. Some refactor, yes.
And the use case? See, this is the difference. Drupal 6+ had a coherent vision in security that anyone talking to the menu system would have a coherent access checked item and so any link provided by the menu system or any caller copying its behavior wouldn't need to think. The point is to make it easy to do the right thing. If you want a code sample, the patch itself provides it in AccessManager::checkNamedRoute .
Edit: let me reiterate: the problem is if any code wants to provide a link to a route with certain arguments then it should be trivial to check whether that page will be accessible to the user or not.
Comment #21
dawehnerPutting access() onto the route is similar to putting access() on the node-type.
Comment #22
chx CreditAttribution: chx commentedWhy?? I must be missing something. In fact, I must be missing a lot of somethings.
Let's take
this snippet, I found it in some test. Now imagine you actually want to display a link to this page. Here's how I would imagine this:
easy as pie. Remember that snap to grid article? I modelled this on the code flow from there. And it seems to me this flows. (Note: I wanted to pass $parameters to
getRouteByName
but #2316537: Upgrade symfony CMF to 1.2, remove confusing parameters from getRouteByName* )I do not even know how would you do this currently.
Edit: to be honest,
$route->getPath($parameters);
would be ideal.Comment #23
chx CreditAttribution: chx commentedA new approach invented by dawehner: a ParameterizedRoute object. This is so pretty:
Comment #25
chx CreditAttribution: chx commenteddawehner points out this thing could possibly be on the Url object. I will rework in that direction. This is getting better and better...
Comment #26
chx CreditAttribution: chx commentedComment #27
chx CreditAttribution: chx commentedComment #28
chx CreditAttribution: chx commentedComment #29
chx CreditAttribution: chx commentedLike this. The access manager is using setter injection, I guess that's fine. Should I write a trait for it?
Comment #31
chx CreditAttribution: chx commentedLike this. The access manager is using setter injection, I guess that's fine. Should I write a trait for it?
Comment #32
chx CreditAttribution: chx commentedOK we can seriously simplify this.
Comment #34
dawehnerYou don't want to do that, believe me! this would leak "data" from the current request into that checking method.
Comment #35
chx CreditAttribution: chx commentedThis is another code smell if #34 is correct. Are you telling me that the request passed to the factory method for a RouteMatch is somehow not the request that should be used whether that particular route match is accessible to a certain account?
This whole thing is so badly over engineered it takes weeks to cut through the layers. Like, I was under the impression just based on the interfaces that RouteProvider is the primary place to get a route but turns out this is not the case, so much so that you actually never should call RouteProvider. More, apparently you should never have a route object yourself but pass the route name to various methods (some of which indicates it takes a route name, but for example UrlGenerator doesn't bother with such details) and the parameters. This, of course, is documented exactly nowhere. We will be here for a long time to fix all this.
Comment #36
dawehnerha, glad that everyone of us makes one (or more) errors while writing code.
Yes, we do currently have an implicit dependency between the action and the user module :)
Comment #37
pwolanin CreditAttribution: pwolanin commenteddiscussing #2323721: [sechole] Link field item and menu link information leakage I'm not sure this is the right direction, I'd rather move to seeing Url object/class as a more pure value object
Comment #38
chx CreditAttribution: chx commentedI have budged on that issue and we won't run access checks from the factory methods but adding these methods is quite necessary. As the ultimate goal , they should be automatic in toString / toRenderArray.
Comment #39
chx CreditAttribution: chx commentedComment #40
znerol CreditAttribution: znerol commentedWould it be possible to convert some access-checks in existing core code to the newly introduced methods? From what I gather from the patch they are currently only invoked from test code.
Comment #41
chx CreditAttribution: chx commentedConsider this typical fragment:
This should be
because it is arguably a bug that we suppose the route
system.theme_settings
is access checked byadminister themes
. Yes, that's how we ship Drupal but there's no guarantee of that.Comment #42
chx CreditAttribution: chx commentedFor now, dawehner asked to skip access check in
toString
so I am descoping that from the issue. I have merged the AccessManager changes from #2317833: SystemBrandingBlock::blockForm should checkNamedRoute instead of permissions . That issue is now a duplicate because I used it here as a demo on how this can be done.I have added a
link to any page
bypass toUrl::access
but not into RouteMatch. Discuss.I was afraid that
#access
would be incompatible with render caching, I added a simple#access_callback
property. The simplicity after is a thing of beauty: theUrl::renderAccess
is a single line of code. I wonder whether we will get any test fails from the new tightening...Comment #44
chx CreditAttribution: chx commentedComment #46
chx CreditAttribution: chx commentedYeah and account is optional, there... that's why we got so many fails.
Comment #48
chx CreditAttribution: chx commentedThat's odd somewhere the merge got lost from the other issue. I ran FileFieldAttributesTest and it passed.
Comment #49
chx CreditAttribution: chx commentedComment #51
chx CreditAttribution: chx commentedComment #53
dawehnerWe don't need the access manager in this class...
If I see it correct the url generator is no longer needed
Comment #54
chx CreditAttribution: chx commentedOh of course, #access_callback should only run when there's no #access already set.
Comment #55
chx CreditAttribution: chx commentedI wrote a test for renderAccess. Caught a random test fail, filed #2330751: Random test failures everywhere due to ->with(Request::create())
Comment #56
znerol CreditAttribution: znerol commentedI like the improved DX of Url::access(). This is also very well illustrated by the code changes in
SystemBrandingBlock
. Still I do not quite understand the changes toRouteMatch
. Can you come up with a use case where the additional methods in that class will improve the situation for core / contrib code?Comment #57
chx CreditAttribution: chx commentedI am not sure whether I can but it costs nothing to have them there. RouteMatch is very new (got added two months ago) and it will only spread for eg #2330363: Enhance the controller resolver to get a route match class . We might want to consider caching the access though because CurrentRouteMatch is already quite widespread.
Comment #58
chx CreditAttribution: chx commentedRemoved RouteMatch because only CurrentRouteMatch is used and that's access checked since the access aware router went in.
Comment #60
chx CreditAttribution: chx commentedI got slightly overeager with removing changes.
Comment #61
chx CreditAttribution: chx commentedComment #62
dawehnerDo you mind asking for a dedicated test coverage for the access callback bit?
instead of get_class() afaik we usually use static, doesn't that work here?
We could think about inlining the accessManager call
Have you considerd to use UserRole::create(...). I really like that new way. Then you can also use $role = UserRole::create(). $role->grantPermissions(); $role->save()
Comment #63
chx CreditAttribution: chx commented> Do you mind asking for a dedicated test coverage for the access callback bit?
Not the least.
> instead of get_class() afaik we usually use static, doesn't that work here?
You are right: it doesn't. __CLASS__ would but that would be Url always.
> We could think about inlining the accessManager call
We could but we are not in an object content, this is a static call so we can't call
$this->accessManager()
. If you mean that instead of ->access() I should copy the whole body of access() , I do not think so.> I really like that new way
Yes, that's very pretty.
Comment #64
chx CreditAttribution: chx commentedComment #65
dawehnerIs it not possible at all to just use
\Drupal::accessManager()->access(...)
?oh php!
Comment #66
chx CreditAttribution: chx commented> Is it not possible at all to just use \Drupal::accessManager()->access(...)?
We discussed on IRC and since there is no accessManager the resulting code would look uglier so -- yes possible, but no we aren't doing it.
Comment #67
tim.plunkettgetControllerFromDefinition supports much more than just class::method. I think you can just always call this and assign it to $callable, right?
Lowercase o in optional
Do we need this or is it just for tests?
Comment #68
dawehner@tim.plunkett
Can you write down what you figured out in IRC?
Comment #69
chx CreditAttribution: chx commented> getControllerFromDefinition supports much more than just class::method. I think you can just always call this and assign it to $callable, right?
Nope. But that's not in scope for this issue as this code is present in drupal_render twice already. Please file a followup for better doxygen. When I tried to summarize it, I failed. This might need a helper method with a longish doxygen.
> Lowercase o in optional
Done; also fixed relevant doxygen at another spot.
> Do we need this or is it just for tests?
Moved it into a TestUrl class.
Comment #70
tim.plunkettYeah, #67.1 is the fault of #2247779: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods.
Note that in #2247779-32: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods @catch called this out as something needing to be documented.
#2323301: Deprecate the "two double colons" check in Renderer::doCallback() was opened to ostensibly address that, but it's still not documenting it, just trying to "fix" it.
Since that is a pre-existing weirdness, RTBC for the rest.
Comment #71
chx CreditAttribution: chx commentedThe gift that keeps giving: #2333265: Add access checking to link render structures adds the access callback introduced in this issue to a number of places and shows that we are displaying an ungodly amount of links which might lead to pages the users can't access.
Comment #72
alexpottProperty is not documented.
The doc block needs a prune :)
This needs a comment - also how do we end up with this? User depends on system which provides the action entity afaik. Or is this for Kernel tests - and if so then we need to enable the system module in my opinion.
Not used.
Let's document the return type.
Comment #73
chx CreditAttribution: chx commented3. I simply removed; let's see whether it fails. The rest is addressed.
Comment #74
dawehnerYeah the fix for actions was wrong, but I know that there was a plugin not found exception at some point. Maybe this got fixed in the meantime by properly clearing some cache on module install.
Comment #75
chx CreditAttribution: chx commentedProbably #2330121: Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema
Comment #76
alexpottCommitted 6115051 and pushed to 8.0.x. Thanks!