Remove 404 validation from link creation

Shouldn't this also remove this code in \Drupal\link\Plugin\Field\FieldWidget\LinkWidget::formElement()?

      // The current field value could have been entered by a different user.
      // However, if it is inaccessible to the current user, do not display it
      // to them.
      '#default_value' => (!$item->isEmpty() && (\Drupal::currentUser()->hasPermission('link to any page') || $item->getUrl()->access())) ? static::getUriAsDisplayableString($item->uri) : NULL,

Fixing the test failures.

Should be green now.

Annotated code to explain the changes in LinkFieldTest, because they'll be hard to understand otherwise:

1. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -101,36 +101,33 @@ function testURLValidation() {
   +      'non/existing/path',
   

   We now allow non-existing paths to be entered!

2. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -101,36 +101,33 @@ function testURLValidation() {
   -      // Missing protcol
   -      'not-an-url',
   

   This is no longer invalid.

3. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -101,36 +101,33 @@ function testURLValidation() {
   -      // Invalid protocol
   -      'invalid://not-a-valid-protocol',
   +      // Disallowed protocol
   +      'disallowed://not-an-allowed-protocol',
   

   This never was invalid, merely disallowed, so adjusted accordingly, so that it's no longer misleading.

4. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -101,36 +101,33 @@ function testURLValidation() {
   -    $invalid_internal_entries = array(
   -      'non/existing/path',
   -    );
   +    $invalid_internal_entries = array();
   

   We now allow any path, but this didn't cause test failures because it never was actually tested (see next bullet).

5. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -101,36 +101,33 @@ function testURLValidation() {
   -    $this->assertValidEntries($field_name, $valid_external_entries + $valid_internal_entries);
   -    $this->assertInvalidEntries($field_name, $invalid_external_entries + $invalid_internal_entries);
   +    $this->assertValidEntries($field_name, array_merge($valid_external_entries, $valid_internal_entries));
   +    $this->assertInvalidEntries($field_name, array_merge($invalid_external_entries, $invalid_internal_entries));
   ...
   -    $this->assertInvalidEntries($field_name, $valid_internal_entries + $invalid_external_entries);
   +    $this->assertInvalidEntries($field_name, array_merge($valid_internal_entries, $invalid_external_entries));
   ...
   -    $this->assertInvalidEntries($field_name, $valid_external_entries + $invalid_internal_entries);
   +    $this->assertInvalidEntries($field_name, array_merge($valid_external_entries, $invalid_internal_entries));
   

   This is a bug ever since this test coverage was introduced in #2054011: Implement built-in support for internal URLs… and yep, that means that nothing in $invalid_internal_entries was ever tested, and that the first case of $valid_internal_entries was never tested.
   These array_merge()s fix that.

6. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -634,20 +633,6 @@ function addMenuLink($parent = '', $path = '<front>', $menu_name = 'tools', $exp
   -    foreach (array('-&-', 'admin/people/permissions', '#') as $link_path) {
   

   All of these, when prefixed with user-path:, are actually valid URIs. Hence removed this test case.

Just to be clear, we certainly have to deal with the following situation:

User entered entity:node/1 and you see the node title immediately (once we have autocompletion).

I kinda think that depending on the site people might want to have the 404/403 behaviour, it is also partially a UX improvement to know that you link to a non-existing thing.

Given that we could make the access/validation level an additional setting on the field level itself.

#9: that's a superbly excellent remark. Thoughts:

• I don't think effulgentsia/pwolanin/I considered the exposing of the entity title in here, which is indeed problematic. Linking to inaccessible paths doesn't expose any information, so there's no security problem there.
• BUT! Merely allowing to enter entity: URIs that are inaccessible itself also doesn't expose the title. What you're referring to is only actually a problem when we have the entity autocomplete widget. And we could still make it so that the entity autocomplete widget doesn't autocomplete inaccessible URIs. Then there is no harm.

#10: Agreed that that would be a valuable thing to be able to configure.

Waiting for others to chime in regarding that. If we agree we want that, then we'll have to decide whether to make it a field setting or a widget setting. If it'd be a field setting, we can move the logic into LinkTypeConstraint. If it'd be a widget setting, we conditionally execute the existing logic.

There is another point: If you allow people to link to stuff they don't have access to, but also doesn't exist, they can't edit those links anymore,
as they would be hidden from the menu UI.

... removing the access checking is certainly not possible.

dawehner and I had a discussion about whether doing what this issue is currently doing — removing all 403/404 validation from the UI — has other repercussions. Turns out it does: it'd either lead to unavoidable information exposure in the menu UI, or to the user not being able to see the links he just created.

Hence I think we really need further discussion about the plan in this issue's summary.

11:44:17 dawehner: WimLeers: still
11:44:33 dawehner: WimLeers: you link to a thing which does not exist and suddenly you can't edit the menu link anymore
11:44:45 WimLeers: dawehner: why wouldn't you be able to edit the menu link anymore?
11:45:00 dawehner: WimLeers: well, because the listing of menu items would have to do access checking
11:45:15 dawehner: because otherwise you would disclose the label / path alias of the menu link
11:47:20 WimLeers: hrm that's indeed what happens. Interesting.
11:48:05 WimLeers: dawehner: I'd say that in this logic, we'd remove the access checking from the menu UI
11:48:19 WimLeers: dawehner: being able to access the menu UI means  you get to see the full list of links there
11:48:30 WimLeers: dawehner: but when rendered to you anywhere else, we do want the access checking
11:48:41 WimLeers: dawehner: being able to use the menu UI already requires elevated permissions anyway
11:48:57 WimLeers: it's kind of weird that you're only able to see a subset of the menu in that case
11:49:05 dawehner: WimLeers: in that logic you make drupal insecure!
11:49:15 WimLeers: that's what I was thinking too
11:49:16 WimLeers: but how?
11:49:23 dawehner: you are wrong
11:49:30 dawehner: administer menu is not restricted
11:49:51 WimLeers: you need a permission to access 'admin/structure/menu/manage/main'
11:49:57 dawehner: ... there are intranet sites where not every user should be able to link to anything
11:50:00 dawehner: right
11:50:15 dawehner: but this permission is different to READ ALL CONTENT TITLES
11:50:35 WimLeers: right
11:50:46 WimLeers: I'm just playing the devil's advocate here
11:51:03 WimLeers: Now we have solid reasoning why it should be a field or widget setting I think, and why we can't just remove access checking in the UI
11:51:19 WimLeers: dawehner++

So the case in question is mostly where a user links to a path?

Can we fall back to showing the path (the user input) instead of the label?

I guess the problem is that if someone else entered the path alias that contains the title, we don't want to show that either?

Can we fall back to showing the path (the user input) instead of the label?

In the widget: definitely.

In the menu tree UI: it'd only be possible if for every link that the user cannot access, we render the URL as either the empty string or the stored URI, without resolving the URI to an URL.

pwolanin in chat also thinks this should be a widget setting. Marking NW for doing that.

Actually - a field setting would make more sense if that's possible - so if I change the widget I don't lose the validation

Opened up a new critical issue, given that its a security problem to not check access.

#2419693: Move URI access validation from widget to field constraint

My 2 cents as someone who's attended multiple UX studies and seen users smashing their faces into tables trying to do what should be basic CMS 101 stuff with Drupal menus... the main use case we're trying to support with this issue is:

"As a site builder, I want to create the information architecture of my site by building the entire navigation structure out first, before populating it with content."

This is allowed by every other HTML authoring tool and CMS I've ever used—including Drupal, up until Drupal 6. So IMO, bypassing 404 checking only is fine (this is what I would expect System module's "link to any page" permission to do, instead of what it currently does around 403s), because that is the crux of the regression we introduced in D6. (If there are other use cases out there, I am not aware of them, and I'm guessing they are definitely not as important to support as this one.)

I don't follow the administrator 1, administrator 2 scenario. Don't both these admins have "Administer menus and menu items" permission? I would think most people would assume that means they should see all the links and have the ability to administer them. I don't know how to reconcile that with information disclosure of the node titles, but I will say that I don't think I've ever had a menu admin that wasn't a content admin. I consider administrating menus a higher level privilege.

@davidhernandez - I tend to agree with you in practice, and it would make the logic easier if we could just bypass access checking for those admins.

However, one use case we discussed was where there are menu admins who maintain different parts of an intranet site (different departments?) and where information should not be able to see information restricted to the other sections.

Or alternatively, a menu admin who is supposed to be updated the public/anonymous facing menu for a site, but shouldn't be able to see content titles protected by node access.

#22: indeed. What we need to really solve that is … gasp … hierarchical permissions. That's definitely D9.

#23: that almost makes the case for Menu entity access control handler (perhaps implemented as a permission generated automatically for every menu, just like what we do for node bundles.).

Why does core need to care about any of those use cases? That'd be for Fancy Menu Permissions Module in contrib.

Yes! The issue title is all I've ever wanted since 2008. :) Thanks.

here's no need to validate 404 on output, right?

Well, let's say there is no performant way.

Added some first basic change. One general question is whether a link field with type INTERNAL is meant to be internal or just relative to the Drupal installation directory?

@effulgentsia
Good points. Especially I think this is somehow what the user expects to be available.

Let's just quickly fix the failure. -&- now seems to be an allowed URI.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -94,8 +94,12 @@ public static function validateUriElement($element, FormStateInterface $form_sta
+      $disallowed = FALSE;

That's kind of a double negation - wouldn't it be clearer if we moved to $allowed = TRUE ?

That's kind of a double negation - wouldn't it be clearer if we moved to $allowed = TRUE ?

Well, we still need the full chain of arguments and then need a negation at the end for the constraint violation. But yeah, its maybe worth to try it out once.

Yes! The issue title is all I've ever wanted since 2008. :) Thanks.

ROFL!

#2419693: Move URI access validation from widget to field constraint landed! As have other patches. #32 no longer applies.

Feedback: I wonder if we want to move this into a constraint as well? I.e. move the existing logic into a constraint, but don't use it by default. But that way, it's unit tested, and anybody who wants to disallow 404s, can just enable that constraint.

Alright, let's start from scratch.

Mh, that DX is weird.

Let's see.

.

Boolean logic is hard to get right on the first time.

Alright.

This patch is looking *awesome*! :)

1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -75,7 +76,7 @@ public function validate($value, Constraint $constraint) {
   +        $this->context->addViolation($constraint->message, array('@uri' => LinkWidget::getUriAsDisplayableString($value->uri)));
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -0,0 +1,54 @@
   +        $this->context->addViolation($constraint->message, array('@uri' => LinkWidget::getUriAsDisplayableString($value->uri)));
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php
   @@ -0,0 +1,63 @@
   +          $this->context->addViolation($constraint->message, array('@uri' => LinkWidget::getUriAsDisplayableString($value->uri)));
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
   @@ -75,7 +76,7 @@ public function validate($value, Constraint $constraint) {
   +        $this->context->addViolation($this->message, array('@uri' => LinkWidget::getUriAsDisplayableString($link_item->uri)));
   

   To avoid doing this, effulgentsia and I added \Drupal\link\Plugin\Field\FieldWidget\LinkWidget::flagErrors() to replace the @uri value dynamically in the widget :)

   See commit 089a68f4a8453911df31656f04c48f9f0e410527, issue #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme.

   i.e. this should not be necessary.

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraint.php
   @@ -0,0 +1,24 @@
   + * Defines an protocol validation constraint for links to external URLs.
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraint.php
   @@ -0,0 +1,24 @@
   + * Defines an protocol validation constraint for links to broken internal URLs.
   

   s/an/a/

3. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraint.php
   @@ -0,0 +1,24 @@
   + *   label = @Translation("No harmful external protocols", context = "Validation"),
   

   What about "dangerous" instead of "harmful"?

4. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -0,0 +1,54 @@
   +      } // If the URL is malformed this constraint cannot check further.
   +      catch (\InvalidArgumentException $e) {
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php
   @@ -0,0 +1,63 @@
   +      } // If the URL is malformed this constraint cannot check further.
   +      catch (\InvalidArgumentException $e) {
   

   Hrm, do we want the comment to be on its own line, above the catch-statement?

i.e. this should not be necessary.

Ah this is cool. Thank you!

Hrm, do we want the comment to be on its own line, above the catch-statement?

Sure, let's do that.

(applies, removing needs reroll tag)

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -57,7 +57,7 @@ public static function defaultSettings() {
-  protected static function getUriAsDisplayableString($uri) {
+  public static function getUriAsDisplayableString($uri) {

This is now no longer necessary either.

Other than that, this code looks ready. But I think we want unit tests for each constraint, much like Upchuk did in #2419693: Move URI access validation from widget to field constraint for LinkAccessConstraint?

There we go.

WOW, that was fast!

Really only nitpicks, but I believe they can make a difference in long-term maintainability: it'd make the tests and therefore the code easier to grok for future readers.

After this, RTBC.

1. +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidatorTest.php
   @@ -0,0 +1,114 @@
   +    // Not valid protocols.
   

   Nit: Invalid.

2. +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidatorTest.php
   @@ -0,0 +1,114 @@
   +  public function testValidateWithInvalidUrl() {
   ...
   +    $link->expects($this->any())
   +      ->method('getUrl')
   +      ->willThrowException(new \InvalidArgumentException());
   ...
   +    $context->expects($this->never())
   +      ->method('addViolation');
   
   +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php
   @@ -0,0 +1,112 @@
   +  public function testValidateWithInvalidUrl() {
   

   This is confusing.

   The function name suggests this is an invalid URL as in "considered invalid by this constraint".

   But it's really an invalid URL as in considered invalid by the Url class, which only happens in case of a schemeless URI.

   So I think this should be named testValidateWithMalformedUri.

   It could also use an @see Url::fromUri

3. +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidatorTest.php
   @@ -0,0 +1,114 @@
   +  public function testValidateForInternalUrl() {
   

   I think a name like testValidateIgnoresInternalUrls would be a clearer name.

4. +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php
   @@ -0,0 +1,112 @@
   +    }
   +
   +
   +    $constraint = new LinkNotExistingInternalConstraint();
   ...
   +    }
   +
   +
   +    return $data;
   

   Nit: Let's remove one blank line, so we have 1, not 2.

5. +++ b/core/modules/link/tests/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php
   @@ -0,0 +1,112 @@
   +    // External URL
   +    $data[] = [Url::fromUri('https://www.drupal.org'), TRUE];
   +
   +    // Existing routed URL.
   +    $url = Url::fromRoute('example.existing_route');
   +
   +    $url_generator = $this->getMock('Drupal\Core\Routing\UrlGeneratorInterface');
   +    $url_generator->expects($this->any())
   +      ->method('generateFromRoute')
   +      ->with('example.existing_route', [], [])
   +      ->willReturn('/example/existing');
   +    $url->setUrlGenerator($url_generator);
   +
   +    $data[] = [$url, TRUE];
   +    // Not existing routed URL.
   +    $url = Url::fromRoute('example.not_existing_route');
   +
   +    $url_generator = $this->getMock('Drupal\Core\Routing\UrlGeneratorInterface');
   +    $url_generator->expects($this->any())
   +      ->method('generateFromRoute')
   +      ->with('example.not_existing_route', [], [])
   +      ->willThrowException(new RouteNotFoundException());
   +    $url->setUrlGenerator($url_generator);
   +
   +    $data[] = [$url, FALSE];
   

   Nit: inconsistent code formatting impairs legibility. Let's have a blank line before each comment at least.

Fixed the nits.

Committed 1a72848 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

This is probably a stupid question, but how does the committed patch actually reflect the issue title?

I see a LinkNotExistingInternal constraint being added to LinkItem so it seems we do validate that you cannot enter a non existing path? I also don't see tests for successfully adding a link to e.g. /wp-content/why-drupal-sucks (sorry, couldn't resist).

My 2 cents: we keep the 404 validation from link creation and leave it for a contributed module to do the job.

This was already committed awhile back, and seems to work as indicated from manual testing (which made me openly weep with happiness), so moving to fixed. If there are other things to be discussed, we should do so in follow-up issues.