JsonApiResource\Link objects with inaccessible target urls should not be normalized

So this would help jsonapi_hypermedia, see #3079664: Move cacheability bubbling complexity from LinkProviderManager to the overriding normalizer. Tagging Contributed project soft blocker.

I think this is basically what this issue is proposing.

Two questions:

1. I don't see how to make Link actually respect the access result?
2. Given point 1, why can't the caller of the code just add the access result's cacheability to the first argument of the Link constructor (which is CacheableMetadata $cacheability).

?

The only solution for #8.1 that I see is this.

Re-roll of patch #9 against latest 9.1.x branch.

1. +++ b/core/modules/jsonapi/src/Normalizer/LinkCollectionNormalizer.php
   @@ -75,7 +77,11 @@ public function normalize($object, $format = NULL, array $context = []) {
   +        $access = $link->getUri()->access(NULL, TRUE);
   

   🤓 I'd rather explicitly inject the current user rather than relying on the NULL-falls-back-to-current-user-in-request-context behavior.

   Core went with with this implicit magic in many places because it couldn't break backwards compatibility, but we can be explicit within JSON:API.

2. +++ b/core/modules/jsonapi/src/Normalizer/LinkCollectionNormalizer.php
   @@ -75,7 +77,11 @@ public function normalize($object, $format = NULL, array $context = []) {
   +        $normalized[$link_key] = $access->isAllowed()
   +          ? new CacheableNormalization($cacheability, $normalization)
   +          : new CacheableOmission($cacheability);
   

   I think that this is good, but I'd love to see the concrete impact on the normalized output.

   When we add the necessary test coverage, that will become obvious of course! 👍

   I think this means inaccessible links will simply be omitted, because we don't have anything collecting omittedd links. But … I don't remember the JSON:API implementation details well enough to know that for sure.

+++ b/core/lib/Drupal/Core/Url.php
@@ -809,15 +810,22 @@ public function getInternalPath() {
+   * @return bool|\Drupal\Core\Access\AccessResultInterface
+   *   The access result. Returns a boolean if $return_as_object is FALSE (this
+   *   is the default) and otherwise an AccessResultInterface object.
+   *   When a boolean is returned, the result of AccessInterface::isAllowed() is
+   *   returned, i.e. TRUE means access is explicitly allowed, FALSE means
+   *   access is either explicitly forbidden or "no opinion".

This bit in particular is something that is IMHO an oversight/omission in the current state of affairs — this pattern exists everywhere else except here!

+++ b/core/modules/jsonapi/src/Normalizer/LinkCollectionNormalizer.php
@@ -77,9 +77,18 @@
+        // Checking access on links is not about access to the link itself;
+        // it is about whether the current user has access to the route that is
+        // *targeted* by the link. This is done on a "best effort" basis. That
+        // is, some links target routes that depend on a request to determine if
+        // they're accessible or not. Some other links might target routes to
+        // which the current user will clearly not have access, in that case
+        // this code proactively removes those links from the response.

😍🙏

Great comment!

Seems like a security improvement. How are we supposed to get a red/green run with MRs?

#25: wow, that commit is indeed rather scary and requires the highest level of scrutiny possible!

I was going to suggest the same that @larowlan already suggested a week ago: the risk in this behavior change seems to be too great. A new method with this specific behavior expectation baked in seems a safer option.

But … all that has become irrelevant thanks to @gabesullice pairing with @tim.plunkett apparently 😄🥳

#31 shows how much simpler and more local the changes became — now instead of a high-risk targeted change in infrastructure affecting every single request processed by Drupal, it became a change only affecting the JSON:API module, and instead of being complex it became a trivial refactoring!

When this reaches RTBC — and I think it is very close — then I think a git diff -M5% of RelationshipRouteAccessCheck would help see every reviewer how little this issue is actually changing 🤓👍

Aside of that "present" instead of "absent" typo, I think this is RTBC now!

RTBC++

I probably would have fixed the two nits on commit but the bit about replacing a typehint with an assert needs more discussion. I can't see why that's necessary.

@gabesullice the ? is unnecessary... you could already pass NULL to access.... see https://3v4l.org/spMXd - I'm removing it because it is an unnecessary change. If you want to use the ? in new code fine... but then do ?AccountInterface $account - the = NULL is redundant.

Crediting @larowlan and @jibran for code review.

Committed adf684d and pushed to 9.2.x. Thanks!

diff --git a/core/lib/Drupal/Core/Url.php b/core/lib/Drupal/Core/Url.php
index d59203f666..776a8f778a 100644
--- a/core/lib/Drupal/Core/Url.php
+++ b/core/lib/Drupal/Core/Url.php
@@ -819,7 +819,7 @@ public function getInternalPath() {
    *   returned, i.e. TRUE means access is explicitly allowed, FALSE means
    *   access is either explicitly forbidden or "no opinion".
    */
-  public function access(?AccountInterface $account = NULL, $return_as_object = FALSE) {
+  public function access(AccountInterface $account = NULL, $return_as_object = FALSE) {
     if ($this->isRouted()) {
       return $this->accessManager()->checkNamedRoute($this->getRouteName(), $this->getRouteParameters(), $account, $return_as_object);
     }

Fixed on commit.