Skip to content

Commit 3515323

Browse files
williamdeswilliamdes
committed
Move the security check code into a middleware
1 parent 1fa421c commit 3515323

8 files changed

Lines changed: 173 additions & 169 deletions

File tree

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,6 @@ parameters:
66
count: 2
77
path: src/Console/Installer.php
88

9-
-
10-
message: '#^Property App\\Controller\\AppController\:\:\$readonly_whitelist type has no value type specified in iterable type array\.$#'
11-
identifier: missingType.iterableValue
12-
count: 1
13-
path: src/Controller/AppController.php
14-
15-
-
16-
message: '#^Property App\\Controller\\AppController\:\:\$whitelist type has no value type specified in iterable type array\.$#'
17-
identifier: missingType.iterableValue
18-
count: 1
19-
path: src/Controller/AppController.php
20-
219
-
2210
message: '#^Method App\\Controller\\Component\\GithubApiComponent\:\:apiRequest\(\) has parameter \$data with no value type specified in iterable type array\.$#'
2311
identifier: missingType.iterableValue

‎src/Application.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
namespace App;
2020

21+
use App\Middleware\AuthenticationMiddleware;
2122
use App\Middleware\CsrfProtectionMiddleware;
2223
use App\Middleware\HostHeaderMiddleware;
2324
use Cake\Core\Configure;
@@ -90,7 +91,10 @@ public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
9091

9192
// Cross Site Request Forgery (CSRF) Protection Middleware
9293
// https://book.cakephp.org/5/en/security/csrf.html#cross-site-request-forgery-csrf-middleware
93-
->add(new CsrfProtectionMiddleware(['httponly' => true]));
94+
->add(new CsrfProtectionMiddleware(['httponly' => true]))
95+
96+
// Check for access controls
97+
->add(new AuthenticationMiddleware());
9498

9599
return $middlewareQueue;
96100
}

‎src/Controller/AppController.php‎

Lines changed: 14 additions & 151 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,9 @@
2525
use App\Model\Table\NotificationsTable;
2626
use Cake\Controller\Controller;
2727
use Cake\Event\EventInterface;
28-
use Cake\Http\Response;
2928
use Cake\ORM\TableRegistry;
3029
use Cake\Routing\Router;
3130

