Problem/Motivation

The "Administer vocabularies and terms" permission gives you permissions to create taxonomy terms, see the vocabulary and taxonomy term listing and create and edit the vocabularies.

Usually you want editor users to be able to create taxonomy terms but you don't want them to add additional fields, create new/delete vocabularies etcetera.

Proposed resolution

There have been a number of related/duplicate issue as this has been requested by many users over the years. Since many of those did not move forward for a long time, this is an attempt to combine multiple issues/patches together into a complete, consistent solution that addresses several related aspects, see the summary of changes and related issues below.

Main changes:
1. Add an "access taxonomy overview" permission that allows to to access the vocabulary and term overviews. For the per-vocabulary term overview, this is exposed/checked through a new custom entity access operation. This allows custom/contrib code to limit access per vocabulary.
2. Add per-vocabulary create term permissions, just like we already have edit and delete permissions.

Necessary/related "secondary" changes in the patch:
1. The term overview form needs to check for create/edit/delete operation link access. It also needs to make the drag and drop feature only available if the user has update access to all terms.
2. The vocabulary list builder empty message also needs to check for create access for vocabularies
3. forum.module currently has a single administer taxonomy permission and expected to also see an edit operation, because the parent overview form of its custom overview form now checks for edit permission of the edit link. Form changes the URL of the link anyway, so this patch simplifies that now by completely replacing the operations column as it also currently also removes the delete operation. As a possible follow-up, it might be possible to completely refactor the forum UI and integrate it back into the standard taxonomy UI as its access handling is now much more flexible and has a better UI in general.
4. Improves REST permission test coverage for specific create/update/delete access.
5. Uses the entity operations API to display edit/delete/... operations, which takes care of the necessary access logic for point 1 as well as allowing to alter/add additional operations.
6. Adds a custom term list builder to handle the previously provided destination URL on operations. This can be removed again if/once #2767857: Add destination to edit, delete, enable, disable links in entity list builders lands.
7. Adjust taxonomy help to consider the different permissions.

(Yes, many things)

#1038330: Allow specific vocabulary permissions to work on vocabulary admin pages:
Very old patch that adds add permission. Cosed as a duplicate of this.

#2469567: Entity operations for terms are hardcoded in taxonomy terms's overview page:
Adds permission checks to operations on overview form, adds a term list builder to do that and also adds destination link there. There is also a separate issue to always add the destination in the list builder. That approach was

#2650898: ListBuilders do not check $entity->access() for operation links:
Adds some access checks to operations, including a view check for vocabulary for viewing the term overview page, so it conflicts with this. That part can then be removed for that issue.

#2599128: Allow user with edit terms permission to access vocabulary overview:
Grants access to overview page through term vocabulary access controller if user either has edit or delete permission. Eventually that issue ended up with the same approach as this, a separate access overview permission. David_Rothstein there suggested an access overview per vocabulary permission, which would be more flexible but this now combines a single permission with an entity operation for flexibility. It also provides test coverage that was the starting point for the extensive test coverage now added here.

Does have one interesting fix for forum by allowing term edit access with the administer forums permission. This was originally included here but undone since forum actually has its own edit form and that would allow more access than is currently possible in HEAD.

#2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission:
Main change is to allow view vocabulary access with the access content permission like other similar REST related issues. That results in the problem with the overview route, which it also solves by using the administer taxonomy permission directly. Overlaps heavily with this issue and conflicts with the approach in the issue above.

@dawehner suggested a custom entity operation to wrap the permission check. That's quite interesting as it allows to customize it through entity access hooks. This is now done in this issue.

#2658956: Taxonomy vocabulary data not available as views fields:
Did the same change as the issue above, was committed and reverted because it made the taxonomy overview page public due to that.

#2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission:
Proposes what should actually already be possible for edit/delete, this does that while also adding the check for CREATE. If this lands, it can also be closed as a dpulicate.

#2687775: OverviewTerms should not list operations for which the current user does not have the access permission:
Another duplicate of the missing operation access on term overview, also closed as duplicate.

Remaining tasks

User interface changes

API changes

Data model changes

The "Administer vocabularies and terms" permission gives you permissions to create taxonomy terms, see the vocabulary and taxonomy term listing and create and edit the vocabularies.

Usually you want editor users to be able to create taxonomy terms but you don't want them to add additional fields, create new vocabularies etcetera.

So I propose to split the permissions into:

  • Administer vocabularies
  • Access the taxonomy overview page
  • Add terms in vocabulary name
  • Edit terms in vocabulary name (already implemented)
  • Delete terms in vocabulary name (already implemented)

In the patch that I'm going to add in a few minutes (after simpletests completed successfully on my local machine) I've added these changes.

I also fixed the "operations" lists that currently show you operations that you might not have permissions for.

This is my first attempt on drupal 8 so please check my code for places where I don't use the API properly.

related:
#340652: Edit/delete terms permission per vocabulary
Taxonomy Access Fix

CommentFileSizeAuthor
#216 terms_terms_editor.png10.96 KBBerdir
#216 terms_terms_admin.png22.66 KBBerdir
#216 terms_empty_editor.png12.39 KBBerdir
#216 terms_empty_admin.png22.12 KBBerdir
#216 vocabulary_empty_editor.png16.48 KBBerdir
#216 vocabulary_empty_admin.png20.46 KBBerdir
#205 split_administer-8.5-1848686-205-interdiff.txt627 bytesBerdir
#205 split_administer-8.5-1848686-205.patch40.02 KBBerdir
#203 split_administer-8.5-1848686-203.patch39.41 KBBerdir
#200 split_administer-8.5-1848686-200-interdiff.patch2.89 KBBerdir
#200 split_administer-8.5-1848686-200.patch130.47 KBBerdir
#198 split_administer-8.4-1848686-198-interdiff.txt4.68 KBBerdir
#198 split_administer-8.4-1848686-198.patch42.29 KBBerdir
#196 split_administer-8.4-1848686-196-interdiff.txt3.02 KBBerdir
#196 split_administer-8.4-1848686-196.patch42.27 KBBerdir
#192 split_administer-8.4-1848686-192-interdiff.txt1.81 KBBerdir
#192 split_administer-8.4-1848686-192.patch41.73 KBBerdir
#189 split_administer-8.4-1848686-189-interdiff.txt13.11 KBBerdir
#189 split_administer-8.4-1848686-189.patch41.35 KBBerdir
#186 split_administer-8.4-1848686-184-interdiff.txt22.08 KBBerdir
#186 split_administer-8.4-1848686-184.patch39.97 KBBerdir
#181 split_administer-8.4-1848686-181-interdiff.txt2.71 KBBerdir
#181 split_administer-8.4-1848686-181.patch33.43 KBBerdir
#180 split_administer-8.4-1848686-180-interdiff.txt19.93 KBBerdir
#180 split_administer-8.4-1848686-180.patch33.39 KBBerdir
#172 split_administer-8.4-1848686-172.patch27.67 KBgeertvd
#172 interdiff-1848686-170-172.txt857 bytesgeertvd
#124 split_administer-1848686-124.patch26.35 KBhauruck
#124 interdiff-1848686-122-124.txt3.7 KBhauruck
#2 taxonomy-split-permissions-1848686-2.patch10.01 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 48,525 pass(es), 117 fail(s), and 58 exception(s). View
#10 taxonomy-split-permissions-1848686-10.patch11.39 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 48,737 pass(es), 22 fail(s), and 1 exception(s). View
#11 taxonomy-split-permissions-1848686-11.patch10.75 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 48,772 pass(es), 22 fail(s), and 1 exception(s). View
#13 taxonomy-split-permissions-1848686-13.patch14.05 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 48,782 pass(es), 1 fail(s), and 0 exception(s). View
#18 taxonomy-split-permissions-1848686-18.patch13.78 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 48,813 pass(es). View
#26 taxonomy-split-permissions-1848686-25.patch10.46 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 49,206 pass(es), 3 fail(s), and 0 exception(s). View
#29 taxonomy-split-permissions-1848686-29.patch11.24 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 49,257 pass(es). View
#39 taxonomy-split-permissions-1848686-39.patch19.51 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 49,544 pass(es). View
#43 taxonomy-split-permissions-1848686-43.patch19.48 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 50,648 pass(es). View
#44 taxonomy-split-permissions-1848686-44.patch18.69 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 50,861 pass(es). View
#51 drupal-split_taxonomy_permissions-1848686-51.patch22.46 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-split_taxonomy_permissions-1848686-51.patch. Unable to apply patch. See the log in the details link for more information. View
#53 drupal-split_taxonomy_permissions-1848686-53.patch16.95 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 53,159 pass(es), 34 fail(s), and 3 exception(s). View
#56 drupal-split_taxonomy_permissions-1848686-55.patch18.57 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 53,455 pass(es). View
#57 drupal-split_taxonomy_permissions-1848686-57.patch22.51 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 53,458 pass(es). View
#58 drupal-split_taxonomy_permissions-1848686-58.patch24.88 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 53,443 pass(es). View
#62 drupal-split_taxonomy_permissions-1848686-62-do-not-test.patch22.62 KBJohnny vd Laar
#62 drupal-split_taxonomy_permissions-1848686-62.patch25.19 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 54,229 pass(es). View
#64 drupal-split_taxonomy_permissions-1848686-64.patch18.87 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 55,639 pass(es). View
#67 drupal-split_taxonomy_permissions-1848686-67.patch18.6 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 58,726 pass(es), 2 fail(s), and 1 exception(s). View
#71 drupal-split_taxonomy_permissions-1848686-71.patch23.95 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 58,629 pass(es), 2 fail(s), and 4 exception(s). View
#74 drupal-split_taxonomy_permissions-1848686-74.patch23.95 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 58,612 pass(es), 2 fail(s), and 4 exception(s). View
#76 drupal-split_taxonomy_permissions-1848686-76.patch23.95 KBJohnny vd Laar
FAILED: [[SimpleTest]]: [MySQL] 58,666 pass(es), 2 fail(s), and 4 exception(s). View
#78 drupal-split_taxonomy_permissions-1848686-77.patch24.83 KBJohnny vd Laar
PASSED: [[SimpleTest]]: [MySQL] 59,093 pass(es). View
#89 drupal-split_taxonomy_permissions-1848686-89.patch24.21 KBcyborg_572
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch drupal-split_taxonomy_permissions-1848686-89.patch. Unable to apply patch. See the log in the details link for more information. View
#92 1848686-91.patch16.53 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/taxonomy/src/Form/OverviewTerms.php. View
#95 1848686-95.patch18.42 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 86,047 pass(es), 129 fail(s), and 122 exception(s). View
#95 interdiff.patch3.15 KBpfrenssen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff_44.patch. Unable to apply patch. See the log in the details link for more information. View
#99 split_administer-1848686-99.patch21.07 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,096 pass(es), 3 fail(s), and 2 exception(s). View
#99 interdiff.txt6.92 KBJeroenT
#101 interdiff-99-101.txt419 bytesJeroenT
#101 split_administer-1848686-101.patch21.66 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch split_administer-1848686-101.patch. Unable to apply patch. See the log in the details link for more information. View
#105 split_administer-1848686-105.patch21.62 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,169 pass(es), 4 fail(s), and 1 exception(s). View
#107 split_administer-1848686-107.patch21.67 KBzniki.ru
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,355 pass(es), 6 fail(s), and 2 exception(s). View
#110 split_administer-1848686-110.patch25.38 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch split_administer-1848686-110.patch. Unable to apply patch. See the log in the details link for more information. View
#112 split_administer-1848686-112.patch25.38 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,456 pass(es), 3 fail(s), and 0 exception(s). View
#114 interdiff-1848686-112-114.txt1.73 KBJeroenT
#114 split_administer-1848686-114.patch24.97 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,548 pass(es). View
#118 split_administer-1848686-118.patch24.94 KBhauruck
#120 split_administer-1848686-118.patch24.94 KBwebflo
#122 interdiff-1848686-118-122.txt4.24 KBhauruck
#122 split_administer-1848686-122.patch25.83 KBhauruck
#135 split_administer-1848686-135.patch26.34 KBManuel Garcia
#136 split_administer-1848686-136.patch25.32 KBBambell
#136 interdiff-135-136.txt2.86 KBBambell
#136 taxonomy-create-permission.png13.72 KBBambell
#136 taxonomy-terms-overview-access.png13.75 KBBambell
#136 taxonomy-terms-overview-1-element.png22.78 KBBambell
#139 split_administer-1848686-139.patch26.92 KBBambell
#139 interdiff-136-139.txt2.17 KBBambell
#139 taxonomy-terms-overview-no-drag.png5.9 KBBambell
#143 split_administer-1848686-143.patch26.65 KBManuel Garcia
#145 split_administer-1848686-145.patch26.91 KBAlgeron
#152 split_administer-1848686-152.patch25.69 KBrealityloop
#153 split_administer-1848686-153.patch26.7 KBrealityloop
#161 split_administer-1848686-161.patch26.5 KBManuel Garcia
#163 split_administer-1848686-165.patch25.91 KBmogtofu33
#166 interdiff-1848686-163-166.txt1.52 KBgeertvd
#166 split_administer-1848686-166.patch27.43 KBgeertvd
#168 split_administer-1848686-168.patch27.41 KBmogtofu33
#169 split_administer-8.3-1848686-169.patch27.32 KBmogtofu33
#169 split_administer-8.4-1848686-169.patch27.31 KBmogtofu33
#170 split_administer-8.4-1848686-170.patch27.65 KBgeertvd
#170 split_administer-8.3-1848686-170.patch27.39 KBgeertvd
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Johnny vd Laar’s picture

