Domain Access: Expand node_grants support for update and delete operations

Updated the patch to include the delete operations.

This is the domain_access_node_access_records() hook where the access is declared for grant_update and grant_delete, but not actually referenced in the domain_access_node_grants().

[EDITED]

My longstanding opinion has been that hook_node_access_records() does not apply to unpublished content, since that API is designed to create "lists of content that a user can view," but we do account for "view unpublished nodes."

So this looks like a valid use-case with one caveat: we can't trust queries to tell us what we can edit.

This is largely due to the fact that any arbitrary module can deny edit and delete actions using hook_node_access() and without writing to the node_access table. That inherently makes any query for "what nodes can I edit or delete" untrustworthy.

[EDITED]

It's been a while since I looked at this code. We account for the 'view an unpublished node' case, but not edit or delete (for the reasons noted above).

 'realm' => ($translation->isPublished()) ? 'domain_id' : 'domain_unpublished',

This needs tests, and I am surprised to see that we don't have any for the existing case.

It's probably worth noting why this happens with ACB.

We handle these checks inside domain_access_node_access() since you can't run list queries for "things I can edit or delete".

ACB is doing some complex AND logic around grants that I don't fully follow and not getting the grants for unpublished nodes as it would expect.

We handle these checks inside domain_access_node_access() since you can't run list queries for "things I can edit or delete".

I think that assumption is precisely why the patch is necessary 😅, we're using a custom views Filter which filters based on content which the user can update.

This works fine in a vacuum since node access is granted on an OR basis.

However, once the ACB and domain modules are installed together, the node access becomes an AND, which suddenly makes the 'grant_update' and 'grant_delete' declared by domain_access now relevant.

I unfortunately don't have time at the moment to create a proper test case, however the pseudo-code for the test would be something along the lines of this to showcase that the results are as expected.

use Drupal\Core\Database\Database;

// Test that the user can access nodes which have been specifid as being updatable.
$account = $this->drupalCreateUser([
  'administer nodes',
  'edit domain content',
  'publish to any domain',
]);
$query = Database::getConnection()->select('node', 'n')
  ->fields('n');
$query->addTag('node_access');
$query->addMetaData('op', 'update');
$query->addMetaData('account', $account);
$result = $query->execute()->fetchAll();

Patch/MR branch is outdated.

Rebased against the latest dev.

Patches must be set to Needs Review to trigger tests.

I've rerolled this patch to work with 2.0.x

Additional note: This patch fixes an issue i was having of not being able to edit unpublished nodes.

Hi @lincoln-batsirayi, could you create a new MR for the 2.0.x branch? That would be great.

Hi @mably this was my first time ever doing a MR so there's a chance I’ve done something wrong but i did try my best to follow the instructions in all the available documentation.

The main thing to note here is that the original MR in gitlab is still pointing at `8.x-1.x` because that's when this issue created, so i think that may need to get updated? Now this might be wrong but inside the branch for this issue i merged in 2.0.x (since i want these changes available in there) and then i added in the changes for this patch and pushed those too but i don’t know if this was the correct thing or not, if it wasn’t please let me know what i should've done.

I changed the MR's target to 2.0.x. Looks like a rebase is still needed though.

I tried to do the rebase but unfortunately it looks like i don't have permission force push the branch because i didn’t create the original fork issue, so I'm not really sure where to go from here... any ideas?

Can you try to create a new branch/MR then?

Okay i think that's done now @mably, my apologies for the sh*t show on this, it was my first time with this work stream / workflow but it was invaluable to learn so thanks for that :)

Hi @lincoln-batsirayi, it looks like some tests need to be updated.

Working on it!!

Please review!!

@divyansh.gupta check the Gitlab CI pipeline results. Still some warnings.

And PHPUnit job is failing with this in the logs:

Drupal\Core\Test\Exception\MissingGroupException: Missing @group annotation in Drupal\Tests\domain_access\Functional\DomainAccessUnpublishedGrantsTest in /builds/project/domain/web/core/lib/Drupal/Core/Test/TestDiscovery.php:342

Remember to check that the tests pass all fine before setting to "Needs review". Thanks.

Now all tests are running green,
Please review!!

Thanks a lot @divyansh.gupta, I'll have a look at it.

Let's try to add this to the next release.

Hi @divyansh.gupta and @mably sorry for being so late in coming in on this, I’ve just realised we’ve got a bug on our site related to this issue, essentially I’ve tracked it down to specific code blocks which were removed when the code for this issue was merged, the specific commit I'm talking about is this:
https://git.drupalcode.org/project/domain/-/merge_requests/151/diffs?com...

Please can i ask why that code was removed? The commit just says it was to fix tests which doesn’t give me much

@lincoln-batsirayi looks like some tests were failing as far as I understand it.

May be @divyansh.gupta can explain why he did that.

For now can you create a new issue with an MR with tests for the missing part?