permissions_by_term_node_access too authoritative on unpublished nodes

We have been authoritative in this function, because there were several issues with giving giving access to node view for anonymous users, if the node has not been published. AccessCheck::handleNode() is not taking care of anonymous users.

Additional quirks I'm not quite sure why it's in the code: method_exists($node, 'id').

Yes, you are right. This method does not make sense. It was just a mistake, which current does not "hurt".

Probably
1., it's enough to check if $account contains just the anonymous role and return afterwards AccessResult::forbidden().
2., So the first if-case could be simplified.
3., The logic should be moved to the AccessCheck service.
4., Then there could be developed an KernelBase test to prove the functionality with several cases.

This function is crucial to make PbT work and not cause a serious security leak for > 1000 Drupal sites!!!

This issue is no bug. It is an improvement. If you are willing to work on it and you provide a patch for this, I would review it and commit to PbT on success.

Even if it's not that restrictive, Pbt still has other issues when working with unpublished content managed by other workflow modules such as Content Moderation or Workbench Moderation. Pbt implements node_grants whereas the others don't. Without bypass content access permission, node_access_view_all_nodes() always checks for node grants in node_access table if there is a module implementing node_grants. Pbt does not provide any grant for unpublished nodes. Non-admin users are not able to edit unpublished nodes if Pbt module is enabled.

Context:
* Enabled modules: Content/Workbench Moderation, Permissions by Term
* Users with permission access content overview cannot see unpublished content in /admin/content view
* Users with permissions view own unpublished content and view any unpublished content cannot view draft content.
* Users with permission update any content cannot edit unpublished content. They can only edit published content.
* Users with permission bypass content access don't get the above restrictions.

Changing issue category back to "Bug report"; disallowing all editors (except those with "bypass node access") from editing draft content—even their own that they just created!—certainly can't be acceptable behaviour, can it? This module is a no-go for any sites that use moderation, or indeed where any content might exist in unpublished state. I understand that this approach has fixed other problems for you, but it has done so in the wrong way and at high cost.

Bumping priority

Updating associated version.

How about adding `!$node->isDefaultRevision` to the if statement on line 261?

function permissions_by_term_node_access(NodeInterface $node, $op, AccountInterface $account) {

  if (method_exists($node, 'id') && ($op == 'view' OR $op == 'update' OR $op == 'delete')) {
    if (!$node->isDefaultRevision && !$node->isPublished() && !$account->hasPermission('Bypass content access control', $account)) {
      $eventDispatcher = \Drupal::service('event_dispatcher');
      $accessDeniedEvent = new PermissionsByTermDeniedEvent($node->id());
      $eventDispatcher->dispatch(PermissionsByTermDeniedEvent::NAME, $accessDeniedEvent);

      return AccessResult::forbidden();
    }

    /* @var \Drupal\permissions_by_term\Service\AccessCheck $accessCheck */
    $accessCheck = \Drupal::service('permissions_by_term.access_check');

    return $accessCheck->handleNode($node->id());
  }
}

Is now fixed in 8.x-1.43: https://www.drupal.org/project/permissions_by_term/releases/8.x-1.43

I have removed the entire part which was blocking access for unpublished nodes. This must have been implemented in a previous version of the AccessCheck service. Before it probably was not returning AccessResult::neutral(), if PbT wasn't handing the node because of no relation to a restricted taxonomy term.

So PbT will not block you in your content moderation process.

I have been testing this change - but - please (please!) be so kind and test this release carefully and report any issue related to this change! Feel free to re-open this issue, if you have any objections.

@peter-majmesku, I've just tried 1.43 and I still have the same issue. I have a user with edit permissions on a content type that has a permissions by term taxonomy field on it. The user has the permission. If I create a new node as an administrator, I cannot edit the node with a non administrator user (a user without the Bypass permission).

Have you tested this change with the Workbench moderation module? Are there special permissions combinations or settings that are required?

There is one setting for single term permission inside PbT. You can reach it via configuration. Have you used it?

No ability to edit an node: you have no terms set on this node?

