Problem/Motivation

#2778345: [PP-1] Views argument validators aren't taken into account on access checks - menu links and local task point to 403 hides menu links, local tasks and other links to Views pages whose arguments respond with "Access denied" when missing or invalid, through a route access check. Arguments that respond with "Page not found" were left out, because an access check can only deny with a 403. AccessAwareRouter::checkAccess() turns any result that isn't allowed into a 403. AccessManager::checkNamedRoute() only knows a 404 through ParamNotConvertedException, and makes that result uncacheable.

The previous MR tried three ways around this:

  • Throwing CacheableNotFoundHttpException from the access check, which checkNamedRoute() then had to catch.
  • A route enhancer. checkNamedRoute() only runs param converters, so links never saw it.
  • A param converter throwing ParamNotConvertedException with cacheability. Converters run before access checks, so the view was loaded even when a permission check would deny, and a 403 became a 404. Only one converter can be set per parameter, so it could not apply to routes Views overrides. NULL parameters are never converted, so missing arguments were not caught. A converter also cannot return cacheability when validation passes.

Proposed resolution

Let a forbidden access result carry the response code, defaulting to 403:

  • AccessResultForbidden gets an optional status code. andIf() and orIf() carry it over, the way they carry the reason today.
  • AccessAwareRouter::checkAccess() throws CacheableHttpException with that code.
  • checkNamedRoute() returns a forbidden result with a 404 for ParamNotConvertedException.
  • Neutral results stay 403.
  • Links don't change: anything not allowed is already hidden.

The code travels with the access result, so its cache metadata and the render cache handle caching, and no separate cache is needed.

ValidArgumentAccessCheck can then also handle arguments that respond with "Page not found", by returning a forbidden result with a 404 and the validators' cacheability. That makes #3633769: Support 404 in AccessManager::checkNamedRoute() without killing caching unnecessary.

Remaining tasks

  • Decide which codes are allowed. Only codes decided before the controller runs fit, such as 401, 403, 404 and 410. Redirects are decided by controllers.
  • Decide precedence when checks disagree. check() stops at the first result that isn't allowed, so the order of the checks decides today.

API changes

AccessResult::forbidden() and AccessResultForbidden get an optional status code, 403 by default. Existing callers and access checks don't change.

Comments

graber created an issue. See original summary.

graber’s picture

Issue summary: View changes
kristiaanvandeneynde’s picture

Copied from Slack:

I like that your approach tries to avoid using param converters, I don’t like that we’re adding an extra purpose to a value object that is already problematic (AccessResult). It would probably get the job done, though, so that’s good. I’m just wondering if we’re not supposed to leverage Symfony more somehow. Which is what I was trying to do but then you rightfully pointed out that successful or ignored conversions don’t have the option to return cacheability.