Add a select terms permission

Thank you very much for your patch. Unfortunately, due to some unexpected circumstances, I have been away since January and didn't have time to look at this in depth, yet. Also, it might take some more time until I can actually look at this. Just some general remarks for now, to give you a heads up:

1. I think this might be a useful feature, so I'm open to adding it to this module.
2. We would need test coverage for the new functionality to make sure that we will not break it in the future and that we don't introduce any security issues.
3. Since we are introducing a new permission, we would need to make sure to provide an upgrade path for existing users of the module so that nothing breaks for them. This might be as easy as giving all roles with the permission to view terms the permission to select terms in an update hook. We would also need test coverage for the upgrade path.
4. I'm going to release the final release on the 8.x-2.x branch and the first release on the 8.x-3.x branch very shortly. So the new feature probably won't make it into 8.x-2.x anymore. The patch would have to be ported to 8.x-3.x.
5. The patch would need to be compatible with Drupal 8.9 and 9.0.

Let me know, if you would like to do this. If you feel uncomfortable writing tests or an upgrade path, you could start by just porting the patch to 8.x-3.x and then maybe someone else from the community could add the test coverage and/or the upgrade path. Once you have a patch for 8.x-3.x ready, I'll be happy to review it as soon as possible and give you more detailed feedback. Should you instead decide to release a separate module, please post a link to the module here, so that users interested in this feature will be able to find it.

Thanks for the module!
I have a similar problem to solve: we have unpublished terms and a non-admin user should be able to select those terms without giving the permission to "Administer Taxonomy".
Therefore the following permissions would be helpful:

• Select only published terms
• Select published and unpublished terms

The code I am refering to is located here: \Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection::getReferenceableEntities()

Attached is a patch against 4.x-dev that introduces 4 new permissions:

• select any published term
• select any unpublished term
• select published terms in VOCABULARY
• select unpublished terms in VOCABULARY

The patch will replace the default entity reference selection plugin for Taxonomy terms with an extended version that requires the above permissions. This allows users to just (un-)install the module and instantly switch on/off the new permissions without the need to reconfigure entity reference fields.

The patch also adds an update path that grants the permission to select published terms to all roles with the permission to view published terms, which would have been required to do this previously.

Thanks @FeyP!

I did a functionality test with the following permissions:

• select any published
• select any unpublished
• Select unpublished terms in Regions

and it works as expected. :)
Appreciate your work!

This is not ready. The patch doesn't apply against the current 4.x-dev. It needs a reroll.

I re-rolled a new patch 3110558-13.patch against the latest 4.x-dev. It is the same implementation of the patch 3110558-08.patch in #8.

The code is also available in the branch 3110558-add-a-select-terms-permission of the issue fork.

I used this simpletest.me instance https://master-lfne2jk9mqututivivrssptrv3mpgvgo.tugboatqa.com for testing the patch in D9.3, if you want to try it out.

I've re-rolled the faulty patch in #14 and attaching the new one in 3110558-18.patch.

This patch fixes all the automated tests including all the select term permission asserts, and also refactor the functional update test to check the schema updates 9401 for view term label per VOCABULARY, and 9402 for the select term per VOCABULARY.

The patch is also available in the merge request 3110558-add-a-select-terms-permission of the issue fork.

We have tested https://www.drupal.org/project/taxonomy_access_fix/issues/3110558#commen... and this works for us.
Can this be merged?

Thanks for working on the reroll @technoveltyco. It is much appreciated.

@dunx

Can this be merged?

At some point this will be merged, it is on my todo list. I just need to get to it ;). Originally I planned to work on Drupal 10 compatibility before getting back to this, but since we now have a reroll, I think we'll get this in first and then look at Drupal 10 compatibility so that we don#t need another reroll.

Thanks for testing the MR/patch and reporting back in this issue that it works for you. This definitely helps to build confidence that we have a solid solution. What we still need is a review of the code and especially the test coverage. Since we're dealing with permissions and access checks we can't be too careful ;). I'll do a final review myself before I commit this, but it helps to get the issue merged faster, if other users will do a code review. Except for the test coverage, where it is easy to introduce some copy/paste error and perpetuate a bug, which in this case could have security implications, I'd be interested in opinions whether my design decision to replace the default handler is a good idea or if there would be a better approach.

So ultimately, we need to get this issue to RTBC status. If no other community members will do the review, I will at some point still review this, but it might take longer.

Thanks for the re-roll. I now had a chance to review this more closely. It looks good in general, but there are some things we can improve.

+    elseif ($operation === 'select') {
+      return $entity->isPublished() ? [
+        "{$operation} terms in {$entity->bundle()}",
+        "{$operation} any term",
+      ] : [
+        "{$operation} unpublished terms in {$entity->bundle()}",
+        "{$operation} any unpublished term",
+      ];
+    }

I think we can simplify this. The new code for the select operation would also work for the view operation. So we can use that for both operations and just have an else for the "view label" operation.

+      $this->assertTermAccess($published_terms[$delta], 'view label', FALSE, "The 'view term names in {$this->vocabularies[$delta]->id()}' OR 'view any term name' OR 'administer taxonomy' permission is required.");
+      $this->assertTermAccess($unpublished_terms[$delta], 'view label', FALSE, "The 'view unpublished term names in {$this->vocabularies[$delta]->id()}' OR 'view any unpublished term name' OR 'administer taxonomy' permission is required.");

You added this block to the beginning of the foreach loop for Test the per vocabulary 'select terms in' permission. in TermAccessTest. This is correct, but we need to add the same check for the other newly added select term permissions tested further down.

+    $this->users['select_any_unpublished_vocabulary'] = $this->drupalCreateUser([
+      'select unpublished terms in ' . $this->vocabularies[0]->id(),
+    ]);

This error was already present in the original patch. The permission name should be 'select any unpublished term'.

-class Update9401Test extends UpdateTestBase {
+class UpdateNTest extends UpdateTestBase {

Coder doesn't like these class names.

-  protected function runUpdate(int $previous_schema, int $target_schema, string $raw_message) {
+  protected function runUpdates(int $previous_schema, int $installed_schema_expected, array $target_schemas_available) {

I think we can just get the expected schema from the available schemas now, so we can probably remove the 2nd parameter.

+    // Assert target updates are available.
+    foreach ($target_schemas_available as $target_schema => $raw_message) {
+      $this->assertSession()->responseContains('taxonomy_access_fix module');

I think we can move this out of the loop, should be okay to just assert this once.

-        drupal_get_path('module', 'migrate_drupal'),
+        $extension_path_resolver->getPath('module', 'migrate_drupal'),

Thanks for replacing this deprecated code. However, KernelTestBase has a method
getModulePath() that we can use here as a replacement, so no need to get the
extension.path.resolver service.

The previous patch had a change to .info.yml, which is lost in the new patch:

-core_version_requirement: ^8.8 || ^9
+core_version_requirement: ^9.3

I don't remember now why it was needed, but I've certainly done that for a reason, so I think we need to add this again.

The re-roll also introduces some white space changes Coder doesn't like that we need to fix.

I'll work on a new patch.

Attached is a new patch that addresses #22.

Just did another full review of the code and I found one other thing we should address. There's a place in TermSelectAccessTest where we're testing that permissions without access to select terms can't select any terms. We should have added the new view term names permissions there with the re-roll and then the reorder permission was also missing there. So let's add those. Didn't find anything else.

I have done a final review in #25 and did some manual testing on Drupal 9.4.5 and it looks like everything still works as intended. Given that the main part of the functionality (the replacement of the TermSelection plugin) hasn't changed since the original patch and the feature was tested successfully by @shortspoken and @dunx, I'll mark this as RTBC.

Committed. Thanks all!