wip: fixup a few bits, add more tests

.gitignore

@@ -2,3 +2,4 @@ node_modules
 vendor
 composer.lock
 js/dist
+tests/.phpunit.result.cache

composer.json

@@ -85,7 +85,6 @@
         "test:setup": "Sets up a database for use with integration tests. Execute this only once."
     },
     "require-dev": {
-        "flarum/testing": "^1.0.0",
-        "symfony/var-dumper": "*"
+        "flarum/testing": "^1.0.0"
     }
 }

extend.php

@@ -62,7 +62,7 @@
 
     (new Extend\ApiSerializer(Serializer\PostSerializer::class))
         ->attributes(function (AbstractSerializer $serializer, AbstractModel $post, array $attributes): array {
-            $attributes['canBanIP'] = $serializer->getActor()->can('banIP', $post);
+            $attributes['canBanIP'] = $serializer->getActor()->can('banIP', $post->user);
 
             return $attributes;
         })

src/Access/UserPolicy.php

@@ -24,12 +24,12 @@ class UserPolicy extends AbstractPolicy
      *
      * @return bool|null
      */
-    public function banIP(User $actor, User $user)
+    public function banIP(User $actor, ?User $user)
     {
-        if ($user == null || $actor->id == $user->id || $user->can($this->key)) {
-            return false;
+        if ($user && ($actor->id === $user->id || $user->hasPermission($this->key))) {
+            return $this->deny();
         }
 
-        return $actor->can($this->key, $user);
+        return $actor->hasPermission($this->key);
     }
 }

src/Api/Controllers/CheckIPsController.php

@@ -21,6 +21,7 @@
 use FoF\BanIPs\Validators\BannedIPValidator;
 use Illuminate\Support\Arr;
 use Psr\Http\Message\ServerRequestInterface;
+use Symfony\Contracts\Translation\TranslatorInterface;
 use Tobscure\JsonApi\Document;
 
 class CheckIPsController extends AbstractListController
@@ -33,21 +34,27 @@ class CheckIPsController extends AbstractListController
     /**
      * @var BannedIPRepository
      */
-    private $bannedIPs;
+    protected $bannedIPs;
 
     /**
      * @var BannedIPValidator
      */
-    private $validator;
+    protected $validator;
+
+    /**
+     * @var TranslatorInterface
+     */
+    protected $translator;
 
     /**
      * @param BannedIPRepository $bannedIPs
      * @param BannedIPValidator  $validator
      */
-    public function __construct(BannedIPRepository $bannedIPs, BannedIPValidator $validator)
+    public function __construct(BannedIPRepository $bannedIPs, BannedIPValidator $validator, TranslatorInterface $translator)
     {
         $this->bannedIPs = $bannedIPs;
         $this->validator = $validator;
+        $this->translator = $translator;
     }
 
     /**
@@ -75,6 +82,7 @@ protected function data(ServerRequestInterface $request, Document $document)
 
         $userId = Arr::get($params, 'user');
         $ip = Arr::get($params, 'ip');
+
         $user = $userId != null ? User::where('id', $userId)->orWhere('username', $userId)->first() : null;
 
         $actor->assertCan('banIP', $user);
@@ -94,7 +102,7 @@ protected function data(ServerRequestInterface $request, Document $document)
 
         if (empty($ips)) {
             throw new ValidationException([
-                resolve('translator')->trans('fof-ban-ips.error.no_ips_found_message'),
+                $this->translator->trans('fof-ban-ips.error.no_ips_found_message'),
             ]);
         }
 

src/Commands/CreateBannedIPHandler.php

@@ -55,7 +55,7 @@ public function handle(CreateBannedIP $command)
         $data = $command->data;
 
         $userId = Arr::get($data, 'attributes.userId');
-        $user = $userId != null ? User::where('id', $userId)->orWhere('username', $userId)->firstOrFail() : null;
+        $user = $userId ? User::query()->where('id', $userId)->orWhere('username', $userId)->firstOrFail() : null;
 
         $actor->assertCan('banIP', $user);
 

src/Validators/BannedIPValidator.php

@@ -16,7 +16,7 @@
 class BannedIPValidator extends AbstractValidator
 {
     protected $rules = [
-        'userId'  => ['required', 'integer'],
+        'creatorId'  => ['required', 'integer'],
         'address' => ['required', 'ip', 'unique:banned_ips,address'],
         'reason'  => ['nullable', 'string'],
     ];

tests/fixtures/IPAddressesTrait.php

@@ -122,4 +122,18 @@ public function getRandomNotBannedIPv6(): string
 
         return $notBannedIPs[array_rand($notBannedIPs)];
     }
+
+    public function getRandomBannedIP(): string
+    {
+        $bannedIPs = $this->getAllBanned();
+
+        return $bannedIPs[array_rand($bannedIPs)];
+    }
+
+    public function getRandomNotBannedIP(): string
+    {
+        $notBannedIPs = $this->getAllNotBanned();
+
+        return $notBannedIPs[array_rand($notBannedIPs)];
+    }
 }

tests/integration/api/CreateBannedIPControllerTest.php

@@ -0,0 +1,181 @@
+<?php
+
+namespace FoF\BanIPs\Tests\integration\api;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\TestCase;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use FoF\BanIPs\Tests\fixtures\IPAddressesTrait;
+
+class CreateBannedIPControllerTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+    use IPAddressesTrait;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->prepareDatabase([
+            'banned_ips' => $this->getBannedIPsForDB(),
+            'users' => [
+                $this->normalUser(),
+                ['id' => 3, 'username' => 'moderator', 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', 'email' => '[email protected]', 'is_email_confirmed' => 1, 'last_seen_at' => Carbon::now()->subSecond()],
+                ['id' => 4, 'username' => 'normal2', 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', 'email' => '[email protected]', 'is_email_confirmed' => 1, 'last_seen_at' => Carbon::now()->subSecond()],
+            ],
+            'group_user' => [
+                ['group_id' => 4, 'user_id' => 3],
+            ],
+            'group_permission' => [
+                ['group_id' => 4, 'permission' => 'fof.ban-ips.banIP'],
+                ['group_id' => 4, 'permission' => 'fof.ban-ips.viewBannedIPList'],
+                ['group_id' => 4, 'permission' => 'discussion.viewIpsPosts']
+            ]
+        ]);
+
+        $this->extension('fof-ban-ips');
+    }
+
+    public function adminAndModeratorUserIdProvider(): array
+    {
+        return [
+            ['actorId' => 1, 'userId' => 2],
+            ['actorId' => 1, 'userId' => null],
+            ['actorId' => 3, 'userId' => 2],
+            ['actorId' => 3, 'userId' => null]
+        ];
+    }
+
+    public function test_user_cannot_create_ip_ban_with_no_data()
+    {
+        $response = $this->send(
+            $this->request('POST', '/api/fof/ban-ips', [
+                'authenticatedAs' => 1,
+                'json' => [],
+            ])
+        );
+
+        $this->assertEquals(422, $response->getStatusCode());
+    }
+
+    /**
+     * @dataProvider adminAndModeratorUserIdProvider
+     */
+    public function test_user_with_permission_can_ban_ip_not_already_banned(int $actorId, ?int $userId)
+    {
+        $response = $this->send(
+            $this->request('POST', '/api/fof/ban-ips', [
+                'authenticatedAs' => $actorId,
+                'json' => [
+                    'data' => [
+                        'attributes' => [
+                            'address' => $this->getIPv6NotBanned()[2],
+                            'reason' => 'Testing',
+                            'userId' => $userId
+                        ]
+                    ]
+                ],
+            ])
+        );
+
+        $this->assertEquals(201, $response->getStatusCode());
+
+        $body = $response->getBody();
+
+        $this->assertJson($body);
+
+        $attrs = json_decode($body, true)['data']['attributes'];
+
+        $this->assertEquals($this->getIPv6NotBanned()[2], $attrs['address']);
+        $this->assertEquals('Testing', $attrs['reason']);
+        $this->assertEquals($userId, $attrs['userId']);
+        $this->assertEquals($actorId, $attrs['creatorId']);
+    }
+
+    // /**
+    //  * @dataProvider adminAndModeratorUserIdProvider
+    //  */
+    // public function test_user_with_permission_can_ban_ip_not_already_banned_without_associated_user(int $userId)
+    // {
+    //     $response = $this->send(
+    //         $this->request('POST', '/api/fof/ban-ips', [
+    //             'authenticatedAs' => $userId,
+    //             'json' => [
+    //                 'data' => [
+    //                     'attributes' => [
+    //                         'address' => $this->getIPv4NotBanned()[0],
+    //                         'reason' => 'Testing',
+    //                     ]
+    //                 ]
+    //             ],
+    //         ])
+    //     );
+
+    //     $this->assertEquals(201, $response->getStatusCode());
+
+    //     $body = $response->getBody();
+
+    //     $this->assertJson($body);
+
+    //     $attrs = json_decode($body, true)['data']['attributes'];
+
+    //     $this->assertEquals($this->getIPv4NotBanned()[0], $attrs['address']);
+    //     $this->assertEquals('Testing', $attrs['reason']);
+    //     $this->assertNull($attrs['userId']);
+    //     $this->assertEquals($userId, $attrs['creatorId']);
+    // }
+
+    public function test_user_with_permission_cannot_ban_ip_already_banned()
+    {
+        $response = $this->send(
+            $this->request('POST', '/api/fof/ban-ips', [
+                'authenticatedAs' => 3,
+                'json' => [
+                    'data' => [
+                        'attributes' => [
+                            'address' => $this->getIPv6Banned()[1],
+                            'reason' => 'Testing',
+                        ]
+                    ]
+                ],
+            ])
+        );
+
+        $this->assertEquals(422, $response->getStatusCode());
+
+        $body = (string) $response->getBody();
+        $this->assertJson($body);
+        $this->assertEquals([
+            'errors' => [
+                [
+                    'status' => '422',
+                    'code'   => 'validation_error',
+                    'detail' => 'The address has already been taken.',
+                    'source' => ['pointer' => '/data/attributes/address'],
+                ],
+            ],
+        ], json_decode($body, true));
+    }
+
+    public function test_user_without_permission_cannnot_ban_ip()
+    {
+        $response = $this->send(
+            $this->request('POST', '/api/fof/ban-ips', [
+                'authenticatedAs' => 2,
+                'json' => [
+                    'data' => [
+                        'attributes' => [
+                            'address' => $this->getIPv4NotBanned()[1],
+                            'reason' => 'Testing',
+                        ]
+                    ]
+                ],
+            ])
+        );
+
+        $this->assertEquals(403, $response->getStatusCode());
+    }
+
+    // ... More test cases ...
+
+}

tests/integration/forum/MiddlewareTest.php → tests/integration/forum/AccessTest.php

@@ -19,7 +19,7 @@
 use FoF\BanIPs\Tests\fixtures\IPAddressesTrait;
 use FoF\BanIPs\Tests\fixtures\IPRequestTrait;
 
-class MiddlewareTest extends TestCase
+class AccessTest extends TestCase
 {
     use RetrievesAuthorizedUsers;
     use IPAddressesTrait;
@@ -140,37 +140,6 @@ public function test_not_banned_ips_can_register($notBannedIP)
         $this->assertEquals(201, $response->getStatusCode());
     }
 
-    /**
-     * @dataProvider bannedIPsProvider
-     */
-    public function test_banned_ip_can_login_when_user_is_not_ip_banned($bannedIP)
-    {
-        $response = $this->send(
-            $this->enhancedRequest('POST', '/login', [
-                'serverParams' => [
-                    'REMOTE_ADDR' => $bannedIP,
-                ],
-                'json' => [
-                    'identification' => 'normal',
-                    'password'       => 'too-obscure',
-                ],
-            ])
-        );
-
-        $this->assertEquals(200, $response->getStatusCode());
-
-        // The response body should contain the user ID...
-        $body = (string) $response->getBody();
-        $this->assertJson($body);
-
-        $data = json_decode($body, true);
-        $this->assertEquals(2, $data['userId']);
-
-        // ...and an access token belonging to this user.
-        $token = $data['token'];
-        $this->assertEquals(2, AccessToken::whereToken($token)->firstOrFail()->user_id);
-    }
-
     /**
      * @dataProvider notBannedIPsProvider
      */
@@ -426,10 +395,11 @@ public function test_not_banned_user_is_able_to_access_restriced_path_from_not_b
 
     public function test_banned_user_cannot_access_restricted_path_from_banned_ip()
     {
+        // For the purposes of the test, we will login from a non-banned IP
         $response = $this->send(
             $this->enhancedRequest('POST', '/login', [
                 'serverParams' => [
-                    'REMOTE_ADDR' => $this->getIPv4Banned()[1],
+                    'REMOTE_ADDR' => $this->getIPv4NotBanned()[1],
                 ],
                 'json' => [
                     'identification' => 'ipBanned',
@@ -438,15 +408,17 @@ public function test_banned_user_cannot_access_restricted_path_from_banned_ip()
             ])
         );
 
+        // Make sure we have logged in successfully
+        $this->assertEquals(200, $response->getStatusCode());
+
+        // Now send another request, which is authenticated with a cookie, but from a banned IP
         $response = $this->send($this->enhancedRequest('GET', '/settings', [
             'serverParams'    => ['REMOTE_ADDR' => $this->getIPv4Banned()[1]],
             'cookiesFrom'     => $response,
-            'authenticatedAs' => 3,
         ]));
 
         // assert that we are redirected to /logout
-        //$this->assertEquals(302, $response->getStatusCode());
-
-        $this->assertEquals(401, $response->getStatusCode());
+        $this->assertEquals(302, $response->getStatusCode());
+        $this->assertStringContainsString('/logout?token=', $response->getHeaderLine('Location'));
     }
 }