Restructure node access grant behavior into the node access handler

It would be so nice having this going forward. The hook_node_grants is giving me so much pain these days.

Maybe i should open a new issue complaining about this bug i found out, but to be honest, i don't have ideas on how that should be solved and at the moment i think this could be the proper place where to make sure this bug is taken care of.

Bug Explanation:

• install drupal and translation modules, make the status field translatable on a certain content type
• create a new node of that type, leaving the original node unpublished. Translate it in one language, publish the translation
• visit the nodes as anonymous user. You'll be able to access the published translation and you'll get a 403 on the unpublished original translation
• install a custom module which implements the hook_node_grants hook. It can be completely empty.
• the permissions will be re-generated
• now enjoy the bug: as anonymous you'll be able to access both nodes when you save the published translation. If you save the original node (unpublished), then you won't be able to access neither of the nodes

This happens because NodeGrantDatabaseStorage::write is put to work whenever a module implements the node_grants hook and at that point it doesn't take at all the status of the translation into account.

Is anyone pushing this forward? Should i try writing a patch for what i think is a problem in 9.4, or should i create a different issue for this?

Triaged and this is still relevant.

Postponed on #3038908: Deprecate node_access_view_all_nodes()

Re-titling this back to a ticket about the node access API in general.

node_access_grants the function is being deprecated in #3038908: Deprecate node_access_view_all_nodes()

This is being rescoped again based on https://www.drupal.org/project/drupal/issues/3038908#comment-16174314

Another thing to consider here is that @catch has pointed out there is a lot of complexity in \Drupal\node\Cache\NodeAccessGrantsCacheContext() that probably belongs in the API

The other issue is in, so this isn't postponed anymore.

I've cleaned up the issue summary, because actually hook_node_grants() does return grants within realms, and thus it's not an 'info hook'.

I've really narrowed the scope of the issue for the specific cleanup of essentially being able to make the ideological change from procedural code in the .module file to the exact same code, in a class. Which makes this task pretty straightforward, assuming that the NodeAccessControlHandler is the right place to put that code.

I've then punted further change into a follow-up, since there's no real point in changing how the node access API works if it's all going to get replaced with a generic entity access API...talking of which, does anyone know where that initiative is?

Thank you for taking this! I'm interested in this as part of the module file elimination initiative.

Adding back the tag since we normally keeping historical tags like that.

I feel like this issue should stay as the "restructure..." and we should just create a new issue to deprecate node_access_grants so things don't get confused.

Hmmm...but the original IS: https://www.drupal.org/node/2473041/revisions/8368653/view was actually mostly about moving the code and deprecating node_access_grants, with then massive bit for essentially changing the two hooks for gathering records and grants into a plugin. So I'd argue this is is the right issue to do the deprecation in, and then actually keep it laser focused on that, because it's not going to be trivial...

I took a stab at doing the deprecation, because the function is only called in 5 places in core, so how hard can it be, eh? Well two of the usages are easy to solve, but three of them are in the NodeGrantDatabaseStorage class...which a dependency of NodeAccessControlHandler which does make sense because they were two tightly coupled systems that were teased apart a little.

Logically it does make sense for the node_access_grants to get moved to NodeAccessControlHandler as proposed in the IS, as it's a high-level thing, and not concerned with the actual storage mechanisms etc.

I'm not sure how best to resolve this dependency cycle, seems like the storage is actually doing a decent amount more than simply storage. I suspect that those methods involved will need to accept an additional argument that then the high-level handler can pass in.

I've opened a draft PR to get the ball rolling on this one...I'm now wondering if the resolution on the dependencies is to introduce a third service, which could then be required by these two, and that service would actually do the hook invocation-ing, and thus its internals could eventually get replaced by a plugin based service etc. That might neatly tie everything up, as you'd have:

NodeAccessControlHandler

Helps integrate the node access control system into nodes

NodeGrantDatabaseStorage

Stores the access records / grants in the database and has helpers for altering queries etc.

NodeAccessHookBasedRecordsAndGrantsImplementationHandler

