Moodle
  1. Moodle
  2. MDL-37164

Messaging: prevent users from interacting with themselves and guest user

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.3, 2.4
    • Fix Version/s: 2.5
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      Log in as a user and go to your messages. Search for yourself and your guest user. Check they dont appear in the users found section.

      Search for another user. Check they appear in the users found section.

      Show
      Log in as a user and go to your messages. Search for yourself and your guest user. Check they dont appear in the users found section. Search for another user. Check they appear in the users found section.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-37164_message_self
    • Rank:
      46737

      Description

      In the messaging, any user can search for the guest account, or even their account. From there they can:

      • Add to contact list
      • Block contact
      • Send messages

      I think we should prevent the users from finding and interacting with themselves and they guest account. This means that hacking the user specifying their own ID or the guest account's one should not work.

      1. Go to messaging
      2. Search for 'guest'
      3. Search for <yourname>

      Actual

      • You find the guest account and yourself and can interact.

      Expected

      • You don't find yourself or the guest account

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment - - edited

          Attaching a patch for master to remove guests and self from the search list (I worked on it on the related issue, hence the patch).

          Show
          Frédéric Massart added a comment - - edited Attaching a patch for master to remove guests and self from the search list (I worked on it on the related issue, hence the patch).
          Hide
          Andrew Davis added a comment -

          Adding a possible fix. This is heavily based on Fred's patch. Putting it up for peer review.

          Show
          Andrew Davis added a comment - Adding a possible fix. This is heavily based on Fred's patch. Putting it up for peer review.
          Hide
          Rajesh Taneja added a comment -

          Patch looks good Andrew, few minor things you might want to consider:

          1. As this is based on Fred's patch, it will be nice to have two commits (one from Fred and other from you), so he can get credit for his work.
          2. Extra space after bracket open and before bracket close. https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1462
          3. Missing dot (.) at the end of comment https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1472
          4. Personally I will move $param just above db call for readability purpose and the do array merge. But leaving it upto you
          5. get_in_or_equal will throw exception for empty exceptions array. Previous code used to neglect that and no exception was thrown for empty array.
          Show
          Rajesh Taneja added a comment - Patch looks good Andrew, few minor things you might want to consider: As this is based on Fred's patch, it will be nice to have two commits (one from Fred and other from you), so he can get credit for his work. Extra space after bracket open and before bracket close. https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1462 Missing dot (.) at the end of comment https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1472 Personally I will move $param just above db call for readability purpose and the do array merge. But leaving it upto you get_in_or_equal will throw exception for empty exceptions array. Previous code used to neglect that and no exception was thrown for empty array.
          Hide
          Andrew Davis added a comment -

          Putting this up for peer review again.

          1. Fred is a HQ developer and knows how git works. If he doesn't provide a branch I'm not going to chase him for one.

          2. I couldnt find this. What line number is this on? The link doesnt appear to work.

          3. Done.

          4. I've reshuffled the code a bit. I think it's a bit more logical this way.

          5. The array supplied to get_in_or_equal() will never be empty. It has two elements added to it immediately above the call.

          Show
          Andrew Davis added a comment - Putting this up for peer review again. 1. Fred is a HQ developer and knows how git works. If he doesn't provide a branch I'm not going to chase him for one. 2. I couldnt find this. What line number is this on? The link doesnt appear to work. 3. Done. 4. I've reshuffled the code a bit. I think it's a bit more logical this way. 5. The array supplied to get_in_or_equal() will never be empty. It has two elements added to it immediately above the call.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          1. If patch is provided by developer, we normally try to keep his/her ownership. Anyways leaving that upto you.
          2. if ( empty($exceptions) ) https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1484
          3. -
          4. -
          5. Make sense. Thanks.
          Show
          Rajesh Taneja added a comment - Thanks Andrew, If patch is provided by developer, we normally try to keep his/her ownership. Anyways leaving that upto you. if ( empty($exceptions) ) https://github.com/andyjdavis/moodle/compare/master...MDL-37164_message_self#L1R1484 - - Make sense. Thanks.
          Hide
          Rajesh Taneja added a comment -

          Also, should you consider int as exceptions ?

          Show
          Rajesh Taneja added a comment - Also, should you consider int as exceptions ?
          Hide
          Andrew Davis added a comment -

          I'm sorry, I don't understand your last comment. Can you rephrase it?

          I've fixed everything else.

          Show
          Andrew Davis added a comment - I'm sorry, I don't understand your last comment. Can you rephrase it? I've fixed everything else.
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          I meant your fix was right, I somehow missed the addition of guest and self.

          Cheers.

          Show
          Rajesh Taneja added a comment - Hello Andrew, I meant your fix was right, I somehow missed the addition of guest and self. Cheers.
          Hide
          Andrew Davis added a comment -

          Adding 2.3 and 2.4 versions. Git cherry-pick was not playing nice so I reimplemented the fix in the various branches which is why the commit IDs don't match.

          Show
          Andrew Davis added a comment - Adding 2.3 and 2.4 versions. Git cherry-pick was not playing nice so I reimplemented the fix in the various branches which is why the commit IDs don't match.
          Hide
          Dan Poltawski added a comment -

          This seems to be an improvement rather than a bug - its not being requested by any outside users, so i'm going to integrate it as such (master only).

          Show
          Dan Poltawski added a comment - This seems to be an improvement rather than a bug - its not being requested by any outside users, so i'm going to integrate it as such (master only).
          Hide
          Dan Poltawski added a comment -

          Integrated to master only. Thanks!

          Show
          Dan Poltawski added a comment - Integrated to master only. Thanks!
          Hide
          Dan Poltawski added a comment -

          And it fails!

          core_message_external_testcase::test_search_contacts
          Failed asserting that actual size 5 matches expected size 6.

          /Users/Shared/Jenkins/Home/git_repositories/master/message/tests/externallib_test.php:359
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Dan Poltawski added a comment - And it fails! core_message_external_testcase::test_search_contacts Failed asserting that actual size 5 matches expected size 6. /Users/Shared/Jenkins/Home/git_repositories/master/message/tests/externallib_test.php:359 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Dan Poltawski added a comment -

          I've fixed that failure (after consulting briefly with thread) as I wanted unit tests passing quickly:
          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=61a73b43c13da988872aab4125a43d0ced322ae9;hp=4c5dc0e3672b4fff4e95c07d94cb7250161e2e6f

          Show
          Dan Poltawski added a comment - I've fixed that failure (after consulting briefly with thread) as I wanted unit tests passing quickly: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=61a73b43c13da988872aab4125a43d0ced322ae9;hp=4c5dc0e3672b4fff4e95c07d94cb7250161e2e6f
          Hide
          Dan Poltawski added a comment -

          Spoke too soon:
          Configuration read from /Users/danp/git/integration/phpunit.xml

          ......E

          Time: 5 seconds, Memory: 64.75Mb

          There was 1 error:

          1) core_message_external_testcase::test_search_contacts
          dml_exception: ERROR: Mixed types of sql query parameters!!

          /Users/danp/git/integration/lib/dml/moodle_database.php:799
          /Users/danp/git/integration/lib/dml/pgsql_native_moodle_database.php:750
          /Users/danp/git/integration/message/lib.php:1545
          /Users/danp/git/integration/message/externallib.php:522
          /Users/danp/git/integration/message/tests/externallib_test.php:359
          /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

          To re-run:
          vendor/bin/phpunit core_message_external_testcase message/tests/externallib_test.php

          FAILURES!
          Tests: 7, Assertions: 45, Errors: 1.

          Show
          Dan Poltawski added a comment - Spoke too soon: Configuration read from /Users/danp/git/integration/phpunit.xml ......E Time: 5 seconds, Memory: 64.75Mb There was 1 error: 1) core_message_external_testcase::test_search_contacts dml_exception: ERROR: Mixed types of sql query parameters!! /Users/danp/git/integration/lib/dml/moodle_database.php:799 /Users/danp/git/integration/lib/dml/pgsql_native_moodle_database.php:750 /Users/danp/git/integration/message/lib.php:1545 /Users/danp/git/integration/message/externallib.php:522 /Users/danp/git/integration/message/tests/externallib_test.php:359 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: vendor/bin/phpunit core_message_external_testcase message/tests/externallib_test.php FAILURES! Tests: 7, Assertions: 45, Errors: 1.
          Hide
          Dan Poltawski added a comment -

          I've reverted this as it seems broken, sorry. Reopening.

          Show
          Dan Poltawski added a comment - I've reverted this as it seems broken, sorry. Reopening.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Davis added a comment -

          I've fixed up the unit tests. Resubmitting for integration.

          Show
          Andrew Davis added a comment - I've fixed up the unit tests. Resubmitting for integration.
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Cheers!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Cheers!
          Hide
          Damyon Wiese added a comment -

          Thanks all, this has been pushed to master only.

          Show
          Damyon Wiese added a comment - Thanks all, this has been pushed to master only.
          Hide
          Jason Fowler added a comment -

          All good Andrew.

          Show
          Jason Fowler added a comment - All good Andrew.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: