From f7c0a3b7e95c1fecae9c1fcfae39fedd0149c3b0 Mon Sep 17 00:00:00 2001
From: IanM <ian@morland.me>
Date: Sun, 16 Jun 2024 11:45:28 +0100
Subject: [PATCH] fix: suspended users can remove avatar

---
 extensions/suspend/extend.php                 |  4 +-
 .../PreventAvatarDeletionBySuspendedUser.php  | 18 ++++
 .../api/users/RemoveAvatarTest.php            | 89 +++++++++++++++++++
 .../src/User/Command/DeleteAvatarHandler.php  |  4 +-
 4 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 extensions/suspend/src/Listener/PreventAvatarDeletionBySuspendedUser.php
 create mode 100644 extensions/suspend/tests/integration/api/users/RemoveAvatarTest.php

diff --git a/extensions/suspend/extend.php b/extensions/suspend/extend.php
index fe924d00b7..5a94d7433e 100644
--- a/extensions/suspend/extend.php
+++ b/extensions/suspend/extend.php
@@ -19,6 +19,7 @@
 use Flarum\Suspend\Notification\UserUnsuspendedBlueprint;
 use Flarum\Suspend\Query\SuspendedFilterGambit;
 use Flarum\Suspend\RevokeAccessFromSuspendedUsers;
+use Flarum\User\Event\AvatarDeleting;
 use Flarum\User\Event\Saving;
 use Flarum\User\Filter\UserFilterer;
 use Flarum\User\Search\UserSearcher;
@@ -50,7 +51,8 @@
     (new Extend\Event())
         ->listen(Saving::class, Listener\SaveSuspensionToDatabase::class)
         ->listen(Suspended::class, Listener\SendNotificationWhenUserIsSuspended::class)
-        ->listen(Unsuspended::class, Listener\SendNotificationWhenUserIsUnsuspended::class),
+        ->listen(Unsuspended::class, Listener\SendNotificationWhenUserIsUnsuspended::class)
+        ->listen(AvatarDeleting::class, Listener\PreventAvatarDeletionBySuspendedUser::class),
 
     (new Extend\Policy())
         ->modelPolicy(User::class, UserPolicy::class),
diff --git a/extensions/suspend/src/Listener/PreventAvatarDeletionBySuspendedUser.php b/extensions/suspend/src/Listener/PreventAvatarDeletionBySuspendedUser.php
new file mode 100644
index 0000000000..9d3595e394
--- /dev/null
+++ b/extensions/suspend/src/Listener/PreventAvatarDeletionBySuspendedUser.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Flarum\Suspend\Listener;
+
+use Flarum\User\Exception\PermissionDeniedException;
+
+class PreventAvatarDeletionBySuspendedUser
+{
+    public function handle($event)
+    {
+        $actor = $event->actor;
+        $user = $event->user;
+
+        if ($actor->id === $user->id && $user->suspended_until) {
+            throw new PermissionDeniedException();
+        }
+    }
+}
diff --git a/extensions/suspend/tests/integration/api/users/RemoveAvatarTest.php b/extensions/suspend/tests/integration/api/users/RemoveAvatarTest.php
new file mode 100644
index 0000000000..a2b9fc9ef7
--- /dev/null
+++ b/extensions/suspend/tests/integration/api/users/RemoveAvatarTest.php
@@ -0,0 +1,89 @@
+<?php
+
+namespace Flarum\Suspend\Tests\integration\api\users;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Psr\Http\Message\ResponseInterface;
+
+class RemoveAvatarTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('flarum-suspend');
+
+        $this->prepareDatabase([
+            'users' => [
+                ['id' => 1, 'username' => 'Muralf', 'email' => 'muralf@machine.local', 'is_email_confirmed' => 1],
+                $this->normalUser(),
+                ['id' => 3, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1, 'suspended_until' => Carbon::now()->addDay(), 'suspend_message' => 'You have been suspended.', 'suspend_reason' => 'Suspended for acme reasons.'],
+                ['id' => 4, 'username' => 'acme4', 'email' => 'acme4@machine.local', 'is_email_confirmed' => 1],
+                ['id' => 5, 'username' => 'acme5', 'email' => 'acme5@machine.local', 'is_email_confirmed' => 1, 'suspended_until' => Carbon::now()->subDay(), 'suspend_message' => 'You have been suspended.', 'suspend_reason' => 'Suspended for acme reasons.'],
+            ],
+            'groups' => [
+                ['id' => 5, 'name_singular' => 'can_edit_users', 'name_plural' => 'can_edit_users', 'is_hidden' => 0]
+            ],
+            'group_user' => [
+                ['user_id' => 2, 'group_id' => 5]
+            ],
+            'group_permission' => [
+                ['permission' => 'user.edit', 'group_id' => 5],
+            ]
+        ]);
+    }
+
+    /**
+     * @test
+     * @dataProvider allowedToRemoveAvatar
+     */
+    public function can_remove_avatar_if_allowed(?int $authenticatedAs, int $targetUserId)
+    {
+        $response = $this->sendRemoveAvatarRequest($authenticatedAs, $targetUserId);
+
+        $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents());
+    }
+
+    /**
+     * @test
+     * @dataProvider notAllowedToRemoveAvatar
+     */
+    public function cannot_remove_avatar_if_not_allowed(?int $authenticatedAs, int $targetUserId)
+    {
+        $response = $this->sendRemoveAvatarRequest($authenticatedAs, $targetUserId);
+
+        $this->assertEquals(403, $response->getStatusCode(), $response->getBody()->getContents());
+    }
+
+    public function allowedToRemoveAvatar(): array
+    {
+        return [
+            [1, 4, 'Admin can remove avatar of normal user'],
+            [4, 4, 'Normal user can remove their own avatar'],
+            [1, 3, 'Admin can remove avatar of suspended user'],
+            [2, 3, 'Normal user with permission can remove avatar of suspended user'],
+        ];
+    }
+
+    public function notAllowedToRemoveAvatar(): array
+    {
+        return [
+            [4, 2, 'Normal user cannot remove avatar of another user'],
+            [4, 3, 'Normal user cannot remove avatar of suspended user'],
+            [3, 3, 'Suspended user cannot remove their own avatar'],
+        ];
+    }
+
+    protected function sendRemoveAvatarRequest(?int $authenticatedAs, int $targetUserId): ResponseInterface
+    {
+        return $this->send(
+            $this->request('DELETE', "/api/users/$targetUserId/avatar", [
+                'authenticatedAs' => $authenticatedAs,
+            ])
+        );
+    }
+}
diff --git a/framework/core/src/User/Command/DeleteAvatarHandler.php b/framework/core/src/User/Command/DeleteAvatarHandler.php
index f54954a4af..3784342b76 100644
--- a/framework/core/src/User/Command/DeleteAvatarHandler.php
+++ b/framework/core/src/User/Command/DeleteAvatarHandler.php
@@ -56,12 +56,12 @@ public function handle(DeleteAvatar $command)
             $actor->assertCan('edit', $user);
         }
 
-        $this->uploader->remove($user);
-
         $this->events->dispatch(
             new AvatarDeleting($user, $actor)
         );
 
+        $this->uploader->remove($user);
+
         $user->save();
 
         $this->dispatchEventsFor($user, $actor);