Skip to content

Commit 74c73de

Browse files
CarlSchwanCarlSchwan
committed
perf: Remove MiddlewareUtils overhead
Avoid doing some reflection Signed-off-by: Carl Schwan <carl@carlschwan.eu>
1 parent 083ffeb commit 74c73de

12 files changed

Lines changed: 81 additions & 120 deletions

File tree

‎apps/settings/tests/Controller/AuthSettingsControllerTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ public function testCreateDisabledBySystemConfig(): void {
152152
$expected = new JSONResponse();
153153
$expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE);
154154

155-
$this->assertEquals($expected, $this->controller->create($name)->getData());
155+
$this->assertEquals($expected, $this->controller->create($name));
156156
}
157157

158158
public function testCreateSessionNotAvailable(): void {
@@ -165,7 +165,7 @@ public function testCreateSessionNotAvailable(): void {
165165
$expected = new JSONResponse();
166166
$expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE);
167167

168-
$this->assertEquals($expected, $this->controller->create($name)->getData());
168+
$this->assertEquals($expected, $this->controller->create($name));
169169
}
170170

171171
public function testCreateInvalidToken(): void {
@@ -185,7 +185,7 @@ public function testCreateInvalidToken(): void {
185185
$expected = new JSONResponse();
186186
$expected->setStatus(Http::STATUS_SERVICE_UNAVAILABLE);
187187

188-
$this->assertEquals($expected, $this->controller->create($name)->getData());
188+
$this->assertEquals($expected, $this->controller->create($name));
189189
}
190190

191191
public function testDestroy(): void {

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@
9797
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
9898
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
9999
'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php',
100+
'OCP\\AppFramework\\Http\\Attribute\\NoSubAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php',
100101
'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php',
101102
'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php',
102103
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
@@ -1177,7 +1178,6 @@
11771178
'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php',
11781179
'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php',
11791180
'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
1180-
'OC\\AppFramework\\Middleware\\MiddlewareUtils' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php',
11811181
'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php',
11821182
'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
11831183
'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
138138
'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php',
139139
'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php',
140140
'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php',
141+
'OCP\\AppFramework\\Http\\Attribute\\NoSubAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoSubAdminRequired.php',
141142
'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php',
142143
'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php',
143144
'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php',
@@ -1218,7 +1219,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
12181219
'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php',
12191220
'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php',
12201221
'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php',
1221-
'OC\\AppFramework\\Middleware\\MiddlewareUtils' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php',
12221222
'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php',
12231223
'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php',
12241224
'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php',

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
use OC\AppFramework\Middleware\AdditionalScriptsMiddleware;
1818
use OC\AppFramework\Middleware\CompressionMiddleware;
1919
use OC\AppFramework\Middleware\MiddlewareDispatcher;
20-
use OC\AppFramework\Middleware\MiddlewareUtils;
2120
use OC\AppFramework\Middleware\NotModifiedMiddleware;
2221
use OC\AppFramework\Middleware\OCSMiddleware;
2322
use OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware;
@@ -205,7 +204,7 @@ public function __construct(
205204

206205
$securityMiddleware = new SecurityMiddleware(
207206
$c->get(IRequest::class),
208-
$c->get(MiddlewareUtils::class),
207+
$c->get(ControllerMethodReflector::class),
209208
$c->get(INavigationManager::class),
210209
$c->get(IURLGenerator::class),
211210
$c->get(LoggerInterface::class),

‎lib/private/AppFramework/Middleware/MiddlewareUtils.php‎

Lines changed: 0 additions & 60 deletions
This file was deleted.

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99
namespace OC\AppFramework\Middleware\Security;
1010

11-
use OC\AppFramework\Middleware\MiddlewareUtils;
1211
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
12+
use OC\AppFramework\Utility\ControllerMethodReflector;
1313
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
1414
use OC\User\Session;
1515
use OCP\AppFramework\Controller;
@@ -35,20 +35,18 @@ class CORSMiddleware extends Middleware {
3535

3636
public function __construct(
3737
private readonly IRequest $request,
38-
private readonly MiddlewareUtils $middlewareUtils,
38+
private readonly ControllerMethodReflector $reflector,
3939
private readonly Session $session,
4040
private readonly IThrottler $throttler,
4141
) {
4242
}
4343

4444
#[Override]
4545
public function beforeController(Controller $controller, string $methodName): void {
46-
$reflectionMethod = new ReflectionMethod($controller, $methodName);
47-
4846
// ensure that @CORS annotated API routes are not used in conjunction
4947
// with session authentication since this enables CSRF attack vectors
50-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)
51-
&& (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) {
48+
if ($this->reflector->hasAnnotationOrAttribute('CORS', CORS::class)
49+
&& (!$this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class) || $this->session->isLoggedIn())) {
5250
$user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null;
5351
$pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null;
5452

@@ -77,7 +75,7 @@ public function afterController(Controller $controller, string $methodName, Resp
7775

7876
if (isset($this->request->server['HTTP_ORIGIN'])) {
7977
$reflectionMethod = new ReflectionMethod($controller, $methodName);
80-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) {
78+
if ($this->reflector->hasAnnotationOrAttribute('CORS', CORS::class)) {
8179
// allow credentials headers must not be true or CSRF is possible
8280
// otherwise
8381
foreach ($response->getHeaders() as $header => $value) {

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,18 @@
1010
namespace OC\AppFramework\Middleware\Security;
1111

1212
use OC\AppFramework\Http\Request;
13-
use OC\AppFramework\Middleware\MiddlewareUtils;
1413
use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException;
14+
use OC\AppFramework\Utility\ControllerMethodReflector;
1515
use OCP\AppFramework\Controller;
1616
use OCP\AppFramework\Http;
1717
use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired;
1818
use OCP\AppFramework\Http\Response;
1919
use OCP\AppFramework\Middleware;
20-
use ReflectionMethod;
2120

2221
class SameSiteCookieMiddleware extends Middleware {
2322
public function __construct(
2423
private readonly Request $request,
25-
private readonly MiddlewareUtils $middlewareUtils,
24+
private readonly ControllerMethodReflector $reflector,
2625
) {
2726
}
2827

@@ -36,8 +35,7 @@ public function beforeController(Controller $controller, string $methodName): vo
3635
return;
3736
}
3837

39-
$reflectionMethod = new ReflectionMethod($controller, $methodName);
40-
$noSSC = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class);
38+
$noSSC = $this->reflector->hasAnnotationOrAttribute('NoSameSiteCookieRequired', NoSameSiteCookieRequired::class);
4139
if ($noSSC) {
4240
return;
4341
}

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

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
namespace OC\AppFramework\Middleware\Security;
1111

12-
use OC\AppFramework\Middleware\MiddlewareUtils;
1312
use OC\AppFramework\Middleware\Security\Exceptions\AdminIpNotAllowedException;
1413
use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException;
1514
use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException;
@@ -19,6 +18,7 @@
1918
use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException;
2019
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
2120
use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
21+
use OC\AppFramework\Utility\ControllerMethodReflector;
2222
use OC\Security\CSRF\CsrfTokenManager;
2323
use OC\Settings\AuthorizedGroupMapper;
2424
use OC\User\Session;
@@ -64,7 +64,7 @@ class SecurityMiddleware extends Middleware {
6464

6565
public function __construct(
6666
private readonly IRequest $request,
67-
private readonly MiddlewareUtils $middlewareUtils,
67+
private readonly ControllerMethodReflector $reflector,
6868
private readonly INavigationManager $navigationManager,
6969
private readonly IURLGenerator $urlGenerator,
7070
private readonly LoggerInterface $logger,
@@ -118,18 +118,16 @@ public function beforeController(Controller $controller, string $methodName): vo
118118
$this->navigationManager->setActiveEntry('spreed');
119119
}
120120

121-
$reflectionMethod = new ReflectionMethod($controller, $methodName);
122-
123121
// security checks
124-
$isPublicPage = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class);
122+
$isPublicPage = $this->reflector->hasAnnotationOrAttribute('PublicPage', PublicPage::class);
125123

126-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class)) {
124+
if ($this->reflector->hasAnnotationOrAttribute('ExAppRequired', ExAppRequired::class)) {
127125
if (!$this->userSession instanceof Session || $this->userSession->getSession()->get('app_api') !== true) {
128126
throw new ExAppRequiredException();
129127
}
130128
} elseif (!$isPublicPage) {
131129
$authorized = false;
132-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class)) {
130+
if ($this->reflector->hasAnnotationOrAttribute(null, AppApiAdminAccessWithoutUser::class)) {
133131
// this attribute allows ExApp to access admin endpoints only if "userId" is "null"
134132
if ($this->userSession instanceof Session && $this->userSession->getSession()->get('app_api') === true && $this->userSession->getUser() === null) {
135133
$authorized = true;
@@ -140,15 +138,15 @@ public function beforeController(Controller $controller, string $methodName): vo
140138
throw new NotLoggedInException();
141139
}
142140

143-
if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
141+
if (!$authorized && $this->reflector->hasAnnotationOrAttribute('AuthorizedAdminSetting', AuthorizedAdminSetting::class)) {
144142
$authorized = $this->isAdminUser();
145143

146-
if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) {
144+
if (!$authorized && $this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)) {
147145
$authorized = $this->isSubAdmin();
148146
}
149147

150148
if (!$authorized) {
151-
$settingClasses = $this->middlewareUtils->getAuthorizedAdminSettingClasses($reflectionMethod);
149+
$settingClasses = $this->getAuthorizedAdminSettingClasses();
152150
$authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser());
153151
foreach ($settingClasses as $settingClass) {
154152
$authorized = in_array($settingClass, $authorizedClasses, true);
@@ -165,40 +163,40 @@ public function beforeController(Controller $controller, string $methodName): vo
165163
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions'));
166164
}
167165
}
168-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
166+
if ($this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)
169167
&& !$this->isSubAdmin()
170168
&& !$this->isAdminUser()
171169
&& !$authorized) {
172170
throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin'));
173171
}
174-
if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
175-
&& !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
172+
if (!$this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)
173+
&& !$this->reflector->hasAnnotationOrAttribute('NoAdminRequired', NoAdminRequired::class)
176174
&& !$this->isAdminUser()
177175
&& !$authorized) {
178176
throw new NotAdminException($this->l10n->t('Logged in account must be an admin'));
179177
}
180-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
178+
if ($this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)
181179
&& !$this->remoteAddress->allowsAdminActions()) {
182180
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions'));
183181
}
184-
if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)
185-
&& !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class)
182+
if (!$this->reflector->hasAnnotationOrAttribute('SubAdminRequired', SubAdminRequired::class)
183+
&& !$this->reflector->hasAnnotationOrAttribute('NoAdminRequired', NoAdminRequired::class)
186184
&& !$this->remoteAddress->allowsAdminActions()) {
187185
throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions'));
188186
}
189187

190188
}
191189

192190
// Check for strict cookie requirement
193-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class)
194-
|| !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
191+
if ($this->reflector->hasAnnotationOrAttribute('StrictCookieRequired', StrictCookiesRequired::class)
192+
|| !$this->reflector->hasAnnotationOrAttribute('NoCSRFRequired', NoCSRFRequired::class)) {
195193
if (!$this->request->passesStrictCookieCheck()) {
196194
throw new StrictCookieMissingException();
197195
}
198196
}
199197
// CSRF check - also registers the CSRF token since the session may be closed later
200198
Server::get(CsrfTokenManager::class)->generateSessionToken();
201-
if ($this->isInvalidCSRFRequired($reflectionMethod)) {
199+
if ($this->isInvalidCSRFRequired()) {
202200
/*
203201
* Only allow the CSRF check to fail on OCS Requests. This kind of
204202
* hacks around that we have no full token auth in place yet and we
@@ -229,8 +227,8 @@ public function beforeController(Controller $controller, string $methodName): vo
229227
}
230228
}
231229

232-
private function isInvalidCSRFRequired(ReflectionMethod $reflectionMethod): bool {
233-
if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) {
230+
private function isInvalidCSRFRequired(): bool {
231+
if ($this->reflector->hasAnnotationOrAttribute('NoCSRFRequired', NoCSRFRequired::class)) {
234232
return false;
235233
}
236234

@@ -296,4 +294,24 @@ public function afterException(Controller $controller, string $methodName, \Exce
296294

297295
throw $exception;
298296
}
297+
298+
/**
299+
* @param ReflectionMethod $reflectionMethod
300+
* @return string[]
301+
*/
302+
public function getAuthorizedAdminSettingClasses(): array {
303+
$classes = [];
304+
if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
305+
$classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings'));
306+
}
307+
308+
$attribute = $this->reflector->getAttribute(AuthorizedAdminSetting::class);
309+
if ($attribute !== null) {
310+
/** @var AuthorizedAdminSetting $setting */
311+
$setting = $attribute->newInstance();
312+
$classes[] = $setting->getSettings();
313+
}
314+
315+
return $classes;
316+
}
299317
}

‎lib/private/AppFramework/Utility/ControllerMethodReflector.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,19 @@ public function hasAnnotationOrAttribute(?string $annotationName, string $attrib
147147
return false;
148148
}
149149

150+
/**
151+
* @template T
152+
* @param class-string<T> $attributeClass
153+
* @return ?T
154+
*/
155+
public function getAttribute(string $attributeClass): ?object {
156+
$attributes = $this->reflectionMethod->getAttributes($attributeClass);
157+
if (!empty($attributes)) {
158+
return $attributes[0];
159+
}
160+
return null;
161+
}
162+
150163
/**
151164
* Check if a method contains an annotation
152165
* @param string $name the name of the annotation

0 commit comments

Comments
 (0)