Only a subsection of the users on our site have the permission to 'Read private messages'. Therefore, when writing a message, I think there should be an access check performed to ensure that all users that are recipients of a message can actually read the message, and that any recipients that don't have that permission are rejected. (Same goes for the autocomplete field too).

Having looked through the module code, it looks like the function privatemsg_recipient_access() should be responsible for this kind of check. However, since the recipient type array for 'user' defines neither a 'view callback' nor a 'view access' permission, this function will always return TRUE no matter what user is being checked. Is this the intention?

By simply adding a 'view callback' key to the user recipient type array, we could perform a 'read private messages' permission check on the recipient user quite trivially. I'm happy to supply a patch for this if this sounds right.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

torrance123’s picture

The more I look at it, the less I understand how that function is meant to work, and why for the vast majority of 'write' permission checks, the recipient object isn't passed at all — for example in privatemsg_autocomplete.

But anyway, these changes/additions check that all recipients have 'read privatemsg' permission:

/**
 * Implements hook_privatemsg_recipient_types_info().
 */
function privatemsg_privatemsg_recipient_type_info() {
  return array(
    'user' => array(
      'name' => t('User'),
      'description' => t('Enter a user name to write a message to a user.'),
      'load' => 'privatemsg_user_load_multiple',
      'format' => 'privatemsg_username',
      'autocomplete' => 'privatemsg_user_autocomplete',
      'write callback' => '_privatemsg_recipient_user_write_permission',
      // Make sure this comes always last.
      '#weight' => 50,
    ),
  );
}

function _privatemsg_recipient_user_write_permission($recipient) {
  if (is_object($recipient)) {
    return user_access('read privatemsg', $recipient);
  }
  else return TRUE;
}
Berdir’s picture

Yes, only users with read access should actually receive a message.

I don't fully understand yet in which cases this is not correctly checked. We do have some tests to verify that only users with read permissions can receive messages I think.

Can you provide your changes as a patch, and detailed instructions on how to reproduce the problem? Automated tests would be even better but we can look into that once we have manual instructions.

torrance123’s picture

Hi Berdir, I'll start by explaining how to reproduce the issue we're seeing.

  1. On a brand-spanking new installation of Drupal, install privatemsg and dependencies.
  2. Set up a user role that has access to both read and write private messages; leave authenticated users with no privatemsg permissions at all (see attached screenshot).
  3. Create a test user of 'authenticated role' and a test user than can read and write private messages (in our case, this role is named 'cms admin').
  4. When logged in as a cms admin, go to /messages/new, enter the name of the authenticated user and observe that this field autocompletes with the authenticated user's name.
  5. Complete the message and send to the authenticated user. Observe that the message sends without error and a thread is created between the cms admin and authenticated user.
  6. Optionally, log in as the authenticated user role and confirm that you have no access to /messages or /messages/new, etc.

So to be clear: the CMS admin role (who has permission to both read and write private messages) can send messages to the authenticated user (who doesn't have access to read or write private messages).

Whilst the authenticated user cannot actually read these messages being sent to them, I think the behaviour of the module should be a) filter out such users from the autocomplete and b) refuse to send messages to users who can't read them in the first place.

torrance123’s picture

This is the patch to ensure you can only send messages to users with read permissions; it doesn't fix the autocomplete, however.

As I said, I'm unsure if this is the correct area to be patching and I'm confused as to how privatemsg_recipient_access() is meant to work.

diff --git a/privatemsg.module b/privatemsg.module
index a09ef17..786f15d 100644
--- a/privatemsg.module
+++ b/privatemsg.module
@@ -2656,12 +2656,20 @@ function privatemsg_privatemsg_recipient_type_info() {
       'load' => 'privatemsg_user_load_multiple',
       'format' => 'privatemsg_username',
       'autocomplete' => 'privatemsg_user_autocomplete',
+      'write callback' => '_privatemsg_recipient_user_write_permission',
       // Make sure this comes always last.
       '#weight' => 50,
     ),
   );
 }
 
+function _privatemsg_recipient_user_write_permission($recipient) {
+  if (is_object($recipient)) {
+    return user_access('read privatemsg', $recipient);
+  }
+  else return TRUE;
+}
+
 /**
  * Implements callback_recipient_autocomplete().
  */
Berdir’s picture

Please upload the patch as a .patch file so that the testbot picks it up and runs the tests against it.

torrance123’s picture

Please upload the patch as a .patch file so that the testbot picks it up and runs the tests against it.

Sure thing.

torrance123’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-6.patch, failed testing.

torrance123’s picture

That patch clearly failed. Sad face. I'm failing to understand the structure of the privatemsg code with regards to permissions, so I'm not really in a position to rewrite the patch.

Can I at least have you confirm that you're able to reproduce the errant behaviour in comment 3?

torrance123’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

I've modified the patch slightly to pass all test cases, and have included an additional test case to test for issue.

petednz’s picture

Hi Berdir - is it likely you will be able to look at the above in a reasonable timeframe. Only pushing as my sense/understanding is the module is effectively unusable as it is for anyone who needs to have any control over who can be sent messages.
Appreciate your engagement earlier this month - we are doing our best to provide the patches required to get this working again. Would be very helpful to our efforts if we could get a thumbs up, or a 'you guys have it so wrong' ;-)

torrance123’s picture

Hi Berdir, have you had a chance to look at my patch/tests?

torrance123’s picture

I'm bumping this up: I've provided patches and tests, so would really appreciate some interaction from the maintainers of this module.

torrance123’s picture

Updated tests and patch for this issue. Would really appreciate some interaction from the maintainers on this!

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-14.patch, failed testing.

