You are here

Can send messages to users who do not have 'read' permission

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.

Comments

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:

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

On a brand-spanking new installation of Drupal, install privatemsg and dependencies.

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).

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').

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.

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.

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.

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' ;-)

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.

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...

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.

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.

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.