These issues might be duplicate / fixed by the patch:
http://drupal.org/node/1196002
http://drupal.org/node/1839604

Johnny vd Laar’s picture

Status: Active » Needs review
FileSize
10.01 KB
FAILED: [[SimpleTest]]: [MySQL] 48,525 pass(es), 117 fail(s), and 58 exception(s). View

Ok lets see if the testbot agrees with this.

ParisLiakos’s picture

This will also deceprate http://drupal.org/project/taxonomy_access_fix right?

Johnny vd Laar’s picture

Indeed this functionality would be included in core then.

Status: Needs review » Needs work

The last submitted patch, taxonomy-split-permissions-1848686-2.patch, failed testing.

Johnny vd Laar’s picture

Ai, I will look into this when I'm at work on monday.

lpalgarvio’s picture

Issue summary: View changes

edit/delete and related links

lpalgarvio’s picture

and keep edit/delete terms by vocabulary permission as per
#340652: Edit/delete terms permission per vocabulary
and
#1839604: Add taxonomy term permission

lpalgarvio’s picture

Title: Split "Administer vocabularies and terms" permission in multiple permissions » Split "Administer vocabularies and terms" permission: Access the taxonomy overview page and Add terms in vocabulary name
Issue tags: +Security improvements, +Usability, +taxonomy permissions
Johnny vd Laar’s picture

If I understand you correctly then you say that the free tagging vocabulary should also check on the new "add term" permission?

If so what would be the response that the system should give if someone tries to make a new term via the free tagging vocabulary? Should there be an error raised during validation of the form?

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
11.39 KB
FAILED: [[SimpleTest]]: [MySQL] 48,737 pass(es), 22 fail(s), and 1 exception(s). View

Attached patch should fix the failing tests and it adds a validation check for taxonomy terms added in free tagging taxonomy fields.

Johnny vd Laar’s picture

FileSize
10.75 KB
FAILED: [[SimpleTest]]: [MySQL] 48,772 pass(es), 22 fail(s), and 1 exception(s). View

Fixed a small bug with the operations drop down.

Status: Needs review » Needs work

The last submitted patch, taxonomy-split-permissions-1848686-11.patch, failed testing.

Johnny vd Laar’s picture

FileSize
14.05 KB
FAILED: [[SimpleTest]]: [MySQL] 48,782 pass(es), 1 fail(s), and 0 exception(s). View

Okay, a new attempt with the new failed tests.

Johnny vd Laar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, taxonomy-split-permissions-1848686-13.patch, failed testing.

lpalgarvio’s picture

the whole motivation behind taxonomy access fix module was to mainly provide Add terms per vocabulary permission.
permit users to Add terms to a given vocabulary whom they are allowed to, as set by the admin, without being able to do that to all vocabularies or administer them.

Edit terms and Delete terms are the complements to Add, respectively for editing and deleting terms - these are already implemented in core and are just noted for reference.

as for Access taxonomy overview page permission, its a secondary goal - important, but not as crucial as Add terms in my opinion

i'm invioning permissions this way:
- Administer vocabularies and terms
- Access taxonomy overview page
- Add terms in (...)
- Edit terms in (...)
- Delete terms (...)

Johnny vd Laar’s picture

I think that is the functionality that the patch currently provides, right?

I will look into the last failed test tomorrow. But I'm not really sure why it has failed.

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "plugin.manager.field.widget" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:690

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
13.78 KB
PASSED: [[SimpleTest]]: [MySQL] 48,813 pass(es). View

Ok this should fix the latest failing test. I'm not so happy that the setUp now logs in twice but I don't think it would be good to take this out of the parent class.

Johnny vd Laar’s picture

Now the test passed, is there anything left for me to do to get this accepted? Or just wait and hope?

danbohea’s picture

If the patch in #18 is accepted into D8 core will it be added to D7 core as well or will that require a separate issue?

Johnny vd Laar’s picture

I don't think this will be put into D7 core as it introduces new permissions and changes existing permissions and will break existing websites because of that.

danbohea’s picture

OK, sure. For anyone looking for a D7 solution, check out http://drupal.org/node/1038330#comment-6528288 for a core patch and http://drupal.org/project/taxonomy_access_fix for a module. I opted for the latter and it seems to work fine.

ParisLiakos’s picture

Isn't #1038330: Allow specific vocabulary permissions to work on vocabulary admin pages a duplicate?

@#19: Someone will review the patch sooner or later..when we all think it is ready we will RTBC it and then a core maintainer will commit it.

I willl try to review it , but no promises till weekend

Johnny vd Laar’s picture

That issue is partially related because this issues fixes the situation where you need administer vocabularies and term permissions to see the taxonomy overview.

I see a small flaw in my patch. If you have administer taxonomy permission then I think you should also have access to the taxonomy overview. Does anyone have an opinion on this?

lpalgarvio’s picture

yes, it should imply that =)
good work!

Johnny vd Laar’s picture

Issue tags: -Usability
FileSize
10.46 KB
FAILED: [[SimpleTest]]: [MySQL] 49,206 pass(es), 3 fail(s), and 0 exception(s). View

Modified the patch to better work with the administer taxonomy permission patch. This also makes the tests a bit better and makes the patch smaller.

Johnny vd Laar’s picture

Issue tags: +Usability

hey why did it remove the tag.

Status: Needs review » Needs work

The last submitted patch, taxonomy-split-permissions-1848686-25.patch, failed testing.

Johnny vd Laar’s picture

FileSize
11.24 KB
PASSED: [[SimpleTest]]: [MySQL] 49,257 pass(es). View

Hmm strange that breakpoint test failing... Here a fix for the rdfa failed test.

Lets hope that the breakpoint bug was a glitch in the matrix ;)

Johnny vd Laar’s picture

Status: Needs work » Needs review
Johnny vd Laar’s picture

I think this change is better, can't think of anything else to improve. Is anyone able to review this?

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This looks awesome!

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -87,11 +87,22 @@ function taxonomy_help($path, $arg) {
+      'create terms in ' . $vocabulary->vid => array(
+        'title' => t('Add terms in %vocabulary', array('%vocabulary' => $vocabulary->name)),
+      ),
+    );

