Problem/Motivation

Contact module doesn't implement AccessControlHandler for messages which prevents this module to be used with entityform_block module.

Proposed resolution

Access handler implementation that checks for "access site-wide contact form" for create access.
Additionally, we should add admin permission.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#19 core-contact-access-control-handler-2427713-10.patch3.26 KBdashaforbes
#17 core-contact-access-control-handler-2427713-10.patch3.27 KBdashaforbes
#13 core-contact-access-control-handler-2427713-10.patch3.27 KBdashaforbes
#12 core-contact-access-control-handler-2427713-6.patch3.27 KBdashaforbes
#6 core-contact-access-control-handler-2427713-TEST-ONLY.patch1.54 KBmbovan
#6 core-contact-access-control-handler-2427713-6-interdiff.txt1.54 KBmbovan
#6 core-contact-access-control-handler-2427713-6.patch3.27 KBmbovan
#3 core-contact-access-control-handler-2427713-3.patch1.73 KBmbovan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Thanks, we might have this in contact_storage already.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

contact_storage only has the admin permission.

mbovan’s picture

mbovan CreditAttribution: mbovan commented
Status: Active » Needs review
FileSize
core-contact-access-control-handler-2427713-3.patch1.73 KB

Added AccessControlHandler class and admin_permission for Message entity entity type.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We're not testing that at the moment, not sure how we want to do that.

Maybe we could check for contact message create access instead of contact_form view access on contact.site_page_form?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work
mbovan’s picture

mbovan CreditAttribution: mbovan commented
FileSize
core-contact-access-control-handler-2427713-6.patch3.27 KB
core-contact-access-control-handler-2427713-6-interdiff.txt1.54 KB
core-contact-access-control-handler-2427713-TEST-ONLY.patch1.54 KB

Added tests coverage.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: core-contact-access-control-handler-2427713-TEST-ONLY.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Failed as expected.

@mbovan: If you upload the test only patch first, then the issue status will not be set back to needs work.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/modules/contact/src/Tests/MessageEntityTest.php
@@ -62,6 +62,15 @@ public function testMessageMethods() {
+    $access_user = $this->createUser(['uid' => 3], array('access site-wide contact form'));
+    $admin = $this->createUser(['uid' => 4], array('administer contact forms'));

Can you make the array format consistent on these lines? We shouldn't mix old form array() and new form [] in the one line. Personal preference is on [], but let you decide ;)

Minor nit, other than that - looks great - thank you for the patch and the test!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work
dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
FileSize
core-contact-access-control-handler-2427713-6.patch3.27 KB

Changed array syntax lines 68 and 69 as per comment #10

dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
FileSize
core-contact-access-control-handler-2427713-10.patch3.27 KB

changed patch name

dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
Status: Needs work » Needs review

The last submitted patch, 12: core-contact-access-control-handler-2427713-6.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: core-contact-access-control-handler-2427713-10.patch, failed testing.

dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
Status: Needs work » Needs review
FileSize
core-contact-access-control-handler-2427713-10.patch3.27 KB

Status: Needs review » Needs work

The last submitted patch, 17: core-contact-access-control-handler-2427713-10.patch, failed testing.

dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
FileSize
core-contact-access-control-handler-2427713-10.patch3.26 KB
dashaforbes’s picture

dashaforbes CreditAttribution: dashaforbes commented
Status: Needs work » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

@dashaforbes: Remember to provide an interdiff when working on an existing patch, had to download and diff myself to make sure that only the requested changes were made. See https://www.drupal.org/documentation/git/interdiff.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 4f33bd4 and pushed to 8.0.x. Thanks!

  • alexpott committed 4f33bd4 on 8.0.x 
    Issue #2427713 by dashaforbes, mbovan: Contact module doesn't implement...

Status: Fixed » Closed (fixed)

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