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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

litwol’s picture

Status: Reviewed & tested by the community » Active

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

Berdir’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Active » Needs review
NaheemSays’s picture

Just wondering what the benefits are of having things moved to privatemsg_thread_load instead of keeping them in privatemsg_view?

Berdir’s picture

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

NaheemSays’s picture

The 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?

Berdir’s picture

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

NaheemSays’s picture

Status: Needs review » Needs work

This patch no longer applies cleanly and needs to be rerolled.

Also, regarding:

-  $fragments['where'][]       = 'pmi.uid = %d';
-  $fragments['query_args'][]  = $account->uid;
+  if (privatemsg_user_access('read all private messages', $account)) {
+    // if user has read all permission, try to get the message he is the recipient, if not possible, just load the first.
+    $fragments['where'][]       = 'pmi.uid = %d OR pmi.uid = pmi.uid';
+    $fragments['query_args'][]  = $account->uid;
+  }
+  else {
+    $fragments['where'][]       = 'pmi.uid = %d';
+    $fragments['query_args'][]  = $account->uid;
+  }

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

Berdir’s picture

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

Berdir’s picture

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

Berdir’s picture

Status: Needs work » Needs review
NaheemSays’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly and it works.

It did take me a while to figure out:

+    $message = current($thread['messages']);
+    $thread['subject'] = $message['subject'];

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

NaheemSays’s picture

Status: Reviewed & tested by the community » Needs work

This patch no longer applies - Hunks 1 and 2 fail.

Berdir’s picture

Re-roll..

Berdir’s picture

Status: Needs work » Needs review
NaheemSays’s picture

Status: Needs review » Reviewed & tested by the community

Applies cleanly and works as expected.

NaheemSays’s picture

Status: Active » Reviewed & tested by the community
FileSize
7.12 KB

re rolled.

litwol’s picture

Status: Reviewed & tested by the community » Fixed

Good patch, i like that a @todo item was fially completed :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.