AssignOwnerNode allows assigning to anonymous users due to misspelling of key #selection_settings

This was a good finding! The fact that all test are fine after that change and obviously have been before, let me assume that this part has a lack of test coverage.

This Patch #7 Works as expected !!

I also can confirm, that the patch in #7 works as expected. In my opinion this fixes a trivial security issue as an anonymous user can be selected, even it was not intended by an administrator. The attached test adds coverage that was missing before. Together with the feedback #10 i think its ok to set this issue to RTBC.

1. +++ b/core/modules/node/src/Tests/NodeActionsConfigurationTest.php
   @@ -81,4 +81,39 @@ public function testAssignOwnerNodeActionConfiguration() {
   +  public function testAssignOwnerNodeActionAnonymous() {
   

   Nice to see the new test coverage being added.

2. +++ b/core/modules/node/src/Tests/NodeActionsConfigurationTest.php
   @@ -81,4 +81,39 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // We don't want to receive the anonymous user here.
   

   We're only testing the negative case. As we're adding new test coverage of the autocomplete field I think we should at least add positive test coverage that it works too.

3. +++ b/core/modules/node/src/Tests/NodeActionsConfigurationTest.php
   @@ -81,4 +81,39 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    $expected = array();
   +    $this->assertIdentical($expected, $data);
   

   Let's just change this to:
   $this->assertIdentical([], $data);
   There's no point making someone wonder what $expected equals.

4. Can someone check to see if an issue exists for or file a followup to convert this class to use entity query instead of directly querying the database?

+++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
@@ -80,4 +82,37 @@ public function testAssignOwnerNodeActionConfiguration() {
+   * Tests if autocomplete field does not show anonymous user.

I don't think this new test adds any value, it is basically re-testing something that is already covered by \Drupal\Tests\system\Functional\Entity\EntityReferenceSelection\EntityReferenceSelectionAccessTest::testUserHandler().

The only thing that it's preventing is not allowing future patches to reintroduce the typo in Drupal\node\Plugin\Action\AssignOwnerNode, but chances of that happening are pretty close to 0 :)

he makes sure that the search is working (finds not anonymous users)

This is the part that is already tested in \Drupal\Tests\system\Functional\Entity\EntityReferenceSelection\EntityReferenceSelectionAccessTest::testUserHandler() :)

Closed #3065171: Code mistake un AssignOwnerNode.php file as a duplicate issue.

Closed https://www.drupal.org/project/drupal/issues/3094840#comment-13357098 as a duplicate issue.

By the way, I think it is a little bit frightening to see that a simple typo (e.g. just an extra "t" in selection_settings) has not been solved since the issue was opened 3 years ago.

Could we consider fixing the typo and maybe open a another issue for the missing tests explained in #26 and #27 ?

Rerolled the patch based on #27, meanwhile, removed the typo from core/misc/cspell/dictionary.txt. No interdiff/raw-interdiff as the patch size is very small.

Applying IS template

Basically looks great, thanks! A bunch of mostly trivial nits and 1 point of real substance at the end:

1. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +   * Tests if autocomplete field does not show anonymous user.
   

   "Tests that the autocomplete field does not show the anonymous user."

2. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // Create 200 Users, to force the action's configuration page to show up
   +    // an autocomplete field instead of a select field.
   

   "Create 200 users to force the action's configuration page to use an autocomplete field instead of a select field."

3. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // See \Drupal\node\Plugin\Action\AssignOwnerNode::buildConfigurationForm().
   

   s/See/@see/ ? Although then it'd be 81 chars wide. :( Maybe just leave it like this. ;)

4. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    for ($i = 0; $i < 200; ++$i) {
   

   This works, but we don't depend on incrementing $i before evaluating it here. Core basically always uses $i++ so we should stick with that for consistency (unless we actually needed the ++$i behavior for something).

5. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // Ensure that anonymous user exists.
   

   s/that/that the/

6. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // not be able to reference the anonymous user anyway.
   

   s/ anyway//

7. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    // Get the autocomplete url of the owner_uid textfield.
   

   s/url/URL/

8. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    $autocomplete_field = $this->xpath('//input[@name="owner_uid"]')[0];
   

   Would this be cleaner with $this->getSession()->getPage()->findField('owner_uid') ?

   I think the xpath() is clear enough, but some folks prefer to use the specific helpers instead of xpath(). /shrug

9. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,32 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    $this->assertEmpty($data, 'Anonymous users are not included in the results.');
   

   Since it's a new assertion, let's not add the message argument. We're trying to get rid of those. ;)

10. #12.2 is not addressed. The test should also make sure autocomplete works like it's supposed to. ;)

Thanks!
-Derek

@dww, many thanks for reviewing!

Addressing #39 and #12.2 by adding the following lines.

+++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
@@ -85,4 +87,38 @@ public function testAssignOwnerNodeActionConfiguration() {
+    // Make sure that autocomplete works.
+    $user = $this->drupalCreateUser();
+    $data = Json::decode($this->drupalGet($autocomplete_url, ['query' => ['q' => $user->getAccountName(), '_format' => 'json']]));
+    $this->assertNotEmpty($data);

s/See/@see/ ? Although then it'd be 81 chars wide. :( Maybe just leave it like this. ;)

Re #39.3, instead of s/See/@see/, appended/moved See to the end of the previous line.

Thanks, that was fast!

Interdiff would have been helpful...

Almost there:

1. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,38 @@ public function testAssignOwnerNodeActionConfiguration() {
   +   * Tests that the autocomplete field does not show the anonymous user.
   ...
   +  public function testAssignOwnerNodeActionAnonymous() {
   ...
   +    // Make sure that autocomplete works.
   

   Whoops, the function name no longer matches the scope of what's tested.

   Maybe:

   * Tests the autocomplete field when configuring the AssignOwnerNode action.
   testAssignOwnerNodeActionAutocomplete()

   ?

2. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,38 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    $data = Json::decode($this->drupalGet($autocomplete_url, ['query' => ['q' => 'Anonymous', '_format' => 'json']]));
   

   Now that we have the assertions above, this could use:

   // Make sure the autocomplete does not show the anonymous user.

Thanks!
-Derek

I have made an interdiff, but forgot to attach, attaching first. sorry.

Addressing #41

+++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
@@ -85,4 +87,39 @@ public function testAssignOwnerNodeActionConfiguration() {
+    $data = Json::decode($this->drupalGet($autocomplete_url, ['query' => ['q' => 'Anonymous', '_format' => 'json']]));

Instead of using hardcoded account name "Anonymous", I'd like passing it the account name got from the system.

// $anonymous = User::getAnonymousUser();
'q' => $anonymous->getAccountName()

1. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,40 @@ public function testAssignOwnerNodeActionConfiguration() {
   +   */
   +  public function testAssignOwnerNodeActionAutocomplete() {
   +    // Create 200 users to force the action's configuration page to use an
   +    // autocomplete field instead of a select field. See
   +    // \Drupal\node\Plugin\Action\AssignOwnerNode::buildConfigurationForm().
   +    for ($i = 0; $i < 200; $i++) {
   +      $this->drupalCreateUser();
   +    }
   +
   

   ouch, this is weird. The code is also super ugly and does raw database queries against the user table. I'm seriously wondering if we should create a follow-up and kill that conditionial and just always an autocomplete? Other places don't do this either, the node edit form is always an autocomplete, so why bother with this?

   Then in that follow-up we could just remove this part.

2. +++ b/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php
   @@ -85,4 +87,40 @@ public function testAssignOwnerNodeActionConfiguration() {
   +    $this->assertNotNull($anonymous);
   +    // Make sure the autocomplete does not show the anonymous user.
   +    $data = Json::decode($this->drupalGet($autocomplete_url, ['query' => ['q' => $anonymous->getAccountName(), '_format' => 'json']]));
   +    $this->assertEmpty($data);
   

   anonymous doesn't have an account name, it only has a display name.

   So this is searching for ''.

Re: #45.1 (see also #12.4): Yup, that's a good idea.

#3163934: Remove raw DB queries in AssignOwnerNode::buildConfigurationForm() and always use autocomplete

Re: #45.2: looks like we should go back to patch #43 then.

NeedsFollowup ;)

Cheers,
-Derek

anonymous doesn't have an account name, it only has a display name.

@Berdir, you're right, thanks! calling getDisplayName() instead.

Note: unsurprisingly, some folks prefer the current behavior. ;) See #3094840: Allow assigning Anonymous owner in AssignOwnerNode action I don't think we should do anything about that here, but I wanted to raise visibility for that.

Expected fail

You need to upload the test-only first, then the full patch, or the bot gets confused. You'll probably want to re-upload #47 so the last patch in here is the real / passing one.

Test failure is as expected:

1) Drupal\Tests\node\Functional\NodeActionsConfigurationTest::testAssignOwnerNodeActionAutocomplete
Failed asserting that an array is empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:118
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/var/www/html/vendor/phpunit/phpunit/src/Framework/Assert.php:827
/var/www/html/core/modules/node/tests/src/Functional/NodeActionsConfigurationTest.php:122

No CS violations.

Other than the follow-ups, I have nothing else to complain about. ;)

Once the latest patch is the keeper, this is RTBC.

Thanks!
-Derek

Re-uploading the patch in #47, thanks again, @dww!

Yup, that's the RTBC patch we need, +1.

Thanks!
-Derek

Giving this a more specific name for a more useful commit message.

Un-assigning megadesk3000 since they haven't worked on this since 2017.

Fixing the summary. Not sure we need a release note snippet for this, but adding one.

Actually, set these as remaining tasks:

1. Decide if #3094840: Allow assigning Anonymous owner in AssignOwnerNode action is worth worrying about before fixing this.
2. Decide if we need a CR for this change.
3. Commit.

I think only a committer can really decide those, so leaving RTBC.

Cheers,
-Derek

Also adding a Follow-ups section.

I've stared at this one a bit and I wonder if we should "do" #3094840: Allow assigning Anonymous owner in AssignOwnerNode action and forget about the original intention. Like that is how it is working already today. We allow nodes to be assigned to anonymous users as part of the user cancel process already.... ah I have answer for why not.

This is only the case on sites with more that 200 users. On sites with less you can't assign to the anonymous user because we do a select and dropdown list instead where we have

SELECT uid, name FROM {users_field_data} WHERE uid > 0 

Also given that I don;t think we need a CR for this change. We've got all the follow-ups to address whether we want keep this behaviour and to move away from directly querying tables.

Committed c3152e5 and pushed to 9.1.x. Thanks!

Given this doesn't apply to 9.0.x without extra work and maybe someone will disagree about the lack of CR or the direction going to commit to 9.1.x only for now and see what, if anything, happens.

Thought about that too, as there's also #3163934: Remove raw DB queries in AssignOwnerNode::buildConfigurationForm() and always use autocomplete, which removes those queries completely and maybe then we can include the anonymous part there? Assumig we can agree on that, but I think the argument is the same as the one I made for removing the select-mode. It's the same on the node edit form, it also allows to select anonymous, so why should this be different?