Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If you go to the view messages page for a thread you are not a part of, while the messages are hidden, the participants line is not. I think a proper access denied needs to be done so that nothing is shown.
A further question about this:
If there is something attached to the message, I assume that will not be shown as the message is not?
Comment | File | Size | Author |
---|---|---|---|
#18 | privatemsg.load_thread.patch | 7.12 KB | NaheemSays |
#15 | privatemsg.load_thread4.patch | 7.45 KB | Berdir |
#11 | privatemsg.load_thread3.patch | 7.49 KB | Berdir |
#8 | privatemsg.load_thread2.patch | 8.64 KB | Berdir |
#3 | privatemsg.load_thread.patch | 7.4 KB | Berdir |
Comments
Comment #1
litwol CreditAttribution: litwol commentedif user is trying to view a message that he's not part of, he should be denied access to any and all information associated to that message. We should go as far as making the person believe that the message does not *exist* to discourage them trying to continue probing.
This kind of access check should certainly happen on the database level. our query should attach the necessary joins to prevent returning any sql result if user is not participating.
Comment #2
BerdirYes, we could either do that in privatemsg_view_access or (imho, better) in privatemsg_thread_load(). Currently, that function only checks if thread_is an integer. We could improve that and load the thread there. Then we can also properly check the permissions (user is either a recipient or as the read all permission).
When loading the thread there (including recipients and array of messages), we could simplify privatemsg_view, because we don't need to load that there anymore, we just need to display the data returned by privatemsg_thread_load.
Comment #3
BerdirOk, attached is a first patch.
- Fully loads the thread in privatemsg_thread_load now, including participants and messages.
- privatemsg_view only displays the data in $thread.
- renamed ['content'] to ['#value']. That makes it possible to use drupal_render() instead of doing the rendering and sorting on our own.
- above changes cut down privatemsg_view to ~25 loc. of course, much of the removed code has simply be moved to privatemsg_thread_load, but it should be easier to understand now. I even added some inline comments ;)
- added a check in privatemsg_sql_load to always load a message when the user has read all private mesages permission, even if he is not a recipient.
Comment #4
BerdirComment #5
NaheemSays CreditAttribution: NaheemSays commentedJust wondering what the benefits are of having things moved to privatemsg_thread_load instead of keeping them in privatemsg_view?
Comment #6
BerdirI think it is a more clean approach, separating logic from display. We could also more easily re-use the $thread data if we need it somewhere else and drupal automatically displays a 404 error message, if the thread contains no messages.
Comment #7
NaheemSays CreditAttribution: NaheemSays commentedThe read all messages permission does not work on my install (maybe the
OR pmi.uid = pmi.uid
does not work on mysql?)Apart from that it works fine so I am in two minds - set it to rtbc or not? (the read all messages has never worked yet)
Leaving as CNR - maybe not add that bit in yet and leave it for a later patch?
Comment #8
BerdirTo be honest, I haven't really tested that line, I just added it in the idea of not breaking that feature any further. Trying now, it seems that I also need to add the same check to the messages query, or _privatemsg_load will never be called. (obviously).
Also fixed a minor bug with the subject and some notices when someone is viewing a and the reply form isn't displayed.
Comment #9
NaheemSays CreditAttribution: NaheemSays commentedThis patch no longer applies cleanly and needs to be rerolled.
Also, regarding:
I would be more comfortable if the handling of messages of another user was left to the other issue so that its all dealt in one place.
However that can stay in if you feel like it. It just seems extra to this patch (even though the normal procedure here does seem to be to fix as much stuff up as possible with each patch...)
Comment #10
BerdirYes, I know. The reason I haven't yet re-rolled this patch is that I can't find my git branch for this issue anymore -.-.
I'm currently working on the flush issue, will re-roll this after.
I'm am not sure about the read all stuff. My idea was to not break more than it already is. However, it may be better to not include it right now, because I haven't thought much about the "big picture" (on how to handle read all ) yet.
Comment #11
BerdirUpdated a few comments and updated to latest -dev. I removed the read all stuff for now. We can still re-use what I found out here in another patch.
Comment #12
BerdirComment #13
NaheemSays CreditAttribution: NaheemSays commentedPatch applies cleanly and it works.
It did take me a while to figure out:
As it seemed to work without the first line, but then I figured we want the subject of the first message to show as the title, not the one that ios for the last (even though for the majority of cases, they should be the same...)
Setting to rtbc
Comment #14
NaheemSays CreditAttribution: NaheemSays commentedThis patch no longer applies - Hunks 1 and 2 fail.
Comment #15
BerdirRe-roll..
Comment #16
BerdirComment #17
NaheemSays CreditAttribution: NaheemSays commentedApplies cleanly and works as expected.
Comment #18
NaheemSays CreditAttribution: NaheemSays commentedre rolled.
Comment #19
litwol CreditAttribution: litwol commentedGood patch, i like that a @todo item was fially completed :)