Skip to content

Expand usage of LimitWikiAccess middleware #937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 6 additions & 34 deletions app/Http/Controllers/WikiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
16 changes: 2 additions & 14 deletions app/Http/Controllers/WikiEntityImportController.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,22 +32,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);
Expand All @@ -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) {
Expand Down
16 changes: 2 additions & 14 deletions app/Http/Controllers/WikiLogoController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 1 addition & 7 deletions app/Http/Controllers/WikiProfileController.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

use App\Helper\ProfileValidator;
use App\Rules\NonEmptyJsonRule;
use App\Wiki;
use App\WikiProfile;
use Illuminate\Http\Request;

Expand All @@ -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');
Expand Down
15 changes: 2 additions & 13 deletions app/Http/Controllers/WikiSettingController.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,29 +40,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,
],
[
Expand Down
28 changes: 19 additions & 9 deletions app/Http/Middleware/LimitWikiAccess.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
2 changes: 1 addition & 1 deletion app/Jobs/SendEmptyWikiNotificationsJob.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion app/Wiki.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -150,7 +154,7 @@ public function publicSettings()
);
}

public function wikiManagers()
public function wikiManagersWithEmail()
{
// TODO should this be hasMany ?
/**
Expand Down
18 changes: 10 additions & 8 deletions routes/api.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
83 changes: 83 additions & 0 deletions tests/Middleware/LimitWikiAccessTest.php
Original file line number Diff line number Diff line change
@@ -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);
}
}
11 changes: 11 additions & 0 deletions tests/Routes/Wiki/DeleteWikiTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
11 changes: 11 additions & 0 deletions tests/Routes/Wiki/DetailsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading
Loading