domain_access_node_access ignores node access records

@agentrickard,

Here, we want to act on unpublished nodes and provide additional granularity for node editing permissions... In this case, these would be considered super-permissions, similar to 'edit any page content'.

This is not quite so, according to the code, as there's another check:

  if ($op == 'view' && $manager->checkEntityAccess($node, $account)) {
    /** @var \Drupal\user\UserInterface $user */
    if ($node->isPublished()) {
      $allowed = TRUE;
    }

which allows access by just making sure a node is published and user has access to the domain(s) the node belongs to. Isn't it already checked for via core's node access and domain_access's implementation of the two node access hooks? Why do we override it? I would understand your argument if it only applied to $account->hasPermission('view unpublished domain content') which can be turned off via permissions, but the first condition is defeating the purpose of node access system IMHO.

If some other module returns FALSE, then access should not be granted.

And by "some other module" you mean a custom module, since only custom modules can be made aware of what domain_access is doing and take counter-action? Again, I think this is a wrong approach, and if Drupal core is made this way, we need to initiate a broader discussion on node-access based ecosystem of modules.

I have a feeling something is definitely not right here. A Drupal developer needs a clean way to combine various access control mechanisms relying on node access in a way no overhead is done on every node access check. After all, the whole idea of having "locks and keys" is to avoid unnecessary computations (node access records + hook_node_acces) during run-time and only calculate the run-time value which is user-dependant (i.e. user grants).

Would be good to have acb's, pbf and other node access implementations work nicely together and this is what hasn't been covered and hasn't changed with D8's release, unfortunately.

@agentrickard, thanks for replying back, it seems I understand the rationale behind the code in domain_access much better now.

Few considerations anyways:

hook_node_access() is designed to override the Node Access system at the page level. Node Access (and modules like DA, ACB, etc.) only work on lists.

My interpretation is a bit different. hook_node_access is definitely the only possible way of overriding access control logic, and it is really important. However, the logic is well known and includes 2 steps:
- apply any overrides via hook_node_access
- if no decision has been taken, proceed to the standard node grants check

For performance reason hook_node_access is not called on each node to be shown in a list as the result of a SELECT query, however, it is not a replacement of any kind to node grants check. Both are parts of the same mechanism, and one may expect an identical behaviour regardless whether it was a single node operation (view/update/delete) or a SELECT query.

Because hook_node_access works at the individual node level, we can _never_ trust the {node_access} table to return a list of editable content. Core node module breaks this in node_node_access().

First of all, I doubt the core node module breaks {node_access} table. This is simply impossible since {node_access} can only be used by a "node access module", i.e. a module like domain_access, acb, pbf etc. node module itself doesn't define any grants apart from the global "anyone can view everything", as it doesn't have the 2 hooks and it's own node_access_grants function is really nothing but a way to alter node grants and add a default one "anyone can view everything". Btw, this function answers the question about how to integrate 2 or more node access modules:

After the default grants have been loaded, we allow modules to alter the grants array by reference. This hook allows for complex business logic to be applied when integrating multiple node access modules.

which relates to these lines:

  // Allow modules to alter the assigned grants.
  \Drupal::moduleHandler()->alter('node_grants', $grants, $account, $op);

The above comment is a straightforward guideline saying "you are welcome to use node grants in a way to make everyone happy" and it doesn't restrict to hook_node_access usage only.

All that said, do you have a suggestion for how to solve the View/Edit/Delete dilemma -- aside from adding to grant storage?

Why adding to grant storage isn't a good idea?

I don't think that running two Node Access modules at the same time is ever a good idea. There are too many things that can go wrong because the grants system is OR-based.

It is a necessity if one wants to save time and benefit from what this open source world can give!

For ACB that would mean running the Node Access check inside an implementation of hook_node_access(), though you still have to deal with the issue of the permissions set in node_node_access().

I'm not really following what you mean here. ACB doesn't have its own implementation of hook_node_access() simply because it doesn't care (and you can't blame it for that!) about what others do with access control, it only operates on grants which is a clean and nice idea IMHO.

My interpretation of the API design is that calling the node grants system inside hook_node_access() is a bad idea.

I think it all depends on the circumstances. Normally you only use hook_node_access as an "ultimate say" in a decision, in case you either don't have grants defined for some reason, or you want to enable some configuration. I agree however that any custom alterations aimed at "marrying" two node access modules must be done in a custom integration module, but in my ideal world, it should only deal with hook_node_access implementations overrides. The reason is simple: node grants via {node_access} alterations are already covered by ACB.

On a separate note, ACB does have some code to integrate with domain_access which is not in hook_node_access:

function acb_node_grants_alter(&$grants, AccountInterface $account, $op) {
  ...
  // For our ACB grants, we need the 'Edit any domain content' and 'Delete any domain content' permissions to be enabled to
  // enable other modules to control it effectively (due to the AND-logic we're implementing).
  // Add them to the user grants if not already present if Domain Access is active.
  // Additionally, fork $grants so we don't add them permanently to the referenced grants array.

Finally, and very important: I see this integration module's task pretty complex if not impossible when it comes to hook_node_access as a way to override undesired behaviour. Let's say domain_access grants access where it shouldn't. The custom module now has to consider this, and "undo" the "allow" by "denying". But what if the custom module doesn't want to deny, it just wants to disable the "allow" by turning it into "I'm neutral" so that {node_access} could be used? Or what if it needs to allow access while it has been denied - no allows will be considered at all if there's at least one deny.