Don't allow admin users to access their own contact form

Does what it says on the tin and, barring some crazy circumstance where someone would want to contact themselves, a good move. This is for the D7 patch btw; haven't looked at the D6 one.

reviewing

No problems with patches.

Works like a charm in D6... you have my stamp of approval. "Contact" tab is there under regular circumstances, apply the patch and "contact" tab disappears for my user but is still there for other users. Tried it in D7 too.

Attached are screen shots of d6..

unassign

Please include a simple test to make sure this stays fixed for both normal users and user-admin users.

Is is this simple? Since now in 7.x you can give anonymous users permission to view user contact forms, it would be odd if you can only view your own contact form if you are logged out.

See also #345541: Link to contact form in user account and e-mails leads to 403..

subscribe

Patch based on #2 with tests for review.

Re-rolled patch from #13.

Tested the patch in #15 with success. With the patch the contact page of user 1 is reachable as user 1, without it's not reachable (see attached screenshots 1 and 2). The contact form of user 1 is still accessible as an anonymous user with the "Use users' personal contact forms" permission (see attached screenshot 3).

I reviewed the patch and the code looks good and has the necessary tests so this looks RTBC to me. It basically just prevents the personal contact form tab access check from returning TRUE if the visitor tries to view his/her own contact form, while preserving the administrators' rights to view all users' personal contact forms. The "can not view own contact form" check was already there before but was not in the right order, leaving administrators with the possibility of viewing their own contact form.

This was committed to D8: a478bad.

Here are the patches for 7.x.

Clean backport :)

I'm not sure this makes sense - if I'm an administrator who just turned on the Contact module, one of the first things I'd probably do is go to my own account and see if the tab is there (and probably try to contact myself also to test how it works).

I ran "git blame" to see when this code was originally added, and after a long slog through Git history I came across #70895: usability: don't show contact when viewing *own* profile. Reading the comments there, leaving the tab there for administrators was indeed done on purpose (and for the reason I mentioned above).

So personally I think this should be rolled back in Drupal 8. But maybe either way we need better code comments here to explain the logic of why the code does what it does.

This needs re-roll for D8

the code discussed in 7.x patch is already in 8.x. See below taken from 8.x code as of today.

