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
    • Rank:
      38779

      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');
      }
      

        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: