Skip to content

Commit 164c625

Browse files
LukeTowersLukeTowers
committed
Improve permission checking of Theme Editor permissions
Also call things what they are. Took long enough but the truth always prevails.
1 parent 6c3c7aa commit 164c625

5 files changed

Lines changed: 317 additions & 15 deletions

File tree

‎modules/cms/controllers/Index.php‎

Lines changed: 81 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
use Illuminate\Support\Facades\Lang;
2525
use Illuminate\Support\Facades\Request;
2626
use System\Helpers\DateTime;
27+
use Winter\Storm\Auth\AuthorizationException;
2728
use Winter\Storm\Exception\ApplicationException;
2829
use Winter\Storm\Halcyon\Datasource\DatasourceInterface;
2930
use Winter\Storm\Router\Router as StormRouter;
@@ -92,25 +93,43 @@ public function __construct()
9293

9394
$this->theme = $theme;
9495

95-
new TemplateList($this, 'pageList', function () use ($theme) {
96-
return Page::listInTheme($theme, true);
97-
});
96+
if ($this->user->hasAccess('cms.manage_pages')) {
97+
new TemplateList($this, 'pageList', function () use ($theme) {
98+
return Page::listInTheme($theme, true);
99+
});
100+
}
98101

99-
new TemplateList($this, 'partialList', function () use ($theme) {
100-
return Partial::listInTheme($theme, true);
101-
});
102+
if ($this->user->hasAccess('cms.manage_partials')) {
103+
new TemplateList($this, 'partialList', function () use ($theme) {
104+
return Partial::listInTheme($theme, true);
105+
});
106+
}
102107

103-
new TemplateList($this, 'layoutList', function () use ($theme) {
104-
return Layout::listInTheme($theme, true);
105-
});
108+
if ($this->user->hasAccess('cms.manage_layouts')) {
109+
new TemplateList($this, 'layoutList', function () use ($theme) {
110+
return Layout::listInTheme($theme, true);
111+
});
112+
}
106113

107-
new TemplateList($this, 'contentList', function () use ($theme) {
108-
return Content::listInTheme($theme, true);
109-
});
114+
if ($this->user->hasAccess('cms.manage_content')) {
115+
new TemplateList($this, 'contentList', function () use ($theme) {
116+
return Content::listInTheme($theme, true);
117+
});
118+
}
110119

111-
new ComponentList($this, 'componentList');
120+
if (
121+
$this->user->hasAccess([
122+
'cms.manage_pages',
123+
'cms.manage_partials',
124+
'cms.manage_layouts',
125+
], false)
126+
) {
127+
new ComponentList($this, 'componentList');
128+
}
112129

113-
new AssetList($this, 'assetList');
130+
if ($this->user->hasAccess('cms.manage_assets')) {
131+
new AssetList($this, 'assetList');
132+
}
114133
}
115134
catch (Exception $ex) {
116135
$this->handleError($ex);
@@ -148,6 +167,8 @@ public function index_onOpenTemplate(): array
148167
$this->validateRequestTheme();
149168

150169
$type = Request::input('type');
170+
$this->validateRequestType($type);
171+
151172
$template = $this->loadTemplate($type, Request::input('path'));
152173
$widget = $this->makeTemplateFormWidget($type, $template);
153174

@@ -179,7 +200,10 @@ public function index_onOpenTemplate(): array
179200
public function onSave(): array
180201
{
181202
$this->validateRequestTheme();
203+
182204
$type = Request::input('templateType');
205+
$this->validateRequestType($type);
206+
183207
$templatePath = trim(Request::input('templatePath'));
184208
$template = $templatePath ? $this->loadTemplate($type, $templatePath) : $this->createTemplate($type);
185209
$formWidget = $this->makeTemplateFormWidget($type, $template);
@@ -263,6 +287,8 @@ public function onOpenConcurrencyResolveForm(): string
263287
public function onCreateTemplate(): array
264288
{
265289
$type = Request::input('type');
290+
$this->validateRequestType($type);
291+
266292
$template = $this->createTemplate($type);
267293

268294
if ($type === 'asset') {
@@ -294,6 +320,8 @@ public function onDeleteTemplates(): array
294320
$this->validateRequestTheme();
295321

296322
$type = Request::input('type');
323+
$this->validateRequestType($type);
324+
297325
$templates = Request::input('template');
298326
$error = null;
299327
$deleted = [];
@@ -344,6 +372,7 @@ public function onDelete(): void
344372
$this->validateRequestTheme();
345373

346374
$type = Request::input('templateType');
375+
$this->validateRequestType($type);
347376

348377
$this->loadTemplate($type, trim(Request::input('templatePath')))->delete();
349378

@@ -415,7 +444,10 @@ public function onExpandMarkupToken(): string
415444
public function onCommit(): array
416445
{
417446
$this->validateRequestTheme();
447+
418448
$type = Request::input('templateType');
449+
$this->validateRequestType($type);
450+
419451
$template = $this->loadTemplate($type, trim(Request::input('templatePath')));
420452

421453
if ($this->canCommitTemplate($template)) {
@@ -439,7 +471,10 @@ public function onCommit(): array
439471
public function onReset(): array
440472
{
441473
$this->validateRequestTheme();
474+
442475
$type = Request::input('templateType');
476+
$this->validateRequestType($type);
477+
443478
$template = $this->loadTemplate($type, trim(Request::input('templatePath')));
444479

445480
if ($this->canResetTemplate($template)) {
@@ -555,6 +590,38 @@ protected function validateRequestTheme(): void
555590
}
556591
}
557592

593+
/**
594+
* Validates that the given request type is a valid type, and that the user has the relevant
595+
* permission to access it.
596+
*
597+
* @throws AuthorizationException if the user doesn't have permission to access the type
598+
*/
599+
protected function validateRequestType(string $type): void
600+
{
601+
$this->resolveTypeClassName($type);
602+
603+
if (!$this->user->hasAccess($this->getRelevantPermissionForType($type))) {
604+
throw new AuthorizationException(Lang::get('cms::lang.template.type_not_permitted', [
605+
'type' => str_plural($type),
606+
'permission' => $this->getRelevantPermissionForType($type),
607+
]));
608+
}
609+
}
610+
611+
/**
612+
* Gets the relevant permission required for a specific template type.
613+
*/
614+
protected function getRelevantPermissionForType(string $type): string
615+
{
616+
return match ($type) {
617+
'page' => 'cms.manage_pages',
618+
'partial' => 'cms.manage_partials',
619+
'layout' => 'cms.manage_layouts',
620+
'content' => 'cms.manage_content',
621+
'asset' => 'cms.manage_assets',
622+
};
623+
}
624+
558625
/**
559626
* Resolves a template type to its class name
560627
*/

‎modules/cms/lang/en/lang.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@
171171
'not_found' => "AJAX handler ':name' was not found.",
172172
],
173173
'cms' => [
174-
'menu_label' => 'CMS',
174+
'menu_label' => 'Theme Editor',
175175
],
176176
'sidebar' => [
177177
'add' => 'Add',
@@ -271,6 +271,7 @@
271271
'no_list_records' => 'No records found',
272272
'delete_confirm' => 'Delete selected templates?',
273273
'order_by' => 'Order by',
274+
'type_not_permitted' => 'You require the ":permission" permission to manage :type',
274275
],
275276
'permissions' => [
276277
'name' => 'CMS',

‎modules/cms/tests/controllers/IndexPermissionTest.php‎

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
<?php
2+
3+
namespace Cms\Tests\Controllers;
4+
5+
use Backend\Tests\Fixtures\Models\UserFixture;
6+
use Cms\Controllers\Index;
7+
use System\Tests\Bootstrap\PluginTestCase;
8+
use Winter\Storm\Auth\AuthorizationException;
9+
10+
class IndexPermissionTest extends PluginTestCase
11+
{
12+
protected Index $controller;
13+
14+
protected static array $allTypes = ['page', 'partial', 'layout', 'content', 'asset'];
15+
16+
protected static array $permissionMap = [
17+
'page' => 'cms.manage_pages',
18+
'partial' => 'cms.manage_partials',
19+
'layout' => 'cms.manage_layouts',
20+
'content' => 'cms.manage_content',
21+
'asset' => 'cms.manage_assets',
22+
];
23+
24+
//
25+
// getRelevantPermissionForType mapping
26+
//
27+
28+
/**
29+
* @dataProvider permissionMappingProvider
30+
*/
31+
public function testGetRelevantPermissionForType(string $type, string $expectedPermission): void
32+
{
33+
$this->actingAs((new UserFixture)->asSuperUser());
34+
$controller = new Index;
35+
36+
$result = self::callProtectedMethod($controller, 'getRelevantPermissionForType', [$type]);
37+
$this->assertEquals($expectedPermission, $result);
38+
}
39+
40+
public static function permissionMappingProvider(): array
41+
{
42+
return [
43+
'page maps to cms.manage_pages' => ['page', 'cms.manage_pages'],
44+
'partial maps to cms.manage_partials' => ['partial', 'cms.manage_partials'],
45+
'layout maps to cms.manage_layouts' => ['layout', 'cms.manage_layouts'],
46+
'content maps to cms.manage_content' => ['content', 'cms.manage_content'],
47+
'asset maps to cms.manage_assets' => ['asset', 'cms.manage_assets'],
48+
];
49+
}
50+
51+
//
52+
// validateRequestType — allowed access
53+
//
54+
55+
/**
56+
* @dataProvider allowedAccessProvider
57+
*/
58+
public function testValidateRequestTypeAllowed(string $type, array $permissions): void
59+
{
60+
$user = new UserFixture;
61+
foreach ($permissions as $permission) {
62+
$user->withPermission($permission, true);
63+
}
64+
$this->actingAs($user);
65+
$controller = new Index;
66+
67+
// Should not throw
68+
self::callProtectedMethod($controller, 'validateRequestType', [$type]);
69+
$this->assertTrue(true);
70+
}
71+
72+
public static function allowedAccessProvider(): array
73+
{
74+
return [
75+
'pages user can access page' => ['page', ['cms.manage_pages']],
76+
'partials user can access partial' => ['partial', ['cms.manage_partials']],
77+
'layouts user can access layout' => ['layout', ['cms.manage_layouts']],
78+
'content user can access content' => ['content', ['cms.manage_content']],
79+
'assets user can access asset' => ['asset', ['cms.manage_assets']],
80+
'all permissions can access page' => ['page', [
81+
'cms.manage_pages', 'cms.manage_partials', 'cms.manage_layouts',
82+
'cms.manage_content', 'cms.manage_assets',
83+
]],
84+
'all permissions can access asset' => ['asset', [
85+
'cms.manage_pages', 'cms.manage_partials', 'cms.manage_layouts',
86+
'cms.manage_content', 'cms.manage_assets',
87+
]],
88+
];
89+
}
90+
91+
public function testSuperuserCanAccessAllTypes(): void
92+
{
93+
$this->actingAs((new UserFixture)->asSuperUser());
94+
$controller = new Index;
95+
96+
foreach (self::$allTypes as $type) {
97+
self::callProtectedMethod($controller, 'validateRequestType', [$type]);
98+
}
99+
$this->assertTrue(true);
100+
}
101+
102+
//
103+
// validateRequestType — denied access
104+
//
105+
106+
/**
107+
* @dataProvider deniedAccessProvider
108+
*/
109+
public function testValidateRequestTypeDenied(string $grantedPermission, string $deniedType): void
110+
{
111+
$user = (new UserFixture)->withPermission($grantedPermission, true);
112+
$this->actingAs($user);
113+
$controller = new Index;
114+
115+
$this->expectException(AuthorizationException::class);
116+
self::callProtectedMethod($controller, 'validateRequestType', [$deniedType]);
117+
}
118+
119+
public static function deniedAccessProvider(): array
120+
{
121+
$cases = [];
122+
foreach (self::$permissionMap as $grantedType => $grantedPermission) {
123+
foreach (self::$allTypes as $targetType) {
124+
if ($targetType === $grantedType) {
125+
continue;
126+
}
127+
$cases["$grantedType user denied $targetType"] = [$grantedPermission, $targetType];
128+
}
129+
}
130+
return $cases;
131+
}
132+
133+
//
134+
// Constructor widget registration
135+
//
136+
137+
public function testSuperuserGetsAllWidgets(): void
138+
{
139+
$this->actingAs((new UserFixture)->asSuperUser());
140+
$controller = new Index;
141+
142+
$this->assertNotNull($controller->widget->pageList ?? null, 'pageList should be registered');
143+
$this->assertNotNull($controller->widget->partialList ?? null, 'partialList should be registered');
144+
$this->assertNotNull($controller->widget->layoutList ?? null, 'layoutList should be registered');
145+
$this->assertNotNull($controller->widget->contentList ?? null, 'contentList should be registered');
146+
$this->assertNotNull($controller->widget->assetList ?? null, 'assetList should be registered');
147+
$this->assertNotNull($controller->widget->componentList ?? null, 'componentList should be registered');
148+
}
149+
150+
public function testPagesOnlyUserGetsOnlyPageList(): void
151+
{
152+
$user = (new UserFixture)->withPermission('cms.manage_pages', true);
153+
$this->actingAs($user);
154+
$controller = new Index;
155+
156+
$this->assertNotNull($controller->widget->pageList ?? null, 'pageList should be registered');
157+
$this->assertNull($controller->widget->partialList ?? null, 'partialList should not be registered');
158+
$this->assertNull($controller->widget->layoutList ?? null, 'layoutList should not be registered');
159+
$this->assertNull($controller->widget->contentList ?? null, 'contentList should not be registered');
160+
$this->assertNull($controller->widget->assetList ?? null, 'assetList should not be registered');
161+
// Pages user should get componentList since components are usable in pages
162+
$this->assertNotNull($controller->widget->componentList ?? null, 'componentList should be registered for pages user');
163+
}
164+
165+
public function testAssetsOnlyUserGetsOnlyAssetList(): void
166+
{
167+
$user = (new UserFixture)->withPermission('cms.manage_assets', true);
168+
$this->actingAs($user);
169+
$controller = new Index;
170+
171+
$this->assertNull($controller->widget->pageList ?? null, 'pageList should not be registered');
172+
$this->assertNull($controller->widget->partialList ?? null, 'partialList should not be registered');
173+
$this->assertNull($controller->widget->layoutList ?? null, 'layoutList should not be registered');
174+
$this->assertNull($controller->widget->contentList ?? null, 'contentList should not be registered');
175+
$this->assertNotNull($controller->widget->assetList ?? null, 'assetList should be registered');
176+
$this->assertNull($controller->widget->componentList ?? null, 'componentList should not be registered for assets-only user');
177+
}
178+
179+
public function testPagesAndLayoutsUserGetsComponentList(): void
180+
{
181+
$user = (new UserFixture)
182+
->withPermission('cms.manage_pages', true)
183+
->withPermission('cms.manage_layouts', true);
184+
$this->actingAs($user);
185+
$controller = new Index;
186+
187+
$this->assertNotNull($controller->widget->pageList ?? null, 'pageList should be registered');
188+
$this->assertNull($controller->widget->partialList ?? null, 'partialList should not be registered');
189+
$this->assertNotNull($controller->widget->layoutList ?? null, 'layoutList should be registered');
190+
$this->assertNull($controller->widget->contentList ?? null, 'contentList should not be registered');
191+
$this->assertNull($controller->widget->assetList ?? null, 'assetList should not be registered');
192+
$this->assertNotNull($controller->widget->componentList ?? null, 'componentList should be registered');
193+
}
194+
195+
public function testContentOnlyUserDoesNotGetComponentList(): void
196+
{
197+
$user = (new UserFixture)->withPermission('cms.manage_content', true);
198+
$this->actingAs($user);
199+
$controller = new Index;
200+
201+
$this->assertNotNull($controller->widget->contentList ?? null, 'contentList should be registered');
202+
$this->assertNull($controller->widget->componentList ?? null, 'componentList should not be registered for content-only user');
203+
}
204+
}

0 commit comments

Comments
 (0)