Merge pull request #2622 from RaiderIO/development

Release v11.7.1 - Rate limiting updates, more bugfixes
RaiderIO · Nov 19, 2024 · 392da14 · 392da14
2 parents 788eb5a + c2a192c
commit 392da14
Show file tree

Hide file tree

Showing 23 changed files with 196 additions and 47 deletions.
diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
@@ -2,6 +2,8 @@
 
 namespace App\Exceptions;
 
+use App\Exceptions\Logging\HandlerLoggingInterface;
+use Auth;
 use Illuminate\Auth\Access\AuthorizationException;
 use Illuminate\Auth\AuthenticationException;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
@@ -12,6 +14,7 @@
 use Symfony\Component\HttpFoundation\Exception\BadRequestException;
 use Symfony\Component\HttpKernel\Exception\HttpException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\HttpKernel\Exception\TooManyRequestsHttpException;
 use Teapot\StatusCode;
 use Throwable;
 
@@ -29,7 +32,7 @@ class Handler extends ExceptionHandler
         ModelNotFoundException::class,
         TokenMismatchException::class,
         ValidationException::class,
-        BadRequestException::class
+        BadRequestException::class,
     ];
 
     /**
@@ -43,6 +46,13 @@ class Handler extends ExceptionHandler
      */
     public function report(Throwable $e): void
     {
+        $handlerLogging = app()->make(HandlerLoggingInterface::class);
+
+        if ($e instanceof TooManyRequestsHttpException) {
+            $user = Auth::user();
+            $handlerLogging->tooManyRequests(request()?->ip() ?? 'unknown IP', $user?->id, $user?->name, $e);
+        }
+
         parent::report($e);
     }
 

diff --git a/app/Exceptions/Logging/HandlerLogging.php b/app/Exceptions/Logging/HandlerLogging.php
@@ -0,0 +1,14 @@
+<?php
+
+namespace App\Exceptions\Logging;
+
+use App\Logging\RollbarStructuredLogging;
+use Throwable;
+
+class HandlerLogging extends RollbarStructuredLogging implements HandlerLoggingInterface
+{
+    public function tooManyRequests(string $ip, ?int $userId, ?string $username, Throwable $throwable): void
+    {
+        $this->error(__METHOD__, get_defined_vars());
+    }
+}
diff --git a/app/Exceptions/Logging/HandlerLoggingInterface.php b/app/Exceptions/Logging/HandlerLoggingInterface.php
@@ -0,0 +1,8 @@
+<?php
+
+namespace App\Exceptions\Logging;
+
+interface HandlerLoggingInterface
+{
+    public function tooManyRequests(string $ip, ?int $userId, ?string $username, \Throwable $throwable): void;
+}
diff --git a/app/Http/Controllers/Ajax/AjaxUserController.php b/app/Http/Controllers/Ajax/AjaxUserController.php
@@ -4,6 +4,9 @@
 
 use App\Http\Controllers\Controller;
 use App\Http\Requests\User\UserFormRequest;
+use App\Logic\Datatables\ColumnHandler\Npc\NameColumnHandler;
+use App\Logic\Datatables\ColumnHandler\Users\EmailColumnHandler;
+use App\Logic\Datatables\ColumnHandler\Users\IdColumnHandler;
 use App\Logic\Datatables\UsersDatatablesHandler;
 use App\Models\User;
 use Auth;
@@ -22,7 +25,14 @@ public function get(Request $request)
     {
         $users = User::with(['patreonUserLink', 'roles', 'dungeonroutes'])->selectRaw('users.*');
 
-        $datatablesResult = (new UsersDatatablesHandler($request))
+        $datatablesHandler = (new UsersDatatablesHandler($request));
+
+        $datatablesResult = $datatablesHandler
+            ->addColumnHandler([
+                new IdColumnHandler($datatablesHandler),
+                new EmailColumnHandler($datatablesHandler),
+                new NameColumnHandler($datatablesHandler),
+            ])
             ->setBuilder($users)
             ->applyRequestToBuilder()
             ->getResult();

diff --git a/app/Http/Controllers/MDTImportController.php b/app/Http/Controllers/MDTImportController.php
@@ -76,7 +76,9 @@ public function import(ImportStringFormRequest $request, MDTImportStringServiceI
 
         $validated = $request->validated();
 
-        $sandbox = $validated['mdt_import_sandbox'] ?? false;
+        // If you're logged in, we will use the sandbox setting. Otherwise, we will ignore it.
+        $sandbox = Auth::check() ? ($validated['mdt_import_sandbox'] ?? false) : false;
+
         // @TODO This should be handled differently imho
         if ($sandbox || ($user !== null && $user->canCreateDungeonRoute())) {
             $string = $validated['import_string'];

diff --git a/app/Logic/Datatables/ColumnHandler/DatatablesColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DatatablesColumnHandler.php
@@ -26,9 +26,13 @@ public function __construct(
     }
 
     /**
-     * @return mixed
+     * @param Builder $subBuilder
+     * @param         $columnData
+     * @param         $order
+     * @param         $generalSearch
+     * @return void
      */
-    abstract protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch);
+    abstract protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void;
 
     public function getDtHandler(): DatatablesHandler
     {

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/AuthorNameColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/AuthorNameColumnHandler.php
@@ -19,7 +19,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'author.name');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
         // Only order
         if ($order !== null) {

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonColumnHandler.php
@@ -21,7 +21,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'dungeon_id');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
         // If we should search for this value
         if ($columnData['searchable'] === 'true') {

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonRouteAffixesColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonRouteAffixesColumnHandler.php
@@ -20,7 +20,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'affixes.id');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
 
         $affixes = $columnData['search']['value'];

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonRouteAttributesColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/DungeonRouteAttributesColumnHandler.php
@@ -20,7 +20,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'routeattributes.name');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
         $routeattributes = $columnData['search']['value'];
         // If filtering or ordering

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/EnemyForcesColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/EnemyForcesColumnHandler.php
@@ -19,7 +19,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'enemy_forces');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
         $views = $columnData['search']['value'];
         //        if (!empty($views)) {

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/RatingColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/RatingColumnHandler.php
@@ -20,7 +20,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'rating');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
 
         //        $rating = $columnData['search']['value'];

diff --git a/app/Logic/Datatables/ColumnHandler/DungeonRoutes/ViewsColumnHandler.php b/app/Logic/Datatables/ColumnHandler/DungeonRoutes/ViewsColumnHandler.php
@@ -19,7 +19,7 @@ public function __construct(DatatablesHandler $dtHandler)
         parent::__construct($dtHandler, 'views');
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
         $views = $columnData['search']['value'];
         //        if (!empty($views)) {

diff --git a/app/Logic/Datatables/ColumnHandler/SimpleColumnHandler.php b/app/Logic/Datatables/ColumnHandler/SimpleColumnHandler.php
@@ -13,13 +13,30 @@
 
 class SimpleColumnHandler extends DatatablesColumnHandler
 {
+    /**
+     * @var string[] The simple column handler may use these column names - the columns that are requested are coming
+     * from the front end and cannot be trusted - so add a filter here.
+     */
+    const VALID_COLUMN_NAMES = [
+        'id',
+        'title',
+        'public_key',
+        'name',
+        'email'
+    ];
+
     public function __construct(DatatablesHandler $dtHandler, $columnName, $columnData = null)
     {
         parent::__construct($dtHandler, $columnName, $columnData);
     }
 
-    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch)
+    protected function applyFilter(Builder $subBuilder, $columnData, $order, $generalSearch): void
     {
+        // If the column name is not valid, ignore it entirely
+        if( !in_array($this->getColumnName(), self::VALID_COLUMN_NAMES) ) {
+            return;
+        }
+
         // If we should search for this value
         if ($columnData['searchable'] === 'true') {
             $searchValue = $columnData['search']['value'] ?? $generalSearch;

diff --git a/app/Logic/Datatables/ColumnHandler/Users/IdColumnHandler.php b/app/Logic/Datatables/ColumnHandler/Users/IdColumnHandler.php
@@ -0,0 +1,14 @@
+<?php
+
+namespace App\Logic\Datatables\ColumnHandler\Users;
+
+use App\Logic\Datatables\ColumnHandler\SimpleColumnHandler;
+use App\Logic\Datatables\DatatablesHandler;
+
+class IdColumnHandler extends SimpleColumnHandler
+{
+    public function __construct(DatatablesHandler $dtHandler)
+    {
+        parent::__construct($dtHandler, 'id', 'users.id');
+    }
+}
diff --git a/app/Logic/Datatables/DatatablesHandler.php b/app/Logic/Datatables/DatatablesHandler.php
@@ -17,7 +17,6 @@
 
 abstract class DatatablesHandler
 {
-    const VALID_COLUMN_NAMES = ['title', 'public_key', 'name', 'email'];
 
     protected Builder $builder;
 
@@ -89,12 +88,10 @@ public function applyRequestToBuilder(): DatatablesHandler
             foreach ($columns as $column) {
                 $columnName = $column['name'];
                 // Only if the column name was set - column name comes from the client, do not trust it!!
-                if (!empty($columnName) && in_array($columnName, self::VALID_COLUMN_NAMES)) {
-                    // Only if not handled by a custom column handler
-                    if (!isset($this->columnHandlers[$columnName])) {
-                        // Handle filtering/sorting by this column
-                        (new SimpleColumnHandler($this, $columnName))->applyToBuilder($subBuilder);
-                    }
+                // Only if not handled by a custom column handler already
+                if (!empty($columnName) && !isset($this->columnHandlers[$columnName])) {
+                    // Handle filtering/sorting by this column
+                    (new SimpleColumnHandler($this, $columnName))->applyToBuilder($subBuilder);
                 }
             }
         });

diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php
@@ -6,6 +6,7 @@
 use App\Models\User;
 use Auth;
 use Illuminate\Database\Eloquent\Model;
+use Illuminate\Http\Request;
 use Illuminate\Support\ServiceProvider;
 use Rollbar\Payload\Level;
 use Rollbar\Rollbar;
@@ -40,6 +41,13 @@ public function boot(): void
                 'correlationId' => correlationId(),
             ],
         ]);
