From f178087e758b642191c6ecf84a0ecdc235eab18a Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Fri, 22 Sep 2023 19:26:16 +0100 Subject: [PATCH] wip: fixup a few bits, add more tests --- .gitignore | 1 + composer.json | 3 +- extend.php | 2 +- src/Access/UserPolicy.php | 8 +- src/Api/Controllers/CheckIPsController.php | 16 +- src/Commands/CreateBannedIPHandler.php | 2 +- src/Validators/BannedIPValidator.php | 2 +- tests/fixtures/IPAddressesTrait.php | 14 ++ .../api/CreateBannedIPControllerTest.php | 181 ++++++++++++++++++ .../{MiddlewareTest.php => AccessTest.php} | 46 +---- 10 files changed, 225 insertions(+), 50 deletions(-) create mode 100644 tests/integration/api/CreateBannedIPControllerTest.php rename tests/integration/forum/{MiddlewareTest.php => AccessTest.php} (92%) diff --git a/.gitignore b/.gitignore index d34e123..0b4ff3b 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ node_modules vendor composer.lock js/dist +tests/.phpunit.result.cache diff --git a/composer.json b/composer.json index bb04494..77086a2 100644 --- a/composer.json +++ b/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" } } diff --git a/extend.php b/extend.php index 614d3ca..b6313a3 100644 --- a/extend.php +++ b/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; }) diff --git a/src/Access/UserPolicy.php b/src/Access/UserPolicy.php index e26ab76..6fd1fd0 100644 --- a/src/Access/UserPolicy.php +++ b/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); } } diff --git a/src/Api/Controllers/CheckIPsController.php b/src/Api/Controllers/CheckIPsController.php index 353b2b6..109cd0f 100644 --- a/src/Api/Controllers/CheckIPsController.php +++ b/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'), ]); } diff --git a/src/Commands/CreateBannedIPHandler.php b/src/Commands/CreateBannedIPHandler.php index 274fc83..292a852 100644 --- a/src/Commands/CreateBannedIPHandler.php +++ b/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); diff --git a/src/Validators/BannedIPValidator.php b/src/Validators/BannedIPValidator.php index 168fbcd..70a0e24 100644 --- a/src/Validators/BannedIPValidator.php +++ b/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'], ]; diff --git a/tests/fixtures/IPAddressesTrait.php b/tests/fixtures/IPAddressesTrait.php index 9a75107..22a330e 100644 --- a/tests/fixtures/IPAddressesTrait.php +++ b/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)]; + } } diff --git a/tests/integration/api/CreateBannedIPControllerTest.php b/tests/integration/api/CreateBannedIPControllerTest.php new file mode 100644 index 0000000..fae5f77 --- /dev/null +++ b/tests/integration/api/CreateBannedIPControllerTest.php @@ -0,0 +1,181 @@ +prepareDatabase([ + 'banned_ips' => $this->getBannedIPsForDB(), + 'users' => [ + $this->normalUser(), + ['id' => 3, 'username' => 'moderator', 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', 'email' => 'moderator@machine.local', 'is_email_confirmed' => 1, 'last_seen_at' => Carbon::now()->subSecond()], + ['id' => 4, 'username' => 'normal2', 'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', 'email' => 'normal2@machine.local', '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 ... + +} diff --git a/tests/integration/forum/MiddlewareTest.php b/tests/integration/forum/AccessTest.php similarity index 92% rename from tests/integration/forum/MiddlewareTest.php rename to tests/integration/forum/AccessTest.php index f0f24f9..50f4c7b 100644 --- a/tests/integration/forum/MiddlewareTest.php +++ b/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')); } }