diff --git a/README.md b/README.md index e9404cb..6c13195 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,14 @@ return [ */ 'log_registration_exception' => true, + + /* + * When set to true, the required permission/role names are added to the exception + * message. This could be considered an information leak in some contexts, so + * the default setting is false here for optimum safety. + */ + + 'display_permission_in_exception' => false, ]; ``` diff --git a/composer.json b/composer.json index b01b5db..ce69bc3 100644 --- a/composer.json +++ b/composer.json @@ -40,7 +40,7 @@ "codeclimate/php-test-reporter": "^0.4.4", "monolog/monolog": "^1.23", "orchestra/testbench": "^3.2.0", - "phpunit/phpunit": "^5.7|^6.0", + "phpunit/phpunit": "^5.7|^6.0|^7.0", "squizlabs/php_codesniffer": "^3.1" }, "autoload": { diff --git a/config/permission.php b/config/permission.php index 7a4eab9..6f79028 100644 --- a/config/permission.php +++ b/config/permission.php @@ -53,4 +53,21 @@ */ 'cache_expiration_time' => 60 * 24, + + /* + * By default we'll make an entry in the application log when the permissions + * could not be loaded. Normally this only occurs while installing the packages. + * + * If for some reason you want to disable that logging, set this value to false. + */ + + 'log_registration_exception' => true, + + /* + * When set to true, the required permission/role names are added to the exception + * message. This could be considered an information leak in some contexts, so + * the default setting is false here for optimum safety. + */ + + 'display_permission_in_exception' => false, ]; diff --git a/src/Exceptions/UnauthorizedException.php b/src/Exceptions/UnauthorizedException.php index bbf44b3..5545cc5 100644 --- a/src/Exceptions/UnauthorizedException.php +++ b/src/Exceptions/UnauthorizedException.php @@ -2,7 +2,6 @@ namespace Maklad\Permission\Exceptions; -use http\Exception; use Symfony\Component\HttpKernel\Exception\HttpException; /** @@ -11,14 +10,18 @@ */ class UnauthorizedException extends HttpException { + private $requiredRoles = []; + private $requiredPermissions = []; /** * UnauthorizedException constructor. * * @param $statusCode * @param null $message + * @param array $requiredRoles + * @param array $requiredPermissions */ - public function __construct($statusCode, $message = null) + public function __construct($statusCode, $message = null, $requiredRoles = [], $requiredPermissions = []) { parent::__construct($statusCode, $message); @@ -26,5 +29,28 @@ public function __construct($statusCode, $message = null) $logger = \app('log'); $logger->alert($message); } + + $this->requiredRoles = $requiredRoles; + $this->requiredPermissions = $requiredPermissions; + } + + /** + * Return Required Roles + * + * @return array + */ + public function getRequiredRoles(): array + { + return $this->requiredRoles; + } + + /** + * Return Required Permissions + * + * @return array + */ + public function getRequiredPermissions(): array + { + return $this->requiredPermissions; } } diff --git a/src/Exceptions/UnauthorizedPermission.php b/src/Exceptions/UnauthorizedPermission.php index a40a94f..9caf033 100644 --- a/src/Exceptions/UnauthorizedPermission.php +++ b/src/Exceptions/UnauthorizedPermission.php @@ -8,4 +8,15 @@ */ class UnauthorizedPermission extends UnauthorizedException { + /** + * UnauthorizedPermission constructor. + * + * @param $statusCode + * @param null $message + * @param array $requiredPermissions + */ + public function __construct($statusCode, $message = null, $requiredPermissions = []) + { + parent::__construct($statusCode, $message, [], $requiredPermissions); + } } diff --git a/src/Exceptions/UnauthorizedRole.php b/src/Exceptions/UnauthorizedRole.php index 4d3aa6e..8de4fb6 100644 --- a/src/Exceptions/UnauthorizedRole.php +++ b/src/Exceptions/UnauthorizedRole.php @@ -8,4 +8,15 @@ */ class UnauthorizedRole extends UnauthorizedException { + /** + * UnauthorizedPermission constructor. + * + * @param $statusCode + * @param null $message + * @param array $requiredRoles + */ + public function __construct($statusCode, $message = null, $requiredRoles = []) + { + parent::__construct($statusCode, $message, $requiredRoles); + } } diff --git a/src/Helpers.php b/src/Helpers.php index ea51f74..ad60448 100644 --- a/src/Helpers.php +++ b/src/Helpers.php @@ -87,7 +87,12 @@ public function getRoleDoesNotExistMessage(string $name, string $guardName): str */ public function getUnauthorizedRoleMessage(string $roles): string { - return "User does not have the right roles `{$roles}`."; + $message = "User does not have the right roles `{$roles}`."; + if (! config('permission.display_permission_in_exception')) { + $message = 'User does not have the right roles.'; + } + + return $message; } /** @@ -97,7 +102,12 @@ public function getUnauthorizedRoleMessage(string $roles): string */ public function getUnauthorizedPermissionMessage(string $permissions): string { - return "User does not have the right permissions `{$permissions}`."; + $message = "User does not have the right permissions `{$permissions}`."; + if (! config('permission.display_permission_in_exception')) { + $message = 'User does not have the right permissions.'; + } + + return $message; } /** diff --git a/src/Middlewares/PermissionMiddleware.php b/src/Middlewares/PermissionMiddleware.php index 61fadb1..a0fb162 100644 --- a/src/Middlewares/PermissionMiddleware.php +++ b/src/Middlewares/PermissionMiddleware.php @@ -4,6 +4,7 @@ namespace Maklad\Permission\Middlewares; use Closure; +use Maklad\Permission\Exceptions\UnauthorizedPermission; use Maklad\Permission\Exceptions\UserNotLoggedIn; use Maklad\Permission\Helpers; @@ -33,7 +34,11 @@ public function handle($request, Closure $next, $permission) if (! app('auth')->user()->hasAnyPermission($permissions)) { $helpers = new Helpers(); - throw new UserNotLoggedIn(403, $helpers->getUnauthorizedPermissionMessage(implode(', ', $permissions))); + throw new UnauthorizedPermission( + 403, + $helpers->getUnauthorizedPermissionMessage(implode(', ', $permissions)), + $permissions + ); } return $next($request); diff --git a/src/Middlewares/RoleMiddleware.php b/src/Middlewares/RoleMiddleware.php index a4457ef..92db2d4 100644 --- a/src/Middlewares/RoleMiddleware.php +++ b/src/Middlewares/RoleMiddleware.php @@ -4,6 +4,7 @@ namespace Maklad\Permission\Middlewares; use Closure; +use Maklad\Permission\Exceptions\UnauthorizedRole; use Maklad\Permission\Exceptions\UserNotLoggedIn; use Maklad\Permission\Helpers; @@ -32,7 +33,7 @@ public function handle($request, Closure $next, $role) if (! app('auth')->user()->hasAnyRole($roles)) { $helpers = new Helpers(); - throw new UserNotLoggedIn(403, $helpers->getUnauthorizedRoleMessage(implode(', ', $roles))); + throw new UnauthorizedRole(403, $helpers->getUnauthorizedRoleMessage(implode(', ', $roles)), $roles); } return $next($request); diff --git a/tests/HasPermissionsTest.php b/tests/HasPermissionsTest.php index 613e8b7..e6461de 100644 --- a/tests/HasPermissionsTest.php +++ b/tests/HasPermissionsTest.php @@ -24,7 +24,7 @@ public function it_throws_an_exception_when_assigning_a_permission_that_does_not $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionDoesNotExist::class); @@ -43,7 +43,7 @@ public function it_throws_an_exception_when_assigning_a_permission_to_a_user_fro $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); diff --git a/tests/HasRolesTest.php b/tests/HasRolesTest.php index 63ceb90..9b6a9b0 100644 --- a/tests/HasRolesTest.php +++ b/tests/HasRolesTest.php @@ -66,7 +66,7 @@ public function it_throws_an_exception_when_assigning_a_role_that_does_not_exist $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(RoleDoesNotExist::class); @@ -85,7 +85,7 @@ public function it_can_only_assign_roles_from_the_correct_guard() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(RoleDoesNotExist::class); @@ -104,7 +104,7 @@ public function it_throws_an_exception_when_assigning_a_role_from_a_different_gu $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); @@ -169,7 +169,7 @@ public function it_throws_an_exception_when_syncing_a_role_from_another_guard() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); @@ -323,7 +323,7 @@ public function it_throws_an_exception_when_the_permission_does_not_exist() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionDoesNotExist::class); @@ -342,7 +342,7 @@ public function it_throws_an_exception_when_the_permission_does_not_exist_for_th $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionDoesNotExist::class); diff --git a/tests/MiddlewareTest.php b/tests/MiddlewareTest.php index 1aa9627..ceaa1fe 100644 --- a/tests/MiddlewareTest.php +++ b/tests/MiddlewareTest.php @@ -30,7 +30,7 @@ public function a_guest_cannot_access_a_route_protected_by_the_role_middleware() $can_logs = [false, true]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -93,10 +93,10 @@ public function a_user_cannot_access_a_route_protected_by_the_role_middleware_if $this->testUser->assignRole(['testRole']); $can_logs = [false, true]; + $show_permissions = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); - + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( $this->roleMiddleware, @@ -105,8 +105,12 @@ public function a_user_cannot_access_a_route_protected_by_the_role_middleware_if 403 ); - $message = $this->helpers->getUnauthorizedRoleMessage('testRole2'); - $this->assertLogMessage($message, Logger::ALERT); + foreach ($show_permissions as $show_permission) { + config('permission.display_permission_in_exception', $show_permission); + $message = $this->helpers->getUnauthorizedRoleMessage('testRole2'); + $this->assertShowPermission($message, 'testRole2'); + $this->assertLogMessage($message, Logger::ALERT); + } } } @@ -116,9 +120,10 @@ public function a_user_cannot_access_a_route_protected_by_role_middleware_if_hav Auth::login($this->testUser); $can_logs = [false, true]; + $show_permissions = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -128,8 +133,13 @@ public function a_user_cannot_access_a_route_protected_by_role_middleware_if_hav 403 ); - $message = $this->helpers->getUnauthorizedRoleMessage('testRole, testRole2'); - $this->assertLogMessage($message, Logger::ALERT); + foreach ($show_permissions as $show_permission) { + config('permission.display_permission_in_exception', $show_permission); + $message = $this->helpers->getUnauthorizedRoleMessage('testRole, testRole2'); + $this->assertShowPermission($message, 'testRole'); + $this->assertShowPermission($message, 'testRole2'); + $this->assertLogMessage($message, Logger::ALERT); + } } } @@ -139,9 +149,10 @@ public function a_user_cannot_access_a_route_protected_by_role_middleware_if_rol Auth::login($this->testUser); $can_logs = [false, true]; + $show_permissions = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -151,8 +162,12 @@ public function a_user_cannot_access_a_route_protected_by_role_middleware_if_rol 403 ); - $message = $this->helpers->getUnauthorizedRoleMessage(''); - $this->assertLogMessage($message, Logger::ALERT); + foreach ($show_permissions as $show_permission) { + config('permission.display_permission_in_exception', $show_permission); + $message = $this->helpers->getUnauthorizedRoleMessage('test'); + $this->assertShowPermission($message, 'test'); + $this->assertLogMessage($message, Logger::ALERT); + } } } @@ -162,7 +177,7 @@ public function a_guest_cannot_access_a_route_protected_by_the_permission_middle $can_logs = [false, true]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -225,9 +240,10 @@ public function a_user_cannot_access_a_route_protected_by_the_permission_middlew $this->testUser->givePermissionTo('edit-articles'); $can_logs = [false, true]; + $show_permissions = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -237,8 +253,12 @@ public function a_user_cannot_access_a_route_protected_by_the_permission_middlew 403 ); - $message = $this->helpers->getUnauthorizedPermissionMessage('edit-news'); - $this->assertLogMessage($message, Logger::ALERT); + foreach ($show_permissions as $show_permission) { + config('permission.display_permission_in_exception', $show_permission); + $message = $this->helpers->getUnauthorizedPermissionMessage('edit-news'); + $this->assertShowPermission($message, 'edit-news'); + $this->assertLogMessage($message, Logger::ALERT); + } } } @@ -248,9 +268,10 @@ public function a_user_cannot_access_a_route_protected_by_permission_middleware_ Auth::login($this->testUser); $can_logs = [false, true]; + $show_permissions = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); $this->assertEquals( $this->runMiddleware( @@ -260,9 +281,43 @@ public function a_user_cannot_access_a_route_protected_by_permission_middleware_ 403 ); - $message = $this->helpers->getUnauthorizedPermissionMessage('edit-articles, edit-news'); - $this->assertLogMessage($message, Logger::ALERT); + foreach ($show_permissions as $show_permission) { + config('permission.display_permission_in_exception', $show_permission); + $message = $this->helpers->getUnauthorizedPermissionMessage('edit-articles, edit-news'); + $this->assertShowPermission($message, 'edit-articles'); + $this->assertShowPermission($message, 'edit-news'); + $this->assertLogMessage($message, Logger::ALERT); + } + } + } + + /** @test */ + public function the_required_roles_can_be_fetched_from_the_exception() + { + Auth::login($this->testUser); + $requiredRoles = []; + try { + $this->roleMiddleware->handle(new Request(), function () { + return (new Response())->setContent(''); + }, 'testRole'); + } catch (UnauthorizedException $e) { + $requiredRoles = $e->getRequiredRoles(); + } + $this->assertEquals(['testRole'], $requiredRoles); + } + /** @test */ + public function the_required_permissions_can_be_fetched_from_the_exception() + { + Auth::login($this->testUser); + $requiredPermissions = []; + try { + $this->permissionMiddleware->handle(new Request(), function () { + return (new Response())->setContent(''); + }, 'edit-articles'); + } catch (UnauthorizedException $e) { + $requiredPermissions = $e->getRequiredPermissions(); } + $this->assertEquals(['edit-articles'], $requiredPermissions); } protected function runMiddleware($middleware, $parameter) diff --git a/tests/PermissionTest.php b/tests/PermissionTest.php index ef9957b..024cb1e 100644 --- a/tests/PermissionTest.php +++ b/tests/PermissionTest.php @@ -14,7 +14,7 @@ public function it_throws_an_exception_when_the_permission_already_exists() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionAlreadyExists::class); diff --git a/tests/RoleTest.php b/tests/RoleTest.php index b66c533..49f05ba 100644 --- a/tests/RoleTest.php +++ b/tests/RoleTest.php @@ -41,7 +41,7 @@ public function it_throws_an_exception_when_the_role_already_exists() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(RoleAlreadyExists::class); @@ -69,7 +69,7 @@ public function it_throws_an_exception_when_given_a_permission_that_does_not_exi $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionDoesNotExist::class); @@ -88,7 +88,7 @@ public function it_throws_an_exception_when_given_a_permission_that_belongs_to_a $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); @@ -137,7 +137,7 @@ public function it_throws_an_exception_when_syncing_permissions_that_do_not_exis $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->testUserRole->givePermissionTo('edit-articles'); @@ -158,7 +158,7 @@ public function it_throws_an_exception_when_syncing_permissions_that_belong_to_a $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); @@ -219,7 +219,7 @@ public function it_throws_an_exception_if_the_permission_does_not_exist() $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(PermissionDoesNotExist::class); @@ -246,7 +246,7 @@ public function it_throws_an_exception_when_a_permission_of_the_wrong_guard_is_p $can_logs = [true, false]; foreach ($can_logs as $can_log) { - $this->app['config']->set('permission.log_registration_exception', $can_log); + config('permission.log_registration_exception', $can_log); try { $this->expectException(GuardDoesNotMatch::class); diff --git a/tests/TestCase.php b/tests/TestCase.php index 1254821..52535a2 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -74,7 +74,7 @@ protected function getPackageProviders($app) { return [ PermissionServiceProvider::class, - MongodbServiceProvider::class + MongodbServiceProvider::class, ]; } @@ -91,7 +91,7 @@ protected function getEnvironmentSetUp($app) 'port' => '27017', 'driver' => 'mongodb', 'database' => 'laravel_permission_mongodb_test', - 'prefix' => '' + 'prefix' => '', ]); $app['config']->set('view.paths', [__DIR__ . '/resources/views']); @@ -178,9 +178,9 @@ protected function assertLogged($message, $level) protected function hasLog($message, $level) { return \collect($this->app['log']->getMonolog()->getHandlers())->filter(function ($handler) use ( - $message, - $level - ) { + $message, + $level + ) { return $handler instanceof TestHandler && $handler->hasRecordThatContains($message, $level); })->count() > 0; } @@ -196,4 +196,16 @@ protected function assertLogMessage($message, $level) $this->assertNotLogged($message, $level); } } + + /** + * @param $message + */ + protected function assertShowPermission($message, $role_permission) + { + if (\config('permission.display_permission_in_exception')) { + $this->assertContains($role_permission, $message); + } else { + $this->assertNotContains($role_permission, $message); + } + } }