MDL-63547 core_message: can_delete_conversation expects a conversationid

message/classes/api.php

@@ -628,17 +628,30 @@ public static function get_profile($userid, $otheruserid) {
      *
      * @param int $userid The user id of who we want to delete the messages for (this may be done by the admin
      *  but will still seem as if it was by the user)
+     * @param int $conversationid The id of the conversation
      * @return bool Returns true if a user can delete the conversation, false otherwise.
      */
-    public static function can_delete_conversation($userid) {
+    public static function can_delete_conversation(int $userid, int $conversationid = null) : bool {
         global $USER;
 
+        if (is_null($conversationid)) {
+            debugging('\core_message\api::can_delete_conversation() now expects a \'conversationid\' to be passed.',
+                DEBUG_DEVELOPER);
+            return false;
+        }
+
         $systemcontext = \context_system::instance();
 
-        // Let's check if the user is allowed to delete this conversation.
-        if (has_capability('moodle/site:deleteanymessage', $systemcontext) ||
-            ((has_capability('moodle/site:deleteownmessage', $systemcontext) &&
-                $USER->id == $userid))) {
+        if (has_capability('moodle/site:deleteanymessage', $systemcontext)) {
+            return true;
+        }
+
+        if (!self::is_user_in_conversation($userid, $conversationid)) {
+            return false;
+        }
+
+        if (has_capability('moodle/site:deleteownmessage', $systemcontext) &&
+                $USER->id == $userid) {
             return true;
         }
 

message/externallib.php

@@ -2587,10 +2587,12 @@ public static function delete_conversation($userid, $otheruserid) {
         $user = core_user::get_user($params['userid'], '*', MUST_EXIST);
         core_user::require_active_user($user);
 
-        if (\core_message\api::can_delete_conversation($user->id)) {
-            if ($conversationid = \core_message\api::get_conversation_between_users([$userid, $otheruserid])) {
-                \core_message\api::delete_conversation_by_id($user->id, $conversationid);
-            }
+        if (!$conversationid = \core_message\api::get_conversation_between_users([$userid, $otheruserid])) {
+            return [];
+        }
+
+        if (\core_message\api::can_delete_conversation($user->id, $conversationid)) {
+            \core_message\api::delete_conversation_by_id($user->id, $conversationid);
             $status = true;
         } else {
             throw new moodle_exception('You do not have permission to delete messages');

message/tests/api_test.php

@@ -1146,17 +1146,26 @@ public function test_can_delete_conversation() {
         $user1 = self::getDataGenerator()->create_user();
         $user2 = self::getDataGenerator()->create_user();
 
+        // Send some messages back and forth.
+        $time = 1;
+        $this->send_fake_message($user1, $user2, 'Yo!', 0, $time + 1);
+        $this->send_fake_message($user2, $user1, 'Sup mang?', 0, $time + 2);
+        $this->send_fake_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 3);
+        $this->send_fake_message($user2, $user1, 'Word.', 0, $time + 4);
+
+        $conversationid = \core_message\api::get_conversation_between_users([$user1->id, $user2->id]);
+
         // The admin can do anything.
-        $this->assertTrue(\core_message\api::can_delete_conversation($user1->id));
+        $this->assertTrue(\core_message\api::can_delete_conversation($user1->id, $conversationid));
 
         // Set as the user 1.
         $this->setUser($user1);
 
         // They can delete their own messages.
-        $this->assertTrue(\core_message\api::can_delete_conversation($user1->id));
+        $this->assertTrue(\core_message\api::can_delete_conversation($user1->id, $conversationid));
 
         // They can't delete someone elses.
-        $this->assertFalse(\core_message\api::can_delete_conversation($user2->id));
+        $this->assertFalse(\core_message\api::can_delete_conversation($user2->id, $conversationid));
     }
 
     /**

message/tests/externallib_test.php

@@ -3445,6 +3445,13 @@ public function test_delete_conversation_as_other_user_without_cap() {
         $user2 = self::getDataGenerator()->create_user();
         $user3 = self::getDataGenerator()->create_user();
 
+        // Send some messages back and forth.
+        $time = time();
+        $this->send_message($user1, $user2, 'Yo!', 0, $time);
+        $this->send_message($user2, $user1, 'Sup mang?', 0, $time + 1);
+        $this->send_message($user1, $user2, 'Writing PHPUnit tests!', 0, $time + 2);
+        $this->send_message($user2, $user1, 'Word.', 0, $time + 3);
+
         // The person wanting to delete the conversation.
         $this->setUser($user3);
 

message/upgrade.txt

@@ -29,6 +29,8 @@ information provided here is intended especially for developers.
 * The following methods have been deprecated and should not be used any more:
   - \core_message\api::is_user_blocked()
   - \core_message\api::delete_conversation()
+* The method \core_message\api::can_delete_conversation() now expects a 'conversationid' to be passed
+  as the second parameter.
 * The following web services have been deprecated. Please do not call these any more.
   - core_message_external::block_contacts, please use core_message_external::block_user instead.
   - core_message_external::unblock_contacts, please use core_message_external::unblock_user instead.