Problem/Motivation

Drupal (confusingly) invokes hook_node_access() with $op referencing 'delete' when the create node form is displayed. This non-security head-scratcher was noticed on a security.drupal.org issue by @ragnarkurm:

Operation 'deleted' originates from form 'delete' button check. This kind of weird too to check deleted button on yet non-existent node, but probably not security issue.

Proposed resolution

pwolanin:

...yes, this just from the check to set #access on the delete button. This could be optimized by checking $node->isNew()...

Remaining tasks

  1. Credit pwolanin and ragnarkurm for debugging this.
CommentFileSizeAuthor
#13 interdiff.2825516.5-13.txt609 bytesmikelutz
#13 2825516-13.drupal.hooknodeaccess-is-called-with-op--delete-when-the-create-node-form-is-shown.patch591 bytesmikelutz
#5 node-hook_node_access_op_delete_on_create-2825516-0.patch946 bytesamwhalen

Comments

cilefen created an issue. See original summary.

cilefen’s picture

cilefen commented
Title: hook_node_access() is called with $op == 'delete' when the create node form is shown » hook_node_access() is called with $op = 'delete' when the create node form is shown
Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amwhalen’s picture

amwhalen commented
StatusFileSize
new node-hook_node_access_op_delete_on_create-2825516-0.patch946 bytes

Taking a stab at a patch using the suggestion from pwolanin.

amwhalen’s picture

amwhalen commented
Status: Active » Needs review
anas_maw’s picture

anas_maw commented

The last patch disable hook_node_access call on create, i think it should be called with operation 'create'

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

I'm not following the comment in #7.

+++ b/core/modules/node/src/NodeForm.php
@@ -244,7 +244,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
+    $element['delete']['#access'] = $node->isNew() || $node->access('delete');

Shouldn't this be !$node->isNew() && $node->access('delete')? Otherwise the delete button will be there on new content pages?

However, if that is the case, it indicates a lack of test coverage for the delete button not appearing on node add forms...

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

m.abdulqader’s picture

m.abdulqader commented

Really confusing I want to edit the permission on creating some node and the $op value is delete.?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2825516-13.drupal.hooknodeaccess-is-called-with-op--delete-when-the-create-node-form-is-shown.patch591 bytes
new interdiff.2825516.5-13.txt609 bytes

The parent call already checks if the node is new and decides whether to show the delete button or not (and does the access check). There's no reason for node_form to replace the access check, and it shouldn't modify the delete key unless it exists..

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

All tests still passing, and this removes the call to node access, so I think this is ready to go. It doesn't add a test, but I'm not sure a test is needed to ensure an access check isn't made--that would be a rabbit hole I think :)

larowlan credited pwolanin.

larowlan credited ragnarkurm.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

  • larowlan committed 771adc4 on 8.8.x 
    Issue #2825516 by mikelutz, amwhalen, jhedstrom, pwolanin, ragnarkurm:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 771adc4 and pushed to 8.8.x. Thanks!

c/p to 8.7.x

  • larowlan committed 6726ae2 on 8.7.x 
    Issue #2825516 by mikelutz, amwhalen, jhedstrom, pwolanin, ragnarkurm:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.