From 6d6670e24676d82cdd1780a2ea359afa526ee172 Mon Sep 17 00:00:00 2001 From: "waffle.lord" Date: Fri, 20 Sep 2024 10:37:16 -0400 Subject: [PATCH 1/4] setup download counting --- app/Http/Controllers/ModVersionController.php | 23 +++++++++++++++++++ app/Models/Mod.php | 5 ++++ app/Models/ModVersion.php | 5 ++++ app/Providers/AppServiceProvider.php | 7 ++++++ resources/views/mod/show.blade.php | 6 ++--- routes/web.php | 5 ++++ 6 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 app/Http/Controllers/ModVersionController.php diff --git a/app/Http/Controllers/ModVersionController.php b/app/Http/Controllers/ModVersionController.php new file mode 100644 index 0000000..95f471c --- /dev/null +++ b/app/Http/Controllers/ModVersionController.php @@ -0,0 +1,23 @@ +where("version", $version)->first(); + + if ($modVersion == null) { + abort(404); + } + + $modVersion->downloads++; + $modVersion->save(); + + return redirect($modVersion->link); + } +} diff --git a/app/Models/Mod.php b/app/Models/Mod.php index 292a49c..f173c33 100644 --- a/app/Models/Mod.php +++ b/app/Models/Mod.php @@ -46,6 +46,11 @@ class Mod extends Model $this->saveQuietly(); } + public function downloadUrl(): string + { + return "/mod/download/$this->id/{$this->latestVersion->version}"; + } + /** * The relationship between a mod and its users. * diff --git a/app/Models/ModVersion.php b/app/Models/ModVersion.php index 15d867b..43938ff 100644 --- a/app/Models/ModVersion.php +++ b/app/Models/ModVersion.php @@ -130,4 +130,9 @@ class ModVersion extends Model ->orderByDesc('version_patch') ->orderByDesc('version_pre_release'); } + + public function downloadUrl(): string + { + return "/mod/download/$this->mod_id/$this->version"; + } } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 0b307fd..87dcd48 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -12,8 +12,11 @@ use App\Observers\ModObserver; use App\Observers\ModVersionObserver; use App\Observers\SptVersionObserver; use Carbon\Carbon; +use Illuminate\Cache\RateLimiting\Limit; use Illuminate\Database\Eloquent\Model; +use Illuminate\Http\Request; use Illuminate\Support\Facades\Gate; +use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Number; use Illuminate\Support\ServiceProvider; @@ -44,6 +47,10 @@ class AppServiceProvider extends ServiceProvider Gate::define('viewPulse', function (User $user) { return $user->isAdmin(); }); + + RateLimiter::for('modDownloads', function (Request $request) { + return Limit::perMinute(3)->by($request->user()?->id ?: $request->ip()); + }); } /** diff --git a/resources/views/mod/show.blade.php b/resources/views/mod/show.blade.php index e6350d5..935d1a0 100644 --- a/resources/views/mod/show.blade.php +++ b/resources/views/mod/show.blade.php @@ -54,7 +54,7 @@ {{-- Mobile Download Button --}} - + @@ -102,7 +102,7 @@
- + {{ __('Version') }} {{ $version->version }}

{{ Number::downloads($version->downloads) }} {{ __('Downloads') }}

@@ -147,7 +147,7 @@
{{-- Desktop Download Button --}} - diff --git a/routes/web.php b/routes/web.php index 4641d92..3c355df 100644 --- a/routes/web.php +++ b/routes/web.php @@ -1,6 +1,7 @@ group(function () { Route::get('/mod/{mod}/{slug}', 'show')->where(['mod' => '[0-9]+'])->name('mod.show'); }); + Route::middleware(['throttle:modDownloads'])->controller(ModVersionController::class)->group(function () { + Route::get('/mod/download/{mod}/{version}', 'show')->where(['mod' => '[0-9]+']); + }); + Route::controller(UserController::class)->group(function () { Route::get('/user/{user}/{username}', 'show')->where(['user' => '[0-9]+'])->name('user.show'); }); From 8a156d8a43f8ceb044ad387c6db602e01d190cd0 Mon Sep 17 00:00:00 2001 From: "waffle.lord" Date: Fri, 20 Sep 2024 10:46:26 -0400 Subject: [PATCH 2/4] update controller to recalculate downloads and return 307 specifically --- app/Http/Controllers/ModVersionController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/ModVersionController.php b/app/Http/Controllers/ModVersionController.php index 95f471c..abee2c7 100644 --- a/app/Http/Controllers/ModVersionController.php +++ b/app/Http/Controllers/ModVersionController.php @@ -17,7 +17,8 @@ class ModVersionController extends Controller $modVersion->downloads++; $modVersion->save(); + $modVersion->mod->calculateDownloads(); - return redirect($modVersion->link); + return redirect($modVersion->link, 307); } } From deca405976306e62d3be575363ba3f4bd03a71b6 Mon Sep 17 00:00:00 2001 From: Refringe Date: Wed, 25 Sep 2024 19:15:41 +0000 Subject: [PATCH 3/4] Pint PHP Style Fixes [no ci] --- app/Http/Controllers/ModVersionController.php | 2 +- app/Providers/AppServiceProvider.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/ModVersionController.php b/app/Http/Controllers/ModVersionController.php index abee2c7..714aa5c 100644 --- a/app/Http/Controllers/ModVersionController.php +++ b/app/Http/Controllers/ModVersionController.php @@ -9,7 +9,7 @@ class ModVersionController extends Controller { public function show(int $modId, string $version): RedirectResponse { - $modVersion = ModVersion::where("mod_id", $modId)->where("version", $version)->first(); + $modVersion = ModVersion::where('mod_id', $modId)->where('version', $version)->first(); if ($modVersion == null) { abort(404); diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 87dcd48..95df1c8 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -49,7 +49,7 @@ class AppServiceProvider extends ServiceProvider }); RateLimiter::for('modDownloads', function (Request $request) { - return Limit::perMinute(3)->by($request->user()?->id ?: $request->ip()); + return Limit::perMinute(3)->by($request->user()?->id ?: $request->ip()); }); } From 7e1c66f2501182a7f6cad02bd0af7a693ddef817 Mon Sep 17 00:00:00 2001 From: Refringe Date: Wed, 25 Sep 2024 17:02:03 -0400 Subject: [PATCH 4/4] Download Count Review Reviewed the download count PR work and made some changes: - Updated the download link route to include the mod's slug for easier identification. - Moved rate limiter from the route middleware (the entire controller) to just the show method in the controller. - Created a ModVersionPolicy that the controller can check against. - Moves download increment logic into the model. - Defers the call to the download increment logic (now run in the background) - Updated the route to have a name, and the downloadUrl methods to build the URL dynamically using the route name. - Wrote some tests to check URL building, download counting, and rate limiting. # Conflicts: # app/Http/Controllers/ModVersionController.php # app/Providers/AppServiceProvider.php --- app/Http/Controllers/ModVersionController.php | 32 +++++++-- app/Livewire/User/FollowCard.php | 2 + app/Models/Mod.php | 14 ++-- app/Models/ModVersion.php | 21 +++++- app/Policies/ModVersionPolicy.php | 68 +++++++++++++++++++ app/Providers/AppServiceProvider.php | 7 -- routes/web.php | 9 ++- tests/Feature/Mod/ModTest.php | 9 +++ tests/Feature/Mod/ModVersionTest.php | 46 +++++++++++++ 9 files changed, 185 insertions(+), 23 deletions(-) create mode 100644 app/Policies/ModVersionPolicy.php diff --git a/app/Http/Controllers/ModVersionController.php b/app/Http/Controllers/ModVersionController.php index 714aa5c..090c0d7 100644 --- a/app/Http/Controllers/ModVersionController.php +++ b/app/Http/Controllers/ModVersionController.php @@ -3,22 +3,40 @@ namespace App\Http\Controllers; use App\Models\ModVersion; +use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Http\RedirectResponse; +use Illuminate\Http\Request; +use Illuminate\Support\Facades\RateLimiter; class ModVersionController extends Controller { - public function show(int $modId, string $version): RedirectResponse - { - $modVersion = ModVersion::where('mod_id', $modId)->where('version', $version)->first(); + use AuthorizesRequests; - if ($modVersion == null) { + public function show(Request $request, int $modId, string $slug, string $version): RedirectResponse + { + $modVersion = ModVersion::whereModId($modId) + ->whereVersion($version) + ->firstOrFail(); + + if ($modVersion->mod->slug !== $slug) { abort(404); } - $modVersion->downloads++; - $modVersion->save(); - $modVersion->mod->calculateDownloads(); + $this->authorize('view', $modVersion); + // Rate limit the downloads. + $rateKey = 'mod-download:'.($request->user()?->id ?: $request->ip()); + if (RateLimiter::tooManyAttempts($rateKey, maxAttempts: 5)) { // Max attempts is per minute. + abort(429); + } + + // Increment downloads counts in the background. + defer(fn () => $modVersion->incrementDownloads()); + + // Increment the rate limiter. + RateLimiter::increment($rateKey); + + // Redirect to the download link, using a 307 status code to prevent browsers from caching. return redirect($modVersion->link, 307); } } diff --git a/app/Livewire/User/FollowCard.php b/app/Livewire/User/FollowCard.php index c62b53f..325b585 100644 --- a/app/Livewire/User/FollowCard.php +++ b/app/Livewire/User/FollowCard.php @@ -138,6 +138,8 @@ class FollowCard extends Component // Add the follow status based on the preloaded IDs. $user->follows = $followingIds->contains($user->id); + // TODO: The above follows property doesn't exist on the User model. What was I smoking? + return $user; }); } diff --git a/app/Models/Mod.php b/app/Models/Mod.php index f173c33..83fb891 100644 --- a/app/Models/Mod.php +++ b/app/Models/Mod.php @@ -5,7 +5,6 @@ namespace App\Models; use App\Http\Filters\V1\QueryFilter; use App\Models\Scopes\DisabledScope; use App\Models\Scopes\PublishedScope; -use Database\Factories\ModFactory; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; @@ -22,9 +21,7 @@ use Laravel\Scout\Searchable; class Mod extends Model { - /** @use HasFactory */ use HasFactory; - use Searchable; use SoftDeletes; @@ -46,9 +43,16 @@ class Mod extends Model $this->saveQuietly(); } - public function downloadUrl(): string + /** + * Build the URL to download the latest version of this mod. + */ + public function downloadUrl(bool $absolute = false): string { - return "/mod/download/$this->id/{$this->latestVersion->version}"; + return route('mod.version.download', [ + $this->id, + $this->slug, + $this->latestVersion->version, + ], absolute: $absolute); } /** diff --git a/app/Models/ModVersion.php b/app/Models/ModVersion.php index 43938ff..406cfaa 100644 --- a/app/Models/ModVersion.php +++ b/app/Models/ModVersion.php @@ -131,8 +131,25 @@ class ModVersion extends Model ->orderByDesc('version_pre_release'); } - public function downloadUrl(): string + /** + * Build the download URL for this mod version. + */ + public function downloadUrl(bool $absolute = false): string { - return "/mod/download/$this->mod_id/$this->version"; + return route('mod.version.download', [$this->mod->id, $this->mod->slug, $this->version], absolute: $absolute); + } + + /** + * Increment the download count for this mod version. + */ + public function incrementDownloads(): int + { + $this->downloads++; + $this->save(); + + // Recalculate the total download count for this mod. + $this->mod->calculateDownloads(); + + return $this->downloads; } } diff --git a/app/Policies/ModVersionPolicy.php b/app/Policies/ModVersionPolicy.php new file mode 100644 index 0000000..2b54cb7 --- /dev/null +++ b/app/Policies/ModVersionPolicy.php @@ -0,0 +1,68 @@ +isAdmin(); }); - - RateLimiter::for('modDownloads', function (Request $request) { - return Limit::perMinute(3)->by($request->user()?->id ?: $request->ip()); - }); } /** diff --git a/routes/web.php b/routes/web.php index 3c355df..d718b8b 100644 --- a/routes/web.php +++ b/routes/web.php @@ -16,8 +16,13 @@ Route::middleware(['auth.banned'])->group(function () { Route::get('/mod/{mod}/{slug}', 'show')->where(['mod' => '[0-9]+'])->name('mod.show'); }); - Route::middleware(['throttle:modDownloads'])->controller(ModVersionController::class)->group(function () { - Route::get('/mod/download/{mod}/{version}', 'show')->where(['mod' => '[0-9]+']); + Route::controller(ModVersionController::class)->group(function () { + Route::get('/mod/download/{mod}/{slug}/{version}', 'show') + ->where([ + 'mod' => '[0-9]+', + 'slug' => '[a-z0-9-]+', + ]) + ->name('mod.version.download'); }); Route::controller(UserController::class)->group(function () { diff --git a/tests/Feature/Mod/ModTest.php b/tests/Feature/Mod/ModTest.php index f853a64..1514cf8 100644 --- a/tests/Feature/Mod/ModTest.php +++ b/tests/Feature/Mod/ModTest.php @@ -50,3 +50,12 @@ it('displays the latest version on the mod detail page', function () { // Assert the latest version is in the latest download button $response->assertSeeText(__('Download Latest Version')." ($latestVersion)"); }); + +it('builds download links using the latest mod version', function () { + $mod = Mod::factory()->create(['id' => 1, 'slug' => 'test-mod']); + ModVersion::factory()->recycle($mod)->create(['version' => '1.2.3']); + ModVersion::factory()->recycle($mod)->create(['version' => '1.3.0']); + $modVersion = ModVersion::factory()->recycle($mod)->create(['version' => '1.3.4']); + + expect($mod->downloadUrl())->toEqual("/mod/download/$mod->id/$mod->slug/$modVersion->version"); +}); diff --git a/tests/Feature/Mod/ModVersionTest.php b/tests/Feature/Mod/ModVersionTest.php index 22c6be6..ebbdd58 100644 --- a/tests/Feature/Mod/ModVersionTest.php +++ b/tests/Feature/Mod/ModVersionTest.php @@ -1,5 +1,6 @@ mod->updated_at)->not->toEqual($originalDate) ->and($version->mod->updated_at->format('Y-m-d'))->toEqual(now()->format('Y-m-d')); }); + +it('builds download links using the specified version', function () { + $mod = Mod::factory()->create(['id' => 1, 'slug' => 'test-mod']); + $modVersion1 = ModVersion::factory()->recycle($mod)->create(['version' => '1.2.3']); + $modVersion2 = ModVersion::factory()->recycle($mod)->create(['version' => '1.3.0']); + $modVersion3 = ModVersion::factory()->recycle($mod)->create(['version' => '1.3.4']); + + expect($modVersion1->downloadUrl())->toEqual("/mod/download/$mod->id/$mod->slug/$modVersion1->version") + ->and($modVersion2->downloadUrl())->toEqual("/mod/download/$mod->id/$mod->slug/$modVersion2->version") + ->and($modVersion3->downloadUrl())->toEqual("/mod/download/$mod->id/$mod->slug/$modVersion3->version"); +}); + +it('increments download counts when downloaded', function () { + $mod = Mod::factory()->create(['downloads' => 0]); + $modVersion = ModVersion::factory()->recycle($mod)->create(['downloads' => 0]); + + $request = $this->get($modVersion->downloadUrl()); + $request->assertStatus(307); + + $modVersion->refresh(); + + expect($modVersion->downloads)->toEqual(1) + ->and($modVersion->mod->downloads)->toEqual(1); +}); + +it('rate limits download links from being hit', function () { + $mod = Mod::factory()->create(['downloads' => 0]); + $modVersion = ModVersion::factory()->recycle($mod)->create(['downloads' => 0]); + + // The first 5 requests should be fine. + for ($i = 0; $i < 5; $i++) { + $request = $this->get($modVersion->downloadUrl()); + $request->assertStatus(307); + } + + // The 6th request should be rate limited. + $request = $this->get($modVersion->downloadUrl()); + $request->assertStatus(429); + + $modVersion->refresh(); + + // The download count should still be 5. + expect($modVersion->downloads)->toEqual(5) + ->and($modVersion->mod->downloads)->toEqual(5); +});