Problem/Motivation

Settings default settings for personal contact forms to disabled, setting specific user personal contact form to enabled and visiting contact form of mentioned user with another user who has permission to visit contact forms should result in access allowed and instead it returns access denied.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Normal, security related but too permissive instead of too open.

To replicate

* Enable contact module.
* Set default settings for personal contact forms to disabled (unchecked).
* Create a new user.
* User sets personal contact forms to enabled.
* Create a user with permission to access user contact forms.
* Try to visit contact form of first user with second user.

Proposed resolution

Provided patch and test.

User interface changes

None

CommentFileSizeAuthor
#21 contact-form-2393577-21-interdiff.txt1.72 KBBerdir
#21 contact-form-2393577-21.patch2.05 KBBerdir
#16 contact-form-2393577-16.patch2.05 KBvladan.me
#15 contact-form-2393577-15-tests.patch1.05 KBvladan.me
#6 contact-form-2393577-4.patch2.17 KBvladan.me
#5 contact-form-2393577-3-tests.patch1.32 KBvladan.me
#2 contact-form-2393577-2-tests.patch1.33 KBvladan.me
#1 contact-form-2393577-1.patch2.16 KBvladan.me
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
Status: Active » Needs review
FileSize
contact-form-2393577-1.patch2.16 KB

Providing patch and test.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
FileSize
contact-form-2393577-2-tests.patch1.33 KB

Test only to confirm the issue.

The last submitted patch, 1: contact-form-2393577-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: contact-form-2393577-2-tests.patch, failed testing.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
FileSize
contact-form-2393577-3-tests.patch1.32 KB

Eh, wrong version of Drupal, rerolled. First test now.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
FileSize
contact-form-2393577-4.patch2.17 KB

And full patch.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/modules/contact/src/Access/ContactPageAccess.php
@@ -86,8 +86,10 @@ public function access(UserInterface $user, AccountInterface $account) {
-    if (isset($account_data) && empty($account_data)) {
-      return $access;
+    if (isset($account_data)) {
+      if (empty($account_data)) {
+        return $access;
+      }

I don't understand how these are functionally different

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented

Enough to make it fail :)
Ok, here's full debug information:
Code snippet is this one:

    $account_data = $this->userData->get('contact', $contact_account->id(), 'enabled');
    if (isset($account_data) && empty($account_data)) {
      return $access;
    }
    // If the requested user did not save a preference yet, deny access if the
    // configured default is disabled.
    else if (!$this->configFactory->get('contact.settings')->get('user_default_enabled')) {
      return $access;
    }

    return $access->orIf(AccessResult::allowedIfHasPermission($account, 'access user contact forms'));

1) when user's settings are disabled then $account_data gets value 0
2) when user's settings are enabled then $account_data gets value 1

1) it immediately gets into 

 if (isset($account_data) && empty($account_data)) {
      return $access;
    }

as it's set and it's empty
2) it skips first condition but enters second one
when it notices that default settings are disabled by default but never actually confirms that user has set to 1
so, when it enters this second, it again returns Access Neutral which in the end converts to access denied

So, this patch will make it enter into isset($account_data) but will not fallback to wrong (default settings) check.

Hopefully, tests will prove the point.

The last submitted patch, 5: contact-form-2393577-3-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: contact-form-2393577-4.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

There's a mismatch there with webUser vs web_user, did you create the patch against an older version of core? make sure to use the latest 8.0.x branch.

larowlan’s picture

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

ah didn't see the elseif - thanks

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Priority: Major » Normal 
+++ b/core/modules/contact/src/Access/ContactPageAccess.php
@@ -86,8 +86,10 @@ public function access(UserInterface $user, AccountInterface $account) {
     // If the requested user has disabled their contact form, do not allow users
     // to contact them.
     $account_data = $this->userData->get('contact', $contact_account->id(), 'enabled');
-    if (isset($account_data) && empty($account_data)) {
-      return $access;
+    if (isset($account_data)) {
+      if (empty($account_data)) {
+        return $access;
+      }
     }

Wondering if we can improve the comments a bit to explain this better. Would it help to move the comment there inside the first if?

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
FileSize
contact-form-2393577-15-tests.patch1.05 KB

With the help of @Berdir putting a new round of patches with improved comments and reduced logic. Now against dev version.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
FileSize
contact-form-2393577-16.patch2.05 KB

And full patch.

vladan.me’s picture

vladan.me CreditAttribution: vladan.me commented
Status: Needs work » Needs review

The last submitted patch, 15: contact-form-2393577-15-tests.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Thanks!

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -185,6 +185,15 @@ function testPersonalContactAccess() {
+    // Test with disabled global default contact form
+    // in combination with a user that has the contact form enabled.

80 col rule, but could be fixed on commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/contact/src/Access/ContactPageAccess.php
@@ -83,11 +83,13 @@ public function access(UserInterface $user, AccountInterface $account) {
+      // Forbid access if the requested user has disabled his contact form.

Docs should be gender-neutral - see (for example) #1284436: Make comment text gender-neutral in system_clean_url_settings(). I'd say // Forbid access if the requested user has disabled their contact form.

Let's fix #19 before commit - since we also need to fix the above.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
contact-form-2393577-21.patch2.05 KB
contact-form-2393577-21-interdiff.txt1.72 KB

Updated the comments.

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 3967195 and pushed to 8.0.x. Thanks!

  • alexpott committed 3967195 on 8.0.x 
    Issue #2393577 by vladan.me, Berdir: Access issue with default settings...

Status: Fixed » Closed (fixed)

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