Access to 'Reset to alphabetical' denied for users without administer permission

+1

Real issue, proposed solutions seams reasonable, would love to see this implemented.

Closed #3002803: Unable to reset to alphabetical without administer content permission as a duplicate issue.

Attached patch implements a 'reset all weights' operation on the \Drupal\taxonomy\VocabularyAccessControlHandler::checkAccess() method and applies it to the Term overview form.

Fixed Drupal code style.

This works for me. Good job!

This works for me also. Thank you!

Patch applied without issue to 8.9.x and fixes the issue, do 3 reviews over 9 months count as reviewed?

Also as I believe this is a non-disruptive bug fix, this can be applied to all active versions.

Rerolled the patch for 9.1. Please review.

updated patch with fixed undefined variable error and coding standard errors.
please review.

I have reviewed the attached #22patch, Here are the results:
* Patch applied cleanly without any issues.
* After applying the patch,
saw some more coding standard errors in updated files

drupal_core % vendor/squizlabs/php_codesniffer/bin/phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php

FILE: /sample/drupal_core/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
--------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------
 9 | ERROR | [x] Missing class doc comment
--------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------
Time: 166ms; Memory: 8MB

drupal_core % vendor/squizlabs/php_codesniffer/bin/phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml core/modules/taxonomy/src/Form/OverviewTerms.php

FILE: /sample/drupal_core/core/modules/taxonomy/src/Form/OverviewTerms.php
------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------
 535 | WARNING | Line exceeds 80 characters; contains 90 characters
 540 | WARNING | Line exceeds 80 characters; contains 96 characters
------------------------------------------------------------------------------------------------

Time: 123ms; Memory: 12MB

drupal_core % vendor/squizlabs/php_codesniffer/bin/phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php

FILE: /sample/drupal_core/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------
 26 | ERROR   | [x] Missing function doc comment
 39 | WARNING | [x] There must be no blank line following an inline comment
------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------

Time: 97ms; Memory: 10MB

updated patch with fixed above coding standard errors.
please review.

Patch #23 is working for me.

@meghasharma, Welcome to Drupal! Thank you for running phpcs and checking the coding standards. However, this issue isn't about fixing coding standards in the modified files and having those changes in the patch become noise and make it harder to review the patch.

1. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -6,6 +6,9 @@
   +/**
   + * HTML routes for entities.
   + */
   

   Out of scope.

2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -536,12 +532,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -        // Give terms at the root level a weight in sequence with terms on previous pages.
   +        // Give terms at the root level a weight in sequence with terms
   +        // on previous pages.
   ...
   -        // Terms not at the root level can safely start from 0 because they're all on this page.
   +        // Terms not at the root level can safely start from 0 because they're
   +        // all on this page.
   

   Also out of scope

3. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -23,6 +23,9 @@ class VocabularyPermissionsTest extends TaxonomyTestBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Out of scope

4. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -37,7 +40,6 @@ protected function setUp(): void {
   -
   

   Out of scope

+++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
@@ -142,6 +143,8 @@ public function testTaxonomyVocabularyOverviewPermissions() {
+    $this->drupalPostForm(NULL, [], 'Reset to alphabetical');
+    $assert_session->statusCodeEquals(200);

So here the the the list gets reset to alphabetical. That proves the button is there but how do we know that the data has actually been reordered? Where is the test for that?

The actual reordering is tested here: https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/modules/taxo...

Removed the out of scope changes mentioned in #26

The rerolls since #11 have muddled up the logic somewhat, likely because of underlying changes to the form that git/patch couldn't sort out by itself. Here's what I can see at the moment:

1. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -314,7 +312,7 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   -    if ($create_access->isAllowed()) {
   +    if (!empty($pending_term_ids) || $vocabulary_hierarchy === VocabularyInterface::HIERARCHY_MULTIPLE) {
   

   This is the wrong check to replace with this logic. It's determining whether the "add term" link should be shown so it should be checking create access.

2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -326,13 +324,13 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   +        'weight' => empty($pending_term_ids) && $vocabulary_hierarchy !== VocabularyInterface::HIERARCHY_MULTIPLE ? $this->t('Weight') : NULL,
   

   This is the second place this logic exists (it's functionally the same as line 316). Turn this into a variable if the need to use it mutliple times remains.

3. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -326,13 +324,13 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   -    $this->renderer->addCacheableDependency($form['terms'], $create_access);
   

   This will need to be reinstated because of point 1.

Updating the comments based on feedback in #32
Test cases are passing on local as well.

Attached patch replaces the call to the deprecation drupalPostForm() with submitForm()

Patch looks good to me.

It is looking good :) however:

1. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -442,7 +441,6 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   -    $this->renderer->addCacheableDependency($form['terms'], $update_tree_access);
   

   Why has this been moved? This looks like an unrelated change.

2. +++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
   @@ -23,6 +24,33 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +        $pending_term_ids = $storage_controller->getTermIdsWithPendingRevisions();
   +        if (!empty($pending_term_ids)) {
   +          return AccessResult::forbidden()->setReason("$capital_name contains 1 or more terms with multiple parents. Drag and drop of terms with multiple parents is not supported, but you can re-enable drag-and-drop support by editing each term to include only a single parent.");
   +        }
   

   Error message needs to reflect the reason for failure.

#40.1 Returned the line to its original location. Having the cache dependencies added on subsequent lines was helping readability for me, but there is a merit to the smallest change possible I suppose

#40.2 Copied the message from \Drupal\taxonomy\Form\OverviewTerms::buildForm() so it accurately reflects the reason for failure.

Looks good, thank you.

Re #40.1, the idea is to give core committers less to figure out when reading the patch. I've seen so many issues knocked back with "unrelated change" :)

I wonder if they'll insist on tests for the error messages? That's a grey area for me, so as far as I'm concerned this can go back to RTBC.

+++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
@@ -23,6 +24,33 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+
+        $tree = $storage_controller->loadTree($entity->id(), 0, NULL, TRUE);
+        $access_result = AccessResult::allowed();
+        foreach ($tree as $term) {
+          $access_result = $access_result->andIf($term->access('update', $account, TRUE));
+        }
+

I don't think it's reasonable to load the entire taxonomy tree, which can contain potentially hundreds of thousands of terms, to check access here. Why not make the button access rely on the administer taxonomy permission instead?

@dpi here is a patch as per your comment.

Regardless, I think we shouldnt need to do this in the access check either. Checking for the administer permission is enough

I removed the hunk of code related to $tree = $storage_controller->loadTree($entity->id(), 0, NULL, TRUE); and added a simple permission check.

I also dont fully understand why this is in access control. We dont need to determine whether *it is possible* to reorder. We only need to check whether the user should have *permission* to reorder. This responsibility should remain with \Drupal\taxonomy\Form\OverviewTerms

I also removed these checks as \Drupal\taxonomy\Form\OverviewTerms already takes care of them. (line 308).

Random test fail.

I'm going to reproduce in my enviroment and check if it's working as planned to be

Tested as Admin and another created user(without administer taxonomy), and it worked as intented ,reseting to alphabetical order.

Thanks @Johnny Santos for manually testing this!

In addition to manual testing, the patch also needs code review to be marked "Reviewed and tested by the community". It looks like the test hit a known random fail, so I am requeuing it.

I check the tests about this case. I don't find any cause about the random fail, but I have added new checks about different context.
Please confirm that I correctly understood that;
The user only can "Reset to alphabetical" WHEN
- The user has the permission "administer taxonomy", OR
- The user has the permission "edit terms in ..."
I have generated new patch with the new checks in the test.

The files uploaded in my last comment (#52) are wrong. I did not properly generated the patches, for this reason they did not pass the test.

I have regenerated them again. Sorry for the confusion, this is one of my first contributions to the community.

This time, I had problems with a blank line during static analysis. Sorry again.

I think this is the final version of the patch files.

@Ipeidro, welcome to the Drupal Community! We're happy to have you helping.
It's perfectly normal to make mistakes sometimes, none of us is free from sending a wrong patch from time to time.

Just some tips for you:

You can easily install yarn on your environment and run ./core/scripts/dev/commit-code-check.sh to run the custom commands check before sending the patch (more info).
You can also use the phpcs commands (see here) to check for coding standards on your code.
This way you know beforehand if your patch will pass the initial checks, and save time of having to wait the test results here.

Another good practice to follow is to always provide an interdiff file when sending a patch based on a previous one, so that way other developers can more easily see what you changed on the code between the patches. (see here).

Thanks for the patch,
I have applied it cleanly and it worked perfectly according to the condition.
Now the user can Reset to alphabetical if they have "administer taxonomy " or edit term permissions.

+++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
@@ -23,6 +23,16 @@ class VocabularyAccessControlHandler extends EntityAccessControlHandler {
+        return AccessResult::allowedIfHasPermissions($account, [
+          'access taxonomy overview',
+          'edit terms in ' . $entity->id(),
+        ]);

Not convinced this is the correct permission to be applying here. Access to the taxonomy overview doesn't automatically imply the right to edit the arrangement of terms.

I think it should be if they have either "edit terms in [vocabulary-name]" or "administer taxonomy" (which is handled in the lines prior to these ones).

Made changes as per comment #60, please review.

Thanks Ravi, the "edit terms in" line is correct in #55. It's the 'access taxonomy overview' I was questioning, though I was tired so I didn't really see that it was tempered by the presence of the next line already.

Having had more time to think about it I think #55 can be called RTBC. The code looks good, test coverage is good and it's passing, I don't see any outstanding feedback, and it does what the IS says it should.

Reuploading #55 because of the 2 day test cycle.

Random fail, back to RTBC

Queued a 10.0.x test-run

+++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
@@ -23,6 +23,16 @@ class VocabularyAccessControlHandler extends EntityAccessControlHandler {
+        if ($account->hasPermission('administer taxonomy')) {
+          return AccessResult::allowed()->cachePerPermissions();
+        }
+
+        return AccessResult::allowedIfHasPermissions($account, [
+          'access taxonomy overview',
+          'edit terms in ' . $entity->id(),
+        ]);

We could simplify this to

return AccessResult::allowedIfHasPermission($account, 'administer taxonomy')->orIf(AccessResult::allowedIfHasPermissions($account, ['access taxonomy overview', 'edit terms in ' $entity->id()]));

But I don't feel strongly about it.

Let's see what the 10.0.x result comes back (although HEAD is broken atm due to container/ci changes)

starting a retest on D10.

I agree with #67. Applied.

Oops. I ran the auto-formatter in PhpStorm but I must have had it badly highlighted.

+++ b/core/modules/taxonomy/src/VocabularyAccessControlHandler.php
@@ -23,6 +23,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
 
+      case 'reset all weights':
+        return AccessResult::allowedIfHasPermission($account, 'administer taxonomy')
+          ->orIf(AccessResult::allowedIfHasPermissions($account, [
+            'access taxonomy overview',
+            'edit terms in ' . $entity->id(),
+          ]));
+

I'm not sure the check for access overview is needed at all? What's the reason for limiting it to that?

We could just an OR check for admin or edit all permission as a single check?

I will review this

I think the question in question is valid.
I believe it should be analyzed better!

#73 fair point, I agree. New patch attached.

Patch #77 Tested and Applied successfully on Drupal 9.5.x.

Applied patch #77 successfully on Drupal 9.5.x. and work fine .Refer to the screenshot.
Thank you

Patch in #77 applies and addresses point raised in #73. Moving back to RTBC.

Applied patch #77 successfully on Drupal 9.4.3. and work fine.
Thank you!

Test fail looks unrelated... moving back to NR for new tests.

Tests passed so back to RTBC

Test failures are unrelated. Couldn't find if tracked elsewhere, asked on https://drupal.slack.com/archives/C1BMUQ9U6/p1676468080110019.

Did my best with issue credit, but this is a long issue with lots of participants so apologies for any oversights.

This looks good now - since it's adding a new entity access case, going to commit to 10.1.x only.

As described in https://www.drupal.org/project/drupal/issues/3403785, the committed solution does not hide the drag'n'drop icons nor the Show row weights link if there is a term with multiple parents. Therefore, I'm attaching a patch that fixes this for 9.5.x.

Overlooked one line to revert.