Moodle
  1. Moodle
  2. MDL-32082

Add message_current_user_is_involved() to improve readability

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2, 2.3
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      Log in as a student. Go to http://localhost/moodle/dev/master/message/index.php?user=3&viewing=recentconversations where user == the student's user id. Your own recent conversations should be displayed.

      Edit the url to contain the user id of another user. You should get an error.

      Click through the options in the messaging navigation drop down. All pages should display without error.

      Show
      Log in as a student. Go to http://localhost/moodle/dev/master/message/index.php?user=3&viewing=recentconversations where user == the student's user id. Your own recent conversations should be displayed. Edit the url to contain the user id of another user. You should get an error. Click through the options in the messaging navigation drop down. All pages should display without error.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-32082_messaging_readability

      Description

      Originally suggested by Eloy in MDL-31834. Some of the security checks could be made more readable. For example

      if (!message_is_currentuser_any_of($user1, $user2) &&
              !has_capability('moodle/site:readallmessages', $context)) {
          print_error('accessdenied','admin');
      }

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          I'm not sure I like the name. It's a bit confusing.

          Perhaps this function could take an array of users rather than individual elements.

          Show
          Michael de Raadt added a comment - I'm not sure I like the name. It's a bit confusing. Perhaps this function could take an array of users rather than individual elements.
          Hide
          Andrew Davis added a comment -

          Adding testing instructions. Im also marking this a security issue as it more or less gives away what was in a very recent security issue.

          Show
          Andrew Davis added a comment - Adding testing instructions. Im also marking this a security issue as it more or less gives away what was in a very recent security issue.
          Hide
          Ankit Agarwal added a comment -

          Hi Andrew,
          Patch is a perfect...just two small things

          • You missed @return in the php doc block for the function
          • Not sure but, since we are introducing the check inside an API, will it make sense to check if $user->id is set before making the other checks? (will make the function more generic for future use? )

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Andrew, Patch is a perfect...just two small things You missed @return in the php doc block for the function Not sure but, since we are introducing the check inside an API, will it make sense to check if $user->id is set before making the other checks? (will make the function more generic for future use? ) Thanks
          Hide
          Andrew Davis added a comment -

          Adding 2.2 and 2.1 stable branches.

          I've made those changes Ankit. Both the addition of the phpdocs @return and a safety check if an invalid user object is supplied.

          Show
          Andrew Davis added a comment - Adding 2.2 and 2.1 stable branches. I've made those changes Ankit. Both the addition of the phpdocs @return and a safety check if an invalid user object is supplied.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (21, 22 & master)

          I've amended the title to the final name of the function and also have deleted the "security flag" to avoid spreading this as such.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master) I've amended the title to the final name of the function and also have deleted the "security flag" to avoid spreading this as such. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Andrew

          User can see his/her message only. Access denied error appears when trying to view recent conversation of other user.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Andrew User can see his/her message only. Access denied error appears when trying to view recent conversation of other user.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: