diff --git a/app/Http/Controllers/WikiController.php b/app/Http/Controllers/WikiController.php index 2c194efd..00c580db 100644 --- a/app/Http/Controllers/WikiController.php +++ b/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, diff --git a/app/Http/Controllers/WikiEntityImportController.php b/app/Http/Controllers/WikiEntityImportController.php index 77b0572b..b00ab044 100644 --- a/app/Http/Controllers/WikiEntityImportController.php +++ b/app/Http/Controllers/WikiEntityImportController.php @@ -32,14 +32,7 @@ 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]); } @@ -47,7 +40,6 @@ public function get(Request $request): \Illuminate\Http\JsonResponse 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 +51,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) { diff --git a/app/Http/Controllers/WikiLogoController.php b/app/Http/Controllers/WikiLogoController.php index c8c7de04..57faf60d 100644 --- a/app/Http/Controllers/WikiLogoController.php +++ b/app/Http/Controllers/WikiLogoController.php @@ -20,27 +20,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; diff --git a/app/Http/Controllers/WikiProfileController.php b/app/Http/Controllers/WikiProfileController.php index d7f84131..2cb0c7f4 100644 --- a/app/Http/Controllers/WikiProfileController.php +++ b/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'); diff --git a/app/Http/Controllers/WikiSettingController.php b/app/Http/Controllers/WikiSettingController.php index d1cf925b..c27b7f01 100644 --- a/app/Http/Controllers/WikiSettingController.php +++ b/app/Http/Controllers/WikiSettingController.php @@ -40,7 +40,6 @@ 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)), ]); @@ -48,21 +47,11 @@ public function update($setting, Request $request) $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, ], [ diff --git a/app/Http/Middleware/LimitWikiAccess.php b/app/Http/Middleware/LimitWikiAccess.php index 041676be..ba70f89a 100644 --- a/app/Http/Middleware/LimitWikiAccess.php +++ b/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); } } diff --git a/app/Jobs/SendEmptyWikiNotificationsJob.php b/app/Jobs/SendEmptyWikiNotificationsJob.php index 6fe58b1c..312ee69a 100644 --- a/app/Jobs/SendEmptyWikiNotificationsJob.php +++ b/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 diff --git a/app/Wiki.php b/app/Wiki.php index c2dab597..8fcafb05 100644 --- a/app/Wiki.php +++ b/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 ? /** diff --git a/routes/api.php b/routes/api.php index 015ffd58..e58bf537 100644 --- a/routes/api.php +++ b/routes/api.php @@ -38,14 +38,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); diff --git a/tests/Middleware/LimitWikiAccessTest.php b/tests/Middleware/LimitWikiAccessTest.php new file mode 100644 index 00000000..066e1335 --- /dev/null +++ b/tests/Middleware/LimitWikiAccessTest.php @@ -0,0 +1,83 @@ +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); + } +} diff --git a/tests/Routes/Wiki/DeleteWikiTest.php b/tests/Routes/Wiki/DeleteWikiTest.php index 4411ed20..66c8de65 100644 --- a/tests/Routes/Wiki/DeleteWikiTest.php +++ b/tests/Routes/Wiki/DeleteWikiTest.php @@ -34,4 +34,15 @@ public function testDelete() Wiki::withTrashed()->find($wiki->id)->wiki_deletion_reason ); } + + 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, 'api') + ->post('wiki/delete', ['wiki' => $otherWiki->id]) + ->assertStatus(403); + } } diff --git a/tests/Routes/Wiki/DetailsTest.php b/tests/Routes/Wiki/DetailsTest.php index 06b6a907..a793988c 100644 --- a/tests/Routes/Wiki/DetailsTest.php +++ b/tests/Routes/Wiki/DetailsTest.php @@ -61,6 +61,17 @@ public function testSkipsNonPublicSettings() $this->assertEquals(1, $publicSettings[0]['value']); } + 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, 'api') + ->postJson($this->route, ['wiki' => $otherWiki->id]) + ->assertStatus(403); + } + public function testWikiProfile() { $wiki = Wiki::factory()->create(); diff --git a/tests/Routes/Wiki/LogoUpdateTest.php b/tests/Routes/Wiki/LogoUpdateTest.php index 95a57a61..cb39b304 100644 --- a/tests/Routes/Wiki/LogoUpdateTest.php +++ b/tests/Routes/Wiki/LogoUpdateTest.php @@ -54,4 +54,17 @@ public function testUpdate() $this->assertSame(64, $logo->height()); $this->assertSame(64, $logo->width()); } + + 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]); + $file = UploadedFile::fake() + ->createWithContent("logo_200x200.png", file_get_contents(__DIR__ . "/../../data/logo_200x200.png")); + $this->actingAs($user, 'api') + ->post('wiki/logo/update', ['wiki' => $otherWiki->id, 'logo' => $file]) + ->assertStatus(403); + } } diff --git a/tests/Routes/Wiki/SettingUpdateTest.php b/tests/Routes/Wiki/SettingUpdateTest.php index 1495486b..3ead4a93 100644 --- a/tests/Routes/Wiki/SettingUpdateTest.php +++ b/tests/Routes/Wiki/SettingUpdateTest.php @@ -26,11 +26,12 @@ class SettingUpdateTest extends TestCase public function testSetInvalidSetting() { $settingName = 'iDoNotExistAsASetting'; - + $wiki = Wiki::factory()->create(); $user = User::factory()->create(['verified' => true]); + WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]); $this->actingAs($user, 'api') ->json('POST', str_replace('foo', $settingName, $this->route), [ - 'wiki' => 1, + 'wiki' => $wiki->id, 'setting' => $settingName, 'value' => '1', ]) @@ -38,18 +39,20 @@ public function testSetInvalidSetting() ->assertJsonStructure(['errors' => ['setting']]); } - public function testValidSettingNoWiki() + public function testFailOnWrongWikiManager(): void { $settingName = 'wwExtEnableConfirmAccount'; - + $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, 'api') - ->json('POST', str_replace('foo', $settingName, $this->route), [ - 'wiki' => 99856, - 'setting' => $settingName, - 'value' => '1', - ]) - ->assertStatus(401); + ->postJson(str_replace('foo', $settingName, $this->route), [ + 'wiki' => $otherWiki->id, + 'setting' => $settingName, + 'value' => '1' + ]) + ->assertStatus(403); } static public function provideValidSettings() @@ -154,11 +157,13 @@ static public function provideValidSettingsBadValues() */ public function testValidSettingBadValues($settingName, $settingValue) { + $wiki = Wiki::factory()->create(); $user = User::factory()->create(['verified' => true]); + WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]); $this->actingAs($user, 'api') ->json('POST', str_replace('foo', $settingName, $this->route), [ - 'wiki' => 1, + 'wiki' => $wiki->id, 'setting' => $settingName, 'value' => $settingValue, ])