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
Comment #1
torrance123 CreditAttribution: torrance123 commentedThe 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:
Comment #2
BerdirYes, 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.
Comment #3
torrance123 CreditAttribution: torrance123 commentedHi Berdir, I'll start by explaining how to reproduce the issue we're seeing.
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.
Comment #4
torrance123 CreditAttribution: torrance123 commentedThis 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.Comment #5
BerdirPlease upload the patch as a .patch file so that the testbot picks it up and runs the tests against it.
Comment #6
torrance123 CreditAttribution: torrance123 commentedSure thing.
Comment #7
torrance123 CreditAttribution: torrance123 commentedComment #9
torrance123 CreditAttribution: torrance123 commentedThat 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?
Comment #10
torrance123 CreditAttribution: torrance123 commentedI've modified the patch slightly to pass all test cases, and have included an additional test case to test for issue.
Comment #11
petednz CreditAttribution: petednz commentedHi 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' ;-)
Comment #12
torrance123 CreditAttribution: torrance123 commentedHi Berdir, have you had a chance to look at my patch/tests?
Comment #13
torrance123 CreditAttribution: torrance123 commentedI'm bumping this up: I've provided patches and tests, so would really appreciate some interaction from the maintainers of this module.
Comment #14
torrance123 CreditAttribution: torrance123 commentedUpdated tests and patch for this issue. Would really appreciate some interaction from the maintainers on this!
Comment #16
torrance123 CreditAttribution: torrance123 commentedAnd again, minus the syntax error.
Comment #17
ptmkenny CreditAttribution: ptmkenny commentedMeta issue: http://drupal.org/node/138950
Comment #18
BerdirNeeds phpdoc with @param and @return documentation.
Always use curly braces even for one line conditions.
Comment #19
BerdirDouble post.
Comment #20
BerdirAlso, 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.
Comment #21
ptmkenny CreditAttribution: ptmkenny commentedThis 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.
Comment #22
ptmkenny CreditAttribution: ptmkenny commentedClosed https://drupal.org/node/138950 and https://drupal.org/node/1653462 as duplicates.
Comment #24
ptmkenny CreditAttribution: ptmkenny commentedFixed whitespace error.
Comment #26
Berdirfluent calls should be formatted like this:
<?php
db_update('role_permission')
->fields(array('permission' => 'receive privatemsg'))
->condition('permission', 'read privatemsg')
->execute();
Add something like:
Verify if it is possible to write a message to the given recipient.
The uid 1 check not necessary, user_access() takes care of 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...
Comment #27
ptmkenny CreditAttribution: ptmkenny commentedRegarding 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.
Comment #28
BerdirAh, 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.
Comment #29
Berdir#24: privatemsg-write-permissions-1928502-24.patch queued for re-testing.
Comment #31
ptmkenny CreditAttribution: ptmkenny commentedOk, 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.
Comment #32
BerdirSee http://drupal.org/coding-standards and http://drupal.org/coding-standards/docs
Comment #33
Berdir#31: privatemsg-write-permissions-1928502-31.patch queued for re-testing.
Comment #35
ptmkenny CreditAttribution: ptmkenny commentedRe-rolling for the latest dev and including the comment style fixes that were removed from the other patch.
Comment #37
ptmkenny CreditAttribution: ptmkenny commentedFixed 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:
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.
Comment #38
BerdirThis 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.
Comment #40
Berdir#38: privatemsg-write-permissions-1928502-38.patch queued for re-testing.
Comment #42
ptmkenny CreditAttribution: ptmkenny commentedAdded a test as described in #38.
Comment #44
ptmkenny CreditAttribution: ptmkenny commentedI 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.
Comment #45
ptmkenny CreditAttribution: ptmkenny commentedComment #47
ptmkenny CreditAttribution: ptmkenny commentedForgot to switch receive back to read for the permissions.
Comment #49
ptmkenny CreditAttribution: ptmkenny commented#47: privatemsg-write-permissions-1928502-47-tests-only.patch queued for re-testing.
Comment #51
ivnish CreditAttribution: ivnish commented