EntityForm::actions() adds 'delete' without checking access

Should be as easy as adding #access => $entity->access('delete') for the delete form submit element?

First drafted patch.

You should be able to use $this->entity, this is already done in actionsElement()

Ohhhh....Thanks Berdir for comment. Updated patch as per comment.

5: set-delete-access-2084323-5.patch queued for re-testing.

$this->entity->access(), not $this->access()

updated patch.

Awww, bad contact message entity :)

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -247,6 +247,7 @@ protected function actions(array $form, array &$form_state) {
+        '#access' => $this->entity->access('delete'),

In actionsElement, we have

  // We cannot delete an entity that has not been created yet.
    if ($this->entity->isNew()) {
      unset($element['delete']);
    }

Either that should be moved here, or this should be moved there. But not both please.

Yep, makes sense.

Discussed this change with Berdir and it makes sense to let the entity access controller deny delete access in general if the entity is new.

This should have been reproducible with NodeType, except it explicitly added checks in NodeTypeFormController::actions().

I think we can delete that line, and write a test for it that fails without this fix.

Ah, great hint, made me go through our form controllers and it turns out that a ton of them manually check delete access, so a lot of custom overrides that we can remove.

A number of them remove the delete button unconditionally, like date format edit and actions edit, didn't touch them.

Also added the test coverage, found a place where it nicely fit in I think.

Beautiful! Thanks.

Updating the patch with re-roll. Please review.

Re-roll looks good, thanks.

25: drupal8-2084323-25.patch queued for re-testing.

git rebase re-roll, shortcut files were moved.

29: drupal8-2084323-29.patch queued for re-testing.

Back to green.

Yes, previous fail was a bot hickup (the "failed to create directory" kind of fails)

Re-roll, but let's get #2238077: Rename EntityFormController to EntityForm in first.

Note that delete is now a link, but we can still control access for the link...

Re-roll, fixed the node type tests now that the label is different and is a link. Reverted changes to ProfileForm (not in interdiff), #216064: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link messed that up, as it is now no longer there by default as it's only added if there's a delete-form, so it's defines a new form element. Opened #2250377: ProfileForm::actions() defines new delete element instead of altering it as it expects.

Looks good.

Committed/pushed to 8.x, thanks!