function _contact_personal_tab_access($account) {
  global $user;

  // Anonymous users cannot have contact forms.
  if (!$account->uid) {
    return FALSE;
  }

  // Users may not contact themselves.
  if ($user->uid == $account->uid) {
    return FALSE;
  }

  // User administrators should always have access to personal contact forms.
  if (user_access('administer users')) {
    return TRUE;
  }

@David_Rothstein has rightly pointed in #20 that support for this functionality was intentionally added. But by looking at the code, it can be seen that the "can not view own contact form" check was also already there but was not in the right order, as explained in #16. So I think we have a bit of dilemma here and we need to make a decision now on whether the functionality should be removed or not.

If we go with removing this functionality (as was the original intent of this issue), then we can simply RTBC this issue. The fix for D8 has been committed. And I just tested that the patches from #18 still apply cleanly to D7 (commit # 1990861)

If we go with keeping this functionality, then we need to re-roll what got committed to D8 in #17. A patch is here in this regard.

In either case, the issue needs more review.

Accidentally got the patch tested for D7. Though I don't know why the previous patch failed for D8. But lets make a decision first on what to do with this issue before working on the patch again...

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -116,14 +116,7 @@ function testPersonalContactAccess() {
-    $this->drupalLogout();
-    $this->drupalLogin($this->adminUser);
-    $this->drupalGet('user/' . $this->adminUser->id() . '/contact');
-    $this->assertResponse(403);

tests should be fixed, not removed

Fixed the test instead of removing it as per #27. Also, trying to fix what got the tests to fail in #23.

Again, the patch is intended to re-roll what got committed in #17 as per the suggestion in #20.

This should pass the tests...

Because retesting the patch went successful...

Hello, I installed drupal 8.2.x-dev and could not replicate the above issue.
Screen shot attached.
I also tried it with drupal 7.x-dev and could replicate it.

Create a patch for 9.1 and tested on my local working fine, kindly check.

Tested the patch on local clean install of drupal 9.1x

1. Before applying patch contact link is not visible in user profile
2. After applying patch contact link is visible
3. Anonymous user get access denied if they access particular users form example /user/1/contact
4. if authenticated user tried to access different users contact form he gets page not found example if user with uid 1 tries to access form of user with id 3 /user/3/contact

Looks good, waiting for tests to PASS
This can be moved to RTBC if code review is also fine

Patch applied cleanly on drupal 9.1x, Patch looks good, +1 for RTBC.

+++ b/core/modules/contact/tests/src/Functional/Views/ContactLinkTest.php
@@ -71,9 +71,8 @@ public function testContactLink() {
+    // The admin user has access to all contact links including his own.

Let's not use gendered language here - their own was in the original comment, let's retain that

It's not clear what the remaining tasks of this issue are, it was committed in Mar 2020, but there's still a patch.

Can we get an issue summary update reflecting why the issue is still open? Thanks.

Hi @larowlan,

Just updated for

Let's not use gendered language here - their own was in the original comment, let's retain that

, kindly check.

Thanks,
Shakti.

Comment change looks good, waiting for review from @larowlan

Setting NW for the issue summary update requested in #56.

+++ b/core/modules/contact/tests/src/Functional/Views/ContactLinkTest.php
@@ -71,7 +71,7 @@ public function testContactLink() {
+    // The admin user has access to all contact links beside their own.

Personally, I think 'including their own' is easier to read.

Agree with #60
updated patch as suggested in #60.

@adityasingh, thanks for the reroll.

This needs an issue summary update, requested in #56 and #62. To assist that process I have added the issue summary template and trimmed it to only those sections that need to be finished before this can be marked RTBC. The general guidelines for summaries provides guidance on what needs to be added. This work is suitable for a novice.

Oh, up to date screenshots are needed, I've marked a place in the issue summary where they can be added. I don't know if the screen shots from #52 are up to date. I haven't looked at the changes to the patch.

Adding screenshots.

Applied patch #62 and it works fine.

Before patch:

After patch:

I was unable to reproduce the error in in 9.2.x. As far I understood the point of the issue is to be unable to contact yourself, and as it is in 9.2.x seems impossible to do so. Maybe I'm doing something wrong. I have interest in filling the summary update but I could use steps to recreate the issue.

@guilhermevp, Welcome to Drupal. I too am finding this a bit confusing and like you I find that this is already fixed.

So what needs to be done? I read through the issue and found the answer in, #20. It points out that this overrides decisions made in an earlier patch. That history should be in the Issue Summary, enough so reviewers understand the problem. And in the 'remaining tasks' to state the next step, like 'Decide if the ability for the admin to contact themselves should be removed' and thus remove the capabilities pointed to in the issue referred to in #20.

@guilhermevp, would you like to work on the Issue Summary update?

@guilhermevp I was able to find the error in in 9.2.x. Added screenshot for the reference.
patch #62 applied cleanly and working as expected.

@quietone

I dabbled in making a summary update. Let me know what you think.

@guilhermevp, Very nice update. Good job! To further improve it this string "who just turned" can be changed to "who had just turned" and instead of the link with the text, 'this issue', change it to

[#70895]

which will expand it to show the issue number and issue title.

Putting on my reviewers hat, I think this can be further improved to make it easier for a reviewer. The first thing I notice is the title. Is it accurate? I don't think so because not we are deciding whether to have the tab or not. Maybe change it to 'Should the Contact yourself tab be in the user profile for the admin'. What do you think?

The next thing it would be helpful to know is that this issue was committed. Since it is not so common that an issue continues after commit (and not a backport) let's emphasize that. In the fist section, problem/motivation add a beginning paragraph. Something like this "This issue was committed in commit (put the hash link here) and then in #20 David_Rothstein pointed out that the behavior was a deliberate choice in #70895: usability: don't show contact when viewing *own* profile. The issue is currently to discuss whether to restore the previous behavior or not."

Then, there are a lot of blank sections. As a reviewer I don't know if these are to be filled out later or not. So for 'steps to reproduce' I search the issue to see if is they are available in a comment and then copy/paste. If not, either add it myself or add TBA (to be added). In this case, 'TBA' is fine. And in 'Proposed resolution' I would add the two choices.

Either keep the current behavior, no contact tab for the admin on their profile page
or
Restore the contact tab for the admin on their profile page

And then, to finish off the sections, in 'user interface changes' add 'TBD'.

@guilhermevp, looking forward to your changes!

@Madhu kumar, Remember to read the issue before adding a comment. The screenshots you have added were already added in #65.

@quietone

Thanks for the feeback, I tried to change things a bit from your suggestions, but most of it was just perfect summarization of the updates the issue needed. Again, thanks!

Can someone who has worked on this lately update the issue summary with steps to reproduce the issue identified (this makes it easier to test that it is fixed) as well as the details of the proposed approach?

I recommend closing this issue and creating a new one that references it. Even if the issue summary is cleaned up, the comment history will more likely confuse than it will help. That commit history is bizarre/

@guilhermevp did a great job updating the issue summary, and some of his beneficial changes have already been reverted. This is a very old issue that started in 7.x. A new issue can repeat the valuable parts from here, but remove the noise.

I think the issue title of "Should the Admin be able to contact themself in the user profile" that @guilhermevp is the right approach. We need to determine if that is an acceptable change before anything is altered. This is still being interpreted as a bug when it isn't a bug. It's currently expected behavior (that I wouldn't mind seeing changed 🙂).

If you don't agree, remove the "needs followup" tag, but I think it will vastly increase the likelihood of contributors understanding what is going on.

I agree, this is really confusing. Also agree it's not a bug since it was an intentional change. The only problem with closing this to create a new issue is there is a valid patch.

I've created #3221619: Allow users with 'Administer users' permission to use their own contact form. @adityasingh you can upload your patch to that issue but I wouldn't mark it Needs Review until there is consensus that the change should be made.

I'm also updating the issue back to the original title and summary so that it's clear what was actually changed here.

Thanks @pameeela! I updated [##3221619] with the patch from here, documented that the contributors from this issue should receive credit if it lands, and tagged it to hopefully get product manager approval on making a users own contact form visible.

I believe this should be marked as ‘Fixed’ since it was committed to 8.x way back when. But now I’m not sure because there’s just a comment linking to a commit so maybe it was committed via another issue.

@bnjmnm and @pameeela, thanks for sorting this out. It has been in my too hard basket for a while now.

I can confirm this was committed. changing fixed. The followup has been created, removing tag.

commit a478badbf8b1e7fe28fb0680272aa70845393268
Author: Dries <dries@buytaert.net>
Date:   Fri Jun 29 09:55:56 2012 -0400

    - Patch #602234 by rocket_nova, neochief: contact yourself tab in user profile when you're admin.