Expand usage of LimitWikiAccess middleware

app/Http/Controllers/WikiController.php

@@ -200,22 +200,8 @@ public function create(Request $request): \Illuminate\Http\Response
 
     public function delete(Request $request): \Illuminate\Http\JsonResponse
     {
-        $user = $request->user();
-
-        $request->validate([
-            'wiki' => 'required|numeric',
-        ]);
-
-        $wikiId = $request->input('wiki');
-        $userId = $user->id;
         $wikiDeletionReason = $request->input('deletionReasons');
-
-        // Check that the requesting user manages the wiki
-        if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
-            // The deletion was requested by a user that does not manage the wiki
-            return response()->json('Unauthorized', 401);
-        }
-        $wiki = Wiki::find($wikiId);
+        $wiki = $request->attributes->get('wiki');
 
         if(isset($wikiDeletionReason)){
             //Save the wiki deletion reason
@@ -231,25 +217,11 @@ public function delete(Request $request): \Illuminate\Http\JsonResponse
     // TODO should this just be get wiki?
     public function getWikiDetailsForIdForOwner(Request $request): \Illuminate\Http\Response
     {
-        $user = $request->user();
-
-        $wikiId = $request->input('wiki');
-
-        // TODO general check to make sure current user can manage the wiki
-        // this should probably be middle ware?
-        // TODO only do 1 query where instead of 2?
-        $test = WikiManager::where('user_id', $user->id)
-      ->where('wiki_id', $wikiId)
-      ->first();
-        if (! $test) {
-            abort(403);
-        }
-
-        $wiki = Wiki::where('id', $wikiId)
-      ->with('wikiManagers')
-      ->with('wikiDbVersion')
-      ->with('wikiLatestProfile')
-      ->with('publicSettings')->first();
+        $wiki = $request->attributes->get('wiki')
+            ->with('wikiManagers')
+            ->with('wikiDbVersion')
+            ->with('wikiLatestProfile')
+            ->with('publicSettings')->first();
 
         $res = [
             'success' => true,

app/Http/Controllers/WikiEntityImportController.php

@@ -5,7 +5,6 @@
 use App\WikiEntityImportStatus;
 use Illuminate\Http\Request;
 use Illuminate\Validation\Rule;
-use App\Wiki;
 use App\WikiEntityImport;
 use App\Jobs\WikiEntityImportJob;
 use Carbon\Carbon;
@@ -32,22 +31,14 @@ public function __construct(CollectorRegistry $registry)
     }
     public function get(Request $request): \Illuminate\Http\JsonResponse
     {
-        $validatedInput = $request->validate([
-            'wiki' => ['required', 'integer'],
-        ]);
-        $wiki = Wiki::find($validatedInput['wiki']);
-        if (!$wiki) {
-            abort(404, 'No such wiki');
-        }
-
+        $wiki = $request->attributes->get('wiki');
         $imports = $wiki->wikiEntityImports()->get();
         return response()->json(['data' => $imports]);
     }
 
     public function create(Request $request): \Illuminate\Http\JsonResponse
     {
         $validatedInput = $request->validate([
-            'wiki' => ['required', 'integer'],
             'source_wiki_url' => ['required', 'url'],
             'entity_ids' => ['required', 'string', function (string $attr, mixed $value, \Closure $fail) {
                 $chunks = explode(',', $value);
@@ -59,11 +50,7 @@ public function create(Request $request): \Illuminate\Http\JsonResponse
             }],
         ]);
 
-        $wiki = Wiki::find($validatedInput['wiki']);
-        if (!$wiki) {
-            abort(404, 'No such wiki');
-        }
-
+        $wiki = $request->attributes->get('wiki');
         $imports = $wiki->wikiEntityImports()->get();
         foreach ($imports as $import) {
             if ($import->status === WikiEntityImportStatus::Success) {

app/Http/Controllers/WikiLogoController.php

@@ -3,8 +3,6 @@
 namespace App\Http\Controllers;
 
 use App\Jobs\SetWikiLogo;
-use App\Wiki;
-use App\WikiManager;
 use App\WikiSetting;
 use Illuminate\Http\Request;
 
@@ -20,27 +18,15 @@ class WikiLogoController extends Controller
     public function update(Request $request)
     {
         $request->validate([
-            'wiki' => 'required|numeric',
             'logo' => 'required|mimes:png',
         ]);
 
-        $user = $request->user();
-
-        $wikiId = $request->input('wiki');
-        $userId = $user->id;
-
-        // Check that the requesting user manages the wiki
-        // TODO turn this into a generic guard for all of these types of routes...
-        if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
-            // The logo update was requested by a user that does not manage the wiki
-            return response()->json('Unauthorized', 401);
-        }
+        $wiki = $request->attributes->get('wiki');
 
         // run the job to set the wiki logo
-        ( new SetWikiLogo('id', $wikiId, $request->file('logo')->getRealPath()) )->handle();
+        ( new SetWikiLogo('id', $wiki->id, $request->file('logo')->getRealPath()) )->handle();
 
         // get the logo URL from the settings
-        $wiki = Wiki::find($wikiId);
         $wgLogoSetting = $wiki->settings()->firstWhere(['name' => WikiSetting::wgLogo])->value;
 
         $res['success'] = true;

app/Http/Controllers/WikiProfileController.php

@@ -4,7 +4,6 @@
 
 use App\Helper\ProfileValidator;
 use App\Rules\NonEmptyJsonRule;
-use App\Wiki;
 use App\WikiProfile;
 use Illuminate\Http\Request;
 
@@ -19,16 +18,11 @@ public function __construct(ProfileValidator $profileValidator)
 
     public function create(Request $request): \Illuminate\Http\JsonResponse
     {
+        $wiki = $request->attributes->get('wiki');
         $validatedInput = $request->validate([
-            'wiki' => ['required', 'integer'],
             'profile' => ['required', 'json', new NonEmptyJsonRule]
         ]);
 
-        $wiki = Wiki::find($validatedInput['wiki']);
-        if (!$wiki) {
-            abort(404, 'No such wiki');
-        }
-
         $rawProfile = json_decode($validatedInput['profile'], true);
         $profileValidator = $this->profileValidator->validate($rawProfile);
         $profileValidator->validateWithBag('post');

app/Http/Controllers/WikiSettingController.php

@@ -4,7 +4,6 @@
 
 use App\Rules\SettingCaptchaQuestions;
 use App\Rules\SettingWikibaseManifestEquivEntities;
-use App\WikiManager;
 use App\WikiSetting;
 use Illuminate\Http\Request;
 
@@ -40,29 +39,18 @@ public function update($setting, Request $request)
         $settingValidations = $this->getSettingValidations();
 
         $request->validate([
-            'wiki' => 'required|numeric',
             // Allow both internal and external setting names, see normalizeSetting
             'setting' => 'required|string|in:'.implode(',', array_keys($settingValidations)),
         ]);
         $settingName = $request->input('setting');
 
         $request->validate(['value' => $settingValidations[$settingName]]);
         $value = $request->input('value');
-
-        $user = $request->user();
-        $wikiId = $request->input('wiki');
-        $userId = $user->id;
-
-        // Check that the requesting user manages the wiki
-        // TODO turn this into a generic guard for all of these types of routes...
-        if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
-            // The deletion was requested by a user that does not manage the wiki
-            return response()->json('Unauthorized', 401);
-        }
+        $wiki = $request->attributes->get('wiki');
 
         WikiSetting::updateOrCreate(
             [
-                'wiki_id' => $wikiId,
+                'wiki_id' => $wiki->id,
                 'name' => $settingName,
             ],
             [

app/Http/Middleware/LimitWikiAccess.php

@@ -5,26 +5,36 @@
 use Closure;
 use Illuminate\Http\Request;
 use Symfony\Component\HttpFoundation\Response;
-use App\WikiManager;
+use App\Wiki;
 
 class LimitWikiAccess
 {
     /**
-     * Handle an incoming request.
-     *
-     * @param  \Closure(\Illuminate\Http\Request): (\Symfony\Component\HttpFoundation\Response)  $next
+     * Reject any incoming request unless the user is a manager of the
+     * requested wiki. If the user is authorized, inject the wiki
+     * object into the request context.
      */
     public function handle(Request $request, Closure $next): Response
     {
-        $userHasAccess = WikiManager::where([
-            'user_id' => $request->user()?->id,
-            'wiki_id' => $request->input('wiki'),
-        ])->exists();
+        $validatedInput = $request->validate([
+            'wiki' => ['required', 'integer']
+        ]);
 
-        if (!$userHasAccess) {
+        $wiki = Wiki::find($validatedInput['wiki']);
+
+        if (!$wiki) {
+            abort(404, 'No such wiki');
+        }
+
+        $wikiManager = $wiki->wikiManagers()
+            ->where('user_id', $request->user()?->id)
+            ->first();
+
+        if (!$wikiManager) {
             abort(403);
         }
 
+        $request->attributes->set('wiki', $wiki);
         return $next($request);
     }
 }

app/Jobs/SendEmptyWikiNotificationsJob.php

@@ -59,7 +59,7 @@ public function checkIfWikiIsOldAndEmpty(Wiki $wiki)
 
     public function sendEmptyWikiNotification (Wiki $wiki): void
     {
-        $wikiManagers = $wiki->wikiManagers()->get();
+        $wikiManagers = $wiki->wikiManagersWithEmail()->get();
 
         foreach($wikiManagers as $wikiManager) {
             // we think the order here matters, so that people do not get spammed in case creating a record fails

app/Wiki.php

@@ -108,6 +108,10 @@ public function wikiEntityImports(): \Illuminate\Database\Eloquent\Relations\Has
         return $this->hasMany(WikiEntityImport::class);
     }
 
+    public function wikiManagers(): \Illuminate\Database\Eloquent\Relations\HasMany {
+        return $this->hasMany(WikiManager::class);
+    }
+
     /**
      * @return \Illuminate\Database\Eloquent\Relations\HasOne
      *
@@ -150,7 +154,7 @@ public function publicSettings()
     );
     }
 
-    public function wikiManagers()
+    public function wikiManagersWithEmail()
     {
         // TODO should this be hasMany ?
         /**

routes/api.php

@@ -39,14 +39,16 @@
         });
         $router->group(['prefix' => 'wiki', 'middleware' => ['verified']], function () use ($router) {
             $router->post('create', ['uses' => 'WikiController@create']);
-            $router->post('delete', ['uses' => 'WikiController@delete']);
-            $router->post('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
-            $router->get('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
-            $router->post('logo/update', ['uses' => 'WikiLogoController@update']);
-            $router->post('setting/{setting}/update', ['uses' => 'WikiSettingController@update']);
-            $router->get('entityImport', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiEntityImportController@get']);
-            $router->post('entityImport', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiEntityImportController@create']);
-            $router->post('profile', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiProfileController@create']);
+            $router->group(['middleware' => 'limit_wiki_access'], function () use ($router) {
+                $router->post('delete', ['uses' => 'WikiController@delete']);
+                $router->post('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
+                $router->get('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
+                $router->post('logo/update', ['uses' => 'WikiLogoController@update']);
+                $router->post('setting/{setting}/update', ['uses' => 'WikiSettingController@update']);
+                $router->get('entityImport', ['uses' => 'WikiEntityImportController@get']);
+                $router->post('entityImport', ['uses' => 'WikiEntityImportController@create']);
+                $router->post('profile', ['uses' => 'WikiProfileController@create']);
+            });
         });
         $router->apiResource('deletedWikiMetrics', 'DeletedWikiMetricsController')->only(['index'])
             ->middleware(AuthorisedUsersForDeletedWikiMetricsMiddleware::class);

tests/Middleware/LimitWikiAccessTest.php

@@ -0,0 +1,83 @@
+<?php
+
+namespace Tests\Jobs;
+
+use App\User;
+use App\Wiki;
+use App\WikiManager;
+use Illuminate\Http\Request;
+use Tests\TestCase;
+use Illuminate\Support\Facades\Route;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+
+class LimitWikiAccessTest extends TestCase
+{
+    use RefreshDatabase;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+        Route::middleware('limit_wiki_access')->get('/endpoint', function (Request $request) {
+            return response()->json([
+                'wiki_id' => $request->attributes->get('wiki')->id
+            ]);
+        });
+    }
+
+    public function tearDown(): void
+    {
+        parent::tearDown();
+    }
+
+    private function createWikiAndUser(): array
+    {
+        $wiki = Wiki::factory()->create();
+        $user = User::factory()->create(['verified' => true]);
+        WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]);
+        return array($wiki, $user);
+    }
+
+    private function getURI(Wiki $wiki): string
+    {
+        return "/endpoint?wiki={$wiki->id}";
+    }
+
+    public function testSuccess(): void
+    {
+        [$wiki, $user] = $this->createWikiAndUser();
+
+        $this->actingAs($user)
+            ->json('GET', $this->getURI($wiki))
+            ->assertStatus(200)
+            ->assertJson(['wiki_id' => $wiki->id]);
+    }
+
+    public function testFailOnWrongWikiManager(): void
+    {
+        $userWiki = Wiki::factory()->create();
+        $otherWiki = Wiki::factory()->create();
+        $user = User::factory()->create(['verified' => true]);
+        WikiManager::factory()->create(['wiki_id' => $userWiki->id, 'user_id' => $user->id]);
+        $this->actingAs($user)->json('GET', $this->getURI($otherWiki))->assertStatus(403);
+    }
+
+    public function testFailOnDeletedWiki(): void
+    {
+        [$wiki, $user] = $this->createWikiAndUser();
+        $wiki->wikiManagers()->delete();
+        $wiki->delete();
+        $this->actingAs($user)->json('GET', $this->getURI($wiki))->assertStatus(404);
+    }
+
+    public function testFailOnMissingWiki(): void
+    {
+        [$wiki, $user] = $this->createWikiAndUser();
+        $this->actingAs($user)->json('GET', '/endpoint')->assertStatus(422);
+    }
+
+    public function testFailOnMissingUser(): void
+    {
+        [$wiki, $user] = $this->createWikiAndUser();
+        $this->json('GET', $this->getURI($wiki))->assertStatus(403);
+    }
+}