The new service that provides the single 'API' for node access control modules, as in, they really only need to implement a couple of hooks, nicely documented by this class, and they are done. They very specifically do not need to implement anything else. This is the class that in the future could switch to collecting plugins and calling methods on them.

Splitting things up might help them to be more maintainable and make it clearer to developers what a record, realm and grant are etc.

Anyway, I shall un-assign for now and I shall move it along if I have some more time.

Hmmm...but the original IS: https://www.drupal.org/node/2473041/revisions/8368653/view was actually mostly about moving the code and deprecating node_access_grants

That was 11 years ago.

The main driver from the recent comments here is to get this method out of the .module file, and not get hung up on rearchitecture discussions. See this slack thread https://drupal.slack.com/archives/C079NQPQUEN/p1770341452769079. That is why I'm suggesting splitting the deprecation into a new issue.

I think a 3rd service is our only way forward here, we can't change the API of NodeGrantDatabaseStorage like you've got in that MR without massive BC issues, and as you say we'll have circular dependency issues if we add the method to the access control handler.

That was what we originally had in #3038908: Deprecate node_access_view_all_nodes() which went through several rescopes.

Okay, so you want this issue to be about some sort of 'restructure' and then we spin out another ticket for the 'deprecation only'.

That's fine, happy to go with the flow.

But to be clear, the 'deprecation only' work will have to introduce a new service as mentioned above. You're happy with that, and it literally would only have the single method?

Also...I did the refactor to introduce another service in the draft PR. Went for minimal changes rather than trying to dial in the right names for things, as obviously that can come later.

I'll leave it as Needs work while waiting on clarity of direction for this ticket and any spin-off.

This actually looks pretty close, I have a suggestion to convert to camel case.

We also do not need an interface for this, the service name can just be the class.

We also need a rebase due to the deprecation removals in main.

@nicxvan made those changes as requested. So setting to Needs review.

We'd need a CR too, and to update the code with the correct link too I assume?

There's still my question from #38 where I'm asking about if continuing in this ticket is the right thing to do etc.?

I added the CR and some suggestions to update the CR links in the deprecation notices.

I think this is ready once those are applied and it's green.

I think this is fine to do here like this, then the follow up can address the api questions.

Added some more minor feedback, I'm also not a huge fan of the name of the class/method NodeGrantsHelper::nodeAccessGrants is a bit strange IMO. I don't have any better suggestions at this time though so won't hold the issue up on it.

How about NodeGrants::grantAccess
Or NodeGrants::accessGrants

I added some suggestions, but if we change the name they will all need to be updated.

Needs work for the suggestions that need applying, then I think it's ready.

Incorporated all the suggested changes, so moving back to Needs review.

I think this is ready.

I rebased due to the service autowire issue, the only change I made was:

Drupal\node\NodeGrantsHelper:
  autowire: true

to

Drupal\node\NodeGrantsHelper: ~

So I think it's still ok to RTBC.

All feedback has been addressed.
We need some followups created for refactoring.

Couple small suggetions on the MR.

Applied all suggestions.

I bumped the removal version of node_access_grants() to D13, since it is a "public" function and search_api at the very least uses it. I would go ahead and commit that, but I also just spotted that we're using property promotion on the new NodeGrantsHelper argument for the NodeGrantDatabaseStorage and NodeAccessGrantsCacheContext constructors. @berdir has pointed out in other similar issues that doing so makes the class property type nullable, and then making it not nullable later is a hassle, so let's assign the property the old way.

making it not nullable later is a hassle, so let's assign the property the old way.

We've done this countless times, where is it coming up as a hassle?

I've made the changes, but IMO it would be a shame if we have to do this for every new constructor prop going forward.

Committed c21604f and pushed to main. Thanks!

Needs a separate MR for 11.x, so setting to patch to be ported.

cherry picked onto 11.x

Added a couple changes to the service definitions in the node.services.yml because they aren't autowired by default in 11.x, but I think they're minor enough for me to RTBC.

If someone else can RTBC +1, I can commit.

Thanks @godotislate those changes look good

+1 just compared them only difference is the services.

Committed 26f4179 and pushed to 11.x. Thanks!

Thanks!

Reformat the CR and publish.