Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The documentation for hook_node_access() does not reflect what is in the current code.
The enforcement of create access has moved to hook_ENTITY_TYPE_create_access(). Node module doesn't use node_node_access() for permissions, instead using NodeAccessControlHandler::checkCreateAccess().
This patch:
- Updates the documentation of hook_node_access().
- Updates the documentation of node_node_access().
- Adds a negative test case to NodeCreationTest to ensure that create access permissions are handled securely.
Comment | File | Size | Author |
---|---|---|---|
#46 | 2348203-46.patch | 10.9 KB | init90 |
#39 | 2348203-39.patch | 4.38 KB | govind.maloo |
#34 | 2348203-34.patch | 3.75 KB | govind.maloo |
#25 | interdiff-22-25.txt | 510 bytes | mohit_aghera |
#25 | 2348203-node-access-documentation-25.patch | 4.15 KB | mohit_aghera |
Issue fork drupal-2348203
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
dawehnerWell, hook_node_access() should just be executed in case you have a single node, not when you have a list of entries.
Comment #2
agentrickardIt's also not invoked for user 1 or roles that can Administer content.
However, it does appear that the 'create' operation is no longer invoked by hook_node_access().
Adding a die() statement here should cause \Drupal\node\Tests\NodeCreationTest to crash, but it does not.
Tracking through the code it looks like the work is handled by different hooks in D8.
From EntityAccessHandler.
Though I can't see where Node module invokes these, either, so I may just be misreading.
Comment #3
agentrickardComment #4
agentrickardHere's the patch. Updating the issue summary with details.
Comment #5
agentrickard@chx points out that 'existant' is not a word ;-p.
Comment #6
chx CreditAttribution: chx commentedThis is no longer a documentation-only patch. The mention of Drupal 8 in doxygen is very odd,
ag ' \* .*Drupal 8'|grep -vi 'deprecated\|test'
tells me this is simply not done elsewhere, the @see should be enough and https://www.drupal.org/node/1862420 should be amended to include the new hook and altogether made cleaner.Comment #7
agentrickardThis is already documented on that page:
Docblock update to remove the Drupal 8 reference.
Comment #8
chx CreditAttribution: chx commentedYes but the hook_ENTITY_TYPE_create_access hook is not mentioned AFAICS
Comment #9
agentrickardI see. I think the above quote is a typo.
Comment #10
agentrickardUpdated docs page.
Comment #11
chx CreditAttribution: chx commentedComment #12
catchWhile I don't think it will ever happen in practice, someone could theoretically call node_node_access() directly with $op = 'create'.
This makes me think we should split this hunk (and only this hunk) out to an 8.1.x follow-up.
Comment #13
chx CreditAttribution: chx commentedWhy? It's a hook implementation, you are absolutely not supposed to call those.
Comment #14
catchYes, you're not supposed to do lots of things.
We decided to commit things that break code doing things it should not for minor releases, unless it's necessary to fix a bug.
That way, if you have a site that has a module that does lots of horrible things, it breaks once every six months (or if we're lucky, just once when 8.1.x comes out then never again when the module is fixed), not every month.
This patch is a fairly extreme example, but the dead code can be removed in 8.1.x as early as tomorrow, so it doesn't hurt much to leave it dead for another 4.5 months in 8.0.x, then the entire branch will be lopped off.
Then in the unlikely case some custom module somewhere is re-animating that dead code, it'll blow up in April instead of January.
Comment #15
agentrickardShouldn't we at least push the documentation portion of this patch? This was a huge point of confusion for me when working on module ports.
Comment #16
catchWell I'd commit the whole thing if the hunk pointed out in #12 was split to an 8.1.x follow-up...
Comment #17
agentrickardRevised per #16.
Comment #19
BerdirUnfortunately 8.1.0 is out now, so with the same argument, we need to update the @todo to 8.2 now?
Or we switch to 8.2.x, do it and backport without that part.
If we're going to touch node_node_access() and removing one case is OK then I would even consider to remove the hook entirely and move that remaining logic into checkAccess() where everything else is checked.
Comment #20
BerdirAlso not sure this is a (major) bug, there is no functional problem here, just wrong documentation and dead code?
Comment #21
xjmThanks @Berdir.
Let's actually just remove the "and will be removed in 8.1" from this @todo. It should be formatted like:
Then we can commit the docs fix to both branches now and consider what @Berdir suggests in a followup. The test case suggested in the summary also sounds like a good followup.
I think we should also add a CR for this to communicate that this was previously changed.
The case I would see for this to still be major is if the result of following the documentation would be information disclosure when the hook didn't fire. Is there a scenario where that would happen? E.g. if some other implementation allowed create access, but your implementation was trying to override and forbid it.
Comment #22
dagmarCreated placeholder for the followup. #2730439: Follow up: hook_node_access() no longer fires for the 'create' operation
Comment #24
BerdirWe discussed that at the entity field issue triage meeting in Dublin.
It is not deprecated, it does not exist. And we do not support calling hooks directly, so we can just remove this.
the only thing we didn't agree on was if it can be removed in a patch or a minor release.
Comment #25
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedUpdating patch for 8.2.x branch.
Removing comment as mentioned by @berdir
Comment #26
xjmWe also did agree in Dublin that this issue is major, because by documenting access control that does not actually work it could even lead to information disclosure for what appears to be "supported" code.
Comment #31
BerdirI don't care if it should be a patch or a minor release, doesn't matter, better to get it into 8.7 than waiting another 2 years :)
Comment #32
BerdirMarked #2825524: hook_node_access() is not called for 'create', contradicting its documentation as a duplicate.
Comment #33
alexpottThis patch still needs a CR and unfortunately it does not apply :(
Comment #34
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #35
walii CreditAttribution: walii commentedi think this hook work only for "anonymous" role
Comment #36
walii CreditAttribution: walii commentedif any role have permission "Bypass content access control" , this hook not call
Comment #37
BerdirNo, this works for everyone (except create access), might there might be caching involved.
Edit: Yes, the bypass permission bypasses everything including this hook. That's not the same as only for anonymous users, though :)
@alexpott: Not sure what you want to do in an CR. We have a CR for entity access that also documents that create access is separate since 2012 :) https://www.drupal.org/node/1862420.
Comment #38
alexpott@Berdir I was just making sure that we were not avoiding taking #21 in account. It's great that a CR already exists.
I would add a
above
switch ($op)
because it helps explain why create is not in the list without having to read the methods docblock.Comment #39
govind.maloo CreditAttribution: govind.maloo at Salsa Digital commentedComment #40
BerdirI think @alexpott meant "Node" instead of "Note". Note could also be an option, but then it would need to be "Note that...".
Also, make sure you provide an interdiff, that makes it easier to review the difference: https://www.drupal.org/documentation/git/interdiff
Comment #41
alexpottI meant
Note
- the that is superfluous but it can be added so// Note that create access is handled by hook_ENTITY_TYPE_create_access().
is great.Comment #42
BerdirOk, needs a reroll anyway.
Comment #44
alexpott#3061217: Clarify if hook_node_access() $node argument can or cannot be a string is a duplicate of this but it also contains an important fix to hook_node_access() docs that's not here.
Also I ponder if we should be maintaining hook_node_access() docs in node.api and not pointing to the docs in entity.api.php.
Comment #46
init90Here is update with changes according to #44
Comment #48
mxr576and here we are... =]
So my colleague, @boobaa, hast just bumped into this dead code related to the "create" operation in
node_node_access()
. He actually spotted the missingbreak
after the "create" case in the switch, long story short, we unnecessary spent time figuring out that how this bug is not discovered by any core tests...How can we help to get this fix in core?
Comment #49
alexpottI deliberated about the changes to node_node_access and whether they'd have any impact. Some places in contrib are calling this directly!!! I'm guessing never with an op of create because it is totally broken. Also there are places in contrib that say "copying from node_node_access" which is a very good reason to fix this broken code. I also agree with @Berdir who has pointed out that that node_node_access shouldn't really exist but that can be done in a task follow-up.
@catch did ask for that hunk to be broken out before BUT in the interim things have got even worse and the $access from the create is not even being used. Atm if you call node_node_access with an op of
create
it will return you the access result for edit permissions. That is totally wrong.I'm going to commit this to 9.1.x but I believe it should be backported to 8.9.x as this code has gone from wrong to dangerous.
Committed aa8d81c and pushed to 9.1.x. Thanks!
Removed unused use on commit.
Comment #51
alexpottDiscussed with @catch and we agreed to backport this to 9.1.x given the changes since the last rtbc.