32-
use function in_array;
33-
3431
/**
3532
* Application Controller.
3633
*
@@ -44,28 +41,8 @@ class AppController extends Controller
4441
protected NotificationsTable $Notifications;
4542
protected DevelopersTable $Developers;
4643

47-
/** @var array */
48-
public $whitelist = [
49-
'Developers',
50-
'Pages',
51-
'Incidents' => ['create'],
52-
'Events',
53-
];
54-
55-
/** @var array */
56-
public $readonly_whitelist = [
57-
'Developers',
58-
'Pages',
59-
'Reports' => [
60-
'index',
61-
'view',
62-
'data_tables',
63-
],
64-
'Incidents' => ['view'],
65-
];
66-
6744
/** @var string[] */
68-
public $css_files = [
45+
public array $css_files = [
6946
'jquery.dataTables',
7047
'jquery.dataTables_themeroller',
7148
'bootstrap.min',
@@ -76,7 +53,7 @@ class AppController extends Controller
7653
];
7754

7855
/** @var string[] */
79-
public $js_files = [
56+
public array $js_files = [
8057
'jquery',
8158
'jquery.dataTables.min',
8259
'bootstrap',
@@ -107,8 +84,6 @@ class AppController extends Controller
10784
* Initialization hook method.
10885
*
10986
* Use this method to add common initialization code like loading components.
110-
*
111-
* @return void Nothing
11287
*/
11388
public function initialize(): void
11489
{
@@ -117,144 +92,32 @@ public function initialize(): void
11792
$this->Notifications = $this->fetchTable('Notifications');
11893
$this->Developers = $this->fetchTable('Developers');
11994

120-
/* $this->loadComponent(
121-
'Auth', [
122-
'loginAction' => [
123-
'controller' => 'Developer',
124-
'action' => 'login'
125-
],
126-
'authError' => 'Did you really think you are allowed to see that?',
127-
'authenticate' => [
128-
'Form' => [
129-
'fields' => ['username' => 'email']
130-
]
131-
]
132-
]
133-
);
134-
*/
95+
$this->set('js_files', $this->js_files);
96+
$this->set('css_files', $this->css_files);
97+
$this->set('baseURL', Router::url('/', true));
13598
}
13699

137-
/**
138-
* @return Response|void Returns a Response if a redirect is needed
139-
*/
140100
public function beforeFilter(EventInterface $event)
141101
{
142102
$controllerName = $this->request->getParam('controller');
143103
$this->set('current_controller', $controllerName);
144104

145-
$devId = $this->request->getSession()->read('Developer.id');
146-
if ($devId !== null) {
147-
$response = $this->checkReadonlyAccess();
148-
if ($response !== null) {
149-
// This is a security check
150-
// The response can be printed if you remove this line
151-
return $response;
152-
}
153-
}
105+
// Attributes where set in the Authentication Middleware
106+
$currentDeveloper = $this->request->getAttribute('current_developer');
154107

155-
$current_developer = null;
156-
if ($devId !== null) {
157-
// Check if the user still exists in the database
158-
$current_developer = TableRegistry::getTableLocator()->get('Developers')->
159-
findById($devId)->all()->first();
160-
}
108+
$notificationCount = 0;
161109

162-
$notif_count = 0;
163-
if ($current_developer !== null) {
164-
$notif_count = TableRegistry::getTableLocator()->get('Notifications')->find(
110+
// The user is logged in
111+
if ($currentDeveloper !== null) {
112+
$this->set('read_only', $this->request->getSession()->read('read_only'));
113+
$notificationCount = TableRegistry::getTableLocator()->get('Notifications')->find(
165114
'all',
166-
conditions: ['developer_id' => $current_developer['id']]
115+
conditions: ['developer_id' => $currentDeveloper['id']]
167116
)->count();
168-
169-
$this->set('current_developer', $current_developer);
170-
$this->set('developer_signed_in', true);
171-
172-
$read_only = false;
173-
if ($this->request->getSession()->read('read_only')) {
174-
$read_only = true;
175-
}
176-
177-
$this->set('read_only', $read_only);
178117
} else {
179-
$this->set('developer_signed_in', false);
180118
$this->set('read_only', true);
181-
$response = $this->checkAccess();
182-
if ($response !== null) {
183-
// This is a security check
184-
// The response can be printed if you remove this line
185-
return $response;
186-
}
187119
}
188120

189-
$this->set('notif_count', $notif_count);
190-
$this->set('js_files', $this->js_files);
191-
$this->set('css_files', $this->css_files);
192-
$this->set('baseURL', Router::url('/', true));
193-
}
194-
195-
protected function checkAccess(): ?Response
196-
{
197-
$controllerName = $this->request->getParam('controller');
198-
$action = $this->request->getParam('action');
199-
200-
if (in_array($controllerName, $this->whitelist)) {
201-
return null;
202-
}
203-
204-
if (
205-
isset($this->whitelist[$controllerName])
206-
&& in_array($action, $this->whitelist[$controllerName])
207-
) {
208-
return null;
209-
}
210-
211-
$flash_class = 'alert';
212-
$this->Flash->set(
213-
'You need to be signed in to do this',
214-
['params' => ['class' => $flash_class]]
215-
);
216-
217-
// save the return url
218-
$ret_url = Router::url($this->request->getRequestTarget(), true);
219-
$this->request->getSession()->write('last_page', $ret_url);
220-
221-
return $this->redirect('/');
222-
}
223-
224-
protected function checkReadonlyAccess(): ?Response
225-
{
226-
$controllerName = $this->request->getParam('controller');
227-
$action = $this->request->getParam('action');
228-
$read_only = $this->request->getSession()->read('read_only');
229-
230-
// If developer has commit access on phpmyadmin/phpmyadmin
231-
if (! $read_only) {
232-
return null;
233-
}
234-
235-
if (in_array($controllerName, $this->readonly_whitelist)) {
236-
return null;
237-
}
238-
239-
if (
240-
isset($this->readonly_whitelist[$controllerName])
241-
&& in_array($action, $this->readonly_whitelist[$controllerName])
242-
) {
243-
return null;
244-
}
245-
246-
$this->request->getSession()->destroy();
247-
$this->request->getSession()->write('last_page', '');
248-
249-
$flash_class = 'alert';
250-
$this->Flash->set(
251-
'You need to have commit access on phpmyadmin/phpmyadmin '
252-
. 'repository on Github.com to do this',
253-
[
254-
'params' => ['class' => $flash_class],
255-
]
256-
);
257-
258-
return $this->redirect('/');
121+
$this->set('notif_count', $notificationCount);
259122
}
260123
}

‎src/Controller/NotificationsController.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,9 @@ public function index(): void
7676

7777
public function data_tables(): Response
7878
{
79+
$devId = $this->request->getSession()->read('Developer.id');
7980
$current_developer = TableRegistry::getTableLocator()->get('Developers')->
80-
findById($this->request->getSession()->read('Developer.id'))->all()->first();
81+
findById($devId)->all()->first();
8182

8283
$aColumns = [
8384
'report_id' => 'Reports.id',

0 commit comments

Comments
 (0)