The issue is not from Pbt itself, but instead from the core Node Grants system. Amongst Pbt, Workbench Moderation (WbM), and Content Moderation (CM); Pbt is the only module implementing hook_node_grants() (see #4). When there is an implementation of hook_node_grants(), all access checks will refer to the node_access table. Pbt does not provide a grant for unpublished nodes, and neither CM/WbM do. Thus, accessing to unpublished nodes will always get access denied without the bypass content access permission.

To make Pbt work with CM/WbM, I have a workaround by implementing node_grants for unpublished nodes.

/**
 * Implements hook_node_access_records().
 */
function my_module_node_access_records(NodeInterface $node) {
  if (!$node->isPublished()) {
    // No module implements this hook for unpublished nodes, so we do.
    $grants[] = [
      'realm' => 'my_module',
      'gid' => $node->id(),
      'grant_view' => 1,
      'grant_update' => 0,
      'grant_delete' => 0,
      'nid' => $node->id(),
    ];

    return $grants;
  }
}

/**
 * Implements hook_node_grants().
 */
function my_module_node_grants(AccountInterface $account, $op) {
  if ($op == 'view') {
    $query = \Drupal::database()->select('node_access', 'na')
      ->fields('na', ['gid'])
      ->condition('na.realm', 'my_module');

    $gids = $query->execute()->fetchCol();

    foreach ($gids as $gid) {
      $grants['my_module'][] = $gid;
    }

    return $grants;
  }
}

/**
 * Implements hook_node_access().
 */
function my_module_node_access(NodeInterface $node, $op, AccountInterface $account) {
  if (!$node->isPublished() && $op == 'view') {
    $content_moderation_enabled = \Drupal::moduleHandler()->moduleExists('content_moderation');
    $workbench_moderation_enabled = \Drupal::moduleHandler()->moduleExists('workbench_moderation');
    if ($content_moderation_enabled || $workbench_moderation_enabled) {
      $access_result = AccessResult::allowedIfHasPermission($account, 'view any unpublished content');
      if ($access_result->isForbidden()) {
        $owner_access = AccessResult::allowedIfHasPermission($account, 'view own unpublished content');
        $owner_access = $owner_access->andIf(AccessResult::allowedIf($node->getOwnerId() == $account->id() || $node->getRevisionUserId() == $account->id()));
        $access_result = $access_result->orIf($owner_access);
      }
      return $access_result->addCacheableDependency($node);
    }
  }
  return AccessResult::neutral()->addCacheableDependency($node);
}

It'd be good if Pbt can include a submodule implementing the above hooks.

This patch added the above hooks to a submodule permissions_by_term_moderation.

Thanks. I will check this on coming weekend.

It is crucial, that users without the "Bypass content access control" are able to edit unpublished nodes.

There should not be an extra module necessary for this, which dependency to the content moderation or workbench moderation. This should be implemented in the base module.

Right now I am not sure, if it has even anything to do with node access records. Because I have commented out

permissions_by_term_node_access_records()

and the unpublished node was still unaccessible. Have you checked which function of PbT disallows the access to unpublished nodes?

> Thus, accessing to unpublished nodes will always get access denied without the bypass content access permission.

I gave an user all node permissions except the "Bypass content access control" permission. The PbT module was uninstalled and all permission rebuild at /admin/reports/status/rebuild. What was the result on accessing the node with this user?:

• On node view (e.g. /node/4) I got a "Access denied page"

Then I was enabling PbT and then I have rebuild the permissions. I was able to edit this node, without having set any term permissions via PbT. Afterwards I have added term permissions and gave my user the permission for this node via them: I was able to edit the node. Then I have removed permissions from this user via terms: I was still able to edit this node with this user.

PbT is not "too" authoritative anymore since the commit from https://www.drupal.org/commitlog/commit/69545/5c74ed084dbf16df99fe3a27be....

I will not add any extra module for workbench moderation or content moderation. Please create an new project on Drupal.org for this. This goes quick: https://www.drupal.org/node/1068942. Let me know, when you have created it. Then I will link from PbT's project page to your project.

I am closing this issue, since PbT works as designed. Please feel free to re-open this issue, if you have any objections.

> I am happy to expand the code

Great! As I have written, please create an new module project for that, which has the Permissions by Term module as dependency: https://www.drupal.org/node/1068942.

I am not going to support the content moderation and workbench moderation modules. I do not need them. Also I do not integration a submodule for this into the Permissions by Term module. If you need this module: please create an new module project for it. I will link to it. You can also ask me questions regarding PbT via issues.

> I am happy to expand the code, but I'm unsure where I should be working.

Permissions as controlled in hook_node_access() and hook_node_access_records() in the modules/permissions_by_term/permissions_by_term.module file. Also the KernelEventListener class is restricting access for nodes. Check the onKernelRequest method there.

The Permissions by Term module is based on node access records. Make sure you understand how they work: https://api.drupal.org/api/drupal/core%21modules%21node%21node.api.php/f...

> I tried setting a break point in the PermissionsByEntityKernelEventSubscriber from the permissions_by_entity submodule

The permissions_by_entity submodule is only handling permissions on custom entities and it is experimental. The Permissions by Term module is handling access to Drupal nodes (node entity type - shipped with Drupal core).

If you need support for the content moderation or workbench moderation module, please create:

1. New issue, which is aiming exactly on this modules
2. A Drupal instance at Simplytest.me, where I am able to reproduce your issue
3. A step-by-step list which describes what you have done (a simple step-by-step LIST - no paragraphed text)

Has anyone made a module that supports Permissions By Term with Content Moderation moderation support, this being in Core now on 8.4 and later?

Just ran into this bug on a fresh 9.4.1 and PbT 3.1.17 install. On install, non-admin users lost all access to own unpublished content in views, node/edit, etc. Tried flushing caches, rebuilding permissions, etc, but no change.

Uninstalling PbT cleared the bug and restored access own unpublished content functionality.

Unfortunately, this makes PbT unusable for us, whereas it could have been a huge help in our publishing model.

Happy to contribute troubleshooting and testing time, when a real fix for this issue is available.

Thanks

We're in a similar position to @higherform

Drupal 9.4.5, PBT 3.1.19

Users cannot see unpublished content in views listings, node/edit.

Unfortunately, we'll need to look for another solution to our use-case for now.