Skip to content

Commit bfc37af

Browse files
nickvergessennickvergessen
authored
Merge pull request #36928 from nextcloud/techdebt/noid/bruteforce-protection-attribute
feat(middleware): Migrate BruteForceProtection annotation to PHP Attribute and allow multiple
2 parents 4d4a223 + 2b49861 commit bfc37af

File tree

7 files changed

+307
-81
lines changed

7 files changed

+307
-81
lines changed

‎lib/composer/composer/autoload_classmap.php

+1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
'OCP\\AppFramework\\Db\\QBMapper' => $baseDir . '/lib/public/AppFramework/Db/QBMapper.php',
3636
'OCP\\AppFramework\\Db\\TTransactional' => $baseDir . '/lib/public/AppFramework/Db/TTransactional.php',
3737
'OCP\\AppFramework\\Http' => $baseDir . '/lib/public/AppFramework/Http.php',
38+
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => $baseDir . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
3839
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
3940
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => $baseDir . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
4041
'OCP\\AppFramework\\Http\\DataDisplayResponse' => $baseDir . '/lib/public/AppFramework/Http/DataDisplayResponse.php',

‎lib/composer/composer/autoload_static.php

+1
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
6868
'OCP\\AppFramework\\Db\\QBMapper' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/QBMapper.php',
6969
'OCP\\AppFramework\\Db\\TTransactional' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/TTransactional.php',
7070
'OCP\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http.php',
71+
'OCP\\AppFramework\\Http\\Attribute\\BruteForceProtection' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/BruteForceProtection.php',
7172
'OCP\\AppFramework\\Http\\Attribute\\UseSession' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UseSession.php',
7273
'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php',
7374
'OCP\\AppFramework\\Http\\DataDisplayResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/DataDisplayResponse.php',

‎lib/private/AppFramework/DependencyInjection/DIContainer.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai
292292
new OC\AppFramework\Middleware\Security\BruteForceMiddleware(
293293
$c->get(IControllerMethodReflector::class),
294294
$c->get(OC\Security\Bruteforce\Throttler::class),
295-
$c->get(IRequest::class)
295+
$c->get(IRequest::class),
296+
$c->get(LoggerInterface::class)
296297
)
297298
);
298299
$dispatcher->registerMiddleware(

‎lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php

+52-15
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
declare(strict_types=1);
44

55
/**
6+
* @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
67
* @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
78
*
89
* @author Christoph Wurst <christoph@winzerhof-wurst.at>
@@ -31,13 +32,16 @@
3132
use OC\Security\Bruteforce\Throttler;
3233
use OCP\AppFramework\Controller;
3334
use OCP\AppFramework\Http;
35+
use OCP\AppFramework\Http\Attribute\BruteForceProtection;
3436
use OCP\AppFramework\Http\Response;
3537
use OCP\AppFramework\Http\TooManyRequestsResponse;
3638
use OCP\AppFramework\Middleware;
3739
use OCP\AppFramework\OCS\OCSException;
3840
use OCP\AppFramework\OCSController;
3941
use OCP\IRequest;
4042
use OCP\Security\Bruteforce\MaxDelayReached;
43+
use Psr\Log\LoggerInterface;
44+
use ReflectionMethod;
4145

4246
/**
4347
* Class BruteForceMiddleware performs the bruteforce protection for controllers
@@ -47,16 +51,12 @@
4751
* @package OC\AppFramework\Middleware\Security
4852
*/
4953
class BruteForceMiddleware extends Middleware {
50-
private ControllerMethodReflector $reflector;
51-
private Throttler $throttler;
52-
private IRequest $request;
53-
54-
public function __construct(ControllerMethodReflector $controllerMethodReflector,
55-
Throttler $throttler,
56-
IRequest $request) {
57-
$this->reflector = $controllerMethodReflector;
58-
$this->throttler = $throttler;
59-
$this->request = $request;
54+
public function __construct(
55+
protected ControllerMethodReflector $reflector,
56+
protected Throttler $throttler,
57+
protected IRequest $request,
58+
protected LoggerInterface $logger,
59+
) {
6060
}
6161

6262
/**
@@ -68,18 +68,55 @@ public function beforeController($controller, $methodName) {
6868
if ($this->reflector->hasAnnotation('BruteForceProtection')) {
6969
$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
7070
$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), $action);
71+
} else {
72+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
73+
$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
74+
75+
if (!empty($attributes)) {
76+
$remoteAddress = $this->request->getRemoteAddress();
77+
78+
foreach ($attributes as $attribute) {
79+
/** @var BruteForceProtection $protection */
80+
$protection = $attribute->newInstance();
81+
$action = $protection->getAction();
82+
$this->throttler->sleepDelayOrThrowOnMax($remoteAddress, $action);
83+
}
84+
}
7185
}
7286
}
7387

7488
/**
7589
* {@inheritDoc}
7690
*/
7791
public function afterController($controller, $methodName, Response $response) {
78-
if ($this->reflector->hasAnnotation('BruteForceProtection') && $response->isThrottled()) {
79-
$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
80-
$ip = $this->request->getRemoteAddress();
81-
$this->throttler->sleepDelay($ip, $action);
82-
$this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata());
92+
if ($response->isThrottled()) {
93+
if ($this->reflector->hasAnnotation('BruteForceProtection')) {
94+
$action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action');
95+
$ip = $this->request->getRemoteAddress();
96+
$this->throttler->sleepDelay($ip, $action);
97+
$this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata());
98+
} else {
99+
$reflectionMethod = new ReflectionMethod($controller, $methodName);
100+
$attributes = $reflectionMethod->getAttributes(BruteForceProtection::class);
101+
102+
if (!empty($attributes)) {
103+
$ip = $this->request->getRemoteAddress();
104+
$metaData = $response->getThrottleMetadata();
105+
106+
foreach ($attributes as $attribute) {
107+
/** @var BruteForceProtection $protection */
108+
$protection = $attribute->newInstance();
109+
$action = $protection->getAction();
110+
111+
if (!isset($metaData['action']) || $metaData['action'] === $action) {
112+
$this->throttler->sleepDelay($ip, $action);
113+
$this->throttler->registerAttempt($action, $ip, $metaData);
114+
}
115+
}
116+
} else {
117+
$this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.');
118+
}
119+
}
83120
}
84121

85122
return parent::afterController($controller, $methodName, $response);

‎lib/public/AppFramework/Http/Attribute/BruteForceProtection.php

+52
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
7+
*
8+
* @author Joas Schilling <coding@schilljs.com>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*/
25+
26+
namespace OCP\AppFramework\Http\Attribute;
27+
28+
use Attribute;
29+
30+
/**
31+
* Attribute for controller methods that want to protect passwords, keys, tokens
32+
* or other data against brute force
33+
*
34+
* @since 27.0.0
35+
*/
36+
#[Attribute(Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]
37+
class BruteForceProtection {
38+
/**
39+
* @since 27.0.0
40+
*/
41+
public function __construct(
42+
protected string $action
43+
) {
44+
}
45+
46+
/**
47+
* @since 27.0.0
48+
*/
49+
public function getAction(): string {
50+
return $this->action;
51+
}
52+
}

‎lib/public/AppFramework/Http/Attribute/UseSession.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
declare(strict_types=1);
44

5-
/*
5+
/**
66
* @copyright 2023 Christoph Wurst <christoph@winzerhof-wurst.at>
77
*
88
* @author 2023 Christoph Wurst <christoph@winzerhof-wurst.at>

0 commit comments

Comments
 (0)