Skip to content

Commit 285f186

Browse files
AndrewKostkaoutdooracorn
authored and
deer-wmde
committed
Expand usage of LimitWikiAccess middleware (#937)
* Refactor LimitWikiAccess middleware * Add missing test cases prior to refactoring * Refactor routes to use LimitWikiAccess middleware * Rename LimitWikiAccessText to LimitWikiAccessTest * Use RefreshDatabase for LimitWikiAccessTest * Return 404 for non-existent wikis * Minor code cleanup during review * Remove now unused imports * Add indents to method chain --------- Co-authored-by: Ollie <oliver.hyde@wikimedia.de>
1 parent ec884b9 commit 285f186

14 files changed

Lines changed: 182 additions & 117 deletions

‎app/Http/Controllers/WikiController.php‎

Lines changed: 6 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -200,22 +200,8 @@ public function create(Request $request): \Illuminate\Http\Response
200200

201201
public function delete(Request $request): \Illuminate\Http\JsonResponse
202202
{
203-
$user = $request->user();
204-
205-
$request->validate([
206-
'wiki' => 'required|numeric',
207-
]);
208-
209-
$wikiId = $request->input('wiki');
210-
$userId = $user->id;
211203
$wikiDeletionReason = $request->input('deletionReasons');
212-
213-
// Check that the requesting user manages the wiki
214-
if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
215-
// The deletion was requested by a user that does not manage the wiki
216-
return response()->json('Unauthorized', 401);
217-
}
218-
$wiki = Wiki::find($wikiId);
204+
$wiki = $request->attributes->get('wiki');
219205

220206
if(isset($wikiDeletionReason)){
221207
//Save the wiki deletion reason
@@ -231,25 +217,11 @@ public function delete(Request $request): \Illuminate\Http\JsonResponse
231217
// TODO should this just be get wiki?
232218
public function getWikiDetailsForIdForOwner(Request $request): \Illuminate\Http\Response
233219
{
234-
$user = $request->user();
235-
236-
$wikiId = $request->input('wiki');
237-
238-
// TODO general check to make sure current user can manage the wiki
239-
// this should probably be middle ware?
240-
// TODO only do 1 query where instead of 2?
241-
$test = WikiManager::where('user_id', $user->id)
242-
->where('wiki_id', $wikiId)
243-
->first();
244-
if (! $test) {
245-
abort(403);
246-
}
247-
248-
$wiki = Wiki::where('id', $wikiId)
249-
->with('wikiManagers')
250-
->with('wikiDbVersion')
251-
->with('wikiLatestProfile')
252-
->with('publicSettings')->first();
220+
$wiki = $request->attributes->get('wiki')
221+
->with('wikiManagers')
222+
->with('wikiDbVersion')
223+
->with('wikiLatestProfile')
224+
->with('publicSettings')->first();
253225

254226
$res = [
255227
'success' => true,

‎app/Http/Controllers/WikiEntityImportController.php‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
use App\WikiEntityImportStatus;
66
use Illuminate\Http\Request;
77
use Illuminate\Validation\Rule;
8-
use App\Wiki;
98
use App\WikiEntityImport;
109
use App\Jobs\WikiEntityImportJob;
1110
use Carbon\Carbon;
@@ -32,22 +31,14 @@ public function __construct(CollectorRegistry $registry)
3231
}
3332
public function get(Request $request): \Illuminate\Http\JsonResponse
3433
{
35-
$validatedInput = $request->validate([
36-
'wiki' => ['required', 'integer'],
37-
]);
38-
$wiki = Wiki::find($validatedInput['wiki']);
39-
if (!$wiki) {
40-
abort(404, 'No such wiki');
41-
}
42-
34+
$wiki = $request->attributes->get('wiki');
4335
$imports = $wiki->wikiEntityImports()->get();
4436
return response()->json(['data' => $imports]);
4537
}
4638

4739
public function create(Request $request): \Illuminate\Http\JsonResponse
4840
{
4941
$validatedInput = $request->validate([
50-
'wiki' => ['required', 'integer'],
5142
'source_wiki_url' => ['required', 'url'],
5243
'entity_ids' => ['required', 'string', function (string $attr, mixed $value, \Closure $fail) {
5344
$chunks = explode(',', $value);
@@ -59,11 +50,7 @@ public function create(Request $request): \Illuminate\Http\JsonResponse
5950
}],
6051
]);
6152

62-
$wiki = Wiki::find($validatedInput['wiki']);
63-
if (!$wiki) {
64-
abort(404, 'No such wiki');
65-
}
66-
53+
$wiki = $request->attributes->get('wiki');
6754
$imports = $wiki->wikiEntityImports()->get();
6855
foreach ($imports as $import) {
6956
if ($import->status === WikiEntityImportStatus::Success) {

‎app/Http/Controllers/WikiLogoController.php‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
namespace App\Http\Controllers;
44

55
use App\Jobs\SetWikiLogo;
6-
use App\Wiki;
7-
use App\WikiManager;
86
use App\WikiSetting;
97
use Illuminate\Http\Request;
108

@@ -20,27 +18,15 @@ class WikiLogoController extends Controller
2018
public function update(Request $request)
2119
{
2220
$request->validate([
23-
'wiki' => 'required|numeric',
2421
'logo' => 'required|mimes:png',
2522
]);
2623

27-
$user = $request->user();
28-
29-
$wikiId = $request->input('wiki');
30-
$userId = $user->id;
31-
32-
// Check that the requesting user manages the wiki
33-
// TODO turn this into a generic guard for all of these types of routes...
34-
if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
35-
// The logo update was requested by a user that does not manage the wiki
36-
return response()->json('Unauthorized', 401);
37-
}
24+
$wiki = $request->attributes->get('wiki');
3825

3926
// run the job to set the wiki logo
40-
( new SetWikiLogo('id', $wikiId, $request->file('logo')->getRealPath()) )->handle();
27+
( new SetWikiLogo('id', $wiki->id, $request->file('logo')->getRealPath()) )->handle();
4128

4229
// get the logo URL from the settings
43-
$wiki = Wiki::find($wikiId);
4430
$wgLogoSetting = $wiki->settings()->firstWhere(['name' => WikiSetting::wgLogo])->value;
4531

4632
$res['success'] = true;

‎app/Http/Controllers/WikiProfileController.php‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44

55
use App\Helper\ProfileValidator;
66
use App\Rules\NonEmptyJsonRule;
7-
use App\Wiki;
87
use App\WikiProfile;
98
use Illuminate\Http\Request;
109

@@ -19,16 +18,11 @@ public function __construct(ProfileValidator $profileValidator)
1918

2019
public function create(Request $request): \Illuminate\Http\JsonResponse
2120
{
21+
$wiki = $request->attributes->get('wiki');
2222
$validatedInput = $request->validate([
23-
'wiki' => ['required', 'integer'],
2423
'profile' => ['required', 'json', new NonEmptyJsonRule]
2524
]);
2625

27-
$wiki = Wiki::find($validatedInput['wiki']);
28-
if (!$wiki) {
29-
abort(404, 'No such wiki');
30-
}
31-
3226
$rawProfile = json_decode($validatedInput['profile'], true);
3327
$profileValidator = $this->profileValidator->validate($rawProfile);
3428
$profileValidator->validateWithBag('post');

‎app/Http/Controllers/WikiSettingController.php‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44

55
use App\Rules\SettingCaptchaQuestions;
66
use App\Rules\SettingWikibaseManifestEquivEntities;
7-
use App\WikiManager;
87
use App\WikiSetting;
98
use Illuminate\Http\Request;
109

@@ -40,29 +39,18 @@ public function update($setting, Request $request)
4039
$settingValidations = $this->getSettingValidations();
4140

4241
$request->validate([
43-
'wiki' => 'required|numeric',
4442
// Allow both internal and external setting names, see normalizeSetting
4543
'setting' => 'required|string|in:'.implode(',', array_keys($settingValidations)),
4644
]);
4745
$settingName = $request->input('setting');
4846

4947
$request->validate(['value' => $settingValidations[$settingName]]);
5048
$value = $request->input('value');
51-
52-
$user = $request->user();
53-
$wikiId = $request->input('wiki');
54-
$userId = $user->id;
55-
56-
// Check that the requesting user manages the wiki
57-
// TODO turn this into a generic guard for all of these types of routes...
58-
if (WikiManager::where('user_id', $userId)->where('wiki_id', $wikiId)->count() !== 1) {
59-
// The deletion was requested by a user that does not manage the wiki
60-
return response()->json('Unauthorized', 401);
61-
}
49+
$wiki = $request->attributes->get('wiki');
6250

6351
WikiSetting::updateOrCreate(
6452
[
65-
'wiki_id' => $wikiId,
53+
'wiki_id' => $wiki->id,
6654
'name' => $settingName,
6755
],
6856
[

‎app/Http/Middleware/LimitWikiAccess.php‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,26 +5,36 @@
55
use Closure;
66
use Illuminate\Http\Request;
77
use Symfony\Component\HttpFoundation\Response;
8-
use App\WikiManager;
8+
use App\Wiki;
99

1010
class LimitWikiAccess
1111
{
1212
/**
13-
* Handle an incoming request.
14-
*
15-
* @param \Closure(\Illuminate\Http\Request): (\Symfony\Component\HttpFoundation\Response) $next
13+
* Reject any incoming request unless the user is a manager of the
14+
* requested wiki. If the user is authorized, inject the wiki
15+
* object into the request context.
1616
*/
1717
public function handle(Request $request, Closure $next): Response
1818
{
19-
$userHasAccess = WikiManager::where([
20-
'user_id' => $request->user()?->id,
21-
'wiki_id' => $request->input('wiki'),
22-
])->exists();
19+
$validatedInput = $request->validate([
20+
'wiki' => ['required', 'integer']
21+
]);
2322

24-
if (!$userHasAccess) {
23+
$wiki = Wiki::find($validatedInput['wiki']);
24+
25+
if (!$wiki) {
26+
abort(404, 'No such wiki');
27+
}
28+
29+
$wikiManager = $wiki->wikiManagers()
30+
->where('user_id', $request->user()?->id)
31+
->first();
32+
33+
if (!$wikiManager) {
2534
abort(403);
2635
}
2736

37+
$request->attributes->set('wiki', $wiki);
2838
return $next($request);
2939
}
3040
}

‎app/Jobs/SendEmptyWikiNotificationsJob.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function checkIfWikiIsOldAndEmpty(Wiki $wiki)
5959

6060
public function sendEmptyWikiNotification (Wiki $wiki): void
6161
{
62-
$wikiManagers = $wiki->wikiManagers()->get();
62+
$wikiManagers = $wiki->wikiManagersWithEmail()->get();
6363

6464
foreach($wikiManagers as $wikiManager) {
6565
// we think the order here matters, so that people do not get spammed in case creating a record fails

‎app/Wiki.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,10 @@ public function wikiEntityImports(): \Illuminate\Database\Eloquent\Relations\Has
108108
return $this->hasMany(WikiEntityImport::class);
109109
}
110110

111+
public function wikiManagers(): \Illuminate\Database\Eloquent\Relations\HasMany {
112+
return $this->hasMany(WikiManager::class);
113+
}
114+
111115
/**
112116
* @return \Illuminate\Database\Eloquent\Relations\HasOne
113117
*
@@ -150,7 +154,7 @@ public function publicSettings()
150154
);
151155
}
152156

153-
public function wikiManagers()
157+
public function wikiManagersWithEmail()
154158
{
155159
// TODO should this be hasMany ?
156160
/**

‎routes/api.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,16 @@
3939
});
4040
$router->group(['prefix' => 'wiki', 'middleware' => ['verified']], function () use ($router) {
4141
$router->post('create', ['uses' => 'WikiController@create']);
42-
$router->post('delete', ['uses' => 'WikiController@delete']);
43-
$router->post('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
44-
$router->get('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
45-
$router->post('logo/update', ['uses' => 'WikiLogoController@update']);
46-
$router->post('setting/{setting}/update', ['uses' => 'WikiSettingController@update']);
47-
$router->get('entityImport', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiEntityImportController@get']);
48-
$router->post('entityImport', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiEntityImportController@create']);
49-
$router->post('profile', ['middleware' => 'limit_wiki_access', 'uses' => 'WikiProfileController@create']);
42+
$router->group(['middleware' => 'limit_wiki_access'], function () use ($router) {
43+
$router->post('delete', ['uses' => 'WikiController@delete']);
44+
$router->post('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
45+
$router->get('details', ['uses' => 'WikiController@getWikiDetailsForIdForOwner']);
46+
$router->post('logo/update', ['uses' => 'WikiLogoController@update']);
47+
$router->post('setting/{setting}/update', ['uses' => 'WikiSettingController@update']);
48+
$router->get('entityImport', ['uses' => 'WikiEntityImportController@get']);
49+
$router->post('entityImport', ['uses' => 'WikiEntityImportController@create']);
50+
$router->post('profile', ['uses' => 'WikiProfileController@create']);
51+
});
5052
});
5153
$router->apiResource('deletedWikiMetrics', 'DeletedWikiMetricsController')->only(['index'])
5254
->middleware(AuthorisedUsersForDeletedWikiMetricsMiddleware::class);

‎tests/Middleware/LimitWikiAccessTest.php‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
<?php
2+
3+
namespace Tests\Jobs;
4+
5+
use App\User;
6+
use App\Wiki;
7+
use App\WikiManager;
8+
use Illuminate\Http\Request;
9+
use Tests\TestCase;
10+
use Illuminate\Support\Facades\Route;
11+
use Illuminate\Foundation\Testing\RefreshDatabase;
12+
13+
class LimitWikiAccessTest extends TestCase
14+
{
15+
use RefreshDatabase;
16+
17+
public function setUp(): void
18+
{
19+
parent::setUp();
20+
Route::middleware('limit_wiki_access')->get('/endpoint', function (Request $request) {
21+
return response()->json([
22+
'wiki_id' => $request->attributes->get('wiki')->id
23+
]);
24+
});
25+
}
26+
27+
public function tearDown(): void
28+
{
29+
parent::tearDown();
30+
}
31+
32+
private function createWikiAndUser(): array
33+
{
34+
$wiki = Wiki::factory()->create();
35+
$user = User::factory()->create(['verified' => true]);
36+
WikiManager::factory()->create(['wiki_id' => $wiki->id, 'user_id' => $user->id]);
37+
return array($wiki, $user);
38+
}
39+
40+
private function getURI(Wiki $wiki): string
41+
{
42+
return "/endpoint?wiki={$wiki->id}";
43+
}
44+
45+
public function testSuccess(): void
46+
{
47+
[$wiki, $user] = $this->createWikiAndUser();
48+
49+
$this->actingAs($user)
50+
->json('GET', $this->getURI($wiki))
51+
->assertStatus(200)
52+
->assertJson(['wiki_id' => $wiki->id]);
53+
}
54+
55+
public function testFailOnWrongWikiManager(): void
56+
{
57+
$userWiki = Wiki::factory()->create();
58+
$otherWiki = Wiki::factory()->create();
59+
$user = User::factory()->create(['verified' => true]);
60+
WikiManager::factory()->create(['wiki_id' => $userWiki->id, 'user_id' => $user->id]);
61+
$this->actingAs($user)->json('GET', $this->getURI($otherWiki))->assertStatus(403);
62+
}
63+
64+
public function testFailOnDeletedWiki(): void
65+
{
66+
[$wiki, $user] = $this->createWikiAndUser();
67+
$wiki->wikiManagers()->delete();
68+
$wiki->delete();
69+
$this->actingAs($user)->json('GET', $this->getURI($wiki))->assertStatus(404);
70+
}
71+
72+
public function testFailOnMissingWiki(): void
73+
{
74+
[$wiki, $user] = $this->createWikiAndUser();
75+
$this->actingAs($user)->json('GET', '/endpoint')->assertStatus(422);
76+
}
77+
78+
public function testFailOnMissingUser(): void
79+
{
80+
[$wiki, $user] = $this->createWikiAndUser();
81+
$this->json('GET', $this->getURI($wiki))->assertStatus(403);
82+
}
83+
}

0 commit comments

Comments
 (0)