+
+        // Ensure that we know the original IP address that made the request
+        // https://khalilst.medium.com/get-real-client-ip-behind-cloudflare-in-laravel-189cb89059ff
+        Request::setTrustedProxies(
+            ['REMOTE_ADDR'],
+            Request::HEADER_X_FORWARDED_FOR
+        );
     }
 
     /**

diff --git a/app/Providers/LoggingServiceProvider.php b/app/Providers/LoggingServiceProvider.php
@@ -2,6 +2,8 @@
 
 namespace App\Providers;
 
+use App\Exceptions\Logging\HandlerLogging;
+use App\Exceptions\Logging\HandlerLoggingInterface;
 use App\Http\Middleware\Logging\DebugInfoContextLoggerLogging;
 use App\Http\Middleware\Logging\DebugInfoContextLoggerLoggingInterface;
 use App\Service\AffixGroup\Logging\AffixGroupEaseTierServiceLogging;
@@ -82,6 +84,9 @@ public function register(): void
     {
         parent::register();
 
+        // Exceptions
+        $this->app->bind(HandlerLoggingInterface::class, HandlerLogging::class);
+
         // Middleware
         $this->app->bind(DebugInfoContextLoggerLoggingInterface::class, DebugInfoContextLoggerLogging::class);
 

diff --git a/app/Providers/RouteServiceProvider.php b/app/Providers/RouteServiceProvider.php
@@ -2,6 +2,8 @@
 
 namespace App\Providers;
 
+use App\Models\Laratrust\Role;
+use App\Models\User;
 use Illuminate\Cache\RateLimiting\Limit;
 use Illuminate\Foundation\Support\Providers\RouteServiceProvider as ServiceProvider;
 use Illuminate\Http\Request;
@@ -10,6 +12,8 @@
 
 class RouteServiceProvider extends ServiceProvider
 {
+    private const RATE_LIMIT_OVERRIDE = 999999;
+
     /**
      * Define your route model bindings, pattern filters, etc.
      */
@@ -63,35 +67,54 @@ protected function mapApiRoutes(): void
     protected function configureRateLimiting(): void
     {
         RateLimiter::for('create-dungeonroute', function (Request $request) {
-            return Limit::perHour(20)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 20)->by($this->userKey($request));
         });
         RateLimiter::for('create-tag', function (Request $request) {
-            return Limit::perHour(60)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 60)->by($this->userKey($request));
         });
         RateLimiter::for('create-team', function (Request $request) {
-            return Limit::perHour(5)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 5)->by($this->userKey($request));
         });
         RateLimiter::for('create-reports', function (Request $request) {
-            return Limit::perHour(60)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 60)->by($this->userKey($request));
         });
         RateLimiter::for('create-user', function (Request $request) {
-            return Limit::perHour(5)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 5)->by($this->userKey($request));
         });
 
         // Heavy GET requests
         RateLimiter::for('search-dungeonroute', function (Request $request) {
-            return Limit::perHour(300)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 300)->by($this->userKey($request));
         });
 
         // This consumes the same resources as creating a route - so we limit it
         RateLimiter::for('mdt-details', function (Request $request) {
-            return Limit::perHour(60)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 60)->by($this->userKey($request));
         });
         RateLimiter::for('mdt-export', function (Request $request) {
-            return Limit::perHour(60)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 60)->by($this->userKey($request));
         });
         RateLimiter::for('simulate', function (Request $request) {
-            return Limit::perHour(120)->by($request->user()?->id ?: $request->ip());
+            return $this->noLimitForExemptions($request) ?? Limit::perHour(self::RATE_LIMIT_OVERRIDE ?? 120)->by($this->userKey($request));
         });
     }
+
+    private function noLimitForExemptions(Request $request): ?Limit
+    {
+        /** @var User|null $user */
+        $user = $request->user();
+
+        if ($user?->hasRole(Role::ROLE_ADMIN) || $user?->hasRole(Role::ROLE_INTERNAL_TEAM)) {
+            return Limit::none();
+        }
+
+        return null;
+    }
+
+    private function userKey(Request $request): string
+    {
+        /** @var User|null $user */
+        $user = $request->user();
+        return $user?->id ?: $request->ip();
+    }
 }