Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.4
    • Fix Version/s: 1.9.9
    • Component/s: Messages
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      24813

      Description

      A non-admin user is unable to see the message history after searching for a message and clicking on the "context" link of an incoming message.

      1. 20091211_mdl_19146.patch
        4 kB
        Rossiani Wijaya
      1. readmessage.png
        5 kB
      2. screenshot.png
        160 kB

        Issue Links

          Activity

          Hide
          Shamim Rezaie added a comment - - edited

          FIX:

          in message/history.php around line 23:
          the following line:
          if (has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM))) { //

          should change to:

          if ($userid1 == $USER->id || has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM))) { // Able to see any discussion

          Show
          Shamim Rezaie added a comment - - edited FIX: in message/history.php around line 23: the following line: if (has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM))) { // should change to: if ($userid1 == $USER->id || has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM))) { // Able to see any discussion
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi, could you please have a look at this and see what you think
          Cheers

          Show
          Sam Hemelryk added a comment - Hi Rossi, could you please have a look at this and see what you think Cheers
          Hide
          Rossiani Wijaya added a comment -

          i think the user needs to have permission to view the message. the permission could be set in Site Admin->users->Permissions ->define roles.

          Show
          Rossiani Wijaya added a comment - i think the user needs to have permission to view the message. the permission could be set in Site Admin->users->Permissions ->define roles.
          Hide
          Shamim Rezaie added a comment - - edited

          Hi,

          The problem is not about permission.
          The exact problem is that users are not able to see the history of chats that have been initiated by others.
          It needs a code change to be fixed (I mentioned the needed change in my first comment. the change should be applied to line 55).
          I attached an image to illustrate the problem.

          Show
          Shamim Rezaie added a comment - - edited Hi, The problem is not about permission. The exact problem is that users are not able to see the history of chats that have been initiated by others. It needs a code change to be fixed (I mentioned the needed change in my first comment. the change should be applied to line 55). I attached an image to illustrate the problem.
          Hide
          Rossiani Wijaya added a comment -

          Hi Shamim,

          I managed to view the chat context initiated by others without updating the code. Could you try to update to the latest version of moodle?

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Shamim, I managed to view the chat context initiated by others without updating the code. Could you try to update to the latest version of moodle? Thanks Rosie
          Hide
          Shamim Rezaie added a comment -

          Hi Rosie,

          I took that snapshot from moodle.org which I believe uses the latest version.

          Show
          Shamim Rezaie added a comment - Hi Rosie, I took that snapshot from moodle.org which I believe uses the latest version.
          Hide
          Rossiani Wijaya added a comment -

          Hi Shamim,

          I re-tested the issue and confirm that the issue could be fixed with define roles.

          In order to be able to read all the messages, user's role need to have 'allow' permission.

          when i tested the issue by setting 'read all messages on site' to not set, I get the same issue as yours. Then i change the permission to allow, I am able to view all the messages.

          so could you check that your user's role has 'read all messages on site' permission set to allow.

          I attached the row for setting up the read messages permission for the user define roles.

          Thanks
          rosie

          Show
          Rossiani Wijaya added a comment - Hi Shamim, I re-tested the issue and confirm that the issue could be fixed with define roles. In order to be able to read all the messages, user's role need to have 'allow' permission. when i tested the issue by setting 'read all messages on site' to not set, I get the same issue as yours. Then i change the permission to allow, I am able to view all the messages. so could you check that your user's role has 'read all messages on site' permission set to allow. I attached the row for setting up the read messages permission for the user define roles. Thanks rosie
          Hide
          Shamim Rezaie added a comment -

          Hi Rosie,

          Thank you for taking care of this issue.
          To be honest, I think that no one (except admins) should have "read all messages" permission. Having this permission will allow users to read all messages, even those that are not related to them. i.e. chat history of other users.

          Shamim

          Show
          Shamim Rezaie added a comment - Hi Rosie, Thank you for taking care of this issue. To be honest, I think that no one (except admins) should have "read all messages" permission. Having this permission will allow users to read all messages, even those that are not related to them. i.e. chat history of other users. Shamim
          Hide
          Shamim Rezaie added a comment -

          Hi Rosie,

          As nothing has been done yet, I just am curios why you marked this issue as resolved.

          Thanks
          Shamim

          Show
          Shamim Rezaie added a comment - Hi Rosie, As nothing has been done yet, I just am curios why you marked this issue as resolved. Thanks Shamim
          Hide
          Rossiani Wijaya added a comment -

          The issue is resolved because in order to view the history message, user's role need to have read message permission and it could be set in site admin.

          Show
          Rossiani Wijaya added a comment - The issue is resolved because in order to view the history message, user's role need to have read message permission and it could be set in site admin.
          Hide
          Sam Hemelryk added a comment -

          Hi guys, I've had a read through the notes and played around with the system and have come to the following conclusions.

          Rosie: Users should always me able to see conversations that they participated in.

          Shamim: The if statement that you suggested be modified has an else - if the user does not have the `moodle/site:readallmessages` then user2 is automatically set to the current user.
          This means that user1 should always be the other party, whether the instantiated the conversation or not.
          As long as user1 is the other user the modification you suggested should not be needed.
          In saying that if you are able to produce the situation where you are only seeing your own messages then chances are there is a history link that is getting user1 and user2 around the wrong way.
          If you can reproduce the error could you please do the following:

          1. Find out the user id of the user you will be login as.
          2. Take note of the link that takes you to the history page and make note of the user1 and user2 arguments on it. user1 should be the id of the other user, user2 should be the id of the user you are logged in as.
          3. Let us know how you go

          If everything is fine then we will go back to the code and look for further things that may have gone wrong
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've had a read through the notes and played around with the system and have come to the following conclusions. Rosie: Users should always me able to see conversations that they participated in. Shamim: The if statement that you suggested be modified has an else - if the user does not have the `moodle/site:readallmessages` then user2 is automatically set to the current user. This means that user1 should always be the other party, whether the instantiated the conversation or not. As long as user1 is the other user the modification you suggested should not be needed. In saying that if you are able to produce the situation where you are only seeing your own messages then chances are there is a history link that is getting user1 and user2 around the wrong way. If you can reproduce the error could you please do the following: Find out the user id of the user you will be login as. Take note of the link that takes you to the history page and make note of the user1 and user2 arguments on it. user1 should be the id of the other user, user2 should be the id of the user you are logged in as. Let us know how you go If everything is fine then we will go back to the code and look for further things that may have gone wrong Cheers Sam
          Hide
          Shamim Rezaie added a comment -

          Hi Rosie
          Thank you for your reply.

          I still think that nobody (except admin) should have "readallmessages" privilege. Because by having that privilege, the user would be able to see the chat history of ANY user. I mean if user A has that privilege, he would be able to read the history of a chat between user B and user C.

          Show
          Shamim Rezaie added a comment - Hi Rosie Thank you for your reply. I still think that nobody (except admin) should have "readallmessages" privilege. Because by having that privilege, the user would be able to see the chat history of ANY user. I mean if user A has that privilege, he would be able to read the history of a chat between user B and user C.
          Hide
          Shamim Rezaie added a comment -

          Hi Sam,

          // ...
          } else

          { $userid2 = $USER->id; // Can only see messages involving yourself $user2 = $USER; }

          regarding the ELSE statement that you mentioned, I'd like to say:

          We want to see the chat history between $user1 and $user2.
          The code (current implementation) always assumes that $user2 is current logged in user which is NOT always true.

          Whenever a user wants to see the history of one of his/her chats that has been initiated by another party, $user2 would be that party and $user1 would be current logged in user.

          So by executing code, we will go to the ELSE branch because the user has not (and should not have in normal case) required privilege. Then (inside the ELSE branch) the value of $user2 would change to $USER->id which is current logged in users id. So in this case both $user1 and $user2 would refer to logged in user and the user would end up with the history of chats with himself.

          Show
          Shamim Rezaie added a comment - Hi Sam, // ... } else { $userid2 = $USER->id; // Can only see messages involving yourself $user2 = $USER; } regarding the ELSE statement that you mentioned, I'd like to say: We want to see the chat history between $user1 and $user2. The code (current implementation) always assumes that $user2 is current logged in user which is NOT always true. Whenever a user wants to see the history of one of his/her chats that has been initiated by another party, $user2 would be that party and $user1 would be current logged in user. So by executing code, we will go to the ELSE branch because the user has not (and should not have in normal case) required privilege. Then (inside the ELSE branch) the value of $user2 would change to $USER->id which is current logged in users id. So in this case both $user1 and $user2 would refer to logged in user and the user would end up with the history of chats with himself.
          Hide
          Sam Hemelryk added a comment -

          Hi Shamim,

          Thats all correct, if user1 is the currently logged in user and user2 is other party then inevitably you will only end up seeing message that you have sent and that is the issue here.

          However regarding the solution to it all of the methods within the messaging API regarding history designed so for user2 being the currently logged in user, the only exception to this is one call to message_history_link() in lib.php:line465

          I think the most appropriate solution to this is to both fix the call to message_history_link() and add support to history.php so that it doesn't matter whether user1 or user2 is the current user things work and all security checks are requirements still occur.

          This way the code still conforms to the way in which messaging was designed and should there be other instances where user1 and user2 end up the wrong way round things still work.

          What you yourself and Rossi think?
          And Shamim was the it the history link after a keyword search that was leading you, or are there more links like this one?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Shamim, Thats all correct, if user1 is the currently logged in user and user2 is other party then inevitably you will only end up seeing message that you have sent and that is the issue here. However regarding the solution to it all of the methods within the messaging API regarding history designed so for user2 being the currently logged in user, the only exception to this is one call to message_history_link() in lib.php:line465 I think the most appropriate solution to this is to both fix the call to message_history_link() and add support to history.php so that it doesn't matter whether user1 or user2 is the current user things work and all security checks are requirements still occur. This way the code still conforms to the way in which messaging was designed and should there be other instances where user1 and user2 end up the wrong way round things still work. What you yourself and Rossi think? And Shamim was the it the history link after a keyword search that was leading you, or are there more links like this one? Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          +1 for not caring whether the current user is user1 or user2. I think it will make the code more robust.

          Show
          Rossiani Wijaya added a comment - +1 for not caring whether the current user is user1 or user2. I think it will make the code more robust.
          Hide
          Shamim Rezaie added a comment -

          So now that everyone know and accept the problem, I'd like to point to my suggested fix:

          as a quick fix, by modifying the mentioned IF to
          if ($userid1 == $USER->id || has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM)))
          the problem would be fixed

          Show
          Shamim Rezaie added a comment - So now that everyone know and accept the problem, I'd like to point to my suggested fix: as a quick fix, by modifying the mentioned IF to if ($userid1 == $USER->id || has_capability('moodle/site:readallmessages', get_context_instance(CONTEXT_SYSTEM))) the problem would be fixed
          Hide
          Sam Hemelryk added a comment -

          Hi Shamim,

          history.php will certainly be fixed, but the solution will be a bit bigger than what you are suggesting so as to make the code clearer and remove the redundant database calls if user1 is the current user or user2 is the current user.
          Rosie is working on a patch at the moment that will clean up the code in history.php as well as fix the broken link in lib.php. Hopefully she will have a patch uploaded here shortly.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Shamim, history.php will certainly be fixed, but the solution will be a bit bigger than what you are suggesting so as to make the code clearer and remove the redundant database calls if user1 is the current user or user2 is the current user. Rosie is working on a patch at the moment that will clean up the code in history.php as well as fix the broken link in lib.php. Hopefully she will have a patch uploaded here shortly. Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          Hi all,

          I created a patch for history.php and lib.php in order to fix the issue.

          Rosie

          Show
          Rossiani Wijaya added a comment - Hi all, I created a patch for history.php and lib.php in order to fix the issue. Rosie
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi,
          That patch looks good thank you.
          Shamim are you happy with that patch? thank you for sticking with this issue and pushing it through by the way.
          It is great when someone in the community actually goes out of their way to look into a problem and find a solution to it.
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rossi, That patch looks good thank you. Shamim are you happy with that patch? thank you for sticking with this issue and pushing it through by the way. It is great when someone in the community actually goes out of their way to look into a problem and find a solution to it. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi,

          Did this end up being commit or is it still in the waiting?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rossi, Did this end up being commit or is it still in the waiting? Cheers Sam
          Hide
          Andrew Davis added a comment -

          Hello everyone. I believe that this issue has been resolved by the recent 2.0 refactoring. Does this still need to be resolved in 1.9.X?

          Show
          Andrew Davis added a comment - Hello everyone. I believe that this issue has been resolved by the recent 2.0 refactoring. Does this still need to be resolved in 1.9.X?
          Hide
          Rossiani Wijaya added a comment -

          Commit patch to 1.9_stable.

          Thanks everyone.

          Resolved.

          Show
          Rossiani Wijaya added a comment - Commit patch to 1.9_stable. Thanks everyone. Resolved.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: