Move URI access validation from widget to field constraint

Will take a stab at it.

D

Here's a patch, no tests yet (not sure if any are needed).

One question: the error message of the `LinkTypeConstraint` plugin fits perfectly well with the error message for the new Access constraint so I copied it from there. Should we not adjust it in `LinkTypeConstraint` to better reflect the validation error? Not sure why this: or you do not have access to it is in it :)

D

But, if we're gonna hide them in the Menu UI, we should also prevent you from making them, since otherwise, you can create a link that you then can't find or edit.

That's a UX goal rather than a security one. And core's only link widget already provides this desired UX. So this isn't fixing a bug or addressing security. But, it is a good code move that will make it easier for contrib widgets and REST services to have the desired UX by default, so recategorizing as a Major task.

One of the reasons of the fail in the interdiff :)

Starting the bot for the other ones because I noticed something weird when I was doing partial local testing. While it was testing validation of links, at some point it was using the url input element instead of textfield with an internal URI. According to the widget this shouldn't happen...

D

Alright, found the problem. When I stripped the widget of the validation method, I also removed its call from the `#element_validate` render key. This means that the default Url validation kicked in before the constrain and that provides a different error message. I replaced it with an empty array so the constraint is used instead.

re #8:

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -97,6 +97,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#element_validate' => array(),

Would be worth a comment then :-)

Also, minor : [] rather than array() ?

@dawehner Will do. My next step was to try and see how I can create the entities through the API to test the constraint.

@yched Totally right. About the array, I was wondering myself :)

D

Yeah, there is a problem with the validation using the API..

The problem is the following: when creating entities using the link field and through a form widget, there is a value massage method that calls the getUserEnteredStringAsUrimethod to transform a schemaless URI into the default user-path schema. The problem with that is that when you create a simple entity like so:

$link = entity_create('menu_link_content', array(
    'title' => 'bla',
    'link' => 'your-alias'
  ));

...the massaging doesn't happen and the Url::fromUri method throws an exception. And this means that entities using the link field and which do not actively implement some sort of hook to transform schemaless URI's into the default schema, will be broken for creation through the API.

Any thoughts on where best to implement this value massaging?

D

I actually saw inside a menu_link_content test this entity_create usage:

$options = array(
      'menu_name' => 'menu_test',
      'bundle' => 'menu_link_content',
      'link' => [['uri' => 'user-path:<front>']],
    );
$link = entity_create('menu_link_content', $options);

So yeah, obviously you can prefix internal paths such as aliases with the schema. But I;m not sure it's a good thing. Why are we asking the "API user" to provide more information than the "form user"? And not to mention the dependency it puts on knowing whether the schema is necessary or not. Is there no way we can make this happen somewhere on entity_create level?

Something like this maybe? :)

function link_entity_presave(Drupal\Core\Entity\EntityInterface $entity) {
  $definitions = $entity->getFieldDefinitions();
  if (!empty($definitions)) {
    foreach ($definitions as $definition) {
      if ($definition->getType() == 'link') {
        $field_name = $definition->getName();
        $uri = $entity->{$field_name}->uri;
        if (parse_url($uri, PHP_URL_SCHEME) === NULL) {
          $entity->{$field_name}->uri = 'user-path:' . $uri;
        }
      }
    }
  }
}

This way we can also remove this from the form.

This is 100% legit. When you work on an entity at the api level, you work with raw values, the same you would read from an $entity loaded from db.

Then widgets provide UX, that doesnt affect the shape of the values when working with entities in code.

Alrighty.

Here is the updated test to include also API level validation.

Don't mind the patches in the previous comment, there are some obvious testing mistakes in it :)

These should be better. The interdiff is from #8.

Yeah, turns out that #2417647: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route added a slash to the user-path scheme and I hadn't pulled since then :) Let's see if it works now.

Interdiff still from #8;

Hey there,

Counting the failure is tricky because I noticed a different number of fails for the different invalid URIs.

if I create an entity with an invalid URI in the link field I get 3 violations (2 from the 2 constraints of the Link module and one from the PrimitiveTypeConstraint). In the test however, sometimes it's only one, sometimes all 3...

I corrected the expectation message to use String::format() in the meantime. What do you suggest about the first issue? Is it important to know the number of constraints?

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -162,7 +136,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      '#element_validate' => array(array(get_called_class(), 'validateUriElement')),
   +      '#element_validate' => [],
   

   Let's instead just remove #element_validate altogether.

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
   @@ -0,0 +1,73 @@
   + *   label = @Translation("Link uri can be accessed by the user.", context = "Validation"),
   

   Either s/uri/URI/, or just s/uri//.

3. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
   @@ -0,0 +1,73 @@
   +      $link_item = $value;
   ...
   +        $url = $link_item->getUrl();
   

   Why not delete the first line and do $value->getUrl() on the second line?

4. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
   @@ -0,0 +1,73 @@
   +      catch (\InvalidArgumentException $e) {
   

   What exactly can throw that exception? We didn't have that before.

5. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -169,7 +175,30 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
   +      $message = String::format('The path \'@uri\' is either invalid or you do not have access to it.', array('@uri' => $this->getStringAsUri($invalid_value)));
   

   You could also have used double quotes, then there is no need for the escaping. But, that doesn't really matter.

6. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -169,7 +175,30 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
   +   *   The potentially scheme-less URI
   ...
   +   *   A URI with the default scheme if none exists
   

   Missing trailing period.

7. +++ b/core/modules/link/src/Tests/LinkFieldTest.php
   @@ -169,7 +175,30 @@ protected function assertInvalidEntries($field_name, array $invalid_entries) {
   +   * @return string
   

   Should have a blank line before it.

But actually, I don't think we want any of the changes to LinkFieldTest, because that is an integration test. I think we want a unit test similar to \Drupal\Tests\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidatorTest.

As discussed with @Wim Leers, the following actions will be taken:

1. Leave as it is but add comment to reflect the need to keep #element_validate as an empty array
2, 3. Make the corrections
4. Exception being thrown by the Url::fromUri method so try..catch needs to stay
5,6,7. Make the corrections

Additionally, will need to create a unit test for LinkAccessConstraint which also means injecting the current user into the constraint class in order to mock it.

Another additionally, as discussed with @dawehner, will need to change the error message of LinkTypeConstraint to better reflect what it does. Integration tests for this constraint will follow in another issue.

Just triggering the bot to see if anything fails..

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -162,7 +136,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#element_validate' => array(),
   

   Let's change it to [].

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
   @@ -0,0 +1,25 @@
   +class LinkAccessConstraint extends Constraint {
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -0,0 +1,84 @@
   +class LinkAccessConstraintValidator implements ConstraintValidatorInterface, ContainerInjectionInterface {
   

   Why split this up in separate classes? What's the gain?

Hey @Wim,

The validator had to be split from the constraint in order for the injection to work. The constraint is a plugin and gets built with the ContainerFactoryPluginInterface and the validator can implement the ContainerInjectionInterface. If they are both in one class, then they will get built twice with 2 different create() methods.

D

I see :) Funky restriction, but seems legit. Thanks for the explanation!

Alright, starting from scratch basically with

• a LinkAccessConstraint and separate validator class that focus only on the access aspect. Rest remains inside the LinkWidget for now
• minor change to the message of the LinkTypeConstraint to better reflect what actually it does

Unit tests are coming as well.

Trying again with more consolidated non-access related error messages.

Another test, hopefully this should pass :)

Here we go again. This time there is a unit test for the LinkAccessConstrainValidator.

If test is green, the whole patch can be reviewed manually.

+++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
@@ -0,0 +1,82 @@
+ * @group validation2

Left 'validation2' by accident. Will fix it after manual review if the automated test passes.

Looking close! :)

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -180,7 +177,7 @@ public static function validateUriElement($element, FormStateInterface $form_sta
   -        $form_state->setError($element, t("The path '@link_path' is either invalid or you do not have access to it.", ['@link_path' => static::getUriAsDisplayableString($uri)]));
   +        $form_state->setError($element, t("The path '@link_path' is invalid.", ['@link_path' => static::getUriAsDisplayableString($uri)]));
   
   +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
   @@ -0,0 +1,25 @@
   +  public $message = "The path '@uri' is either invalid or you do not have access to it.";
   

   I'm not sure if we want to change the messaging here, because it amounts to a subtle information disclosure.

   I think we'll have to keep the messages the same.

   @dawehner, @effulgentsia, please confirm.

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -0,0 +1,78 @@
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   +use Drupal\Core\Session\AccountProxyInterface;
   

   Nit: let's swap these two, then all "use" statements are ordered alphabetically.

3. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -0,0 +1,78 @@
   +   * @param AccountProxyInterface $current_user
   

   Needs fully qualified name.

4. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -0,0 +1,78 @@
   +   * {@inheritDoc}
   

   Capital D.

5. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
   @@ -0,0 +1,78 @@
   +  }
   +}
   

   Nit: blank line between last method's closing brace and the class' closing brace.