torrance123’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

And again, minus the syntax error.

ptmkenny’s picture

Berdir’s picture

Status: Needs review » Needs work
+++ b/privatemsg.moduleundefined
@@ -2656,12 +2656,20 @@ function privatemsg_privatemsg_recipient_type_info() {
+function _privatemsg_recipient_user_write_permission($recipient) {

Needs phpdoc with @param and @return documentation.

+++ b/privatemsg.moduleundefined
@@ -2656,12 +2656,20 @@ function privatemsg_privatemsg_recipient_type_info() {
+  else return TRUE;

Always use curly braces even for one line conditions.

Berdir’s picture

Double post.

Berdir’s picture

Also, just noticed when looking at privatemsg_get_link() that we explicitly check the read privatemsg permission of the recipient.

Which makes it even more obvious and shows that this is the expected behavior, we're just not yet applying it everywhere cleanly.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
51.88 KB

This patch combines the patch in #19 from https://drupal.org/node/1653462 with torrance123's patch in #16 above. I also fixed the curly braces as per #18, but I didn't add the @return documentation because I don't really feel qualified to write that.

ptmkenny’s picture

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-21.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
51.88 KB

Fixed whitespace error.

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-24.patch, failed testing.

Berdir’s picture

+++ b/privatemsg.installundefined
@@ -426,3 +426,10 @@ function privatemsg_update_7202() {
+  db_update('role_permission')->fields(array('permission' => 'receive privatemsg'))->condition('permission', 'read privatemsg')->execute();

fluent calls should be formatted like this:

<?php
db_update('role_permission')
->fields(array('permission' => 'receive privatemsg'))
->condition('permission', 'read privatemsg')
->execute();

+++ b/privatemsg.moduleundefined
@@ -2743,12 +2746,22 @@ function privatemsg_privatemsg_recipient_type_info() {
+function _privatemsg_recipient_user_write_permission($recipient) {

Add something like:

Verify if it is possible to write a message to the given recipient.

+++ b/privatemsg.moduleundefined
@@ -2743,12 +2746,22 @@ function privatemsg_privatemsg_recipient_type_info() {
+    return user_access('write privatemsg') && ($recipient->uid == 1 || user_access('read privatemsg', $recipient) || user_access('administer privatemsg settings', $recipient));

The uid 1 check not necessary, user_access() takes care of that.

+++ b/privatemsg.moduleundefined
@@ -2767,6 +2780,10 @@ function privatemsg_user_autocomplete($fragment, $names, $limit) {
   foreach ($accounts as $account) {
+    // Only display recipients with permission to read private messages.
+    if (!_privatemsg_recipient_user_write_permission($account)) {
+      continue;

The problem with this is that if you have a lot of users that can't receive messages, you will have a lot of matches, then remove all of them and end up with no suggestions.

So what we should try to do is adjust the query so that only allowed users are returned. That will mean a join to users_roles, get the roles with that permission (user_roles() is I think the function for that) and add a condition to rid on that table.

Also, how do you create your patches? I have a feeling that you're doing something in a non-optimal way...

ptmkenny’s picture

Regarding making patches, I'm not sure what I'm doing wrong. I was trying to follow the instructions here: http://drupal.org/node/707484

Basically, I:
1. Clone the git repository.
2. Create a branch with the issue number and check out that branch.
3. Edit the files and commit my changes.
4. git diff 7.x-2.x > myfile.patch
5. git checkout 7.x-2.x
6. git apply myfile.patch

If that works, I assume the patch is safe to upload. However, this last patch (#24) applies cleanly on my own machine so I don't know what I'm doing wrong.

Berdir’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev

Ah, the version of this issue is wrong, that needs to match the version you're working with. So changing it and then requesting a re-test.

And yes, that sounds good.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-24.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
51.94 KB

Ok, this patch addresses all of the concerns in #26 except for the last part about rewriting the query. I also renamed one instance of "read privatemsg" that I missed previously.

@Berdir: By the way, where is the best place to read up on how to format code for Drupal in general/this module in particular? For example, I didn't know how to format the permissions rename update hook, so I just cut and paste a similar permissions rename update hook from the System module for updating from 6.x to 7.x.

Berdir’s picture

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-31.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
52.91 KB

Re-rolling for the latest dev and including the comment style fixes that were removed from the other patch.

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-35.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
53.09 KB

Fixed misnamed permission in the test.

EDIT: Ok, since this patch passed, I will just note that (as far as I know) all concerns voiced so far have been addressed except this one:

So what we should try to do is adjust the query so that only allowed users are returned. That will mean a join to users_roles, get the roles with that permission (user_roles() is I think the function for that) and add a condition to rid on that table.

Unfortunately, I only have a very cursory understanding of SQL, so I won't be able to fix this myself. In the meantime, I'm happy to keep the patch current and address any other concerns.

Berdir’s picture

This should do the trick.

No test coverage for the issue that I described. One way to do that would be to go add 11 users in alphabetical order, the first 10 don't have permission, the 11th does. Then the autocomplete should return the 11th, which should work with my implementation and not with the previous one.

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-38.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-38.patch, failed testing.

ptmkenny’s picture

Added a test as described in #38.

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-41.patch, failed testing.

ptmkenny’s picture

I just realized I shouldn't add all the tests because that leads to massive failures. Here's the new test for the issue reported in #26.

ptmkenny’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-43-tests-only.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
2.15 KB

Forgot to switch receive back to read for the permissions.

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-47-tests-only.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-write-permissions-1928502-47-tests-only.patch, failed testing.

ivnish’s picture

Issue summary: View changes
Status: Needs work » Closed (outdated)