Hmm why dont we use the vocabulary machine name here?
Eg what node does is:
Create %type content
What dont we do sth similar
Create %vocabulary_machine_name terms.
Unless this is the pattern we already use in taxonomy, so just ignore this

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -369,25 +381,40 @@ function taxonomy_admin_paths() {
-function taxonomy_term_access($op, $term) {
-  if (!$term || !in_array($op, array('edit', 'delete'), TRUE)) {
-    // If there was no term to check against, or the $op was not one of the
+function taxonomy_term_access($op, $obj) {
+  if (!$obj || !in_array($op, array('create', 'edit', 'delete'), TRUE)) {
+    // If there was no object to check against, or the $op was not one of the
     // supported ones, we return access denied.
     return FALSE;
   }
+  return user_access("$op terms in $obj->vid") || user_access('administer taxonomy');
+}

Not a big fan of $obj.
Why dont we keep this argument as $term and initialize it with NULL?
I dont see any special condition for create op anyways

Finally this is gonna need some tests for new permissions i think

Johnny vd Laar’s picture

I think there is a seperate issue for using machine names in the permissions.

I'm not really sure about what you mean with your second statement. Because it needs to check on the create permission for this vocabulary so I can't initialize it with NULL?

The extra tests, which one would you suggest?
* Check whether you cannot create terms without the create terms permission.
* Check whether you cannot access the taxonomy overview without the tax overview permission?

ParisLiakos’s picture

ah..i see..sorry i misread your docblock, so nevermind this.
Anyways $obj is not a good name, should be $object..Dunno if we could find a better name for DX.

About tests:
Exactly...and maybe also the opposite, if you could and have time

Edit: about permission names, i found this:
#995156: Use vocabulary machine name for permissions which was closed for this #1552396: Convert vocabularies into configuration

Johnny vd Laar’s picture

I will rename to object and add extra tests.

rschwab’s picture

ParisLiakos’s picture

Johnny vd Laar’s picture

Oh that means that I have to check some stuff too now :D

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
19.51 KB
PASSED: [[SimpleTest]]: [MySQL] 49,544 pass(es). View

I've added some tests and I fixed the patch with the new machine name based permissions.

Johnny vd Laar’s picture

Can someone check whether I created the tests according to Drupal standards? I am not really familiar with how these test functions are supposed to be named.

dawehner’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->admin_user = $this->drupalCreateUser(array('administer taxonomy'));
...
+    $this->vocabulary = $this->createVocabulary();

If you store something on the object you should probably document it.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertText(t('Vocabulary name'), 'Vocabulary overview opened successfully.');

You could check assertResponse(200); to check whether the user got access or not.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+  function testVocabularyPermissionsTaxonomyOverviewUserFail() {

You could do this in the previous test function already, no need for a full new drupal installation to test that :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertNoText(t('Vocabulary name'), 'Vocabulary overview opened successfully.');

Here you can then check assertResponse(404);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+  function testVocabularyPermissionsTaxonomyOverviewUserSucceed() {
...
+  function testVocabularyPermissionsTaxonomyTermAdmin() {

similar to above. In general I think it's helpful to keep the documentation you have written

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->drupalGet('taxonomy/term/' . $term->tid . '/edit');

A small possible simplification, you could use $tterm->uri()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertText($edit['name'], 'Edit taxonomy term form opened successfully.');

Yeah another place to check access directly.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -370,25 +382,48 @@ function taxonomy_admin_paths() {
+function taxonomy_term_access($op, $object) {

This feels wrong, why not have two differnt access functions, one for terms one for vocabularies?

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/TaggedWithTest.phpundefined
@@ -38,6 +38,8 @@ public static function getInfo() {
+    $this->permissions[] = 'administer taxonomy';

What about moving it after the parent setup? This is just convenience.

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/TaggedWithTest.phpundefined
@@ -38,6 +38,8 @@ public static function getInfo() {
+    ¶

whitespace error ...

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/WizardTestBase.phpundefined
@@ -13,6 +13,8 @@
+  // Permissions for this user to test with.
+  protected $permissions = array();

Please use proper documentation of this property see drupal.org/node/1354

Johnny vd Laar’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -370,25 +382,48 @@ function taxonomy_admin_paths() {
+function taxonomy_term_access($op, $object) {

This feels wrong, why not have two differnt access functions, one for terms one for vocabularies?

I agree but how should I name the function then as it'll be the create access check for creating a term.

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/TaggedWithTest.phpundefined
@@ -38,6 +38,8 @@ public static function getInfo() {
+    $this->permissions[] = 'administer taxonomy';

What about moving it after the parent setup? This is just convenience.

This is not possible as the parent actually sets the permission.

Johnny vd Laar’s picture

FileSize
19.48 KB
PASSED: [[SimpleTest]]: [MySQL] 50,648 pass(es). View

Modified the tests a bit based on the comments of dawehner.

Johnny vd Laar’s picture

FileSize
18.69 KB
PASSED: [[SimpleTest]]: [MySQL] 50,861 pass(es). View

Now with a new split function:
function taxonomy_term_access($op, $term) {

and

function taxonomy_term_create_access($vocabulary) {

Johnny vd Laar’s picture

I guess I've fixed all comments people made now. Can someone please check this?

ParisLiakos’s picture

Also, related: #1862758: [Followup] Implement entity access API for terms and vocabularies
I guess this will get committed sooner than this one, so i am giving the heads up

Johnny vd Laar’s picture

I guess that is a much better solution so that sounds good.

Johnny vd Laar’s picture

Should we close this issue now as won't fix?

dawehner’s picture

Well I think having new permissions certainly would help, but yeah this patch has to be rewritten quite a lot.

ParisLiakos’s picture

the other patch is rtbc
once it gets committed i will roll a patch on top of it

ParisLiakos’s picture

Status: Needs review » Needs work
FileSize
22.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-split_taxonomy_permissions-1848686-51.patch. Unable to apply patch. See the log in the details link for more information. View

the problem as i see it with the current approach is that a user with access taxonomy overview can change the weight of the vocabularies and tags..reorder them and reparent them, even though he might not have permissions to edit them.
Also current tests, fail to catch this, which is a security isssue.

i tried to fix this in top of #1862758: [Followup] Implement entity access API for terms and vocabularies I eventually run out of time and patience. i attach the patch, for anyone that wants to keep it up

Johnny vd Laar’s picture

When http://drupal.org/node/1862758 gets in I will look into it. I don't have much time to do this at the moment.

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
16.95 KB
FAILED: [[SimpleTest]]: [MySQL] 53,159 pass(es), 34 fail(s), and 3 exception(s). View

I've created a new tag without the "Create term" permission because I'm waiting for this patch:
https://drupal.org/node/1862758#comment-7171044

to be accepted.

Status: Needs review » Needs work

The last submitted patch, drupal-split_taxonomy_permissions-1848686-53.patch, failed testing.

Johnny vd Laar’s picture

Fixed the broken tests (hopefully). Next step is to add the create taxonomy permission and to fix this issue:

the problem as i see it with the current approach is that a user with access taxonomy overview can change the weight of the vocabularies and tags..reorder them and reparent them, even though he might not have permissions to edit them.
Also current tests, fail to catch this, which is a security isssue.

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
18.57 KB
PASSED: [[SimpleTest]]: [MySQL] 53,455 pass(es). View
Johnny vd Laar’s picture

FileSize
22.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,458 pass(es). View

Here is a fix for the taxonomy overview weight problem. I'm not really sure about the fix because each term could have a different access result now... so what to do if one term has edit access and the other one doesn't have access? Should we show the tabledrag or not? I assume we shouldn't.

Johnny vd Laar’s picture

FileSize
24.88 KB
PASSED: [[SimpleTest]]: [MySQL] 53,443 pass(es). View

I have incorporated this patch:
https://drupal.org/node/1862758#comment-7171044

and now added a new create permission as well. So now the patch includes all the features that where included here:
http://drupal.org/node/1848686#comment-6955198

xjm’s picture

Thanks @Johnny vd Laar! Usually when you combine your patch with another, it's best to include your patch by itself (rolled against the other) in a patchname-do-not-test.patch file so that reviewers can tell which code needs to be reviewed.

Nice work on the tests so far. I think it would be better to test each permission separately, to confirm that one isn't bypassing another. Right now we are testing (possibly duplicating tests of) the administer taxonomy permission.

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
    @@ -0,0 +1,202 @@
    +  function setUp() {
    +    parent::setUp();
    +  }
    

    This isn't doing anything; it can be removed.

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
    @@ -0,0 +1,202 @@
    +    $user = $this->drupalCreateUser(array("administer taxonomy", "edit terms in {$vocabulary->id()}", "delete terms in {$vocabulary->id()}"));
    

    Shouldn't "administer taxonomy" supercede the other two?

Johnny vd Laar’s picture

Yes you are right about 1 and 2.

Nice work on the tests so far. I think it would be better to test each permission separately, to confirm that one isn't bypassing another. Right now we are testing (possibly duplicating tests of) the administer taxonomy permission.

With this you mean that we should have a function for "administer taxonomy", one function for "edit terms", etc. And in each function create a user with this permission and without the permission?

xjm’s picture

With this you mean that we should have a function for "administer taxonomy", one function for "edit terms", etc. And in each function create a user with this permission and without the permission?

It doesn't need to be separate test methods, just separate users in the same test method since we don't need a fresh Drupal installation to log in and out to test separate users. Also, the "administer taxonomy" permission may already have its own test coverage, so you'll want to check for that in the existing tests.

Johnny vd Laar’s picture

FileSize
22.62 KB
25.19 KB
PASSED: [[SimpleTest]]: [MySQL] 54,229 pass(es). View

I have modified the tests based on your code and added removed the included patch. I also fixed a bug with the "edit terms in" permission because it is called "update terms in" on other places. (https://drupal.org/node/1862758#comment-7261618).

Johnny vd Laar’s picture

These issues got committed:
https://drupal.org/node/1862758

and

https://drupal.org/node/1862758

So I will update this patch. It should be a lot smaller now as most of the tests are already included in latest release.

Johnny vd Laar’s picture

FileSize
18.87 KB
PASSED: [[SimpleTest]]: [MySQL] 55,639 pass(es). View

Wow lots of things changed since last time ;-) but the patch is now updated. Also with the new table theming.

Johnny vd Laar’s picture

Would this still be possible to add for d8 or will this only be possible for d9? In that case I'll wait till after d8 has been released to put time in this.

ParisLiakos’s picture

i believe it can go in since its an api addition not an api change, but that would still require for thresholds to be down, which is far from true right now

Johnny vd Laar’s picture

FileSize
18.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,726 pass(es), 2 fail(s), and 1 exception(s). View

This patch is not complete but it's a first stab at it.

Johnny vd Laar’s picture

If someone looks into this. These things don't look optimal to me:

Inside TaxonomyOverviewAccessCheck

  public function access(Route $route, Request $request) {
    $access = user_access('administer taxonomy') || user_access('access taxonomy overview');

    return $access ? static::ALLOW : static::DENY;
  }

I think I should be able to do this:

$account = $request->attributes->get('_account');

But I get errors that account is NULL???

Inside OverviewTerms->buildForm I do this:

    $access_controller = \Drupal::entityManager()->getAccessController('taxonomy_term');
    if ($access_controller->createAccess($taxonomy_vocabulary->id())) {
      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', array('@link' => url('admin/structure/taxonomy/manage/' . $taxonomy_vocabulary->id() . '/add')));
    }
    else {
      $empty = $this->t('No terms available.');
    }

Is this the right way to do this? Because there is also a TaxonomyTermCreateAccess class. But I don't know how to call it.

In VocabularyListController->render I do this:

    if (user_access('administer vocabularies')) {
      $build['#empty'] = t('No vocabularies available. <a href="@link">Add vocabulary</a>.', array('@link' => url('admin/structure/taxonomy/add')));
    }
    else {
      $build['#empty'] = t('No vocabularies available.');
    }

But I actually want to check the create permission of vocabularies. But I don't know how to do this.

Status: Needs review » Needs work

The last submitted patch, drupal-split_taxonomy_permissions-1848686-67.patch, failed testing.

ParisLiakos’s picture

About the _account thing:
You should use Drupal::currentUser() the attribute in the request had many problems so we switched it to a separate service

Johnny vd Laar’s picture

FileSize
23.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,629 pass(es), 2 fail(s), and 4 exception(s). View

Attached version includes #70 and also has create check for tagging.

Johnny vd Laar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-split_taxonomy_permissions-1848686-71.patch, failed testing.

Johnny vd Laar’s picture

Status: Needs work » Needs review
FileSize
23.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,612 pass(es), 2 fail(s), and 4 exception(s). View

This should fix all tests. I think the patch includes all functionalities that my original patch contained.

The last submitted patch, drupal-split_taxonomy_permissions-1848686-74.patch, failed testing.

Johnny vd Laar’s picture

Status: Needs review » Needs work
FileSize
23.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,666 pass(es), 2 fail(s), and 4 exception(s). View

Gah I added an old patch.

Johnny vd Laar’s picture

Status: Needs work » Needs review
Johnny vd Laar’s picture

FileSize
24.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,093 pass(es). View

As long as I'm talking to myself, nobody will notice me being a n00b ;-)

MrHaroldA’s picture

As long as I'm talking to myself, nobody will notice me being a n00b ;-)

I noticed! ;)

Status: Needs review » Needs work
Issue tags: -Security improvements, -Usability, -Needs tests, -taxonomy permissions

The last submitted patch, drupal-split_taxonomy_permissions-1848686-77.patch, failed testing.

Johnny vd Laar’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements, +Usability, +Needs tests, +taxonomy permissions
Johnny vd Laar’s picture

Yay, can someone look into this such that we can get this finally committed?

Johnny vd Laar’s picture

Issue summary: View changes

oops

JeroenT’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

RobLoach’s picture

Full CRUD permissions would be great.

gmaldonado’s picture

Assigned: Unassigned » gmaldonado

I will try to reroll the last patch provided in the comments

gmaldonado’s picture

Assigned: gmaldonado » Unassigned
cyborg_572’s picture

Assigned: Unassigned » cyborg_572

I'm going to attempt the re-roll

JeroenT’s picture

Assigned: cyborg_572 » Unassigned

cyborg_572, Are you still working on this?

cyborg_572’s picture

FileSize
24.21 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch drupal-split_taxonomy_permissions-1848686-89.patch. Unable to apply patch. See the log in the details link for more information. View

Ah, I guess not, sorry. My my patch imploded on me (I think I missed a few conflicts before committing), and I haven't had a chance to try again. I'll attach what I had, despite it not working.

pfrenssen’s picture

Looking at what would be needed to reroll this to current HEAD:

  1. AutocompleteTagsWidget no longer exists. This is currently handled by \Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection which operates completely differently.
  2. AutocompleteWidget no longer exists. This is now handled by \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget.
  3. TermAccessController has become \Drupal\taxonomy\TermAccessControlHandler.
  4. VocabularyAccessController no longer exists. Part of its functionality now seems to be covered by \Drupal\taxonomy\TermAccessControlHandler::checkCreateAccess().
  5. TaggedWithTest has moved to \Drupal\views\Tests\Wizard\TaggedWithTest.
  6. The code in the following classes and files has diverged and needs to be rerolled: EntityReferenceAutoCreateTest, OverviewTerms, VocabularyListBuilder, taxonomy.module, taxonomy.services.yml.

Almost nothing applies any more :-/

pfrenssen’s picture

This is what a git rebase + manual fixing of the most glaring merge conflicts looks like. This is missing everything from the files that have been removed, but it's a starting point.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
16.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/taxonomy/src/Form/OverviewTerms.php. View

Let's see how bad this really is :)

Status: Needs review » Needs work

The last submitted patch, 92: 1848686-91.patch, failed testing.

Manuel Garcia’s picture

Thanks @pfrenssen for keeping this moving!

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -204,14 +204,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', ['@link' => $this->url('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])]),

Looks like this should be a semicolon instead of a comma ending the line.

+++ b/core/modules/node/src/Tests/NodeAccessBaseTableTest.php
@@ -99,7 +99,7 @@ function testNodeAccessBasic() {
+      $simple_users[$i] = $this->drupalCreateUser(array('access content', 'create article content', 'create terms in tags'));

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -256,28 +265,36 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+      $operations = array();

@@ -285,10 +302,14 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+        $form['terms'][$key]['operations'] = array(
+          '#type' => 'operations',
+          '#links' => $operations,
+        );

@@ -318,32 +339,40 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    $form['terms']['#header'] = array($this->t('Name'));

+++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
@@ -15,6 +15,33 @@
+    $authenticated_user = $this->drupalCreateUser(array());
...
+    $proper_user = $this->drupalCreateUser(array('access taxonomy overview'));

@@ -57,8 +84,35 @@ function testVocabularyPermissionsTaxonomyTerm() {
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "create terms in {$vocabulary->id()}"));
...
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "edit terms in {$vocabulary->id()}"));

@@ -82,7 +136,7 @@ function testVocabularyPermissionsTaxonomyTerm() {
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "delete terms in {$vocabulary->id()}"));

+++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
@@ -46,11 +46,14 @@ public function getDefaultOperations(EntityInterface $entity) {
+      $operations['add'] = array(
+        'title' => t('Add terms'),
+        'weight' => 10,
+        'url' => Url::fromRoute('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $entity->id()]),
+      );

@@ -83,7 +91,14 @@ public function render() {
+      $build['table']['#empty'] = t('No vocabularies available. <a href="@link">Add vocabulary</a>.', array('@link' => \Drupal::url('entity.taxonomy_vocabulary.add_form')));

+++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
@@ -15,6 +15,13 @@
+  protected $permissions = array();

@@ -24,8 +31,10 @@
+    $this->permissions = array_merge($this->permissions, array('administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions'));

We should use short array syntax on these.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
18.42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 86,047 pass(es), 129 fail(s), and 122 exception(s). View
3.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff_44.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed the fatal error in OverviewTerms and added the new dynamic permissions to TaxonomyPermissions like is done in current D8 HEAD.

I'm actually also in favor of providing full CRUD permissions in this issue as suggested by @RobLoach in #84. Shall we do that? Would be great for REST integration.

@Manuel Garcia, thanks a lot for the review! This evidently will need a lot of work to get it back in a working state and adhering to our current practices. I just saw your comment now, so I have not yet addressed your remarks.

Status: Needs review » Needs work

The last submitted patch, 95: interdiff.patch, failed testing.

The last submitted patch, 95: 1848686-95.patch, failed testing.

Manuel Garcia’s picture

I agree with #84 as well, it would be very nice!

JeroenT’s picture

Status: Needs work » Needs review
FileSize
21.07 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,096 pass(es), 3 fail(s), and 2 exception(s). View
6.92 KB

This should fix a lot of the failing tests.

Status: Needs review » Needs work

The last submitted patch, 99: split_administer-1848686-99.patch, failed testing.

JeroenT’s picture

FileSize
419 bytes
21.66 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch split_administer-1848686-101.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed failing tests in VocabularyPermissionsTest.

JeroenT’s picture

Status: Needs work » Needs review

The last submitted patch, 89: drupal-split_taxonomy_permissions-1848686-89.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 101: split_administer-1848686-101.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
21.62 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,169 pass(es), 4 fail(s), and 1 exception(s). View

This is the right patch.

Status: Needs review » Needs work

The last submitted patch, 105: split_administer-1848686-105.patch, failed testing.

zniki.ru’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
21.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,355 pass(es), 6 fail(s), and 2 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 107: split_administer-1848686-107.patch, failed testing.

JeroenT’s picture

Assigned: Unassigned » JeroenT
JeroenT’s picture

Assigned: JeroenT » Unassigned
Status: Needs work » Needs review
Issue tags: +DUGBE0609
FileSize
25.38 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch split_administer-1848686-110.patch. Unable to apply patch. See the log in the details link for more information. View

This should fix the failures.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 110: split_administer-1848686-110.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
25.38 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,456 pass(es), 3 fail(s), and 0 exception(s). View

This patch should apply..

Status: Needs review » Needs work

The last submitted patch, 112: split_administer-1848686-112.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
24.97 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,548 pass(es). View

Patch should be green again.

pfrenssen’s picture

WOW! Great work Jeroen!!

pfrenssen’s picture

Status: Needs review » Needs work

Really excited by this, did a quick "drive by" review in Dreditor during my lunch break.

  1. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -67,19 +67,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        if($form['terms'][$key]['#term']->access('delete')) {
    +          unset($form['terms'][$key]['operations']['#links']['delete']);
    

    Coding standards: put a space between the 'if' and the opening parenthesis.

    This also reads strange: if the user has access to delete the term, we remove the delete link?

  2. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -67,19 +67,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        if($form['terms'][$key]['#term']->access('update')) {
    

    Missing space between 'if' and opening parenthesis.

  3. +++ b/core/modules/taxonomy/src/Access/TaxonomyOverviewAccessCheck.php
    @@ -0,0 +1,28 @@
    +/**
    + * @file
    + * Contains \Drupal\taxonomy\Access\TaxonomyOverviewAccessCheck
    + */
    

    Missing period at the end of the sentence.

  4. +++ b/core/modules/taxonomy/src/Access/TaxonomyOverviewAccessCheck.php
    @@ -0,0 +1,28 @@
    +  /**
    +   * @param AccountInterface $account
    +   * @return AccessResult
    +   */
    

    {@inheritdoc}

  5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -204,14 +212,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', ['@link' => $this->url('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])]);
    

    For readability I would split out the replacement strings in a separate $args variable.

  6. +++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
    @@ -15,6 +15,33 @@
    +  function testVocabularyPermissionsVocabulary() {
    

    This needs more test coverage, the test docblock describes a full CRUD test, but the test actually only tests if a user with the right permission can access the administration page, and that the link is there.

  7. +++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
    @@ -15,6 +15,33 @@
    +    // VocabularyTest.php already tests for user with "administer taxonomy" permissions.
    

    Documentation exceeds 80 characters.

  8. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -25,6 +30,51 @@ class VocabularyListBuilder extends DraggableListBuilder {
    +  /**
    +   * Constructs a new VocabularyListBuilder object.
    +   *
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   *   The entity type definition.
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $storage
    +   *   The entity storage class.
    +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    +   *   The entity manager service.
    +   * @param \Drupal\Core\Session\AccountInterface $current_user
    +   *   The current user.
    +   */
    +  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, AccountInterface $current_user, EntityManagerInterface $entity_manager) {
    

    The order of the arguments in the docblock doesn't match the actual order in the function declaration.

pfrenssen’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs update path

This does not meet the beta criteria for 8.0.x. I consulted @alexpott on IRC and this should be moved to 8.1.x. We should also add an update path.

hauruck’s picture

Re-roll: This great patch doesn't apply to RC3 anymore, so here is a re-roll. No actual changes to #114 has been made.

webflo’s picture

Status: Needs work » Needs review

Go testbot.

webflo’s picture

FileSize
24.94 KB
webflo’s picture

Ok i get it. We don't have automated tests for 8.1.x.

hauruck’s picture

FileSize
25.83 KB
4.24 KB

I have made a new patch. The access was right but some permissions like "delete term" only worked when called over url and not over the UI. The following changes has been made:

  • Only vocabularies where the user has the write to add a term is show in the overview
  • The Term overview pages can also be access if the user have the new "Access the taxonomy vocabulary overview page" permission since. Otherwise user get an "access denied" when clicking "list terms" on vocabulary overview page.

The suggestions from #116 has also been Incorporated, except 5.

webflo’s picture

  1. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -46,11 +96,14 @@ public function getDefaultOperations(EntityInterface $entity) {
    +    if ($taxonomy_term_access_control_handler->createAccess($entity->id())) {
    

    The list builder class has a current user, pass it on to the access control handler.

  2. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -68,8 +126,11 @@ public function buildHeader() {
    +    if ($taxonomy_term_access_control_handler->createAccess($entity->id())) {
    

    Invoke createAccess with $this->current_user.

hauruck’s picture

New patch: changes has been made to the logic on taxonomy page. More similar to original patch. Instead of removing the row with vocabularies where the user didn't have the right to create new terms, all vocabularies are shown with the minimum of a "view terms" button.

DuaelFr’s picture

Hi! Thanks everyone for contributing to this issue. I'm a strong supporter of Taxonomy Access Fix so I really hope that this could be integrated in a future release of the Core.

First, I'd like to give my 2 cts about the last patch :

  1. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -4,7 +4,7 @@ entity.taxonomy_vocabulary.collection:
    -    _permission: 'administer taxonomy'
    +    _access_taxonomy_overview: 'TRUE'
    
    @@ -72,7 +72,7 @@ entity.taxonomy_vocabulary.overview_form:
    -    _entity_access: 'taxonomy_vocabulary.view'
    +    _access_taxonomy_overview: 'TRUE'
    

    Do we really need to declare an access service there?
    Couldn't we just use _permission: 'administer taxonomy+access taxonomy overview' ?

  2. +++ b/core/modules/taxonomy/taxonomy.services.yml
    @@ -4,3 +4,7 @@ services:
    diff --git a/core/modules/views/src/Tests/Wizard/WizardTestBase.php b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    
    diff --git a/core/modules/views/src/Tests/Wizard/WizardTestBase.php b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    index 7d8d9c0..0e35233 100644
    
    index 7d8d9c0..0e35233 100644
    --- a/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    
    --- a/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    
    +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
    @@ -15,6 +15,13 @@
    
    @@ -15,6 +15,13 @@
     abstract class WizardTestBase extends ViewTestBase {
     
       /**
    +   * Permissions for this user to test with.
    +   *
    +   * @var array
    +   */
    +  protected $permissions = [];
    +
    +  /**
        * Modules to enable.
        *
        * @var array
    @@ -24,8 +31,10 @@
    
    @@ -24,8 +31,10 @@
       protected function setUp() {
         parent::setUp();
     
    +    $this->permissions = array_merge($this->permissions, ['administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions']);
    +
         // Create and log in a user with administer views permission.
    -    $views_admin = $this->drupalCreateUser(array('administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions'));
    +    $views_admin = $this->drupalCreateUser($this->permissions);
         $this->drupalLogin($views_admin);
         $this->drupalPlaceBlock('local_actions_block');
       }
     
    

    All this part looks out of scope.

-----

Then I'd like to elaborate on the upgrade path. We absolutely need one and we need to define the mapping between the current permissions and the new ones. This is my proposal:

Permission Given if
Administer vocabularies The role has 'Administer vocabularies and terms'
Access the taxonomy overview page The role has 'Administer vocabularies and terms'
Vocabulary: add terms The role has 'Administer vocabularies and terms'
Vocabulary: delete terms The role has 'Edit terms in Vocabulary'
Vocabulary: edit terms The role has 'Delete terms from Vocabulary'

-----

One last thought, in \Drupal\taxonomy\TermAccessControlHandler::checkAccess() the 'access content' permission is used to check access for the 'view' operation. Shouldn't we add a new permission per vocabulary to handle this case?

Manuel Garcia’s picture

Status: Needs review » Needs work

Thanks @DuaelFr for the review!

tstoeckler’s picture

Just tried out the patch. It seems the access checking for the OverviewTerms is not working correctly. For a user with only the access taxonomy overview permission (but not administer taxonomy) the weight fields and operations get properly hidden, but the "Save" and "Reset to alphabetical" buttons are still shown and the latter even works, which is not nice.

Johnny vd Laar’s picture

Version: 8.1.x-dev » 9.x-dev

As a more administrative question. Would a change like this be a BC breaking change that needs to go into D9? Because I think that this is the case.

DuaelFr’s picture

Version: 9.x-dev » 8.1.x-dev

I don't agree.
It's really easy to provide an upgrade path for this kind of issue and, if necessary, we could keep the old permission machine name in case some contrib rely on it.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

miro_dietiker’s picture

I'm confused about consistency in naming.

The whole comments mention a "create term" permission and suddenly the permission label is "Add term"?
Note that for node creation, the permission uses the label "Create" while the effective button is labeled "Add content" and all other UI mentions.
There is only a single permission related to "Add" that is field.
So i think we should name it "Create term" and create a follow-up to switch consistently to Add content / add term, ...

ciss’s picture

ciss’s picture

(Note to self: don't try to add related issues while on mobile.)

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Rerolling, let's continue work on this!

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
26.34 KB

Fixed conflict on core/modules/taxonomy/src/Form/OverviewTerms.php

Setting it to needs review to see what the bot says, but this patch still needs work based on #125, #127 and #131.

Bambell’s picture

I did the changes suggested in #125 and #127. I attached a screenshot of the terms overview page after the change (the "Save" and "Reset to alphabetical" buttons are gone with access taxonomy overview permission only (shouldn't we also remove the message above, about re-ordering?)). As for #131, despite the entire discussion going around a "Add terms" permission, the permission does read "Tags: Create terms" on /admin/people/permissions (screenshot attached). The button on /admin/structure/taxonomy/manage/tags/overview does read "Add term". So the inconsistency seems... properly preserved ! Also, the text on the terms overview page state to use the drag-and-drop handles even if there's only 1 element (and thus no handles), this should probably be fixed (screenshot attached)?

miro_dietiker’s picture

Status: Needs review » Needs work

Looking great. A bit more feedback.

The inconsistency about "Add" in #131 was fixed in #135. Pity @Manuel Garcia did neither provide an interdiff, nor list the items he fixed. I think the situation is fine (aka consistently inconsistent ;-D)

Yeah the intro text in the term overview is wrong now. It still states dragging although you don't have permissions.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -199,14 +207,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
       '#header' => array($this->t('Name'), $this->t('Weight'), $this->t('Operations')),

The weight column is still unconditionally defined in the header?

Manuel Garcia’s picture

@miro_dietiker my patch on #135 is just a reroll of #124, I did not fix any issues mentioned here, no need for a interdiff.

All i did was fix a simple merge conflict on core/modules/taxonomy/src/Form/OverviewTerms.php, which was the reason why we had to reroll the patch.

Bambell’s picture

Status: Needs work » Needs review
FileSize
26.92 KB
2.17 KB
5.9 KB

Note also that now the intro text in the term overview is wrong. It still states dragging although you don't have permissions!

I changed this in taxonomy_help(), while keeping the different phrasings, dependently of the terms hierarchy.

The weight column is still unconditionally defined in the header?
That line looks pointless, actually. If we look at :

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -313,44 +342,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    $form['terms']['#header'] = array($this->t('Name'));

$form['terms']['#header'] is overwritten here and never used after the first declaration. Then, the Weight column is properly added :

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -313,44 +342,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    if ($change_weight_access) {
+      $form['terms']['#header'][] = $this->t('Weight');

I removed it. No change in UI when testing locally.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ressa’s picture

This would be great to get into Drupal 8.3, or is there a contrib module which accomplishes the same thing?

EDIT: To answer my own question - Yes, and it works fine: Taxonomy Access Fix.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Status: Needs review » Needs work

Rerolling last patch...

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
26.65 KB
(8.3.x) $ git apply --index split_administer-1848686-139.patch 
error: patch failed: core/modules/taxonomy/src/Form/OverviewTerms.php:313
error: core/modules/taxonomy/src/Form/OverviewTerms.php: patch does not apply
error: patch failed: core/modules/taxonomy/src/VocabularyListBuilder.php:56
error: core/modules/taxonomy/src/VocabularyListBuilder.php: patch does not apply
error: patch failed: core/modules/taxonomy/taxonomy.module:66
error: core/modules/taxonomy/taxonomy.module: patch does not apply

Manually fixed conflicts in:

  • core/modules/taxonomy/src/VocabularyListBuilder.php
  • core/modules/taxonomy/src/Form/OverviewTerms.php

The rebase did the rest.

Status: Needs review » Needs work

The last submitted patch, 143: split_administer-1848686-143.patch, failed testing.

Algeron’s picture

I couldn't get #143 to apply on 8.2.3. New patch in attachment.

nikathone’s picture

Status: Needs work » Needs review

To trigger test on the latest patch

The last submitted patch, 124: split_administer-1848686-124.patch, failed testing.

Manuel Garcia’s picture

@Algeron Thanks for working on this, but the patch on #143 is against 8.3.x, and still applies cleanly. We have to do this on 8.3.x not 8.2.x.

Status: Needs review » Needs work

The last submitted patch, 145: split_administer-1848686-145.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joegraduate’s picture

Status: Needs work » Needs review

Patch in #143 still applies cleanly against 8.4.x.

realityloop’s picture

#143 doesn't apply against 8.3.0-rc1.

Attached does.

realityloop’s picture

previous was missing one file..

#143 doesn't apply against 8.3.0-rc1.

Attached does.

The last submitted patch, 152: split_administer-1848686-152.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 153: split_administer-1848686-153.patch, failed testing.

eelkeblok’s picture

Am I correct in assuming #153/#153 is just intended to be able to apply this patch to 8.3 for anyone who is actively using this in their projects? It would need to apply cleanly to the current HEAD, which is 8.4.

eelkeblok’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

@eelkeblok try the patch in #143

eelkeblok’s picture

Status: Needs review » Needs work

I don't need the patch, I'm just trying to understand the nature of the patch from #153. At some point this will need to be applied in the project Git and the committer will need to understand what patch to use.

Setting back to needs work. It now looks like #143 does not, in fact, apply to 8.4 (see test results).

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Issue tags: +Needs reroll

Rerolling...

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
26.5 KB

Here is a reroll of #143 against 8.4.x

Manually fixed on:

  • VocabularyPermissionsTest.php
  • VocabularyListBuilder.php
  • OverviewTerms.php
  • NodeAccessBaseTableTest.php

Status: Needs review » Needs work

The last submitted patch, 161: split_administer-1848686-161.patch, failed testing.

mogtofu33’s picture

Patch #161 do not apply anymore on 8.4.x.

Here is a fixed one.

mogtofu33’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 163: split_administer-1848686-165.patch, failed testing.

geertvd’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
27.43 KB

This should fix those failing tests

Status: Needs review » Needs work

The last submitted patch, 166: split_administer-1848686-166.patch, failed testing.

mogtofu33’s picture

Status: Needs work » Needs review
FileSize
27.41 KB

Should fix remaining failed tests.

mogtofu33’s picture

Here is a new patch with code standard fixes.
And a Backport for 8.3 fixed.

The last submitted patch, 170: split_administer-8.4-1848686-170.patch, failed testing. View results

geertvd’s picture

J-Lee’s picture

Applied the 8.3 patch from #170.
It works fine but I have a small feature request.

It would be fine, if the user could access the taxonomy vocabulary overview page via the toolbar without given the "access administration pages" permission to this user.

Maybe it would be good to show only the vocabulary for which the user has rights.

eelkeblok’s picture

The access administration pages permission is a pretty basic permission that only controls if a user can view any pages at all in the admin area (anything that is marked as an admin page and is rendered using the admin theme). Making an exception on that here would break the expectations for that permission. Maybe you can accomplish what you want by altering the route to not be an admin page (similar to what the setting does that controls if node edit screens are considered admin pages or not).

J-Lee’s picture

Thank you for the explanation. That is a good idea. I will try it.

Edit: I have created a GitHub Gist for this, if someone need this.

seanB’s picture

+++ b/core/modules/taxonomy/src/Access/TaxonomyOverviewAccessCheck.php
@@ -0,0 +1,25 @@
+    return AccessResult::allowedIf($access);

I think this needs cachePerPermissions(). Or maybe even better use AccessResult::allowedIfHasPermissions($account, ['administer taxonomy', 'access taxonomy overview'], 'OR')

nikathone’s picture

Status: Needs review » Needs work

#172 didn't apply on 8.3.4 but #170 did the trick.

MacSim’s picture

#170 looks fine to me. Tested & approved :)

Berdir’s picture

There are a ton of related issues around this that all make similar, more or less overlapping changes.

Trying to get an overview of them to figure how to move forward. Clearly a lot of people could use this given how many issues we have and how old they are.

Current status:
Vocabulary has "administer taxonomy" as admin permission. The overview route is access controlled with entity_access: taxonomy_vocabulary.view. Only users with administer taxonomy can access term overview pages, create and edit terms there. Users with edit and delete permissions can only do that through taxonomy pages, they have no way to get an overview of all terms in a vocabualary.

This:
Introduces a new permission to access overview page, removes the entity_access from the route definition and uses permissions instead. Does not change vocabulary access. Introduces create terms in vocabulary X permission, updates REST tests to use that and makes sure that create/delete/edit operations for terms (and forum as a result of that) on overview respect access.

Limitation: overview permission is for all vocabularies, it is not possible to give access only to one vocabulary.

#1038330: Allow specific vocabulary permissions to work on vocabulary admin pages:
Very old patch that adds add permission. I think we can close that as a duplicate of this and will do so after posting this issue.

#2469567: Entity operations for terms are hardcoded in taxonomy terms's overview page:
Adds permission checks to operations on overview form, adds a term list builder to do that and also adds destination link there. There is also a separate issue to always add the destination in the list builder. The approach there is interesting but I think it could also be closed as a duplicate of this, maybe we can merge that approach into this issue.

#2650898: ListBuilders do not check $entity->access() for operation links:
Adds some access checks to operations, including a view check for vocabulary for viewing the term overview page, so it conflicts with this.

#2599128: Allow user with edit terms permission to access vocabulary overview:
Grants access to overview page through term vocabulary access controller if user either has edit or delete permission. I worked on that before and now agree that this approach is flawed. I think this can also be closed as a duplicate of this. David_Rothstein there suggested an access overview per vocabulary permission, which would solve the current limitation of this issue. It also has some test that we could move over here. So I think we can close that as a duplicate of this as well. Will do that.

Does have one interesting fix for forum by allowing edit access with the administer forums permission. But I think it currently does that for all terms which is too much.

#2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission:
Main change is to allow view vocabulary access with the access content permission like other similar REST related issues. That results in the problem with the overview route, which it also solves by using the administer taxonomy permission directly. Overlaps heavily with this issue and conflicts with the approach in the issue above.

@dawehner suggested a custom entity operation to wrap the permission check. I was sceptical at first because we have no real rules on adding custom operations. But that's quite interesting as it would allow to customize it through entity access hooks.

#2658956: Taxonomy vocabulary data not available as views fields:
Did the same change as the issue above, was committed and reverted because it made the taxonomy overview page public due to that.

#2824408: To be able to create/update/delete Term entities via REST, one should not have to grant the 'administer taxonomy' permission:
Proposes what should actually already be possible for edit/delete, this does that while also adding the check for CREATE. Possibly another duplicate.

(Edit, moar!):
#2687775: OverviewTerms should not list operations for which the current user does not have the access permission:
Another duplicate of the missing operation access on term overview, also going to close that as duplicate.

Proposal:

Postpone other issues on this, decide on the exact approach for the overview access here, get it in, which will make most of the other issues trivial or duplicates.

For the access, we have 4 different options I think:
A) A single permission like proposed here
B) A single permission, checked with e.g. $vocabulary->access('access term overview'), allows custom/contrib code to easily check access.
C) A permission per vocabulary
D) A permission per vocabulary, checked with e.g. $vocabulary->access('access term overview'), allows custom/contrib code to easily check access.

I think I would suggest B or D. I'll try to implement B here to see how that looks. From there, going to D would be fairly simple.

Berdir’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs update path
FileSize
33.39 KB
19.93 KB

This adds the taxonomy list builder for the operations to avoid having to duplicate that and uses that for the operation links. We only need the custom class until #2767857: Add destination to edit, delete, enable, disable links in entity list builders is fixed.

Also added the new access operation and using that, with a single permision. Kept the permission check on the vocabulary overview itself as we have no list acess API.

Also a ton of related cleanup like removing the access check as we no longer need that.

This is tagged with Needs update path but I don't think it needs one, we just add an optional new permission with full BC. This is also tagged with Needs tests, but I think we are adding enough test coverage.

All taxonomy tests are passing locally.

Considering the amount of time this was open and how many duplicates/related issues we have, setting to major.

Berdir’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Thanks @Berdir, great roundup of the related issues. Agreed with the priority of major (given how many duplicates and partial duplicates there are) and I agree that a new permission for the overview page is a good option, since the page provides advanced functionality rather than normal entity access.

  1. +++ b/core/modules/forum/forum.module
    @@ -349,6 +352,22 @@ function forum_form_node_form_alter(&$form, FormStateInterface $form_state, $for
    +function forum_taxonomy_term_access(TermInterface $term, $operation, AccountInterface $account) {
    +  // Allow update access to terms in the forum vocabulary with the
    +  // administer forums permission.
    +  $forum_vid = \Drupal::config('forum.settings')->get('vocabulary');
    +  if ($operation === 'update' && $term->bundle() === $forum_vid) {
    +    return AccessResult::allowedIfHasPermission($account, 'administer forums');
    +  }
    

    This maybe should be a separate issue scope?

  2. +++ b/core/modules/forum/forum.module
    @@ -349,6 +352,22 @@ function forum_form_node_form_alter(&$form, FormStateInterface $form_state, $for
    +}
    +
    +
    +/**
      * Implements hook_preprocess_HOOK() for block templates.
    

    Nit: extra newline.

  3. +++ b/core/modules/forum/tests/src/Functional/ForumIndexTest.php
    @@ -57,6 +57,8 @@ public function testForumIndexStatus() {
    +    $this->assertSession()->linkExists(t('edit forum'), 0, 'edit forum link is not accessible to user with administer forums permission');
    

    Shouldn't this be "Edit forum link is accessible to user with administer forums permission"?

  4. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -260,39 +276,30 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +      if ($operations = $this->termListBuilder->getOperations($term)) {
    +        // Allow access to operations if there is at least one term with
    +        // operations.
    
    @@ -322,44 +329,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    if (($taxonomy_vocabulary->getHierarchy() != VocabularyInterface::HIERARCHY_MULTIPLE && count($tree) > 1) && $change_weight_access) {
           $form['actions'] = ['#type' => 'actions', '#tree' => FALSE];
           $form['actions']['submit'] = [
             '#type' => 'submit',
             '#value' => $this->t('Save'),
             '#button_type' => 'primary',
    +        '#access' => $change_weight_access,
           ];
           $form['actions']['reset_alphabetical'] = [
             '#type' => 'submit',
             '#submit' => ['::submitReset'],
             '#value' => $this->t('Reset to alphabetical'),
    +        '#access' => $change_weight_access,
    

    This all with the access check for reordering seems like it should have test coverage. Also, maybe it is a separate issue scope?

  5. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -75,6 +75,17 @@ function taxonomy_help($route_name, RouteMatchInterface $route_match) {
         case 'entity.taxonomy_vocabulary.overview_form':
           $vocabulary = $route_match->getParameter('taxonomy_vocabulary');
    +      if (!\Drupal::currentUser()->hasPermission('administer taxonomy')) {
    +        // There is no drag-and-drop handles.
    +        switch ($vocabulary->getHierarchy()) {
    +          case TAXONOMY_HIERARCHY_DISABLED:
    +            return '<p>' . t('%capital_name contains the following terms.', ['%capital_name' => Unicode::ucfirst($vocabulary->label())]) . '</p>';
    +          case TAXONOMY_HIERARCHY_SINGLE:
    +            return '<p>' . t('%capital_name contains terms grouped under parent terms', ['%capital_name' => Unicode::ucfirst($vocabulary->label())]) . '</p>';
    +          case TAXONOMY_HIERARCHY_MULTIPLE:
    +            return '<p>' . t('%capital_name contains terms with multiple parents.', ['%capital_name' => Unicode::ucfirst($vocabulary->label())]) . '</p>';
    +        }
    +      }
    

    This seems out of scope?

  6. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -4,7 +4,7 @@ entity.taxonomy_vocabulary.collection:
    -    _permission: 'administer taxonomy'
    +    _permission: 'access taxonomy overview+administer taxonomy'
    

    Wait, is this making it so that you need both the new permission and administer taxonomy to access the page? Shouldn't it be either/or?

  7. The patch includes test coverage for the new permission through the UI, but should we also have test coverage via REST?

I haven't tested the patch manually yet but there's a lot of updated functionality so we should probably test it carefully.

xjm’s picture

Title: Split "Administer vocabularies and terms" permission: Access the taxonomy overview page and Add terms in vocabulary name » Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)
xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

NW for #183.

Berdir’s picture

Thanks for the review. Response to it first, I'll try to update the issue summary based on my previous comment and this asap.

1. This part is interesting. Versions of this exist in #2469567: Entity operations for terms are hardcoded in taxonomy terms's overview page, this issue and I believe also some others. Right now, if you create a user with *only* the administer forums permission, without any other permission, you are able to edit forum terms, but *only* through /admin/structure/forum and the special forum edit route. This approach goes farther than that, as it also allows edit access on the normal taxonomy edit path, so it might be better to just override edit access on the Overview form, which extends the normal term overview form, which is why we need *something* as we now inherit the access check. That said, I do think the whole UI at /admin/structure/forum more or less only exists because taxonomy module in the past was not flexible enough to just grant specific access, so maybe we could do a follow-up to just drop that whole thing? Not quite sure how we'd do that in 8.x. So for now, changed it a bit to just re-create the operations completely in the forum specific overview form, then we don't have to worry about whatever permissions the parent defines.

2. no longer relevant then.

3. I think mink test messages *should* explain the error, not the success condition. It is passed to \Behat\Mink\Exception\ExpectationException::__construct() through assert(). See some direct usages in e.g. \Drupal\FunctionalTests\AssertLegacyTrait::assertNoFieldById() and \Drupal\Tests\book\FunctionalJavascript\BookJavascriptTest::assertOrderInPage(). But as we convert our existing tests and have tons of indirection through traits and so on, we have thousands of wrong calls and wrong documentation.. I just removed the message for now as I think the default is probably sufficient.

4. Similar to forum stuff, we need to change it to account for cases where users can access the overview without having edit permission to all shown terms, because only then they can reorder them. But agreed on the test coverage. #2599128: Allow user with edit terms permission to access vocabulary overview already had quite extensive test coverage for that issue, I copied that over and updated and expanded for lots of combinations of overview/create/edit/delete permissions.

5. Yeah, it seems, but I think it was actually a bad merge. We have specific help texts about the drag and drop things for that page and this was supposed to provide alternatives, not completely new things. Fixed, removed deprecated usages and added test coverage.

6. \Drupal\user\Access\PermissionAccessCheck, + is OR, , is AND. IIRC, we initially added that to roles because views needed it and then expanded to permissions with the same syntax. No idea why + is OR, agreed that looks more like an AND.

7. This patch updates the REST test coverage for terms and uses the specific edit, delete (those already existed, not sure why they were not used yet) and the new create permissions. The new overview permission does not affect REST in any way, we have #2808217: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission to focus on that part.

Ha. Initially thought that it would be pretty easy to address your points/questions but turns out that it wasn't at all, thanks for the great review, @xjm++

Wim Leers’s picture

@Berdir asked me to review this issue. I can't believe I wasn't already following it. #179 is a truly epic (EPIC!) comment pulling together many, many issues. With 87 followers, this is clearly one of the most impactful issues to fix.

  1. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -56,23 +56,37 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    // Ensure there is always an operations column.
    +    if (count($form['terms']['#header']) === 1) {
    +      $form['terms']['#header'][] = $this->t('Operations');
    +    }
    

    I don't understand why the changes below would make this necessary?

  2. +++ b/core/modules/forum/tests/src/Functional/ForumIndexTest.php
    --- a/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -42,14 +42,17 @@ protected function setUpAuthorization($method) {
    +        $this->grantPermissionsToTestedRole(['create terms in camelids']);
    
    +++ b/core/modules/taxonomy/src/TermAccessControlHandler.php
    @@ -38,7 +38,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    -    return AccessResult::allowedIfHasPermission($account, 'administer taxonomy');
    +    return AccessResult::allowedIfHasPermissions($account, ["create terms in $entity_bundle", 'administer taxonomy'], 'OR');
    

    Changes here look 👍

    (Made possible by the change in TermAccessControlHandler).

  3. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -15,6 +15,7 @@
    + *     "access" = "Drupal\taxonomy\VocabularyAccessControlHandler",
    

    👏

  4. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -204,17 +213,28 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    if ($access_control_handler->createAccess($taxonomy_vocabulary->id())) {
    

    This works, and is okay for admin routes because \Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes ensures no caching of admin responses by Dynamic Page Cache.

    But … this is not associating the cacheability metadata. I'm not a fan of adding more access checking that is ignoring cacheability metadata.

    Should IMHO be changed to

    $term_create_access =
     $access_control_handler->createAccess(…, NULL,
     TRUE);
    if ($term_create_access->isAllowed() {
      $empty = 'no term available. <a href=…';
    }
    else {
      …
    }
    $form['terms'] = [
      '#access' => AccessResult::allowed()->addCacheableDependency($term_create_access),
      '#empty' => $empty,
    …
    ];
    

    If you feel strongly that this should continue to just use booleans, fine, let's keep it like that then.

    Actually, that's probably best because it'd be confusing to have only this one bit do things correctly, and all the other access checking still using booleans.

  5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -204,17 +213,28 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    // all terms, show operations if the user is allowed to see any operation
    +    // for at least one term.
    
    @@ -260,39 +280,30 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +      if ($operations = $this->termListBuilder->getOperations($term)) {
    +        // Allow access to operations if there is at least one term with
    +        // operations.
    

    The comment says we do this for all terms if there is at least one term with operations. But \Drupal\Core\Entity\EntityListBuilderInterface::getOperations() specifically documents that it's returning the operations for the given entity. So I'm confused by this.

  6. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -204,17 +213,28 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    $change_weight_access = TRUE;
    
    @@ -260,39 +280,30 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +      $edit_access = $term->access('update');
    +      $change_weight_access &= $edit_access;
    
    @@ -322,44 +333,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    if ($change_weight_access) {
    

    I find this very hard to understand, maybe that's just me though.

  7. +++ b/core/modules/taxonomy/src/TermListBuilder.php
    @@ -0,0 +1,65 @@
    +class TermListBuilder extends EntityListBuilder {
    

    Can we get a @todo here for changing this when #2767857: Add destination to edit, delete, enable, disable links in entity list builders lands?

  8. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -80,7 +138,13 @@ public function render() {
    +    $create_access = $this->entityTypeManager->getAccessControlHandler('taxonomy_vocabulary')->createAccess();
    +    if ($create_access) {
    

    Same remark here as earlier about cacheability metadata. In this case though it seems actually appropriate to do it right?

  9. +++ b/core/modules/taxonomy/taxonomy.module
    +++ b/core/modules/taxonomy/taxonomy.module
    @@ -75,13 +75,25 @@ function taxonomy_help($route_name, RouteMatchInterface $route_match) {
    

    Wow, I didn't know permission-dependent help was a thing?

  10. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -74,7 +74,7 @@ entity.taxonomy_vocabulary.overview_form:
    -    _entity_access: 'taxonomy_vocabulary.view'
    +    _entity_access: 'taxonomy_vocabulary.access taxonomy overview'
    

    First time I see something like this, makes total sense!

Berdir’s picture

Thanks for the review.

Some responses, will work on a patch to update things later.

1. This changes the header, the code below the rows. Basically, there is a possible case where a user only has administer forums permissions then the parent form will not show any operations and will not add the operations column. That's why we have to make sure it exists. See below (point 5) for a suggestion related to that.

4. Fine with me to change that. Just like coding standards must only be done as part of the stuff that you touch anyway, the same can be said about things like this.

5. This is related to 1. If you have 10 terms, and you have acess to any operation on at least one, we show the operations column. If you have none, we currently do not show it at all. IMHO that is a super rare case, why would you give someone access to and not even also grant edit access? So we could drop that special case and just always add operations column, which in those case would be empty. Fewer special cases there and we can also drop the code for this in the forum subclass (point 1)

6. The idea here is the oposite. To be able to use the drag and drop re-order thing on the UI, the assumption is that you need to have edit access to *all* terms, otherwise you wouldn't be allowed to change its weight. Maybe this would be more readable if we convert to AccessResult and andIf() instead of boolean logic.

7. Maybe we can just get that other issue committed and then we can skip introducing this class at all :) but will add a todo.

9. Yep, crazy stuff, also as we actually combine permission and access dependant help ;)

10. Yes, spaces is a bit weird but works.

Berdir’s picture

1./5. Discussed with @yoroy. Because we don't have any other list/table that dynamically hides the operations column, we also don't need to add it here in this issue, we do enough already. This simplifes the code a bit.
4. Using access results now and also adding to the form, but I did it a bit differently by using the renderer service, imho the trick through #access but only using it to have the cacheability metadata is kinda weird.
6. Now using access result objects, this might make it a bit easier to understand?
7. Added todo, still hoping the other issue gets in soon.
8. changed to use access results.

Status: Needs review » Needs work

The last submitted patch, 189: split_administer-8.4-1848686-189.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jibran’s picture

Can't wait to convert this page to a view but first, we need draggable views in core.

Berdir’s picture

@jibran: Yeah, I fear that is not going to happen any time soon :)

This fixes the test fail, the table key only exists if there is one or no vocabulary, otherwise so far we've been setting bogus #empty key in a non-existing render array.

Berdir’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the issue summary, summary of all changes and related issues.

Berdir’s picture

Also created https://www.drupal.org/node/2902390 as a change record.

IMHO, this is ready, only possible change that I see is the removal of the term list builder if the referenced issue is committed.

larowlan’s picture

Looking good, some reviews - and tagging for review by product manager, as additional stuff to parse on permissions page has been cause for concern in past, so making sure its good on that front

  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -42,14 +42,17 @@ protected function setUpAuthorization($method) {
    +        $this->grantPermissionsToTestedRole(['edit terms in camelids', 'create url aliases']);
    +        break;
    +      case 'DELETE':
    +        $this->grantPermissionsToTestedRole(['delete terms in camelids']);
    

    nit: phpcs dictates should be empty line after break.

  2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -36,17 +39,35 @@ class OverviewTerms extends FormBase {
    +    $this->renderer = $renderer ?: \Drupal::service('renderer');
    

    is this because of the forum subclass? when would it be null?

  3. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -260,39 +293,26 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +        $form['terms'][$key]['weight'] = [
    +          '#type' => 'weight',
    +          '#delta' => $delta,
    +          '#title' => $this->t('Weight for added term'),
    +          '#title_display' => 'invisible',
    +          '#default_value' => $term->getWeight(),
    

    we're losing cacheability metadata here? shouldn't we be using #access so its added/retained?

  4. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -322,34 +342,42 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    $this->renderer->addCacheableDependency($form['terms'], $change_weight_access);
    

    Is this the only reason we inject renderer? couldn't we use #access and avoid it?

  5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -322,34 +342,42 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    if (($taxonomy_vocabulary->getHierarchy() != VocabularyInterface::HIERARCHY_MULTIPLE && count($tree) > 1) && $change_weight_access->isAllowed()) {
    

    Again, I think we're losing the cacheability metadata here

  6. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -36,16 +94,23 @@ public function getDefaultOperations(EntityInterface $entity) {
    +    if ($entity->access('access taxonomy overview')) {
    ...
    +    if ($taxonomy_term_access_control_handler->createAccess($entity->id())) {
    
    @@ -57,6 +122,11 @@ public function getDefaultOperations(EntityInterface $entity) {
    +    if ($this->currentUser->hasPermission('administer vocabularies')) {
    
    @@ -80,7 +150,25 @@ public function render() {
    +        $build['table']['#empty'] = t('No vocabularies available. <a href="@link">Add vocabulary</a>.', [
    ...
    +        $build['table']['#empty'] = t('No vocabularies available.');
    

    I think we're losing cacheability metadata here too

  7. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
    @@ -9,10 +11,206 @@
    +    // Visit vocabulary overview without terms. 'Add term' should not be shown.
    +    $this->drupalGet('admin/structure/taxonomy/manage/' . $vocabulary2_id . '/overview');
    +    $assert_session->statusCodeEquals(403);
    

    this comment seems wrong

  8. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
    @@ -62,6 +260,33 @@ public function testVocabularyPermissionsTaxonomyTerm() {
    +    $terms = taxonomy_term_load_multiple_by_name($edit['name[0][value]']);
    

    was surprised that this isn't deprecated :-O

Berdir’s picture

1. Fixed I think.

2. Just usual trying to be nice, not specifically the forum subclass but possibly something else subclassing it.

3. Setting #access only if we allow access would be incomplete anyway and we can't define the element with negative access in a table AFAIK, so no reason to bother with it sometimes IMHO. but see also 5.

4. We can't really use access because we don't want to *deny* access, just display a different message. So the only thing we could do is a fake always allowed access and inherit the cacheability metadata like suggested by Wim in #187 but I find that strange or merging cacheability ourself, which would als not be trivial. But yes, we just need renderer for that.

5. No we don't, because we add it to the form structure above. Unless we'd add partial caching on the form (would wouldn't make sense :)), it doesn't matter where on the form we add the cacheability metadata.

6. In operations yes, but that's not possible yet: #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons. None of the parent methods add any cacheablity metadata either. And it's fairly theoretical for now, as we don't actually cache admin routes yet and have the permission default cache context.

7. Fixed.

8. Yep, but it probably will be at some point, so updated to use loadByProperties() directly.

dawehner’s picture

These all feels quite nitpicky, sorry :(

  1. +++ b/core/modules/forum/src/Form/Overview.php
    @@ -58,21 +58,30 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +            ]
    

    Let's have a trailing comma :P

  2. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -204,17 +226,28 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', ['@link' => Url::fromRoute('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])->toString()]);
    ...
    -      '#empty' => $this->t('No terms available. <a href=":link">Add term</a>.', [':link' => $this->url('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])]),
    

    Is there a reason we went from :link to @link?

  3. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
    @@ -322,34 +342,42 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
    +    if (($taxonomy_vocabulary->getHierarchy() != VocabularyInterface::HIERARCHY_MULTIPLE && count($tree) > 1) && $change_weight_access->isAllowed()) {
    

    When you want to add the additional () around the first condition group we could also !==

  4. +++ b/core/modules/taxonomy/src/TaxonomyPermissions.php
    @@ -48,19 +49,30 @@ public static function create(ContainerInterface $container) {
    +   * @param \Drupal\taxonomy\Entity\Vocabulary $vocabulary
    ...
    +  protected function buildPermissions(Vocabulary $vocabulary) {
    

    Is there a reason we don't use the interface here?

  5. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
    @@ -80,7 +150,25 @@ public function render() {
    -    $build['table']['#empty'] = t('No vocabularies available. <a href=":link">Add vocabulary</a>.', [':link' => \Drupal::url('entity.taxonomy_vocabulary.add_form')]);
    ...
    +        $build['table']['#empty'] = t('No vocabularies available. <a href="@link">Add vocabulary</a>.', [
    +          '@link' => Url::fromRoute('entity.taxonomy_vocabulary.add_form')->toString()
    

    Another string change from @link to :link without a need?

  6. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
    @@ -9,10 +11,204 @@
    +  function testTaxonomyVocabularyOverviewPermissions() {
    

    Let's add "public" here.

Berdir’s picture

Thanks, some of those were very non-nitpicky, the @link thing was a wrong merge I guess.

dawehner’s picture

6. \Drupal\user\Access\PermissionAccessCheck, + is OR, , is AND. IIRC, we initially added that to roles because views needed it and then expanded to permissions with the same syntax. No idea why + is OR, agreed that looks more like an AND.

I think I can explain where this confusion is coming from.

In views you kinda allow people to select items from let's say, a certain taxonomy tag. In this case you want to use "+" for OR, so people get items from both tags. At least for me it would be super nice to use "&&" and "||" or so, as there is no way for ambiguity.

Berdir’s picture

The last submitted patch, 200: split_administer-8.5-1848686-200.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 200: split_administer-8.5-1848686-200-interdiff.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review
FileSize
39.41 KB

Damn, I knew it was a mistake to suggest something like that, messed up the patch. Diffed against an older commit.

Status: Needs review » Needs work

The last submitted patch, 203: split_administer-8.5-1848686-203.patch, failed testing. View results

Berdir’s picture

Do need to explicitly specify the default list builder.

Status: Needs review » Needs work

The last submitted patch, 205: split_administer-8.5-1848686-205.patch, failed testing. View results

Wim Leers’s picture

Great progress here! I especially like the simplification you did in #189 wrt #188.5 :)

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

I think the patch is in a stage that we can get the product manager involved.

zwerg’s picture

+1 for this feature, need it soon. Is there a plan when it would be applied to the core?

Wim Leers’s picture

Glad to hear you'd benefit from this too, @zwerg! It's marked Reviewed & Tested By the Community. That means it can be committed any time now — a Drupal core committer will do a final round of review, and if they don't have remarks, it will be committed. So we just need to be patient now until a core committer has time.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs product manager review +Needs usability review

@webchick has asked that we go first to the usability team for anything that's less impactful than adding, like, an entire module, so doing that.

FWIW I'd personally be comfortable making a call on this permission myself (I've gotten decent at the permissions-devil's-advocate game), but backing @larowlan's play here and setting NR for usability review in lieu of product manager review. Happily, the usability team convenes weekly nowadays. :)

xjm’s picture

Issue tags: +Needs screenshots

Oh, and to that end, screenshots of all the user-facing changes would be good. Of the overview, before and after, and before-and-after of the permissions page with multiple vocabularies and the new overview and create permissions.

Also, is there any change of splitting e.g. the forums changes out into a blocking issue instead so this issue isn't quite so overwhelming?

Berdir’s picture

Issue tags: -Needs screenshots

There are no visual user facing changes except in one edge case, when you have the permission to see the term overview page but have no edit/delete permissions, then you have an empty operations column. I already discussed that part with @yoroy earlier and he confirmed that this is OK. To be correct, he'd prefer if it would not be like that, but it is the same for all views based lists as well, e.g. not being able to edit any nodes but seeing the overview has the same affect.

And I don't think we need to have a screenshot of the added permissions?

I'm also not sure what exactly should be discussed with the usability team, the only actual change here is the introduction of those permissions, so the only thing we could discuss is the labels and descriptions of the access overview and create new terms, which are AFAIK in line with e.g. the node permissions. And whether or not that is the right approach in general is I think not really a usability question?

The patch is so big and touches so many thing to make sure that we do *not* introduce any visual or functional changes except respecting the new permissions. I'm also not sure it makes sense to split up the e.g. forum changes, that part could in theory be moved, but it's actually a pretty small part of the patch and it wouldn't really make sense on its own as it changes nothing.

yoroy’s picture

  1. What are the new flows that get enabled with this patch (this is outlined in the issue summary)
  2. Which (new?) screens or additions to existing screens are part of these flows?

For 1, I think the functionality that's being refactored/extended here makes sense and these changes are clearly outlined in the issue summary. So that's fine.

For 2, quickly scanning the patch I see quite a few t() things that add new user interface texts, so some screenshots of what people get to see or get told would indeed be useful.

Berdir’s picture

FileSize
20.46 KB
16.48 KB
22.12 KB
12.39 KB
22.66 KB
10.96 KB

Ok, here we go, screenshots of the screens that have slightly different texts. Basically, it's just the help and empty texts. There are of course also various variations of those, e.g. any combination of add/edit/delete permissions for the given vocabulary.

To test this, I created a editor role that has access to see the help text and only the access taxonomy overview, then created an editor use with that role.

Admin: Empty vocabulary list

Editor: Empty vocabulary list

Admin: Empty term overview

Editor: Empty term overview

Admin: Non-empty term overview

Editor: Non-empty term overview

Berdir’s picture

yoroy’s picture

Issue tags: -Needs usability review

Thanks @berdir, it was not obvious to me that these are only variations on existing patterns. No biggie: "Tags contains the following terms." does not really add useful information and could be left out.

Happy to see these more fine grained permissions coming to core :)

Wim Leers’s picture

Does #218 mean this is back at RTBC?

Berdir’s picture

IMHO yes, there is nothing that needs to be changed and it has been approved.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Alright, let's do this then!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3529e20 and pushed to 8.5.x. Thanks!

  • catch committed 3529e20 on 8.5.x
    Issue #1848686 by Johnny vd Laar, Berdir, JeroenT, geertvd, mogtofu33,...
miro_dietiker’s picture

Awesome! FYI Patch 205 still perfectly applies to 8.4.x, with a small conflict in a test for 8.3.x.

Manuel Garcia’s picture

Awesome, thank you all!

Very glad to hear this change is helping out on that side @Wim Leers :D

Johnny vd Laar’s picture

Wow, glad to see this finally in. 5 years after I added the first patch :D

xjm’s picture

Added some minor reviewer credit,

DuaelFr’s picture

Thank you @xjm!

zwerg’s picture

Does this work in 7.x core too?

Wim Leers’s picture

@zwerg: No, this only applies to Drupal 8. If you want to see this happen for Drupal 7 too, please create a new issue. 🙂

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

idebr’s picture

This patch introduced visual regressions and invalid markup on the vocabulary listing and taxonomy term listing pages. This issue is being fixed in #2920672: OverviewTerms page has invalid table HTML when the user does not have access to some terms

DuaelFr’s picture

Issue tags: +8.5.0 highlights