6. +++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
   @@ -0,0 +1,82 @@
   +  public function testValidate($value, $user, $valid) {
   +
   +    $context = $this->getMock('Symfony\Component\Validator\ExecutionContextInterface');
   

   Nit: No blank line here.

7. +++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
   @@ -0,0 +1,82 @@
   +      ['user_admin' => true, 'url_admin' => true, 'valid' => true],
   +      ['user_admin' => true, 'url_admin' => false, 'valid' => true],
   +      ['user_admin' => false, 'url_admin' => true, 'valid' => true],
   +      ['user_admin' => false, 'url_admin' => false, 'valid' => false],
   

   s/true/TRUE/
   s/false/FALSE/

8. +++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
   @@ -0,0 +1,82 @@
   +      // Mock a URL object that returns a boolean indicating user access.
   ...
   +      // Mock a link object that returns the URL object.
   

   s/URL/Url/, because we're referring to a specific classname.

9. +++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
   @@ -0,0 +1,82 @@
   +      // Mock a user object that returns a boolean indicating user access to all
   +      // links.
   +      $user = $this->getMock('Drupal\Core\Session\AccountProxyInterface');
   +      $user->expects($this->any())
   +        ->method('hasPermission')
   

   Let's add a ->with() line to clarify which permission this is mocking.

10. +++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
    @@ -0,0 +1,82 @@
    +        ->willReturn($case['user_admin']);
    

    I think "user_admin" may be a bit confusing, what about "may_link_any_page", or something like that?

Hey Wim, thanks for the review.

Will make all the changes.

Regarding the first one (changing the message): I changed the messaging because it doesn't reflect reality (doesn't do any sort of access check) and that exact message fits perfectly with the new LinkAccessConstraint. Additionally, since integration testing is not possible by checking the type of constraint that added a violation, we have to rely on the message it throws (still don't know why that is though :) ). This is yet another reason why the two messages need to be separate. You'll notice I also changed it in the LinkTypeConstraint. This I discussed already with @dawehner, maybe you remember :)

D

Did you discuss the information disclosure aspect with dawehner? I don't remember, sorry. (Too many things happening at the same time in the whole menu links/URI set of issues.)

In case someone wants to change that and set 404==403 its already required to write some code, so this would be just additional effort.

Ah, right, that was our conclusion.

Then please ignore #40.1! :)

Here we go. Hope it still passes :)

Let me know if you spot any other issues.

Looks perfect.

Alright, here it is, a small error creeped in that was caught in the meantime by #2418237: Fix incorrect @covers in PHPUnit tests I guess.

Back to RTBC.

+++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
@@ -0,0 +1,82 @@
+      ['may_link_any_page' => TRUE, 'url_access' => false, 'valid' => TRUE],

A lowercase false creeped in here, can be fixed on commit though.

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraint.php
@@ -0,0 +1,25 @@
+  public $message = "The path '@uri' is either invalid or you do not have access to it.";

I think this message should be The path '@uri' is inaccessible.'. We already have the minor information disclosure.

Here we go. Hope it still passes :)

Here we go again :)

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
@@ -0,0 +1,79 @@
+      $allowed = TRUE;
+      try {
+        $url = $value->getUrl();
+        // Disallow URLs if the current user doesn't have the 'link to any page'
+        // permission nor can access this URI.
+        $allowed = $this->current_user->hasPermission('link to any page') || $url->access();
+      }
+      catch (\InvalidArgumentException $e) {
+        $allowed = FALSE;
+      }

I think this constraint should have no opinion if the url is not valid. So something like this:

  /**
   * {@inheritdoc}
   */
  public function validate($value, Constraint $constraint) {
    if (isset($value)) {
      try {
        $url = $value->getUrl();
      }
       // If the URL is malformed this constraint can not check access.
      catch (\InvalidArgumentException $e) {
        return;
      }

      // Disallow URLs if the current user doesn't have the 'link to any page'
      // permission nor can access this URI.
      $allowed = $this->current_user->hasPermission('link to any page') || $url->access();
      if (!$allowed) {
        $this->context->addViolation($constraint->message, array('@uri' => $value->uri));
      }
    }
  }

+++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
@@ -0,0 +1,82 @@
+
+  public function provideTestValidate() {

There is consensus that this methods are called provider and have a doc block that points to the method(s) they provide data for.

Hey there,

Made the changes. Additionally, I also added some docblock info to the test class and method.

+++ b/core/modules/link/tests/src/LinkAccessConstraintValidatorTest.php
@@ -0,0 +1,102 @@
+   * @dataProvider provideTestValidate

This was the problem. Fixed now by changing to providerValidate (to match the changed method name)

Here you go.

Here we go @YesCT.

Here we are.

looks like the concerns have been addressed. (and some nits we discussed in irc)

Alex Pott has been all up in this issue's bizness, so assigning to him.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9d4d62d and pushed to 8.0.x. Thanks!

Upchuk++ — thanks! :)